Skip to content

feat: Support OC-Checksum in bulk upload #51729

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.

Sign up for GitHub

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

Jump to bottom
Merged
artonge merged 1 commit into from
Apr 15, 2025
Merged

feat: Support OC-Checksum in bulk upload #51729

artonge merged 1 commit into from
Apr 15, 2025

Conversation

@artonge
Copy link
Contributor

@artonge artonge commented Mar 26, 2025

To align with the rest of Nextcloud.

@artonge artonge requested a review from a team as a code owner March 26, 2025 15:33
@artonge artonge requested review from Altahrim, come-nc and provokateurin and removed request for a team March 26, 2025 15:33
@artonge artonge self-assigned this Mar 26, 2025
provokateurin
provokateurin reviewed Mar 26, 2025
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm we do actually validate the checksum? I thought we simply accepted the checksum as is and only the Desktop client is verifying it when downloading a file.
Please link where this happens for other uploads, I'd be interested to know!

Anyway I don't think we should really do this, as this can easily turn into a DOS if a user uploads a very big file.

mgallien
mgallien requested changes Mar 26, 2025
View reviewed changes
Copy link
Contributor

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also need to remove those lines https://github.com/nextcloud/server/pull/51729/files#diff-5f5684fb699f86734d9aa4fd1cba195ada4b5635b29b9c82822bec2bc78692b7R190-R192

@artonge
Copy link
Contributor Author

artonge commented Mar 26, 2025

do actually validate the checksum?

We usually don't, but in the case of bulk upload where the request is manually forged and parsed, it feels safer to do it.

Anyway I don't think we should really do this, as this can easily turn into a DOS if a user uploads a very big file.

This is bulk upload, so only small files are transferred.

@artonge artonge force-pushed the artonge/support_oc_checksum_in_bulk_upload branch from 95780b3 to bad7319 Compare March 26, 2025 19:44
@artonge artonge added 3. to review Waiting for reviews feature: files php Pull requests that update Php code enhancement labels Mar 26, 2025
@artonge artonge added this to the Nextcloud 32 milestone Mar 26, 2025
@artonge artonge force-pushed the artonge/support_oc_checksum_in_bulk_upload branch 7 times, most recently from 85dc4c3 to 9926567 Compare March 27, 2025 10:17
@provokateurin
Copy link
Member

provokateurin commented Mar 27, 2025

This is bulk upload, so only small files are transferred.

But there could be a lot of small files in a bulk upload, leading to the same DOS situation.

@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

But there could be a lot of small files in a bulk upload, leading to the same DOS situation.

As of now, it's not reported as an issue, so I think we should keep it. It makes parsing much more reliable, and hash computing is cheap enough.

mgallien
mgallien requested changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a change in the capabilities or clients musk keep sending the header for compatibility with older server releases

@joshtrichards joshtrichards added the hotspot: file transfer performance upload & download performance related optimizations label Mar 27, 2025
@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

I think we need a change in the capabilities or clients musk keep sending the header for compatibility with older server releases

I was thinking, the desktop client drops the old header in the next release, and once the current desktop client version is out of support, we drop it server side. Does it address your concern?

@mgallien
Copy link
Contributor

mgallien commented Mar 31, 2025

I think we need a change in the capabilities or clients musk keep sending the header for compatibility with older server releases

I was thinking, the desktop client drops the old header in the next release, and once the current desktop client version is out of support, we drop it server side. Does it address your concern?

we discussed it and reached the conclusion that the client needs to check the version of the server to figure out when to send the former header x-file-md5
that should enable the client to stop sending it as soon as a release from server drops it until all supported server releases stops requiring it

mgallien

This comment was marked as resolved.

Sign in to view

mgallien added a commit to nextcloud/desktop that referenced this pull request Mar 31, 2025
@mgallien
only send the md5 custom checksum header when server expects it
1d72aa5 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien mgallien mentioned this pull request Mar 31, 2025
Merged
@mgallien

This comment was marked as resolved.

Sign in to view
mgallien added a commit to nextcloud/desktop that referenced this pull request Mar 31, 2025
@mgallien
only send the md5 custom checksum header when server expects it
e416015 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
mgallien
mgallien requested changes Mar 31, 2025
View reviewed changes
Copy link
Contributor

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works fine
sorry for the noise
you may want to have a better commit message
the current one seems unrelated to the commit content

@artonge artonge force-pushed the artonge/support_oc_checksum_in_bulk_upload branch from 9926567 to 1e8227d Compare March 31, 2025 15:24
@artonge
feat: Support OC-Checksum in bulk upload
3bb2a92 
To align with the rest of Nextcloud.

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/support_oc_checksum_in_bulk_upload branch from 462442b to 3bb2a92 Compare April 1, 2025 12:35
@artonge artonge requested a review from mgallien April 2, 2025 13:32
@artonge
Copy link
Contributor Author

artonge commented Apr 7, 2025

@mgallien please approve if this works for the desktop client :).

mgallien added a commit to nextcloud/desktop that referenced this pull request Apr 10, 2025
@mgallien
only send the md5 custom checksum header when server expects it
9f14092 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
backportbot bot pushed a commit to nextcloud/desktop that referenced this pull request Apr 10, 2025
@mgallien @backportbot
only send the md5 custom checksum header when server expects it
ec0e2d6 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
camilasan pushed a commit to nextcloud/desktop that referenced this pull request Apr 10, 2025
@mgallien @camilasan
only send the md5 custom checksum header when server expects it
975b1e0 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
@artonge artonge merged commit b2a187d into master Apr 15, 2025
200 of 207 checks passed
@artonge artonge deleted the artonge/support_oc_checksum_in_bulk_upload branch April 15, 2025 07:49
memurats pushed a commit to nextmcloud/desktop that referenced this pull request Jun 4, 2025
@mgallien @memurats
only send the md5 custom checksum header when server expects it
59fdb00 
server before 32.0.0 release expects a custom header with an MD5
checksum during bulk upload

the header name is: X-File-MD5

see nextcloud/server#51729

Signed-off-by: Matthieu Gallien <[email protected]>
@nextcloud-bot nextcloud-bot mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgallien mgallien mgallien approved these changes

@provokateurin provokateurin provokateurin approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@come-nc come-nc Awaiting requested review from come-nc come-nc is a code owner automatically assigned from nextcloud/server-backend

Assignees

@artonge artonge

Labels

3. to review Waiting for reviews enhancement feature: files hotspot: file transfer performance upload & download performance related optimizations php Pull requests that update Php code

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

6 participants

@artonge @provokateurin @mgallien @joshtrichards @skjnldsv