Skip to content

Await the webhook signature verification so it can actually reject #8684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
izaitsevfb wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Await the webhook signature verification so it can actually reject #8684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 100 additions & 14 deletions terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { createHmac } from 'crypto';

import { handle } from './handler';
import check_run_event from '../../test/resources/github_check_run_event.json';

Expand All @@ -10,11 +12,34 @@ jest.mock('../kms', () => ({
}),
}));

const TEST_SECRET = 'TEST_SECRET';

const sign = (payload: string, algorithm: 'sha256' | 'sha1' = 'sha256'): string =>
`${algorithm}=${createHmac(algorithm, TEST_SECRET).update(payload).digest('hex')}`;

const signedHeaders = (payload: string, event = 'push') => ({
'X-Hub-Signature-256': sign(payload),
'X-GitHub-Event': event,
});

// A workflow_job/queued event IS actionable, so `sendActionRequest` assertions below are
// only meaningful when the payload is this one — see the positive control test.
const queuedWorkflowJob = JSON.stringify({
action: 'queued',
installation: { id: 42 },
repository: { name: 'pytorch', owner: { login: 'pytorch' } },
workflow_job: {
id: 1234,
labels: ['linux.2xlarge'],
html_url: 'https://github.com/pytorch/pytorch/actions/runs/1',
},
});

