Await the webhook signature verification so it can actually reject

[izaitsevfb]

✴️ iz2: filed on behalf of @izaitsevfb.

What's wrong

In the vendored terraform-aws-github-runner webhook lambda, the signature check is:

if (!webhooks.verify(payload, signature)) {
  console.error('Unable to verify signature!');
  return 401;
}

Webhooks#verify from @octokit/webhooks v12 is async — the constructor binds it straight to
the async function verify(secret, eventPayload, signature) in @octokit/webhooks-methods. So the
call returns a Promise, a Promise is always truthy, !Promise is always false, and the 401
branch is dead code. Any request carrying a non-empty x-hub-signature header passes the check.

A synchronous throw inside an async function becomes a rejected Promise rather than a falsy value,
so nothing recovers the check on the error path either.

Why CI didn't catch it

handler.test.ts had no test asserting 401 on an invalid signature. Its cases passed hardcoded
sha1=… values and asserted 200 — they passed because verification was bypassed, so a green
suite was actively evidence for the wrong conclusion.

The change

• await the verification result.
• Prefer x-hub-signature-256, falling back to the legacy x-hub-signature. GitHub sends both;
  the handler previously only ever read the sha1 one. ?? rather than || is deliberate: a
  present-but-invalid sha256 header must not be downgraded to sha1.
• Wrap the call so a throwing verifier returns 401 instead of an unhandled rejection.

Tests

Adds the negative coverage that was missing, and a positive control:

• signature does not match the payload → 401, nothing enqueued
• payload tampered with after signing → 401, nothing enqueued
• signature made with a different secret → 401, nothing enqueued
• unrecognisable digest → 401
• verifier throws → 401 rather than an unhandled rejection
• invalid sha256 plus a valid sha1 header → still 401 (precedence)
• valid sha1 alone, with no sha256 header → 200 (back-compat)
• correctly signed workflow_job/queued → 200 and enqueued

That last one is the positive control: it proves the actionable path really does fire when the
signature is valid, which is what makes every not.toBeCalled() assertion above non-vacuous.

Verified locally: 12/12 pass; prettier --check and eslint clean; tsc --noEmit reports no
errors outside node_modules. Mutation control — restoring the missing await makes the suite
fail, and it is exactly the new negative tests that fail, so they genuinely detect the defect.

Deploying this

This turns a check that never ran into one that does, so confirm the configured
GITHUB_APP_WEBHOOK_SECRET for each deployment actually matches the webhook secret on the
corresponding GitHub App before rolling out. Because verification has never executed, a stale or
mismatched secret cannot have shown up as an error until now — it would surface for the first time
as a 401 on every delivery once this lands. Roll out per-environment and watch the 4xx rate rather
than deploying everywhere at once.

Two adjacent gaps left deliberately out of scope, both pre-existing:

• A missing signature still returns 500 where 400/401 would be more accurate. Unchanged to
  keep this diff minimal and avoid perturbing existing alerting.
• Signed deliveries are replayable: X-GitHub-Delivery is not recorded and workflow-job IDs are not
  deduplicated downstream.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
torchci	Ignored		Aug 31, 2026 11:40pm UTC

[jeanschmidt]

This change is high criticality, monitoring and alerting should be included in it, ideally we need a env var we can control to enable/disable from enforcing to warning, for deployment + rollback

Review summary

🟠 1 major · 🟡 2 minor · ⚪ 1 nit

• 🟠 The claimed sha1 back-compat does not exist: @​octokit/webhooks 12.2.0 verifies only sha256, so the new sha1-only test asserts 200 where the handler returns 401 — terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts:122
• 🟡 The only test covering the new catch branch asserts a 401 that the non-throwing path also produces, so it cannot detect that it stopped covering the catch — terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts:101
• 🟡 The new catch around webhooks.verify maps a server-side misconfiguration (blank secret) to 401, contradicting the 500 returned four lines above — terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.ts:40
• ⚪ Two re-added test names describe check_run action/status filtering that handler.ts does not implement and the tests do not exercise — terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts:133

Extended analysis (ai-generated section)

What this changes

