feat(security): sign and verify inbound webhook callbacks (HMAC + timestamp) - #662
Open
TheWeirdDee wants to merge 1 commit into
Open
feat(security): sign and verify inbound webhook callbacks (HMAC + timestamp)#662TheWeirdDee wants to merge 1 commit into
TheWeirdDee wants to merge 1 commit into
Conversation
…estamp)
POST /api/sep24/callback — the endpoint an anchor uses to report deposit/
withdrawal status — accepted any unauthenticated POST body and applied it
directly (sep24Service.handleAnchorCallback), including flipping a
transaction to "completed". Any third party who could reach the endpoint
could forge that callback.
- New verifyInboundWebhookSignature(endpoint) middleware, applied to the
SEP-24 callback route. Requires X-Signature (hex HMAC-SHA256 of the raw
request body) and X-Timestamp (Unix seconds), in order:
1. both headers present
2. timestamp within ±WEBHOOK_REPLAY_WINDOW_SECONDS (default 300s) of now
3. signature matches HMAC-SHA256(secret, rawBody) for at least one of
the endpoint's active secrets, via crypto.timingSafeEqual (never ===)
4. this exact signature hasn't been used before (atomic replay-nonce
check, run last so a caller without the secret can't use it to
pollute the nonce store)
Rejections are a generic 401 — the response never reveals which check
failed.
- Needs the exact raw request bytes (not a JSON.stringify() of req.body,
which isn't guaranteed byte-identical to what was signed). bodyParsing.js
now captures this via express.json()'s verify callback onto req.rawBody,
exposed through a jsonBodyParser() factory so test apps use the identical
parser config as production.
- New inbound_webhook_secrets table + inboundWebhookSecretService.js:
per-endpoint secrets stored as AES-256-GCM ciphertext (never plaintext),
plus a keyed HMAC-SHA256 fingerprint — the same at-rest pattern already
used for outbound webhook secrets (webhookService.js). Multiple secrets
can be active per endpoint at once, which is what makes rotation with a
grace period possible: rotateSecret(endpoint, { graceSeconds }) keeps the
old secret valid for a bounded window instead of invalidating it the
instant the new one is generated, and revokeSecret() cuts a specific
secret off early regardless of any grace period.
- Extracted the keyed-hash helper (hashSecret/WEBHOOK_SECRET_KEY) out of
webhookService.js into utils/webhookSecretHash.js so it has no dependency
on @stellar/stellar-sdk, which webhookService.js only needs for unrelated
Horizon SSE streaming. webhookService.js re-exports both for backward
compatibility.
- Server startup provisions a secret for sep24_callback if one doesn't
exist yet (ensureSecretExists, idempotent) and logs it once so an
operator can configure it on the anchor's side without a separate manual
step; use rotateSecret()/revokeSecret() afterwards.
- Replay-nonce store reuses cacheService.js (Redis, with an always-on
in-memory LRU fallback) via a new setIfNotExists() — a single atomic
SET...NX operation, not a get()-then-set() with a race window.
- New AUTH_INVALID_WEBHOOK_SIGNATURE (401) error code in the shared
catalogue, consistent with the existing AUTH_* codes.
- 18 tests in backend/__tests__/webhookSignature.test.js (issue asked for
≥8): valid signature, missing X-Signature, missing X-Timestamp, malformed
signature, tampered body, wrong secret, stale timestamp, future
timestamp, non-numeric timestamp, replay, no active secret, generic error
body, plus secret-service coverage (never-plaintext storage, idempotent
bootstrap, rotation with/without grace period, early revocation,
metadata-only listing). Runs against a real Express app + a real
throwaway migrated SQLite DB, not mocks.
Also found, but did not fix (separate, unrelated, already broken on main):
webhookService.js's registerWebhook() inserts into a `secret` column that
migration 003_webhooks.js never created (only `secret_hash` exists) — the
outbound POST /api/webhooks registration path throws "no such column:
secret" against the real migrated schema. Reproduces on a clean checkout
with no changes from this branch; several existing tests
(webhookDeliveryRetry.test.js) already fail because of it. Flagging for a
separate fix.
Closes FinChippay#631
🤖 Greptile AI Code ReviewGreptile will automatically review this PR (12 file(s) changed). Review gates:
|
Author
|
Closing — opened with unwanted AI-attribution text in the description that the author did not approve. |
🤖 Greptile AI Code ReviewGreptile will automatically review this PR (12 file(s) changed). Review gates:
|
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
Closes #631 (Issue #62 — Inbound Webhook HMAC Signature Verification & Replay Window).
POST /api/sep24/callback— the endpoint an anchor calls to report deposit/withdrawal status — accepted any unauthenticated request body and applied it directly viasep24Service.handleAnchorCallback(), including flipping a transaction to"completed". Any third party who could reach the endpoint could forge that callback. This PR requires and verifies a per-endpoint HMAC signature + timestamp before the handler ever runs.What changed
verifyInboundWebhookSignature(endpoint)middleware (backend/src/middleware/), applied to the SEP-24 callback route. Checks, in order:X-SignatureandX-Timestampboth presentX-Timestamp(Unix seconds) within±WEBHOOK_REPLAY_WINDOW_SECONDS(default 300s) of nowX-SignaturematchesHMAC-SHA256(secret, rawBody)for at least one of the endpoint's active secrets, viacrypto.timingSafeEqual(never===)Every rejection path returns the same generic
401— the response never reveals which check failed.Raw body capture: HMAC verification needs the exact bytes the sender signed — re-serializing
req.bodywithJSON.stringify()isn't guaranteed to reproduce that (key order, whitespace, unicode escaping can all differ).bodyParsing.jsnow captures it viaexpress.json()'sverifycallback ontoreq.rawBody, exposed through ajsonBodyParser()factory so test apps use the identical parser config as production.Per-endpoint secret storage (new
inbound_webhook_secretstable +inboundWebhookSecretService.js): secrets are stored as AES-256-GCM ciphertext (never plaintext) plus a keyed HMAC-SHA256 fingerprint — the same at-rest pattern already used for outbound webhook secrets inwebhookService.js. Multiple secrets can be active per endpoint at once, which is what makes rotation with a grace period possible:rotateSecret(endpoint, { graceSeconds })keeps the old secret valid for a bounded window instead of invalidating it the instant the new one exists, andrevokeSecret()cuts a specific secret off immediately regardless of any grace period.Extracted
hashSecret/WEBHOOK_SECRET_KEYout ofwebhookService.jsinto a new dependency-freeutils/webhookSecretHash.js, shared by both the outbound and inbound paths.webhookService.jsonly needed@stellar/stellar-sdkfor unrelated Horizon SSE streaming — importing it just to reuse the hash helper would have dragged that whole dependency chain intoinboundWebhookSecretService.jsfor no reason (and, in this environment's Jest setup, that chain hits a pre-existing broken transitive ESM dependency — see Testing notes).Server startup provisions a secret for
sep24_callbackif one doesn't exist yet (ensureSecretExists, idempotent — safe to call on every boot) and logs it once so an operator can configure it on the anchor's side without a separate manual step.rotateSecret()/revokeSecret()handle changing it afterwards.Replay-nonce store reuses
cacheService.js(Redis, with an always-on in-memory LRU fallback) via a newsetIfNotExists()— a single atomicSET key val EX ttl NX, not aget()thenset()with a race window between them.New
AUTH_INVALID_WEBHOOK_SIGNATURE(401) error code in the shared catalogue, consistent with the existingAUTH_*codes.Tests
18 tests in
backend/__tests__/webhookSignature.test.js(issue asked for ≥8): valid signature, missingX-Signature, missingX-Timestamp, malformed signature, tampered body, wrong secret, stale timestamp, future timestamp, non-numeric timestamp, replay, no active secret configured, generic (non-leaking) error body — plus secret-service coverage (never stores plaintext, idempotent bootstrap, rotation with/without grace period, early revocation, metadata-only listing). All run against a real Express app and a real throwaway migrated SQLite DB (not mocks) —npx jest webhookSignature: 18/18 passing.npx eslint 'src/**/*.js': 0 errors, 0 warnings in any file this PR touches (repo-wide run shows only pre-existing, unrelated warnings/1 error elsewhere).Testing gap I couldn't close in this environment:
backend/__tests__/sep24.test.js(and a few other suites —health.test.js,turretsHealth.test.js,healthDependencies.test.js) fail to even load here withSyntaxError: Cannot use import statement outside a modulefrom@stellar/stellar-sdk's@noble/hashessub-dependency — a pre-existing Jest/Babel transform gap, unrelated to this change (reproduces identically on a clean checkout ofmain, with notransformIgnorePatternscovering that package, and matches the repo's ownci: skip failing test jobs on pull requestshistory). I verified mysep24.jsdiff by inspection instead (3 lines: one import, oneSEP24_CALLBACK_ENDPOINTconstant, one middleware insertion in the route chain) and covered the actual verification logic directly through my own test suite, which doesn't go through that import chain.Also found, not fixed (separate, unrelated, pre-existing)
webhookService.js'sregisterWebhook()(the outbound webhook registration path,POST /api/webhooks) inserts into asecretcolumn that migration003_webhooks.jsnever created — onlysecret_hashexists in the real migrated schema. Reproduces on a cleanmaincheckout with zero changes from this branch (table webhooks has no column named secret), and is whywebhookDeliveryRetry.test.jsalready has failing tests today. Flagging this clearly rather than bundling an unrelated fix into a security PR — happy to open a separate PR for it if useful.