WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 8 weeks ago

#52642 closed defect (bug) (fixed)

Site Health resets htpasswd authorization on scroll

Reported by: WebDragon Owned by: Clorith
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.6
Component: Site Health Keywords: good-first-bug has-patch
Focuses: rest-api Cc:

Description

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.

Attachments (1)

52642.diff (1.9 KB) - added by SergeyBiryukov 8 weeks ago.

Download all attachments as: .zip

Change History (16)

#1 @TimothyBlynJacobs
5 months ago

  • Focuses rest-api added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 5.6.2 to 5.6

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?

#2 @Clorith
5 months ago

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.

#3 @TimothyBlynJacobs
5 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Future Release to 5.8

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.

#4 @Clorith
3 months ago

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

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


2 months ago

This ticket was mentioned in PR #1320 on WordPress/wordpress-develop by costdev.


8 weeks ago

  • Keywords has-patch added; needs-patch removed

Add Authorization header test only when wp_is_site_protected_by_basic_auth returns false.

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

#7 follow-up: @prbot
8 weeks ago

Clorith commented on PR #1320:

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)

#8 @Clorith
8 weeks ago

  • Owner set to Clorith
  • Status changed from new to reviewing

#9 in reply to: ↑ 7 @costdev
8 weeks ago

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!

Last edited 8 weeks ago by costdev (previous) (diff)

#10 @Clorith
8 weeks ago

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

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
8 weeks ago

#11 @SergeyBiryukov
8 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

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.

#12 @costdev
8 weeks ago

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

#13 @Clorith
8 weeks ago

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.

#14 @SergeyBiryukov
8 weeks ago

Thanks for the confirmation! :)

#15 @SergeyBiryukov
8 weeks ago

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

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.

Note: See TracTickets for help on using tickets.