feat: POST /consent/log — persist invite-page consent records (ENG-568) - #498
Merged
Conversation
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).
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…rigin 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…uter-mutated url
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.
…t canonical clientTimestamp - 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…ction path match 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8
…ate limiter's own IP 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves ENG-568 (decision: build the endpoint).
Why
The getflash.io/invite landing page has displayed a transactional/marketing consent flow since launch and POSTed the record to
https://api.flashapp.me/consent/log— an endpoint that never existed. The call is fail-open on the page, so nothing broke visibly, but every consent record was silently dropped while the page displayed a compliance promise. Surfaced during the ENG-567 invite-page incident; flash-site#28 deliberately left the page's call in place pending this decision.What
POST /consent/log(src/servers/consent-log.ts) — mounted on the public server only (type === "main"), before the JWT middleware. 204 on success, no internals leaked on failure.src/domain/consent-log) — pure validator for anonymous web input: required version + both consent legs with booleanoptedIn, every string bounded, unknown fields discarded. Accepts the page's current field names (page,timestamp) as sent.src/services/mongoose/models/consent-log.ts) — append-only evidence records; invite token stored only as a hash (same rule as the invites collection), client timestamp kept verbatim as an untrusted assertion withreceivedAtas the trustworthy ordering field.consent_log_ipprefix wired through the standardRateLimitConfigmachinery). A rate-limiter store fault returns 503 rather than failing open on a public write.Not in scope
The page currently sends the raw invite token inside the consent record; server-side hashing makes that safe at rest. A follow-up flash-site change could stop sending the token and let the server correlate by hash — noted on ENG-568.
Verification
test/flash/unit/domain/consent-log.spec.ts(14 tests: happy path, minimal record, unknown-field discard, 10 rejection cases)@configmocks gained the new getter)api.flashapp.meroutes the whole host to this service (verified live via/healthz)🤖 Generated with Claude Code
https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV