Add PDF 2.0 AES-256 password encryption#9
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds PDF 2.0 revision 6 AES-256 password encryption for buffered and streamed output, including cryptographic primitives, R6 material generation, encrypted serialization, public API integration, validation scripts, benchmarks, runtime documentation, and Node 20 support. ChangesPDF encryption
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/encryption/r6.ts (1)
74-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGenerated owner credential bypasses
preparePassword/SASLprep.The design doc (docs/design/encryption.md lines 180-189) states the internally generated owner credential "is prepared like any other owner password," but here it's passed straight from
generatedOwnerPasswordintor6Hash/aesCbcEncryptNoPaddingwithout going throughpreparePassword. Currently harmless (the base64url alphabet is untouched by SASLprep), but it silently diverges from the documented contract and would break invisibly if the alphabet/generation strategy ever changes.♻️ Route the generated credential through preparePassword
const ownerPassword = options.ownerPassword === undefined - ? generatedOwnerPassword(random) + ? preparePassword(new TextDecoder().decode(generatedOwnerPassword(random)), "ownerPassword") : preparePassword(options.ownerPassword, "ownerPassword");As per docs/design/encryption.md: encoded as a standards-valid printable-ASCII password within the R6 byte limit... It is prepared like any other owner password, used only to derive O and OE, then discarded with the other ephemeral secret material
Also applies to: 111-114
🤖 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/encryption/r6.ts` around lines 74 - 91, Route the value returned by generatedOwnerPassword through preparePassword before using it in the R6 owner-password derivation flow, including the corresponding path at the other noted call site. Ensure the prepared bytes, rather than the raw generated credential, are passed to r6Hash and aesCbcEncryptNoPadding, while preserving cleanup of ephemeral secret material.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/encryption/r6.ts`:
- Around line 122-167: Update the finally block in the key-derivation function
to zeroize fileKey when the try block fails, while preserving it for successful
returns because the caller needs the returned key. Ensure the cleanup covers the
existing password and permissions buffers without changing the successful
encryption flow.
In `@src/encryption/writer.ts`:
- Around line 200-224: Update saveEncryptedPdf to honor updateFieldAppearances
before pdf.flush(), using pdf-lib’s existing appearance-update mechanism and the
destructured option (including its default true). Ensure field appearances are
updated before prepareEncryption and either encrypted writer path runs.
- Around line 161-224: Serialize encrypted saves per PDFDocument so only one
save can execute the prepare, write, and cleanup sequence at a time. Add a
per-document guard used by saveEncryptedPdf around prepareEncryption and the
writer calls, and release it in all completion and failure paths. Preserve the
existing cleanup behavior while ensuring concurrent calls cannot overlap trailer
mutation or encryption output.
In `@src/stream.ts`:
- Around line 106-119: Move the encryption setup in the stream flow, including
prepareEncryption and encryptedFrame assignment, inside the existing try/finally
cleanup scope so failures during preparation or pre-measurement always restore
the document state. Preserve normal encryption behavior, and add a regression
test confirming setup failure removes /Encrypt, its registered dictionary, and
replacement file IDs from the PDF.
---
Nitpick comments:
In `@src/encryption/r6.ts`:
- Around line 74-91: Route the value returned by generatedOwnerPassword through
preparePassword before using it in the R6 owner-password derivation flow,
including the corresponding path at the other noted call site. Ensure the
prepared bytes, rather than the raw generated credential, are passed to r6Hash
and aesCbcEncryptNoPadding, while preserving cleanup of ephemeral secret
material.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a51a4d13-16a1-4ca8-b63a-9fbb93d28310
📒 Files selected for processing (25)
.gitignoreREADME.mddocs/design/encryption.mdpackage.jsonscripts/bench-memory.tsscripts/bench-worker.tsscripts/generate-saslprep-tables.pyscripts/smoke-encryption-runtimes.tsscripts/verify-encryption.mjssrc/cli.tssrc/document.tssrc/encryption/permissions.tssrc/encryption/r6.tssrc/encryption/saslprep-tables.tssrc/encryption/saslprep.tssrc/encryption/serialize.tssrc/encryption/types.tssrc/encryption/web-crypto.tssrc/encryption/writer.tssrc/index.tssrc/save.tssrc/stream.tstest/encryption/core.test.tstest/encryption/writer.test.tstest/stream.test.ts
|
Addressed the review in 207bc82:
Validation: 199 tests passed, |
What changed
flowToPdf,renderToPdf, caller-ownedsavePdf, andstreamFlowUser impact
Calling
pdf.save()remains unencrypted; caller-owned documents usesavePdf(pdf, { encryption }).Validation
pnpm test— 18 files, 195 testspnpm run typecheckpnpm run build— ESM, CJS, and declarationsThe design document remains explicit that Acrobat/Firefox manual checks and final security review are release gates.
Summary by CodeRabbit
streamFlow).savePdffor saving caller-owned documents with optional password/owner password and configurable permissions and metadata encryption.pdf.save()remains unencrypted.