Site Health resets htpasswd authorization on scroll

[WebDragon]

Website is behind HTPasswd Basic Auth. Sign in.
Sign in to wordpress admin.
Navigate to Site Health tools page
Scroll down, and wait a bit.
HTPasswd asks for re-auth. Sign in again.
scroll down and wait a bit. Lather rinse repeat.

problem tested and confirmed on three separate sites after initial discovery.

[TimothyBlynJacobs]

Thanks for the ticket @WebDragon!

My inclination is that this is caused by the Site Health test that checks for whether the Authorization header is properly making it's way back to WordPress.

@clorith perhaps we should skip that test if wp_is_site_protected_by_basic_auth returns true?

[Clorith]

I agree @TimothyBlynJacobs, the question is if we should just remove the check altogether, like the HTTPS check if on a local setup, or let the test run, but add the check there to return a success state without doing the Authorization header check call?

I tend to lean towards the latter, that way it's possible for a user to look at the passed tests section and see it reliably.

[TimothyBlynJacobs]

Good call, I think you're right and we should skip performing the check but still return a valid result, maybe including some additional text that it wasn't directly performed.

[Clorith]

Having looked at how the test is set up, I actually will re-evaluate my previous suggestion, since the test sets headers to run at all, this will lead to more confusing logic than just outright skipping that test if not applicable.

With that knowledge in hand, we should instead do the same as with the REST API availability check, and only conditionally add it if wp_is_site_protected_by_basic_auth returns false

Add Authorization header test only when wp_is_site_protected_by_basic_auth returns false.

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

Thank you for the patch @costdev, this looks spot on for what is needed here!

I can't seem to find an associated WordPress.org user for you, would you mind leaving a comment on the relevant trac ticket as well, for the sake of us keeping tabs on who has worked on the ticket? (you can also ​link your GitHub account with your WordPress.org one from ​https://profiles.wordpress.org/me)

[costdev]

Hi @Clorith, coming out of the shadows and leaving a comment as requested!
Also linked my GitHub account with my WordPress.org one - thanks for the advice!

[Clorith]

In 51057:

Site Health: Conditionally run Authorization header test.

The test to confirm if Authorization headers can be used and recognized by WordPress needs to include a username and password combination that WordPress can compare against during the testing phase. The inclusion of credentials here would unfortunately also invalidate any existing basic auth session for the site, for example if the user had added this as an extra layer of security on their back-end.

This test is now skipped if the wp_is_site_protected_by_basic_auth() function detects that basic auth is being used, since the act of using basic auth to access the site confirms that this feature is working as expected in the first place.

Props WebDragon, TimothyBlynJacobs, costdev.
Fixes #52642.

[SergeyBiryukov]

wp_is_site_protected_by_basic_auth() is defined in wp-includes/load.php and is used without a function_exists() check in:

• wp-admin/authorize-application.php​
• wp-admin/user-edit.php​

Why do we check if it exists here?

Same goes for rest_url(), we use it without a function_exists() check right above in the same method​, and then for some reason check if it exists :)

It seems to me that these two checks are redundant, see 52642.diff​.

[costdev]

Good point @SergeyBiryukov and well spotted! The diff you attached looks good. How do we proceed?

[Clorith]

You're quire right @SergeyBiryukov, the rest_url check is a remnant of the Health Check plugin (which did not have REST endpoints back then) I believe, so contextually in that regard it made sense to have the same check around the wp_is_site_protected_by_basic_auth.

We should be able to safely just remove both those function_exists() checks here, as you mentioned.

[SergeyBiryukov]

Thanks for the confirmation! :)

[SergeyBiryukov]

In 51066:

Site Health: Remove unnecessary function_exists() checks from WP_Site_Health::get_tests().

By the time the tests run, both wp_is_site_protected_by_basic_auth() and rest_url() functions are available, so there is no need to check for their existence.

Follow-up to [44986], [51057].

Props Clorith, costdev, SergeyBiryukov.
Fixes #52642.