Add test for SA-CORE-2024-002

Reading https://www.drupal.org/sa-core-2024-002 believe this is a good coverage.

In reply to:

Reading https://www.drupal.org/sa-core-2024-002 believe this is a good coverage.

I don't think the following paragraph contains sufficient information to determine if it is good test coverage or not:

Under certain uncommon site configurations, a bug in the CKEditor 5 module can cause some image uploads to move the entire webroot to a different location on the file system. This could be exploited by a malicious user to take down a site.

The test code in the MR doesn't appear to have anything to do with that, if one doesn't already know the internal details of the vulnerability as I do.

The commit corresponding to the SA might be more helpful to review.

Ideally, it would be reviewed by someone with access to the original private issue report (which I've added to the summary). Reviewing it might be a good task for one of our provisional Security Team members. (If you're given access, provisional team, remember not to make any of the info from the private issue public in the course of posting here.)

Another step I would expect to see for reviewing this would to be to revert the commit that fixed SA-CORE-2025-002, and have the test fail as expected, and then document that failure here on the issue.

Crediting folks who contributed to the fix and review thereof on the private issue.

Uploaded an MR with the fix reverted and the unit test added fails

Fail          0.117s testInvalidFile                                                                 
Log           0.442s *** Process execution output ***                                                
    PHPUnit 10.5.46 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.22
    Configuration: /builds/issue/drupal-3527408/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    Time: 00:00.123, Memory: 10.00 MB
    
    CKEditor5Image Controller (Drupal\Tests\ckeditor5\Unit\CKEditor5ImageController)
     ✘ Invalid file
       ┐
       ├ Failed asserting that exception message 'Destination file path is not writable' contains 'The file "foo.txt" exceeds the upload limit defined in your form.'.

So believe this is test coverage works

I still don't think we're quite at a complete RTBC from the above, but I re-reviewed the full details of the private issue and will add my +1 to the RTBC confirming that it does indeed provide test coverage that should prevent the vulnerability from being reintroduced, despite that it is not possible to create an actual full exploit test in this situation. :)

Committed to 11.x, and cherry-picked to 11.2.x, 10.6.x, and 10.5.x as a test coverage improvement for a security issue. Thanks everyone!

This broke head on 10.5.x and 10.6.x https://git.drupalcode.org/project/drupal/-/jobs/5716804

Reverting from there and setting to patch to be ported - the class Drupal\file\Upload\FileUploadHandlerInterface was only added in 11.1 (from memory).

Whoopsidaisy, configuring email notifications for core pipelines is clearly still an issue for me. Only thing I miss about DrupalCI. RIP, Testbot.

I dug a 10.2.x patch out of the private issue and spliced out the test. Applied locally, it has this diff against the previous commit:

diff --git a/core/modules/ckeditor5/tests/src/Unit/CKEditor5ImageControllerTest.php b/core/modules/ckeditor5/tests/src/Unit/CKEditor5ImageControllerTest.php
index bb69c28c6fb..eef04de7a16 100644
--- a/core/modules/ckeditor5/tests/src/Unit/CKEditor5ImageControllerTest.php
+++ b/core/modules/ckeditor5/tests/src/Unit/CKEditor5ImageControllerTest.php
@@ -5,22 +5,25 @@
 namespace Drupal\Tests\ckeditor5\Unit;
 
 use Drupal\ckeditor5\Controller\CKEditor5ImageController;
-use Drupal\ckeditor5\Plugin\CKEditor5PluginManagerInterface;
 use Drupal\Core\Entity\EntityConstraintViolationList;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Entity\EntityTypeRepositoryInterface;
 use Drupal\Core\File\FileSystemInterface;
 use Drupal\Core\Lock\LockBackendInterface;
+use Drupal\Core\Session\AccountInterface;
 use Drupal\editor\EditorInterface;
 use Drupal\file\Entity\File;
 use Drupal\file\FileInterface;
-use Drupal\file\Upload\FileUploadHandlerInterface;
+use Drupal\file\Validation\FileValidatorInterface;
 use Drupal\Tests\UnitTestCase;
 use Prophecy\Argument;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\HttpException;
