WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 weeks ago

#18852 accepted enhancement

Nginx rewrite rules

Reported by: johnbillion Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: minor Version: 3.3
Component: Rewrite Rules Keywords: has-patch needs-testing
Focuses: Cc:

Description (last modified by dd32)

On the Permalinks screen we show rewrite rules for IIS and for mod_rewrite on Apache. Nginx is getting pretty popular now, so we should think about showing Nginx rewrite rules on this screen.

Unfortunately WordPress can't write directly to Nginx's rewrite configuration, but we can show the required rules for convenience and we can allow plugins to filter them if necessary (in the samw way IIS and mod_rewrite rules can be filtered).

Attachments (3)

18852.patch (5.4 KB) - added by johnbillion 3 years ago.
18852.2.patch (5.5 KB) - added by johnbillion 3 years ago.
18852.3.patch (7.9 KB) - added by johnbillion 22 months ago.

Download all attachments as: .zip

Change History (41)

johnbillion3 years ago

comment:1 johnbillion3 years ago

  • Keywords has-patch added

First pass at a patch to see where it gets us.

got_url_rewrite() is a new function which is a wrapper for detecting if we've got any method enabled for URL rewrites (mod_rewrite, IIS7 with permalinks, or Nginx).

The suggested rewrite rules are what I'm running on a couple of sites and what seems to be the most popular form of rewrites for WordPress on Nginx.

Patch also introduces a new $is_nginx global.

Thoughts, suggestions & feedback welcome.

comment:3 dd323 years ago

  • Description modified (diff)

Those rules can be slimed down a bit, no need to check for the file-exists check as try_files handles that through it's priority queue:

location / {
    try_files $uri $uri/ /index.php$args;
}

ie. If File exists, Elseif, If it's a Directory, Else, Lets pass it to WordPress

johnbillion3 years ago

comment:4 johnbillion3 years ago

Updated patch. Slim down rewrite rules from dd32 and tweak permalink screen message.

comment:5 nacin22 months ago

  • Milestone changed from Awaiting Review to 3.5

index.php$is_args$args is best. $is_args is ? if there are $args, empty if not.

I think we should do this for 3.5. 18852.2.patch looks comprehensive; I like it.

comment:6 follow-up: devesine22 months ago

Unless I'm misreading, this looks like it only produces the rewrite rules for single-site installs. Multisite subdomain also generates this for mod_rewrite:

# uploaded files
RewriteRule ^files/(.+) wp-includes/ms-files.php?file=$1 [L]

and subdirectory generates this:

# uploaded files
RewriteRule ^([_0-9a-zA-Z-]+/)?files/(.+) wp-includes/ms-files.php?file=$2 [L]

# add a trailing slash to /wp-admin
RewriteRule ^([_0-9a-zA-Z-]+/)?wp-admin$ $1wp-admin/ [R=301,L]
# [...]
RewriteRule  ^[_0-9a-zA-Z-]+/(wp-(content|admin|includes).*) $1 [L]
RewriteRule  ^[_0-9a-zA-Z-]+/(.*\.php)$ $1 [L]

