Wire AES-GCM encryption into serialization layer#1251
Merged
TooTallNate merged 29 commits intomainfrom Mar 4, 2026
Merged
Conversation
Contributor
🦋 Changeset detectedLatest commit: bc01397 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (49 failed)mongodb (1 failed):
turso (48 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
…llers to import once per run
…rkflowRun in resume-hook
…ncrypted, complex type round-trips
…import once per run
…sts, fix stale comments
The rebase incorrectly picked up older versions of these files from early encryption branch commits. The main versions are correct and up-to-date.
- Remove Vercel-specific error message from maybeDecrypt (core should not reference VERCEL_DEPLOYMENT_KEY) - Move stream encryption/decryption from transport layer (WorkflowServerReadableStream/WritableStream) to framing layer (getSerializeStream/getDeserializeStream). Frame length headers stay in the clear so frame boundaries are always parseable regardless of transport chunking; encryption wraps the frame payload. - Remove explicit Promise<unknown> return types from all 4 hydrate functions. On main these had inferred types (any from devalue), so callers didn't need casts. The encryption branch added explicit annotations that broke this. - Revert unnecessary type casts in run.ts, step-handler.ts, hook.ts that were only needed due to the explicit Promise<unknown> annotations - Revert closureVars type from unknown back to Record<string, any> in context-storage.ts to match the contract with getClosureVars - Fix hydrateWorkflowArguments JSDoc for unused _runId parameter
…gacy comments - Remove unused _runId parameter from WorkflowServerReadableStream constructor and all 4 call sites - Deduplicate processFrames decryption: decrypt first and reassign format/payload, then fall through to single deserialization path - Add comments on all legacy non-Uint8Array branches explaining when this happens (specVersion 1 runs stored data as plain JSON arrays) - Fix duplicate code block in hydrateStepReturnValue
Thread the encryption key through the entire stream serialization chain so that ReadableStream and WritableStream values are encrypted/decrypted at the framing level. - Add optional cryptoKey param to getExternalReducers, getStepReducers, getExternalRevivers, getStepRevivers - Pass cryptoKey to getSerializeStream/getDeserializeStream at all 8 internal call sites within reducers/revivers - Thread key from dehydrate/hydrate functions into their reducers/revivers - Cache encryption key in Run class (resolved once via getEncryptionKey(), reused for returnValue, getReadable(), etc.) - Make Run#getReadable() async to resolve the cached key before creating the deserialize stream - Add encryptionKey to step context storage so getWritable() can access it during step execution
… add stream encryption tests Change cryptoKey parameter from optional (cryptoKey?) to required-but- nullable (cryptoKey: CryptoKey | undefined) on all 6 functions: - getSerializeStream, getDeserializeStream - getExternalReducers, getStepReducers - getExternalRevivers, getStepRevivers This ensures every call site must explicitly pass the key or undefined, making it impossible to accidentally omit it and silently skip encryption. Add 7 stream encryption round-trip tests: - Encrypted frames have 'encr' prefix inside length header - Full round-trip: encrypt serialize -> decrypt deserialize - Concatenated encrypted frames (transport coalescing) - Split encrypted frames (transport splitting) - Error when encrypted data encountered without key - No encryption when key is undefined - Large payload round-trip Full audit confirms all encryption key threading is complete: - All 8 dehydrate/hydrate functions pass key to reducers/revivers - All stream serialize/deserialize call sites pass key - Run class caches key for reuse across returnValue and getReadable() - Step context storage carries key for getWritable()
…reams - Revert Run#getReadable() to synchronous (non-breaking API). The encryption key is passed as a Promise through the chain and resolved lazily inside the first async transform() call. - Add EncryptionKeyParam type alias that accepts CryptoKey, undefined, or Promise<CryptoKey | undefined>. Used by getSerializeStream, getDeserializeStream, and all reducer/reviver functions. - Key promises are resolved once on first use via a keyState cache object inside each stream's transform closure. - Fix CLI showStream to resolve encryption key from world when runId is provided via --run flag, instead of passing undefined. - Remove incorrect CLI warning that --run is not supported for streams (it is now needed for encrypted stream decryption).
Member
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
Some issues claude flagged:
- Pre-existing: workflow.test.ts lines 3360, 3434, 3510, 3567 These 4 calls are dehydrateWorkflowArguments([], ops) — passing ops (a Promise[]) as the runId parameter and omitting key. This pre-dates this PR (the key param already existed on the base branch), but it's worth noting these tests are broken and should be fixed in this PR or a follow-up.
maybeDecryptthrows WorkflowRuntimeError when encrypted data has no key (serialization.ts:1527). This is the correct behavior, but the throw is unguarded in all hydrate callers. For example, in Run.pollReturnValue() (run.ts:206), if getEncryptionKey() resolves to undefined but the stored output was encrypted, hydrateWorkflowReturnValue will throw. This surfaces as a rejected promise on run.returnValue with no recovery path. The error message is good, but callers should be aware this can happen during a production key-rotation or misconfiguration scenario.- getDeserializeStream — controller.error() on encrypted-data-without-key (serialization.ts:346-355). When the deserialize stream encounters an encrypted frame but has no key, it calls controller.error() and returns. The writable side is not explicitly aborted — writers may hang or silently fail depending on timing. This is a minor edge case since it only happens in a misconfiguration scenario, but a writer.abort() or controller.terminate() would be more robust.
- decodeFormatPrefix throws plain Error on unknown format (serialization.ts:181). If a future format prefix (e.g., "cbor") is encountered by older code, decodeFormatPrefix throws Error: Unknown serialization format: "xxxx". This is a plain Error, not a WorkflowRuntimeError, making it harder to programmatically distinguish from other errors. All hydrate functions expose this throw path. Consider using WorkflowRuntimeError for consistency.
- Key-fetch rejection timing in streams. If the cryptoKey promise (passed as EncryptionKeyParam) rejects before the first stream frame arrives, the rejection is unobserved until data actually flows. This means a key-fetch failure (e.g., network error fetching the derived key) won't surface until the stream gets its first chunk. Not a bug, but worth documenting.
- Fix 4 broken dehydrateWorkflowArguments calls in workflow.test.ts that were passing ops as runId (missing runId and key params) - Use WorkflowRuntimeError instead of plain Error in decodeFormatPrefix for unknown serialization formats, for consistency and programmatic error handling - Document maybeDecrypt throw behavior: callers should be aware this surfaces as a rejected promise during key rotation/misconfiguration - Document key-fetch rejection timing in streams: promise rejection won't surface until the first chunk is processed
Member
Author
|
Addressed all 5 issues from the review in bc01397:
|
VaguelySerious
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wires AES-GCM encryption into the serialization layer, building on the async deserialization ordering fix from #1246.
Encryption
maybeEncrypt/maybeDecryptinserialization.tswithencr4-byte format prefixkey: CryptoKey | undefinedKey management
getEncryptionKeyForRunoverloaded to accept context forstart(), fetchWorkflowRunin resume-hookContinues from #957 (closed due to base branch deletion), with the EventsConsumer async ordering fix now landed via #1246.