Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#47077 closed enhancement (fixed)

Add Support for REDIRECT_HTTP_AUTHORIZATION to REST Server

Reported by: dshanske's profile dshanske Owned by: timothyblynjacobs's profile 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 5 years ago.
47077.2.diff (2.8 KB) - added by cklosows 5 years ago.
47077.3.diff (2.9 KB) - added by TimothyBlynJacobs 5 years ago.

Download all attachments as: .zip

Change History (19)

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


5 years ago

#2 @TimothyBlynJacobs
5 years 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
5 years ago

#3 @dshanske
5 years 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.


5 years ago

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


5 years ago

#6 @TimothyBlynJacobs
5 years 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.


5 years ago

#8 @cklosows
5 years 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 5 years ago by cklosows (previous) (diff)

@cklosows
5 years ago

#9 @cklosows
5 years 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.


5 years ago
#10

#11 @TimothyBlynJacobs
5 years ago

Latest patch fixes minor CS issues.

#12 @TimothyBlynJacobs
5 years 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
5 years 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
5 years ago

  • Keywords reporter-feedback removed

#15 @dshanske
5 years 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
5 years 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.