CLDSRV-863: add checksums to PutObject and UploadPart#6094
CLDSRV-863: add checksums to PutObject and UploadPart#6094leif-scality wants to merge 1 commit 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 |
leif-scality
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
config.json
Outdated
| "clusters": 1, | ||
| "log": { | ||
| "logLevel": "info", | ||
| "logLevel": "trace", |
There was a problem hiding this comment.
Log level changed to trace in committed config. This will produce extremely verbose logs in any environment using the default config.
— Claude Code
| const mdOnlyHeader = request.headers['x-amz-meta-mdonly']; | ||
| const mdOnlySize = request.headers['x-amz-meta-size']; | ||
|
|
||
| // console.log('============== createAndStoreObject'); |
There was a problem hiding this comment.
Commented-out console.log statements left in production code. Remove before merging.
— Claude Code
| const { data } = require('../../../data/wrapper'); | ||
| const { prepareStream, stripTrailingChecksumStream } = require('./prepareStream'); | ||
| // const { prepareStream, prepareStream2, stripTrailingChecksumStream } = require('./prepareStream'); | ||
| const { prepareStream2 } = require('./prepareStream'); |
There was a problem hiding this comment.
Commented-out imports and console.log left in production code. The old prepareStream and stripTrailingChecksumStream imports are commented out instead of removed.
— Claude Code
| // if (!dataStreamTmp) { | ||
| // return process.nextTick(() => cb(errors.InvalidArgument)); | ||
| // } | ||
| // const dataStream = stripTrailingChecksumStream(dataStreamTmp, log, cbOnce); |
There was a problem hiding this comment.
Commented-out old code block (lines 63-67) should be removed, not left as dead code.
— Claude Code
| const valid = checksumedStream.validateChecksum(); | ||
| if (valid !== null) { | ||
| // console.log(valid); | ||
| return cbOnce(errors.BadDigest); |
There was a problem hiding this comment.
When checksum validation fails, the stored data is not cleaned up. checkHashMatchMD5 deletes stored data on MD5 mismatch, but here the data is left orphaned. Should call data.batchDelete on dataRetrievalInfo before returning the error.
— Claude Code
| // Authentication header, while the chunked upload method | ||
| // requires V4: in such case we don't get any V4 params | ||
| // and we should return an error to the client. | ||
| return null; // FIXME: use CB |
There was a problem hiding this comment.
return null with FIXME comment — the caller (dataStore in storeObject.js) passes the result directly to data.put without a null check. The old prepareStream had a null guard (if (!dataStreamTmp)) but it was commented out. This will crash with a TypeError when piping from null.
— Claude Code
| checksumAlgo.expected, | ||
| checksumAlgo.isTrailer, | ||
| ); | ||
| stream.pipe(checksumedStream); |
There was a problem hiding this comment.
No error listener on ChecksumTransform. The STREAMING-AWS4-HMAC-SHA256-PAYLOAD, UNSIGNED-PAYLOAD, and default cases pipe into a ChecksumTransform but never attach an error event handler. Unhandled stream errors will crash the process.
— Claude Code
| return checksumedStream; | ||
| } | ||
| case 'STREAMING-UNSIGNED-PAYLOAD-TRAILER': { | ||
| // console.log(checksumAlgo); |
There was a problem hiding this comment.
Commented-out console.log left in production code. Remove before merging.
— Claude Code
|
|
||
| _transform(chunk, encoding, callback) { | ||
| const input = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); | ||
| // console.log("chunk: '%s'", input); |
There was a problem hiding this comment.
Commented-out console.log left in production code.
— Claude Code
|
|
||
| validateChecksum() { | ||
| if (this.isTrailer) { | ||
| // FIXME: Handle trailer is missing |
There was a problem hiding this comment.
FIXME comment indicates a known unhandled case: if setExpectedChecksum is never called, trailerChecksumValue is undefined, producing a MalformedChecksum error instead of a more appropriate missing trailer error. This should be resolved 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.
Stray question comment added to an unrelated file. Remove before merging.
— Claude Code
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
FIXME and question comments should be resolved or removed before merging.
— Claude Code
| "test-gcp": "mocha -t 40000 test/GCP/ --exit", | ||
| "test-routes": "mocha -t 40000 test/routes/ --exit", | ||
| "test": "mocha -t 40000 test/ --exit", | ||
| "test": "mocha -t 40000 test/xAmzChecksum.js --exit", |
There was a problem hiding this comment.
Test script hardcoded to only run xAmzChecksum.js instead of the full test suite. This will cause other functional tests to stop running.
— 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 context. If this Veeam code path needs updating for prepareStream2, track it as a follow-up issue.
— Claude Code
|
|
||
| const trailerAlgo = trailer.slice('x-amz-checksum-'.length); | ||
| if (!(trailerAlgo in algorithms)) { | ||
| return { error: ChecksumError.AlgoNotSupported, details: { algorithm: trailerAlgo } };; |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
163fccb to
5b6aa59
Compare
|
Issues found: (1) Crash bug - prepareStream2 returns null but caller has no null guard (2) Data leak - stored data not cleaned up when validateChecksum fails (3) Missing stream error handlers on ChecksumTransform (4) config.json logLevel changed to trace (5) Test script narrowed to single file (6) Debug artifacts - commented-out console.log and code blocks (7) Unresolved FIXME/TODO comments (8) Double semicolons in validateChecksums.js:205 -- Review by Claude Code |
| const mdOnlyHeader = request.headers['x-amz-meta-mdonly']; | ||
| const mdOnlySize = request.headers['x-amz-meta-size']; | ||
|
|
||
| // console.log('============== createAndStoreObject'); |
There was a problem hiding this comment.
Commented-out debug console.log statements should be removed before merging. There are multiple instances throughout this file.
— Claude Code
| // Authentication header, while the chunked upload method | ||
| // requires V4: in such case we don't get any V4 params | ||
| // and we should return an error to the client. | ||
| return null; // FIXME: use CB |
There was a problem hiding this comment.
This returns null with a FIXME comment. The caller in storeObject.js will pass null to data.put(), causing a crash. Should call errCb(errors.InvalidArgument) instead.
— Claude Code
| checksumAlgo.expected, | ||
| checksumAlgo.isTrailer, | ||
| ); | ||
| stream.pipe(checksumedStream); |
There was a problem hiding this comment.
No error handler on ChecksumTransform streams. If ChecksumTransform emits an error, it will crash the process. The TrailingChecksumTransform path correctly adds .on('error', errCb) at line 87, but ChecksumTransform never gets one in any branch. Add checksumedStream.on('error', errCb) in each case.
— Claude Code
|
|
||
| const { data } = require('../../../data/wrapper'); | ||
| const { prepareStream, stripTrailingChecksumStream } = require('./prepareStream'); | ||
| // const { prepareStream, prepareStream2, stripTrailingChecksumStream } = require('./prepareStream'); |
There was a problem hiding this comment.
Commented-out imports should be removed.
— Claude Code
| // return process.nextTick(() => cb(errors.InvalidArgument)); | ||
| // } | ||
| // const dataStream = stripTrailingChecksumStream(dataStreamTmp, log, cbOnce); | ||
| // console.log('...................................'); |
There was a problem hiding this comment.
Commented-out code and debug console.log should be removed.
— Claude Code
|
|
||
| // console.log('================', | ||
| // checksumedStream.algoName, checksumedStream.digest, checksumedStream.expectedDigest); | ||
| const valid = checksumedStream.validateChecksum(); |
There was a problem hiding this comment.
validateChecksum() reads this.digest set in _flush(). The data.put callback fires when the backend finishes consuming data, but _flush() runs when the stream ends. If _flush() has not completed, this.digest is undefined and validation silently passes (non-trailer) or produces wrong results (trailer). Consider validating inside _flush() or waiting for the finish event.
— Claude Code
| const valid = checksumedStream.validateChecksum(); | ||
| if (valid !== null) { | ||
| // console.log(valid); | ||
| return cbOnce(errors.BadDigest); |
There was a problem hiding this comment.
When checksum validation fails, stored data is not cleaned up. The existing checkHashMatchMD5 deletes data on MD5 mismatch via data.batchDelete. This path should do the same, otherwise failed-checksum objects leak storage.
— Claude Code
|
|
||
| _transform(chunk, encoding, callback) { | ||
| const input = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); | ||
| // console.log("chunk: '%s'", input); |
There was a problem hiding this comment.
Debug console.log (commented out) should be removed.
— Claude Code
| const input = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); | ||
| // console.log("chunk: '%s'", input); | ||
| this.hash.update(input, encoding); | ||
| callback(null, input, encoding); |
There was a problem hiding this comment.
The Transform _transform callback signature is callback(error, chunk). The third argument encoding is not part of the API and will be ignored.
— 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 development note comment should not be committed.
— Claude Code
| if (!trailer.startsWith('x-amz-checksum-')) { | ||
| return { error: 'invalid x-amz-trailer' }; | ||
| } | ||
|
|
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
FIXME and inline question on line 36 are dev notes that should be resolved or removed before merging.
— Claude Code
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
TODO without description. The prepareStream call here likely needs migrating to prepareStream2 as well.
— Claude Code
| return checksumedStream; | ||
| } | ||
| case 'STREAMING-UNSIGNED-PAYLOAD-TRAILER': { | ||
| // console.log(checksumAlgo); |
There was a problem hiding this comment.
Commented-out console.log should be removed.
— Claude Code
| return checksumedStream; | ||
| } | ||
| default: { | ||
| // console.log(checksumAlgo); |
There was a problem hiding this comment.
Commented-out console.log should be removed.
— Claude Code
|
Critical issues: |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
5b6aa59 to
aa38363
Compare
| v4Transform.headers = request.headers; | ||
| stream = v4Transform; | ||
|
|
||
| const checksumedStream = new ChecksumTransform( |
There was a problem hiding this comment.
No error event handler is attached to any ChecksumTransform instance in this function. If _flush rejects (e.g. CRC digest throws), the stream emits an unhandled error event which will crash the process. Add .on('error', errCb) like TrailingChecksumTransform does at line 92.
— Claude Code
| const valid = checksumedStream.validateChecksum(); | ||
| if (valid !== null) { | ||
| // console.log(valid); | ||
| return cbOnce(errors.BadDigest); |
There was a problem hiding this comment.
When checksum validation fails, the data that was already stored is not deleted. Compare with checkHashMatchMD5 just below, which calls data.batchDelete before returning errors.BadDigest. This will leave orphaned data in the backend.
— Claude Code
|
|
||
| const { data } = require('../../../data/wrapper'); | ||
| const { prepareStream, stripTrailingChecksumStream } = require('./prepareStream'); | ||
| // const { prepareStream, prepareStream2, stripTrailingChecksumStream } = require('./prepareStream'); |
There was a problem hiding this comment.
Commented-out code and debug console.log statements throughout this file. These should be removed before merging.
— Claude Code
| const mdOnlyHeader = request.headers['x-amz-meta-mdonly']; | ||
| const mdOnlySize = request.headers['x-amz-meta-size']; | ||
|
|
||
| // console.log('============== createAndStoreObject'); |
There was a problem hiding this comment.
Commented-out console.log debug statements should be removed before merging.
— Claude Code
| if ('x-amz-trailer' in headers) { | ||
| const trailer = headers['x-amz-trailer']; | ||
| if (!trailer.startsWith('x-amz-checksum-')) { | ||
| return { error: 'invalid x-amz-trailer' }; |
There was a problem hiding this comment.
This returns a raw error string 'invalid x-amz-trailer' instead of a ChecksumError enum value. arsenalErrorFromChecksumError won't match any case and will fall through to the default returning BadDigest, which is incorrect for an invalid trailer header. Use a proper ChecksumError value.
— Claude Code
|
|
||
| if (xAmzChecksumCnt === 0) { | ||
| // There was no x-amz-checksum- or x-amz-trailer | ||
| return { algorithm: 'crc64nvme', isTrailer: false, expected: undefined }; |
There was a problem hiding this comment.
When no checksum headers are present, this defaults to crc64nvme. This means every regular request (without any checksum headers) will compute a CRC64 hash of the entire body, adding CPU overhead for no benefit since there is nothing to validate against (expected: undefined and validateChecksum() returns null for that case). Consider returning a sentinel value indicating no checksum is needed.
— Claude Code
|
|
||
| validateChecksum() { | ||
| if (this.isTrailer) { | ||
| // FIXME: Handle trailer is missing |
There was a problem hiding this comment.
Multiple FIXME/TODO comments in this file indicate unfinished work: line 24 ("FIXME: Handle trailer is missing"), line 69 (commented-out console.log). These should be resolved or tracked before merging.
— Claude Code
| const checksumAlgo = getChecksumDataFromHeaders(request.headers); | ||
| if (checksumAlgo.error) { | ||
| log.debug('invalid checksum headers', checksumAlgo); | ||
| return errCb(arsenalErrorFromChecksumError(checksumAlgo)); // FIXME sometimes we use CB sometimes we use null |
There was a problem hiding this comment.
Double-callback bug: errCb() is called here (which calls cbOnce), and its return value (undefined) is returned. Back in storeObject.js:71, the \!checksumedStream check then fires cbOnce(errors.InvalidArgument) — but cbOnce was already invoked by errCb. Even with jsutil.once, only one error reaches the caller, and the real checksum error is replaced by a generic InvalidArgument.
Either return null (like the V4 params check below) and let the caller handle it, or don't return the result of errCb().
— Claude Code
|
|
||
| const trailerAlgo = trailer.slice('x-amz-checksum-'.length); | ||
| if (!(trailerAlgo in algorithms)) { | ||
| return { error: ChecksumError.AlgoNotSupported, details: { algorithm: trailerAlgo } };; |
|
2 participants
No description provided.