In the vendored terraform-aws-github-runner webhook lambda, handler.ts now awaits webhooks.verify and wraps it in a try/catch that returns 401, and it selects the signature as headers['x-hub-signature-256'] ?? headers['x-hub-signature']. Before this, the un-awaited call produced an always-truthy Promise, so both 401 branches were unreachable and any non-empty signature header passed. handler.test.ts is rewritten around a real HMAC helper: negative cases for wrong digest, tampered payload, foreign secret, unparseable header, a throwing verifier, sha256-over-sha1 precedence, a sha1-only back-compat case, and a positive control asserting sendActionRequest fires once for a correctly signed workflow_job/queued. Two files, no production code outside handler.ts.

What it gets right

The fix is the right size for the defect. The await plus the catch closes both failure shapes — a false return and a rejected Promise — and the pre-existing 500 branches at handler.ts:18-31 are left alone. The test rewrite is the substantive part: the old cases carried hardcoded sha1= digests and asserted 200, which they only achieved because verification was bypassed, so replacing them with digests computed from the test secret means removing the await now turns the suite red. The positive control at handler.test.ts:59 is what makes the surrounding not.toBeCalled() assertions non-vacuous, and the comment says so.

Where the risk is

Two regions, and independent reviewers converged on both. The header-selection line handler.ts:17 and the test pinned to it at handler.test.ts:120-124 carry three findings under one cluster. Whether they read as a red CI job or as residual hardening turns on one unsettled fact: the pinned @​octokit/webhooks 12.2.0 pulls @​octokit/webhooks-methods 4.1.0 (yarn.lock:1196-1199, 1216-1222), not the ^5.1.0 direct dependency, and no reviewer could read that library from this checkout. If 4.1.0 is sha256-only, the sha1-only test asserts 200 where the handler returns 401 and the fallback branch has no working path; if it honours sha1=, HMAC-SHA1 stays an accepted algorithm at an endpoint whose only auth is this check, and the stated no-downgrade property holds only when the sha256 header is present. One edit settles all three either way.

The second region is src/local.ts:7-10, which is untouched: it installs bodyParser.json() and passes JSON.stringify(req.body). Awaiting verify makes byte fidelity of the payload load-bearing for the first time, so the dev server can now 401 a correctly signed delivery and the log line will blame the signature. Three confirmed findings share that one edit. lambda.ts:7 forwards the raw body, so the deployed path is unaffected.

The recurring shape is that the fix itself is sound and the exposure is in what enforcement newly reaches: a second caller, a fallback branch never exercised before, and the absence of any signal. That last one compounds — there is no CloudWatch alarm anywhere under terraform-aws-github-runner/, so any newly reachable 401 becomes silent loss of runner scale-up rather than an alert.

Before merge, and after

Settle the sha1 question first: run yarn test with dependencies installed, or read verify in the installed 4.1.0. Whichever way it goes, dropping the fallback at handler.ts:17 and inverting the assertion in "accepts the legacy sha1 signature header when no sha256 header is sent" is a one-line change that also disposes of "keeps HMAC-SHA1 as an accepted authentication algorithm". The console.error assertion suggested for "cannot detect that it stopped covering the catch" is equally cheap and worth taking now.

Reasonable as follow-up: the re-serialized body in src/local.ts, dev-loop only but a real trap; the blank-secret path, where if (!secret) at handler.ts:28 would route a misconfiguration to the 500 branch it already has instead of 401 — note that one was not independently reproduced; the stale check_run test names. The 4xx alarm and staged enforcement belong last because they need a decision, not a patch: whether a log-only window is acceptable is a judgement call about shipping unverified traffic for one more release.

[jeanschmidt]

My agent is unsure about this one, worth flagging to have a look

---

🟠 The claimed sha1 back-compat does not exist: @​octokit/webhooks 12.2.0 verifies only sha256, so the new sha1-only test asserts 200 where the handler returns 401 (ai-generated section)

