Opened 3 years ago

Closed 3 years ago

#12136 closed defect (bug) (fixed)

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 (2)

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

Download all attachments as: .zip

Change History (17)

nacin3 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?

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

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

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

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   wpmuguru3 years ago

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

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.

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

nacin3 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.

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