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
- commit
User interface changes
none
API changes
none
Data model changes
none
Release notes snippet
n/a
Comment | File | Size | Author |
---|---|---|---|
#9 | 3032620-2-again.patch | 1.97 KB | dww |
#2 | 3032620-2.patch | 1.97 KB | alexpott |
#2 | 3032620-2.test-only.patch | 1.31 KB | alexpott |
Comments
Comment #2
alexpottComment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks great to me :)
Comment #5
dwwYes, 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 usesFileUploadResourceTestBase
):All that's to say: definitely RTBC. ;)
Thanks,
-Derek
Comment #6
Wim LeersLooks great, thanks for splitting this off — this now became a trivial commit :)
Comment #7
Wim Leers(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.)
Comment #9
dwwI think the testbot is still recovering from all the trouble. Trying to re-upload #2 to see if this helps.
Comment #10
alexpottSo back to rtbc based on #3 and the further comments.
Comment #12
dww#3035320: Random fails in TimestampAgoFormatterTest and DateTimeTimeAgoFormatterTest
Re-queued #9.
Comment #14
alexpottComment #17
larowlanCommitted e1ec253 and pushed to 8.7.x. Thanks!
C/p as 0da311f4b4 and pushed to 8.6.x
Comment #18
Wim Leers🥳
Thanks for this! 👌
Comment #19
Wim Leers