+use Symfony\Component\Mime\MimeTypeGuesserInterface;
+use Symfony\Component\Validator\ConstraintViolationListInterface;
+use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
 
 /**
  * Tests CKEditor5ImageController.
@@ -30,15 +33,14 @@
  */
 final class CKEditor5ImageControllerTest extends UnitTestCase {
 
-  /**
-   * Tests that upload fails correctly when the file is too large.
-   */
   public function testInvalidFile(): void {
     $file_system = $this->prophesize(FileSystemInterface::class);
     $file_system->move(Argument::any())->shouldNotBeCalled();
     $directory = 'public://';
     $file_system->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY)->willReturn(TRUE);
     $file_system->getDestinationFilename(Argument::cetera())->willReturn('/tmp/foo.txt');
+    $event_dispatcher = $this->prophesize(EventDispatcherInterface::class);
+    $event_dispatcher->dispatch(Argument::any())->willReturnArgument(0);
     $lock = $this->prophesize(LockBackendInterface::class);
     $lock->acquire(Argument::any())->willReturn(TRUE);
     $container = $this->prophesize(ContainerInterface::class);
@@ -55,11 +57,15 @@ public function testInvalidFile(): void {
     $entity_type_repository->getEntityTypeFromClass(File::class)->willReturn('file');
     $container->get('entity_type.repository')->willReturn($entity_type_repository->reveal());
     \Drupal::setContainer($container->reveal());
+    $file_validator = $this->prophesize(FileValidatorInterface::class);
+    $file_validator->validate(Argument::cetera())->willReturn($this->prophesize(ConstraintViolationListInterface::class)->reveal());
     $controller = new CKEditor5ImageController(
       $file_system->reveal(),
-      $this->prophesize(FileUploadHandlerInterface::class)->reveal(),
+      $this->prophesize(AccountInterface::class)->reveal(),
+      $this->prophesize(MimeTypeGuesserInterface::class)->reveal(),
       $lock->reveal(),
-      $this->prophesize(CKEditor5PluginManagerInterface::class)->reveal(),
+      $event_dispatcher->reveal(),
+      $file_validator->reveal(),
     );
     // We can't use vfsstream here because of how Symfony request works.
     $file_uri = tempnam(sys_get_temp_dir(), 'tmp');

Let's see if it passes on 10.6.x.

Seems the unit test keeps failing, re-ran twice

Failing test output is:

---- Drupal\Tests\ckeditor5\Unit\CKEditor5ImageControllerTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-700.xml      0 Drupal\Tests\ckeditor5\Unit\CKEdito
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.23 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\ckeditor5\Unit\CKEditor5ImageControllerTest
    E                                                                   1 / 1
    (100%)R
    
    Time: 00:00.080, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\ckeditor5\Unit\CKEditor5ImageControllerTest::testInvalidFile
    TypeError: Cannot assign null to property
    Drupal\ckeditor5\Controller\CKEditor5ImageController::$fileUploadHandler of
    type Drupal\file\Upload\FileUploadHandler
    
    /builds/issue/drupal-3527408/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php:99
    /builds/issue/drupal-3527408/core/modules/ckeditor5/tests/src/Unit/CKEditor5ImageControllerTest.php:62
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/Command.php:146
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/Command.php:99
    
    --
    
    There was 1 risky test:
    
    1)
    Drupal\Tests\ckeditor5\Unit\CKEditor5ImageControllerTest::testInvalidFile
    This test did not perform any assertions
    
    /builds/issue/drupal-3527408/core/tests/Drupal/Tests/Listeners/DrupalListener.php:62
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/Framework/TestResult.php:453
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/Framework/TestResult.php:981
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/Command.php:146
    /builds/issue/drupal-3527408/vendor/phpunit/phpunit/src/TextUI/Command.php:99
    
    ERRORS!
    Tests: 1, Assertions: 0, Errors: 1, Risky: 1.
    
    Remaining self deprecation notices (1)
    
      1x: Calling
    Drupal\ckeditor5\Controller\CKEditor5ImageController::__construct() with
    the $current_user argument is deprecated in drupal:10.3.0 and is removed
    from drupal:11.0.0. See https://www.drupal.org/node/3388990
        1x in CKEditor5ImageControllerTest::testInvalidFile from

Seems like a good backport now.

Committed and pushed e4a89e5ef80 to 10.6.x and f1f81bba162 to 10.5.x. Thanks!