fix(webhooks): accept raw request bytes for signature verification#1578
fix(webhooks): accept raw request bytes for signature verification#1578
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSignature verification and construction now accept a typed WebhookPayload (string, bytes, or object). A new decode utility converts payloads to a deterministic UTF‑8 string; signatures are computed over Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Widens constructEvent/verifyHeader payload to string | Uint8Array | ArrayBuffer | object. Raw-bytes paths HMAC the exact bytes the server signed; object path keeps the legacy JSON.stringify behavior for back-compat. constructEvent now parses after verification on the raw paths, matching the pattern used by Stripe, GitHub, and Svix. Hardening only — the object path remains unsafe to on-the-wire mutations that round-trip through JSON.parse/JSON.stringify to the same canonical form (whitespace, key order, unicode escapes, duplicate keys). Callers should migrate to passing the raw body.
nicknisi
force-pushed
the
fix/webhook-signature-raw-bytes
branch
from
0efe598 to
d16dbdf
Compare
Greptile SummaryThis PR widens Confidence Score: 5/5Safe to merge; all findings are P2 style/edge-case concerns with no present runtime defects on normal payloads. No P0 or P1 issues found. The BOM + JSON.parse concern is a degenerate edge case (server signing BOM-prefixed JSON is non-standard) and is P2. The branching style inconsistency between webhooks.ts and actions.ts is cosmetic. All previous P1 reviewer concerns are resolved. src/common/crypto/decode-payload.ts — BOM handling in the post-verification parse path. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/webhooks/webhooks.ts (1)
9-26: ⚡ Quick winUnify payload decoding/signable conversion to a single shared helper.
Line 9-26 duplicates raw-byte decoding logic already present in
src/common/crypto/signature-provider.ts(Line 98-114). Keeping two implementations risks future drift between “what gets signed” and “what gets parsed.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/webhooks.ts` around lines 9 - 26, The parseVerifiedPayload function duplicates raw-byte decoding logic from signature-provider.ts; extract a single shared helper (e.g., decodePayloadToString or normalizeSignablePayload) that handles string | Uint8Array | ArrayBuffer | Record<string, unknown> and returns a decoded JSON string or parsed object consistently, then replace parseVerifiedPayload with a thin wrapper that uses that helper and update signature-provider.ts to call the same helper; ensure the helper preserves the existing behavior for TextDecoder('utf-8') and returns/throws the same types (EventResponse parsing remains in parseVerifiedPayload or the wrapper), and add the helper to a common utility module and import it from both webhooks.ts and signature-provider.ts.src/webhooks/webhooks.spec.ts (1)
214-334: ⚡ Quick winAdd one direct
ArrayBuffertest for parity with new runtime branches.This suite validates
string/Buffer/Uint8Array, but notArrayBuffer, while bothparseVerifiedPayloadandtoSignableStringinclude explicitArrayBufferbranches.📌 Minimal test addition
+ it('verifies when payload is an ArrayBuffer matching the signed bytes', async () => { + const rawBody = JSON.stringify(mockWebhook); + const hash = signRaw(rawBody, timestamp, secret); + const sigHeader = `t=${timestamp}, v1=${hash}`; + const arrayBuffer = new TextEncoder().encode(rawBody).buffer; + + const webhook = await workos.webhooks.constructEvent({ + payload: arrayBuffer, + sigHeader, + secret, + }); + + expect(webhook.id).toEqual('wh_123'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/webhooks.spec.ts` around lines 214 - 334, The test suite for raw-byte payloads is missing an ArrayBuffer case which leaves the ArrayBuffer branches in parseVerifiedPayload and toSignableString untested; add a new it block similar to "verifies when payload is a Uint8Array matching the signed bytes" that builds signedBytes = JSON.stringify(mockWebhook), computes hash via signRaw(signedBytes, timestamp, secret), constructs an ArrayBuffer from the raw string (e.g., via TextEncoder().encode(...).buffer or Buffer.from(...).buffer), calls workos.webhooks.constructEvent with payload set to that ArrayBuffer and the computed sigHeader/secret, and assert webhook.id === 'wh_123' to ensure the ArrayBuffer path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/webhooks/webhooks.spec.ts`:
- Around line 214-334: The test suite for raw-byte payloads is missing an
ArrayBuffer case which leaves the ArrayBuffer branches in parseVerifiedPayload
and toSignableString untested; add a new it block similar to "verifies when
payload is a Uint8Array matching the signed bytes" that builds signedBytes =
JSON.stringify(mockWebhook), computes hash via signRaw(signedBytes, timestamp,
secret), constructs an ArrayBuffer from the raw string (e.g., via
TextEncoder().encode(...).buffer or Buffer.from(...).buffer), calls
workos.webhooks.constructEvent with payload set to that ArrayBuffer and the
computed sigHeader/secret, and assert webhook.id === 'wh_123' to ensure the
ArrayBuffer path is exercised.
In `@src/webhooks/webhooks.ts`:
- Around line 9-26: The parseVerifiedPayload function duplicates raw-byte
decoding logic from signature-provider.ts; extract a single shared helper (e.g.,
decodePayloadToString or normalizeSignablePayload) that handles string |
Uint8Array | ArrayBuffer | Record<string, unknown> and returns a decoded JSON
string or parsed object consistently, then replace parseVerifiedPayload with a
thin wrapper that uses that helper and update signature-provider.ts to call the
same helper; ensure the helper preserves the existing behavior for
TextDecoder('utf-8') and returns/throws the same types (EventResponse parsing
remains in parseVerifiedPayload or the wrapper), and add the helper to a common
utility module and import it from both webhooks.ts and signature-provider.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84f1affd-d544-4f0c-ad15-3440f048dd9d
📒 Files selected for processing (3)
src/common/crypto/signature-provider.tssrc/webhooks/webhooks.spec.tssrc/webhooks/webhooks.ts
TextDecoder strips a leading UTF-8 BOM (0xEF 0xBB 0xBF) by default.
An attacker could prepend BOM bytes to a Buffer/Uint8Array payload and
pass signature verification because the HMAC was computed on the
BOM-stripped string. Use { ignoreBOM: true } to preserve all bytes.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/webhooks/webhooks.spec.ts (1)
214-262: ⚡ Quick winPlease add an
ArrayBuffercase here too.Both
parseVerifiedPayloadinsrc/webhooks/webhooks.ts:20-25andtoSignableStringinsrc/common/crypto/signature-provider.ts:112-115have a separateArrayBufferbranch, but this suite only exercisesstring,Buffer, andUint8Array. One small positive test would close the gap on the last new public payload type.Example test shape
+ it('verifies when payload is an ArrayBuffer matching the signed bytes', async () => { + const rawBody = JSON.stringify(mockWebhook); + const hash = signRaw(rawBody, timestamp, secret); + const sigHeader = `t=${timestamp}, v1=${hash}`; + const bytes = new TextEncoder().encode(rawBody); + + const webhook = await workos.webhooks.constructEvent({ + payload: bytes.buffer.slice( + bytes.byteOffset, + bytes.byteOffset + bytes.byteLength, + ), + sigHeader, + secret, + }); + + expect(webhook.id).toEqual('wh_123'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/webhooks.spec.ts` around lines 214 - 262, Add a fourth spec that mirrors the existing "Uint8Array" test but passes an ArrayBuffer payload to workos.webhooks.constructEvent so the ArrayBuffer branch in parseVerifiedPayload and toSignableString is covered; use the same rawBody = JSON.stringify(mockWebhook), compute hash with signRaw(rawBody, timestamp, secret), build sigHeader = `t=${timestamp}, v1=${hash}`, create bytes via new TextEncoder().encode(rawBody).buffer (or bytes.buffer) and call constructEvent({ payload: arrayBuffer, sigHeader, secret }) and assert webhook.id === 'wh_123'.src/webhooks/webhooks.ts (1)
9-28: ⚡ Quick winExtract the raw-payload decoding into one shared helper.
parseVerifiedPayloadnow re-implements the sameUint8Array/ArrayBuffer/ BOM handling astoSignableStringinsrc/common/crypto/signature-provider.ts:98-118. Since these two paths define what gets verified and what gets parsed afterward, letting them drift will recreate signing/parsing mismatches. Please move the decode logic to a single shared helper and reuse it from both sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/webhooks.ts` around lines 9 - 28, The payload decoding logic duplicated between parseVerifiedPayload and toSignableString should be extracted into a single helper (e.g., decodeRawPayload or normalizePayloadForSigning) placed in the shared common/crypto module; implement the helper to accept string | Uint8Array | ArrayBuffer | Record<string, unknown>, return either the original string or a UTF-8 decoded string from Uint8Array/ArrayBuffer using TextDecoder with ignoreBOM:true, and preserve passing through Record inputs; then replace the decoding branches inside parseVerifiedPayload and the toSignableString implementation in signature-provider.ts to call this helper so both verification and parsing use the identical BOM-safe decoding path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/webhooks/webhooks.spec.ts`:
- Around line 214-262: Add a fourth spec that mirrors the existing "Uint8Array"
test but passes an ArrayBuffer payload to workos.webhooks.constructEvent so the
ArrayBuffer branch in parseVerifiedPayload and toSignableString is covered; use
the same rawBody = JSON.stringify(mockWebhook), compute hash with
signRaw(rawBody, timestamp, secret), build sigHeader = `t=${timestamp},
v1=${hash}`, create bytes via new TextEncoder().encode(rawBody).buffer (or
bytes.buffer) and call constructEvent({ payload: arrayBuffer, sigHeader, secret
}) and assert webhook.id === 'wh_123'.
In `@src/webhooks/webhooks.ts`:
- Around line 9-28: The payload decoding logic duplicated between
parseVerifiedPayload and toSignableString should be extracted into a single
helper (e.g., decodeRawPayload or normalizePayloadForSigning) placed in the
shared common/crypto module; implement the helper to accept string | Uint8Array
| ArrayBuffer | Record<string, unknown>, return either the original string or a
UTF-8 decoded string from Uint8Array/ArrayBuffer using TextDecoder with
ignoreBOM:true, and preserve passing through Record inputs; then replace the
decoding branches inside parseVerifiedPayload and the toSignableString
implementation in signature-provider.ts to call this helper so both verification
and parsing use the identical BOM-safe decoding path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ad3e0ca-41d2-4f16-81ee-aaced90ffba5
📒 Files selected for processing (3)
src/common/crypto/signature-provider.tssrc/webhooks/webhooks.spec.tssrc/webhooks/webhooks.ts
Addresses PR review feedback: - Remove `| unknown` from verifyHeader/computeSignature unions so TypeScript actually enforces the payload type at call sites. - Extract `decodePayloadToString` and `WebhookPayload` type into a shared module to eliminate duplicated decode logic between signature-provider.ts and webhooks.ts. - Add missing ArrayBuffer test case. - Fix actions.ts call sites surfaced by the tighter types.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
constructAction now accepts WebhookPayload (string | Uint8Array | ArrayBuffer | Record) but was casting directly to ActionPayload without parsing. Raw-bytes payloads would pass verification but silently fail deserialization. Parse via decodePayloadToString + JSON.parse for non-object payloads, matching the webhook pattern.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace instanceof Uint8Array/ArrayBuffer checks with ArrayBuffer.isView() and Object.prototype.toString.call() to handle payloads from different JS realms (Workers, iframes, VM contexts). Extract shared isBinaryPayload() helper into decode-payload.ts and use it consistently in webhooks.ts and actions.ts. Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
2 participants
Summary
constructEventandverifyHeaderpayload type to acceptstring | Uint8Array | ArrayBufferin addition to parsed objectsJSON.stringifybehavior for back-compatconstructEventparses after verification on raw-bytes pathsdecodePayloadToStringhelper ensures signing and parsing always use identical decodingArrayBuffer.isView/Object.prototype.toString.call) instead ofinstanceofchecks, so payloads from Workers, iframes, or VM contexts are handled correctlyTest plan
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
WorkOS Docs webhook snippets need updating to pass raw request bodies instead of
JSON.parse-ing beforeconstructEvent(separate docs PR).Link to Devin session: https://app.devin.ai/sessions/1170d117f26f431bbc866077728682f6
Requested by: @nicknisi