Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#43751 closed defect (bug) (fixed)

REST API: Attachments controller should respect "Max upload file size" and "Site upload space" in multisite

Reported by: danielbachhuber's profile danielbachhuber Owned by: pento's profile pento
Milestone: 4.9.8 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch has-unit-tests fixed-major
Focuses: multisite, rest-api Cc:

Description

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

If an uploaded file fails either check, then an error should be returned and WordPress shouldn't save the file.

Related https://github.com/WordPress/gutenberg/issues/6154

Attachments (2)

43751.diff (6.7 KB) - added by flixos90 6 years ago.
43751.2.diff (6.7 KB) - added by danielbachhuber 6 years ago.

Download all attachments as: .zip

Change History (15)

@flixos90
6 years ago

#1 @flixos90
6 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

Working on this was quite terrible, but at least I was able to figure out the sources of the issue, and 43751.diff is "working". Thoughts:

  • The logic to check both these things with an upload is in check_upload_size().
  • This function is hooked into wp_handle_upload_prefilter, but not into wp_handle_sideload_prefilter. That is one problem, because depending on how you upload the file via the REST API, it may use the latter.
  • Related to that, in unit tests, we need to hook the function into yet another filter, wp_handle_mock_upload_filter because the REST API uses that with tests and $_FILES leveraged.
  • The worst part about all of this (and the second problem) is that check_upload_size() has a terrible condition that it may just wp_die() under certain circumstances. I added the condition of a REST_REQUEST in there, so that it no longer dies during a REST API request. However, since that constant cannot be set during tests, I included a hack for unit tests in the WP_REST_Attachments_Controller that set $_POST['html-upload'], just so that it doesn't die. I'd love for this to not be necessary, therefore I vote for reopening #42061 and introducing that function, if only for the sake of unit tests. If that gets available, this hack would no longer be necessary.

#2 @danielbachhuber
6 years ago

  • Summary changed from Attachments controller should respect "Max upload file size" and "Site upload space" in multisite to REST API: Attachments controller should respect "Max upload file size" and "Site upload space" in multisite

#3 @danielbachhuber
6 years ago

  • Milestone changed from 5.0 to 4.9.8
  • Owner set to pento
  • Status changed from new to assigned

Thanks for your work on this, @flixos90.

In 43751.2.diff, I've:

  • Internalized check_upload_size() to WP_REST_Attachments_Controller so we can avoid the legacy code paths.
  • Updated WP_REST_Attachments_Controller->check_upload_size() to return descriptive WP_Error objects instead of monkeying about with the $files data.
  • Updated the unit tests accordingly.

I think it'd be great to land this in 4.9.8.

#4 @pento
6 years ago

@danielbachhuber: Looks like the unit tests are failing as a result of [43437]. 🙂

#5 @danielbachhuber
6 years ago

@pento The tests pass locally for me. I think you forgot to run grunt build.

$ grunt patch:https://core.trac.wordpress.org/attachment/ticket/43751/43751.2.diff
$ grunt build
$ phpunit --group 43751 -c tests/phpunit/multisite.xml
Installing...
Installing network...
Running as multisite...
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.22 by Sebastian Bergmann and contributors.

....                                                                4 / 4 (100%)

Time: 3.01 seconds, Memory: 82.00MB

OK (4 tests, 16 assertions)

This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.


6 years ago

#7 @pento
6 years ago

🙃

#8 @pento
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43462:

REST API: Attachments controller should respect upload limits.

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

Props flixos90, danielbachhuber.
Fixes #43751.

#9 @pento
6 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for backport.

#10 @SergeyBiryukov
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43489:

REST API: Attachments controller should respect upload limits.

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

Props flixos90, danielbachhuber.
Merges [43462] to the 4.9 branch.
Fixes #43751.

#11 @pedromendonca
6 years ago

@pento shouldn't the variable in the string This file is too big. Files must be less than %s KB in size. be changed to %d to merge with already the existent This file is too big. Files must be less than %d KB in size. string in core?

[EDITED]
Nevermind @pento, just checked that the other strings were already changed in the trunk, it's all merged and consistent :)

Last edited 6 years ago by pedromendonca (previous) (diff)

#12 follow-up: @danielbachhuber
6 years ago

@pedromendonca Gave me a minor heart attack there.

#13 in reply to: ↑ 12 @pedromendonca
6 years ago

@danielbachhuber sorry :)

The current GlotPress includes the new beta strings, but still have the old deprecated ones. Later I've checked the current trunk code and everything is ok.

Last edited 6 years ago by pedromendonca (previous) (diff)
Note: See TracTickets for help on using tickets.