Skip to content

fix: implement HMAC-SHA256 signature verification for OpenZeppelin webhook#34

Open
memosr wants to merge 1 commit into
circlefin:masterfrom
memosr:fix/oz-webhook-hmac-verification
Open

fix: implement HMAC-SHA256 signature verification for OpenZeppelin webhook#34
memosr wants to merge 1 commit into
circlefin:masterfrom
memosr:fix/oz-webhook-hmac-verification

Conversation

@memosr
Copy link
Copy Markdown

@memosr memosr commented Apr 12, 2026

Problem

The OpenZeppelin Relayer webhook endpoint (app/api/openzepellin/webhook/route.ts) accepts any POST request without verifying its origin. The code even has a comment admitting this:

// In production, you would verify the signature from the relayer
// using the WEBHOOK_SIGNING_KEY you configured.

This means anyone can POST to this endpoint and forge transaction confirmations, triggering false status updates for arbitrary transactions.

Fix

Implemented HMAC-SHA256 signature verification using Node's built-in crypto module:

  • Raw body is read as text (required for stable HMAC computation)
  • verifySignature() computes HMAC-SHA256 using OZ_WEBHOOK_SIGNING_KEY env var
  • Signature compared with timingSafeEqual to prevent timing attacks
  • Returns 401 if key is missing, header is absent, or signature is invalid
+ import { createHmac, timingSafeEqual } from "crypto";
+
+ async function verifySignature(req, rawBody) {
+   const signingKey = process.env.OZ_WEBHOOK_SIGNING_KEY;
+   const signature = req.headers.get("x-openzeppelin-signature");
+   const expected = createHmac("sha256", signingKey).update(rawBody).digest("hex");
+   return timingSafeEqual(Buffer.from(signature), Buffer.from(expected));
+ }
+
+ const rawBody = await req.text();
+ if (!(await verifySignature(req, rawBody))) {
+   return NextResponse.json({ error: "Invalid signature" }, { status: 401 });
+ }

Impact

  • Security: Prevents forged webhook requests from triggering false transaction confirmations
  • Risk: Low — requires OZ_WEBHOOK_SIGNING_KEY to be set in environment variables

@Cassxbt
Copy link
Copy Markdown

Cassxbt commented May 12, 2026

Good direction overall: reading the raw body before parsing and using timingSafeEqual are the right shape for this verification.

One blocking mismatch with the OpenZeppelin Relayer implementation: the relayer sends the HMAC in the X-Signature header, and the signature is base64-encoded, not hex-encoded.

Source: OpenZeppelin Relayer computes STANDARD.encode(code_bytes) after HMAC-SHA256 here:
https://github.com/OpenZeppelin/openzeppelin-relayer/blob/cf190cf62350b690503dad7e06c4bcfaa3322717/src/services/notification/mod.rs#L63-L74

And sends that value as X-Signature here:
https://github.com/OpenZeppelin/openzeppelin-relayer/blob/cf190cf62350b690503dad7e06c4bcfaa3322717/src/services/notification/mod.rs#L86-L89

This PR currently reads x-openzeppelin-signature and computes .digest("hex"), so valid Relayer webhook notifications would fail verification. A safer implementation would read X-Signature, decode it from base64, and compare it against the raw HMAC bytes, e.g. createHmac(...).update(rawBody).digest() with the same length check before timingSafeEqual. A small regression test with a known payload/secret pair would also lock this behavior down.

@memosr
Copy link
Copy Markdown
Author

memosr commented May 12, 2026

Thanks for the thorough review, @Cassxbt

Both issues addressed in 9a93290

  • Header changed from x-openzeppelin-signature - X-Signature
  • Signature now decoded from base64 with explicit length check before timingSafeEqual
  • HMAC uses raw bytes (.digest()) instead of hex
  • Extracted verification logic into a pure checkSignature() function in a separate module for testability
  • Added 5 unit tests in route.test.ts covering: valid signature, wrong secret, tampered body, old hex format rejection, and empty/malformed header

All tests pass. Ready for another look!

@Cassxbt
Copy link
Copy Markdown

Cassxbt commented May 12, 2026

Re-checked the updated implementation.

The original OpenZeppelin Relayer mismatch is resolved: the route now reads X-Signature, and checkSignature() compares the raw HMAC-SHA256 bytes against the base64-decoded header with a length check before timingSafeEqual.

One remaining test integration issue: I don’t see a runnable test command in package.json, and running the added node:test file directly with Node 22 fails on the extensionless TypeScript import:

ERR_MODULE_NOT_FOUND: Cannot find module .../verify-signature

So the implementation concern is fixed, but the regression test should be wired into a runnable command or adjusted for the repo’s test runner/module resolution. That way future maintainers can actually run the coverage you added.

…bhook

- Read X-Signature header and base64-decode the signature (matches OpenZeppelin Relayer spec)
- Use timingSafeEqual with explicit length check on raw HMAC bytes
- Extract verification logic into pure checkSignature() module for testability
- Add 5 unit tests covering valid sig, wrong secret, tampered body, hex format, empty header
- Add npm test script using node:test with tsx loader
@memosr memosr force-pushed the fix/oz-webhook-hmac-verification branch from 9a93290 to 8ceead1 Compare May 12, 2026 19:55
@memosr
Copy link
Copy Markdown
Author

memosr commented May 12, 2026

Thanks

Test integration fixed in 8ceead1

  • Added "test" script to package.json: node --import tsx --test app/api/openzepellin/webhook/route.test.ts
  • Added tsx@^4.19.4 to devDependencies (~1MB loader, no heavy test framework needed since the test file uses Node's built-in node:test runner)
  • The --import tsx flag registers tsx as a loader before the file is parsed, resolving the extensionless TypeScript import issue on Node 22

Confirmed npm test runs all 5 tests successfully:

1- accepts a valid signature
2- rejects a signature produced with the wrong secret
3- rejects a signature when the body has been tampered
4- rejects a hex-encoded signature
5- rejects an empty signature header ℹ tests 5 pass

I also rebased the branch onto the latest master so the diff is now clean (no stale package-lock.json changes). ready for another look

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.

2 participants