The codex nginx page has an example which includes this (which are materially equivalent to what we've been using in a number of production sites):

# Add trailing slash to */wp-admin requests.
rewrite /wp-admin$ $scheme://$host$uri/ permanent;

# Directives to send expires headers and turn off 404 error logging.
location ~* \.(js|css|png|jpg|jpeg|gif|ico)$ {
	expires 24h;
	log_not_found off;
}

# Pass uploaded files to wp-includes/ms-files.php.
rewrite /files/$ /index.php last;

if ($uri !~ wp-content/plugins) {
	rewrite /files/(.+)$ /wp-includes/ms-files.php?file=$1 last;
}

# Rewrite multisite '.../wp-.*' and '.../*.php'.
if (!-e $request_filename) {
	rewrite ^/[_0-9a-zA-Z-]+(/wp-.*) $1 last;
	rewrite ^/[_0-9a-zA-Z-]+.*(/wp-admin/.*\.php)$ $1 last;
	rewrite ^/[_0-9a-zA-Z-]+(/.*\.php)$ $1 last;
}

Should those multisite-specific rules be part of this patch as well?

comment:7 in reply to: ↑ 6 johnbillion22 months ago

  • Keywords needs-refresh added
  • Owner set to johnbillion
  • Status changed from new to accepted

Replying to devesine:

Unless I'm misreading, this looks like it only produces the rewrite rules for single-site installs.

Correct. I'll update the patch (it needs a refresh anyway).

comment:8 johnbillion22 months ago

Also, the nginx page on Codex is a mess.

johnbillion22 months ago

comment:9 johnbillion22 months ago

  • Keywords needs-testing added; needs-refresh removed

18852.3.patch is a tentatively updated patch (against r21157).

Included in the patch are single site rewrite rules which are displayed on the Permalinks screen, and Multisite rewrite rules which are displayed on the Network Setup screen.

The try_files directive (?) has been updated as per Nacin's suggestion.

The Multisite rules will need verifying for correctness as they are not the same as those found on the Codex page for nginx. Instead they more closely resemble the mod_rewrite and IIS rewrite rules for Multisite. I have just deployed these rewrite rules to an nginx site of mine, but I've yet to test them thoroughly.

location / {
	try_files $uri $uri/ /index.php$is_args$args;
}

# Uploaded files
rewrite ^/([_0-9a-zA-Z-]+/)?files/(.+) /wp-includes/ms-files.php?file=$2 last;

# Add trailing slash to /wp-admin
rewrite /wp-admin$ $scheme://$host$uri/ permanent;

if (!-e $request_filename) {
	rewrite ^/[_0-9a-zA-Z-]+(/wp-(content|admin|includes).*) $1 last;
	rewrite ^/[_0-9a-zA-Z-]+(/.*\.php)$ $1 last;
}

In addition, I don't believe the rewrite rules take into account single sites installs in subdirectories. I've not tested it. We may need to include the base subdirectory (eg. /foo for a site at example.com/foo at the start of each rewrite rule as nginx doesn't have a RewriteBase equivalent.

comment:10 mattrude22 months ago

  • Cc matt@… added

comment:11 devesine22 months ago

Multisite:

In the ms-files directive:

$nginx_config_file .= 'rewrite ^/' . ( $subdomain_install ? '' : '([_0-9a-zA-Z-]+/)?' ) . 'files/(.+) /wp-includes/ms-files.php?file=$2 last;' . "\n\n"; 

$2 should be $1 if $subdomain_install is true (since there isn't the subdomain regex capture group, then), something like like so:

$nginx_config_file .= 'rewrite ^/' . ( $subdomain_install ? '' : '([_0-9a-zA-Z-]+/)?' ) . 'files/(.+) /wp-includes/ms-files.php?file=' . ( $subdomain_install ? '$2' : '$1 ) . ' last;' . "\n\n"; 

Aside from that, these directives work correctly as far as I can tell for multisite installs.

Single site installed in a subdirectory

As you observed, these rules do not take into account a single site which is installed in a subdirectory (WP_SITEURL and WP_HOME both 'http://example.com/blog', or WP_SITEURL 'http://example.com/blog/wp' and WP_HOME 'http://example.com/blog'). Adding the base subdirectory to the location directive and the index.php part of try_files works fine:

	location /blog {
		try_files $uri $uri/ /blog/index.php$is_args$args;
	}

comment:12 devesine22 months ago

  • Cc justin@… added

comment:13 follow-up: johnkleinschmidt22 months ago

  • Cc jk@… added

This may be out of the scope of this ticket, but what about including external rewrite rules (eg those added via $wp_rewrite->add_external_rule or $wp_rewrite->add_rule) in the nginx configuration display? Right now those rewrite rules only make their way to .htaccess files... it would be great to include those rules for nginx users.

comment:14 in reply to: ↑ 13 nacin22 months ago

Replying to johnkleinschmidt:

This may be out of the scope of this ticket, but what about including external rewrite rules (eg those added via $wp_rewrite->add_external_rule or $wp_rewrite->add_rule) in the nginx configuration display? Right now those rewrite rules only make their way to .htaccess files... it would be great to include those rules for nginx users.

Are the regex syntaxes identical? Should be noted that A) these don't work for IIS either, and B) using external rewrite rules is very rare and should be used sparingly as it is.

comment:15 johnkleinschmidt22 months ago

I believe the regex syntaxes are identical; though there may be cases they are not. Though ideally there would be use cases for performance reasons where you would want to encapsulate a rewrite rule in a location. eg:

#Apache rewrite rule:
RewriteRule ^/news$ /blog/category/correspondents

#Nginx rewrite rule
rewrite ^/news$ /blog/category/correspondents last;

#Better performing nginx rewrite rule:
location ^~ /news {
    rewrite ^ /blog/category/correspondents last;
}

comment:17 devesine21 months ago

I've used that extension, and it does what it says on the tin with no apparent problem.

comment:18 brianlayman21 months ago

  • Cc Brian@… added

I think what is listed in this ticket is fine, but I want to raise awareness of the dangers of the try_files simply tossing all traffic that ends in .php over to be processed by fastcgi/whatever.
In some configurations a constructed url along the lines of :
http://example.com/wp-content/uploads/2012/1/1/notrealla.jpg/.php
will allow the file notrealla.jpg to be sent to the php engine for processing. In that way a php file can be uploaded as a .jpg and then executed.

That's described here: http://forum.nginx.org/read.php?2,124297,page=1

Also I haven't seen anyone reference the official pange nginx has describing configuration WordPress:
http://wiki.nginx.org/Wordpress

That config works quite well.

Version 0, edited 21 months ago by brianlayman (next)

comment:19 nacin19 months ago

  • Milestone changed from 3.5 to Future Release

There doesn't appear to be a consensus on this yet, and I'm weary of going forward especially given potential security gotchas.

I am thinking that we should solidify a Codex page (similar to how nginx has one for us) that is our definitive, official, suggested best practice for both single-site and multisite. Once we get there, we can go forward here.

I really, really want this, but I don't think it is something we can rush.

comment:20 rmccue17 months ago

+1, but we do need to be careful about this, since cgi.fix_pathinfo has been known to cause security issues.

comment:21 johnbillion17 months ago

Yeah as Nacin said above we definitely need to get a solid consensus in place on the Codex page before this goes in.

comment:22 dcowgill13 months ago

  • Cc dcowgill@… added

comment:23 sennza11 months ago

  • Cc bronson@… added

comment:24 retlehs10 months ago

  • Cc retlehs added

comment:25 mbijon10 months ago

  • Cc mike@… added

I think the importance of this has escalted since this ticket was opened in 2011.

Since Jeremy Felt made nginx easy & standard in VVV, https://github.com/10up/varying-vagrant-vagrants, I've been talking to more people about it & almost every major WP dev shop is running nginx. Most of us have our own configs & a patchwork of things we do to support it, so it might be time to put more engineering into making it more standard.

Worth noting that on Top 1,000 sites, nginx has surpassed Apache in popularity: http://w3techs.com/blog/entry/nginx_just_became_the_most_used_web_server_among_the_top_1000_websites

---

ADDITIONAL REQUIREMENTS?

Is it worth extending this ticket, or adding a separate one for WP_Rewrite's non_wp_rules()?

If we're displaying nginx rules in the UX, then that method should alert in some way too. Or, better, get refactored to write nginx rules too.

What if we implemented something like the $scheme parameter that handles HTTP/HTTPS URLs now? That would enable plugins to add more-extensive nginx rule support, without the need to debug & maintain this in core (yet).
(Plus, plugins have a faster|easier update cycle in the event new rules are needed to fix security issues)

comment:26 follow-up: jeremyfelt10 months ago

It would be great to see Nginx pushed a bit more alongside Apache in the WordPress workflow.

I do think a lot of the value will come from having a well vetted config available before WordPress is installed. Either as part of the Codex or, better, as a default conf file of sorts that can be moved on the server with instruction.

It seems somewhat more of a chicken/egg thing than Apache has been. Your ideal PHP-FPM configuration and basic handling of static assets will probably already be setup in order for you to get through the installation. Once installed, offering a more complete setup may be helpful, though I'd be interested in seeing examples of the different configs that would come out of that process.

The nginx-wp-common.conf that we've pieced together in VVV has served pretty well thus far, though I'm sure there's plenty of room for improvement. Ignoring the Vagrant/VM specific directives like gzip off;, it may be a good starting point for discussion and I'm willing to make tweaks to that default config as needed if it can provide a good testing ground for core.

My overall opinion is probably that core can help guide correctly on Nginx, but will probably never be able to automatically solve the things that the ease of .htaccess just working allows for.

From the perspective of solving annoyances, it would be nice to have the $is_nginx global so that WordPress can report its "got_rewrite" status properly without a plugin.

comment:27 johnbillion8 months ago

I've split out the support for URL rewrites in Nginx to #25098.

I'll be refreshing the patch for this ticket at some point over the next few days.

comment:28 johnbillion7 months ago

  • Milestone changed from Future Release to 3.8

Moving to 3.8 so we can get it in early in the cycle. Patch needs a refresh.

comment:29 miyauchi6 months ago

  • Cc miya@… added

comment:30 pothi6 months ago

  • Cc pothi added

comment:31 3flex5 months ago

  • Cc 3flex added

comment:32 in reply to: ↑ 26 ; follow-up: rmccue5 months ago

I'd like to start working on this again, since I think we can make some traction here.

Replying to jeremyfelt:

The nginx-wp-common.conf that we've pieced together in VVV has served pretty well thus far, though I'm sure there's plenty of room for improvement. Ignoring the Vagrant/VM specific directives like gzip off;, it may be a good starting point for discussion and I'm willing to make tweaks to that default config as needed if it can provide a good testing ground for core.

I think this is a great start, and matches the standard config I use too.

Going over individual bits of the config:

try_files $uri $uri/ /index.php?$args;

That should be /index.php$is_args$args instead ($is_args = "?" if $args is non-empty, or "" otherwise), per best practices.

# Add trailing slash to */wp-admin requests.
rewrite /wp-admin$ $scheme://$host$uri/ permanent;

Do we need this? :)

# Pass uploaded files to wp-includes/ms-files.php.
rewrite /files/$ /index.php last;

if ($uri !~ wp-content/plugins) {
    rewrite /files/(.+)$ /wp-includes/ms-files.php?file=$1 last;
}

# Rewrite multisite in a subdirectory '.../wp-.*' and '.../*.php'.
if (!-e $request_filename) {
    rewrite ^/[_0-9a-zA-Z-]+(/wp-.*) $1 last;
    rewrite ^/[_0-9a-zA-Z-]+.*(/wp-admin/.*\.php)$ $1 last;
    rewrite ^/[_0-9a-zA-Z-]+(/.*\.php)$ $1 last;
}

I'd separate this into an nginx-wp-multisite.conf personally.

    # The amount of time for upstream to wait for a fastcgi process to send data.
    # We keep this *extremely* high so that one can be lazy when remote debugging.
	fastcgi_read_timeout 3600s;

    # Buffer size for reading the header of the backend FastCGI process.
    # This defaults to the value of a single fastcgi_buffers, so does not
    # need to be specified in our case, but it's good to be explicit.
	fastcgi_buffer_size 128k;

    # The number and size of the buffers into which the reply from the FastCGI
    # process in the backend is read.
    #
    # 4 buffers at 128k means that any reply by FastCGI greater than 512k goes
    # to disk and replies under 512k are handled directly in memory.
    fastcgi_buffers 4 128k;

I think we should avoid duplicating fastcgi_params content where possible, and this stuff should definitely live there. (Also, interesting note, you've got mixed spaces and tabs in the file :) )

fastcgi_pass   php;

(I'm not sure how we can *not* hardcode this, so this is probably a good idea. Anyone using other FPM setups can obviously roll their own config.)

comment:33 follow-up: johnbillion5 months ago

Bear in mind we'll only be outputting rules that relate to rewrites (see 18852.3.patch for example). Any other config such as fastcgi-pass are not part of the rules that WordPress will generate.

I think @nacin was going to see if he could speak to a sysadmin at wordpress.com to review the rules in the most recent patch. Did you get anywhere with that nacin?

comment:34 in reply to: ↑ 33 rmccue5 months ago

Replying to johnbillion:

Bear in mind we'll only be outputting rules that relate to rewrites (see 18852.3.patch for example). Any other config such as fastcgi-pass are not part of the rules that WordPress will generate.

If we do that, what's actually needed? Just using index.php$is_args$args as the final try_files?

We might be better off pointing them to the Codex in that case. The text in attachment:18852.3.patch is a little misleading; on most systems, the location block should go into the site-specific config file rather than the global nginx.conf.

comment:35 nacin5 months ago

  • Milestone changed from 3.8 to Future Release
  • Severity changed from normal to minor

I think @nacin was going to see if he could speak to a sysadmin at wordpress.com to review the rules in the most recent patch. Did you get anywhere with that nacin?

I didn't get a chance to yet.

I think the try_files line + a link to the codex might be the most for what we want to do.

comment:36 in reply to: ↑ 32 jeremyfelt7 weeks ago

Replying to rmccue:

# Add trailing slash to */wp-admin requests.
rewrite /wp-admin$ $scheme://$host$uri/ permanent;

Do we need this? :)

It's good to have and I think I finally figured out why. :)

For unauthenticated users:

  • http://network.com/site1/wp-admin/ results in the proper redirect_to query var
  • http://network.com/site1/wp-admin drops site1 from the redirect_to query var

This is more annoying than anything else. Without digging too much, we currently do $path = str_replace ( '/wp-admin/', '/', $path ); in 3.8.1 ms-settings. Something similar is done in trunk via #27003.

Edit: Actually, some or most of that is wrong.

rewrite ^[_0-9a-zA-Z-]+.*/wp-admin$ $scheme://$host$uri/ permanent; would do the real job here.

Last edited 7 weeks ago by jeremyfelt (previous) (diff)

comment:37 follow-up: johnbillion7 weeks ago

It looks like it never got noted in this ticket, but the consensus after a couple of discussions on IRC was that displaying Nginx rewrites rules here would be potentially confusing for users, especially given that Nginx conf files can be (and usually are) split up depending on whether you've got virtual hosts configured, whether your WordPress configuration is in separate files, etc etc etc.

We should just display a link to the Nginx page on the Codex here in place of the .htacess rules when the site is running Nginx.

#25098 took care of the actual Nginx support.

comment:38 in reply to: ↑ 37 DrewAPicture7 weeks ago

Replying to johnbillion:

We should just display a link to the Nginx page on the Codex here in place of the .htacess rules when the site is running Nginx.

The Codex URL is http://codex.wordpress.org/Nginx

And if we ever do decide to display the rules here, we'll need to change the file header notes added in [26931] for the $is_ngninx global.

Note: See TracTickets for help on using tickets.