jesmrec
commented
2 months ago I have uploaded the text file with my server to test
thanks! but, your server is not reachable from our side :(
Closed geobzmm closed 1 month ago
jesmrec
commented
2 months ago I have uploaded the text file with my server to test
thanks! but, your server is not reachable from our side :(
appiekap653
commented
2 months ago Oh wait, I need to change the cloudflare firewall to allow other countries...
Never thought about it.
appiekap653
commented
2 months ago Should work now
jesmrec
commented
2 months ago Well, i can state:
Accept-Encoding does not have to do, at least directly. The identity value should be aligned but does not cause the downloaded error
The lack of Content-length is the main responsible, because the transfer is consolidated after checking the condition
where totalToTransfer is directly get from the response header. 0 value in that header breaks the condition and leads the logic to error. This is because this, as @appiekap653 correctly discovered (good job!! š )
@TheOneRing @felix-schwarz do you do some control over Content-length to consolidate downloads.
jvillafanez
commented
2 months ago According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding , Transfer-Encoding: chunked without Content-Length is expected, but oCIS isn't sending the Transfer-Encoding header, so someone else should be including that header.
I don't know anything about cloudfare, but maybe https://github.com/cloudflare/workers-rs/issues/511 is related? (not linking it because I don't want to bring attention if it's wrong)
appiekap653
commented
2 months ago I think this problem is mostly on the Traefik side.
Since you are sending Accept-Encoding identity, the response should not get compressed.
Somehow Traefik is still compressing the response and omits the Content-Length header and instead sends a Transfer-Encoding: chunked header.
When I leave the Compress Middleware in place but add an exclude for the mime type of the file we try to download, it also works without problems.
In my case:
compress-owncloud:
compress:
excludedContentTypes:
- image/jpegWhat Owncloud could change is to add the ability to download Binary responses in chunks and therefore not error out when chunked responses are given.
What Traefik should do is adhere to the identity header and don't compress accordingly.
appiekap653
commented
2 months ago According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding ,
Transfer-Encoding: chunkedwithoutContent-Lengthis expected, but oCIS isn't sending theTransfer-Encodingheader, so someone else should be including that header.I don't know anything about cloudfare, but maybe
https://github.com/cloudflare/workers-rs/issues/511is related? (not linking it because I don't want to bring attention if it's wrong)
I have also tried it with cloudflare proxy disabled, to be sure it wasn't Cloudflare that created the issue. Unfortunately it had the same results...
appiekap653
commented
2 months ago I have searched the Traefik code base, but so far, it seems that Traefik is adhering to the identity header and doesn't compress it accordingly.
Could it be that since the request is traveling like this:
Client -> Traefik -> Ocis proxy -> Ocis storage service -> Ocis Proxy -> Traefik -> Client
The proxy in Ocis is removing something in the headers. What makes Traefik compress the response?
jesmrec
commented
2 months ago thanks for the research again @appiekap653 . Trying to involve more people here because the problem goes beyond the Android app, as you discovered by yourself ;)
dragotin
commented
1 month ago @jvillafanez @2403905 could you try to get to a solution here? This is already dangling for long and blocking the Android team. Thx.
jesmrec
commented
1 month ago We did some checks. With only Traefik, the Content-Length header is OK (@jvillafanez), but as we noticed in your server @appiekap653, you use Cloudflare as well. You stated that with the disabled Cloudflare, it doesn't work either. Did you do any try with anything in the middle? i mean, straight from server to client. Just trying to figure out... it's a little crazy this.
The point is, any kind of middleware is killing the header out, that we need in advance to handle the transfer. The header departs from its origin. We'll do same PoC as well.
Any more ideas and thoughts from anyone will be appreciated.
jvillafanez
commented
1 month ago It's a wild theory, but maybe what happens is:
Accept-Encoding: identity to CloudflareAccept-Encoding: br, gzip to Treafik (source https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#accept-encoding)Transfer-Encoding: chunked header and without content-length. For possible reasons:
Since we haven't been able to reproduce the issue so far, I think that "something" is causing Cloudflare to return that kind of response. I don't have access to Cloudflare, so my hands are tied.
As for the Android client, I guess it's being picky with the header in order to have a minor guarantee that the download is fine.
What is clear is that the Content-Length header isn't being sent to the client somehow, and the Android client is having problems because of that missing header. The problem is that we don't know under what conditions the Content-Length header isn't sent.
peterjpierre
commented
1 month ago @linkp @chaotix- @geobzmm I also use only php-fpm instances and had the same problem. by adding the line
SetEnv ap_trust_cgilike_clto the.htaccessfile, my problem was solved.
I am running Owncloud on cPanel and tried adding SetEnv ap_trust_cgilike_cl to the .htaccess but then the error comes Sync failed due to malformed server configuration :(
appiekap653
commented
1 month ago With or without Cloudflare proxy the same problem happens.
Did you do any try with anything in the middle? i mean, straight from server to client. Just trying to figure out... it's a little crazy this.
The only thing in the middle is Traefik.
Since we haven't been able to reproduce the issue so far, I think that "something" is causing Cloudflare to return that kind of response. I don't have access to Cloudflare, so my hands are tied.
Please forget about Cloudflare, with or without cloudflare proxy, the same issue. Have you tried using my configuration to reproduce the issue with Traefik version 2.11.2? https://github.com/owncloud/android/issues/4352#issuecomment-2104242737
You can always still use my server which I have send in the .txt file as requested before. That server is still available with the issue present. I will disable Cloudflare Proxy for you to prevent digging into Cloudflare again...
If you want I can also send my full config, but since more people have already stated having the same issue, and since I only used pretty much the default configuration from the documentation, only addition is Authentik as Idp. and added Onlyoffice from the documentation after I already posted this issue, I doubt that it won't be reproducible.
appiekap653
commented
1 month ago @jvillafanez
I can't reproduce the issue with traefik 2.10.4 : it always sends a content-length header. Maybe I'm not setting the right options... these are the ones I'm using for the ocis container.
- "traefik.enable=true" - "traefik.http.routers.ocis.entrypoints=https" - "traefik.http.routers.ocis.rule=Host(`ocis.${DOMAIN:-owncloud.test}`)" - "traefik.http.routers.ocis.tls.certresolver=http" - "traefik.http.routers.ocis.service=ocis" - "traefik.http.services.ocis.loadbalancer.server.port=9200" - "traefik.http.middlewares.ocis.compress=true"
I can see why you cannot reproduce the issue. with the above (your) configuration, you only create a Middleware named ocis with compress=true, but you never assign the Middleware to your router named ocis.
you need to add: "traefik.http.routers.ocis.middlewares=ocis@docker"
otherwise you will never have compress Middleware enabled and then you will never experience the issue.
also keep in mind this issue: https://github.com/traefik/traefik/issues/9363 Traefik has issues with Middleware being created with the docker provider and are instructing people to use the file provider to declare Middleware. Then you can refer to the Middleware using docker labels to bind them to your routers.
You can look at my posted configuration for an example of using the file provider for the Compress Middleware and using the docker provider for the router declaration using labels.
In your case it will look like this:
Middlewares.yaml ~ saved here: {Your Docker Persistent Data Dir}/traefik/rules
http:
middlewares:
compress:
compress: {}Ocis Container labels:
- "traefik.enable=true"
- "traefik.http.routers.ocis.entrypoints=https"
- "traefik.http.routers.ocis.rule=Host(`ocis.${DOMAIN:-owncloud.test}`)"
- "traefik.http.routers.ocis.tls.certresolver=http"
- "traefik.http.routers.ocis.service=ocis"
- "traefik.http.services.ocis.loadbalancer.server.port=9200"
- "traefik.http.routers.ocis.middlewares=compress@file"Traefik labels:
volumes:
- {Your Docker Persistent Data Dir}/traefik/rules:/rules:ro
command:
- --providers.file.directory=/rules
- --providers.file.watch=trueMight worth to mention that OP is using / running:
ownCloud server version: Infinite Scale Community v4.0.3
but other users mention php-fpm bot oCIS doesn't use PHP so this could be something different or even independent from the underlying server variant (oCIS vs. ownCloud 10).
Also not even sure if using php-fpm is officially supported for the PHP variant of ownCloud.
There are mentions about ocis not using php but when I inspect my ocis website at the developer panel in Firefox I still see GET requests to endpoints like: "filename": "/remote.php/dav/meta/a612f42f-884a-4e05-9cae-43c29da25f1c%24afe73312-f57a-4e4a-8200-146cf936ac24"
would this suggest that ocis is also using php and therefore it could indeed be a server-side issue with both ocis and oc10?
iasdeoupxe
commented
1 month ago AFAICT oCIS is written in Go and not using PHP at all. Those could be from the Desktop client and used for compatibility reason between oCIS and ownCloud 10.
jvillafanez
commented
1 month ago you need to add: "traefik.http.routers.ocis.middlewares=ocis@docker"
Confirmed and reproduced. That was the missing piece for me.
@butonic could you check if the following patch in reva is good enough to fix the issue?
diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go
index 191d9cc04..54735431b 100644
--- a/internal/http/services/owncloud/ocdav/get.go
+++ b/internal/http/services/owncloud/ocdav/get.go
@@ -132,6 +132,7 @@ func (s *svc) handleGet(ctx context.Context, w http.ResponseWriter, r *http.Requ
defer httpRes.Body.Close()
copyHeader(w.Header(), httpRes.Header)
+ w.Header().Set("Content-Length", strconv.FormatUint(sRes.GetInfo().GetSize(), 10))
w.WriteHeader(httpRes.StatusCode)
if httpRes.StatusCode != http.StatusOK && httpRes.StatusCode != http.StatusPartialContent {I think that works for me. It seems we aren't actively sending the content length so maybe it's internally calculated by traefik for common scenarios.
appiekap653
commented
1 month ago Great to here you can reproduce it now.
Do you still need my server to test? I have disabled cloudflare proxy and have removed all other Middleware and removed my firewall rules for my server for your testing purposes. This leaves my server pretty vulnerable...
So it would be nice if you let me know if you still need it as soon as possible. So I can re-enable my security.
jesmrec
commented
1 month ago @appiekap653 please reenable, Don't be unsafe. We'll keep all of you up to date. @jvillafanez can reproduce, so that we can go with his env. Thanks a lot for letting us using your instance.
butonic
commented
1 month ago @jvillafanez hm when I debug a GET request the httpRes Headers that are copied with
copyHeader(w.Header(), httpRes.Header)they already contain the Content-Length.
š reading up on traefik compression, Content-Length and Accept-Encoding headers. Found: https://github.com/traefik/traefik/issues/7040#issuecomment-787875740 and that Transfer-Encoding in removed in HTTP/2.
AFAICT traefik will compress the ocis response, set a content-encoding: gzip header AND since the content length has changed removes the Content-Length header. It does not add a transfer-encoding header because http2 forbids it. Similar to cloudflare, traefix strems the content and thus has to remove the correct Content-Length when using compression.
butonic
commented
1 month ago @geobzmm regarding PHP in the url. We are just providing the same URLs for backwards compatability. Actually, the remote.php/ can be omitted from all URLs, but some clients or libraries still have it hardcodet.
Also, I'm a little worried about HTTP/2 and traefik dropping the Content-Lenght when enabling the compression middleware. AFAIK clients rely on the Content-Length header ... we need to investigate, but please disable compression for now to be on the safe side.
appiekap653
commented
1 month ago @butonic when sending the header Accept-Encoding Identity, Traefik shouldn't compress the response with gzip. And when looking at the codebase of the Compress Middleware it doesn't indeed.
jvillafanez
commented
1 month ago Quoting https://pkg.go.dev/net/http#ResponseWriter Write([]byte) (int, error)
// If [ResponseWriter.WriteHeader] has not yet been called, Write calls
// WriteHeader(http.StatusOK) before writing the data. If the Header
// does not contain a Content-Type line, Write adds a Content-Type set
// to the result of passing the initial 512 bytes of written data to
// [DetectContentType]. Additionally, if the total size of all written
// data is under a few KB and there are no Flush calls, the
// Content-Length header is added automatically.So golang automatically adds the content-length header for small files, but it doesn't for big files.
As seen in https://github.com/owncloud/android/issues/4352#issuecomment-2120052168 I don't think we're actively sending the header, so we could be relying on golang automatically sending the header (which don't work for big files) or on traefik doing some magic (which might depend on specific configuration)
butonic
commented
1 month ago ok, stepping through the GET code with a 7k file we indeed return no Content-Length header. @appiekap653 @jvillafanez so that is only another reason why the Content-Length header might be missing.
IMO clients cannot rely on it as the filesize as it actually only describes the length of the body, which might have been encoded in a different lenght. In propfinds owncloud introduced the oc:size property instead of the getcontentlength header for exactly this reason, IIRC.
now ... since HTTP/2 no longer mandates Content-Length ... should we still set it to the file length? even if it does not match the body length? Lets's look at the HTTP2 spec 8.1.2.6. Malformed Requests and Responses:
[...] A request or response that includes a payload body can include a content-length header field. A request or response is also malformed if the value of a content-length header field does not equal the sum of the DATA frame payload lengths that form the body [...]
Ok, what is the 'payload length' of a DATA frame? Go read 4.1 Frame Format and 6.1 DATA. I'll wait here.
Now. AFAICT the Content-Length header, if present, has to match the sum of the length of all DATA frame payloads. Transfer encoding is forbidden in HTTP/2 ...
What about 8.4. Content-Encoding? Seeing this:
Unlike Transfer-Encoding (Section 6.1 of [HTTP/1.1]), the codings listed in Content-Encoding are a characteristic of the representation; the representation is defined in terms of the coded form, and all other metadata about the representation is about the coded form unless otherwise noted in the metadata definition. Typically, the representation is only decoded just prior to rendering or analogous usage.
And 8.6. Content-Length with
Because Content-Length is used for message delimitation in HTTP/1.1, its field value can impact how the message is parsed by downstream recipients even when the immediate connection is not using HTTP/1.1. If the message is forwarded by a downstream intermediary, a Content-Length field value that is inconsistent with the received message framing might cause a security failure due to request smuggling or response splitting.
As a result, a sender MUST NOT forward a message with a Content-Length header field value that is known to be incorrect.
We MUST NOT set Content-Length to the size of the file as it may not represent the filesize expected by the client.
This also applies to the ETAG, and we had problems when enabling compression back in the day: https://github.com/owncloud/core/issues/9005
For etags we introduced X-OC-Etag.
AFAICT we need to introduce a OC-Size header (using a X- prefix is deprecated) if clients rely on this ...
AFAIU the android check tries to ensure all bytes have been received. HTTP/2 as an END_STREAM frame for that. AFAICT even if we set Content-Length to the uncompressed file length as @jvillafanez did in https://github.com/owncloud/android/issues/4352#issuecomment-2120052168 treafik would drop it for HTTP/2, anyway.
IMO the android app needs to learn how to handle HTTP/2 content length ... I don't see a reason to introduce a new OC-Size, yet.
jvillafanez
commented
1 month ago Just one thing: as far as I know, the android client doesn't support HTTP2, so all the requests are HTTP1.1. Not sure about the rest of the clients.
HTTP2 might be handled by traefik at the moment. Basically, a client sends a HTTP2 request to traefik (assuming it's supported on both sides) and then traefik sends a HTTP1.1 request to oCIS (otherwise, I'm not sure how you have a HTTP2 response from traefik, but an HTTP1.1 one if it's accessed directly to oCIS)
The android client is doing an HTTP1.1 request with an accept-encoding: identity header. This means that the response should NOT be encoded / compressed.
In this case, the content-length header should match the length of the transferred data, which should also match the size of the file (because of the requested accept-encoding). If there is a mismatch, then something went wrong (the connection dies and only half of the file has been transferred, for example).
Using the content-length header is useful to show the download progress. Regardless of the content being compressed or not, knowing that you need to download 2.5GB and that you have downloaded 2MB is better than showing a spinner wheel for more than 30 minutes
It also might be a good idea for the android client to check for the content-encoding header in the response (if any) to check if the content is compressed or not even if the accept-encoding: identity has been requested just to be on the safe side because then, if a content-length header is sent, then it will mean the size of the transfer (might be compressed) and not the size of the file. I mean, it's convenient that the transfer length matches the file size, but it won't always be the case.
As mention, the android client should also handle the case where no content-length header is available, although considering the above, it should be a rare case.
As for how all of this will work with HTTP2, at least for the android client I'd say "not supported at the moment".
felix-schwarz
commented
1 month ago @jesmrec @butonic The iOS client just relies on the HTTP client layer baked into the OS. That layer handles Content-Length, Transfer-Encoding and compression transparently, so the client doesn't need to worry about these. If the data arrives in chunked and/or compressed form, that layer pieces it back together and/or decompresses it before handing it to the app.
Therefore, the iOS client does not need to (and doesn't) use the value of the Content-Length header of the response at all. To verify the integrity of a file, it uses the checksum if one was provided by the server earlier. And it sends an If-Match header with the ETag of the file to ensure it gets the version it expects (or a 412 Precondition Failed response if that is no longer the current version).
On a more general note, expect that enabling compression will regularly lead to chunked responses because the transferred data is typically compressed on the fly and sent in chunks, rather than compressed as a whole before being sent.
JuancaG05
commented
1 month ago Thanks for clarifying @felix-schwarz. If you don't use Content-Length, how do you know the total size of a file then? Are you using oc:size from the PROPFIND response? And if so, when do you perform that PROPFIND?
We do need to know the total size of the file to be able to show the progress of the download (in a progress bar, for example).
jvillafanez
commented
1 month ago Maybe, instead of a progress bar, you could just show the number of bytes written on the phone. This way, you can show progress (by increasing the number), and you don't need to know the final content length nor whether the content is compressed. Same with iOS, I expect you to write the uncompressed data (coming from the OS / library) regardless of how the transfer is done (compressed or not).
Advantages:
Just a little remark: this would show bytes written, not transferred.
If we can know the size of the file, we could also enhance the UX with something like 12MB / 45MB. In any case, knowing the final size is optional.
felix-schwarz
commented
1 month ago @JuancaG05 Yes, the file size, like the Etag that's sent with If-Match, are originating from a previous PROPFIND.
By sending the Etag for the file from the last PROPFIND as If-Match header, the app can ensure it gets either:
PROPFIND returned)412 status response indicating the file has changed in the meantime, so a PROPFIND on the file can be sent to update the metadata for the file and a download subsequently be re-attemptedReceiving a newer/different version of the file than what the last PROPFIND returned wouldn't be good for several reasons:
PROPFIND (f.ex. while discovering changes), from the outdated one (at the time of download) to the actual one (for the file that was already downloaded). This would result in the app discarding the file it just downloaded - because it has to assume the file is has downloaded is a different version than the file on the server, due to the Etag change.jesmrec
commented
1 month ago Thanks everyone for your inputs. So, i think we should keep trusting Content-Length. If it is missing, there are several options (all the ones all of you mentioned):
Moving to http2 is not an option today to fix the problem mentioned in first message, probably it'll take longer to do, but we'll put it in our minds. Also, using identity (no compression) is a non-aligned value with the other clients that uses compression, something to check in close future.
JuancaG05
commented
1 month ago Hey, I just uploaded an APK with a potential fix for this problem. Could you test it to see if it works for you? š You can find it here:
Link: https://infinite.owncloud.com/s/AXxVvNToDiqtMxd Password: zI<6:2zW%c9J
appiekap653
commented
1 month ago For me that works now
JuancaG05
commented
1 month ago What about the rest? Could you take a minute to test it? š
Link: https://infinite.owncloud.com/s/AXxVvNToDiqtMxd Password: zI<6:2zW%c9J
@geobzmm @chaotix- @Noni006 @linkp @dapkdapk @iasdeoupxe @JohnHardline @NoSilentRunning @peterjpierre @ whoever wants to test it? š
peterjpierre
commented
1 month ago I can confirm it is working now as well ... will there also be a corrected version for iPhone?
jesmrec
commented
1 month ago I can confirm it is working now as well ... will there also be a corrected version for iPhone?
are you reproducing this in iOS as well??
peterjpierre
commented
1 month ago A friend of mine has an iPhone and he has the same problem not being able to download / open files
jesmrec
commented
1 month ago A friend of mine has an iPhone and he has the same problem not being able to download / open files
Not sure if it'd be the same problem, because the iOS app prevents the situation we fixed (check here how it works). In any case, it'd useful to create an issue in the iOS open repo with all details as posible, so that @felix-schwarz could take a look. Here the topic should be the Android app. Thanks for testing!
JuancaG05
commented
1 month ago Well, this is already sent to Google for review and very soon it will be available in the Play Store, becoming the 4.2.2 version of the app.
Thanks to everyone who was involved here, but specially to the community for reporting, helping and collaborating with us! ā¤ļø
Closing this ticket, but in any case, if you keep reproducing it with the new 4.2.2 version or whatever, feel free to open a new issue!
Actual behaviour
Creating a .txt file on another device containing a few words or some notes in a accessible folder then syncs as expected to all devices - server, laptop, desktop and Mobile / Cell. the .txt file can be opened / viewed on server, laptop (win 10), desktop (win 11) but Android App displays message "Download Failed". All other "document" formats appear to work fine (eg. .pdf, .docx, .xlsx, .doc etc...).
Expected behaviour
The .txt file should open and show the text in the file or be downloadable to the Android device (neither of which is possible).
Steps to reproduce
Can this problem be reproduced with the official owncloud server? (url: https://demo.owncloud.org, user: test, password: test) Not tested on Official Server
Environment data
Android version: v13
Device model: Samsung Galaxy S20+5G
Stock or customized system: Stock One UI v5.1
ownCloud app version: v4.2.1
ownCloud server version: Infinite Scale Community v4.0.3 running webclient v7.1.2
Logs
ownCloud App Log (data/owncloud.log)