Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#45753 closed defect (bug) (fixed)

REST API: Allow header not set when OPTIONS request is made to a route that contains URL params

Reported by: noisysocks's profile noisysocks Owned by: kadamwhite's profile kadamwhite
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.0.2
Component: REST API Keywords: has-unit-tests has-patch needs-testing
Focuses: Cc:

Description

The REST API does not correctly set an Allow header when an OPTIONS request is made to a route that contains a URL param, e.g. /wp/v2/posts/(?P<id>[\d]+). This affects the posts and blocks routes.

Steps to reproduce:

  1. Create and publish a post. Note the ID of the post.
  2. As an admin, make an OPTIONS http://local.wordpress.test/wp-json/wp/v2/posts/123 request, replacing local.wordpress.test with the URL of your testing environment, and 123 with the ID you noted in (1).

Expected result:

The response headers should contain an Allow header:

Allow: GET, POST, PUT, PATCH, DELETE

Actual result:

The response is missing an Allow header.

Possible cause:

This is possibly to do with rest_handle_options_request not calling $request->set_url_params() unlike dispatch which correctly parses and sets URL parameters.

Attachments (3)

45753.1.diff (1.2 KB) - added by killua99 6 years ago.
WIP: Currently it does what the ticket said to do. On my TODO list are, the Unit Test and perhaps some minor tweaks. I would like to get some help with the authentication level on my local test I'm not able to do a Basic authentication.
45753.2.diff (2.7 KB) - added by killua99 6 years ago.
WIP: Patch with simple Unit Test. It doesn't test against a login user with capabilities edit_posts.
45753.3.diff (4.8 KB) - added by kadamwhite 6 years ago.
Add test assertion for logged-in user, mirror test to attachments controller, and run filters in tests instead of causing side-effects within rest_handle_options_request.

Download all attachments as: .zip

Change History (11)

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

#2 @desrosj
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.2

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


6 years ago

#4 @desrosj
6 years ago

  • Owner set to killua99
  • Status changed from new to assigned

@killua99 has agreed to take the first pass at a patch for this.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

@killua99
6 years ago

WIP: Currently it does what the ticket said to do. On my TODO list are, the Unit Test and perhaps some minor tweaks. I would like to get some help with the authentication level on my local test I'm not able to do a Basic authentication.

This ticket was mentioned in Slack in #core-restapi by killua99. View the logs.


6 years ago

@killua99
6 years ago

WIP: Patch with simple Unit Test. It doesn't test against a login user with capabilities edit_posts.

@kadamwhite
6 years ago

Add test assertion for logged-in user, mirror test to attachments controller, and run filters in tests instead of causing side-effects within rest_handle_options_request.

#7 @kadamwhite
6 years ago

  • Keywords has-unit-tests has-patch needs-testing added; needs-patch removed
  • Owner changed from killua99 to kadamwhite
  • Status changed from assigned to accepted

Thanks for the patch, @killua99 ! I've updated it with a test for a logged-in user, and duplicated it to another controller as a spot-check that the fix is applied at the base API level and not specifically to posts alone.

I noticed that our other extant tests for Allow headers (see test_allow_header_sent) were manually triggering the rest_post_dispatch filter in order to apply the desired Allow header. By adding a call to that filter to the unit tests — mirroring the behavior within the controller when an actual request is being dispatched — I was able to remove the call to rest_send_allow_header within rest_handle_options_request, eliminating a benign but unnecessary side-effect. The rest of the patch looks great as-is.

This could use one other pair of eyes and a manual test, then I think we can land it!

#8 @kadamwhite
6 years ago

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

In 44933:

REST API: Ensure "Allow" header is returned for OPTIONS requests.

This changeset ensures $request->set_url_params() is called while fulfilling OPTIONS requests, where previously it was skipped because OPTIONS requests short-circuit the logic in dispatch which handles this setup for other request methods. Omitting the URL parameters prevented the Allow header from being set.

Props killua99, noisysocks.
Fixes #45753.

Note: See TracTickets for help on using tickets.