Skip to content
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

nextcloud: Empty Files on chunked uploads #252980

Open
jzbor opened this issue Sep 2, 2023 · 9 comments
Open

nextcloud: Empty Files on chunked uploads #252980

jzbor opened this issue Sep 2, 2023 · 9 comments
Labels
0.kind: bug Something is broken

Comments

@jzbor
Copy link
Contributor

jzbor commented Sep 2, 2023

Describe the bug

I own an Android Tablet with Nextcloud Sync supposedly supported by the OEM. However files that are uploaded are empty (0 bytes in size). It seems to be an issue with Nextcloud, as described in the issue below.

One user suggests enabling fastcgi_request_buffering. I am not sure if this causes any unknown regressions, but I have not been able to configure Nextcloud to test it. My suggestion is to either expose the option via the module for users to change or evaluate changing it in nixpkgs.

Upstream issue: #7995

Steps To Reproduce

Although I have not tested it with curl myself other Nextcloud users seem to have been able to reproduce the issue with this command:

curl -v -X PUT --header "Transfer-Encoding: chunked" -d @testfile.bin "http://ubuntu.local/remote.php/webdav/testfile.bin" -u admin:admin

Expected behavior

Nextcloud should work with chunked uploads.

Notify maintainers

@schneefux @bachp @globin @Ma27

Metadata

