On upgrading to Drupal 10.2.2 we found that file_save_upload() is now broken. According to its documentation:
Parameters ... array $validators ... To allow all extensions, you must explicitly set this array to ['file_validate_extensions' => ''].
That has worked fine for years. With the upgrade, it blows up. In the debugger we can see that the failure happens in the new FileValidator::validate() method, where an attempt to invoke array_unshift() on the empty string raises a TypeError exception.
$validators parameter set as instructed in the documentation quoted aboveEither fix the code to match the documentation, or revise the documentation to match the modified behavior of the software.
None
The API behavior will once again match the documentation.
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | 3420802-nr-bot.txt | 1.56 KB | needs-review-queue-bot |
| #4 | before-array_unshift.png | 210.89 KB | bkline |
| #4 | 3420802-exception-caught.png | 215.44 KB | bkline |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
cilefen commentedProvide the exception message. You alluded to it but left it out. We may need a stack trace too.
Comment #3
cilefen commentedAlso provide the custom code that exhibits the bug.
Comment #4
bklineSorry. I assumed when I gave the exception class (
TypeError) and the method wherearray_unshift()is invoked (FileValidator::validate()) you'd have enough information. It's no surprise that array_unshift() is unhappy when the first argument isn't an array. 😉Here's the exception:
[Mon Feb 12 06:57:09.773233 2024] [php:notice] [pid 788] [client 192.168.65.1:24832] Uncaught PHP Exception TypeError: "array_unshift(): Argument #1 ($array) must be of type array, string given" at /var/www/web/core/modules/file/src/Validation/FileValidator.php line 49, referer: http://ebms.localhost:8081/articles/import/27530
In this case, the apache logs don't get the full stack trace, so I've captured the call stack from the debugger. This is right before
array_unshift()is called with$optionsset to an empty string:and here is the call stack immediately after the exception is thrown:
Here's the custom code calling file_save_upload():
I'm not 100% certain you'll be able to open this GitHub URL, as our client has recently added an additional login requirement, which sometimes blocks access and sometimes doesn't (I haven't yet completely figured out the pattern):
https://github.com/NCIOCPL/ebms/blob/main/web/modules/custom/ebms_import...
Comment #5
bklineComment #7
kim.pepperWriting a test to reproduce this.
Comment #8
bklineLet me know if you're not able to get to the GitHub link I provided, and I'll come up with another way to provide you with our custom code.
Comment #10
kim.pepperAdded a fix to check if the validator
$optionsis an array in\Drupal\file\Validation\FileValidator::validate().We can do a test only build to make sure it's fixed.
Comment #11
kim.pepperSo the default MR pipeline passes and the test-only pipeline fails, so I think we have a fix.
Comment #12
bklineUsing your patch to avoid blowing up on the
array_unshift()call, I'm now getting "Only files with the following extensions are allowed: jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp." I see that your test uses a file with a name ending in ".txt" which is one of the extensions on the default list, whereas my test with an extension not on the list fails, meaning the advice in the documentation no longer works (that is, it doesn't achieve the goal of allowing any filename extension). What happens if your test uses a filename like "aaa.bbb"?Comment #13
kim.pepperLooks like there is more than one issue. I think this should fix the issue, but we need to add a test for the changed logic in
\Drupal\file\Upload\FileUploadHandler::handleExtensionValidation(). Not sure if that should be a separate issue?Comment #14
kim.pepperComment #15
kim.pepperI'd really like to sort this out in #3403253: Simplify how 'allow all extensions' file upload validation works
Comment #16
bklineI'll test in the morning (EST).
Comment #17
kim.pepperI added a test for
\Drupal\file\Upload\FileUploadHandler::handleExtensionValidation()but needed to use reflection in order to get full test coverage of a protected method.Comment #18
bklineConfirming that it works as documented again with the patch.
We understand that eventually (that is, before we get to D11) we need to replace
with
(or whatever the documentation for the replacement for the soon-to-be-deprecated
file_save_upload()function tells us to use).Thanks!
Comment #19
kim.pepperBumping this to major, given this effectively stops file uploads that 'allow all' extensions.
Comment #20
larowlanLeft some questions on the MR, thanks for jumping on this - and for testing @bkline@rksystems.com
Comment #22
kim.pepperTests are green and all feedback has been addressed.
Comment #23
kim.pepperComment #24
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #25
kim.pepperReview bot is complaining about a mysql file not included in the MR so disabling.
Comment #26
nod_yeah sometimes the bot picks up some stuff from someone's test issue or something and it causes problems. cleaned it up.
Comment #27
smustgrave commentedAppears all feedback has been addressed
But can MR be updated for 11.x
Comment #28
kim.pepperIt's a bug in 10.2.x so I assumed it should be against that?
Comment #29
smustgrave commentedWouldn't this issue appear in 11.x and 10.3 though?
Comment #31
kim.pepperYeah makes sense.
I first tried to change the target branch but that meant I needed to rebase on every change from 10.2.x to 11.x 😬
I then decided it would be better to create a new branch off 11.x and cherry pick the commits from
3420802-filesaveupload-broken-byonto3420802-11.x-filesaveupload-broken. Somehow it didn't get the change inFileValidatorso I manually made that change in another commit.I'm sure there is an easier way to do this. 😅
Comment #32
smustgrave commentedThanks for the quick response!
Comment #33
alexpottCommitted and pushed 4caea1c8e7 to 11.x and 7fa87dc411 to 10.3.x and f35b1b1e79 to 10.2.x. Thanks!
It's a shame that the new constraint supports an empty extension list. Also it feels even less explicit (and therefore easier to mistakenly dod) than the old way. Can we file a follow-up to deprecate configuring \Drupal\file\Plugin\Validation\Constraint\FileExtensionConstraint without extensions and implement a whole other constraint for the purpose of allowing any files. Not you can not (at least in past) configure a file field via the UI to accept any extension. The constraint should be called InsecureFileUpload or something that will make you think a lot before ever using.
Comment #37
kim.pepperThanks @alexpott. I started looking at how we could improve "unsafe mode" in #3403253: Simplify how 'allow all extensions' file upload validation works but didn't get too far.