CLDSRV-863: Checksums for PutObject and UploadPart#6105
CLDSRV-863: Checksums for PutObject and UploadPart#6105leif-scality wants to merge 7 commits intodevelopment/9.4from
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
The FIXME: merge this comment and // Why do we need this check... are dev notes that should be resolved or removed before merging.
— Claude Code
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Dev note // Do we use this one or vaults? should be resolved and removed before merging.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
The // TODO comment here indicates veeam/utils.js still uses the old prepareStream which is no longer wired to checksum validation. This should be addressed in this PR or tracked explicitly.
— Claude Code
Review by Claude Code |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (74.19%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
@@ Coverage Diff @@
## development/9.4 #6105 +/- ##
===================================================
- Coverage 84.38% 84.14% -0.25%
===================================================
Files 206 207 +1
Lines 13329 13528 +199
===================================================
+ Hits 11248 11383 +135
- Misses 2081 2145 +64
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
Debug comments left in the code: // FIXME: merge this and // Why do we need this check if we have unsupportedSignatureChecksums? appear to be development notes that should be resolved or removed before merging.
— Claude Code
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
This comment // Do we use this one or vaults? looks like a development note that should be resolved and removed before merging.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Bare // TODO comment with no description. Either add context about what needs to be done (presumably migrating to prepareStream2) or remove if not actionable.
— Claude Code
|
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Leftover development comment: // Do we use this one or vaults? — resolve or remove before merging.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Bare // TODO with no description. This call still uses the old prepareStream which doesn't do checksum validation. Either migrate this to prepareStream2 or add a clear explanation of why it's deferred.
— Claude Code
Review by Claude Code |
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Debug comment left in production code. This looks like a personal note that should be removed before merging.
— Claude Code
| // If the x-amz-trailer header is present the request is using one of the | ||
| // trailing checksum algorithms, which are not supported. | ||
| if (headers['x-amz-trailer'] !== undefined && | ||
| if (headers['x-amz-trailer'] !== undefined && // Why do we need this check if we have unsupportedSignatureChecksums? |
There was a problem hiding this comment.
Debug comment left in production code. This looks like a personal note that should be removed before merging.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Bare TODO left in production code. This should either be addressed or removed before merging.
— Claude Code
| checksumedStream.on('error', errCb); | ||
| stream.pipe(checksumedStream); | ||
| return { error: null, stream: checksumedStream }; | ||
| } |
There was a problem hiding this comment.
UNSIGNED-PAYLOAD and default cases have identical code. Consider combining them by removing the UNSIGNED-PAYLOAD case and letting it fall through to default.
— Claude Code
| if (checksumedStream.stream.writableFinished) { | ||
| return doValidate(); | ||
| } | ||
| checksumedStream.stream.once('finish', doValidate); |
There was a problem hiding this comment.
Race condition risk: if the data backend callback fires before ChecksumTransform emits finish, this attaches a finish listener. But if an error on the checksum stream prevents finish from ever firing (e.g. stream destroyed), doValidate never runs and cbOnce never fires, leaving the request hanging. Consider also listening for the error event as a safety net.
— Claude Code
| if (xAmzChecksumCnt !== 0) { | ||
| return { | ||
| error: ChecksumError.TrailerAndChecksum, | ||
| details: { trailer: headers['x-amz-trailer'], checksum: checksumHeaders }, |
There was a problem hiding this comment.
Typo in comment: "If AWS there is a mismatch" should be "If there is a mismatch".
— Claude Code
Review by Claude Code |
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Unfinished TODO left in production code. Should be addressed or tracked in a ticket before merging.
— Claude Code
| checksumedStream.on('error', errCb); | ||
| stream.pipe(checksumedStream); | ||
| return { error: null, stream: checksumedStream }; | ||
| } |
There was a problem hiding this comment.
The UNSIGNED-PAYLOAD case and default case are identical. Consider combining them by removing the UNSIGNED-PAYLOAD case and letting it fall through to default.
— Claude Code
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Debug question left in production code. Remove before merging.
— Claude Code
| // If the x-amz-trailer header is present the request is using one of the | ||
| // trailing checksum algorithms, which are not supported. | ||
| if (headers['x-amz-trailer'] !== undefined && | ||
| if (headers['x-amz-trailer'] !== undefined && // Why do we need this check if we have unsupportedSignatureChecksums? |
There was a problem hiding this comment.
Debug question left in production code. Remove before merging.
— Claude Code
| return callback(); | ||
| }) | ||
| .catch(callback); | ||
| } |
There was a problem hiding this comment.
Double-callback risk: if callback() throws synchronously inside .then(), .catch(callback) calls it again. Move callback() out of the .then() return chain to prevent this.
— Claude Code
Review by Claude Code |
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Debug comment left in code. This looks like a development note that should not be committed.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Debug TODO left in code. This should either be addressed or removed before merging.
— Claude Code
| // If the x-amz-trailer header is present the request is using one of the | ||
| // trailing checksum algorithms, which are not supported. | ||
| if (headers['x-amz-trailer'] !== undefined && | ||
| if (headers['x-amz-trailer'] !== undefined && // Why do we need this check if we have unsupportedSignatureChecksums? |
There was a problem hiding this comment.
Debug comment left in code. This reads like an open question during development, not a permanent comment. Remove the comment or resolve the question.
— Claude Code
| stream.pipe(checksumedStream); | ||
| return { error: null, stream: checksumedStream }; | ||
| } | ||
| case 'UNSIGNED-PAYLOAD': { |
There was a problem hiding this comment.
The UNSIGNED-PAYLOAD and default cases are identical. Consider collapsing them into a single fallthrough to reduce duplication.
— Claude Code
| stream.pipe(checksumedStream); | ||
| return { error: null, stream: checksumedStream }; | ||
| } | ||
| default: { |
There was a problem hiding this comment.
STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER (signed payload with trailer) is not handled. If a client sends this content-sha256 value, it falls through to the default case, which does not set up V4Transform for signature verification. Should this be handled explicitly, or rejected with an error?
— Claude Code
| _flush(callback) { | ||
| Promise.resolve(this.algo.digestFromHash(this.hash)) | ||
| .then(digest => { this.digest = digest; }) | ||
| .then(() => callback(), callback); |
There was a problem hiding this comment.
If digestFromHash rejects, callback is called with the raw error. The Transform stream emits this as an error event handled by errCb. In storeObject.js, after data.put completes, the onError handler calls cbOnce(err) with this raw error rather than a proper Arsenal error, which may leak internal details to the client. Consider wrapping to ensure a clean Arsenal error.
— Claude Code
Review by Claude Code |
2 participants
No description provided.