Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 2 months ago

#11015 closed enhancement (wontfix)

Admin Ajax actions should pass relevant global variables

Reported by: filosofo Owned by: ryan
Priority: normal Milestone:
Component: JavaScript Version: 2.9
Severity: normal Keywords: admin-ajax has-patch
Cc: Focuses:

Description

The wp_ajax_* action hooks should pass the global $_REQUEST variable so that callbacks can reduce the number of global objects they have to be aware of and so that they can get the data in a state prior to being modified by anything else.

Attachments (1)

pass_request_vars.11015.diff (566 bytes ) - added by filosofo 17 years ago.

Download all attachments as: .zip

Change History (11)

#1 @filosofo
17 years ago

  • Milestone 3.02.9

Why not 2.9. It's simple and there are no backwards-compatibility issues. :-)

#2 follow-up: @azaozz
17 years ago

-1. Don't see the benefit in doing this. If you have a function that accepts data from different contexts, you would always want to filter what comes from $_REQUEST.

#3 in reply to: ↑ 2 @filosofo
17 years ago

Replying to azaozz:

-1. Don't see the benefit in doing this.

The benefit is

  • You don't have to access global objects in your callback
  • You can be agnostic about whether the "action" is from POST or GET.
  • Testing is more straightforward.
  • There's no harm.

This helps avoid having to check whether "action" is set in POST or GET, and it allows code reuse: I can use the same callback in different places.

#4 @westi
17 years ago

  • Milestone 2.9
  • Resolutionwontfix
  • Status newclosed

Strong -1

It should be obvious inside a function when you are accessing a superglobal.

Passing $_REQUEST as a argument hides the use of the superglobal in the function.

That makes it much easier to audit code and identify where user supplied data is/isnt being trusted.

This ticket was mentioned in PR #11100 on WordPress/wordpress-develop by @adamsilverstein.


6 months ago
#5

## Summary

Builds on #11015. Adds dimension validation to the sideload endpoint.

  • Adds validate_image_dimensions() private method to WP_REST_Attachments_Controller
  • Validates uploaded image dimensions against expected size constraints in the wp/v2/media/<id>/sideload endpoint
  • Moves wp_getimagesize() call earlier in sideload_item() to validate before metadata handling

### Validation rules:

  • 'original' size: must match original attachment dimensions exactly
  • 'full' and 'scaled' sizes: requires positive dimensions only
  • Regular sizes: dimensions must not exceed registered size maximums (with 1px tolerance for rounding differences)

## Test plan

  • [x] test_sideload_item_rejects_oversized_dimensions — uploads 640x480 image as thumbnail (150x150), expects 400 with rest_upload_dimension_mismatch
  • [x] test_sideload_item_accepts_valid_dimensions — uploads 50x50 image as thumbnail, expects 200

Corresponding Gutenberg PR: https://github.com/WordPress/gutenberg/pull/74903

🤖 Generated with Claude Code

@adamsilverstein commented on PR #11100:


6 months ago
#6

This worked well in my manual testing.

@adamsilverstein commented on PR #11100:


4 months ago
#7

Pushed c57a9752cc addressing @apermo's two suggestions (in_array() + dimension_exceeds_max() helper).

Re-merging trunk: skipped intentionally for now. Trunk currently has the entire client-side media feature removed (commit c863860ccf, "Media: Remove client-side media processing feature for now."), including the sideload endpoint this PR validates against. The feature is being re-introduced in #11324, which is still open. Merging trunk here today would just delete the endpoint this PR builds on, so it's better to wait for #11324 to land and then rebase. CI on this branch will look red until then for the same reason.

@westonruter commented on PR #11100:


3 months ago
#8

I fixed some PHPStan errors in 5634d1d. The remaining issues are addressed by the types added in https://github.com/WordPress/wordpress-develop/pull/12003.

@westonruter commented on PR #11100:


3 months ago
#9

The changes look good, but there aren't tests added. Are these still coming?

@adamsilverstein commented on PR #11100:


2 months ago
#10

The changes look good, but there aren't tests added. Are these still coming?

Added.

Note: See TracTickets for help on using tickets.