WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#51723 closed task (blessed) (fixed)

App Passwords: Regenerate .htaccess for 5.6

Reported by: TimothyBlynJacobs Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Login and Registration Keywords: has-patch commit
Focuses: rest-api Cc:

Description

Application passwords introduced a new rule to .htaccess to handle the authorization header: https://github.com/WordPress/wordpress-develop/blob/5bad4e7f8d6cf67fbcea1e7c763cc41bceaa810b/src/wp-includes/class-wp-rewrite.php#L1512

We should make sure that on upgrade, the .htaccess file is regenerated.

I presume this would be done by adding an upgrade_560() function, but I'm not sure how the db version conditionals are chosen.

Attachments (2)

51723.diff (2.4 KB) - added by SergeyBiryukov 7 months ago.
51723.2.diff (773 bytes) - added by pbiron 7 months ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
7 months ago

Just bumping $wp_db_version to the current commit number is enough to trigger the flush_rewrite_rules() call in wp-admin/admin.php.

That's how it was done in [47018] to add the new favicon rewrite rules, no upgrade routine needed :)

flush_rewrite_rules() does a "hard" flush by default, which includes updating the .htaccess file.

Version 0, edited 7 months ago by SergeyBiryukov (next)

#2 @TimothyBlynJacobs
7 months ago

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

In 49534:

App Passwords: Bump database version to flush rewrite rules.

Application Passwords introduced a new Rewrite Rule to handle the Authorization header on certain systems. This bumps the database version so the change is applied to sites upon upgrading to 5.6.

Fixes #51723.

#3 @SergeyBiryukov
7 months ago

  • Keywords has-patch commit added; needs-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

It was pointed out by @pbiron in #core-restapi that the .htaccess file was not updated after [49534].

While WP_Rewrite::flush_rules() gets called after a DB version bump, it only calls save_mod_rewrite_rules() if the function exists, however the function is only loaded via wp-admin/includes/admin.php exactly after the flush_rewrite_rules() call in wp-admin/admin.php, so we do need an upgrade routine here.

For reference, it's been a while since we've added or updated .htaccess rules: [2635], [3401], [13676].

51723.diff works as expected in my testing.

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


7 months ago

@pbiron
7 months ago

#5 @pbiron
7 months ago

51723.2.diff is what I was working on as a solution: it simply does a require_once on the file where save_mod_rewrite_rules() is defined before calling flush_rewrite_rules().

This also works as expected in my testing, and is simpler :-) I can't see any downsides to it either, but maybe I'm missing something.

Last edited 7 months ago by pbiron (previous) (diff)

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


7 months ago

#7 @hellofromTonya
7 months ago

  • Keywords dev-feedback added; commit removed

#8 follow-up: @SergeyBiryukov
7 months ago

  • Keywords commit added; dev-feedback removed

To summarize a bit, 51723.diff regenerates the .htaccess file for this specific case, and 51723.2.diff would do it for any DB version bump, which may or may not be necessary.

It looks like the consensus in the pre-RC1 bug scrub was to go with 51723.diff for now and explore 51723.2.diff in a future release.

#9 in reply to: ↑ 8 ; follow-up: @pbiron
7 months ago

Replying to SergeyBiryukov:

To summarize a bit, 51723.diff regenerates the .htaccess file for this specific case, and 51723.2.diff would do it for any DB version bump, which may or may not be necessary.

It looks like the consensus in the pre-RC1 bug scrub was to go with 51723.diff for now and explore 51723.2.diff in a future release.

Yes, the consensus, with which I completely agree, is that this close to 5.6-RC1, the safest thing is to go with 51723.diff.

Here's the rationale (when/if we get around to exploring 51723.2.diff) I used when putting that patch together. In this slack thread, Sergey says:

Just bumping $wp_db_version to the current commit number is enough to trigger flush_rewrite_rules()

That's how it was done in [47018] to add the new favicon rewrite rules, no upgrade routine needed :)

But $wp_db_version is generally bumped for any rewrite rule updates like this. flush_rewrite_rules() does a "hard" flush by default, which includes updating the .htaccess file.

A few points:

  1. the change in [47018] (I now see, I hadn't look at it before) involved only changes to WP rewrite rules (and didn't need a hard flush)
  2. the "rewrite rule updates like this" actually do involve .htaccess changes
  3. since save_mod_rewrite_rules() is not defined at the time flush_rewrite_rules() is called when db_updated is checked, the flush there has never been a "hard" flush. Based on what sergey said in that thread, I figure that was "bug"...which is fixed by that patch.

So, when/if we come back to this in a future version, maybe there's a way we can indicate (in the value for the db_updated option) whether a "hard" flush is needed and do the require_once before calling flush_rewrite_rules() only in that case.

Make sense?

#10 in reply to: ↑ 9 @SergeyBiryukov
7 months ago

Replying to pbiron:

since save_mod_rewrite_rules() is not defined at the time flush_rewrite_rules() is called when db_updated is checked, the flush there has never been a "hard" flush. Based on what sergey said in that thread, I figure that was "bug"...which is fixed by that patch.

Yeah, I guess this was never noticed because .htaccess changes are rare, the last one was 11 years ago in [13676].

So, when/if we come back to this in a future version, maybe there's a way we can indicate (in the value for the db_updated option) whether a "hard" flush is needed and do the require_once before calling flush_rewrite_rules() only in that case.

I think 51723.2.diff should be fine as is even without the additional indication.

This could, however, use some better documentation. Created #51805 for that :)

#11 @SergeyBiryukov
7 months ago

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

In 49632:

App Passwords: Regenerate the .htaccess file to add a new rule.

Application Passwords introduced a new Rewrite Rule to handle the Authorization header on certain systems.

This bumps the database version and updates the file so the change is applied to sites upon upgrading to 5.6.

Follow-up to [49534].

Props pbiron, TimothyBlynJacobs, SergeyBiryukov.
Fixes #51723.

Note: See TracTickets for help on using tickets.