Skip to content

fix: add Content-MD5 header for Object Lock compliance#41241

Merged
NilanshBansal merged 1 commit intoreleasefrom
fix/issue-41240/s3-content-md5-header
Sep 22, 2025
Merged

fix: add Content-MD5 header for Object Lock compliance#41241
NilanshBansal merged 1 commit intoreleasefrom
fix/issue-41240/s3-content-md5-header

Conversation

@NilanshBansal
Copy link
Contributor

@NilanshBansal NilanshBansal commented Sep 19, 2025

Description

Problem:
Users were unable to upload files to S3 buckets with Object Lock enabled, receiving the error:

InvalidRequest: Content-MD5 OR x-amz-checksum- HTTP header is required for Put Object requests with Object Lock parameters

Root Cause:
The S3 plugin's uploadFileInS3 method was not setting the required Content-MD5 header that AWS mandates for uploads to Object Lock-enabled buckets to ensure data integrity.

Solution:

  • Modified uploadFileInS3 method to calculate MD5 checksum of file payload
  • Automatically set Content-MD5 header in ObjectMetadata using Base64-encoded MD5 hash
  • Added graceful error handling with fallback behavior
  • Added content length setting for better upload handling
  • Included test to verify MD5 calculation correctness

Fixes #41240

Automation

/ok-to-test tags="@tag.Sanity"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/17908934321
Commit: e2577b1
Cypress dashboard.
Tags: @tag.Sanity
Spec:


Mon, 22 Sep 2025 09:22:02 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of Amazon S3 uploads by setting accurate content length and adding an integrity checksum (MD5), reducing chances of corrupted or rejected uploads.
    • Enhanced compatibility with stricter S3 configurations, including Object Lock and checksum validation.
    • Better diagnostic logging during uploads for easier troubleshooting.
  • Tests

    • Added unit tests to validate MD5 checksum generation used for upload integrity checks.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Adds pre-upload MD5 computation and Content-Length setting in AmazonS3 upload flow. Sets ObjectMetadata ContentMD5 (Base64) and logs it; falls back without MD5 if algorithm unavailable. Introduces a unit test verifying MD5 hashing and Base64 encoding utilities.

Changes

Cohort / File(s) Summary
S3 upload metadata update
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Compute MD5 of payload, Base64-encode, set as ContentMD5; set ContentLength; log MD5; warn and continue if MD5 algorithm unavailable. No API signature changes.
S3 MD5 unit tests
app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
Add test validating MD5 and Base64 calculation; introduce private helpers bytesToHex and hexToBytes; update imports.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AmazonS3Plugin
  participant AWS_S3

  Client->>AmazonS3Plugin: uploadFileInS3(payload, metadata)
  activate AmazonS3Plugin
  AmazonS3Plugin->>AmazonS3Plugin: Compute MD5(payload)
  alt MD5 available
    AmazonS3Plugin->>AmazonS3Plugin: Set metadata.ContentMD5 (Base64)\nSet metadata.ContentLength
  else MD5 unavailable
    Note over AmazonS3Plugin: Log warning\nProceed without ContentMD5
    AmazonS3Plugin->>AmazonS3Plugin: Set metadata.ContentLength
  end
  AmazonS3Plugin->>AWS_S3: PutObject(payload, metadata)
  AWS_S3-->>AmazonS3Plugin: Response
  deactivate AmazonS3Plugin
  AmazonS3Plugin-->>Client: Result
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A checksum hums before the flight,
Bytes align, their lengths set right.
If MD5 sleeps, we won’t bemoan—
We log, proceed, upload is flown.
S3 nods with guarded grin,
Object Lock lets data in.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: add Content-MD5 header for Object Lock compliance" concisely and accurately summarizes the primary change: adding a Content-MD5 header to S3 uploads to satisfy Object Lock requirements, which directly maps to the uploadFileInS3 changes that compute and set a Base64-encoded MD5 and content length. The phrasing is specific, follows conventional commit style, and is useful for teammates scanning history. No misleading or off-topic wording is present.
Linked Issues Check ✅ Passed The changes directly implement the core coding objective from issue #41240 by computing the MD5 of the payload, Base64-encoding it, and setting ObjectMetadata.setContentMD5 (plus ContentLength) before upload, and a unit test verifying the MD5 calculation was added; this satisfies the integrity-header requirement for PutObject requests with Object Lock. The PR does not add alternative x-amz-checksum header variants nor include explicit backport/release actions for older versions, so those operational steps remain to be addressed outside this code change if required. Overall, the code changes meet the primary linked-issue coding requirements.
Out of Scope Changes Check ✅ Passed The changes are scoped to the S3 plugin upload logic and corresponding unit test only; there are no modifications to exported/public signatures or unrelated modules in the provided summary, so no out-of-scope code changes were detected. Imports and helper logic added are directly relevant to the new MD5 functionality.
Description Check ✅ Passed The PR description clearly explains the problem, root cause, and implemented solution (MD5 calculation, Base64-encoding, setting Content-MD5 and ContentLength, and graceful fallback) and explicitly references Fixes #41240. It includes an /ok-to-test automation tag, the auto-generated Cypress CI placeholder, and documents the added unit test that validates the MD5 calculation. Overall the description aligns with the repository template and provides sufficient motivation and context for reviewers to evaluate the change.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/issue-41240/s3-content-md5-header

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added Bug Something isn't working Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Integrations Product Issues related to a specific integration Needs Triaging Needs attention from maintainers to triage Query Widgets & IDE Pod All issues related to Query, JS, Eval, Widgets & IDE labels Sep 19, 2025
@NilanshBansal
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/17859885587.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 41241.
recreate: .

@github-actions
Copy link

Deploy-Preview-URL: https://ce-41241.dp.appsmith.com

@NilanshBansal NilanshBansal added the ok-to-test Required label for CI label Sep 22, 2025
@NilanshBansal NilanshBansal enabled auto-merge (squash) September 22, 2025 08:12
@NilanshBansal NilanshBansal merged commit 36c34db into release Sep 22, 2025
80 of 82 checks passed
@NilanshBansal NilanshBansal deleted the fix/issue-41240/s3-content-md5-header branch September 22, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Integrations Product Issues related to a specific integration Needs Triaging Needs attention from maintainers to triage ok-to-test Required label for CI Query Widgets & IDE Pod All issues related to Query, JS, Eval, Widgets & IDE

Projects

None yet

2 participants