From 75f00f60f3bc1812e7aeecae89af6dd60232800b Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 12:20:49 -0700 Subject: [PATCH 1/8] =?UTF-8?q?feat:=20POST=20/consent/log=20=E2=80=94=20p?= =?UTF-8?q?ersist=20invite-page=20consent=20records=20(ENG-568)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The getflash.io/invite page has collected transactional/marketing consent since launch and POSTed it to /consent/log — an endpoint that never existed, so every record was silently dropped while the page displayed a compliance promise. Decision on ENG-568: build the endpoint. Public unauthenticated write, so deliberately narrow: 8kb body cap, strict bounded-field validation (unknown fields discarded), per-IP rate limit (20/hr, new consent_log_ip config), and the invite token persisted only as a hash — the same rule the invites collection follows. Records are append-only evidence: version, both consent legs, source URL, UA, client timestamp (verbatim, untrusted), IP, receivedAt. Mounted on the public server only (type === "main"), before the JWT middleware. No ingress change needed — api.flashapp.me routes the whole host to this service (verified via /healthz). --- src/config/yaml.ts | 6 + src/domain/consent-log/index.ts | 108 ++++++++++++++++++ src/domain/rate-limit/errors.ts | 1 + src/domain/rate-limit/index.ts | 11 ++ src/servers/consent-log.ts | 73 ++++++++++++ src/servers/graphql-server.ts | 8 ++ src/services/mongoose/models/consent-log.ts | 52 +++++++++ .../authentication/ops-events-hooks.spec.ts | 1 + .../request-code-destination.spec.ts | 1 + test/flash/unit/domain/consent-log.spec.ts | 88 ++++++++++++++ 10 files changed, 349 insertions(+) create mode 100644 src/domain/consent-log/index.ts create mode 100644 src/servers/consent-log.ts create mode 100644 src/services/mongoose/models/consent-log.ts create mode 100644 test/flash/unit/domain/consent-log.spec.ts diff --git a/src/config/yaml.ts b/src/config/yaml.ts index 5fb234fe9..592ec1c8f 100644 --- a/src/config/yaml.ts +++ b/src/config/yaml.ts @@ -265,6 +265,12 @@ export const getInviteTargetAttemptLimits = () => ({ blockDuration: toSeconds(86400), // 24 hours }) +export const getConsentLogAttemptLimits = () => ({ + points: 20, + duration: toSeconds(3600), // 1 hour + blockDuration: toSeconds(3600), // 1 hour +}) + /** * Card top-up checkout links, per account. * diff --git a/src/domain/consent-log/index.ts b/src/domain/consent-log/index.ts new file mode 100644 index 000000000..a0694069a --- /dev/null +++ b/src/domain/consent-log/index.ts @@ -0,0 +1,108 @@ +import { ValidationError } from "@domain/shared" + +// A consent record submitted by the getflash.io/invite landing page (ENG-568). +// The page has displayed a transactional/marketing consent flow since launch +// and POSTed it to /consent/log — an endpoint that did not exist, so every +// record was silently dropped. This validator is the endpoint's admission +// gate: the request body is anonymous, unauthenticated web input, so every +// field is bounded and everything unknown is discarded rather than stored. + +const MAX_VERSION = 64 +const MAX_URL = 2048 +const MAX_USER_AGENT = 1024 +const MAX_TIMESTAMP = 64 +const MAX_TOKEN = 128 +const MAX_PURPOSE = 256 +const MAX_FREQUENCY = 64 + +// The invite token is secret-bearing (it redeems the invite). It is accepted +// here so the consent record can be tied to its invite, but callers must +// store only its hash — the same rule the invites collection follows. +export type ConsentLogSubmission = { + version: string + sourceUrl?: string + userAgent?: string + clientTimestamp?: string + token?: string + consents: { + transactional: { optedIn: boolean; purpose?: string; frequency?: string } + marketing: { optedIn: boolean; purpose?: string; frequency?: string } + } +} + +const boundedString = ( + value: unknown, + max: number, + field: string, +): string | undefined | ValidationError => { + if (value === undefined || value === null) return undefined + if (typeof value !== "string") return new ValidationError(`${field} must be a string`) + if (value.length > max) return new ValidationError(`${field} exceeds ${max} chars`) + return value +} + +const checkedConsentLeg = ( + value: unknown, + field: string, +): ConsentLogSubmission["consents"]["transactional"] | ValidationError => { + if (typeof value !== "object" || value === null) { + return new ValidationError(`${field} must be an object`) + } + const leg = value as Record + if (typeof leg.optedIn !== "boolean") { + return new ValidationError(`${field}.optedIn must be a boolean`) + } + const purpose = boundedString(leg.purpose, MAX_PURPOSE, `${field}.purpose`) + if (purpose instanceof ValidationError) return purpose + const frequency = boundedString(leg.frequency, MAX_FREQUENCY, `${field}.frequency`) + if (frequency instanceof ValidationError) return frequency + return { optedIn: leg.optedIn, purpose, frequency } +} + +export const checkedToConsentLogSubmission = ( + body: unknown, +): ConsentLogSubmission | ValidationError => { + if (typeof body !== "object" || body === null) { + return new ValidationError("body must be a JSON object") + } + const raw = body as Record + + if (typeof raw.version !== "string" || raw.version.length === 0) { + return new ValidationError("version is required") + } + if (raw.version.length > MAX_VERSION) { + return new ValidationError(`version exceeds ${MAX_VERSION} chars`) + } + + if (typeof raw.consents !== "object" || raw.consents === null) { + return new ValidationError("consents is required") + } + const consents = raw.consents as Record + const transactional = checkedConsentLeg( + consents.transactional, + "consents.transactional", + ) + if (transactional instanceof ValidationError) return transactional + const marketing = checkedConsentLeg(consents.marketing, "consents.marketing") + if (marketing instanceof ValidationError) return marketing + + // The page sends the page URL as `page`; accept `sourceUrl` too so the + // field name can converge without breaking either sender. + const sourceUrl = boundedString(raw.sourceUrl ?? raw.page, MAX_URL, "sourceUrl") + if (sourceUrl instanceof ValidationError) return sourceUrl + const userAgent = boundedString(raw.userAgent, MAX_USER_AGENT, "userAgent") + if (userAgent instanceof ValidationError) return userAgent + const clientTimestamp = boundedString(raw.timestamp, MAX_TIMESTAMP, "timestamp") + if (clientTimestamp instanceof ValidationError) return clientTimestamp + const token = boundedString(raw.token, MAX_TOKEN, "token") + if (token instanceof ValidationError) return token + + return { + version: raw.version, + sourceUrl, + userAgent, + clientTimestamp, + token, + consents: { transactional, marketing }, + } +} diff --git a/src/domain/rate-limit/errors.ts b/src/domain/rate-limit/errors.ts index 8397d0896..aa383cbb9 100644 --- a/src/domain/rate-limit/errors.ts +++ b/src/domain/rate-limit/errors.ts @@ -12,6 +12,7 @@ export class UserCodeAttemptIdentifierRateLimiterExceededError extends RateLimit export class UserCodeAttemptIpRateLimiterExceededError extends RateLimiterExceededError {} export class UserCodeAttemptBlockedCountryIpRateLimiterExceededError extends RateLimiterExceededError {} export class CreateDeviceAccountIpRateLimiterExceededError extends RateLimiterExceededError {} +export class ConsentLogIpRateLimiterExceededError extends RateLimiterExceededError {} export class UserLoginIpRateLimiterExceededError extends RateLimiterExceededError {} export class UserLoginIdentifierRateLimiterExceededError extends RateLimiterExceededError {} export class InvoiceCreateRateLimiterExceededError extends RateLimiterExceededError {} diff --git a/src/domain/rate-limit/index.ts b/src/domain/rate-limit/index.ts index 7ed633050..0c79f5e97 100644 --- a/src/domain/rate-limit/index.ts +++ b/src/domain/rate-limit/index.ts @@ -1,4 +1,5 @@ import { + getConsentLogAttemptLimits, getFailedLoginAttemptPerIpLimits, getFailedLoginAttemptPerLoginIdentifierLimits, getFygaroCheckoutCreateAttemptLimits, @@ -14,6 +15,7 @@ import { } from "@config" import { + ConsentLogIpRateLimiterExceededError, FygaroCheckoutCreateRateLimiterExceededError, FygaroTopupAllowanceRateLimiterExceededError, InviteCreateRateLimiterExceededError, @@ -40,6 +42,7 @@ export const RateLimitPrefix = { inviteCreate: "invite_daily", inviteTarget: "invite_target", fygaroCheckoutCreate: "fygaro_checkout_create", + consentLog: "consent_log_ip", fygaroTopupAllowance: "fygaro_topup_allowance", } as const @@ -110,6 +113,14 @@ export const RateLimitConfig: { [key: string]: RateLimitConfig } = { // no amount argument, so nothing can short-circuit before the trailing-24h // list query runs. Its own key so a customer who has spent the mutation's // budget can still be told what is left of their allowance. + // Public, unauthenticated write endpoint (consent evidence from the + // getflash.io/invite page). Per-IP: one legitimate submission per accept + // click, so the ceiling is far above any real use and far below abuse. + consentLog: { + key: RateLimitPrefix.consentLog, + limits: getConsentLogAttemptLimits(), + error: ConsentLogIpRateLimiterExceededError, + }, fygaroTopupAllowance: { key: RateLimitPrefix.fygaroTopupAllowance, limits: getFygaroTopupAllowanceAttemptLimits(), diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts new file mode 100644 index 000000000..388bd5301 --- /dev/null +++ b/src/servers/consent-log.ts @@ -0,0 +1,73 @@ +import express from "express" + +import { UNSECURE_IP_FROM_REQUEST_OBJECT } from "@config" +import { parseIps } from "@domain/accounts-ips" +import { checkedToConsentLogSubmission } from "@domain/consent-log" +import { RateLimitConfig } from "@domain/rate-limit" +import { RateLimiterExceededError } from "@domain/rate-limit/errors" +import { ValidationError } from "@domain/shared" +import { baseLogger } from "@services/logger" +import { ConsentLogRepository } from "@services/mongoose/models/consent-log" +import { consumeLimiter } from "@services/rate-limit" +import { hashToken } from "@utils" + +// POST /consent/log — compliance evidence from the getflash.io/invite landing +// page (ENG-568). The page has sent these records since launch with the call +// wrapped fail-open; this endpoint finally persists them. +// +// Anonymous by design: the submitter is an invitee with no session. That makes +// this a public unauthenticated write, so it is deliberately narrow — +// tiny body cap, strict field validation, per-IP rate limit, and the invite +// token stored only as a hash. Responses carry no body on success (204) and +// no internals on failure. + +const consentLogRouter = express.Router({ caseSensitive: true }) + +consentLogRouter.use(express.json({ limit: "8kb" })) + +consentLogRouter.post("/log", async (req, res) => { + const ipString = UNSECURE_IP_FROM_REQUEST_OBJECT ? req.ip : req.headers["x-real-ip"] + const ip = parseIps(ipString) + + const limited = await consumeLimiter({ + rateLimitConfig: RateLimitConfig.consentLog, + keyToConsume: ip ?? "", + }) + if (limited instanceof RateLimiterExceededError) { + return res.status(429).json({ error: "too many requests" }) + } + if (limited instanceof Error) { + // Rate-limit store fault. Evidence collection must not become a way to + // probe infrastructure health — refuse rather than fail open on a public + // write endpoint. + baseLogger.error({ error: limited }, "consent-log rate limiter unavailable") + return res.status(503).json({ error: "temporarily unavailable" }) + } + + const submission = checkedToConsentLogSubmission(req.body) + if (submission instanceof ValidationError) { + return res.status(400).json({ error: submission.message }) + } + + try { + await ConsentLogRepository.create({ + version: submission.version, + consents: submission.consents, + // Raw token never touches disk — same rule the invites collection + // follows (it stores tokenHash only). + inviteTokenHash: submission.token ? hashToken(submission.token) : undefined, + sourceUrl: submission.sourceUrl, + userAgent: submission.userAgent, + clientTimestamp: submission.clientTimestamp, + ip, + receivedAt: new Date(), + }) + } catch (err) { + baseLogger.error({ error: err }, "consent-log persist failed") + return res.status(500).json({ error: "could not record consent" }) + } + + return res.status(204).send() +}) + +export default consentLogRouter diff --git a/src/servers/graphql-server.ts b/src/servers/graphql-server.ts index 84ad73500..01b978926 100644 --- a/src/servers/graphql-server.ts +++ b/src/servers/graphql-server.ts @@ -29,6 +29,7 @@ import { parseUnknownDomainErrorFromUnknown } from "@domain/shared" import { MAXIMUM_QUERY_COMPLEXITY, createComplexityPlugin } from "./plugins/complexity" import authRouter from "./authorization" +import consentLogRouter from "./consent-log" import kratosCallback from "./event-handlers/kratos" import { apiKeyRateLimitMiddleware } from "./middlewares/api-key-rate-limit" import healthzHandler from "./middlewares/healthz" @@ -141,6 +142,13 @@ export const startApolloServer = async ({ app.use("/auth", authRouter) app.use("/kratos", kratosCallback) + // Public consent-evidence intake from the getflash.io/invite page + // (ENG-568). Anonymous callers, so it mounts only on the public server and + // BEFORE the JWT middleware below. + if (type === "main") { + app.use("/consent", consentLogRouter) + } + // Health check app.get( "/healthz", diff --git a/src/services/mongoose/models/consent-log.ts b/src/services/mongoose/models/consent-log.ts new file mode 100644 index 000000000..5a217d12e --- /dev/null +++ b/src/services/mongoose/models/consent-log.ts @@ -0,0 +1,52 @@ +import mongoose, { Schema } from "mongoose" + +// Compliance record of a consent submission from the getflash.io/invite +// landing page (ENG-568). Written by the public /consent/log endpoint. +// +// These are evidence records: append-only, never updated, no TTL — the whole +// point is to be able to show later who consented to what, when, from where. +// The invite token is stored ONLY as a hash (same rule as the invites +// collection); the raw token never touches disk here. +export interface ConsentLogRecord { + version: string + consents: { + transactional: { optedIn: boolean; purpose?: string; frequency?: string } + marketing: { optedIn: boolean; purpose?: string; frequency?: string } + } + inviteTokenHash?: string + sourceUrl?: string + userAgent?: string + clientTimestamp?: string + ip?: string + receivedAt: Date +} + +const ConsentLegSchema = new Schema( + { + optedIn: { type: Boolean, required: true }, + purpose: { type: String, maxlength: 256 }, + frequency: { type: String, maxlength: 64 }, + }, + { _id: false }, +) + +const ConsentLogSchema = new Schema({ + version: { type: String, required: true, maxlength: 64 }, + consents: { + transactional: { type: ConsentLegSchema, required: true }, + marketing: { type: ConsentLegSchema, required: true }, + }, + inviteTokenHash: { type: String, maxlength: 128, index: true }, + sourceUrl: { type: String, maxlength: 2048 }, + userAgent: { type: String, maxlength: 1024 }, + // The client's own clock, kept verbatim as evidence of what the client + // asserted; receivedAt below is the trustworthy ordering field. + clientTimestamp: { type: String, maxlength: 64 }, + ip: { type: String, maxlength: 64 }, + receivedAt: { type: Date, required: true, default: Date.now, index: true }, +}) + +export const ConsentLogRepository = mongoose.model( + "ConsentLog", + ConsentLogSchema, +) diff --git a/test/flash/unit/app/authentication/ops-events-hooks.spec.ts b/test/flash/unit/app/authentication/ops-events-hooks.spec.ts index 16bd6e9a8..d981b89ca 100644 --- a/test/flash/unit/app/authentication/ops-events-hooks.spec.ts +++ b/test/flash/unit/app/authentication/ops-events-hooks.spec.ts @@ -26,6 +26,7 @@ jest.mock("@config", () => { UNSECURE_DEFAULT_LOGIN_CODE: undefined, getGeetestConfig: jest.fn(() => ({})), getTestAccounts: jest.fn(() => []), + getConsentLogAttemptLimits: jest.fn(() => limits), getFailedLoginAttemptPerIpLimits: jest.fn(() => limits), getFailedLoginAttemptPerLoginIdentifierLimits: jest.fn(() => limits), getFygaroCheckoutCreateAttemptLimits: jest.fn(() => limits), diff --git a/test/flash/unit/app/authentication/request-code-destination.spec.ts b/test/flash/unit/app/authentication/request-code-destination.spec.ts index b2325b592..d50208799 100644 --- a/test/flash/unit/app/authentication/request-code-destination.spec.ts +++ b/test/flash/unit/app/authentication/request-code-destination.spec.ts @@ -13,6 +13,7 @@ jest.mock("@config", () => { UNSECURE_DEFAULT_LOGIN_CODE: undefined, getGeetestConfig: jest.fn(() => ({})), getTestAccounts: jest.fn(() => []), + getConsentLogAttemptLimits: jest.fn(() => limits), getFailedLoginAttemptPerIpLimits: jest.fn(() => limits), getFailedLoginAttemptPerLoginIdentifierLimits: jest.fn(() => limits), getInvoiceCreateAttemptLimits: jest.fn(() => limits), diff --git a/test/flash/unit/domain/consent-log.spec.ts b/test/flash/unit/domain/consent-log.spec.ts new file mode 100644 index 000000000..62ff49140 --- /dev/null +++ b/test/flash/unit/domain/consent-log.spec.ts @@ -0,0 +1,88 @@ +import { checkedToConsentLogSubmission } from "@domain/consent-log" +import { ValidationError } from "@domain/shared" + +// The /consent/log endpoint takes anonymous, unauthenticated web input. This +// validator is its whole admission gate: everything stored comes through +// here, everything unknown must be discarded, every field must be bounded. + +const validBody = () => ({ + version: "FLASH_CONSENT_V2_2025-09-23", + page: "https://getflash.io/invite/?token=abc", + userAgent: "Mozilla/5.0", + timestamp: "2026-09-01T18:00:00.000Z", + token: "a".repeat(64), + consents: { + transactional: { optedIn: true, purpose: "2FA codes", frequency: "as needed" }, + marketing: { optedIn: false, purpose: "offers", frequency: "up to 4/mo" }, + }, +}) + +describe("checkedToConsentLogSubmission", () => { + it("accepts the shape the invite page actually sends", () => { + const result = checkedToConsentLogSubmission(validBody()) + + expect(result).not.toBeInstanceOf(Error) + if (result instanceof Error) throw result + expect(result.version).toBe("FLASH_CONSENT_V2_2025-09-23") + expect(result.consents.transactional.optedIn).toBe(true) + expect(result.consents.marketing.optedIn).toBe(false) + // The page's `page` field lands as sourceUrl. + expect(result.sourceUrl).toBe("https://getflash.io/invite/?token=abc") + }) + + it("discards fields it does not know", () => { + const body = { ...validBody(), admin: true, $where: "1" } + + const result = checkedToConsentLogSubmission(body) + + if (result instanceof Error) throw result + expect(Object.keys(result).sort()).toEqual([ + "clientTimestamp", + "consents", + "sourceUrl", + "token", + "userAgent", + "version", + ]) + }) + + it.each([ + ["missing version", { version: undefined }], + ["empty version", { version: "" }], + ["non-string version", { version: 42 }], + ["oversized version", { version: "v".repeat(65) }], + ["missing consents", { consents: undefined }], + [ + "non-boolean optedIn", + { consents: { transactional: { optedIn: "yes" }, marketing: { optedIn: false } } }, + ], + ["missing marketing leg", { consents: { transactional: { optedIn: true } } }], + ["oversized token", { token: "t".repeat(129) }], + ["oversized userAgent", { userAgent: "u".repeat(1025) }], + ["non-string page", { page: { toString: "attack" } }], + ])("rejects %s", (_label, overrides) => { + const result = checkedToConsentLogSubmission({ ...validBody(), ...overrides }) + + expect(result).toBeInstanceOf(ValidationError) + }) + + it("rejects non-object bodies outright", () => { + expect(checkedToConsentLogSubmission(null)).toBeInstanceOf(ValidationError) + expect(checkedToConsentLogSubmission("[]")).toBeInstanceOf(ValidationError) + expect(checkedToConsentLogSubmission(undefined)).toBeInstanceOf(ValidationError) + }) + + it("accepts a minimal record: version + bare consent booleans", () => { + const result = checkedToConsentLogSubmission({ + version: "v1", + consents: { + transactional: { optedIn: true }, + marketing: { optedIn: false }, + }, + }) + + if (result instanceof Error) throw result + expect(result.token).toBeUndefined() + expect(result.sourceUrl).toBeUndefined() + }) +}) From 5548bfdc5a7f78b809d769c2546b80654962998d Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 12:36:16 -0700 Subject: [PATCH 2/8] fix(consent-log): CORS, IP fallback fail-closed, JSON error handler, post-log mount, route tests Review fixes for the /consent/log endpoint (ENG-568): - Add cors({ origin: "https://getflash.io" }) to the consent router so the cross-origin browser POST from the invite page passes preflight instead of being silently dropped by the fail-open page call. - Resolve client IP from x-real-ip with x-forwarded-for fallback (matching graphql-main-server), and fail closed with 503 when no IP is resolvable instead of collapsing all traffic into one shared rate-limit bucket. - Add a router-level JSON error handler so body-parser failures answer 400/413 JSON instead of Express's default HTML page (stack traces outside production). - Mount the consent router after PinoHttp (still before the /graphql JWT middleware) so the public write path appears in access logs. - Add test/flash/unit/servers/consent-log.spec.ts covering 204/400/429/503/500 branches, the hash-only token invariant, IP fallback, the error handler, and the CORS preflight. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV --- src/servers/consent-log.ts | 49 +++- src/servers/graphql-server.ts | 15 +- test/flash/unit/servers/consent-log.spec.ts | 251 ++++++++++++++++++++ 3 files changed, 305 insertions(+), 10 deletions(-) create mode 100644 test/flash/unit/servers/consent-log.spec.ts diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index 388bd5301..49fbc50ef 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -1,4 +1,5 @@ -import express from "express" +import cors from "cors" +import express, { NextFunction, Request, Response } from "express" import { UNSECURE_IP_FROM_REQUEST_OBJECT } from "@config" import { parseIps } from "@domain/accounts-ips" @@ -23,15 +24,33 @@ import { hashToken } from "@utils" const consentLogRouter = express.Router({ caseSensitive: true }) +// The submitter is a browser on getflash.io posting cross-origin to +// api.flashapp.me — without CORS the preflight fails and the (fail-open) +// page call silently drops every record. No credentials involved. +consentLogRouter.use(cors({ origin: "https://getflash.io" })) + consentLogRouter.use(express.json({ limit: "8kb" })) consentLogRouter.post("/log", async (req, res) => { - const ipString = UNSECURE_IP_FROM_REQUEST_OBJECT ? req.ip : req.headers["x-real-ip"] + // Same header conventions as the rest of the public surface: x-real-ip + // from ingress, x-forwarded-for as fallback (graphql-main-server does the + // same). A missing IP would collapse the rate limit into one global + // bucket, so treat it as an infrastructure fault, not an open door. + const ipString = UNSECURE_IP_FROM_REQUEST_OBJECT + ? req.ip + : req.headers["x-real-ip"] || req.headers["x-forwarded-for"] const ip = parseIps(ipString) + if (!ip) { + baseLogger.error( + { headers: { "x-real-ip": req.headers["x-real-ip"] } }, + "consent-log request has no resolvable client IP", + ) + return res.status(503).json({ error: "temporarily unavailable" }) + } const limited = await consumeLimiter({ rateLimitConfig: RateLimitConfig.consentLog, - keyToConsume: ip ?? "", + keyToConsume: ip, }) if (limited instanceof RateLimiterExceededError) { return res.status(429).json({ error: "too many requests" }) @@ -70,4 +89,28 @@ consentLogRouter.post("/log", async (req, res) => { return res.status(204).send() }) +// Body-parser failures (malformed JSON, oversized body) would otherwise fall +// through to Express's default handler, which answers with HTML — including a +// stack trace outside production. Keep the contract: JSON only, no internals. +consentLogRouter.use( + ( + err: Error & { type?: string }, + _req: Request, + res: Response, + // Express identifies an error handler by its arity — the 4th arg is + // required even though this terminal handler never calls it. + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _next: NextFunction, + ) => { + if (err.type === "entity.too.large") { + return res.status(413).json({ error: "body too large" }) + } + if (err.type === "entity.parse.failed") { + return res.status(400).json({ error: "invalid body" }) + } + baseLogger.error({ error: err }, "consent-log unhandled error") + return res.status(500).json({ error: "could not record consent" }) + }, +) + export default consentLogRouter diff --git a/src/servers/graphql-server.ts b/src/servers/graphql-server.ts index 01b978926..f27bdcb94 100644 --- a/src/servers/graphql-server.ts +++ b/src/servers/graphql-server.ts @@ -142,13 +142,6 @@ export const startApolloServer = async ({ app.use("/auth", authRouter) app.use("/kratos", kratosCallback) - // Public consent-evidence intake from the getflash.io/invite page - // (ENG-568). Anonymous callers, so it mounts only on the public server and - // BEFORE the JWT middleware below. - if (type === "main") { - app.use("/consent", consentLogRouter) - } - // Health check app.get( "/healthz", @@ -202,6 +195,14 @@ export const startApolloServer = async ({ }), ) + // Public consent-evidence intake from the getflash.io/invite page + // (ENG-568). Anonymous callers, so it mounts only on the public server and + // BEFORE the JWT middleware below — but AFTER PinoHttp, so a public + // unauthenticated write path shows up in the access logs. + if (type === "main") { + app.use("/consent", consentLogRouter) + } + const secret = jwksRsa.expressJwtSecret(getJwksArgs()) as GetVerificationKey // https://github.com/auth0/express-jwt/issues/288#issuecomment-1122524366 app.use(idempotencyMiddleware) // TODO: only needed for public endpoint diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts new file mode 100644 index 000000000..d1340da0f --- /dev/null +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -0,0 +1,251 @@ +import { Request, Response } from "express" + +import { ConsentLogIpRateLimiterExceededError } from "@domain/rate-limit/errors" +import { ConsentLogRepository } from "@services/mongoose/models/consent-log" +import { consumeLimiter } from "@services/rate-limit" +import consentLogRouter from "@servers/consent-log" +import { hashToken } from "@utils" + +// Deterministic IP source: always resolve from headers, the way the k8s +// ingress path does in production. +jest.mock("@config", () => ({ + ...jest.requireActual("@config"), + UNSECURE_IP_FROM_REQUEST_OBJECT: false, +})) + +jest.mock("@services/rate-limit", () => ({ consumeLimiter: jest.fn() })) + +jest.mock("@services/mongoose/models/consent-log", () => ({ + ConsentLogRepository: { create: jest.fn() }, +})) + +const mockedConsumeLimiter = consumeLimiter as jest.MockedFunction +const mockedCreate = ConsentLogRepository.create as jest.MockedFunction< + typeof ConsentLogRepository.create +> + +type Layer = { + route?: { path: string; stack: { handle: (...args: unknown[]) => unknown }[] } + handle: (...args: unknown[]) => unknown +} + +const routerStack = (consentLogRouter as unknown as { stack: Layer[] }).stack + +const postHandler = (() => { + const layer = routerStack.find((l) => l.route?.path === "/log") + if (!layer?.route) throw new Error("no route registered at /log") + return layer.route.stack[0].handle as unknown as ( + req: Request, + res: Response, + ) => Promise +})() + +// Express identifies error handlers by arity 4. +const errorHandler = (() => { + const layer = routerStack.find((l) => !l.route && l.handle.length === 4) + if (!layer) throw new Error("no error handler registered on consent router") + return layer.handle as unknown as ( + err: unknown, + req: Request, + res: Response, + next: () => void, + ) => unknown +})() + +const corsMiddleware = (() => { + const layer = routerStack.find((l) => !l.route && l.handle.name === "corsMiddleware") + if (!layer) throw new Error("no cors middleware registered on consent router") + return layer.handle as unknown as ( + req: unknown, + res: unknown, + next: () => void, + ) => unknown +})() + +const makeRes = () => { + const res = { status: jest.fn(), json: jest.fn(), send: jest.fn() } + res.status.mockReturnValue(res) + return res as unknown as Response & { + status: jest.Mock + json: jest.Mock + send: jest.Mock + } +} + +const validBody = () => ({ + version: "2026-08-01", + page: "https://getflash.io/invite", + userAgent: "jest", + timestamp: "2026-08-30T12:00:00Z", + token: "a".repeat(40), + consents: { + transactional: { optedIn: true, purpose: "receipts", frequency: "per-event" }, + marketing: { optedIn: false }, + }, +}) + +const makeReq = (overrides: Record = {}) => + ({ + headers: { "x-real-ip": "203.0.113.7" }, + body: validBody(), + ...overrides, + }) as unknown as Request + +describe("POST /consent/log", () => { + beforeEach(() => { + mockedConsumeLimiter.mockReset() + mockedCreate.mockReset() + }) + + it("returns 204 on success and stores only the token hash, never the raw token", async () => { + mockedConsumeLimiter.mockResolvedValue(1 as never) + mockedCreate.mockResolvedValue(undefined as never) + + const body = validBody() + const res = makeRes() + await postHandler(makeReq({ body }), res) + + expect(res.status).toHaveBeenCalledWith(204) + expect(mockedCreate).toHaveBeenCalledTimes(1) + + const stored = mockedCreate.mock.calls[0][0] as Record + expect(stored.inviteTokenHash).toBe(hashToken(body.token)) + expect(stored).not.toHaveProperty("token") + expect(JSON.stringify(stored)).not.toContain(body.token) + }) + + it("rate limits per IP and returns 429 when exceeded", async () => { + mockedConsumeLimiter.mockResolvedValue( + new ConsentLogIpRateLimiterExceededError() as never, + ) + + const res = makeRes() + await postHandler(makeReq(), res) + + expect(mockedConsumeLimiter).toHaveBeenCalledWith( + expect.objectContaining({ keyToConsume: "203.0.113.7" }), + ) + expect(res.status).toHaveBeenCalledWith(429) + expect(mockedCreate).not.toHaveBeenCalled() + }) + + it("fails closed with 503 when the rate-limit store is unavailable", async () => { + mockedConsumeLimiter.mockResolvedValue(new Error("redis down") as never) + + const res = makeRes() + await postHandler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(503) + expect(mockedCreate).not.toHaveBeenCalled() + }) + + it("falls back to x-forwarded-for when x-real-ip is absent", async () => { + mockedConsumeLimiter.mockResolvedValue(1 as never) + mockedCreate.mockResolvedValue(undefined as never) + + const res = makeRes() + await postHandler(makeReq({ headers: { "x-forwarded-for": "198.51.100.9" } }), res) + + expect(mockedConsumeLimiter).toHaveBeenCalledWith( + expect.objectContaining({ keyToConsume: "198.51.100.9" }), + ) + expect(res.status).toHaveBeenCalledWith(204) + }) + + it("returns 503 (not a shared global bucket) when no client IP is resolvable", async () => { + const res = makeRes() + await postHandler(makeReq({ headers: {} }), res) + + expect(res.status).toHaveBeenCalledWith(503) + expect(mockedConsumeLimiter).not.toHaveBeenCalled() + expect(mockedCreate).not.toHaveBeenCalled() + }) + + it("maps validation failures to 400 without touching the store", async () => { + mockedConsumeLimiter.mockResolvedValue(1 as never) + + const res = makeRes() + await postHandler(makeReq({ body: { version: "v1" } }), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(mockedCreate).not.toHaveBeenCalled() + }) + + it("returns 500 with a generic body when persistence fails", async () => { + mockedConsumeLimiter.mockResolvedValue(1 as never) + mockedCreate.mockRejectedValue(new Error("mongo down: secret-host:27017")) + + const res = makeRes() + await postHandler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(500) + expect(res.json).toHaveBeenCalledWith({ error: "could not record consent" }) + const sent = JSON.stringify(res.json.mock.calls) + expect(sent).not.toContain("secret-host") + }) +}) + +describe("consent router error handler", () => { + it("answers malformed JSON with a 400 JSON body, not Express's HTML page", () => { + const err = Object.assign(new SyntaxError("Unexpected token"), { + type: "entity.parse.failed", + status: 400, + }) + const res = makeRes() + errorHandler(err, {} as Request, res, jest.fn()) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith({ error: "invalid body" }) + }) + + it("answers oversized bodies with 413", () => { + const err = Object.assign(new Error("request entity too large"), { + type: "entity.too.large", + status: 413, + }) + const res = makeRes() + errorHandler(err, {} as Request, res, jest.fn()) + + expect(res.status).toHaveBeenCalledWith(413) + expect(res.json).toHaveBeenCalledWith({ error: "body too large" }) + }) + + it("answers anything else with a generic 500, leaking no internals", () => { + const err = new Error("boom with /etc/secrets path") + const res = makeRes() + errorHandler(err, {} as Request, res, jest.fn()) + + expect(res.status).toHaveBeenCalledWith(500) + const sent = JSON.stringify(res.json.mock.calls) + expect(sent).not.toContain("/etc/secrets") + }) +}) + +describe("consent router CORS", () => { + it("answers the getflash.io preflight with Access-Control-Allow-Origin", () => { + const headers: Record = {} + const req = { + method: "OPTIONS", + headers: { + "origin": "https://getflash.io", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type", + }, + } + const res = { + statusCode: 200, + setHeader: (name: string, value: string | string[]) => { + headers[name.toLowerCase()] = String(value) + }, + getHeader: (name: string) => headers[name.toLowerCase()], + end: jest.fn(), + } + const next = jest.fn() + + corsMiddleware(req, res, next) + + expect(headers["access-control-allow-origin"]).toBe("https://getflash.io") + expect(res.end).toHaveBeenCalled() + expect(next).not.toHaveBeenCalled() + }) +}) From 9105dee0ad4e0dca1f71121283f645febe1dbf92 Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 12:41:10 -0700 Subject: [PATCH 3/8] fix(consent-log): redact consent bodies from access logs, allow www origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round-2 fixes for the POST /consent/log endpoint (ENG-568): - pino-http re-evaluates customProps at response finish, after the consent router's express.json() has populated req.body — so the access log's 'request completed' line carried the raw invite token on every status path. The body field now goes through redactConsentBodyForLog(), which replaces /consent request bodies with a redaction marker, keeping the access-log line while honoring the raw-token-never-touches-disk rule. - CORS now allows both https://getflash.io and https://www.getflash.io so www visitors' preflights don't silently drop consent records again. - Tests: redaction helper (consent vs non-consent paths), www preflight, and a negative test that arbitrary origins get no allow-origin header. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV --- src/servers/consent-log.ts | 16 +++- src/servers/graphql-server.ts | 13 +++- test/flash/unit/servers/consent-log.spec.ts | 83 ++++++++++++++++++++- 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index 49fbc50ef..7f51ae706 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -27,7 +27,11 @@ const consentLogRouter = express.Router({ caseSensitive: true }) // The submitter is a browser on getflash.io posting cross-origin to // api.flashapp.me — without CORS the preflight fails and the (fail-open) // page call silently drops every record. No credentials involved. -consentLogRouter.use(cors({ origin: "https://getflash.io" })) +// Both the apex and www hosts are allowed: if a visitor reaches the invite +// page at www.getflash.io (or the www→apex redirect hasn't happened before +// the page fires its fail-open POST), an apex-only allowlist would silently +// drop those records — the original incident, resurrected for that subset. +consentLogRouter.use(cors({ origin: ["https://getflash.io", "https://www.getflash.io"] })) consentLogRouter.use(express.json({ limit: "8kb" })) @@ -113,4 +117,14 @@ consentLogRouter.use( }, ) +// Used by the app-level PinoHttp access logger (graphql-server.ts). pino-http +// re-evaluates customProps at response finish — by then express.json() inside +// this router has populated req.body, so logging the raw body would write the +// raw invite token to the access logs on every status path (204/400/429/503). +// Same rule as the invites collection: the raw token never touches disk. +// (authRouter dodges this by mounting BEFORE PinoHttp; the consent router +// mounts after it so the public write path still gets an access-log line.) +export const redactConsentBodyForLog = (req: { url?: string; body?: unknown }) => + req.url?.startsWith("/consent") ? "[consent body redacted]" : req.body + export default consentLogRouter diff --git a/src/servers/graphql-server.ts b/src/servers/graphql-server.ts index f27bdcb94..0d0c61b81 100644 --- a/src/servers/graphql-server.ts +++ b/src/servers/graphql-server.ts @@ -29,7 +29,7 @@ import { parseUnknownDomainErrorFromUnknown } from "@domain/shared" import { MAXIMUM_QUERY_COMPLEXITY, createComplexityPlugin } from "./plugins/complexity" import authRouter from "./authorization" -import consentLogRouter from "./consent-log" +import consentLogRouter, { redactConsentBodyForLog } from "./consent-log" import kratosCallback from "./event-handlers/kratos" import { apiKeyRateLimitMiddleware } from "./middlewares/api-key-rate-limit" import healthzHandler from "./middlewares/healthz" @@ -162,8 +162,13 @@ export const startApolloServer = async ({ // @ts-ignore-next-line no-implicit-any error const account = req["gqlContext"]?.domainAccount return { + // pino-http evaluates customProps again at response finish, after + // the consent router's express.json() has populated req.body — so + // the raw invite token would land in the access logs without this + // redaction. Guarded by tests in + // test/flash/unit/servers/consent-log.spec.ts. // @ts-ignore-next-line no-implicit-any error - "body": req["body"], + "body": redactConsentBodyForLog(req), // @ts-ignore-next-line no-implicit-any error "token.sub": req["token"]?.sub, // @ts-ignore-next-line no-implicit-any error @@ -198,7 +203,9 @@ export const startApolloServer = async ({ // Public consent-evidence intake from the getflash.io/invite page // (ENG-568). Anonymous callers, so it mounts only on the public server and // BEFORE the JWT middleware below — but AFTER PinoHttp, so a public - // unauthenticated write path shows up in the access logs. + // unauthenticated write path shows up in the access logs. The logged body + // for /consent requests is redacted (redactConsentBodyForLog above) so the + // raw invite token never reaches the logs. if (type === "main") { app.use("/consent", consentLogRouter) } diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts index d1340da0f..51d911951 100644 --- a/test/flash/unit/servers/consent-log.spec.ts +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -3,7 +3,7 @@ import { Request, Response } from "express" import { ConsentLogIpRateLimiterExceededError } from "@domain/rate-limit/errors" import { ConsentLogRepository } from "@services/mongoose/models/consent-log" import { consumeLimiter } from "@services/rate-limit" -import consentLogRouter from "@servers/consent-log" +import consentLogRouter, { redactConsentBodyForLog } from "@servers/consent-log" import { hashToken } from "@utils" // Deterministic IP source: always resolve from headers, the way the k8s @@ -248,4 +248,85 @@ describe("consent router CORS", () => { expect(res.end).toHaveBeenCalled() expect(next).not.toHaveBeenCalled() }) + + it("also answers the www.getflash.io preflight — www visitors must not be dropped", () => { + const headers: Record = {} + const req = { + method: "OPTIONS", + headers: { + "origin": "https://www.getflash.io", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type", + }, + } + const res = { + statusCode: 200, + setHeader: (name: string, value: string | string[]) => { + headers[name.toLowerCase()] = String(value) + }, + getHeader: (name: string) => headers[name.toLowerCase()], + end: jest.fn(), + } + const next = jest.fn() + + corsMiddleware(req, res, next) + + expect(headers["access-control-allow-origin"]).toBe("https://www.getflash.io") + expect(res.end).toHaveBeenCalled() + expect(next).not.toHaveBeenCalled() + }) + + it("does not allow arbitrary origins", () => { + const headers: Record = {} + const req = { + method: "OPTIONS", + headers: { + "origin": "https://evil.example", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type", + }, + } + const res = { + statusCode: 200, + setHeader: (name: string, value: string | string[]) => { + headers[name.toLowerCase()] = String(value) + }, + getHeader: (name: string) => headers[name.toLowerCase()], + end: jest.fn(), + } + const next = jest.fn() + + corsMiddleware(req, res, next) + + expect(headers["access-control-allow-origin"]).toBeUndefined() + }) +}) + +describe("access-log body redaction (redactConsentBodyForLog)", () => { + // pino-http re-evaluates customProps at response finish — by then + // express.json() inside the consent router has populated req.body, so an + // unredacted body log would write the raw invite token to disk on every + // status path (204/400/429/503). + it("redacts the body for /consent/log requests so the raw token never reaches logs", () => { + const token = "a".repeat(40) + const logged = redactConsentBodyForLog({ + url: "/consent/log", + body: validBody(), + }) + + expect(logged).toBe("[consent body redacted]") + expect(JSON.stringify(logged)).not.toContain(token) + }) + + it("redacts any path under /consent, on every status path", () => { + expect(redactConsentBodyForLog({ url: "/consent", body: { token: "x" } })).toBe( + "[consent body redacted]", + ) + }) + + it("leaves non-consent request bodies untouched for the access log", () => { + const body = { query: "{ me { id } }" } + expect(redactConsentBodyForLog({ url: "/graphql", body })).toBe(body) + expect(redactConsentBodyForLog({ body })).toBe(body) + }) }) From 0bb0b2546e70c60714ef0a7567f8e75c62f6fe02 Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 12:46:26 -0700 Subject: [PATCH 4/8] fix(consent-log): key access-log redaction on originalUrl, not the router-mutated url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Express 4 strips the mount path off req.url inside a router mounted at /consent (url becomes "/log") and never restores it — the handler ends the response, so the restoring next() never runs. pino-http re-evaluates customProps at response-finish against that mutated req, the one moment req.body actually holds the token-bearing payload, so keying the redaction on req.url passed the raw invite token into the access log on every status path. Keyed on originalUrl (Express never mutates it); tests now model the finish-time shape ({url: "/log", originalUrl: "/consent/log"}) and pin the url-only failure mode. Empirically verified against the repo's express + pino-http: real POST, grep of the emitted line — new helper redacts, url-only helper leaks. --- src/servers/consent-log.ts | 17 ++++++++-- test/flash/unit/servers/consent-log.spec.ts | 37 +++++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index 7f51ae706..1d2104fa5 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -124,7 +124,20 @@ consentLogRouter.use( // Same rule as the invites collection: the raw token never touches disk. // (authRouter dodges this by mounting BEFORE PinoHttp; the consent router // mounts after it so the public write path still gets an access-log line.) -export const redactConsentBodyForLog = (req: { url?: string; body?: unknown }) => - req.url?.startsWith("/consent") ? "[consent body redacted]" : req.body +// Keyed on originalUrl, never url: Express 4 strips the mount path off +// req.url when dispatching into a router mounted at "/consent" (req.url +// becomes "/log") and only restores it on a later next() that never comes, +// because every consent handler terminates the response. pino-http evaluates +// customProps again at response-finish against that mutated req — the moment +// req.body actually holds the parsed (token-bearing) payload — so matching on +// req.url would pass raw tokens straight into the access log. +export const redactConsentBodyForLog = (req: { + originalUrl?: string + url?: string + body?: unknown +}) => + (req.originalUrl ?? req.url)?.startsWith("/consent") + ? "[consent body redacted]" + : req.body export default consentLogRouter diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts index 51d911951..587eb422e 100644 --- a/test/flash/unit/servers/consent-log.spec.ts +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -307,10 +307,15 @@ describe("access-log body redaction (redactConsentBodyForLog)", () => { // express.json() inside the consent router has populated req.body, so an // unredacted body log would write the raw invite token to disk on every // status path (204/400/429/503). - it("redacts the body for /consent/log requests so the raw token never reaches logs", () => { + it("redacts at RESPONSE-FINISH shape: url mutated to /log by the router mount, originalUrl intact", () => { + // Inside a router mounted at "/consent", Express rewrites req.url to + // "/log" and never restores it (the handler ends the response, so the + // restoring next() never runs). This is the shape pino-http actually + // evaluates when the body is populated — url alone is untrustworthy here. const token = "a".repeat(40) const logged = redactConsentBodyForLog({ - url: "/consent/log", + url: "/log", + originalUrl: "/consent/log", body: validBody(), }) @@ -318,15 +323,33 @@ describe("access-log body redaction (redactConsentBodyForLog)", () => { expect(JSON.stringify(logged)).not.toContain(token) }) - it("redacts any path under /consent, on every status path", () => { - expect(redactConsentBodyForLog({ url: "/consent", body: { token: "x" } })).toBe( - "[consent body redacted]", - ) + it("redacts at middleware-time shape too (originalUrl === url)", () => { + expect( + redactConsentBodyForLog({ + url: "/consent/log", + originalUrl: "/consent/log", + body: { token: "x" }, + }), + ).toBe("[consent body redacted]") + }) + + it("would leak if the helper keyed on the mutated url — pin the failure mode", () => { + // The regression this guards: a helper reading only req.url sees "/log" + // at finish time and returns the raw body. originalUrl must win. + expect( + redactConsentBodyForLog({ + url: "/log", + originalUrl: "/consent/log", + body: { token: "x" }, + }), + ).not.toEqual({ token: "x" }) }) it("leaves non-consent request bodies untouched for the access log", () => { const body = { query: "{ me { id } }" } - expect(redactConsentBodyForLog({ url: "/graphql", body })).toBe(body) + expect( + redactConsentBodyForLog({ url: "/graphql", originalUrl: "/graphql", body }), + ).toBe(body) expect(redactConsentBodyForLog({ body })).toBe(body) }) }) From 31a0b056f6459a6851d18e4839185fa9a9dfab3a Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 12:50:29 -0700 Subject: [PATCH 5/8] =?UTF-8?q?fix:=20apply=20review=20fixes=20=E2=80=94?= =?UTF-8?q?=20restore=20rate-limit=20comment=20adjacency,=20accept=20canon?= =?UTF-8?q?ical=20clientTimestamp?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move the consentLog rate-limit block below fygaroTopupAllowance so each comment sits directly above the config it describes. - Accept the canonical clientTimestamp field alongside the page's timestamp, mirroring the sourceUrl/page convergence pattern, with a domain-spec case. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV --- src/domain/consent-log/index.ts | 8 +++++++- src/domain/rate-limit/index.ts | 10 +++++----- test/flash/unit/domain/consent-log.spec.ts | 14 ++++++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/domain/consent-log/index.ts b/src/domain/consent-log/index.ts index a0694069a..c53fcad8d 100644 --- a/src/domain/consent-log/index.ts +++ b/src/domain/consent-log/index.ts @@ -92,7 +92,13 @@ export const checkedToConsentLogSubmission = ( if (sourceUrl instanceof ValidationError) return sourceUrl const userAgent = boundedString(raw.userAgent, MAX_USER_AGENT, "userAgent") if (userAgent instanceof ValidationError) return userAgent - const clientTimestamp = boundedString(raw.timestamp, MAX_TIMESTAMP, "timestamp") + // Same convergence path as sourceUrl: the page sends `timestamp` today; + // accept the canonical `clientTimestamp` too. + const clientTimestamp = boundedString( + raw.clientTimestamp ?? raw.timestamp, + MAX_TIMESTAMP, + "timestamp", + ) if (clientTimestamp instanceof ValidationError) return clientTimestamp const token = boundedString(raw.token, MAX_TOKEN, "token") if (token instanceof ValidationError) return token diff --git a/src/domain/rate-limit/index.ts b/src/domain/rate-limit/index.ts index 0c79f5e97..f5564066e 100644 --- a/src/domain/rate-limit/index.ts +++ b/src/domain/rate-limit/index.ts @@ -113,6 +113,11 @@ export const RateLimitConfig: { [key: string]: RateLimitConfig } = { // no amount argument, so nothing can short-circuit before the trailing-24h // list query runs. Its own key so a customer who has spent the mutation's // budget can still be told what is left of their allowance. + fygaroTopupAllowance: { + key: RateLimitPrefix.fygaroTopupAllowance, + limits: getFygaroTopupAllowanceAttemptLimits(), + error: FygaroTopupAllowanceRateLimiterExceededError, + }, // Public, unauthenticated write endpoint (consent evidence from the // getflash.io/invite page). Per-IP: one legitimate submission per accept // click, so the ceiling is far above any real use and far below abuse. @@ -121,9 +126,4 @@ export const RateLimitConfig: { [key: string]: RateLimitConfig } = { limits: getConsentLogAttemptLimits(), error: ConsentLogIpRateLimiterExceededError, }, - fygaroTopupAllowance: { - key: RateLimitPrefix.fygaroTopupAllowance, - limits: getFygaroTopupAllowanceAttemptLimits(), - error: FygaroTopupAllowanceRateLimiterExceededError, - }, } diff --git a/test/flash/unit/domain/consent-log.spec.ts b/test/flash/unit/domain/consent-log.spec.ts index 62ff49140..73b2e5854 100644 --- a/test/flash/unit/domain/consent-log.spec.ts +++ b/test/flash/unit/domain/consent-log.spec.ts @@ -30,6 +30,20 @@ describe("checkedToConsentLogSubmission", () => { expect(result.sourceUrl).toBe("https://getflash.io/invite/?token=abc") }) + it("accepts the canonical field names (sourceUrl, clientTimestamp) too", () => { + const body = validBody() as Record + delete body.page + delete body.timestamp + body.sourceUrl = "https://getflash.io/invite/?token=abc" + body.clientTimestamp = "2026-09-01T18:00:00.000Z" + + const result = checkedToConsentLogSubmission(body) + + if (result instanceof Error) throw result + expect(result.sourceUrl).toBe("https://getflash.io/invite/?token=abc") + expect(result.clientTimestamp).toBe("2026-09-01T18:00:00.000Z") + }) + it("discards fields it does not know", () => { const body = { ...validBody(), admin: true, $where: "1" } From 35c3db10b238b70d1d6ea858984bac2c68321840 Mon Sep 17 00:00:00 2001 From: Dread Date: Wed, 2 Sep 2026 14:06:38 -0700 Subject: [PATCH 6/8] fix: consume consent-log rate limit before body parsing, tighten redaction path match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two code-review fixes on PR #498: - The per-IP rate limiter was only consumed inside the /log route handler, which runs after express.json() has already parsed the body. A body that fails to parse (oversized or malformed JSON) is routed straight to the router's error handler by Express, skipping the route handler and the limiter entirely — so an attacker could flood the endpoint at wire speed with exactly that traffic shape and never be rate limited. Moved IP resolution + limiter consumption into router-level middleware mounted before express.json(), so every request reaching this router is charged against the bucket regardless of whether its body goes on to parse. Added an end-to-end regression test that runs the actual (unmocked) body-parser through the real middleware chain with an oversized and a malformed body, asserting the limiter is still consumed. - redactConsentBodyForLog matched with startsWith("/consent"), which would also swallow any future unrelated route mounted alongside this router (e.g. /consent-status). Tightened to startsWith("/consent/") so redaction stays scoped to this router's actual mount point, with a test pinning the non-match. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- src/servers/consent-log.ts | 40 +++- test/flash/unit/servers/consent-log.spec.ts | 220 ++++++++++++++++---- 2 files changed, 217 insertions(+), 43 deletions(-) diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index 1d2104fa5..eb063e07a 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -33,9 +33,22 @@ const consentLogRouter = express.Router({ caseSensitive: true }) // drop those records — the original incident, resurrected for that subset. consentLogRouter.use(cors({ origin: ["https://getflash.io", "https://www.getflash.io"] })) -consentLogRouter.use(express.json({ limit: "8kb" })) - -consentLogRouter.post("/log", async (req, res) => { +// Resolves the caller's IP and consumes the per-IP rate limit BEFORE +// express.json() gets a chance to run. This has to be router-level +// middleware, not logic inside the /log handler: express.json() routes a +// parse failure (oversized body -> "entity.too.large", malformed JSON -> +// "entity.parse.failed") straight to the error handler below, skipping the +// route handler entirely. If the limiter were only consumed inside that +// handler, an attacker could flood this endpoint at wire speed with +// oversized/malformed bodies and never touch the limiter — only a +// well-formed submission would ever be capped. Consuming here means every +// request that reaches this router is charged against the bucket +// regardless of whether its body goes on to parse. +const enforceConsentLogRateLimit = async ( + req: Request, + res: Response, + next: NextFunction, +) => { // Same header conventions as the rest of the public surface: x-real-ip // from ingress, x-forwarded-for as fallback (graphql-main-server does the // same). A missing IP would collapse the rate limit into one global @@ -67,6 +80,19 @@ consentLogRouter.post("/log", async (req, res) => { return res.status(503).json({ error: "temporarily unavailable" }) } + // Hand the resolved IP to the route handler (it's stored on the record) + // without re-deriving it from headers a second time. + res.locals.consentLogIp = ip + return next() +} + +consentLogRouter.use(enforceConsentLogRateLimit) + +consentLogRouter.use(express.json({ limit: "8kb" })) + +consentLogRouter.post("/log", async (req, res) => { + const ip = res.locals.consentLogIp as IpAddress + const submission = checkedToConsentLogSubmission(req.body) if (submission instanceof ValidationError) { return res.status(400).json({ error: submission.message }) @@ -131,12 +157,18 @@ consentLogRouter.use( // customProps again at response-finish against that mutated req — the moment // req.body actually holds the parsed (token-bearing) payload — so matching on // req.url would pass raw tokens straight into the access log. +// +// Matched with the trailing slash ("/consent/", not "/consent") so this +// stays scoped to this router's actual mount point — a bare "/consent" +// prefix would also swallow any future unrelated route mounted alongside it +// (e.g. "/consent-status", "/consent-preferences"), redacting bodies that +// have nothing to do with this router. export const redactConsentBodyForLog = (req: { originalUrl?: string url?: string body?: unknown }) => - (req.originalUrl ?? req.url)?.startsWith("/consent") + (req.originalUrl ?? req.url)?.startsWith("/consent/") ? "[consent body redacted]" : req.body diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts index 587eb422e..531dddb9a 100644 --- a/test/flash/unit/servers/consent-log.spec.ts +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -1,3 +1,5 @@ +import { Readable } from "stream" + import { Request, Response } from "express" import { ConsentLogIpRateLimiterExceededError } from "@domain/rate-limit/errors" @@ -62,13 +64,46 @@ const corsMiddleware = (() => { ) => unknown })() -const makeRes = () => { - const res = { status: jest.fn(), json: jest.fn(), send: jest.fn() } +// The per-IP rate limiter, extracted as router-level middleware so it always +// runs ahead of body parsing — see the comment above its definition in +// src/servers/consent-log.ts for why it can't live inside the /log handler. +const rateLimitMiddleware = (() => { + const layer = routerStack.find( + (l) => !l.route && l.handle.name === "enforceConsentLogRateLimit", + ) + if (!layer) throw new Error("no rate-limit middleware registered on consent router") + return layer.handle as unknown as ( + req: Request, + res: Response, + next: (err?: unknown) => void, + ) => Promise +})() + +// The real (unmocked) body-parser middleware express.json() installs — used +// by the end-to-end test below to prove the actual production ordering. +const jsonParserMiddleware = (() => { + const layer = routerStack.find((l) => !l.route && l.handle.name === "jsonParser") + if (!layer) throw new Error("no json body-parser registered on consent router") + return layer.handle as unknown as ( + req: Request, + res: Response, + next: (err?: unknown) => void, + ) => void +})() + +const DEFAULT_IP = "203.0.113.7" as IpAddress + +// postHandler now expects the rate-limit middleware to have already resolved +// the caller's IP onto res.locals — default it here so tests that aren't +// specifically about IP resolution don't have to repeat it. +const makeRes = (locals: Record = { consentLogIp: DEFAULT_IP }) => { + const res = { status: jest.fn(), json: jest.fn(), send: jest.fn(), locals } res.status.mockReturnValue(res) return res as unknown as Response & { status: jest.Mock json: jest.Mock send: jest.Mock + locals: Record } } @@ -98,7 +133,6 @@ describe("POST /consent/log", () => { }) it("returns 204 on success and stores only the token hash, never the raw token", async () => { - mockedConsumeLimiter.mockResolvedValue(1 as never) mockedCreate.mockResolvedValue(undefined as never) const body = validBody() @@ -109,79 +143,173 @@ describe("POST /consent/log", () => { expect(mockedCreate).toHaveBeenCalledTimes(1) const stored = mockedCreate.mock.calls[0][0] as Record + expect(stored.ip).toBe(DEFAULT_IP) expect(stored.inviteTokenHash).toBe(hashToken(body.token)) expect(stored).not.toHaveProperty("token") expect(JSON.stringify(stored)).not.toContain(body.token) }) - it("rate limits per IP and returns 429 when exceeded", async () => { - mockedConsumeLimiter.mockResolvedValue( - new ConsentLogIpRateLimiterExceededError() as never, - ) - + it("maps validation failures to 400 without touching the store", async () => { const res = makeRes() - await postHandler(makeReq(), res) + await postHandler(makeReq({ body: { version: "v1" } }), res) - expect(mockedConsumeLimiter).toHaveBeenCalledWith( - expect.objectContaining({ keyToConsume: "203.0.113.7" }), - ) - expect(res.status).toHaveBeenCalledWith(429) + expect(res.status).toHaveBeenCalledWith(400) expect(mockedCreate).not.toHaveBeenCalled() }) - it("fails closed with 503 when the rate-limit store is unavailable", async () => { - mockedConsumeLimiter.mockResolvedValue(new Error("redis down") as never) + it("returns 500 with a generic body when persistence fails", async () => { + mockedCreate.mockRejectedValue(new Error("mongo down: secret-host:27017")) const res = makeRes() await postHandler(makeReq(), res) - expect(res.status).toHaveBeenCalledWith(503) - expect(mockedCreate).not.toHaveBeenCalled() + expect(res.status).toHaveBeenCalledWith(500) + expect(res.json).toHaveBeenCalledWith({ error: "could not record consent" }) + const sent = JSON.stringify(res.json.mock.calls) + expect(sent).not.toContain("secret-host") + }) +}) + +// The per-IP limiter lives here — router-level middleware mounted BEFORE +// express.json() — precisely so it always runs, whether or not the body +// that follows ever parses. See the end-to-end test below for the +// regression this guards against. +describe("consent router rate limiting (enforceConsentLogRateLimit)", () => { + beforeEach(() => { + mockedConsumeLimiter.mockReset() + }) + + it("consumes the limiter, stores the resolved ip on res.locals, and calls next()", async () => { + mockedConsumeLimiter.mockResolvedValue(1 as never) + + const res = makeRes({}) + const next = jest.fn() + await rateLimitMiddleware(makeReq(), res, next) + + expect(mockedConsumeLimiter).toHaveBeenCalledWith( + expect.objectContaining({ keyToConsume: "203.0.113.7" }), + ) + expect(res.locals.consentLogIp).toBe("203.0.113.7") + expect(next).toHaveBeenCalledTimes(1) + expect(res.status).not.toHaveBeenCalled() }) it("falls back to x-forwarded-for when x-real-ip is absent", async () => { mockedConsumeLimiter.mockResolvedValue(1 as never) - mockedCreate.mockResolvedValue(undefined as never) - const res = makeRes() - await postHandler(makeReq({ headers: { "x-forwarded-for": "198.51.100.9" } }), res) + const res = makeRes({}) + const next = jest.fn() + await rateLimitMiddleware( + makeReq({ headers: { "x-forwarded-for": "198.51.100.9" } }), + res, + next, + ) expect(mockedConsumeLimiter).toHaveBeenCalledWith( expect.objectContaining({ keyToConsume: "198.51.100.9" }), ) - expect(res.status).toHaveBeenCalledWith(204) + expect(res.locals.consentLogIp).toBe("198.51.100.9") + expect(next).toHaveBeenCalledTimes(1) }) - it("returns 503 (not a shared global bucket) when no client IP is resolvable", async () => { - const res = makeRes() - await postHandler(makeReq({ headers: {} }), res) + it("returns 429 and does not call next() when the limit is exceeded", async () => { + mockedConsumeLimiter.mockResolvedValue( + new ConsentLogIpRateLimiterExceededError() as never, + ) + + const res = makeRes({}) + const next = jest.fn() + await rateLimitMiddleware(makeReq(), res, next) + + expect(res.status).toHaveBeenCalledWith(429) + expect(next).not.toHaveBeenCalled() + expect(res.locals.consentLogIp).toBeUndefined() + }) + + it("fails closed with 503 and does not call next() when the rate-limit store is unavailable", async () => { + mockedConsumeLimiter.mockResolvedValue(new Error("redis down") as never) + + const res = makeRes({}) + const next = jest.fn() + await rateLimitMiddleware(makeReq(), res, next) + + expect(res.status).toHaveBeenCalledWith(503) + expect(next).not.toHaveBeenCalled() + }) + + it("returns 503 (not a shared global bucket) without consuming the limiter when no client IP is resolvable", async () => { + const res = makeRes({}) + const next = jest.fn() + await rateLimitMiddleware(makeReq({ headers: {} }), res, next) expect(res.status).toHaveBeenCalledWith(503) expect(mockedConsumeLimiter).not.toHaveBeenCalled() - expect(mockedCreate).not.toHaveBeenCalled() + expect(next).not.toHaveBeenCalled() + }) +}) + +// Regression test for the finding: the limiter used to be consumed only +// inside the /log route handler, which runs AFTER express.json() has parsed +// the body. A malformed or oversized body never reaches the route handler — +// body-parser routes those straight to the error handler — so an attacker +// could flood the endpoint with exactly that traffic shape and never once +// touch the limiter. Wiring the limiter as router-level middleware ahead of +// express.json() closes that gap. This exercises the ACTUAL production +// middleware pulled off the router (not a reimplementation), including the +// real body-parser, so it would have failed against the old ordering. +describe("consent router — rate limiting runs even when the body fails to parse", () => { + beforeEach(() => { + mockedConsumeLimiter.mockReset() + mockedCreate.mockReset() }) - it("maps validation failures to 400 without touching the store", async () => { + const makeRawReq = (bodyStr: string) => { + const req = Readable.from([Buffer.from(bodyStr)]) as unknown as Request & Readable + ;(req as unknown as { headers: Record }).headers = { + "x-real-ip": "203.0.113.7", + "content-type": "application/json", + "content-length": String(Buffer.byteLength(bodyStr)), + } + ;(req as unknown as { method: string }).method = "POST" + return req + } + + const runChain = async (bodyStr: string) => { mockedConsumeLimiter.mockResolvedValue(1 as never) - const res = makeRes() - await postHandler(makeReq({ body: { version: "v1" } }), res) + const req = makeRawReq(bodyStr) + const res = makeRes({}) + + await new Promise((resolve) => { + rateLimitMiddleware(req, res, (rateLimitErr) => { + if (rateLimitErr) return resolve() + jsonParserMiddleware(req, res, (parseErr) => { + if (parseErr) { + errorHandler(parseErr, req, res, jest.fn()) + } + resolve() + }) + }) + }) - expect(res.status).toHaveBeenCalledWith(400) + return res + } + + it("still consumes the limiter for an oversized body, and the body-parser error still surfaces as 413", async () => { + const oversized = JSON.stringify({ padding: "a".repeat(9000) }) + const res = await runChain(oversized) + + expect(mockedConsumeLimiter).toHaveBeenCalledTimes(1) + expect(res.status).toHaveBeenCalledWith(413) expect(mockedCreate).not.toHaveBeenCalled() }) - it("returns 500 with a generic body when persistence fails", async () => { - mockedConsumeLimiter.mockResolvedValue(1 as never) - mockedCreate.mockRejectedValue(new Error("mongo down: secret-host:27017")) - - const res = makeRes() - await postHandler(makeReq(), res) + it("still consumes the limiter for malformed JSON, and the body-parser error still surfaces as 400", async () => { + const res = await runChain("{not valid json") - expect(res.status).toHaveBeenCalledWith(500) - expect(res.json).toHaveBeenCalledWith({ error: "could not record consent" }) - const sent = JSON.stringify(res.json.mock.calls) - expect(sent).not.toContain("secret-host") + expect(mockedConsumeLimiter).toHaveBeenCalledTimes(1) + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith({ error: "invalid body" }) }) }) @@ -352,4 +480,18 @@ describe("access-log body redaction (redactConsentBodyForLog)", () => { ).toBe(body) expect(redactConsentBodyForLog({ body })).toBe(body) }) + + it("does not over-match a sibling route that merely starts with /consent (e.g. /consent-status)", () => { + // A loose "/consent" prefix (no trailing slash) would also swallow any + // future unrelated route mounted alongside this router — this pins the + // helper to this router's actual mount point instead. + const body = { some: "unrelated payload" } + expect( + redactConsentBodyForLog({ + url: "/consent-status", + originalUrl: "/consent-status", + body, + }), + ).toBe(body) + }) }) From 0675439ef5a5546dd45bb4ce8b4ecf38db4ba0b3 Mon Sep 17 00:00:00 2001 From: Dread Date: Wed, 2 Sep 2026 14:15:43 -0700 Subject: [PATCH 7/8] fix: redact consent-log access-log body case-insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Express dispatches into the /consent mount case-insensitively (no case-sensitive-routing setting on the app-level express()), so a request to /CONSENT/log still reaches the /log route and persists a real submission, but its originalUrl fails the case-sensitive startsWith("/consent/") check in redactConsentBodyForLog — leaking the raw invite token into the access log via pino-http's response-finish customProps re-evaluation. Lowercase both sides of the comparison and add regression tests pinning a mixed-case originalUrl (/CONSENT/log, /Consent/Log) to the redacted marker. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- src/servers/consent-log.ts | 12 +++++++++- test/flash/unit/servers/consent-log.spec.ts | 26 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index eb063e07a..638aa78eb 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -163,12 +163,22 @@ consentLogRouter.use( // prefix would also swallow any future unrelated route mounted alongside it // (e.g. "/consent-status", "/consent-preferences"), redacting bodies that // have nothing to do with this router. +// +// Compared lower-cased on both sides: the app-level mount +// (graphql-server.ts, `app.use("/consent", consentLogRouter)`) runs on a +// bare express() with no "case sensitive routing" setting, so Express +// matches that mount case-insensitively regardless of this router's own +// `caseSensitive: true` (which only governs matching *within* the router, +// e.g. "/log" vs "/Log"). A request to "/CONSENT/log" still dispatches into +// this router and still persists — its originalUrl is just differently +// cased. A plain case-sensitive startsWith here would miss that request +// and leak its raw invite token into the access log. export const redactConsentBodyForLog = (req: { originalUrl?: string url?: string body?: unknown }) => - (req.originalUrl ?? req.url)?.startsWith("/consent/") + (req.originalUrl ?? req.url)?.toLowerCase().startsWith("/consent/") ? "[consent body redacted]" : req.body diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts index 531dddb9a..9d0adeb65 100644 --- a/test/flash/unit/servers/consent-log.spec.ts +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -494,4 +494,30 @@ describe("access-log body redaction (redactConsentBodyForLog)", () => { }), ).toBe(body) }) + + it("redacts a differently-cased mount segment that still dispatches into this router (e.g. /CONSENT/log)", () => { + // The app-level mount (`app.use("/consent", consentLogRouter)` in + // graphql-server.ts) runs on a bare express() with no + // "case sensitive routing" setting, so Express matches that mount + // case-insensitively regardless of this router's own + // `caseSensitive: true` (which only governs matching *within* the + // router). A request to "/CONSENT/log" still reaches the "/log" route + // and still persists a real submission — only originalUrl's casing + // differs from the lower-case check. A case-sensitive startsWith here + // would miss it and leak the raw invite token to the access log. + const token = "a".repeat(40) + const logged = redactConsentBodyForLog({ + originalUrl: "/CONSENT/log", + body: validBody(), + }) + + expect(logged).toBe("[consent body redacted]") + expect(JSON.stringify(logged)).not.toContain(token) + }) + + it("redacts regardless of casing on either the mount segment or the sub-route segment", () => { + expect( + redactConsentBodyForLog({ originalUrl: "/Consent/Log", body: validBody() }), + ).toBe("[consent body redacted]") + }) }) From 9784ae355256aa0db9f628e4f1f1f6bda5819a44 Mon Sep 17 00:00:00 2001 From: Dread Date: Wed, 2 Sep 2026 14:43:17 -0700 Subject: [PATCH 8/8] fix(consent-log): stop deriving redaction from the URL, gate on the rate limiter's own IP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review finding on #498: the trailing-slash tightening added alongside the round-2 case-insensitivity fix (matching "/consent/" instead of a bare "/consent" prefix, to stop over-matching the sibling "/consent-status" route) reintroduced the same token-in-logs bug through a third URL shape. A bare "POST /consent" (no trailing slash, no "/log") still dispatches into this router — Express matches the "/consent" mount before route matching happens, so enforceConsentLogRateLimit and express.json() both run, populating req.body with a real invite token — before falling through to a 404 because nothing matches "/" against the registered "/log" route. pino-http re-evaluates customProps at response finish regardless of the 404, and "/consent".toLowerCase().startsWith("/consent/") is false, so the body went through unredacted. This is the second distinct URL-shape bypass in as many rounds (req.url vs originalUrl, then the trailing-slash boundary), so stop matching on the URL at all: redactConsentBodyForLog now gates on res.locals.consentLogIp, which enforceConsentLogRateLimit sets unconditionally for every request Express dispatches into this router, independent of path shape or whether any route inside ultimately matches. Callers outside the router (e.g. /graphql) never set it, so their bodies still pass through untouched. Tests rewritten for the new signature, including a regression case pinning the bare-mount-path bypass this fix closes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- src/servers/consent-log.ts | 50 ++++----- src/servers/graphql-server.ts | 4 +- test/flash/unit/servers/consent-log.spec.ts | 106 ++++++++------------ 3 files changed, 62 insertions(+), 98 deletions(-) diff --git a/src/servers/consent-log.ts b/src/servers/consent-log.ts index 638aa78eb..89ceb3d79 100644 --- a/src/servers/consent-log.ts +++ b/src/servers/consent-log.ts @@ -150,36 +150,26 @@ consentLogRouter.use( // Same rule as the invites collection: the raw token never touches disk. // (authRouter dodges this by mounting BEFORE PinoHttp; the consent router // mounts after it so the public write path still gets an access-log line.) -// Keyed on originalUrl, never url: Express 4 strips the mount path off -// req.url when dispatching into a router mounted at "/consent" (req.url -// becomes "/log") and only restores it on a later next() that never comes, -// because every consent handler terminates the response. pino-http evaluates -// customProps again at response-finish against that mutated req — the moment -// req.body actually holds the parsed (token-bearing) payload — so matching on -// req.url would pass raw tokens straight into the access log. // -// Matched with the trailing slash ("/consent/", not "/consent") so this -// stays scoped to this router's actual mount point — a bare "/consent" -// prefix would also swallow any future unrelated route mounted alongside it -// (e.g. "/consent-status", "/consent-preferences"), redacting bodies that -// have nothing to do with this router. -// -// Compared lower-cased on both sides: the app-level mount -// (graphql-server.ts, `app.use("/consent", consentLogRouter)`) runs on a -// bare express() with no "case sensitive routing" setting, so Express -// matches that mount case-insensitively regardless of this router's own -// `caseSensitive: true` (which only governs matching *within* the router, -// e.g. "/log" vs "/Log"). A request to "/CONSENT/log" still dispatches into -// this router and still persists — its originalUrl is just differently -// cased. A plain case-sensitive startsWith here would miss that request -// and leak its raw invite token into the access log. -export const redactConsentBodyForLog = (req: { - originalUrl?: string - url?: string - body?: unknown -}) => - (req.originalUrl ?? req.url)?.toLowerCase().startsWith("/consent/") - ? "[consent body redacted]" - : req.body +// Gated on res.locals.consentLogIp, not the request URL. Two rounds of +// URL-string matching here each shipped a distinct bypass that leaked a raw +// token into the access log: req.url loses the "/consent" mount prefix that +// Express strips before dispatching into the router (originalUrl fixed +// that), and matching "/consent/" with a trailing slash to avoid swallowing +// a sibling route like "/consent-status" let a bare "POST /consent" (no +// trailing slash, no "/log") straight through unredacted — Express still +// dispatches that request into this router's middleware (so the rate +// limiter runs and express.json() parses a real body) before it falls +// through to a 404 with no matching route, and pino-http logs the +// now-populated req.body at response finish regardless of the 404. +// consentLogIp is the one signal immune to path-shape bypasses: it's set +// unconditionally by enforceConsentLogRateLimit, router-level middleware +// that runs for every request Express dispatches into this router no +// matter which path it targets or whether any route inside ultimately +// matches it. +export const redactConsentBodyForLog = ( + res: { locals?: { consentLogIp?: unknown } }, + body: unknown, +) => (res.locals?.consentLogIp !== undefined ? "[consent body redacted]" : body) export default consentLogRouter diff --git a/src/servers/graphql-server.ts b/src/servers/graphql-server.ts index 0d0c61b81..9fdec0de8 100644 --- a/src/servers/graphql-server.ts +++ b/src/servers/graphql-server.ts @@ -157,7 +157,7 @@ export const startApolloServer = async ({ PinoHttp({ logger: graphqlLogger, wrapSerializers: true, - customProps: (req) => { + customProps: (req, res) => { /* eslint @typescript-eslint/ban-ts-comment: "off" */ // @ts-ignore-next-line no-implicit-any error const account = req["gqlContext"]?.domainAccount @@ -168,7 +168,7 @@ export const startApolloServer = async ({ // redaction. Guarded by tests in // test/flash/unit/servers/consent-log.spec.ts. // @ts-ignore-next-line no-implicit-any error - "body": redactConsentBodyForLog(req), + "body": redactConsentBodyForLog(res, req.body), // @ts-ignore-next-line no-implicit-any error "token.sub": req["token"]?.sub, // @ts-ignore-next-line no-implicit-any error diff --git a/test/flash/unit/servers/consent-log.spec.ts b/test/flash/unit/servers/consent-log.spec.ts index 9d0adeb65..56fd3e2a5 100644 --- a/test/flash/unit/servers/consent-log.spec.ts +++ b/test/flash/unit/servers/consent-log.spec.ts @@ -434,90 +434,64 @@ describe("access-log body redaction (redactConsentBodyForLog)", () => { // pino-http re-evaluates customProps at response finish — by then // express.json() inside the consent router has populated req.body, so an // unredacted body log would write the raw invite token to disk on every - // status path (204/400/429/503). - it("redacts at RESPONSE-FINISH shape: url mutated to /log by the router mount, originalUrl intact", () => { - // Inside a router mounted at "/consent", Express rewrites req.url to - // "/log" and never restores it (the handler ends the response, so the - // restoring next() never runs). This is the shape pino-http actually - // evaluates when the body is populated — url alone is untrustworthy here. + // status path (204/400/429/503/404). + // + // Gated on res.locals.consentLogIp, not the request URL. Two rounds of + // URL-string matching here each shipped a distinct token-leak bypass: + // req.url loses the "/consent" mount prefix Express strips before + // dispatching into the router, and even originalUrl matching broke again + // once the match was tightened to require a trailing slash (to stop + // over-matching a sibling route like "/consent-status") — a bare + // "POST /consent" has no trailing slash, dispatches into this router's + // rate-limit + json-parsing middleware just the same, and then 404s with + // req.body already populated. consentLogIp is set unconditionally by + // enforceConsentLogRateLimit for every request Express dispatches into + // this router, regardless of path shape or whether any route matches — + // the one signal immune to a URL-shape bypass. + + it("redacts once the rate limiter has run, regardless of path shape", () => { const token = "a".repeat(40) - const logged = redactConsentBodyForLog({ - url: "/log", - originalUrl: "/consent/log", - body: validBody(), - }) + const logged = redactConsentBodyForLog( + { locals: { consentLogIp: "1.2.3.4" } }, + validBody(), + ) expect(logged).toBe("[consent body redacted]") expect(JSON.stringify(logged)).not.toContain(token) }) - it("redacts at middleware-time shape too (originalUrl === url)", () => { + it("redacts the bare mount path with no trailing slash and no matching route (e.g. POST /consent)", () => { + // The regression this pins: Express dispatches a bare "/consent" (no + // trailing slash, no "/log") into this router's middleware — the rate + // limiter and express.json() both still run — before falling through to + // a 404 with no matching route. The rate limiter sets consentLogIp + // regardless, so this must still redact even though no route handled it. expect( - redactConsentBodyForLog({ - url: "/consent/log", - originalUrl: "/consent/log", - body: { token: "x" }, - }), + redactConsentBodyForLog({ locals: { consentLogIp: "1.2.3.4" } }, { token: "x" }), ).toBe("[consent body redacted]") }) - it("would leak if the helper keyed on the mutated url — pin the failure mode", () => { - // The regression this guards: a helper reading only req.url sees "/log" - // at finish time and returns the raw body. originalUrl must win. - expect( - redactConsentBodyForLog({ - url: "/log", - originalUrl: "/consent/log", - body: { token: "x" }, - }), - ).not.toEqual({ token: "x" }) - }) - it("leaves non-consent request bodies untouched for the access log", () => { + // A /graphql request never dispatches into the consent router, so + // enforceConsentLogRateLimit never runs and consentLogIp is never set. const body = { query: "{ me { id } }" } - expect( - redactConsentBodyForLog({ url: "/graphql", originalUrl: "/graphql", body }), - ).toBe(body) - expect(redactConsentBodyForLog({ body })).toBe(body) + expect(redactConsentBodyForLog({ locals: {} }, body)).toBe(body) + expect(redactConsentBodyForLog({}, body)).toBe(body) }) - it("does not over-match a sibling route that merely starts with /consent (e.g. /consent-status)", () => { - // A loose "/consent" prefix (no trailing slash) would also swallow any - // future unrelated route mounted alongside this router — this pins the - // helper to this router's actual mount point instead. + it("does not redact a sibling route that merely starts with /consent (e.g. /consent-status)", () => { + // A route mounted alongside this router (not under it) never runs + // enforceConsentLogRateLimit either, so its body must pass through. const body = { some: "unrelated payload" } - expect( - redactConsentBodyForLog({ - url: "/consent-status", - originalUrl: "/consent-status", - body, - }), - ).toBe(body) - }) - - it("redacts a differently-cased mount segment that still dispatches into this router (e.g. /CONSENT/log)", () => { - // The app-level mount (`app.use("/consent", consentLogRouter)` in - // graphql-server.ts) runs on a bare express() with no - // "case sensitive routing" setting, so Express matches that mount - // case-insensitively regardless of this router's own - // `caseSensitive: true` (which only governs matching *within* the - // router). A request to "/CONSENT/log" still reaches the "/log" route - // and still persists a real submission — only originalUrl's casing - // differs from the lower-case check. A case-sensitive startsWith here - // would miss it and leak the raw invite token to the access log. - const token = "a".repeat(40) - const logged = redactConsentBodyForLog({ - originalUrl: "/CONSENT/log", - body: validBody(), - }) - - expect(logged).toBe("[consent body redacted]") - expect(JSON.stringify(logged)).not.toContain(token) + expect(redactConsentBodyForLog({ locals: {} }, body)).toBe(body) }) - it("redacts regardless of casing on either the mount segment or the sub-route segment", () => { + it("treats consentLogIp explicitly set to a falsy-but-defined value as still gated", () => { + // Guards against a future "if (res.locals.consentLogIp)" rewrite that + // would stop redacting for a (currently impossible, but not + // type-prevented) empty-string IP. expect( - redactConsentBodyForLog({ originalUrl: "/Consent/Log", body: validBody() }), + redactConsentBodyForLog({ locals: { consentLogIp: "" } }, { token: "x" }), ).toBe("[consent body redacted]") }) })