#Security improvements

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

Automatically closed - issue fixed for 2 weeks with no activity.

Committed 03a5735 and pushed to 11.x. Thanks!

xjm → credited alexpott → .

xjm → credited lauriii → .

xjm → credited star-szr → .

Adding credits for triage from #12.

xjm, alexpott, star-szr, joelpittet, lauriii

Automatically closed - issue fixed for 2 weeks with no activity.

just curious,
- why https://www.cyber.gov.au/node/add/testXYZ returns 404, but
- why https://www.cyber.gov.au/node/add/тест would return unhandled exception?
should it just return 404?

Patch throws an exception, but it has to be handled somewhere - at the moment any Drupal website shows white screen of death on 'node/add/тест'.

Appears to have unresolved threads.

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 the unit test keeps failing, re-ran twice

xjm → changed the visibility of the branch 3527408-add-test-for to hidden.

xjm → changed the visibility of the branch 3527408-revert-change to hidden.

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.

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

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).

@mcdruid:

I am sorry for the delay in adding a comment. As you said in Comment #26, the Usability team looked at this issue on 2025-05-30. I just reviewed the discussion, and your summary in #26 looks correct.

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell.

Thanks for clarifying that the change will not affect new uploads. In fact, the change to the whitespace default will only affect newly installed sites (or sites that newly install the file module).

At the meeting, we had the impression that stripping punctuation was non-negotiable, so the only change we fully considered was the default handling of whitespace.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Sounds like a great idea.

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!

xjm → credited star-szr → .

Crediting triage as per #19.

xjm → credited alexpott → .

xjm → credited catch → .

Adding triage credits as per #86.

I discussed this with @catch, @xjm, and @alexpott, and regrettably, we agree with Wim's earlier conclusion in #56 and again in #80 that this isn't viable for core in 8.0.

It's definitely eligible to be considered now on that basis, although we'd want to evaluate whether the approach is still valid.

Amending attribution.

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. :)