WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#51638 closed task (blessed) (fixed)

Add Site Health test for verifying the Authorization header works as expected

Reported by: TimothyBlynJacobs Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Site Health Keywords: has-patch commit has-screenshots
Focuses: rest-api Cc:

Description

Application Passwords utilizes the Authorization header to pass the Basic Authentication credentials. In some server configurations, the values sent in the Authorization header won't reach WordPress.

Because of this, we added the wp_populate_basic_auth_from_authorization_header() and the RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}] Mod Rewrite rule. This should account for the vast majority of failures.

This patch adds a test to Site Health to verify that the Authorization header is working as expected. If it isn't, we direct the user to the Permalinks screen which will regenerate their .htaccess file in case the rule was missing.

Attachments (1)

wp-api-generated.diff (1.0 KB) - added by garrett-eclipse 10 months ago.
Updated wp-api-generated.js

Download all attachments as: .zip

Change History (12)

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


10 months ago

  • Keywords has-patch added

#2 @TimothyBlynJacobs
10 months ago

Two changes of note.

  1. I introduced support for an async test to specifying a list of headers that get included in the request. This is so we can fill the Authorization header with a known value.
  2. I added support for a skip_cron entry that can be used to declare that an async test shouldn't be run as cron. This test really doesn't make sense in a loopback environment.

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


10 months ago

#4 @prbot
10 months ago

TimothyBJacobs commented on PR #665:

For reference:

https://i0.wp.com/user-images.githubusercontent.com/3460448/97336223-252e2500-1855-11eb-83aa-591c07d277d3.png
https://i0.wp.com/user-images.githubusercontent.com/3460448/97336316-40993000-1855-11eb-9f33-87c8cc939d26.png
https://i0.wp.com/user-images.githubusercontent.com/3460448/97336531-80f8ae00-1855-11eb-8781-90fc19b8eabb.png

#5 @Clorith
10 months ago

  • Keywords commit has-screenshots added

Looking good, and the copy update is also in place I see, let's get this in as well.

#6 @TimothyBlynJacobs
10 months ago

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

In 49334:

Site Health, App Passwords: Test if the Authorization header is populated correctly.

App Passwords rely on the Authorization header to transport the Basic Auth credentials. For Apache web servers, WordPress automatically includes a RewriteRule to populate the value for servers running in CGI or FastCGI that wouldn't ordinarily populate the value.

This tests if the header is being filled with the expected values. For Apache users, we direct the user to visit the Permalinks settings to flush their permalinks. For all other users, we direct them to a help document on developer.wordpress.org.

Props Clorith, marybaum, TimothyBlynJacobs.
Fixes #51638.

#7 @prbot
10 months ago

TimothyBJacobs commented on PR #665:

Fixed in 0187bbdd7e4554b8c95c22fc9801cb73bd9086f1.

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


10 months ago

@garrett-eclipse
10 months ago

Updated wp-api-generated.js

#9 @garrett-eclipse
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It appears the wp-api-generated.js was overlooked here as it's appearing in diffs off trunk after running grunt test. Uploaded wp-api-generated.diff to address.
Thread - https://wordpress.slack.com/archives/C02RQBWTW/p1603922117261300

#10 @SergeyBiryukov
10 months ago

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

In 49368:

REST API: Regenerate test fixtures after [49334].

Props garrett-eclipse.
Fixes #51638.

#11 @SergeyBiryukov
10 months ago

In 49370:

REST API: Remove accidentally duplicated key in test fixtures.

Follow-up to [49334], [49368].

See #51638.

Note: See TracTickets for help on using tickets.