The description promises "valid sha1 alone, with no sha256 header → 200 (back-compat)" and this test encodes it. But handler.ts:39 verifies through webhooks.verify, which @​octokit/webhooks v12 binds to verify(secret, eventPayload, signature) in @​octokit/webhooks-methods — and that package dropped sha1 in v3.0.0. In v4.x, verify compares the caller's signature against sign(secret, payload), whose algorithm is hardcoded to sha256; a sha1=<40 hex> string is 45 bytes against a 71-byte sha256=<64 hex> expectation, so it can only ever return false. Inferred step, stated plainly: I could not read the library source (no node_modules in this checkout), so the sha256-only behaviour of webhooks-methods 4.1.0 comes from the published API of that package, not from code I opened. Everything else here I read. terraform-aws-github-runner/modules/webhook/lambdas/webhook/yarn.lock:1216-1223 resolves @​octokit/webhooks@^12.2.0 to 12.2.0 with dependency "@​octokit/webhooks-methods" "^4.1.0", resolved at yarn.lock:1196-1199 to 4.1.0 — not the ^5.1.0 direct dependency in package.json:41, which webhooks.verify never touches. The description itself identifies the callee: "the constructor binds it straight to the async function verify(secret, eventPayload, signature) in @​octokit/webhooks-methods". handler.test.ts:122 signs with sign(payload, 'sha1') (handler.test.ts:17-18, createHmac('sha1', …)) and handler.test.ts:123 asserts expect(resp).toBe(200). .github/workflows/lambda-webhook.yml:23 runs make build, and Makefile:9-14 runs yarn test, so this test gates the PR. The same inference makes the fallback headers['x-hub-signature'] at handler.ts:17 a branch that can only produce a 401. The "Lambda Webhook" CI job fails on this PR at the accepts the legacy sha1 signature header when no sha256 header is sent case (401 received, 200 expected), contradicting the stated "Verified locally: 12/12 pass". Independently of the test, the sha1 fallback the description sells as back-compat is unreachable-as-successful: any sender that omits x-hub-signature-256 is rejected, so the fallback adds a branch with no working path rather than the compatibility it advertises. Narrow (what I would pick): delete the sha1 fallback and read only headers['x-hub-signature-256'] at handler.ts:17 — GitHub always sends it — then replace the test at lines 120-124 with the inverse assertion (sha1-only → 401), and keep the precedence test at line 107 as-is. Broader: if a sha1-only sender genuinely has to keep working, verify sha1 explicitly in handler.ts with crypto.timingSafeEqual over createHmac('sha1', secret).update(payload).digest('hex') rather than routing it through webhooks.verify, which will not do it. Either way, confirm the library's behaviour first with a one-liner against the installed 4.1.0.

_{Reviewed by claude-opus-5[1m] at max effort, against 4a50ae2.}

[jeanschmidt]

probably not intentional?

---

🟡 The only test covering the new catch branch asserts a 401 that the non-throwing path also produces, so it cannot detect that it stopped covering the catch (ai-generated section)

This test reaches the catch at handler.ts:40 only because @​octokit/webhooks-methods happens to throw on a falsy eventPayload rather than returning false. Its assertions are expect(resp).toBe(401) and expect(sendActionRequest).not.toBeCalled() — identical to the assertions of the plain invalid-signature test at handler.test.ts:65-72, which exercises the if (!verified) branch at handler.ts:44. Nothing in the test distinguishes which of the two 401 paths ran. handler.ts has two 401 returns with different messages: Unable to verify signature: ${e} at line 41 (catch) and Unable to verify signature! at line 45 (falsy result). The test at handler.test.ts:100-104 asserts neither, only the status. console.error is already replaced with jest.fn() in beforeEach at handler.test.ts:44 and restored at line 49, so the distinguishing message is available to assert on at zero cost. The premise is a library internal, stated in the comment at handler.test.ts:98 and pinned only by @​octokit/webhooks@^12.2.0 in package.json:40. If an octokit bump changes verify to return false for an empty payload instead of throwing, this test stays green while the catch at handler.ts:38-43 becomes uncovered, and the suite silently reports coverage it no longer has. The next person to touch the verification block gets no signal that the throw path is unprotected. Add expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Unable to verify signature:')); after line 102 — the colon distinguishes it from the Unable to verify signature! message on the other 401 path, so the test fails the moment it stops reaching the catch. A heavier alternative is to jest.mock('@​octokit/webhooks') with a verify that rejects, which removes the dependence on library internals entirely; I would take the one-line assertion.

_{Reviewed by claude-opus-5[1m] at max effort, against 4a50ae2.}

[jeanschmidt]

would be better to fail loudly on cases like this, instead of making people seeking where the keys are diverging

---

🟡 The new catch around webhooks.verify maps a server-side misconfiguration (blank secret) to 401, contradicting the 500 returned four lines above (ai-generated section)

