Skip to content

REST API: Remove the temporary file when a raw-body upload fails the size check. - #15134

Open
erycson wants to merge 3 commits into
WordPress:trunkfrom
erycson:fix/rest-upload-temp-file
Open

erycson wants to merge 3 commits into
WordPress:trunkfrom
erycson:fix/rest-upload-temp-file

Conversation

@erycson

@erycson erycson commented Oct 29, 2026

Copy link
Copy Markdown

WP_REST_Attachments_Controller::upload_from_data() writes the request body to a
file from wp_tempnam() before calling check_upload_size(). When that check
fails on multisite (file too big, not enough upload space, or user over quota),
the method returned the error without deleting the file, so every rejected
raw-body upload left a copy of its body in the temporary directory.

This deletes the file before returning, as the method already does when
wp_handle_sideload() fails and as the URL sideload path does after the same
check.

Adds test_create_item_with_data_exceeds_multisite_max_filesize_removes_temp_file()
(ms-required), which checks that no canola-*.tmp is left after a rejected upload.

Tested on a local 7.1.4 multisite (subdirectory): with fileupload_maxk = 1, a
5 KB raw-body upload returned 400 rest_upload_file_too_big and left one
.tmp (5120 bytes) in the temp dir; with the change, the same request leaves none.

Trac ticket: https://core.trac.wordpress.org/ticket/66409

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code, with code-intruder (my own static analysis tool)
Model(s): Claude Opus, Claude Haiku
Used for: code-intruder flagged the temporary file left on the error return and
an AI review triaged the finding. AI helped reproduce it on a local multisite
install (REST request, checking the temp dir before/after) and write the test.
I reviewed and tested the change myself.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…multisite size check.

`WP_REST_Attachments_Controller::upload_from_data()` writes the request body to a file from `wp_tempnam()` before calling `check_upload_size()`. When that check fails (file too big, not enough upload space, or user over quota), the method returned the error without deleting the file, so every rejected upload left a copy of its body in the temporary directory.

Delete the temporary file before returning, as the method already does when `wp_handle_sideload()` fails and as the URL sideload path does for the same check.

Fixes #66409.
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Core Committers: Use this line as a base for the props when committing in SVN:

Props extremsx, umeshnevase.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@umeshnevase

Copy link
Copy Markdown

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: #15134

Environment

  • WordPress: PHPUnit: 7.2-beta3-64914-src (trunk d49cecb + PR commit 74f05b7). Manual: 7.2-beta2-20261028.171134 build (multisite, subdirectory install), with the PR's controller file copied in.
  • PHP: 8.4.27
  • Server: nginx (Laravel Valet)
  • Database: MariaDB 11.7.2
  • Browser: N/A (REST API requests via curl)
  • OS: macOS (Darwin 25.6.0)
  • Theme: N/A
  • Plugins: None activated

Steps taken

1. PHPUnit

  1. Checked out the PR branch on top of trunk.
  2. Ran: WP_MULTISITE=1 phpunit -c tests/phpunit/multisite.xml --filter test_create_item_with_data_exceeds_multisite_max_filesize
  3. Reverted only the source change in class-wp-rest-attachments-controller.php (keeping the new test) and re-ran, to confirm the test fails without the fix.
  4. Restored the patch, re-ran the test, then ran the REST attachments controller tests.

2. Manual test (multisite)

  1. In Network Settings, set Max upload file size to 1 KB and enabled the upload-space check.
  2. Logged in as an administrator and sent POST /wp-json/wp/v2/media with the raw body of a ~198 KB JPEG (Content-Type: image/jpeg, Content-Disposition: attachment; filename=canola.jpg).
  3. Checked the PHP temp directory for leftover canola-*.tmp files.
  4. Repeated the request with the PR's class-wp-rest-attachments-controller.php in place.

Expected Results

When a raw-body REST upload is rejected by the multisite size check, the temporary file is removed.

Actual Results

  1. PHPUnit, without the patch: test_create_item_with_data_exceeds_multisite_max_filesize_removes_temp_file fails, because a leftover canola-*.tmp file remains after the rest_upload_file_too_big error.
  2. PHPUnit, with the patch: the test passes (2 tests, 9 assertions).
  3. PHPUnit, with the patch: the REST attachments controller tests pass (233 tests, 1268 assertions, 2 skipped), so no regressions were found.
  4. Manual, without the patch: the API returns rest_upload_file_too_big (400), but one new canola-*.tmp file is left in the temp dir.
  5. Manual, with the patch: the same 400 response is returned and no temp file is left behind.

Additional Notes

  • The code path only runs on multisite, because check_upload_size() returns early on single-site installs.
  • Not tested manually: the rest_upload_limited_space and rest_upload_user_quota_exceeded branches. They use the same early return, so the same change covers them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants