Skip to content

feat: Enable full object checksum validation on JSON path #2687

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
Open
thiyaguk09 wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: Enable full object checksum validation on JSON path #2687

thiyaguk09 wants to merge 15 commits into from
+664 −7

Conversation

@thiyaguk09
Copy link
Contributor

@thiyaguk09 thiyaguk09 commented Dec 9, 2025

Description

This pull request significantly enhances data integrity for object uploads by introducing comprehensive client-side checksum validation. It ensures that data uploaded through the JSON API path remains consistent by verifying both client-provided and client-calculated CRC32C and MD5 hashes against the server's reported hashes. This mechanism proactively detects and prevents silent data corruption, providing a more reliable upload experience.

Impact

The primary impact is a substantial improvement in data integrity and reliability for object uploads, particularly those utilizing the JSON API path (e.g., small single-chunk uploads).

  • Prevents Silent Data Corruption: By validating client-side hashes (calculated or user-provided) against the server's reported hashes, the SDK proactively detects and stops data corruption that might otherwise go unnoticed.
  • Enhanced Reliability: The stream is immediately destroyed upon hash mismatch, preventing the client from assuming a corrupted file upload was successful.
  • Feature Parity: Enables the use of the X-Goog-Hash header, which is essential for ensuring the integrity of the data payload sent to the server.
  • API Usage: Introduces new configuration options (clientCrc32c, clientMd5Hash, crc32c, md5) that allow users fine-grained control over checksum validation behavior.

Testing

  • Unit and Integration Tests Added? Yes. Extensive test coverage was added and expanded in test/resumable-upload.ts and system-test/kitchen.ts. This includes:

    • Verification of X-Goog-Hash header injection with calculated and client-provided hashes in single and multi-chunk uploads.
    • Validation tests to confirm that the stream is correctly destroyed on both CRC32C and MD5 checksum mismatches between the client and server.
    • A dedicated describe block for Validation of Client Checksums Against Server Response with various success and failure scenarios.

  • Were any tests changed? Yes. Tests were expanded and refactored (e.g., checksum application logic tests).

  • Are any breaking changes necessary? No. This change introduces new features and configuration options but does not appear to break existing functionality. The changes are largely additive and internal refactoring of hash handling and validation logic.

