Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop WP rewrite rules from matching files like wp-admin.css #1317

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

brandonpayton
Copy link
Member

What is this PR doing?

This PR fixes a bug in our WordPress rewrite rules where plugin files named things like "wp-admin.css" were having their preceding plugin-specific path removed.

This was reported in WP.org Slack (#playground-logs):
https://wordpress.slack.com/archives/C06Q5DCKZ3L/p1713585476733489

How is the problem addressed?

This PR tweaks the pattern to only apply to directories. This fixes the problem of matching files but still leaves open the possibility of false positives when plugins or themes contain directories named /wp-admin/, /wp-includes/, etc.

Testing Instructions

Without this fix, the appearence of the lefthand menu bar is obviously broken like:

@brandonpayton brandonpayton requested review from bgrgicak and a team April 24, 2024 16:30
@brandonpayton
Copy link
Member Author

It would be nice to get the quick fix in for @fatihbalsoy who reported the issue, but perhaps we can make a follow-up PR to further reduce the chances of false-positives.

@bgrgicak, do we know more about the expected paths that prefix wp-admin, wp-content, and others that we could use to make this pattern not accidentally match similarly named directories in plugins and themes?

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix @brandonpayton! Let's merge and deploy the site.

@brandonpayton brandonpayton merged commit 2d5a88f into trunk Apr 24, 2024
5 checks passed
@brandonpayton brandonpayton deleted the tighten-wp-rewrite-rule branch April 24, 2024 18:40
@bgrgicak
Copy link
Collaborator

@bgrgicak, do we know more about the expected paths that prefix wp-admin, wp-content, and others that we could use to make this pattern not accidentally match similarly named directories in plugins and themes?

Thanks for the fix! What you did makes sense, I just forgot to add the / and it in my testing it still worked.

@adamziel
Copy link
Collaborator

This fixes the problem of matching files but still leaves open the possibility of false positives when plugins or themes contain directories named /wp-admin/, /wp-includes/, etc

Would that also be a problem with a regular hosted WordPress?

@bgrgicak
Copy link
Collaborator

Would that also be a problem with a regular hosted WordPress?

I tested it on a new multisite and it worked, so this was just Playground bug.

@adamziel
Copy link
Collaborator

I didn't mean the thing this PR fixed. I meant what @brandonpayton mentioned about plugins containing /wp-admin/. If apache uses just the same regex as Playground, only shipped via .htaccess, it should have the same bug, right? In which case it's not ours to fix.

@brandonpayton
Copy link
Member Author

This fixes the problem of matching files but still leaves open the possibility of false positives when plugins or themes contain directories named /wp-admin/, /wp-includes/, etc

I think I was wrong about there being further possibility of false positives for plugins and themes. Both will already be under the wp-content directory which is already matched by the pattern.

/^\/(.*?)(\/wp-(content|admin|includes)\/.*)/g

And the pattern has other aspects which should make it safer for nested directories:

  • anchored to the beginning of the string
  • is non-greedy with the preceding path: .*?
  • ends with .* which consumes the rest of the string

I tested a bit with examples in the dev tools console, and it behaved well with paths like

/something/wp-content/plugins/any-plugin/wp-admin/stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] WordPress [Package][@wp-playground] WordPress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants