[Regression on closed #548] Webhook deliveries with an invalid signature are still persisted with their full payload before the signature is checked

Open Beginner friendly
  • Is something wrong?
#735 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
76/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
postgresql, typescript
Domain
backend, security

Research direction

Read src/github/github-webhooks.service.ts around GithubWebhooksService.handleEvent() and src/github/github-webhooks.controller.ts to trace signature handling and persistence. Add the stated case to github-webhooks.service.spec.ts, first checking its existing mocks and test setup. Done means invalid signatures do not call webhookEventRepo.save and the request receives the intended rejection response.

Written by the indexing model from the issue text.

Description

bug security

Location

  • src/github/github-webhooks.service.ts:104-161 — GithubWebhooksService.handleEvent()
  • src/github/github-webhooks.controller.ts:15-40 — GithubWebhooksController.handle()

Problem

handleEvent receives signatureValid as its fourth argument, but it writes the event row — including the complete, attacker-supplied payload — before that flag is consulted:

let event: WebhookEvent;
try {
  event = await this.webhookEventRepo.save(
    this.webhookEventRepo.create({
      eventType,
      deliveryId: deliveryId ?? null,
      payload,                              // line 133 — full attacker body
      signatureValid,
      status: signatureValid
        ? WebhookEventStatus.RECEIVED
        : WebhookEventStatus.IGNORED,
    }),
  );
} catch (err) { ... }

if (!signatureValid) {                      // line 156 — checked only afterwards
  this.logger.warn(`Rejected webhook delivery ${deliveryId} — invalid signature`);
  return event;
}

The signature check correctly prevents any business logic from running, and verifyGithubSignature itself is sound (constant-time, fails closed on a missing secret, and the controller fails closed when req.rawBody is absent). What is missing is the gate on the database write: an unauthenticated caller's body is committed to webhook_events.payload regardless.

This is the defect described in closed issue #548. The subsequent dedupe work for #308 reorganised this method but the ordering was not restored.

Why it matters

POST /api/github/webhooks is necessarily public — GitHub must be able to reach it without credentials. That makes this an unauthenticated, uncapped write primitive against the primary Postgres instance:

  • WebhookEvent.payload is a JSON column with no size bound of its own, so each request can store a body as large as the Express body-parser limit allows. The only brake is the global throttler (ThrottlerModule's default: 120 requests/minute, app.module.ts:34), which is per-IP; with the body at the parser ceiling that is still a meaningful volume of junk rows per minute per source address, and there is no mechanism anywhere that prunes IGNORED rows (IdempotencyCleanupService only sweeps idempotency_keys).
  • Because the throttler keys on the client IP and there is no trust proxy configuration, requests arriving through a load balancer are bucketed by the proxy's address, which means the practical per-attacker limit is not what the config suggests.
  • Operationally, the webhook_events table is the audit trail an operator reads when a payout goes wrong. Filling it with forged IGNORED rows that carry convincing-looking pull_request/issues payloads is a cheap way to bury a real delivery, and the stored payload is replayed verbatim to anyone inspecting the table.
  • The table also becomes a store of arbitrary third-party content under this platform's control, with no validation ever applied to it (validatePullRequestPayload / validateIssuesEventPayload run only after the signature gate, at lines 164-171).

Reproduction / failure path

No credentials needed. With the service running and GITHUB_WEBHOOK_SECRET set:

for i in $(seq 1 50); do
  curl -s -o /dev/null -X POST http://localhost:4000/api/github/webhooks \
    -H 'Content-Type: application/json' \
    -H 'X-GitHub-Event: pull_request' \
    -H "X-GitHub-Delivery: $(uuidgen)" \
    -H 'X-Hub-Signature-256: sha256=0000000000000000000000000000000000000000000000000000000000000000' \
    -d '{"junk":"'"$(head -c 100000 /dev/zero | tr '\0' 'A')"'"}'
done

Then:

SELECT count(*), pg_size_pretty(sum(pg_column_size(payload))::bigint)
FROM webhook_events WHERE "signatureValid" = false;

Observed: 50 rows, each holding the full 100 KB body, every one returning HTTP 202 with {"received":true,...}. Expected: nothing persisted, and a non-2xx (401) response.

Suggested fix

Move the signature gate ahead of the write in handleEvent, and do not store an unverifiable payload at all:

if (!signatureValid) {
  this.logger.warn(`Rejected webhook delivery ${deliveryId} — invalid signature`);
  throw new UnauthorizedException('Invalid webhook signature');
}

Returning 401 rather than 202 is also what GitHub's own delivery UI expects for a rejected delivery, and it makes a misconfigured secret visible in the repository's webhook delivery log instead of silently invisible.

If a record of rejected attempts is wanted for monitoring, keep it but make it bounded and payload-free: persist only eventType, deliveryId, the source IP and a timestamp (no payload), and give that path its own tight @Throttle on the controller route. Either way the full body must not be written on the unverified path.

Add a github-webhooks.service.spec.ts case asserting webhookEventRepo.save is not called when signatureValid is false, so the ordering is pinned by a test this time.

Dominant language
TypeScript
Stars
2
Forks
90
Avg merge
31m
Merged PRs (30d)
20

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from MergeFi/backend

All issues in MergeFi/backend

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.