nixos/nextcloud: fix nginx routing to store and nix apps #277382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ma27
requested review from
mweinelt,
RaitoBezarius,
bachp,
beardhatcode,
dotlambda,
SuperSandro2000 and
Kranzes
|
I'lltest this out this evening and get back to ye. |
|
I don't think we really want to backport this to 23.05 which supports ends literally in the next days. |
SuperSandro2000
approved these changes
Dec 29, 2023
|
I copied the Forms work as expected. |
Closes NixOS#277206 The bug mentioned above was a symptom of the issue fixed here: when opening the `forms` app which is installed via `extraApps` (or the app store) the site wouldn't work because `.mjs` files had the wrong Content-Type. The actual problem got fixed already[1], however this config was not used for stuff from `/nix-apps` & `/store-apps` which had their own location section with only a `root ;` statement. In fact, this setup isn't strictly supported by Nextcloud upstream[2], so to fix this for good, I decided to follow the upstream suggestion for app directories outside the server root, i.e. linking them back into the store path. This means that the module generates a new derivation now with * `services.nextcloud.package` linked into it via `lndir`. * under `nix-apps` is a symlink to the link farm containing all apps from `services.nextcloud.extraApps`. * under `store-apps` is a symlink to `/var/lib/nextcloud/store-apps`. Since this is only used in the NixOS module that also configures this location for imperatively installed apps, this seems an OK thing to do. Successfully tested the change on a productive Nextcloud 28.0.1 with several apps installed via `extraApps` (`forms`, `cospend`, `maps`, `user_saml` and a few more). [1] 292c74c [2] https://docs.nextcloud.com/server/28/admin_manual/apps_management.html#using-custom-app-directories
Ma27
force-pushed
the
fix-nextcloud-app-routing
branch
from
6f3bc01
to
bae5e65
Compare
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-23.05
git worktree add -d .worktree/backport-277382-to-release-23.05 origin/release-23.05
cd .worktree/backport-277382-to-release-23.05
git switch --create backport-277382-to-release-23.05
git cherry-pick -x bae5e6516269cc804974ef2d5f494d46f7a012c1 |
|
Successfully created backport PR for |
1 task
Ma27
added a commit
to Ma27/nixpkgs
that referenced
this pull request
Feb 10, 2024
PR NixOS#277382 didn't fix just an issue with .mjs files for the `forms` app, but an underlying, more problematic issue: for `/nix-apps` & `/store-apps`, the fcgi config for PHP and the block for assets were never reached. That meant that e.g. `/nix-apps/notes/lib/AppInfo/Application.php` returned the PHP source code as text/plain. Considering that there was never a fundamental change to how this config's structure, I'm pretty sure that the issue was pretty much there since the module exists. After consulting the NixOS security team we agreed that this is most likely harmless because you'd have to use private apps with secrets in the raw PHP code of said app. However, this is still problematic because one important assumption - that PHP code is never sent to the browser - is broken which is why we decided on not mentioning this impact in the previous PR from December 2023. To make sure that we don't regress our nginx config, I decided to add the reproducer which fails on 8bbbb22 as testcase to our integration tests.
13 tasks
github-actions bot
pushed a commit
that referenced
this pull request
Feb 21, 2024
PR #277382 didn't fix just an issue with .mjs files for the `forms` app, but an underlying, more problematic issue: for `/nix-apps` & `/store-apps`, the fcgi config for PHP and the block for assets were never reached. That meant that e.g. `/nix-apps/notes/lib/AppInfo/Application.php` returned the PHP source code as text/plain. Considering that there was never a fundamental change to how this config's structure, I'm pretty sure that the issue was pretty much there since the module exists. After consulting the NixOS security team we agreed that this is most likely harmless because you'd have to use private apps with secrets in the raw PHP code of said app. However, this is still problematic because one important assumption - that PHP code is never sent to the browser - is broken which is why we decided on not mentioning this impact in the previous PR from December 2023. To make sure that we don't regress our nginx config, I decided to add the reproducer which fails on 8bbbb22 as testcase to our integration tests. (cherry picked from commit 37d6961)
Septias
pushed a commit
to Septias/nixpkgs
that referenced
this pull request
Feb 21, 2024
PR NixOS#277382 didn't fix just an issue with .mjs files for the `forms` app, but an underlying, more problematic issue: for `/nix-apps` & `/store-apps`, the fcgi config for PHP and the block for assets were never reached. That meant that e.g. `/nix-apps/notes/lib/AppInfo/Application.php` returned the PHP source code as text/plain. Considering that there was never a fundamental change to how this config's structure, I'm pretty sure that the issue was pretty much there since the module exists. After consulting the NixOS security team we agreed that this is most likely harmless because you'd have to use private apps with secrets in the raw PHP code of said app. However, this is still problematic because one important assumption - that PHP code is never sent to the browser - is broken which is why we decided on not mentioning this impact in the previous PR from December 2023. To make sure that we don't regress our nginx config, I decided to add the reproducer which fails on 8bbbb22 as testcase to our integration tests.
superherointj
pushed a commit
to nixpkgs-friendly/nixpkgs-friendly
that referenced
this pull request
Feb 22, 2024
PR NixOS#277382 didn't fix just an issue with .mjs files for the `forms` app, but an underlying, more problematic issue: for `/nix-apps` & `/store-apps`, the fcgi config for PHP and the block for assets were never reached. That meant that e.g. `/nix-apps/notes/lib/AppInfo/Application.php` returned the PHP source code as text/plain. Considering that there was never a fundamental change to how this config's structure, I'm pretty sure that the issue was pretty much there since the module exists. After consulting the NixOS security team we agreed that this is most likely harmless because you'd have to use private apps with secrets in the raw PHP code of said app. However, this is still problematic because one important assumption - that PHP code is never sent to the browser - is broken which is why we decided on not mentioning this impact in the previous PR from December 2023. To make sure that we don't regress our nginx config, I decided to add the reproducer which fails on 8bbbb22 as testcase to our integration tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Closes #277206
The bug mentioned above was a symptom of the issue fixed here: when opening the
formsapp which is installed viaextraApps(or the app store) the site wouldn't work because.mjsfiles had the wrong Content-Type.The actual problem got fixed already[1], however this config was not used for stuff from
/nix-apps&/store-appswhich had their own location section with only aroot ;statement.In fact, this setup isn't strictly supported by Nextcloud upstream[2], so to fix this for good, I decided to follow the upstream suggestion for app directories outside the server root, i.e. linking them back into the store path.
This means that the module generates a new derivation now with
services.nextcloud.packagelinked into it vialndir.nix-appsis a symlink to the link farm containing all apps fromservices.nextcloud.extraApps.store-appsis a symlink to/var/lib/nextcloud/store-apps. Since this is only used in the NixOS module that also configures this location for imperatively installed apps, this seems an OK thing to do.Successfully tested the change on a productive Nextcloud 28.0.1 with several apps installed via
extraApps(forms,cospend,maps,user_samland a few more).[1] 292c74c
[2] https://docs.nextcloud.com/server/28/admin_manual/apps_management.html#using-custom-app-directories
cc @Silver-Golden for testing.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.