Repository navigation
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
ping @elzody @juliusknorr |
ae02f09 to
35bd166
Compare
|
@timar Will take a closer look at this soon. |
juliusknorr
left a comment
juliusknorr
left a comment
There was a problem hiding this comment.
Generally a good idea, some comments inline
| // whether what is in storage is what it uploaded. This costs no | ||
| // extra read: the bytes are going through us anyway. | ||
| $sha256 = null; | ||
| $content = HashWrapper::wrap($content, 'sha256', function (string $hash) use (&$sha256): void { |
There was a problem hiding this comment.
OC\Files\Stream\HashWrapper is private server API, so we can't rely on it staying stable across the server versions we support. Hashing on write also feels like a server job. nextcloud/server#56057 asks for exactly that, so every write path (DAV without a client checksum, other apps) would get a checksum.
Not a blocker I'd say but just for context.
| return new JSONResponse(['message' => 'File locked'], Http::STATUS_LOCKED); | ||
| } | ||
|
|
||
| $this->storeSha256($file, $sha256); |
There was a problem hiding this comment.
This could store our hash on top of someone else's content. putContent() has already released the lock at this point. If another write lands between that and storeSha256(), the cache ends up with their content and our hash. That's exactly the case this is meant to catch, and the editor would then wrongly treat it as its own. The window is small, but I think it's worth closing. I'm unsure if we can perform a ourselves before the write without interfering with the write lock itself.
There was a problem hiding this comment.
I lack ideas how to mitigate that, the only way I see is that server stores the hash directly.
| * Checksums are stored as space-separated TYPE:HEX pairs, so a file may | ||
| * carry an MD5 or SHA1 from a sync client and no SHA-256 at all. | ||
| */ | ||
| private function getStoredSha256(File $file): ?string { |
There was a problem hiding this comment.
Can we simplify this a bit? Iirc getChecksum() always returns a string, so the try/catch and the is_string check shouldn't be needed. Have not tested but something like this?
private function getStoredSha256(File $file): ?string {
if (!preg_match('/(?:^| )SHA256:([0-9a-f]{64})(?: |$)/i', $file->getChecksum(), $matches)) {
return null;
}
return base64_encode(hex2bin($matches[1]));
}| return new JSONResponse(['message' => 'File locked'], Http::STATUS_LOCKED); | ||
| } | ||
|
|
||
| $this->storeSha256($file, $sha256); |
There was a problem hiding this comment.
Can we add a small integration test in tests/features/wopi.feature? Something like "PutFile, then CheckFileInfo returns the matching SHA256", and "SHA256 is absent after the file was changed via DAV".
After an upload that gets no answer, the editor has to work out whether what is in storage is what it just sent. Without a hash to compare it falls back on the byte count, which cannot tell our own document from another editor's of the same length: the wrong answer either loses the other writer's work or puts a needless conflict in front of the user. Report the file's SHA-256 in the CheckFileInfo SHA256 field so there is something better to compare. The field is only present when a hash is on record, and never for a version: CheckFileInfo is on the document-open path and must not read a whole file to compute one, and the hash on record describes the current contents, not an older version's. For it to be on record when it matters, hash the body of PutFile as it streams past and store the result in the cache field that holds the checksums a sync client sends over DAV. A stream filter of our own does the hashing, so that we do not lean on the server's private HashWrapper. Wrapping the stream costs no extra read, since the bytes pass through us in any case. The hash is recorded from a NodeWrittenEvent listener registered for the duration of the write. The server dispatches that event while it still holds the lock it took for the write and after the cache has been updated, which is also where the DAV endpoint persists its checksum. A competing writer cannot take the lock until we are done, so the hash always describes the bytes it was taken from. Recording it after putContent() returned would have left a window in which another writer's content ends up carrying our hash, the very confusion the hash is meant to prevent. The scanner clears the checksum whenever it sees the file change, and DAV clears it when a client sends none, so what we hand out is never stale: it either describes the current contents or is absent. Files written by other means, or before this change, simply carry no SHA-256 and the editor falls back as it does today. Signed-off-by: Andras Timar <andras.timar@collabora.com>
35bd166 to
93b2c54
Compare
After an upload that gets no answer, the editor has to work out whether what is in storage is what it just sent. Without a hash to compare it falls back on the byte count, which cannot tell our own document from another editor's of the same length: the wrong answer either loses the other writer's work or puts a needless conflict in front of the user.
Report the file's SHA-256 in the CheckFileInfo SHA256 field so there is something better to compare. The field is only present when a hash is on record; CheckFileInfo is on the document-open path and must not read a whole file to compute one.
For it to be on record when it matters, hash the body of PutFile as it streams past and store the result, the way the DAV endpoint stores the checksums a sync client sends. Wrapping the stream costs no extra read, since the bytes pass through us in any case. The scanner clears the checksum whenever a file changes, so what we hand out is never stale: it either describes the current contents or is absent, and setting it after putContent() keeps it that way.
Files written by other means, or before this change, simply carry no SHA-256 and the editor falls back as it does today.
Checklist