describe('handler', () => {
let originalError: Console['error'];

beforeEach(() => {
process.env.GITHUB_APP_WEBHOOK_SECRET = 'TEST_SECRET';
process.env.GITHUB_APP_WEBHOOK_SECRET = TEST_SECRET;
originalError = console.error;
console.error = jest.fn();
jest.clearAllMocks();
Expand All @@ -29,31 +54,92 @@ describe('handler', () => {
expect(resp).toBe(500);
});

it('does not handle other events', async () => {
// Positive control: proves the actionable path really does fire when the signature is valid,
// which is what makes every `not.toBeCalled()` assertion below non-vacuous.
it('enqueues a correctly signed queued workflow_job', async () => {
const resp = await handle(signedHeaders(queuedWorkflowJob, 'workflow_job'), queuedWorkflowJob);
expect(resp).toBe(200);
expect(sendActionRequest).toBeCalledTimes(1);
});

it('returns 401 and does not enqueue when the signature does not match the payload', async () => {
const resp = await handle(
{ 'X-Hub-Signature': 'sha1=4a82d2f60346e16dab3546eb3b56d8dde4d5b659', 'X-GitHub-Event': 'push' },
JSON.stringify(check_run_event),
{ 'X-Hub-Signature-256': `sha256=${'0'.repeat(64)}`, 'X-GitHub-Event': 'workflow_job' },
queuedWorkflowJob,
);
expect(resp).toBe(200);
expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

it('does not handle check_run events with actions other than created', async () => {
const event = { ...check_run_event, action: 'completed' };
it('returns 401 and does not enqueue when the payload was tampered with after signing', async () => {
const headers = signedHeaders(queuedWorkflowJob, 'workflow_job');
const tampered = JSON.stringify({ ...JSON.parse(queuedWorkflowJob), workflow_job: { id: 9999, labels: ['huge'] } });
const resp = await handle(headers, tampered);
expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

it('returns 401 and does not enqueue when the signature was made with a different secret', async () => {
const foreign = `sha256=${createHmac('sha256', 'NOT_THE_SECRET').update(queuedWorkflowJob).digest('hex')}`;
const resp = await handle({ 'X-Hub-Signature-256': foreign, 'X-GitHub-Event': 'workflow_job' }, queuedWorkflowJob);
expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

it('returns 401 when the signature header is not a recognisable digest', async () => {
const resp = await handle(
{ 'X-Hub-Signature': 'sha1=891749859807857017f7ee56a429e8fcead6f3e1', 'X-GitHub-Event': 'push' },
JSON.stringify(event),
{ 'X-Hub-Signature-256': 'not-a-signature', 'X-GitHub-Event': 'workflow_job' },
queuedWorkflowJob,
);
expect(resp).toBe(200);
expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

it('does not handle check_run events with status other than queued', async () => {
const event = { ...check_run_event, check_run: { id: 1234, status: 'completed' } };
// The verifier throws (rather than returning false) on an empty payload; the handler must
// turn that into a 401 instead of an unhandled rejection.
it('returns 401 rather than throwing when the verifier rejects', async () => {
const resp = await handle({ 'X-Hub-Signature-256': sign(''), 'X-GitHub-Event': 'workflow_job' }, '');

@jeanschmidt jeanschmidt Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

// sha256 is preferred, so a bad sha256 must NOT be rescued by a valid legacy sha1 header.
it('rejects an invalid sha256 signature even when a valid sha1 header is present', async () => {
const resp = await handle(
{ 'X-Hub-Signature': 'sha1=73dfae4aa56de5b038af8921b40d7a412ce7ca19', 'X-GitHub-Event': 'push' },
JSON.stringify(event),
{
'X-Hub-Signature-256': `sha256=${'0'.repeat(64)}`,
'X-Hub-Signature': sign(queuedWorkflowJob, 'sha1'),
'X-GitHub-Event': 'workflow_job',
},
queuedWorkflowJob,
);
expect(resp).toBe(401);
expect(sendActionRequest).not.toBeCalled();
});

it('accepts the legacy sha1 signature header when no sha256 header is sent', async () => {
const payload = JSON.stringify(check_run_event);
const resp = await handle({ 'X-Hub-Signature': sign(payload, 'sha1'), 'X-GitHub-Event': 'push' }, payload);

@jeanschmidt jeanschmidt Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(resp).toBe(200);
});

it('does not handle other events', async () => {
const payload = JSON.stringify(check_run_event);
const resp = await handle(signedHeaders(payload), payload);
expect(resp).toBe(200);
expect(sendActionRequest).not.toBeCalled();
});

it('does not handle check_run events with actions other than created', async () => {

@jeanschmidt jeanschmidt Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const payload = JSON.stringify({ ...check_run_event, action: 'completed' });
const resp = await handle(signedHeaders(payload), payload);
expect(resp).toBe(200);
expect(sendActionRequest).not.toBeCalled();
});

it('does not handle check_run events with status other than queued', async () => {
const payload = JSON.stringify({ ...check_run_event, check_run: { id: 1234, status: 'completed' } });
const resp = await handle(signedHeaders(payload), payload);
expect(resp).toBe(200);
expect(sendActionRequest).not.toBeCalled();
});
Expand Down
13 changes: 11 additions & 2 deletions terraform-aws-github-runner/modules/webhook/lambdas/webhook/src/webhook/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ export const handle = async (headers: IncomingHttpHeaders, payload: any): Promis
headers[key.toLowerCase()] = headers[key];
}

const signature = headers['x-hub-signature'] as string;
// Prefer the SHA-256 signature; GitHub still sends the legacy SHA-1 header alongside it.
const signature = (headers['x-hub-signature-256'] ?? headers['x-hub-signature']) as string;
if (!signature) {
console.error("Github event doesn't have signature. This webhook requires a secret to be configured.");
return 500;
Expand All @@ -32,7 +33,15 @@ export const handle = async (headers: IncomingHttpHeaders, payload: any): Promis
const webhooks = new Webhooks({
secret: secret,
});
if (!webhooks.verify(payload, signature)) {
// `verify` is async — without the await this is an always-truthy Promise and the check never rejects.
let verified: boolean;
try {
verified = await webhooks.verify(payload, signature);
} catch (e) {

@jeanschmidt jeanschmidt Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

console.error(`Unable to verify signature: ${e}`);
return 401;
}
if (!verified) {
console.error('Unable to verify signature!');
return 401;
}
Expand Down
Loading