Limit the images that previews keep.

A preview that fails the captcha or the form token keeps no image. A session keeps one image, the directory has a size limit, and abandoned images go at the next upload.

.env.example

@@ -25,6 +25,11 @@ GOOGLE_ANALYTICS_ID=
 # web root. The default is xxx-uploads in the system temporary directory.
 UPLOAD_DIR=
 
+# The most that the upload directory holds, in megabytes (default 100).
+# Past this, previews do not keep new images. Abandoned uploads are
+# removed after a day.
+UPLOAD_DIR_LIMIT_MB=
+
 # Show PHP errors in the page (1) or on stderr (stderr). Leave it empty in
 # production: errors always go to the PHP error log.
 DISPLAY_ERRORS=

get_data.lib.php

@@ -377,11 +377,22 @@ function is_upload_name($name) {
   return is_string($name) && preg_match('/^[0-9a-f]{32}\z/', $name);
 }
 
+// Upload files older than this are abandoned previews
+define('UPLOAD_MAX_AGE', 86400);
+
 /*
  * Move a file from $_FILES into the upload directory, and remember it
- * in the session. Returns the new path, or FALSE.
+ * in the session. Returns the new path, or FALSE. A session keeps one
+ * upload: a new one replaces the last. Abandoned uploads are removed
+ * first, and nothing is kept if the directory would grow past
+ * UPLOAD_DIR_LIMIT_MB (100 MB by default).
  */
 function store_upload($tmp_file) {
+  prune_uploads();
+  remove_pending_upload();
+  $limit = (float)config('UPLOAD_DIR_LIMIT_MB', 100) * 1024 * 1024;
+  if (upload_dir_size() + (int)@filesize($tmp_file) > $limit)
+    return FALSE;
   $name = bin2hex(random_bytes(16));
   $file = upload_dir() . '/' . $name;
   if (!move_uploaded_file($tmp_file, $file))
@@ -390,6 +401,35 @@ function store_upload($tmp_file) {
   return $file;
 }
 
+// The upload files, name => path; nothing else in the directory counts
+function upload_files() {
+  $files = array();
+  foreach (glob(upload_dir() . '/*') ?: array() as $file) {
+    if (is_upload_name(basename($file)) && is_file($file))
+      $files[basename($file)] = $file;
+  }
+  return $files;
+}
+
+function upload_dir_size() {
+  return array_sum(array_map(function ($file) { return (int)@filesize($file); }, upload_files()));
+}
+
+// Remove the uploads that are older than UPLOAD_MAX_AGE
+function prune_uploads() {
+  foreach (upload_files() as $file) {
+    if (@filemtime($file) < time() - UPLOAD_MAX_AGE)
+      @unlink($file);
+  }
+}
+
+// Remove the file of this session's upload, and forget it
+function remove_pending_upload() {
+  if ($file = pending_upload())
+    @unlink($file);
+  unset($_SESSION['pending_upload']);
+}
+
 /*
  * Return the path of the image this session uploaded earlier, or FALSE.
  */

publish/post.lib.php

@@ -121,7 +121,7 @@ function save_data($id = NULL) {
 
   // Prune filesystem
   if ($image_file)
-    prune_images($image_file);
+    prune_images();
 
   // email admin if added to the queue
   if ($action == 'submit')
@@ -156,9 +156,9 @@ function get_mimetype($image_file) {
 }
 
 function save_uploaded_file() {
-  // A new upload in this request
+  // A new upload in this request: PHP removes the file after the request
   if (is_uploaded_file($_FILES['upload']['tmp_name'] ?? '')) {
-    return store_upload($_FILES['upload']['tmp_name']);
+    return $_FILES['upload']['tmp_name'];
   }
   // An image uploaded at the preview step; the session knows which one
   if (($_POST['uploaded'] ?? '') === 'pending') {
@@ -167,22 +167,10 @@ function save_uploaded_file() {
   return FALSE;
 }
 
-function prune_images($image_file) {
+// After a save: remove this session's upload, and any abandoned uploads
-  // Delete the saved image, and anything older than $age.
+function prune_images() {
-  // Only ever touch upload files inside the upload directory.
+  remove_pending_upload();
-  $age = 86400; // a day old
+  prune_uploads();
-  $dir = upload_dir();
-  if (dirname($image_file) === $dir && is_upload_name(basename($image_file))) {
-    @unlink($image_file);
-  }
-  unset($_SESSION['pending_upload']);
-
-  foreach (glob($dir . '/*') as $cleanup_file) {
-    if (is_upload_name(basename($cleanup_file)) &&
-        @filemtime($cleanup_file) < (time() - $age)) {
-      @unlink($cleanup_file);
-    }
-  }
 }
 
 /*

publish/preview.inc.php

@@ -5,10 +5,14 @@ extract($result[0], EXTR_OVERWRITE);
 echo '<p>Please review your submission. Once you\'ve finished, please click "Submit" at the bottom of the page, or click "Preview" to check it again.</p>';
 
 // Keep a new image outside the web root until the story is saved;
-// display.php shows it to this session only.
+// display.php shows it to this session only. A request that failed the
-if (is_uploaded_file($_FILES['upload']['tmp_name'] ?? '') && !isset($validate['upload'])) {
+// captcha or the form token keeps nothing.
+if (is_uploaded_file($_FILES['upload']['tmp_name'] ?? '') &&
+    !isset($validate['upload']) && !isset($validate['captcha']) && !isset($validate['csrf'])) {
   if (store_upload($_FILES['upload']['tmp_name']))
     $file_src = pending_upload_url();
+  else
+    $validate['upload'] = 'The site cannot keep more images just now. Please try again later.';
 } else if (isset($uploaded) && $uploaded === 'pending' && pending_upload() && !$remove_img) {
   $file_src = pending_upload_url();
 }

tests/php/Pages/SubmitTest.php

@@ -2,6 +2,7 @@
 namespace Xxx\Tests\Pages;
 
 use Xxx\Tests\Support\PageTestCase;
+use Xxx\Tests\Support\TestCase;
 
 // Sending a story from publish/ as a visitor
 class SubmitTest extends PageTestCase {
@@ -125,4 +126,81 @@ class SubmitTest extends PageTestCase {
     $this->assertStringNotContainsString('<script>alert(1)</script>', $page['body']);
     $this->assertStringContainsString('value="&quot;&gt;&lt;script&gt;alert(1)&lt;/script&gt;"', $page['body']);
   }
+
+  // Preview the story with an image; returns the answer
+  private function previewImage(array $fields = array()): array {
+    $token = $this->csrfToken($this->get('/publish')['body']);
+    return $this->postFiles('/publish', $this->story($fields + array('action' => 'preview', 'csrf_token' => $token, 'g-recaptcha-response' => 'pass')),
+                            array('upload' => array('a.png', TestCase::png())));
+  }
+
+  public function testAPreviewKeepsTheImageForTheSession(): void {
+    $page = $this->previewImage();
+    $this->assertStringContainsString('/display?pending=', $page['body']);
+    $uploads = $this->uploads();
+    $this->assertCount(1, $uploads);
+    $this->assertSame(basename($uploads[0]), $this->session()['pending_upload']);
+  }
+
+  public function testANewPreviewReplacesTheImage(): void {
+    $this->previewImage();
+    $first = $this->uploads();
+    $this->postFiles('/publish', $this->story(array('action' => 'preview', 'csrf_token' => $this->session()['csrf_token'])),
+                     array('upload' => array('b.png', TestCase::png())));
+    $second = $this->uploads();
+    $this->assertCount(1, $second);
+    $this->assertNotSame($first, $second);
+  }
+
+  public function testAFailedCaptchaKeepsNoImage(): void {
+    $page = $this->previewImage(array('g-recaptcha-response' => 'fail'));
+    $this->assertStringContainsString('Please complete the captcha.', $page['body']);
+    $this->assertSame(array(), $this->uploads());
+    $this->assertArrayNotHasKey('pending_upload', $this->session());
+  }
+
+  public function testAFailedTokenKeepsNoImage(): void {
+    $page = $this->previewImage(array('csrf_token' => 'wrong'));
+    $this->assertStringContainsString('Your session has expired.', $page['body']);
+    $this->assertSame(array(), $this->uploads());
+  }
+
+  public function testAFullUploadDirectoryKeepsNoImage(): void {
+    // tests/php/fixtures/test.env sets UPLOAD_DIR_LIMIT_MB=1
+    mkdir(self::$state . '/tmp/uploads', 0700);
+    $filler = self::$state . '/tmp/uploads/' . str_repeat('f', 32);
+    file_put_contents($filler, str_repeat('x', 1024 * 1024));
+    $page = $this->previewImage();
+    $this->assertStringContainsString('The site cannot keep more images just now.', $page['body']);
+    $this->assertSame(array($filler), $this->uploads());
+  }
+
+  public function testAbandonedUploadsGoAtTheNextUpload(): void {
+    mkdir(self::$state . '/tmp/uploads', 0700);
+    $old = self::$state . '/tmp/uploads/' . str_repeat('a', 32);
+    $recent = self::$state . '/tmp/uploads/' . str_repeat('b', 32);
+    file_put_contents($old, 'x');
+    file_put_contents($recent, 'x');
+    touch($old, time() - 86401);
+    $this->previewImage();
+    $this->assertFileDoesNotExist($old);
+    $this->assertFileExists($recent);
+    $this->assertCount(2, $this->uploads());
+  }
+
+  public function testThePreviewedImageIsSavedThenRemoved(): void {
+    $this->previewImage();
+    $this->post('/publish', $this->story(array('action' => 'submit', 'uploaded' => 'pending', 'csrf_token' => $this->session()['csrf_token'])));
+    $this->assertSame(TestCase::png(), $this->tables()['queue'][0]['img']);
+    $this->assertSame(array(), $this->uploads());
+    $this->assertArrayNotHasKey('pending_upload', $this->session());
+  }
+
+  public function testSubmittingWithAnImageKeepsNoFile(): void {
+    $token = $this->csrfToken($this->get('/publish')['body']);
+    $this->postFiles('/publish', $this->story(array('action' => 'submit', 'csrf_token' => $token, 'g-recaptcha-response' => 'pass')),
+                     array('upload' => array('a.png', TestCase::png())));
+    $this->assertSame(TestCase::png(), $this->tables()['queue'][0]['img']);
+    $this->assertSame(array(), $this->uploads());
+  }
 }

tests/php/Support/PageTestCase.php

@@ -161,13 +161,24 @@ abstract class PageTestCase extends \PHPUnit\Framework\TestCase {
    * name => list of values, 'body' => ...). Cookies go and come back as a
    * browser sends them, without regard to domain or the secure flag.
    */
-  protected function request(string $method, string $path, array $post = array(), array $headers = array()): array {
+  protected function request(string $method, string $path, array $post = array(), array $headers = array(), array $files = array()): array {
     if ($this->cookies) {
       $headers['Cookie'] = implode('; ', array_map(function ($name, $value) { return "$name=" . rawurlencode($value); },
                                                    array_keys($this->cookies), $this->cookies));
     }
     $options = array('method' => $method, 'follow_location' => 0, 'ignore_errors' => TRUE, 'timeout' => 30);
-    if ($method === 'POST') {
+    if ($method === 'POST' && $files) {
+      // A multipart form; $files holds field => array(file name, content)
+      $boundary = 'xxx-' . bin2hex(random_bytes(8));
+      $body = '';
+      foreach ($post as $name => $value)
+        $body .= "--$boundary\r\nContent-Disposition: form-data; name=\"$name\"\r\n\r\n$value\r\n";
+      foreach ($files as $name => list($filename, $content))
+        $body .= "--$boundary\r\nContent-Disposition: form-data; name=\"$name\"; filename=\"$filename\"\r\n"
+               . "Content-Type: application/octet-stream\r\n\r\n$content\r\n";
+      $options['content'] = $body . "--$boundary--\r\n";
+      $headers['Content-Type'] = "multipart/form-data; boundary=$boundary";
+    } else if ($method === 'POST') {
       $options['content'] = http_build_query($post);
       $headers['Content-Type'] = 'application/x-www-form-urlencoded';
     }
@@ -207,6 +218,16 @@ abstract class PageTestCase extends \PHPUnit\Framework\TestCase {
     return $this->request('POST', $path, $post, $headers);
   }
 
+  // A POST with files: $files holds field => array(file name, content)
+  protected function postFiles(string $path, array $post, array $files): array {
+    return $this->request('POST', $path, $post, array(), $files);
+  }
+
+  // The files in the upload directory of the server
+  protected function uploads(): array {
+    return glob(self::$state . '/tmp/uploads/*') ?: array();
+  }
+
   protected function header(array $response, string $name): ?string {
     return $response['headers'][strtolower($name)][0] ?? NULL;
   }

tests/php/Unit/UploadTest.php

@@ -88,28 +88,64 @@ class UploadTest extends TestCase {
     $this->assertFalse(save_uploaded_file());
   }
 
-  public function testPruneDeletesTheSavedFileAndOldUploads(): void {
+  // Files in the upload directory: an old upload, a new one and a file that is not an upload
-    $saved = $this->pending();
+  private function others(): array {
-    $old = upload_dir() . '/' . str_repeat('c', 32);
+    $files = array('old' => upload_dir() . '/' . str_repeat('c', 32), 'new' => upload_dir() . '/' . str_repeat('d', 32),
-    $new = upload_dir() . '/' . str_repeat('d', 32);
+                   'other' => upload_dir() . '/keep-me');
-    $other = upload_dir() . '/keep-me';
+    foreach ($files as $file)
-    foreach (array($old, $new, $other) as $file)
+      file_put_contents($file, 'xx');
-      file_put_contents($file, 'x');
+    touch($files['old'], time() - 86401);
-    touch($old, time() - 86401);
+    touch($files['other'], time() - 86401);
-    touch($other, time() - 86401);
+    return $files;
+  }
 
-    prune_images($saved);
+  public function testPruneImagesDeletesTheSessionUploadAndOldUploads(): void {
+    $saved = $this->pending();
+    $files = $this->others();
+    prune_images();
     $this->assertFileDoesNotExist($saved);
-    $this->assertFileDoesNotExist($old);
+    $this->assertFileDoesNotExist($files['old']);
-    $this->assertFileExists($new);
+    $this->assertFileExists($files['new']);
-    $this->assertFileExists($other);
+    $this->assertFileExists($files['other']);
     $this->assertArrayNotHasKey('pending_upload', $_SESSION);
   }
 
-  public function testPruneLeavesFilesOutsideTheUploadDirectory(): void {
+  public function testPruneUploadsKeepsTheSessionUploadUntilItIsOld(): void {
-    $outside = $this->tmp . '/' . str_repeat('e', 32);
+    $pending = $this->pending();
-    file_put_contents($outside, 'x');
+    prune_uploads();
-    prune_images($outside);
+    $this->assertFileExists($pending);
-    $this->assertFileExists($outside);
+    touch($pending, time() - 86401);
+    prune_uploads();
+    $this->assertFileDoesNotExist($pending);
+  }
+
+  public function testRemovePendingUploadTouchesOnlyTheSessionFile(): void {
+    $files = $this->others();
+    remove_pending_upload();
+    $pending = $this->pending();
+    remove_pending_upload();
+    $this->assertFileDoesNotExist($pending);
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
+    foreach ($files as $file)
+      $this->assertFileExists($file);
+  }
+
+  public function testUploadDirSizeCountsOnlyUploads(): void {
+    $this->others();
+    $this->pending('12345');
+    $this->assertSame(2 + 2 + 5, upload_dir_size());
+  }
+
+  public function testStoreUploadPrunesAndDropsTheLastUploadFirst(): void {
+    $last = $this->pending();
+    $files = $this->others();
+    $file = $this->tmp . '/local';
+    file_put_contents($file, 'x');
+    // Not a real upload, so nothing is kept, but the old files still go
+    $this->assertFalse(store_upload($file));
+    $this->assertFileDoesNotExist($last);
+    $this->assertFileDoesNotExist($files['old']);
+    $this->assertFileExists($files['new']);
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
   }
 }

tests/php/fixtures/test.env

@@ -8,4 +8,5 @@ RECAPTCHA_SITE_KEY=test-site-key
 RECAPTCHA_SECRET=test-secret
 GOOGLE_ANALYTICS_ID=
 UPLOAD_DIR=
+UPLOAD_DIR_LIMIT_MB=1
 DISPLAY_ERRORS=