Make WordPress Core

Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#57752 closed enhancement (fixed)

Improve rest_(allowed|exposed)_cors_headers filters

Reported by: bor0's profile bor0 Owned by: kadamwhite's profile kadamwhite
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: REST API Keywords: has-patch has-unit-tests commit add-to-field-guide
Focuses: rest-api Cc:

Description

In a use case where there are multiple REST endpoints, but we only want to expose specific headers for specific endpoints, the filters rest_allowed_cors_headers and rest_exposed_cors_headers do not have any request in context and are quite limited in that regard.

It would've been ideal if we could write our code as such:

add_filter( 'rest_exposed_cors_headers', function( $headers, $request ) {
	if ( '/foo/bar' === $request->get_route() ) {
		$headers[] = 'X-RateLimit-Limit';
	}
	return $headers;
}, 10, 2 );

Attachments (1)

57752.patch (3.3 KB) - added by bor0 14 months ago.

Download all attachments as: .zip

Change History (19)

@bor0
14 months ago

#1 @bor0
14 months ago

  • Keywords has-patch added

#2 @SergeyBiryukov
14 months ago

  • Milestone changed from Awaiting Review to 6.3

#3 @rachelbaker
14 months ago

@bor0 Would you be able to use the rest_post_dispatch filter for your use case?

#4 @rachelbaker
14 months ago

  • Keywords reporter-feedback added
  • Owner set to rachelbaker
  • Status changed from new to accepted

#5 @bor0
14 months ago

  • Keywords reporter-feedback removed

Hi @rachelbaker,

We could certainly do the following:

add_filter( 'rest_post_dispatch', function( $result, $server, $request ) {
	$server->send_header( 'Access-Control-Allow-Headers', 'X-RateLimit-Limit' );
	return $result;
}, 10, 3 );

But the problem is that this will overwrite the existing headers set by WordPress WP_REST_Server::serve_request.

In addition, we cannot get the existing headers from $server as the function send_header merely wraps the header function and doesn't store these results anywhere, so we end up having to copy-paste:

add_filter( 'rest_post_dispatch', function( $result, $server, $request ) {
	$allow_headers = array(
		'Authorization',
		'X-WP-Nonce',
		'Content-Disposition',
		'Content-MD5',
		'Content-Type',
	);
	$allow_headers[] = 'X-RateLimit-Limit';
	$this->send_header( 'Access-Control-Allow-Headers', implode( ', ', $allow_headers ) );
	return $result;
}, 10, 3 );

#6 @oglekler
10 months ago

  • Keywords needs-testing added

#7 @spacedmonkey
10 months ago

@rachelbaker With beta 1 on the 27th, will this is be committed in time?

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


10 months ago

#9 @chaion07
10 months ago

  • Milestone changed from 6.3 to 6.4

Thanks @bor0 for reporting this. We reviewed this ticket during a recent bug-scrub session and based on the feedback we are updating the milestone to 6.4!

Props to @oglekler

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


10 months ago

#11 @kadamwhite
10 months ago

This approach makes sense to me, given @bor0's argument above about avoiding copy-paste code. I think we should have some tests around it, though -- writing up a basic one now.

This ticket was mentioned in PR #4740 on WordPress/wordpress-develop by kadamwhite.


10 months ago
#12

  • Keywords has-unit-tests added

Add unit tests to initial patch uploaded to ticket.

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

#13 @kadamwhite
10 months ago

  • Focuses rest-api added
  • Keywords needs-testing removed
  • Owner changed from rachelbaker to spacedmonkey
  • Status changed from accepted to assigned

https://github.com/WordPress/wordpress-develop/pull/4740 now contains a version of @bor0's patch with unit tests, which assert the second argument is present, is a request object, and matches the unique dispatched route. The tests were pushed first to validate in CI that the tests fail without the patch, then pass with it.

I'm comfortable moving forward with this one, though I think we're just missing the 6.3 cutoff.

@spacedmonkey assigning to you to take forward, if that's alright.

#14 @spacedmonkey
10 months ago

  • Keywords commit added
  • Milestone changed from 6.4 to 6.3
  • Owner changed from spacedmonkey to kadamwhite

This patch looks good to go. Marking as ready to commit and assigning to @kadamwhite

#15 @kadamwhite
10 months ago

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

In 56096:

REST API: Expose current $request object to cors_header filters in WP_REST_SERVER->serve_request().

Allows headers to be more easily set on a per-response basis when more or less security is needed on a specific route.

Props bor0, rachelbaker, spacedmonkey, chaion07, oglekler, SergeyBiryukov.
Fixes #57752.

#17 @johnbillion
10 months ago

In 56156:

REST API: Correct some filter docblocks.

See #57752

#18 @stevenlinx
10 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.