The catch added at handler.ts:40 is unconditional: every throw out of webhooks.verify becomes return 401 at line 42. verify does not throw for a bad signature — it returns false — so the throws this catch actually absorbs are argument errors (TypeError: ... secret, eventPayload & signature required) raised when the secret or the payload is falsy. That turns "this deployment's webhook secret is blank" into the same HTTP status as "the caller's signature is wrong". handler.ts:28 guards only secret === undefined, so a secret that decrypts to '' passes it: decrypt (src/kms/index.ts:5-18) returns result = encrypted verbatim when KMS_KEY_ID is unset, so GITHUB_APP_WEBHOOK_SECRET='' yields '', not undefined. That '' reaches new Webhooks({secret: ''}) at line 33-35 and makes verify throw rather than return false — which is precisely the behaviour the new test at handler.test.ts:100-104 exercises (via an empty payload) and the comment at handler.test.ts:98 states. The adjacent branch at handler.ts:28-31 establishes the convention that secret problems return 500. A deployment whose GITHUB_APP_WEBHOOK_SECRET is blank rather than mismatched returns 401 on every delivery. The PR's own rollout note tells the operator to compare the configured secret against the GitHub App's secret, so they will chase a mismatch while the real cause is an empty value the handler already has a 500 branch for. The ${e} in the log line does carry the TypeError text, so the cause is recoverable from CloudWatch, but not from the status code or the alerting built on it. Narrow: change handler.ts:28 to if (!secret) { so a blank secret hits the existing Cannot decrypt secret. 500 branch and never reaches verify. Broader: keep that and also distinguish in the catch, e.g. return e instanceof TypeError ? 500 : 401;, so any future argument error is reported as a server fault rather than an auth failure. I would take the narrow one — it removes the only misconfiguration that reaches this catch today and leaves the empty-payload case, which is genuinely a bad request.

_{Reviewed by claude-opus-5[1m] at max effort, against 4a50ae2.}

[jeanschmidt]

nit, should be an easy fix

---

⚪ Two re-added test names describe check_run action/status filtering that handler.ts does not implement and the tests do not exercise (ai-generated section)

The test titles at handler.test.ts:133 and handler.test.ts:140 state as behaviour that the handler filters check_run events by action ('other than created') and by status ('other than queued'). handler.ts has no check_run handling at all: line 53 branches only on githubEvent === 'workflow_job', line 59 on body.action === 'queued' of a workflow_job, and the string 'created' appears nowhere in src/. Both tests also build their headers with signedHeaders(payload), whose event parameter defaults to 'push' (handler.test.ts:20-23), so what they actually assert is the ignore-non-workflow_job path at handler.ts:85. The names are inherited from upstream, but this diff re-adds both it(...) lines with rewritten bodies, which makes stale prose look freshly authored and authoritative. handler.ts:53 if (githubEvent === 'workflow_job') { with the else branch at handler.ts:85 console.info('Ignore event ' + githubEvent); is the only event dispatch; the only action comparison is handler.ts:59 if (body.action === 'queued') { inside the workflow_job branch. Grepping check_run|created across terraform-aws-github-runner/modules/webhook/lambdas/webhook/src returns matches only in handler.test.ts (lines 4, 121, 127, 133-134, 140-141) — no production code mentions check_run. handler.test.ts:20-23 defines const signedHeaders = (payload: string, event = 'push') => ({...}), and lines 135 and 142 call it with one argument, so X-GitHub-Event is 'push' in both tests; the check_run payload's action: 'completed' (line 134) and check_run.status: 'completed' (line 141) are never read by the handler. A maintainer reading the suite concludes the lambda has check_run action/status filtering and that it is under test. Nothing fails at runtime, but the next person touching the event filter in handler.ts:53-59 is told by these two names that a contract exists which the lambda has never had, and may keep or reintroduce dead filtering to satisfy it. Narrow fix: rename both to what they actually assert, e.g. it('ignores a push event carrying a check_run payload', ...) and drop one as redundant with 'does not handle other events' at line 126. Broader fix, if check_run really is meant to be covered: pass 'check_run' as the event — handle(signedHeaders(payload, 'check_run'), payload) — so the header matches the name and the test exercises the ignore path for that event type. I would take the narrow rename, since the handler has no check_run semantics to test.

_{Reviewed by claude-opus-5[1m] at max effort, against 4a50ae2.}