Reject invalid uploads and expand flow tests.

2026-09-26 15:27:51 +02:00
parent 716a609f4e
commit 8f4851e576
9 changed files with 427 additions and 11 deletions
@@ -181,6 +181,24 @@ class SubmitTest extends PageTestCase {
    $this->assertNotSame($first, $second);
  }

+  #[DataProvider('uploadActions')]
+  public function testANestedUploadIsRejectedWithoutSaving(string $action): void {
+    $token = $this->csrfToken($this->get('/publish')['body']);
+    $before = $this->tables();
+    $page = $this->postFiles('/publish', $this->story(array('action' => $action, 'csrf_token' => $token,
+                                                          'g-recaptcha-response' => 'pass')),
+                             array('upload[]' => array('a.png', TestCase::png())));
+    $this->assertSame(200, $page['status']);
+    $this->assertStringContainsString('The upload is invalid. Please choose the image again.', $page['body']);
+    $this->assertSame($before, $this->tables());
+    $this->assertSame(array(), $this->uploads());
+    $this->assertArrayNotHasKey('pending_upload', $this->session());
+  }
+
+  public static function uploadActions(): array {
+    return array('preview' => array('preview'), 'submit' => array('submit'));
+  }
+
  public function testAFailedCaptchaKeepsNoImage(): void {
    $page = $this->previewImage(array('g-recaptcha-response' => 'fail'));
    $this->assertStringContainsString('Please complete the captcha.', $page['body']);
@@ -1,6 +1,7 @@
 <?php
 namespace Xxx\Tests\Unit;

+use PHPUnit\Framework\Attributes\DataProvider;
 use Xxx\Tests\Support\FakeDb;
 use Xxx\Tests\Support\Mail;
 use Xxx\Tests\Support\SiteData;
@@ -110,6 +111,71 @@ class SaveDataTest extends TestCase {
    $this->assertContains("O'Brien\"; DROP TABLE xxx; --", $insert['params']);
  }

+  public static function failedQueries(): array {
+    return array(
+      'visitor insert'     => array(FALSE, 'post', array(), array(), '/^INSERT INTO queue/'),
+      'editor insert'      => array(TRUE, 'submit', array(), array(), '/^INSERT INTO queue/'),
+      'queue update'       => array(TRUE, 'submit', array('id' => '5'), array(), '/^UPDATE queue/'),
+      'live update'        => array(TRUE, 'post', array('id' => '1'), array(), '/^UPDATE xxx/'),
+      'move from queue'    => array(TRUE, 'post', array('id' => '5'), array('queue' => '1'), '/^INSERT INTO xxx/'),
+      'queue image lookup' => array(TRUE, 'post', array('id' => '5', 'uploaded' => '5'), array('queue' => '1'), '/^SELECT img, mime FROM queue/'),
+    );
+  }
+
+  #[DataProvider('failedQueries')]
+  public function testAFailedQueryKeepsTheStoryAndUploadWithoutSendingMail(bool $admin, string $action, array $post, array $get, string $pattern): void {
+    $pending = $this->pendingImage();
+    $session = $_SESSION + array('captcha_ok' => time());
+    $state = $this->tmp . '/failed-save';
+    file_put_contents($state, serialize(array(
+      'tables' => self::TABLES, 'admin' => $admin, 'action' => $action, 'pattern' => $pattern,
+      'post' => $post + array('title' => 'Title', 'author' => 'Ann', 'story' => '<p>Text.</p>', 'uploaded' => 'pending'),
+      'get' => $get, 'session' => $session,
+    )));
+    $file = var_export($state, TRUE);
+    $code = <<<PHP
+      \$state = unserialize(file_get_contents($file));
+      Xxx\Tests\Support\SiteData::install(\$state['tables']);
+      Xxx\Tests\Support\FakeDb::on(\$state['pattern'], FALSE);
+      open_db('open');
+      \$is_admin = \$state['admin'];
+      \$action = \$state['action'];
+      \$_POST = \$state['post'];
+      \$_GET = \$state['get'];
+      \$_REQUEST = array_merge(\$_GET, \$_POST);
+      \$_SESSION = \$state['session'];
+      \$before = Xxx\Tests\Support\SiteData::\$tables;
+      \$returned = FALSE;
+      // db_fail() exits, so save the result at shutdown.
+      register_shutdown_function(function () use (\$before, &\$returned) {
+        file_put_contents($file, serialize(array(
+          'before' => \$before, 'tables' => Xxx\Tests\Support\SiteData::\$tables,
+          'session' => \$_SESSION, 'queries' => Xxx\Tests\Support\FakeDb::queries(),
+          'returned' => \$returned, 'status' => http_response_code(),
+        )));
+      });
+      save_data();
+      \$returned = TRUE;
+      PHP;
+    $result = Mail::run($code, array('UPLOAD_DIR' => $this->tmp . '/uploads'));
+    $this->assertSame(0, $result['status'], $result['stderr']);
+    $this->assertSame('', $result['stderr']);
+    $this->assertSame('<p>Sorry, something went wrong. Please try again later.</p>', $result['stdout']);
+    $this->assertStringContainsString('xxx: database error at post.lib.php:', $result['log']);
+    $this->assertStringNotContainsString('Warning', $result['log']);
+    $this->assertStringNotContainsString('Notice', $result['log']);
+    $this->assertSame(array(), $result['mails']);
+    $saved = unserialize(file_get_contents($state));
+    $this->assertFalse($saved['returned']);
+    $this->assertSame(500, $saved['status']);
+    $this->assertSame($saved['before'], $saved['tables']);
+    $this->assertSame($session, $saved['session']);
+    $this->assertFileExists($pending);
+    $this->assertSame(self::png(), file_get_contents($pending));
+    $this->assertCount(1, preg_grep($pattern, $saved['queries']));
+    $this->assertMatchesRegularExpression($pattern, end($saved['queries']), 'no queries run after the failure');
+  }
+
  // Visitors always add to the queue, and each story sends mail
  public function testVisitorsCanOnlyAddToTheQueue(): void {
    $state = $this->tmp . '/state';
@@ -140,6 +140,47 @@ class SessionTest extends TestCase {
    $this->assertNotSame(persistent_key_hash('key'), $rows[0]['session'], 'the used key is gone');
  }

+  public function testAUsedPersistentCookieCannotBeReplayedAndOtherDevicesStillWork(): void {
+    SiteData::$tables['editors'][] = array('username' => 'ed', 'password' => '', 'email' => '');
+    foreach (array('first-device', 'second-device') as $key) {
+      SiteData::$tables['persistent_sessions'][] = array('username' => 'ed', 'session' => persistent_key_hash($key),
+                                                         'created' => gmdate('Y-m-d H:i:s'));
+    }
+    session_start();
+    $_COOKIE['PersistentAuth'] = 'ed|first-device';
+    $this->assertTrue(check_persistent_login());
+    $this->assertSame('ed', $_SESSION['editor']);
+    $rotated = SiteData::$tables['persistent_sessions'];
+    $this->assertCount(2, $rotated);
+    $this->assertNotContains(persistent_key_hash('first-device'), array_column($rotated, 'session'));
+    $this->assertContains(persistent_key_hash('second-device'), array_column($rotated, 'session'));
+
+    // Replay the old cookie without the authenticated PHP session.
+    session_destroy();
+    $_SESSION = array();
+    session_id(bin2hex(random_bytes(16)));
+    session_start();
+    $_COOKIE['PersistentAuth'] = 'ed|first-device';
+    $this->assertFalse(check_persistent_login());
+    $this->assertArrayNotHasKey('editor', $_SESSION);
+    $this->assertSame($rotated, SiteData::$tables['persistent_sessions']);
+
+    session_destroy();
+    $_SESSION = array();
+    session_id(bin2hex(random_bytes(16)));
+    session_start();
+    $_COOKIE['PersistentAuth'] = 'ed|second-device';
+    $this->assertTrue(check_persistent_login());
+    $this->assertSame('ed', $_SESSION['editor']);
+    $hashes = array_column(SiteData::$tables['persistent_sessions'], 'session');
+    $this->assertCount(2, $hashes);
+    $this->assertNotContains(persistent_key_hash('second-device'), $hashes);
+    foreach ($rotated as $row) {
+      if ($row['session'] !== persistent_key_hash('second-device'))
+        $this->assertContains($row['session'], $hashes, 'the first device keeps its rotated token');
+    }
+  }
+
  public function testCheckPersistentLoginRefusesAnExpiredKey(): void {
    SiteData::$tables['editors'][] = array('username' => 'ed', 'password' => '', 'email' => '');
    SiteData::$tables['persistent_sessions'][] = array('username' => 'ed', 'session' => persistent_key_hash('key'),
@@ -1,6 +1,7 @@
 <?php
 namespace Xxx\Tests\Unit;

+use PHPUnit\Framework\Attributes\DataProvider;
 use Xxx\Tests\Support\TestCase;

 // The upload directory and the pending upload in get_data.lib.php and publish/post.lib.php
@@ -88,6 +89,49 @@ class UploadTest extends TestCase {
    $this->assertFalse(save_uploaded_file());
  }

+  public static function arrayTemporaryNames(): array {
+    return array(
+      'empty array' => array(array()),
+      'file list'   => array(array('/tmp/not-an-upload')),
+    );
+  }
+
+  #[DataProvider('arrayTemporaryNames')]
+  public function testAnArrayTemporaryNameIsRefused(array $name): void {
+    $pending = $this->pending();
+    $_FILES['upload'] = array('tmp_name' => $name, 'name' => array('a.png'), 'error' => array(UPLOAD_ERR_OK));
+    $this->assertFalse(save_uploaded_file());
+    $_POST['uploaded'] = 'pending';
+    $this->assertFalse(save_uploaded_file(), 'an invalid new upload does not select the pending image');
+    $this->assertFileExists($pending);
+    $this->assertSame(basename($pending), $_SESSION['pending_upload']);
+  }
+
+  public static function missingTemporaryNames(): array {
+    return array(
+      'absent entry' => array(array()),
+      'absent name'  => array(array('upload' => array('error' => UPLOAD_ERR_NO_FILE))),
+      'empty name'   => array(array('upload' => array('tmp_name' => ''))),
+    );
+  }
+
+  #[DataProvider('missingTemporaryNames')]
+  public function testAMissingTemporaryNameLeavesThePendingUploadAlone(array $files): void {
+    $pending = $this->pending();
+    $_FILES = $files;
+    $this->assertFalse(save_uploaded_file());
+    $this->assertFileExists($pending);
+    $this->assertSame(basename($pending), $_SESSION['pending_upload']);
+    $_POST['uploaded'] = 'pending';
+    $this->assertSame($pending, save_uploaded_file());
+  }
+
+  public function testAMissingTemporaryFileIsNotSaved(): void {
+    $_FILES['upload'] = array('name' => 'a.png', 'tmp_name' => $this->tmp . '/missing', 'error' => UPLOAD_ERR_OK);
+    $this->assertFalse(save_uploaded_file());
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
+  }
+
  // 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),
@@ -1,6 +1,7 @@
 <?php
 namespace Xxx\Tests\Unit;

+use PHPUnit\Framework\Attributes\DataProvider;
 use Xxx\Tests\Support\FakeRecaptcha;
 use Xxx\Tests\Support\TestCase;

@@ -120,6 +121,79 @@ class ValidateDataTest extends TestCase {
    $this->assertSame(array('upload' => "&lt;b&gt;notes&lt;/b&gt;.txt wasn't an image file. Please go back."), $this->validate(array()));
  }

+  public static function invalidUploadFields(): array {
+    return array(
+      'array filename'       => array('name', array('a.png')),
+      'array temporary file' => array('tmp_name', array('/tmp/not-an-upload')),
+      'array error code'     => array('error', array(UPLOAD_ERR_OK)),
+      'numeric filename'     => array('name', 123),
+      'boolean temporary file' => array('tmp_name', FALSE),
+      'string error code'    => array('error', '0'),
+    );
+  }
+
+  #[DataProvider('invalidUploadFields')]
+  public function testAnInvalidUploadFieldIsRejected(string $field, mixed $value): void {
+    $this->imageOf(strlen(self::png()));
+    $_FILES['upload'][$field] = $value;
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
+  }
+
+  public function testANestedUploadIsRejected(): void {
+    $_FILES['upload'] = array('name' => array('a.png'), 'tmp_name' => array('/tmp/not-an-upload'),
+                              'error' => array(UPLOAD_ERR_OK), 'size' => array(1), 'type' => array('image/png'));
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
+  }
+
+  public static function absentUploads(): array {
+    return array(
+      'no upload entry' => array(array()),
+      'empty entry'     => array(array('upload' => array())),
+      'no file chosen'  => array(array('upload' => array('error' => UPLOAD_ERR_NO_FILE))),
+    );
+  }
+
+  #[DataProvider('absentUploads')]
+  public function testAnAbsentUploadDoesNotBlockATextStory(array $files): void {
+    $_FILES = $files;
+    $this->assertSame(array(), $this->validate(array()));
+  }
+
+  public function testASuccessfulUploadNeedsATemporaryFilename(): void {
+    $_FILES['upload'] = array('name' => 'a.png', 'error' => UPLOAD_ERR_OK);
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+  }
+
+  public function testAMissingTemporaryFileIsRejected(): void {
+    $_FILES['upload'] = array('name' => 'a.png', 'tmp_name' => $this->tmp . '/missing', 'error' => UPLOAD_ERR_OK);
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+    $this->assertArrayNotHasKey('pending_upload', $_SESSION);
+  }
+
+  public function testAScalarUploadEntryIsRejected(): void {
+    $_FILES['upload'] = 'not a file entry';
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+  }
+
+  public function testAnEmptyTemporaryFilenameIsRejected(): void {
+    $_FILES['upload'] = array('name' => 'a.png', 'tmp_name' => '', 'error' => UPLOAD_ERR_OK);
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+  }
+
+  public function testADirectoryIsNotAnUpload(): void {
+    $_FILES['upload'] = array('name' => 'a.png', 'tmp_name' => $this->tmp, 'error' => UPLOAD_ERR_OK);
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+  }
+
+  public function testAnEmptyImageFileIsRejectedWithoutWarnings(): void {
+    $file = $this->tmp . '/empty';
+    file_put_contents($file, '');
+    $_FILES['upload'] = array('name' => 'a.png', 'tmp_name' => $file, 'error' => UPLOAD_ERR_OK);
+    $this->assertArrayHasKey('upload', $this->validate(array()));
+  }
+
  public function testAnImageUploadPasses(): void {
    $file = $this->tmp . '/upload';
    file_put_contents($file, self::png());