WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#12136 closed defect (bug) (fixed)

Improve MS rewrite rules

Reported by: nacin Owned by: wpmuguru
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch
Focuses: multisite 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 4 years ago.
12136.2.diff (921 bytes) - added by nacin 4 years ago.
Includes an optimization from Denis-de-Debernardy from #12497

Download all attachments as: .zip

Change History (17)

nacin4 years ago

comment:1 scribu4 years ago

  • Keywords has-patch added

comment:2 wpmuguru4 years ago

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.

comment:3 nacin4 years ago

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

comment:4 wpmuguru4 years ago

  • 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: nacin4 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 wpmuguru4 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.

comment:7 wpmuguru4 years ago

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

comment:8 nacin4 years ago

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

nacin4 years ago

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

comment:9 nacin4 years ago

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

comment:10 jamescollins4 years ago

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.

comment:11 nacin4 years ago

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.

comment:12 nacin4 years ago

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

comment:13 nacin4 years ago

(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

comment:14 wpmuguru4 years ago

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.

comment:15 wpmuguru4 years ago

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