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

GithubWebhooksService.handleEvent() の周辺にある src/github/github-webhooks.service.ts と src/github/github-webhooks.controller.ts を読み、署名の処理と永続化の流れを追ってください。既存のモックとテストのセットアップを先に確認してから、指定されたケースを github-webhooks.service.spec.ts に追加してください。無効な署名によって webhookEventRepo.save が呼び出されず、リクエストが意図された拒否レスポンスを受け取れば完了です。

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.