[regression] file_save_upload does not properly handle extensions

Provide the exception message. You alluded to it but left it out. We may need a stack trace too.

Also provide the custom code that exhibits the bug.

Sorry. I assumed when I gave the exception class (TypeError) and the method where array_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 $options set 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():

          $validators = ['file_validate_extensions' => ''];
          $file = file_save_upload('file', $validators, FALSE, 0);

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

kim.pepper → made their first commit to this issue’s fork.

Writing a test to reproduce this.

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

Added a fix to check if the validator $options is an array in \Drupal\file\Validation\FileValidator::validate().

We can do a test only build to make sure it's fixed.

So the default MR pipeline passes and the test-only pipeline fails, so I think we have a fix.

Using 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"?

Looks 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?

I'd really like to sort this out in 📌 Simplify how 'allow all extensions' file upload validation works Active

I'll test in the morning (EST).

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

Confirming that it works as documented again with the patch.

We understand that eventually (that is, before we get to D11) we need to replace

          $validators = ['file_validate_extensions' => ''];

with

          $validators = ['FileExtension' => []];

(or whatever the documentation for the replacement for the soon-to-be-deprecated file_save_upload() function tells us to use).

Thanks!

Bumping this to major, given this effectively stops file uploads that 'allow all' extensions.

Left some questions on the MR, thanks for jumping on this - and for testing @bkline@rksystems.com

Tests are green and all feedback has been addressed.

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

Review bot is complaining about a mysql file not included in the MR so disabling.

yeah sometimes the bot picks up some stuff from someone's test issue or something and it causes problems. cleaned it up.

Appears all feedback has been addressed

But can MR be updated for 11.x

It's a bug in 10.2.x so I assumed it should be against that?

Wouldn't this issue appear in 11.x and 10.3 though?

Yeah 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-by onto 3420802-11.x-filesaveupload-broken. Somehow it didn't get the change in FileValidator so I manually made that change in another commit.

I'm sure there is an easier way to do this. 😅

Thanks for the quick response!

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

Thanks @alexpott. I started looking at how we could improve "unsafe mode" in 📌 Simplify how 'allow all extensions' file upload validation works Active but didn't get too far.

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