- system: `"aarch64-linux"`
 - host os: `Linux 6.1.47, NixOS, 23.11 (Tapir), 23.11.20230822.a2eca34`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.17.0`
 - nixpkgs: `/etc/channels/nixpkgs`
@jzbor jzbor added the 0.kind: bug Something is broken label Sep 2, 2023
@Ma27
Copy link
Member

Ma27 commented Sep 4, 2023

Issue is reproducible and indeed turning on request buffering fixes the problem. I'll do some more thorough research, then we'll see if it should be optional or not.

@Ma27
Copy link
Member

Ma27 commented Sep 16, 2023

OK I just realized that turning on fastcgi buffering doesn't necessarily fix the issue.

First of all, let me post the diff of what I have locally:

diff --git a/nixos/modules/services/web-apps/nextcloud.nix b/nixos/modules/services/web-apps/nextcloud.nix
index e0a7e7d4859c..42a75d1709bb 100644
--- a/nixos/modules/services/web-apps/nextcloud.nix
+++ b/nixos/modules/services/web-apps/nextcloud.nix
@@ -714,6 +714,24 @@ in {
           directive and header.
         '';
       };
+      enableFastcgiRequestBuffering = mkOption {
+        type = types.bool;
+        default = false;
+        apply = x: if x then "on" else "off";
+        description = mdDoc ''
+          Whether to buffer requests against fastcgi requests. This is a workaround
+          for `PUT` requests with the `Transfer-Encoding: chunked` header set and
+          an unspecified `Content-Length`. Without request buffering for these requests,
+          Nextcloud will create files with zero bytes length as described in
+          [nextcloud/server#7995](https://github.com/nextcloud/server/issues/7995).
+
+          ::: {.note}
+            Please keep in mind that upstream suggests to not enable this as it might
+            lead to timeouts on large files being uploaded as described in the
+            [administrator manual](https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/big_file_upload_configuration.html#nginx).
+          :::
+        '';
+      };
     };
   };
 
@@ -1184,7 +1202,7 @@ in {
               fastcgi_param front_controller_active true;
               fastcgi_pass unix:${fpm.socket};
               fastcgi_intercept_errors on;
-              fastcgi_request_buffering off;
+              fastcgi_request_buffering ${cfg.nginx.enableFastcgiRequestBuffering};
               fastcgi_read_timeout ${builtins.toString cfg.fastcgiTimeout}s;
             '';
           };
diff --git a/nixos/tests/nextcloud/basic.nix b/nixos/tests/nextcloud/basic.nix
index b7af6d6d7364..bf9e3fc11551 100644
--- a/nixos/tests/nextcloud/basic.nix
+++ b/nixos/tests/nextcloud/basic.nix
@@ -49,6 +49,7 @@ in {
           adminpassFile = "${pkgs.writeText "adminpass" adminpass}"; # Don't try this at home!
           dbtableprefix = "nixos_";
         };
+        nginx.enableFastcgiRequestBuffering = true;
         package = pkgs.${"nextcloud" + (toString nextcloudVersion)};
         autoUpdateApps = {
           enable = true;
@@ -116,5 +117,15 @@ in {
     )
     assert "hi" in client.succeed("cat /mnt/dav/test-shared-file")
     nextcloud.succeed("grep -vE '^HBEGIN:oc_encryption_module' /var/lib/nextcloud-data/data/root/files/test-shared-file")
+
+    with subtest("Create non-empty files with Transfer-Encoding: chunked"):
+        client.succeed(
+          'dd if=/dev/urandom of=testfile.bin bs=1M count=10',
+          'curl --fail -v -X PUT --header "Transfer-Encoding: chunked" -d @testfile.bin "http://nextcloud/remote.php/webdav/testfile.bin" -u ${adminuser}:${adminpass}',
+        )
+
+        nextcloud.succeed('test "$(stat --printf=%s /var/lib/nextcloud-data/data/root/files/testfile.bin)" -ne 0')
+        nextcloud.succeed("ls -lah >&2 /var/lib/nextcloud-data/data/root/files/testfile.bin")
+        client.succeed("ls -lah testfile.bin >&2")
   '';
 })) args

When you execute that, you'll realize that the local file has a size of 10MB (10x 1MB from /dev/urandom), but the remote file only has 5MB (and the hashes from nix-hash - which I used to fingerprint both files' contents - also don't match).

So, this is not a proper solution, rather a mitigation for a subset of the files.

From what I've read in https://docs.cyberduck.io/mountainduck/issues/fastcgi/ & https://sabre.io/dav/clients/finder/ this seems to be a bug (or actually an unimplemented feature) in php-fpm.

I don't want to advertise enabling fastcgi_request_buffering as solution here because apparently it doesn't work properly (please double-check perhaps I'm missing something).

IMHO the proper solution is to make nextcloud return a 411 in this case (which should be fixed upstream).

Do the other maintainers agree with that?

@NateWright
Copy link
Contributor

Hi, can we add this as an option?
I am having a similar issue between Nextcloud and Photoprism. This is a know issue and is resolved with fastcgi_buffering. There docs point to this issue as the fix.
https://docs.photoprism.app/user-guide/settings/sync/
photoprism/photoprism#443

@Ma27
Copy link
Member

Ma27 commented Apr 2, 2024

Which option specifically?
The Transfer-Encoding: chuked doesn't seem to be a proper solution at least.

Correction (which was made after the next comment): the fastcgi_request_buffering thing doesn't seem to be a proper solution.

@NateWright
Copy link
Contributor

I am going to try fastcgi_request_buffering = on and then try chunked_transfer_encoding on; if it does not work.

@Ma27
Copy link
Member

Ma27 commented Apr 2, 2024

Ehh sorry, I was confused. I meant precisely this, see my VM test from above.

@NateWright
Copy link
Contributor

I did some testing and both fastcgi_request_buffering = on and chunked_transfer_encoding on; are required for photoprism to successfully send files to nextcloud over webdav

@Ma27
Copy link
Member

Ma27 commented Apr 6, 2024

Feel free to re-use the patch I have above. However, this is not a universal solution apparently, sometimes this still causes corrupted data which is why I don't want to add it to nixpkgs.

@SuperSandro2000
Copy link
Member

the nextcloud docs say this https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/big_file_upload_configuration.html#nginx about fastcgi_request_buffering and I could only find nextcloud/server#7993 about chunked_transfer_encoding.

There seems to be issues with fastcgi_request_buffering, too nextcloud/server#7995 but they set it for apache somewhat recently nextcloud/server#45035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants