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

PATCH request for TUS upload with wrong checksum gives incorrect response #1755

Open
swoichha opened this issue Mar 3, 2021 · 7 comments
Open
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Type:Discussion

Comments

@swoichha
Copy link
Contributor

swoichha commented Mar 3, 2021

Describe the bug

According to the TUS checksum extension description in https://tus.io/protocols/resumable-upload.html#checksum when a PATCH request is send with an Upload-Checksum header depending on the result the server may respond with one of the following status code:
1.400 Bad Request if the checksum algorithm is not supported by the server
2. 460 Checksum Mismatch if the checksums mismatch
3. 204 No Content if the checksums match and the processing of the data succeeded
But when we send PATCH request with wrong checksum then instead of 460 Checksum Mismatch we get 204 No Content

Steps to reproduce

Steps to reproduce the behavior:

  1. Create a base64 encoding of a file name
  • echo -n "textFile.txt" | base64 (example dGV4dEZpbGUudHh0)
  1. As user Einstein send a POST request to create resource which will give a resource location url.
 curl -k -X POST -u einstein:relativity   https://localhost:9200/remote.php/dav/files/Einstein/  -H 'Tus-Resumable: 1.0.0' -H 'Upload-Length: 5' -H 'Upload-Metadata: filename dGV4dEZpbGUudHh0' -v
  1. User Einstein send PATCH request to upload data to the location url along with a wrong checkum
    For example: correct sha1sum for '12345' is 8cb2237d0679ca88db6464eac60da96345513964 but we are sending wrong value i.e. 01b307acba4f54f55aafc33bb06bbbf6ca803e9a:
curl -k -X PATCH <your TUS resource Location> -u einstein:relativity -H 'Content-Type: application/offset+octet-stream' -H 'Tus-Resumable: 1.0.0' -H 'Upload-Offset: 0' -H'Upload-Checksum:sha1 01b307acba4f54f55aafc33bb06bbbf6ca803e9a' -d '12345' -v

Expected behavior

HTTP/1.1 460 Checksum Mismatch
Tus-Resumable: 1.0.0
Upload-Offset: 5

Actual behavior

HTTP/1.1 204 No content
Tus-Resumable: 1.0.0
Upload-Offset: 5

While doing PROPFIND it can be seen that the file textFile.txt has been created and it gives the correct checksum and ignores the incorrect checksum send during PATCH request.

curl -X PROPFIND -u einstein:relativity https://localhost:9200/remote.php/webdav -k | xmllint --format -
    % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1676  100  1676    0     0  27032      0 --:--:-- --:--:-- --:--:-- 27032
<?xml version="1.0" encoding="utf-8"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/webdav/</d:href>
    <d:propstat>
      <d:prop>
        <oc:id>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OjA2OTRjZjY4LWYxOGUtNDY1ZC1hMzlkLTEzNWFkM2RkMjg3ZA==</oc:id>
        <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OjA2OTRjZjY4LWYxOGUtNDY1ZC1hMzlkLTEzNWFkM2RkMjg3ZA==</oc:fileid>
        <d:getetag>"7bdfa13e5b7a5a4cdd27fb7550617b10"</d:getetag>
        <oc:permissions>DNVCKR</oc:permissions>
        <d:resourcetype>
          <d:collection/>
        </d:resourcetype>
        <oc:size>5</oc:size>
        <d:getlastmodified>Thu, 04 Mar 2021 06:38:01 GMT</d:getlastmodified>
        <oc:favorite>0</oc:favorite>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/webdav/textFile.txt</d:href>
    <d:propstat>
      <d:prop>
        <oc:id>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3Ojg2NzI2NzA5LTNlOTQtNDcxZS04YTAyLTAzOTdlMzdjYzdhYw==</oc:id>
        <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3Ojg2NzI2NzA5LTNlOTQtNDcxZS04YTAyLTAzOTdlMzdjYzdhYw==</oc:fileid>
        <d:getetag>"a5cf632982d978b971264c7edddb2738"</d:getetag>
        <oc:permissions>DNVWR</oc:permissions>
        <d:resourcetype/>
        <d:getcontentlength>5</d:getcontentlength>
        <d:getcontenttype>text/plain; charset=utf-8</d:getcontenttype>
        <d:getlastmodified>Thu, 04 Mar 2021 06:38:01 GMT</d:getlastmodified>
        <oc:checksums>
          <oc:checksum>SHA1:8cb2237d0679ca88db6464eac60da96345513964 MD5:827ccb0eea8a706c4c34a16891f84e7b ADLER32:02f80100</oc:checksum>
        </oc:checksums>
        <oc:favorite>0</oc:favorite>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

@ScharfViktor
Copy link
Contributor

tested on ocis 6.0.0 still relevant

@micbar micbar moved this from Qualification to Prio 2 in Infinite Scale Team Board Jun 27, 2024
@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label Jun 27, 2024
@2403905 2403905 self-assigned this Jul 2, 2024
@2403905
Copy link
Contributor

2403905 commented Jul 11, 2024

@micbar @butonic @aduffeck There are few points for discussion:

@2403905 2403905 removed their assignment Jul 11, 2024
@kobergj
Copy link
Collaborator

kobergj commented Jul 30, 2024

Yes, we need to switch to v2 version if we want this fixed. See accurate description of the problem here: https://github.com/cs3org/reva/blob/edge/pkg/storage/utils/decomposedfs/upload/upload.go#L68-L71 (tusd v1 simply doesn't support it...)

@dragotin
Copy link
Contributor

dragotin commented Aug 9, 2024

How do we continue here? Do we update to version 2? What is the downside?

@kobergj
Copy link
Collaborator

kobergj commented Aug 9, 2024

v2 has landed on master. I'll try making progress here next week 👍

@kobergj kobergj self-assigned this Aug 13, 2024
@kobergj kobergj moved this from Prio 2 to In progress in Infinite Scale Team Board Aug 13, 2024
@kobergj
Copy link
Collaborator

kobergj commented Aug 14, 2024

Tried to add support for chunk based checksum checking here cs3org/reva#4807 but we hit several walls.

A) Mixed checksum format during test.
The format in which the checksum headers are sent differs within test. Tests not specifying a checksum use the tus-php package which base64 encodes the checksum. Test that specify a checksum do not base64 encode. The tus spec states that the header should be base64 encoded.

B) Breaking bug in tus-php package
The tus-php contains a breaking bug. It always sends the checksum for the complete file instead of the checksum for one chunk. ankitpokhrel/tus-php#422 This means the tests are sending wrong requests when making multiple requests.

We could get pass A by adjusting all tests. But B is breaking as we cannot control it. We have two options: fix it upstream (if possible) or switch to different tus package for php tests.

@kobergj kobergj moved this from In progress to blocked in Infinite Scale Team Board Aug 14, 2024
@butonic
Copy link
Member

butonic commented Aug 14, 2024

There does not seem to be another up to date php tus client library: https://tus.io/implementations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Type:Discussion
Projects
Status: blocked
Development

No branches or pull requests

8 participants