Additional Information

  • Refactoring for Robustness: The HashStreamValidator was improved by adding the md5Digest getter, which centralizes the MD5 calculation and caching logic. The related fix in _flush prevents a potential race condition where calling digest() multiple times would cause a runtime error.
  • Internal Consistency: Private helper methods (#validateChecksum, #applyChecksumHeaders) were introduced in resumable-upload.ts to simplify and clarify the hash handling logic, especially ensuring checksum headers are applied correctly across different upload paths.
  • Dependency on Configuration: Proper client-side validation requires users to explicitly enable calculation (crc32c: true, md5: true) or provide the pre-calculated hashes.

Checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease
  • Appropriate docs were updated
  • Appropriate comments were added, particularly in complex areas or places that require background
  • No new warnings or issues will be generated from this change

Fixes #

@thiyaguk09
feat: Enable full object checksum validation on JSON path
93763bf 
Adds validation for client-provided (pre-calculated) and
client-calculated CRC32C and MD5 hashes when the final upload request is
made via the JSON API path (status 200).

This ensures consistency checks are performed even when the `Upload`
stream is finalized, preventing silent data corruption if the
server-reported hash (in the response body) mismatches the client's
expected hash.
@thiyaguk09
fix: duplicate codes removed
b81433c
@thiyaguk09
added unit test cases
38fbb80
@thiyaguk09
fix: Resolve checksum validation failures and add full test suite
1135812 
Adds the 'Validation of Client Checksums Against Server Response' test
suite. Fixes test failures in client-provided hash scenarios by updating
mock responses to ensure server-reported checksums match the client's
expected values.
@thiyaguk09
code refactor
c239551
@thiyaguk09
refactor: Combine and parameterize checksum validation tests
6eb2036 
Refactors four duplicate test cases (CRC32C/MD5 success and failure)
into a single, parameterized test block within the 'Validation of Client
Checksums Against Server Response' suite.

This improves test clarity and reduces code duplication by dynamically
generating test scenarios for post-upload hash validation.
@thiyaguk09
addressing comments
deabdfc
@thiyaguk09
bug fix
4c7cab5
@thiyaguk09
added system test
47aafac
@thiyaguk09
fix system test
674b1d4
@thiyaguk09
bug fix
7e631dc
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Dec 9, 2025
@thiyaguk09 thiyaguk09 mentioned this pull request Dec 9, 2025
Closed
6 tasks
@thiyaguk09
fix: Address stream consumption, checksum, and multi-part session hangs
c9670e7 
This commit introduces several stability fixes for the ResumableUpload
class:

1.  **Fixes Timeouts in Unit Tests:** Updates `makeRequestStream` mocks
to fully drain the request body stream, resolving stream consumption
deadlocks and timeouts in `#startUploading` unit tests.
2.  **Fixes Multi-Part Hang:** Correctly finalizes the `pipeline` for
partial chunks (`isPartialUpload=true`) by calling `pipelineCallback()`
immediately after successful chunk upload, preventing indefinite hangs
in multi-session tests.
3.  **Fixes Single-Chunk Checksum Missing Header:** Applies the
`X-Goog-Hash` header unconditionally in single-chunk mode if a validator
is present, ensuring checksum validation is active even when
`contentLength` is unknown.
@thiyaguk09
code refactor
f049c4e
@thiyaguk09 thiyaguk09 marked this pull request as ready for review December 10, 2025 11:33
@thiyaguk09 thiyaguk09 requested review from a team as code owners December 10, 2025 11:33
ddelgrosso1
ddelgrosso1 reviewed Dec 10, 2025
View reviewed changes
ddelgrosso1
ddelgrosso1 reviewed Dec 10, 2025
View reviewed changes
src/resumable-upload.ts
) ||
this.#validateChecksum(clientMd5HashToValidate, serverMd5, 'MD5')
) {
return;
Copy link
Contributor

@ddelgrosso1 ddelgrosso1 Dec 10, 2025

Choose a reason for hiding this comment

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

Why would we exit early when there is a successful checksum check? Wouldn't we want to continue and do the cleanup below?

Copy link
Contributor Author

@thiyaguk09 thiyaguk09 Dec 11, 2025

Choose a reason for hiding this comment

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

The early exit (return;) is mandatory because if this.#validateChecksum detects a CRC32C or MD5 mismatch, it immediately calls this.destroy(error). This action places the stream in an error state.

Continuing execution without the early exit would lead to a "false success" by incorrectly running the subsequent cleanup code, which includes emitting 'metadata' and 'uploadFinished'. The return ensures that the upload process halts immediately upon data integrity failure, preventing the application from signaling a successful upload when a mismatch occurs. If both checksums pass or are skipped, the process continues normally to cleanup.

shubham-up-47
shubham-up-47 reviewed Dec 17, 2025
View reviewed changes
shubham-up-47
shubham-up-47 previously approved these changes Dec 17, 2025
View reviewed changes
@thiyaguk09
refactor: streamline checksum and resumable upload logic
bd8616a 
Simplify `HashStreamValidator._flush` by utilizing `md5Digest` getter.
@shubham-up-47 shubham-up-47 self-requested a review December 18, 2025 05:30
@thiyaguk09
Copy link
Contributor Author

thiyaguk09 commented Dec 19, 2025

@ddelgrosso1 Just a quick reminder to take a look at this PR when you get a chance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ddelgrosso1 ddelgrosso1 Awaiting requested review from ddelgrosso1

1 more reviewer

@shubham-up-47 shubham-up-47 shubham-up-47 approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

api: storage Issues related to the googleapis/nodejs-storage API. size: l Pull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thiyaguk09 @ddelgrosso1 @shubham-up-47