Skip to content

fix: remove unsupported Web Crypto MD5 implementation#151

Open
YougLin-dev wants to merge 6 commits intoabhay-ramesh:mainfrom
YougLin-dev:fix/md5-bug
Open

fix: remove unsupported Web Crypto MD5 implementation#151
YougLin-dev wants to merge 6 commits intoabhay-ramesh:mainfrom
YougLin-dev:fix/md5-bug

Conversation

@YougLin-dev
Copy link

@YougLin-dev YougLin-dev commented Jan 21, 2026

Summary

  • Web Crypto API (\crypto.subtle.digest) does not support MD5 algorithm (only SHA-1/256/384/512)
  • The previous code would throw an error in browser environments when calling \calculateMD5()\
  • Since batch delete operations require S3 credentials, they should only run on the server anyway
  • Now always using Node.js \crypto.createHash('md5')\

Test plan

  • Verified fix works in test project using \pnpm pack\ installation
  • Batch delete operations still work correctly on server-side

Summary by CodeRabbit

  • Refactor
    • Simplified and unified storage hashing behavior to improve reliability and consistency of storage operations across environments, reducing environment-specific branching and potential inconsistencies.

✏️ Tip: You can customize this high-level summary in your review settings.

Web Crypto API does not support MD5 algorithm (only SHA-1/256/384/512). The previous code would fail in browser environments.

Since batch delete operations require S3 credentials, they should only run on the server anyway. Now always using Node.js crypto.createHash.
@vercel
Copy link
Contributor

vercel bot commented Jan 21, 2026

@YougLin-dev is attempting to deploy a commit to the Abhay's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link

coderabbitai bot commented Jan 21, 2026

📝 Walkthrough

Walkthrough

This change simplifies MD5 generation in the storage client by removing browser/Web Crypto and dynamic-import branches; calculateMD5 now always uses Node.js crypto to produce a base64 MD5 digest and includes comments clarifying Web Crypto MD5 is unsupported and batch delete runs server-side.

Changes

Cohort / File(s) Summary
MD5 Calculation Simplification
packages/pushduck/src/core/storage/client.ts
Removed browser/Web Crypto and dynamic import branches from calculateMD5; now unconditionally imports Node.js crypto and computes base64 MD5. Added clarifying comments about Web Crypto MD5 being unsupported and server-side batch delete execution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I nibbled branches, cleared the maze of code,
One steady tunnel now where bytes unload,
Node hums the tune and MD5 skips a beat,
Clean paws, quick hop — the change is neat! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description covers the problem, solution, and testing, but omits several template sections including type of change, configuration changes, documentation, and other required checklist items. Complete the PR description by filling in the template sections: mark the type of change (bug fix), indicate testing performed, document any security/bundle implications, and complete the pre-release checklist.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: removing Web Crypto MD5 implementation that was unsupported.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/pushduck/src/core/storage/client.ts 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants