fix: reject comment replays under other parents and nonexistent parents

- Comment::store() reads the document's comments and rejects a comment
  whose ID already exists under any parent (69), whose parent is neither
  the document nor an existing comment (71), or whose ID equals the
  document ID, i.e. reuses the document's cipher text (72)
- Database::readComments() catches PDOException, logs it and returns no
  comments, consistent with existsComment()
- test fixture comment gets its own cipher text and matching comment ID,
  instead of reusing the document's cipher text (and thus its ID)
- tests for cross-parent replay, nonexistent parent, reply to an existing
  comment and comment equal to the document

CHANGELOG.md

@@ -1,8 +1,10 @@
 # PrivateBin version history
 
 ## 2.1.1 (not yet released)
-* FIXED: Creating a document failed with a JavaScript error when the `discussion`, `password`, e-mail or QR code option is disabled, as the top navigation accessed the omitted elements
 * ADDED: Translation for Korean (#2071)
+* CHANGED: Database backend now logs errors when reading comments, like it already did for comment existence checks
+* FIXED: Comments must now be unique in cipher text within a document, not reuse existing cipher texts
+* FIXED: Creating a document failed with a JavaScript error when the `discussion`, `password`, e-mail or QR code option is disabled, as the top navigation accessed the omitted elements
 * FIXED: Slovenian plural forms were stored in the wrong order, i.e. "1 uri" instead of "1 ura" for one hour (#1951)
 * FIXED: Lithuanian plural formula selected the "few" form for numbers ending in 0 above 10, i.e. "20 sekundės" instead of "20 sekundžių" (#1956)
 

lib/Data/Database.php

@@ -301,10 +301,15 @@ class Database extends AbstractData
      */
     public function readComments($pasteid)
     {
-        $rows = $this->_select(
-            'SELECT * FROM "' . $this->_sanitizeIdentifier('comment') .
-            '" WHERE "pasteid" = ?', [$pasteid]
-        );
+        try {
+            $rows = $this->_select(
+                'SELECT * FROM "' . $this->_sanitizeIdentifier('comment') .
+                '" WHERE "pasteid" = ?', [$pasteid]
+            );
+        } catch (PDOException $e) {
+            error_log('Error while reading comments from the database: ' . $e->getMessage());
+            return [];
+        }
 
         // create comment list
         $comments = [];

lib/Model/Comment.php

@@ -56,6 +56,26 @@ class Comment extends AbstractModel
             throw new TranslatedException(self::COLLISION_ERROR, 69);
         }
 
+        // Comment IDs must be unique within the paste, regardless of the
+        // parent, and the parent must be either the paste or an existing comment.
+        $commentid    = $this->getId();
+        $parentid     = $this->getParentId();
+        $parentExists = $parentid === $pasteid;
+        if ($commentid === $pasteid) {
+            throw new TranslatedException(self::COLLISION_ERROR, 72);
+        }
+        foreach ($this->_store->readComments($pasteid) as $comment) {
+            if ($comment['id'] === $commentid) {
+                throw new TranslatedException(self::COLLISION_ERROR, 69);
+            }
+            if ($comment['id'] === $parentid) {
+                $parentExists = true;
+            }
+        }
+        if (!$parentExists) {
+            throw new TranslatedException(self::INVALID_DATA_ERROR, 71);
+        }
+
         $this->_data['meta']['created'] = time();
 
         // store comment

tst/Bootstrap.php

@@ -75,7 +75,14 @@ class Helper
      *
      * @var string
      */
-    private static $commentid = '5a52eebf11c4c94b';
+    private static $commentid = '33a5671d11b29d01';
+
+    /**
+     * example comment cipher text, its hash is the comment ID
+     *
+     * @var string
+     */
+    private static $commentCt = 'XNE9E3AGszrjQfV5bxxSuaflS05MViI3rYtG1Nph0Qc=';
 
     /**
      * example comment meta data
@@ -162,6 +169,7 @@ class Helper
     {
         $example = self::$pasteV2;
         $example['adata'] = $example['adata'][0];
+        $example['ct'] = self::$commentCt;
         $example['pasteid'] = $example['parentid'] = self::getPasteId();
         unset($example['meta']['expire']);
         $example['meta'] = array_merge($example['meta'], self::$commentV2);

tst/ControllerTest.php

@@ -663,8 +663,8 @@ class ControllerTest extends TestCase
         $paste = Helper::getPaste();
         $this->_data->create(Helper::getPasteId(), $paste);
         $comment = Helper::getComment();
-        $this->_data->createComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getPasteId(), $comment);
-        $this->assertTrue($this->_data->existsComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getPasteId()), 'comment exists before posting data');
+        $this->_data->createComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getCommentId(), $comment);
+        $this->assertTrue($this->_data->existsComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getCommentId()), 'comment exists before posting data');
         $comment = Helper::getCommentJson();
         $file = Helper::createTempFile();
         file_put_contents($file, $comment);
@@ -678,7 +678,7 @@ class ControllerTest extends TestCase
         ob_end_clean();
         $response = json_decode($content, true);
         $this->assertEquals(1, $response['status'], 'outputs error status');
-        $this->assertTrue($this->_data->existsComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getPasteId()), 'paste exists after posting data');
+        $this->assertTrue($this->_data->existsComment(Helper::getPasteId(), Helper::getPasteId(), Helper::getCommentId()), 'paste exists after posting data');
     }
 
     /**

tst/ModelTest.php

@@ -91,7 +91,7 @@ class ModelTest extends TestCase
 
         $comments = $this->_model->getPaste(Helper::getPasteId())->get()['comments'];
         $this->assertTrue(count($comments) === 1, 'comment exists after storing it');
-        $commentData['id'] = Helper::getPasteId();
+        $commentData['id'] = Helper::getCommentId();
         $commentData['meta']['created'] = current($comments)['meta']['created'];
         $commentData['meta']['icon'] = current($comments)['meta']['icon'];
         $this->assertEquals($commentData, current($comments));
@@ -242,6 +242,75 @@ class ModelTest extends TestCase
         $comment->store();
     }
 
+    public function testCommentDuplicateOtherParent()
+    {
+        $pasteData = Helper::getPastePost();
+        $commentData = Helper::getCommentPost();
+        $this->_model->getPaste(Helper::getPasteId())->delete();
+
+        $paste = $this->_model->getPaste();
+        $paste->setData($pasteData);
+        $paste->store();
+
+        $comment = $paste->getComment(Helper::getPasteId());
+        $comment->setData($commentData);
+        $comment->store();
+        $firstCommentId = $comment->getId();
+
+        // reply to the first comment, using a different ciphertext
+        $replyData = $commentData;
+        $replyData['ct'] = base64_encode('reply ciphertext');
+        $replyData['parentid'] = $firstCommentId;
+        $reply = $paste->getComment($firstCommentId);
+        $reply->setData($replyData);
+        $reply->store();
+        $this->assertTrue($reply->exists(), 'reply to existing comment can be stored');
+
+        // replay of the first comment under the reply as parent
+        $commentData['parentid'] = $reply->getId();
+        $comment = $paste->getComment($reply->getId());
+        $comment->setData($commentData);
+        $this->expectException(Exception::class);
+        $this->expectExceptionCode(69);
+        $comment->store();
+    }
+
+    public function testCommentNonexistentParent()
+    {
+        $pasteData = Helper::getPastePost();
+        $commentData = Helper::getCommentPost();
+        $commentData['parentid'] = '1111111111111111';
+        $this->_model->getPaste(Helper::getPasteId())->delete();
+
+        $paste = $this->_model->getPaste();
+        $paste->setData($pasteData);
+        $paste->store();
+
+        $comment = $paste->getComment('1111111111111111');
+        $comment->setData($commentData);
+        $this->expectException(Exception::class);
+        $this->expectExceptionCode(71);
+        $comment->store();
+    }
+
+    public function testCommentEqualsPaste()
+    {
+        $pasteData = Helper::getPastePost();
+        $commentData = Helper::getCommentPost();
+        $commentData['ct'] = $pasteData['ct'];
+        $this->_model->getPaste(Helper::getPasteId())->delete();
+
+        $paste = $this->_model->getPaste();
+        $paste->setData($pasteData);
+        $paste->store();
+
+        $comment = $paste->getComment(Helper::getPasteId());
+        $comment->setData($commentData);
+        $this->expectException(Exception::class);
+        $this->expectExceptionCode(72);
+        $comment->store();
+    }
+
     public function testImplicitDefaults()
     {
         $pasteData = Helper::getPastePost();
@@ -430,14 +499,14 @@ class ModelTest extends TestCase
         $commentData = Helper::getCommentPost();
         unset($commentData['meta']['icon']);
         $paste = $model->getPaste(Helper::getPasteId());
-        $comment = $paste->getComment(Helper::getPasteId(), Helper::getPasteId());
+        $comment = $paste->getComment(Helper::getPasteId(), Helper::getCommentId());
         $this->assertFalse($comment->exists(), 'comment does not yet exist');
 
         $comment = $paste->getComment(Helper::getPasteId());
         $comment->setData($commentData);
         $comment->store();
 
-        $comment = $paste->getComment(Helper::getPasteId(), Helper::getPasteId());
+        $comment = $paste->getComment(Helper::getPasteId(), Helper::getCommentId());
         $this->assertTrue($comment->exists(), 'comment exists after storing it');
 
         $comment = current($this->_model->getPaste(Helper::getPasteId())->get()['comments']);