Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#43751 new defect (bug)

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

Reported by: danielbachhuber Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch has-unit-tests
Focuses: multisite, rest-api Cc:


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 (1)

43751.diff (6.7 KB) - added by flixos90 2 months ago.

Download all attachments as: .zip

Change History (2)

2 months ago

#1 @flixos90
2 months 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.
Note: See TracTickets for help on using tickets.