-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
Nginx rewrite rule in documentation is incorrect #665
Comments
Yeah, a simply wrong sample config might explain why we see support requests for nginx all the time 🙈 Apart from the regexes not being equivalent, can you elaborate why |
Sure!
So what The other issue with the existing rule is that it only catches paths that are .json .lock or .phar files, meaning that the .yml config files or .php plugin files or etc. can be just fetched directly. So, since (As a disclaimer, I would definitely not call myself an expert on nginx, just a long-time user, so there might be some other technical differences that aren't visible in practice, but everything I wrote here is accurate to the best of my knowledge.) |
Ok, wow, this definitely explains why this doesn't work 🙈 Thank you very much for your explanation! Helped a lot to understand what is going on. ❤️ So
|
I just tasked ChatGPT with this - and it failed miserably at first giving the instructions above, even after multiple correction loops 😆 However, tasking it with adapting the Apache config for nginx at least provided something that looks promising. However, I did many tests with ChatGPT and it often creates stuff that looks promising, but is very wrong in reality. Can you take a look? server {
listen 80;
server_name example.com;
root /var/www/example.com;
location / {
index index.php;
try_files $uri $uri/ /index.php?$args;
}
location ~ /(config|content|content-sample|lib|vendor)(/|$)|^(CHANGELOG\.md|composer\.(json|lock|phar))(/|$)|(^\.|/\.)(?!well-known(/|$)) {
rewrite ^(.*)$ /index.php last;
}
location ~ \.php$ {
include fastcgi_params;
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
fastcgi_pass unix:/run/php/php7.2-fpm.sock; # Replace with appropriate PHP version and path
}
} I don't know much about nginx, but from just comparing it with the original config and yours, it looks like |
Ahhh, I missed that this is how it works! I'm a lot weaker on how Pico works than I am on how NGINX works and the docs I saw all seemed to use the path as a query parameter, so that's what I used when trying to fix it. 😅 You are right that Aside from that, here's a revision of what chat GPT gave you: server {
listen 80;
server_name example.com;
root /var/www/example.com;
# this should be at the server level
index index.php;
# regex is not my strong point but i believe what you need is:
# (^/(config|content|content-sample|lib|vendor)/) - match locations starting with (i.e. in) these directories
# (^/(CHANGELOG\.md|composer\.(json|lock|phar))$) - match locations of these exact files
# (/\.well-known/) - match locations containing this string anywhere
# joined with | to match any of them
location ~ (^/(config|content|content-sample|lib|vendor)/)|(^/(CHANGELOG\.md|composer\.(json|lock|phar))$)|(/\.well-known/) {
# last means that it won't rewrite it again - it'll still get processed by fpm. it's necessary for situations where your rewrites might cause loops.
# so it's not necessary here but doesn't hurt
rewrite .* /index.php$is_args$args last;
}
# it doesn't matter as much here because regex locations get parsed separately
# but it's good practice to remember not to put your root location at the top so it doesn't override all your other locations
location / {
try_files $uri $uri/ /index.php$is_args$args;
}
# i copied this from my existing config
location ~ \.php$ {
include snippets/fastcgi-php.conf;
fastcgi_param SCRIPT_FILENAME $request_filename;
fastcgi_pass unix:/var/run/php/php7.4-fpm.sock; # Replace with appropriate PHP version and path
}
} |
All right, I tested this solution for the URI and it seems to work! server {
# the listen/server_name/etc directives
set $original_request_uri $request_uri;
# ...
location ~\.php$ {
include snippets/fastcgi-php.conf;
fastcgi_param SCRIPT_FILENAME $request_filename;
fastcgi_param REQUEST_URI $original_request_uri; # use original URI
fastcgi_pass unix:/var/run/php/php7.4-fpm.sock; # Replace with appropriate PHP version and path
} |
For conciseness, here's all of it in one place: server {
listen 80;
server_name example.com;
root /var/www/example.com;
index index.php;
set $original_request_uri $request_uri;
location ~ (^/(config|content|content-sample|lib|vendor)/)|(^/(CHANGELOG\.md|composer\.(json|lock|phar))$)|(/\.well-known/) {
rewrite .* /index.php$is_args$args last;
}
location / {
try_files $uri $uri/ /index.php$is_args$args;
}
location ~ \.php$ {
include snippets/fastcgi-php.conf;
fastcgi_param SCRIPT_FILENAME $request_filename;
fastcgi_param REQUEST_URI $original_request_uri;
fastcgi_pass unix:/var/run/php/php7.4-fpm.sock; # Replace with appropriate PHP version and path
}
} I've updated the regex, rewrite, and URI directives in my own configuration to match this and tried them out, and it's all working correctly. Normal pages show up normally, normal files show up normally (including a root |
Thanks again @InspectorCaracal! ❤️
I remember that there were some issues with specially crafted requests, requiring location ~ ^/((config|content|content-sample|lib|vendor|CHANGELOG\.md|composer\.(json|lock|phar))(/|$)|(.+/)?\.(?!well-known(/|$))) {
I guess we can make the regex even simpler (i.e. faster) with the following. Can you please give it a try? rewrite ^ /index.php$is_args$args last;
I guess appending a try_files $uri $uri/ /index.php$is_args$args =404;
A few more questions and notes about this section:
|
Last but not least: Do you want to open a PR to incorporate these changes on both our website (see |
I'll have to check, but I think the location block patterns are only used within NGINX itself and wouldn't affect the server PATH_INFO? I've never encountered patterns ending with (/|$) outside of this example, though, so it's not common practice.
That would break everything. 😅 Everything but the very last parameter in a As for the rest of the php-fpm block: a lot of those directives you're suspecting are needed are needed - but they're boilerplate that are included in the fastcgi snippets that come packaged with nginx. It's why I have the one line running (edit: ... now that I've said that, I'm not actually sure why I included the SCRIPT_FILENAME parameter manually, it's not necessary. I do a lot of "copy my config for something else and tweak it because I'm lazy" when setting up my own sites so I guess it's an artefact from someplace I did need it. 😓) (edit 2: Oh whoops, nope, updating my full config, I rediscovered why it's there - it screwed up my subdir site. I'll poke at it more to verify if it's something specific to my config or if it should be included in this sample as well before doing the PR.) Here is an example of what that file contains. As you can see, the tedious do-every-time work like checking if the file exists and handling the PATH_INFO is already covered. (Which I think resolves your concern about the other regex location block, too.) This file is shipped with nginx when you install it and has been part of its default installation for a long time - a decade or so, I think? So you can definitely count on it being there. And I'll be happy to do a PR! I'll get to that in the next couple days for sure. p.s. using ^ instead of .* in the rewrite works great 👍 |
Glad to see the Nginx config receiving some attention. I got curious about some things, so I went digging in the commit history. Apparently it's been SEVEN years since I wrote the initial draft for it. No wonder I can't remember anything about it anymore! Since I'm so out-of-touch with Nginx, I don't really have much to contribute here. Just wanted to share some quick findings. I looked at my own (very old and untouched) config. It just uses So, I was looking through the file history and I found this commit: picocms/picocms.github.io@8ccd661, where @PhrozenByte replaced large portions of the documentation. This is where So... uh... it's possible that the current config has never actually worked properly, since it sounds like, @PhrozenByte, you updated it purely by reading the docs and possibly never actually tested it? 🙈🫣 Anyway, thanks for all the help with this @InspectorCaracal. 😁 Since I wrote the original version of that page (long ago, on an enthusiastic Nginx-switching binge), I've felt pretty powerless, and a little guilty, seeing people struggle with Nginx more recently. I've lost pretty much all the knowledge I once had on the subject though. 😅 |
This comment was marked as outdated.
This comment was marked as outdated.
Fascinating... I had noticed myself that the current rule does not work very well. I have tested this snippet in a configuration that includes other directives as well, and it seems to be working perfectly (including the fastcgi block). Thanks for fixing! Out of curiosity: If I replace |
The currently given rule in the example config is:
As was mentioned before in #572 (which was closed while unresolved) this doesn't work. The rule should instead be something like this:
That will redirect any requests to those directories or their contents to instead be handled by Pico as content paths
The text was updated successfully, but these errors were encountered: