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

Abierto Apto para principiantes
  • ¿Algo no está bien?
#735 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
2/5
Tiempo estimado
1-3 horas
Aptitud para principiantes
76/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
postgresql, typescript
Área
backend, security

Línea de trabajo

Lee src/github/github-webhooks.service.ts alrededor de GithubWebhooksService.handleEvent() y src/github/github-webhooks.controller.ts para seguir el tratamiento de la firma y la persistencia. Añade el caso indicado a github-webhooks.service.spec.ts, comprobando primero los mocks existentes y la configuración de las pruebas. El objetivo se cumple cuando las firmas no válidas no llaman a webhookEventRepo.save y la solicitud recibe la respuesta de rechazo prevista.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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.

Lenguaje dominante
TypeScript
Estrellas
2
Forks
90
Merge medio
31 min
PR fusionados (30 d)
20

Preparar el entorno

  • Incluye un Dockerfile o un archivo de Docker Compose
  • Sin plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de MergeFi/backend

Todos los issues de MergeFi/backend

Issues similares

Más issues de TypeScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.