Skip to content

fix: the application uses md5 (crypto in helper.ts #1491

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

Open
anupamme wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: the application uses md5 (crypto in helper.ts #1491

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e32c3c7
fix: the application uses md5 (crypto in helper.ts
anupamme Jul 30, 2026
efac9f5
Address review feedback (2 comments)
anupamme Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/internal/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,9 +428,11 @@ export function sanitizeETag(etag = ''): string {
}

export function toMd5(payload: Binary): string {
// use string from browser and buffer from nodejs
// browser support is tested only against minio server
return crypto.createHash('md5').update(Buffer.from(payload)).digest().toString('base64')
// MD5 is intentionally used here solely for the Content-MD5 header required by
// the S3 protocol specification (RFC 1864). It is NOT used for cryptographic
// security or authentication — transport integrity is enforced by TLS.

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Correct the Content-MD5 protocol wording.

Content-MD5 is not universally required by S3; AWS documents it as optional except for specific request types. It is also an end-to-end payload integrity check, so the TLS statement could misleadingly imply that TLS replaces this header. (docs.aws.amazon.com)

-  // MD5 is intentionally used here solely for the Content-MD5 header required by
-  // the S3 protocol specification (RFC 1864). It is NOT used for cryptographic
-  // security or authentication — transport integrity is enforced by TLS.
+  // MD5 is used here solely to generate the Base64 Content-MD5 value for S3
+  // requests that use this legacy payload-integrity header. It is not used for
+  // cryptographic security or authentication.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// MD5 is intentionally used here solely for the Content-MD5 header required by
// the S3 protocol specification (RFC 1864). It is NOT used for cryptographic
// security or authentication — transport integrity is enforced by TLS.
// MD5 is used here solely to generate the Base64 Content-MD5 value for S3
// requests that use this legacy payload-integrity header. It is not used for
// cryptographic security or authentication.
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/internal/helper.ts` around lines 431 - 433, Update the explanatory
comment near the Content-MD5 implementation to state that the header is optional
for most S3 requests and required only for applicable request types, while
describing it as an end-to-end payload integrity check. Remove the implication
that TLS replaces or enforces the header, and retain that MD5 is not used for
cryptographic security or authentication.

// nosemgrep: use-of-md5
return crypto.createHash('md5').update(Buffer.from(payload)).digest().toString('base64') // nosec
}

export function toSha256(payload: Binary): string {
Expand Down
Loading