Merge pull request #1030 from PrivateBin/improvements

Some minor improvements

CHANGELOG.md

@@ -1,5 +1,9 @@
 # PrivateBin version history
 
+  * **1.5.1 (not yet released)**
+    * FIXED: Revert Filesystem purge to limited and randomized lookup (#1030)
+    * FIXED: Catch JSON decode errors when invalid data gets sent to the API (#1030)
+    * FIXED: Support sorting v1 format in mixed version comments in Filesystem backend (#1030)
   * **1.5 (2022-12-11)**
     * ADDED: script for data storage backend migrations (#1012)
     * ADDED: Translations for Turkish, Slovak, Greek and Thai

CREDITS.md

@@ -29,9 +29,8 @@
 * rodehoed - option to exempt ips from the rate-limiter
 * Mark van Holsteijn - Google Cloud Storage backend
 * Austin Huang - Oracle database support
-* Felix J. Ogris - S3 Storage backend
+* Felix J. Ogris - S3 Storage backend, script for data backend migrations, dropped singleton behaviour of data backends
 * Mounir Idrassi & J. Mozdzen - secure YOURLS integration
-* Felix J. Ogris - script for data backend migrations, dropped singleton behaviour of data backends
 
 ## Translations
 * Hexalyse - French

lib/Data/Filesystem.php

@@ -228,7 +228,13 @@ class Filesystem extends AbstractData
                     $comment['parentid'] = $items[2];
 
                     // Store in array
-                    $key            = $this->getOpenSlot($comments, (int) $comment['meta']['created']);
+                    $key            = $this->getOpenSlot(
+                        $comments, (
+                            (int) array_key_exists('created', $comment['meta']) ?
+                            $comment['meta']['created'] : // v2 comments
+                            $comment['meta']['postdate'] // v1 comments
+                        )
+                    );
                     $comments[$key] = $comment;
                 }
             }
@@ -358,12 +364,12 @@ class Filesystem extends AbstractData
     {
         $pastes = array();
         $count  = 0;
+        $opened = 0;
+        $limit  = $batchsize * 10; // try at most 10 times $batchsize pastes before giving up
         $time   = time();
-        foreach ($this->_getPasteIterator() as $file) {
-            if ($file->isDir()) {
-                continue;
-            }
-            $pasteid = $file->getBasename('.php');
+        $files  = $this->getAllPastes();
+        shuffle($files);
+        foreach ($files as $pasteid) {
             if ($this->exists($pasteid)) {
                 $data = $this->read($pasteid);
                 if (
@@ -371,11 +377,13 @@ class Filesystem extends AbstractData
                     $data['meta']['expire_date'] < $time
                 ) {
                     $pastes[] = $pasteid;
-                    ++$count;
-                    if ($count >= $batchsize) {
+                    if (++$count >= $batchsize) {
                         break;
                     }
                 }
+                if (++$opened >= $limit) {
+                    break;
+                }
             }
         }
         return $pastes;
@@ -387,7 +395,7 @@ class Filesystem extends AbstractData
     public function getAllPastes()
     {
         $pastes = array();
-        foreach ($this->_getPasteIterator() as $file) {
+        foreach (new \GlobIterator($this->_path . self::PASTE_FILE_PATTERN) as $file) {
             if ($file->isFile()) {
                 $pastes[] = $file->getBasename('.php');
             }
@@ -431,20 +439,6 @@ class Filesystem extends AbstractData
             '.discussion' . DIRECTORY_SEPARATOR;
     }
 
-    /**
-     * Get an iterator matching paste files.
-     *
-     * Note that creating the iterator issues the glob() call, so we can't pre-
-     * generate this object before files that should get matched exist.
-     *
-     * @access private
-     * @return \GlobIterator
-     */
-    private function _getPasteIterator()
-    {
-        return new \GlobIterator($this->_path . self::PASTE_FILE_PATTERN);
-    }
-
     /**
      * store the data
      *

lib/Request.php

@@ -12,6 +12,8 @@
 
 namespace PrivateBin;
 
+use Exception;
+
 /**
  * Request
  *
@@ -110,9 +112,13 @@ class Request
             case 'POST':
                 // it might be a creation or a deletion, the latter is detected below
                 $this->_operation = 'create';
-                $this->_params    = Json::decode(
-                    file_get_contents(self::$_inputStream)
-                );
+                try {
+                    $this->_params = Json::decode(
+                        file_get_contents(self::$_inputStream)
+                    );
+                } catch (Exception $e) {
+                    // ignore error, $this->_params will remain empty
+                }
                 break;
             default:
                 $this->_params = $_GET;

tst/Bootstrap.php

@@ -149,7 +149,7 @@ class BucketStub extends Bucket
         throw new BadMethodCallException('not supported by this stub');
     }
 
-    public function exists()
+    public function exists(array $options = array())
     {
         return true;
     }

tst/ControllerTest.php

@@ -436,8 +436,6 @@ class ControllerTest extends PHPUnit_Framework_TestCase
      * silently removed, check that this case is handled
      *
      * @runInSeparateProcess
-     * @expectedException Exception
-     * @expectedExceptionCode 90
      */
     public function testCreateBrokenUpload()
     {
@@ -449,7 +447,12 @@ class ControllerTest extends PHPUnit_Framework_TestCase
         $_SERVER['REQUEST_METHOD']        = 'POST';
         $_SERVER['REMOTE_ADDR']           = '::1';
         $this->assertFalse($this->_data->exists(Helper::getPasteId()), 'paste does not exists before posting data');
+        ob_start();
         new Controller;
+        $content = ob_get_contents();
+        ob_end_clean();
+        $response = json_decode($content, true);
+        $this->assertEquals(1, $response['status'], 'outputs error status');
         $this->assertFalse($this->_data->exists(Helper::getPasteId()), 'paste exists after posting data');
     }
 

tst/RequestTest.php

@@ -97,7 +97,7 @@ class RequestTest extends PHPUnit_Framework_TestCase
         Request::setInputStream($file);
         $request = new Request;
         unlink($file);
-        $this->assertTrue($request->isJsonApiCall(), 'is JSON Api call');
+        $this->assertTrue($request->isJsonApiCall(), 'is JSON API call');
         $this->assertEquals('create', $request->getOperation());
         $this->assertEquals('foo', $request->getParam('ct'));
     }
@@ -111,7 +111,7 @@ class RequestTest extends PHPUnit_Framework_TestCase
         file_put_contents($file, '{"ct":"foo"}');
         Request::setInputStream($file);
         $request = new Request;
-        $this->assertTrue($request->isJsonApiCall(), 'is JSON Api call');
+        $this->assertTrue($request->isJsonApiCall(), 'is JSON API call');
         $this->assertEquals('create', $request->getOperation());
         $this->assertEquals('foo', $request->getParam('ct'));
     }
@@ -125,7 +125,7 @@ class RequestTest extends PHPUnit_Framework_TestCase
         $_SERVER['QUERY_STRING']   = $id;
         $_GET[$id]                 = '';
         $request                   = new Request;
-        $this->assertTrue($request->isJsonApiCall(), 'is JSON Api call');
+        $this->assertTrue($request->isJsonApiCall(), 'is JSON API call');
         $this->assertEquals($id, $request->getParam('pasteid'));
         $this->assertEquals('read', $request->getOperation());
     }
@@ -142,12 +142,25 @@ class RequestTest extends PHPUnit_Framework_TestCase
         file_put_contents($file, '{"deletetoken":"bar"}');
         Request::setInputStream($file);
         $request = new Request;
-        $this->assertTrue($request->isJsonApiCall(), 'is JSON Api call');
+        $this->assertTrue($request->isJsonApiCall(), 'is JSON API call');
         $this->assertEquals('delete', $request->getOperation());
         $this->assertEquals($id, $request->getParam('pasteid'));
         $this->assertEquals('bar', $request->getParam('deletetoken'));
     }
 
+    public function testPostGarbage()
+    {
+        $this->reset();
+        $_SERVER['REQUEST_METHOD']        = 'POST';
+        $file                             = tempnam(sys_get_temp_dir(), 'FOO');
+        file_put_contents($file, random_bytes(256));
+        Request::setInputStream($file);
+        $request = new Request;
+        unlink($file);
+        $this->assertFalse($request->isJsonApiCall(), 'is HTML call');
+        $this->assertEquals('create', $request->getOperation());
+    }
+
     public function testReadWithNegotiation()
     {
         $this->reset();