From c4fcef21aa3e1e3be832b0abafebad70dacde9d1 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Sat, 26 Sep 2026 10:04:55 +0200 Subject: [PATCH] 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 | 5 ++ get_data.lib.php | 42 +++++++++++++++- publish/post.lib.php | 26 +++------- publish/preview.inc.php | 8 ++- tests/php/Pages/SubmitTest.php | 78 ++++++++++++++++++++++++++++++ tests/php/Support/PageTestCase.php | 25 +++++++++- tests/php/Unit/UploadTest.php | 72 ++++++++++++++++++++------- tests/php/fixtures/test.env | 1 + 8 files changed, 215 insertions(+), 42 deletions(-) diff --git a/.env.example b/.env.example index de12362..216e30a 100644 --- a/.env.example +++ b/.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= diff --git a/get_data.lib.php b/get_data.lib.php index f8c4ebf..1e6e603 100644 --- a/get_data.lib.php +++ b/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. */ diff --git a/publish/post.lib.php b/publish/post.lib.php index d7fa539..d813a86 100644 --- a/publish/post.lib.php +++ b/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) { - // Delete the saved image, and anything older than $age. - // Only ever touch upload files inside the upload directory. - $age = 86400; // a day old - $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); - } - } +// After a save: remove this session's upload, and any abandoned uploads +function prune_images() { + remove_pending_upload(); + prune_uploads(); } /* diff --git a/publish/preview.inc.php b/publish/preview.inc.php index 35f9ccb..385f19a 100644 --- a/publish/preview.inc.php +++ b/publish/preview.inc.php @@ -5,10 +5,14 @@ extract($result[0], EXTR_OVERWRITE); echo '

Please review your submission. Once you\'ve finished, please click "Submit" at the bottom of the page, or click "Preview" to check it again.

'; // Keep a new image outside the web root until the story is saved; -// display.php shows it to this session only. -if (is_uploaded_file($_FILES['upload']['tmp_name'] ?? '') && !isset($validate['upload'])) { +// display.php shows it to this session only. A request that failed the +// 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(); } diff --git a/tests/php/Pages/SubmitTest.php b/tests/php/Pages/SubmitTest.php index 77596b9..7bbf167 100644 --- a/tests/php/Pages/SubmitTest.php +++ b/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('', $page['body']); $this->assertStringContainsString('value=""><script>alert(1)</script>"', $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()); + } } diff --git a/tests/php/Support/PageTestCase.php b/tests/php/Support/PageTestCase.php index 0a72088..1aacbd7 100644 --- a/tests/php/Support/PageTestCase.php +++ b/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; } diff --git a/tests/php/Unit/UploadTest.php b/tests/php/Unit/UploadTest.php index 380c8b5..3b1c63f 100644 --- a/tests/php/Unit/UploadTest.php +++ b/tests/php/Unit/UploadTest.php @@ -88,28 +88,64 @@ class UploadTest extends TestCase { $this->assertFalse(save_uploaded_file()); } - public function testPruneDeletesTheSavedFileAndOldUploads(): void { - $saved = $this->pending(); - $old = upload_dir() . '/' . str_repeat('c', 32); - $new = upload_dir() . '/' . str_repeat('d', 32); - $other = upload_dir() . '/keep-me'; - foreach (array($old, $new, $other) as $file) - file_put_contents($file, 'x'); - touch($old, time() - 86401); - touch($other, time() - 86401); + // Files in the upload directory: an old upload, a new one and a file that is not an upload + private function others(): array { + $files = array('old' => upload_dir() . '/' . str_repeat('c', 32), 'new' => upload_dir() . '/' . str_repeat('d', 32), + 'other' => upload_dir() . '/keep-me'); + foreach ($files as $file) + file_put_contents($file, 'xx'); + touch($files['old'], time() - 86401); + touch($files['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->assertFileExists($new); - $this->assertFileExists($other); + $this->assertFileDoesNotExist($files['old']); + $this->assertFileExists($files['new']); + $this->assertFileExists($files['other']); $this->assertArrayNotHasKey('pending_upload', $_SESSION); } - public function testPruneLeavesFilesOutsideTheUploadDirectory(): void { - $outside = $this->tmp . '/' . str_repeat('e', 32); - file_put_contents($outside, 'x'); - prune_images($outside); - $this->assertFileExists($outside); + public function testPruneUploadsKeepsTheSessionUploadUntilItIsOld(): void { + $pending = $this->pending(); + prune_uploads(); + $this->assertFileExists($pending); + 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); } } diff --git a/tests/php/fixtures/test.env b/tests/php/fixtures/test.env index b843953..8bf3bf5 100644 --- a/tests/php/fixtures/test.env +++ b/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=