Problem/Motivation

\Drupal\file\Plugin\rest\resource\FileUploadResource is using PHP's builtin basename() which is vulnerable to https://bugs.php.net/bug.php?id=77239

Proposed resolution

Use \Drupal\Core\File\FileSystem::basename() instead and add test coverage.

Remaining tasks

  1. commit

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me :)

The last submitted patch, 2: 3032620-2.test-only.patch, failed testing. View results

dww’s picture

Yes, we discovered this bug at #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) while adding extensive test coverage for the transliterate case.
We tried to instead fix it as part of #2940383: Unify file upload logic of REST and JSON:API but @wim isn't happy about that. ;)
I agree we might as well fix it here, separately, and then All The Other File Issues(tm) can stop worrying and arguing about it. ;)

Patch looks great. Simple fix. Includes a test. Note: the php bug we're solving seems to mainly impact files with an mb char at the front of the filename, which is why our previous test coverage didn't catch this.

FWIW, this applies cleanly to 8.7.x, too, and at least Drupal\Tests\file\Functional\FileUploadJsonBasicAuthTest passes locally (which is a test that uses FileUploadResourceTestBase):

 ./vendor/bin/phpunit --verbose -c core/phpunit.xml ./core/modules/file/tests/src/Functional/FileUploadJsonBasicAuthTest.php
PHPUnit 6.5.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.8
Configuration: /Applications/MAMP-common/htdocs/drupal-8_7/core/phpunit.xml

Testing Drupal\Tests\file\Functional\FileUploadJsonBasicAuthTest
..........                                                        10 / 10 (100%)

Time: 3.05 minutes, Memory: 4.00MB

OK (10 tests, 151 assertions)

All that's to say: definitely RTBC. ;)

Thanks,
-Derek

Wim Leers’s picture

Component: base system » file.module
Issue tags: +API-First Initiative, +Security improvements

Looks great, thanks for splitting this off — this now became a trivial commit :)

Wim Leers’s picture

(This came up in #2940383-35: Unify file upload logic of REST and JSON:API, I argued against expanding the scope of that issue to do this bugfix, since #2940383 is solely about moving code. If we both move and change code, it becomes really hard to review.)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3032620-2.patch, failed testing. View results

dww’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

I think the testbot is still recovering from all the trouble. Trying to re-upload #2 to see if this helps.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

So back to rtbc based on #3 and the further comments.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3032620-2-again.patch, failed testing. View results

dww’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3032620-2-again.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

  • larowlan committed e1ec253 on 8.7.x
    Issue #3032620 by alexpott, dww: \Drupal\file\Plugin\rest\resource\...

  • larowlan committed 0da311f on 8.6.x
    Issue #3032620 by alexpott, dww: \Drupal\file\Plugin\rest\resource\...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed e1ec253 and pushed to 8.7.x. Thanks!

C/p as 0da311f4b4 and pushed to 8.6.x

Wim Leers’s picture

🥳

Thanks for this! 👌

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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