Update nginx.rst -- Reworked the Nginx configs

[jivanpal]

After inspecting the Nginx configs currently seen at [ https://docs.nextcloud.com/server/19/admin_manual/installation/nginx.html ], I have notice that they are very redundant in some areas, and handle specific paths in special ways which are not needed, and which run the risk of needing to become more specific down the road if NextCloud adds new features, as well as not handling all static filetypes. I have reworked the configs from scratch by directly adapting the .htaccess files, and this is the result. Comments are included in the file to give rationale/explanation for why things are written in this new way, and if the maintainers decide to accept this pull request, I will leave it up to them to decide whether to include these expository comments upstream.

[MorrisJobke]

cc @josh4trunks

[jivanpal]

Force-pushed to overwrite commit and make some minor spelling/formatting fixes

[josh4trunks]

Thanks for looking at this @jivanpal! If possibly, please dont force overwrite the commit, I had reviewed the previous version of the file and now I can't see what was changed. Next time, just push another commit on top so we get the whole messy history, lol. No worries we all have typos and need to change things.

[josh4trunks]

I agree with you that we should strive to match the .htaccess file as well as we can without sacrificing security/performance; that way we can more easily incorporate changes added to Nextcloud's codebase. I'll try adding my comments into the file, so its easier to know what I'm asking and you can respond when you have time.

[jivanpal]

@josh4trunks I've made some changes to address your comments, see commit message for details.

[josh4trunks]

If you don't mind giving me a few more day, I still want to test and propose any changes I think need to be made.
Haven't found much time this week, am moving to a new home so I am busy with that, but hopefully I can find some time by early next week.

[jivanpal]

No rush! I can only imagine how cumbersome moving house is in the middle of a pandemic 😅

[kesselb]

NextCloud => Nextcloud

[josh4trunks]

Haven't forgot about this, still on my list, just recovering from moving with a baby and dog, lol. I got my test server plugged in, and should have some time to look at this tomorrow night and will comment.

[jivanpal]

@kesselb, done 😉

[josh4trunks]

@jivanpal, I plan on testing an Apache configuration in the next few days and after that can resolve this comment #2197 (comment).

Do you mind responding to my 4 additional comments posted today? I think 3 of them can be settled very easily. This comment may take a little more back and forth to reach agreement since it affects more lines, but I'll do my best to respond quickly.

Thank you

[jivanpal]

Accidental review, please dismiss.

[jivanpal]

@josh4trunks, you said:

They are in lib/private/Setup.php and I believe are amended to .htaccess assuming the directory is writable. [...] At some point I think ownCloud (at the time) wouldn't even work with nginx unless you routed everything through index.php (except the files listed).

Regarding #5472, if the behaviour in Apache is to not cache, then I'm guessing that it is due to this app using its own .htaccess. Is this a common pattern in Nextcloud? If so, I agree with you saying that Nginx support will require routing everything through /index.php, unless we can standardise an Nginx include file, e.g. at /nginx-include.conf, that one can include in the main config, and that Nexcloud/Nextcloud apps can modify in the way that they currently use .htaccess files. Of course, this would require reloading Nginx after every config change. Proper Nginx support without this sort of hacky workaround would either require apps to not rely on modifying .htaccess rules, or rely on routing everything through /index.php.

Regarding the specific issue with Cache-Control headers there, do you know if Nginx overrides HTTP headers set by PHP? If so, we probably do need to get rid of the expires mentioned by @anoymouserver and instead rely on PHP to set the header, or do some sort of if on $http_cache_control.

[schleyk]

To me it looks like the only issue with the new Nginx config in Nextcloud 20 are the php files, which need routing via index.php. So, I would just add a one liner rewrite rule for those directly into the php location block.
E.g.

...
location~\.php(?:$|/) { # Required for legacy supportrewrite ^/(?!index|remote|public|cron|core\/ajax\/update|status|ocs\/v[12]|updater\/.+|oc[ms]-provider\/.+|.+\/richdocumentscode\/proxy) /index.php$request_uri;fastcgi_split_path_info ^(.+?\.php)(/.*)$;set$path_info$fastcgi_path_info;try_files$fastcgi_script_name =404;
...

This approach seems to work at least in my use case, and it can also be very simply reverted, when the legacy support isn't required anymore.

your rewrite rule fix my issue with the ldap app.

[jolly-jump]

To me it looks like the only issue with the new Nginx config in Nextcloud 20 are the php files, which need routing via index.php. So, I would just add a one liner rewrite rule for those directly into the php location block.
E.g.

...
location~\.php(?:$|/) { # Required for legacy supportrewrite ^/(?!index|remote|public|cron|core\/ajax\/update|status|ocs\/v[12]|updater\/.+|oc[ms]-provider\/.+|.+\/richdocumentscode\/proxy) /index.php$request_uri;fastcgi_split_path_info ^(.+?\.php)(/.*)$;set$path_info$fastcgi_path_info;try_files$fastcgi_script_name =404;
...

This approach seems to work at least in my use case, and it can also be very simply reverted, when the legacy support isn't required anymore.

your rewrite rule fix my issue with the ldap app.

I used this in my NC 20 which was upgraded from way below and then migrated to a new server where I initially put in the nginx-config that was given in the docs (two days ago), which did not work for LDAP

AND did not work for the rainloop addon!

Thanks for the fix. I'll try and tell the rainloop people as well.

[jolly-jump]

cross-referencing rainloop which suffers from the same issue as LDAP-config here
pierre-alain-b/rainloop-nextcloud#215

[etique57]

Also my nginx setup is itself behind a reverse proxy so I'm not sure if rewrite will work.

Hello @corsac-s I'm in the same configuration as you (/nextcloud/ path, reverse-proxy in front) and observes the same effect of the fix as you (general 404).

Did you find a solution?

[corsac-s]

Also my nginx setup is itself behind a reverse proxy so I'm not sure if rewrite will work.

Hello @corsac-s I'm in the same configuration as you (/nextcloud/ path, reverse-proxy in front) and observes the same effect of the fix as you (general 404).

Did you find a solution?

I kind of lost track of what I did and didn't do.

In the end, on my Apache reverse proxy, I use (using Unix sockets but I guess it should be the same with network):

ProxyPass / unix:/srv/cloud/cloud.sock|http://localhost/nextcloud/
ProxyPassReverse / unix:/srv/cloud/cloud.sock|http://localhost/nextcloud/

Then on the nginx part, I use:

 location ^~ /nextcloud {
# a whole lot of stuff there, then
# Rules borrowed from `.htaccess` to hide certain paths from clients
location ~ ^/nextcloud/(?:build|tests|config|lib|3rdparty|templates|data)(?:$|/) { return 404; }
location ~ ^/nextcloud/(?:\.|autotest|occ|issue|indie|db_|console) { return 404; }
# Ensure this block, which passes PHP files to the PHP process, is above the blocks
# which handle static assets (as seen below). If this block is not declared first,
# then Nginx will encounter an infinite rewriting loop when it prepends
# `/nextcloud/index.php` to the URI, resulting in a HTTP 500 error response.
location ~ \.php(?:$|/) {
fastcgi_split_path_info ^(.+?\.php)(/.*)$;
set $path_info $fastcgi_path_info;
try_files $fastcgi_script_name =404;
include fastcgi_params;
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
fastcgi_param PATH_INFO $path_info;
fastcgi_param HTTPS on;
fastcgi_param modHeadersAvailable true; # Avoid sending the security headers twice
fastcgi_param front_controller_active true; # Enable pretty urls
fastcgi_pass php-handler;
fastcgi_intercept_errors on;
fastcgi_request_buffering off;
}
}

Hope that helps.

[jivanpal]

@etique57@corsac-s — The solution should be to change the rewrite rule to account for the subpath/subdir you're using, e.g. if your site is at www.example.com/nextcloud, then change rewrite ^/(?!index... to rewrite ^/nextcloud/(?!index....

I am not sure whether this warrants a PR to modify the default Nginx config, given that the behaviour of the LDAP app relies on modifying Apache .htaccess files in situ. IMO, designing apps so that they don't rely on that would be better. Alternatively, allowing apps to specify Nginx rules in a file to be included via the main config might be acceptable, but that would then rely on app maintainers to provide the rules they need for both Apache and Nginx.

[etique57]

Thanks to both of you. I managed to get it working!

[ChristophWurst]

This approach seems to work at least in my use case, and it can also be very simply reverted, when the legacy support isn't required anymore.

Thanks a lot @jlehtoranta. I could verify that the current config doesn't work with the LDAP wizard and that your config revives the legacy routes. Could you open this as pull request? Otherwise I will do so if nobody sees any other issues with this config. I suppose those who used this trick still use the config in production, right? 😁

[ChristophWurst]

PR submit at #7141.

[KoMa1012]

To me it looks like the only issue with the new Nginx config in Nextcloud 20 are the php files, which need routing via index.php. So, I would just add a one liner rewrite rule for those directly into the php location block.
E.g.

...
location~\.php(?:$|/) { # Required for legacy supportrewrite ^/(?!index|remote|public|cron|core\/ajax\/update|status|ocs\/v[12]|updater\/.+|oc[ms]-provider\/.+|.+\/richdocumentscode\/proxy) /index.php$request_uri;fastcgi_split_path_info ^(.+?\.php)(/.*)$;set$path_info$fastcgi_path_info;try_files$fastcgi_script_name =404;
...

This approach seems to work at least in my use case, and it can also be very simply reverted, when the legacy support isn't required anymore.

your rewrite rule fix my issue with the ldap app.

I used this in my NC 20 which was upgraded from way below and then migrated to a new server where I initially put in the nginx-config that was given in the docs (two days ago), which did not work for LDAP

AND did not work for the rainloop addon!

Thanks for the fix. I'll try and tell the rainloop people as well.

this does not work for me, nextcloud 23 I get:
nginx: [emerg] duplicate upstream "php-handler" in /config/nginx/site-confs/default.orig:1