Ticket #12136 (closed defect (bug): fixed)

Opened 2 years ago

Last modified 22 months ago

Improve MS rewrite rules

Reported by: nacin Owned by: wpmuguru
Priority: normal Milestone: 3.0
Component: Rewrite Rules Version:
Severity: normal Keywords: multisite has-patch
Cc:

Description

I've been looking through the MS rewrite rules, trying to improve performance where possible and possibly coming up with a rule that prevents the need to hard-code wp-content (or, eventually, a custom content directory).

Patch attached what I've simplified so far, which is to reduce the uploaded files and wp-admin trailing slash rules each to a single line, and modify slug handling a bit. Tested on a production subdirectory install.

Attachments

12136.diff Download (1.1 KB) - added by nacin 2 years ago.
12136.2.diff Download (921 bytes) - added by nacin 2 years ago.
Includes an optimization from Denis-de-Debernardy from #12497

Change History

nacin2 years ago

  • Keywords has-patch added

The rewrite base is / or /path/ so in rewriting a request for /path/ron/wp-content/themes/... apache is running the regex on ron/wp-content/themes/... not /ron/wp-content/themes/....

The more specific regex limits the rewrites to what is possible as a blog name and probably provides a bit of security.

I don't have a problem with using the more specific regex instead of [/]+. Does the rest of it look good?

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

This one has been done, I believe. We had discussed in IRC & proceeded.

comment:5 follow-up: ↓ 6   nacin2 years ago

  • Status changed from closed to reopened
  • Resolution fixed deleted

None of these changes were applied. I think there is still some potential for optimization but otherwise we should close as wontfix.

comment:6 in reply to: ↑ 5   wpmuguru2 years ago

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

I did a bit of testing and changing the rewrite rules to those in the patch allowed me to access the admin area from a bogus path

ex.  http://domain.com/none-existant-path/wp-admin let me access the admin area at  http://domain.com/wp-admin.

Forgot to add - [13630] brought the allowed blognames in line with the rewrite rules.

  • Status changed from closed to reopened
  • Resolution wontfix deleted

Going to re-open this for some incremental improvements. The bogus path works on the current rewrite rules as well.

There are numerous improvements that seem okay to me. In order:

  1. Remove RewriteRule ^(.*/)?files/$ index.php [L] and change files/(.*) in the second rule to files/(.+).
  1. Remove the condition on the wp-admin trailing slash rule by improving the rule itself.
  1. Convert the files and wp-admin rules from ^(.*/)? to ^([_0-9a-zA-Z-]+/)?, to match the other rules and current restrictions.
  1. Remove the wp-content/plugins restriction on files, as the conditions for which it was added shouldn't apply under a properly constructed rule.

nacin2 years ago

Includes an optimization from Denis-de-Debernardy from #12497

This has a new patch. Also includes a small optimization from Denis in #12497 (closed as a duplicate).

The issue with wp-content/plugins is that we didn't restrict the initial files rewrite to only one directory deep. Thus it matched:

^blog/files/*
^blog/blah/files/*
^wp-content/plugins/plugin_name/files/*

And that's why we excluded wp-content/plugins.

The proper fix would have been to restrict the depth, via ^([^/]+/)?, which would only have matched the first example above. A desire to instead whitelist directories via _0-9a-zA-Z- also restricts it to a single directory as it prevents a slash, and thus rendering the wp-content/plugins check useless.

Food for thought: especially since .htaccess is generated dynamically, should we strip the directory matching when we're on a subdomain install? Seems like it would greatly simplify the rules (unless I'm mistaken, we'd end up with just ms-files and the standard WP rules).

Also, WPMU's current URL rewriting rules don't work with URLs that contain ".php". I raised this problem a few months ago ( http://trac.mu.wordpress.org/ticket/1167), but a fix hasn't been applied yet.

I had a go at trying to fix it (see the wpmu trac ticket above), but my mod_rewrite knowledge isn't quite up to scratch.

The 3.0 rules are improved over the MU rules, and only restrict .php files in the root directory. They still restrict any files in wp-admin/wp-includes/wp-content, but the file must not exist. This doesn't solve your problem, but I wanted to point it out.

What could solve the problem for subdomain installs would be to scrap subdirectory-specific rules when installing a subdomain-based network.

(In [13675]) Improve multisite rewrite rules. Construct rules dynamically and skip subdirectory-specific rules for subdomain installs. see #12136

(In [13679]) Don't allow access to network.php if running an MU network. It is only for networks created in 3.0. see #11816. Also, fix MS htaccess logic, see #12136

This rewrite rule should be removed to improve the site rewrite efficiency:

RewriteRule ^index\.php$ - [L]

Whether or not it improves the efficiency of a request for index.php, it decreases the efficiency of all other requests (wp-content, wp-admin, wp-includes, css, js, tags, categories, archives, attachments, feeds, comments, etc.). Requests for index.php comprise only a small number of the requests on any site.

  • Status changed from reopened to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.