WordPress.org

Make WordPress Core

#47077 closed enhancement (fixed)

Add Support for REDIRECT_HTTP_AUTHORIZATION to REST Server

Reported by: dshanske Owned by: TimothyBlynJacobs
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: dev-feedback has-unit-tests has-patch
Focuses: Cc:

Description

In WP_REST_Server->get_headers it endeavors to get a subset of headers.

It pulls any header with HTTP, strips the HTTP, and adds it. It adds 'CONTENT_LENGTH', 'CONTENT_MD5', 'CONTENT_TYPE'

Suggesting for compatibility, it pull REDIRECT_HTTP_AUTHORIZATION and add it in as 'Authorization' if HTTP_AUTHORIZATION is not set.

https://github.com/WP-API/Basic-Auth/issues/35
https://github.com/WP-API/Basic-Auth/issues/1

This is mentioned as an issue in multiple auth plugins. In the interest of compatibility, this header should be available to the REST endpoint.

Attachments (3)

47077.diff (976 bytes) - added by dshanske 13 months ago.
47077.2.diff (2.8 KB) - added by cklosows 10 months ago.
47077.3.diff (2.9 KB) - added by TimothyBlynJacobs 10 months ago.

Download all attachments as: .zip

Change History (19)

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


17 months ago

#2 @TimothyBlynJacobs
17 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Version set to 4.4

This seems like a sensible change to me. Do you want to work on a patch @dshanske?

@dshanske
13 months ago

#3 @dshanske
13 months ago

  • Keywords dev-feedback added; needs-patch removed
  • Milestone changed from Future Release to 5.4

This should do it.

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


11 months ago

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


10 months ago

#6 @TimothyBlynJacobs
10 months ago

  • Keywords needs-unit-tests added

Thanks for the patch @dshanske! I think unit tests would be nice to have here.

Should we double check that HTTP_AUTHORIZATION is not set? Or would that be overkill do you think. Thinking if it is possible for the header value to be stomped on if both keys are set somehow.

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


10 months ago

#8 @cklosows
10 months ago

Based off the original request, I believe the REDIRECT_HTTP_AUTHORIZATION value should only be used if the HTTP_AUTHORIZATION value is not set, and maybe also if both values are present and HTTP_AUTHORIZATION is empty:

[...]if HTTP_AUTHORIZATION is not set.

It would be entirely possible for the REDIRECT_HTTP_AUTHORIZATION to stomp on the initial value.

Last edited 10 months ago by cklosows (previous) (diff)

@cklosows
10 months ago

#9 @cklosows
10 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Added the check for an empty HTTP_AUTHORIZATION header, and 4 unit tests around the two headers.

This ticket was mentioned in PR #151 on WordPress/wordpress-develop by TimothyBJacobs.


10 months ago

#11 @TimothyBlynJacobs
10 months ago

Latest patch fixes minor CS issues.

#12 @TimothyBlynJacobs
10 months ago

  • Keywords reporter-feedback has-patch added

@dshanske could you give an example of how this would fix the issue with the different auth plugins? The REST API Server does not pass the WP_REST_Request object to the rest_authentication_errors filter. So any auth plugin that relied upon the Authorization header would have to manually retrieve the header value anyways.

Is the idea that they would use rest_get_server()->get_headers( $_SERVER )['AUTHORIZATION'] instead of checking against $_SERVER themselves?

#13 @dshanske
10 months ago

For errors, reading most of the auth plugins, not just my own, authentication isn't done using the identified filter...it is usually done in determine_current_user and only the results would be passed to the filter.

And using a function called get_headers is where most people would start, rather than reading it themselves

#14 @dshanske
10 months ago

  • Keywords reporter-feedback removed

#15 @dshanske
10 months ago

To be clear, determine_current_user does not have access to the information either. I use get_headers in my token endpoint, which is built using the REST API.

#16 @TimothyBlynJacobs
10 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 47239:

REST API: Add support for the REDIRECT_HTTP_AUTHORIZATION header.

Previously the REST API did not account for server configurations where the Authorization header must be added using ModRewrite. This caused major DUX issues when trying to use custom authentication mechanisms.

Fixes #47077.
Props dshanske, cklosows.

Note: See TracTickets for help on using tickets.