diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4fc40f8c..01937efe 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -471,6 +471,7 @@ surfaces as `deleted_user`. 3. No schema support yet for a doctor working across multiple facilities/locations (PRD edge case 6) — `AvailabilitySlot` currently assumes one calendar per doctor. 4. ~~No audit-log table~~ **Foundation resolved 2026-10-17 (issue #43):** `AuditEvent` (§4.10) + `audit.record()` in the same transaction as the change, append-only at the DB level, wired into booking (both booking paths), doctor verification, payment fund/release/refund, the patient's referral accept/decline and the referral admin actions (`referral.reassign`, `referral.cancel` — Phase 3, issue #68, written as `entity_type = 'Referral'` rows in the same table). **Still open:** the referral's creation and the cascade-driven `completed`/`expired` transitions (no actor to attribute them to yet), the remaining Phase 4 transitions (prescriptions, lab orders, fulfillment), the admin login-throttle clear (`/auth/login-attempts/clear`) still only logs to the console; the app connects as the database owner, which could disable the triggers — enforcing append-only against the owner needs a separate least-privilege DB role (deployment work, issue #22). PRD §7 auditability is therefore "a real mechanism exists" rather than "auditability is done"; remaining coverage stays an open gap. 5. CSRF on the cookie-authenticated routes — the refresh cookie has to be `SameSite=None; Secure` (the Pages frontend is a different site — see `docs/adr/0003`), which means any site the patient visits can make their browser send it to `/auth/refresh` and `/auth/logout`. **Resolved 2026-10-15 (issue #37):** a per-session CSRF token is bound into the refresh token and required in the `X-CSRF-Token` header, the `Origin` header is validated against an explicit allowlist, and CORS is now a per-environment allowlist (`CORS_ORIGINS`) instead of one configured string. The double-submit cookie pattern — the usual answer — is *not* usable here: a cookie set by this API is a third-party cookie to `docforum.github.io`, so the frontend's JavaScript can never read it; the token travels in the login/refresh response body instead. A request with no `Origin` (curl, server-to-server) is allowed through the Origin check but still needs the token. See `backend/src/middleware/csrf.ts`, and `docs/api/README.md` for what the client must send. Consequence: refresh cookies issued before this change carry no `csrf` claim and are rejected, i.e. existing sessions need one re-login. +6. **Added 2026-10-19 (issue #48):** an `IdempotencyKey` row caches a *copy of a response body* for 24 hours (§4.12). Today every opted-in route returns non-PHI data (an appointment id and status, a payment intent), so this is transport bookkeeping and not a patient-data surface — it is not part of the patient-export bundle (§3.1) and no endpoint lists it. **The condition that would change that:** if a route returning clinical free text (consultation notes, intake payloads, lab results) ever opts in, each row becomes a temporary PHI copy subject to right-of-access and retention, and the export registry in §3.1 has to gain a section for it before that route is allowed to opt in. Until then, adding a route to the opted-in list is a deliberate decision, not a default. ### 6.3 What's overengineered (architecture-level, for v1)? 1. Polymorphic fulfillment design (§4.7) may itself be premature abstraction — two small, boring, separate tables might beat one clever polymorphic one at this stage. Flagged as a design smell to revisit, not shipped as-is. @@ -482,6 +483,7 @@ surfaces as `deleted_user`. - **Testing**: unit tests per module (`backend/tests/unit`), integration tests for booking/referral/order flows specifically because those are the correctness-critical paths (`backend/tests/integration`). - **Security — login throttling (Phase 8, issue #35)**: `POST /auth/login` is throttled per normalised email *and* per client IP, in Postgres (`LoginAttempt`, §4.9): exponential backoff with `Retry-After` from `LOGIN_THROTTLE_BACKOFF_AFTER` failures, a temporary lock at `LOGIN_THROTTLE_LOCK_AFTER`, and a cleanup interval in `src/server.ts`. Two things this depends on operationally: (1) `TRUST_PROXY` must be set correctly in production — Render terminates TLS in front of exactly one hop, so `TRUST_PROXY=1`; leaving it unset makes every request look like Render's proxy (one shared bucket for everyone), and `true` lets a client spoof `X-Forwarded-For` for a fresh bucket per request; (2) the throttle deliberately does not reveal whether an account exists — locked, backoff and unknown-account responses are identical, and an absent account still pays the bcrypt comparison cost so response timing doesn't leak either. +- **Idempotent retries (Phase 8, issue #48)**: `POST /appointments`, `POST /payments/intents` and `POST /payments/intents/:id/{fund,release,refund}` honour an `Idempotency-Key` header (`src/middleware/idempotency.ts`). A repeat of the same key with the same request body inside 24 h replays the stored status and body instead of running the mutation again; the same key with a different body is `422`; a second request arriving while the first is still running is `409 IN_PROGRESS` with a `Retry-After`, because ownership is decided by the unique constraint on `(userId, key)` rather than by an in-memory lock that would be correct on one instance and broken on two. Keys are namespaced per user, and 5xx is never cached so a client can retry a failed call with the same key. See §4.12 for the model. - **Migrations**: Prisma migrations are the only way schema changes happen — no manual DB edits, ever. - **Environments**: local (Docker Compose), staging, production — staging/production infra choice is an open item (see ROADMAP). diff --git a/ROADMAP.md b/ROADMAP.md index 3da29b06..3be7975b 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -118,7 +118,7 @@ roadmap. See each phase section below for status and issue links. - [ ] ~~Shared: auth screens (signup/login per role).~~ → already built, `docforum-web` Phase W1. ## Phase 8 — Hardening (safety, audit, security) -**Status: In progress. In addition to the original five items (#18–#20 scoped, two still blocked), four auth-hardening items are scoped as #34, #35, #37 and #39 — #34 and #35 are merged; #37 and #39 are open PRs. An access-control test suite (#44) is built and tested. Each is independent of the others and of Phases 3–7, and each carries its own schema/doc updates.** +**Status: In progress. In addition to the original five items (#18–#20 scoped, two still blocked), four auth-hardening items are scoped as #34, #35, #37 and #39 — #34 and #35 are merged; #37 and #39 are open PRs. An access-control test suite (#44) is built and tested, and `Idempotency-Key` replay protection (#48) is in review. Each is independent of the others and of Phases 3–7, and each carries its own schema/doc updates.** - [x] Email verification on signup, with restricted access for unverified accounts. `User.emailVerifiedAt` + `EmailVerificationToken` (hashed, expiring, single-use), `POST /auth/verify-email`, `POST /auth/verify-email/resend`, and a `requireVerifiedEmail` guard on booking + payment mutations (never on login/browsing). Migration backfills existing rows as verified. Tracked as [issue #34](https://github.com/DocForum/docforum-core/issues/34) (High, 200 pts) — **Done**, merged as PR #137. Found a real dependency while building it: the email *provider* is still an open decision (Phase 6, issue #17), so links can't be delivered on a hosted deployment yet — see `backend/src/modules/auth/services/email-dispatcher.ts` and ARCHITECTURE.md §6.1.5. - [x] Login brute-force protection: per-account and per-IP throttling with backoff. Postgres-backed buckets (`LoginAttempt`), a single conditional upsert so concurrent failures are counted atomically, exponential backoff with `Retry-After`, a bounded temporary lock, an admin clear endpoint, a prune interval in `src/server.ts`, and a `TRUST_PROXY` setting so the IP isn't Render's proxy address. Tracked as [issue #35](https://github.com/DocForum/docforum-core/issues/35) (High, 200 pts) — **Done**, merged as PR #138. diff --git a/backend/.env.example b/backend/.env.example index 8de44ab2..c92fa5c1 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -44,6 +44,15 @@ PATIENT_EXPORT_CLEANUP_INTERVAL_SECONDS=300 PATIENT_EXPORT_STALE_AFTER_SECONDS=900 PATIENT_EXPORT_MAX_ATTEMPTS=3 +# Idempotency-Key replay protection (Phase 8, issue #48). Defaults are in +# src/config/index.ts. The header is optional per request — these only bound +# how long a stored response stays replayable and how long an attempt left +# unfinished by a crashed process is presumed dead. +IDEMPOTENCY_RECORD_TTL_HOURS=24 +IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS=300 +IDEMPOTENCY_MAX_CACHED_BODY_BYTES=262144 +IDEMPOTENCY_PRUNE_INTERVAL_SECONDS=3600 + # Payments (Phase 5.5 — custodial v1, see docs/adr/0004-custodial-payments-v1.md) # Testnet only (docforum-escrow hard rule 4) — real secret keys, never commit # a filled-in .env. Generate + fund your own with: diff --git a/backend/prisma/migrations/20261019113906_add_idempotency_key/migration.sql b/backend/prisma/migrations/20261019113906_add_idempotency_key/migration.sql new file mode 100644 index 00000000..70095ff4 --- /dev/null +++ b/backend/prisma/migrations/20261019113906_add_idempotency_key/migration.sql @@ -0,0 +1,41 @@ +-- Idempotency-Key attempt storage (Phase 8, issue #48). +-- +-- Generated by `node scripts/gen-migration.mjs add_idempotency_key` from a real +-- diff, not hand-written. +-- +-- (userId, key) is both the scope and the concurrency mechanism. Whichever +-- request wins the INSERT owns the attempt; everyone else reads the row and +-- either replays the stored response or is told one is in flight. There is no +-- in-memory lock anywhere, so this behaves identically across instances — the +-- same division of labour as §5.1's conditional slot claim. +-- +-- The unique index is what makes `INSERT ... ON CONFLICT DO NOTHING` safe here, +-- and it is created in the same migration as the table so there is never a +-- window in which rows can be written without it. No foreign key on userId: +-- these rows are short-lived transport bookkeeping (24 h by default), and a +-- foreign key would only make account deletion a slower, more coupled operation +-- for a cache. +-- +-- CreateTable +CREATE TABLE "IdempotencyKey" ( + "id" TEXT NOT NULL, + "userId" TEXT NOT NULL, + "key" TEXT NOT NULL, + "method" TEXT NOT NULL, + "route" TEXT NOT NULL, + "requestHash" TEXT NOT NULL, + "responseStatus" INTEGER, + "responseBody" JSONB, + "responseHeaders" JSONB, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "expiresAt" TIMESTAMP(3) NOT NULL, + + CONSTRAINT "IdempotencyKey_pkey" PRIMARY KEY ("id") +); + +-- CreateIndex +CREATE INDEX "IdempotencyKey_expiresAt_idx" ON "IdempotencyKey"("expiresAt"); + +-- CreateIndex +CREATE UNIQUE INDEX "IdempotencyKey_userId_key_key" ON "IdempotencyKey"("userId", "key"); + diff --git a/backend/prisma/schema.prisma b/backend/prisma/schema.prisma index f19852ee..dd2dfbc4 100644 --- a/backend/prisma/schema.prisma +++ b/backend/prisma/schema.prisma @@ -879,3 +879,48 @@ model AuditEvent { @@index([actorUserId, createdAt]) @@index([createdAt, id]) } + +// --- Idempotency (Phase 8, issue #48) --------------------------------------- +// One row per in-flight or completed `Idempotency-Key` request, so a client +// retrying after a mobile timeout gets the first response back instead of +// creating a second booking or a second payment. +// +// Scoped to (userId, key) — the unique constraint below is the whole +// concurrency mechanism. There is no in-memory lock anywhere: whoever wins +// the INSERT owns the attempt, everyone else reads the row. Two instances of +// this API therefore behave identically to one. +// +// `responseStatus`/`responseBody`/`responseHeaders` are NULL while the +// attempt is still running. That gap is what a concurrent duplicate sees, and +// what it waits on. It is NOT audit history: a 5xx deletes the row outright +// so the client can retry with the same key, and an attempt abandoned by a +// crashed process is taken over once it is older than the in-progress +// timeout. Rows are removed once `expiresAt` passes (24h by default). +// +// Deliberately NOT an `AuditEvent` and not a patient-data record: the cached +// body is a copy of a response the requesting user already received, kept +// only as long as a retry could still arrive, and it is never exported or +// listed. See ARCHITECTURE.md §4.12. +model IdempotencyKey { + id String @id @default(uuid()) + // Scope: a key is only ever compared against the same user's other keys, + // so one user can never replay (or read) another user's stored response. + userId String + key String + method String + // Route pattern, not the URL: '/payments/intents/:id/fund'. Stored so a + // row is self-describing when debugging, and folded into requestHash so a + // key reused on a different route is refused rather than replayed. + route String + // SHA-256 of the canonical request. Same key + same body -> replay; + // same key + different body -> 422. + requestHash String + responseStatus Int? + responseBody Json? + responseHeaders Json? + createdAt DateTime @default(now()) + expiresAt DateTime + + @@unique([userId, key]) + @@index([expiresAt]) +} diff --git a/backend/src/middleware/idempotency.ts b/backend/src/middleware/idempotency.ts new file mode 100644 index 00000000..8ca46152 --- /dev/null +++ b/backend/src/middleware/idempotency.ts @@ -0,0 +1,398 @@ +// `Idempotency-Key` replay protection for money- and appointment-moving +// mutations (Phase 8, issue #48). +// +// The problem: a patient on mobile loses signal mid-request, sees a timeout, +// and retries. The backend cannot tell that retry from a deliberate second +// attempt, so the same tap books twice or funds twice. Worse, the fix cannot +// be "check then write" — two retries arriving 20 ms apart both pass the +// check. Booking already has to solve exactly this shape at the database +// level (ARCHITECTURE.md §5.1), and this is the same problem one layer up. +// +// The mechanism is deliberately the boring one: **the unique constraint on +// (userId, key) is the lock.** There is no in-memory mutex, no `Map`, no +// "pending requests" set in this process — every one of those is correct on +// one instance and silently broken on the second, and would also be lost on a +// restart, which is precisely when a retry storm arrives. Instead: +// +// INSERT ... ON CONFLICT DO NOTHING (the unique index is (userId, key)) +// count 1 -> this request owns the attempt; run the handler and store +// the response +// count 0 -> someone else owns it; read the row and decide +// completed -> replay the stored status/body/headers +// unfinished -> 409 IN_PROGRESS +// different -> 422 (the key was reused for another request) +// stale -> conditional takeover (below) +// +// Every one of those decisions is a single indexed read or a single +// conditional UPDATE, so two instances of this API behave identically to one. +// +// What is deliberately NOT here: +// - No in-memory state, for the reason above. +// - No 5xx caching. A server error is *not* a result — the request may or +// may not have had side effects — so the row is deleted and the client is +// expected to retry with the same key, which re-executes it. Caching a +// 500 would turn a transient failure into a permanent one for that key. +// - No key reuse for a *different* request, even after a crash. A stale +// unfinished row is only ever taken over by a request with the same hash +// (the genuine retry); a different body on the same key is still 422, +// because silently re-running a different operation under an old key is +// how "I reused my key" turns into "I refunded the wrong payment". +// +// Scoping: keys are namespaced per user (`@@unique([userId, key])`) and every +// read filters on `userId`, so one account can neither replay nor read +// another account's stored response body — an `Idempotency-Key` is a guessable +// string, and treating it as a bearer credential would be a data leak. +// +// Storage note: the row holds a copy of a response the requesting user already +// received, kept only while a retry could still arrive (24 h by default, then +// `pruneIdempotencyKeys()` in src/server.ts). It is not patient data, is not an +// `AuditEvent`, and is deliberately not part of the patient-export bundle +// (ARCHITECTURE.md §4.12). +import type { NextFunction, Request, Response } from 'express'; +import { createHash } from 'node:crypto'; +import { config } from '../config'; +import { HttpError } from '../http-error'; +import { prisma } from '../db/prisma-client'; + +export const IDEMPOTENCY_HEADER = 'idempotency-key'; + +// Machine-readable discriminators, matching the `code` field convention +// introduced by issue #34 so docforum-web can branch without parsing prose. +export const CODE_INVALID_KEY = 'INVALID_IDEMPOTENCY_KEY'; +export const CODE_KEY_REUSED = 'IDEMPOTENCY_KEY_REUSED'; +export const CODE_IN_PROGRESS = 'IN_PROGRESS'; + +// What a client should wait before repeating a request that collided with one +// already in flight. Short on purpose — the winner is usually a booking or an +// escrow call that finishes in seconds, and a retry is free. +export const RETRY_AFTER_SECONDS = 1; + +// Bounded so a header can't be used as free storage in the table (a `key` +// column is indexed by (userId, key), and it is echoed back in logs). +const MAX_KEY_LENGTH = 255; +const ACCEPTABLE_KEY = /^[\x21-\x7e]{1,255}$/; + +// A key must be non-empty, printable ASCII and bounded. This is not a secret +// (it is namespaced per user and never authenticates anything), it just has to +// be a stable, comparable token. +export function isValidIdempotencyKey(key: string): boolean { + return key.length <= MAX_KEY_LENGTH && ACCEPTABLE_KEY.test(key); +} + +/** + * Deterministic JSON: object keys sorted, arrays left in order. The hash has to + * match across processes and across JSON key ordering, otherwise the same + * logical body sent twice could hash two ways and 422 against itself. + */ +export function canonicalJson(value: unknown): string { + if (value === null || typeof value !== 'object') return JSON.stringify(value ?? null) ?? 'null'; + if (Array.isArray(value)) return `[${value.map((item) => canonicalJson(item)).join(',')}]`; + const entries = Object.entries(value as Record) + .filter(([, v]) => v !== undefined) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)); + return `{${entries.map(([k, v]) => `${JSON.stringify(k)}:${canonicalJson(v)}`).join(',')}}`; +} + +/** + * What identifies "the same request" for replay purposes: the method, the + * concrete path (so the same key aimed at a *different* `:id` is a mismatch, + * not a replay of another record's response) and the body. + */ +export function hashRequest(method: string, path: string, body: unknown): string { + return createHash('sha256').update(`${method.toUpperCase()}\n${path}\n${canonicalJson(body)}`).digest('hex'); +} + +type Claim = + /** Nobody had this key: this request owns the attempt. */ + | { kind: 'owned' } + /** A completed attempt with the same key and the same body: send this back. */ + | { kind: 'replay'; status: number; body: unknown; location: string | null } + /** Someone else's attempt is still running. */ + | { kind: 'in_progress' } + /** The key exists but was used for a different request. */ + | { kind: 'mismatch' } + /** Could not determine ownership within the retry budget — fail closed. */ + | { kind: 'unknown' }; + +// Bounded: each iteration is one indexed read or one conditional write, and +// every extra iteration needs two concurrent claims to have raced a deletion. +// Exhausting it means refusing, never double-running. +const CLAIM_ATTEMPTS = 3; + +/** + * Decides whether this request owns (userId, key), replays it, or is refused. + * + * The write that decides ownership is `createMany({ skipDuplicates: true })`, + * i.e. `INSERT ... ON CONFLICT DO NOTHING` against the unique index: the + * database returns "I inserted it" or "it was already there", atomically, + * under concurrency. Application logic then only *reads* what the constraint + * decided — the same division of labour as §5.1's `claimOpenSlot`. + */ +async function claimAttempt(params: { + userId: string; + key: string; + method: string; + route: string; + requestHash: string; + ttlMs: number; + inProgressTimeoutMs: number; +}): Promise { + const expiresAt = new Date(Date.now() + params.ttlMs); + + for (let attempt = 0; attempt < CLAIM_ATTEMPTS; attempt += 1) { + const inserted = await prisma.idempotencyKey.createMany({ + data: [ + { + userId: params.userId, + key: params.key, + method: params.method, + route: params.route, + requestHash: params.requestHash, + expiresAt, + }, + ], + // The unique index is (userId, key) and the lookup below is by the same + // pair, so "do nothing on conflict" and "select by" agree on which row + // is the conflict. + skipDuplicates: true, + }); + + if (inserted.count === 1) return { kind: 'owned' }; + + const existing = await prisma.idempotencyKey.findUnique({ + where: { userId_key: { userId: params.userId, key: params.key } }, + }); + // Lost a race with a delete (expired row pruned between the two + // statements). Loop and try the insert again rather than guessing. + if (!existing) continue; + + // Past its TTL the key is free again: a retry this old is a new request, + // not a replay of a forgotten one. + if (existing.expiresAt.getTime() <= Date.now()) { + await prisma.idempotencyKey.deleteMany({ where: { id: existing.id } }); + continue; + } + + // Same key, different request. Checked BEFORE the staleness check below so + // a takeover can never silently re-point an old key at a new operation. + if (existing.requestHash !== params.requestHash) return { kind: 'mismatch' }; + + if (existing.responseStatus !== null) { + return { + kind: 'replay', + status: existing.responseStatus, + body: existing.responseBody ?? null, + location: readStoredLocation(existing.responseHeaders), + }; + } + + // Unfinished, but old enough to be presumed dead: the process that owned it + // died between claiming the key and storing a response. A *genuine* retry + // (identical hash — already established above) may take it over. The UPDATE + // repeats `responseStatus IS NULL` and the exact `createdAt` it read, so + // two contenders still can't both win. + if (Date.now() - existing.createdAt.getTime() >= params.inProgressTimeoutMs) { + const takenOver = await prisma.idempotencyKey.updateMany({ + where: { id: existing.id, responseStatus: null, createdAt: existing.createdAt }, + data: { + method: params.method, + route: params.route, + // Same hash by definition (checked above); restated so the row + // describes the attempt that is actually running. + requestHash: params.requestHash, + createdAt: new Date(), + expiresAt, + }, + }); + if (takenOver.count === 1) return { kind: 'owned' }; + continue; + } + + return { kind: 'in_progress' }; + } + + return { kind: 'unknown' }; +} + +function readStoredLocation(headers: unknown): string | null { + if (!headers || typeof headers !== 'object') return null; + const value = (headers as Record).location; + return typeof value === 'string' ? value : null; +} + +/** + * Records the outcome of the attempt this request owns. + * + * A `5xx` deletes the row rather than caching it: see the header comment. Any + * other status is stored, because for a 4xx the request *did* run and its + * answer is the answer — replaying a deterministic "slot already booked" is + * exactly what the client wants. + */ +async function settleAttempt(params: { + userId: string; + key: string; + status: number; + body: unknown; + location: string | null; +}): Promise { + if (params.status >= 500) { + await prisma.idempotencyKey.deleteMany({ where: { userId: params.userId, key: params.key } }); + return; + } + + // Anything too large to be worth replaying is executed and then forgotten, + // so a retry re-runs the request rather than getting a truncated body back. + // (No current route comes close; this is a bound, not a policy.) + const serialised = JSON.stringify(params.body ?? null); + if (serialised.length > config.idempotency.maxCachedBodyBytes) { + await prisma.idempotencyKey.deleteMany({ where: { userId: params.userId, key: params.key } }); + return; + } + + await prisma.idempotencyKey.updateMany({ + where: { + userId: params.userId, + key: params.key, + // Still ours and still unfinished: if the row was deleted (crash, prune) + // or taken over while the handler ran, this write must not resurrect it. + responseStatus: null, + }, + data: { + responseStatus: params.status, + responseBody: JSON.parse(serialised), + // Only `Location`, and never `Set-Cookie`: a replay must not re-issue a + // credential, and no opted-in route sets one, but a future one must not + // start doing so by accident. + responseHeaders: params.location ? { location: params.location } : undefined, + }, + }); +} + +/** + * Express middleware factory. Opt-in per route: + * + * router.post('/', authenticate, requireRole('patient'), idempotency(), book) + * + * The header is optional — a request without one passes through untouched, so + * this can be mounted on a live route without breaking existing clients — but + * every client that can retry should send one. + */ +export function idempotency() { + return async function idempotencyMiddleware(req: Request, res: Response, next: NextFunction) { + const raw = req.get(IDEMPOTENCY_HEADER); + if (raw === undefined) return next(); + + const key = raw.trim(); + if (!isValidIdempotencyKey(key)) { + throw new HttpError(400, `\`${IDEMPOTENCY_HEADER}\` must be 1-${MAX_KEY_LENGTH} printable ASCII characters.`, CODE_INVALID_KEY); + } + if (!req.user) throw new HttpError(401, 'Authentication required.'); + + // `req.baseUrl` is the router's mount path and `req.route.path` the + // pattern under it, so the stored route reads '/payments/intents/:id/fund' + // rather than a concrete URL — self-describing when debugging, and stable + // across records. Collapsed and de-slashed ('/appointments' + '/' is the + // same route as '/appointments'). The *hash* uses the concrete path (see + // hashRequest). + const pattern = typeof req.route?.path === 'string' ? req.route.path : req.path; + const route = `${req.baseUrl}${pattern}`.replace(/\/{2,}/g, '/').replace(/(.)\/$/, '$1'); + const path = `${req.baseUrl}${req.path}`; + const requestHash = hashRequest(req.method, path, req.body); + + const claim = await claimAttempt({ + userId: req.user.id, + key, + method: req.method, + route, + requestHash, + ttlMs: config.idempotency.recordTtlHours * 60 * 60 * 1000, + inProgressTimeoutMs: config.idempotency.inProgressTimeoutSeconds * 1000, + }); + + if (claim.kind === 'mismatch') { + throw new HttpError( + 422, + 'This Idempotency-Key was already used for a different request. Use a new key.', + CODE_KEY_REUSED, + ); + } + if (claim.kind === 'in_progress' || claim.kind === 'unknown') { + res.set('Retry-After', String(RETRY_AFTER_SECONDS)); + throw new HttpError( + 409, + 'A request with this Idempotency-Key is still in progress. Retry shortly with the same key.', + CODE_IN_PROGRESS, + ); + } + if (claim.kind === 'replay') { + res.set('Idempotency-Replayed', 'true'); + if (claim.location) res.set('Location', claim.location); + res.status(claim.status).json(claim.body); + return; + } + + captureResponse(res, (body) => + settleAttempt({ + userId: req.user!.id, + key, + // Read at settle time: the controller has finished choosing a status. + status: res.statusCode, + body, + // `Location` is read here rather than when the middleware started — + // the controller sets it on the way out. + location: res.get('Location') ?? null, + }), + ); + next(); + }; +} + +/** + * Hooks `res.json` so the outcome of the handler we just released is stored + * *before* the bytes go out. + * + * The write deliberately precedes the response. The alternative (fire-and- + * forget after sending) loses the only durable record of what the client was + * told whenever the process dies in between — which is exactly the retry + * storm this exists for. Every opted-in controller answers with `res.json`, + * including the shared error handler, so one hook covers both the success and + * the failure path. + */ +function captureResponse(res: Response, settle: (body: unknown) => Promise): void { + let settled = false; + const originalJson = res.json.bind(res); + + res.json = (body?: unknown) => { + settled = true; + void settle(body) + .catch(() => { + // Losing the cached response must not turn a successful mutation into + // a failed request. The row stays unfinished, so a retry gets 409 + // until the crash-recovery timeout takes it over — an honest answer + // for an unhappened outcome. + }) + .then(() => originalJson(body)); + return res; + }; + + // Safety net: a handler that ends the response without `res.json` + // (res.end, a bare res.send, a stream) leaves nothing to replay, so the + // attempt is dropped instead of being stuck "in progress" for the whole + // crash-recovery window. + res.on('finish', () => { + if (!settled) void settle(null); + }); +} + +/** + * Deletes rows whose TTL has passed. Called on an interval from src/server.ts, + * same shape as the login-throttle prune (ARCHITECTURE_ESSENTIALS.md: plain + * intervals, no queue, until a real job exists). + * + * Correctness does not depend on it: every lookup filters on `expiresAt` + * first, so a row that outlives its TTL is invisible to replays either way. + */ +export async function pruneIdempotencyKeys(): Promise { + const result = await prisma.idempotencyKey.deleteMany({ where: { expiresAt: { lt: new Date() } } }); + return result.count; +} \ No newline at end of file diff --git a/backend/src/modules/appointments/routes/index.ts b/backend/src/modules/appointments/routes/index.ts index dac47c1f..0037f9c2 100644 --- a/backend/src/modules/appointments/routes/index.ts +++ b/backend/src/modules/appointments/routes/index.ts @@ -1,5 +1,6 @@ import { Router } from 'express'; import { authenticate, requireRole, requireVerifiedEmail } from '../../../middleware/auth'; +import { idempotency } from '../../../middleware/idempotency'; import * as appointmentsController from '../controllers'; export const appointmentsRouter = Router(); @@ -7,11 +8,19 @@ export const appointmentsRouter = Router(); // Booking is a mutation against someone else's calendar, and the patient // gets notifications about it — so it requires a verified address // (Phase 8, issue #34). Browsing (`GET /availability/...`) stays open. +// +// `idempotency()` (Phase 8, issue #48) comes after auth on purpose: the key is +// namespaced per user, so there has to be a user. A retry of this call — the +// one a patient on a flaky connection makes without thinking — replays the +// first response instead of racing §5.1's slot claim for a slot it already +// won. The booking guarantee itself is unchanged and still the reason this +// endpoint exists; this only stops the *duplicate request* from existing. appointmentsRouter.post( '/', authenticate, requireRole('patient'), requireVerifiedEmail, + idempotency(), appointmentsController.book, ); appointmentsRouter.get('/mine', authenticate, requireRole('patient'), appointmentsController.listMine); diff --git a/backend/src/modules/payments/routes/index.ts b/backend/src/modules/payments/routes/index.ts index 94aa9791..75ad05d1 100644 --- a/backend/src/modules/payments/routes/index.ts +++ b/backend/src/modules/payments/routes/index.ts @@ -1,5 +1,6 @@ import { Router } from 'express'; import { authenticate, requireRole, requireVerifiedEmail } from '../../../middleware/auth'; +import { idempotency } from '../../../middleware/idempotency'; import * as paymentsController from '../controllers'; export const paymentsRouter = Router(); @@ -23,6 +24,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.fundIntent, ); paymentsRouter.post( @@ -30,6 +32,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.releaseIntent, ); paymentsRouter.post( @@ -37,5 +40,6 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.refundIntent, ); diff --git a/backend/src/server.ts b/backend/src/server.ts index fa1c9577..4072eda1 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -60,6 +60,24 @@ const pruneTimer = setInterval(() => { }, config.loginThrottle.pruneIntervalSeconds * 1000); pruneTimer.unref(); +// Replay-window cleanup for `Idempotency-Key` rows (Phase 8, issue #48). +// Purely housekeeping: the middleware already treats anything past its TTL as +// absent, so a missed run costs storage, never correctness. +const idempotencyPruneTimer = setInterval(() => { + void pruneIdempotencyKeys() + .then((removed) => { + if (removed > 0) { + // eslint-disable-next-line no-console + console.log(`[jobs] pruned ${removed} expired idempotency key row(s)`); + } + }) + .catch((err) => { + // eslint-disable-next-line no-console + console.error('[jobs] idempotency prune failed:', err); + }); +}, config.idempotency.pruneIntervalSeconds * 1000); +idempotencyPruneTimer.unref(); + // Patient data export worker + expiry cleanup (Phase 8, issue #40). startPatientExportJobs(); startEscrowEventPoller(); diff --git a/backend/tests/integration/idempotency.test.mts b/backend/tests/integration/idempotency.test.mts new file mode 100644 index 00000000..ba417226 --- /dev/null +++ b/backend/tests/integration/idempotency.test.mts @@ -0,0 +1,666 @@ +// Integration tests for `Idempotency-Key` replay protection (Phase 8, issue +// #48), against a real Postgres — because the guarantee lives in the database. +// +// A retry of a booking or a payment must return the first response instead of +// running the mutation again, a retry that carries a *different* body must be +// refused, and two requests arriving at the same instant with the same key must +// not both run. The last one is the point: none of this is provable against a +// mock, because the thing under test is the unique index on (userId, key) +// deciding ownership between two concurrent writers. +// +// Covered here: +// - replay of a completed POST /appointments and POST /payments/intents +// - 422 when one key is reused for a different body / target id +// - 409 IN_PROGRESS while a request with the same key is still running, +// proven deterministically against a handler held open by a gate, plus a +// genuine simultaneous burst (8 concurrent requests, one key) +// - keys are scoped per user: a second account reusing the first account's +// key string runs its own request and never sees the first response +// - a retried fund/release/refund replays instead of making a second escrow +// call (the fake SDK counts calls — this is the payments half of #48, and +// what issue #24's reconcile job would otherwise have to clean up) +// - 5xx is NOT cached: the client can retry the same key and have it run +// - an attempt left unfinished by a dead process is taken over, but only by +// an identical request +// - unopted-in routes are untouched, and a malformed key is a 400 +// +// Self-contained embedded Postgres on its own port (5443), same pattern as the +// other files in this directory, so it runs alongside them. The escrow SDK is +// faked — on-chain behaviour is the testnet suite's job. +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import express from 'express'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import EmbeddedPostgres from 'embedded-postgres'; + +const BACKEND_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const PORT = 5443; +const runId = Date.now(); +const PAYER_SECRET = `IDEMPAYERSENTINEL${runId}`; +const RELEASER_SECRET = `IDEMRELEASERSENTINEL${runId}`; +const TX_HASH = 'd'.repeat(64); + +const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'docforum-idempotency-pg-')); +const pg = new EmbeddedPostgres({ + databaseDir: dataDir, + user: 'docforum', + password: 'docforum', + port: PORT, + persistent: false, +}); +const databaseUrl = `postgresql://docforum:docforum@localhost:${PORT}/docforum_test`; + +// Small enough that the "oversized response" route below exceeds it and the +// ordinary booking/intent responses (a few hundred bytes) do not. Read from +// config at import time, so it has to be set before the dynamic imports. +const MAX_CACHED_BODY_BYTES = 1024; + +type App = ReturnType; +type Prisma = typeof import('../../src/db/prisma-client.js').prisma; + +const emailedTokens = new Map(); + +// Every call the fake SDK receives, so a test can assert that a replayed +// payment request did NOT reach the chain a second time. +const sdkCalls = { createEscrow: 0, release: 0, refund: 0 }; + +describe('Idempotency-Key replay protection', () => { + let app: App; + let prisma: Prisma; + let idempotency: typeof import('../../src/middleware/idempotency.js'); + let errorHandler: typeof import('../../src/middleware/error-handler.js').errorHandler; + let admin: { id: string; token: string }; + let doctor: { userId: string; profileId: string; token: string }; + let patients: { userId: string; token: string; email: string }[]; + let facilityId: string; + let slots: string[]; + let patientProfileId: string; + + async function signup(role: 'patient' | 'doctor', label: string) { + const email = `${label}-${runId}@example.com`; + const res = await request(app).post('/auth/signup').send({ role, email, password: 'longenough123', fullName: `${label} Fullname` }); + expect(res.status).toBe(200); + const token = emailedTokens.get(email); + expect(token, `no verification email captured for ${email}`).toBeTruthy(); + expect((await request(app).post('/auth/verify-email').send({ token })).status).toBe(200); + return { userId: res.body.user.id as string, token: res.body.accessToken as string, email }; + } + + const asPatient = (index: number) => ({ Authorization: `Bearer ${patients[index]!.token}` }); + const asAdmin = () => ({ Authorization: `Bearer ${admin.token}` }); + + const rowFor = (userId: string, key: string) => prisma.idempotencyKey.findUnique({ where: { userId_key: { userId, key } } }); + const appointmentCount = (slotId: string) => prisma.appointment.count({ where: { availabilitySlotId: slotId } }); + const intentCount = (orderId: string) => prisma.paymentIntent.count({ where: { orderId } }); + + async function bookSlot(slotId: string, key?: string, patientIndex = 0) { + const req = request(app).post('/appointments').set(asPatient(patientIndex)).send({ availabilitySlotId: slotId }); + return key === undefined ? req : req.set('Idempotency-Key', key); + } + + const intentBody = (orderId: string) => ({ orderType: 'lab_order', orderId, patientProfileId, facilityId, amountStroops: '500000' }); + + const postIntent = (body: unknown, headers: Record) => + request(app).post('/payments/intents').set(asAdmin()).set(headers).send(body as object); + const postPaymentAction = (id: string, action: string, headers: Record = {}) => + request(app).post(`/payments/intents/${id}/${action}`).set(asAdmin()).set(headers).send({}); + + beforeAll(async () => { + await pg.initialise(); + await pg.start(); + await pg.createDatabase('docforum_test'); + execFileSync('npx', ['prisma', 'migrate', 'deploy'], { + cwd: BACKEND_ROOT, + env: { ...process.env, DATABASE_URL: databaseUrl }, + stdio: 'pipe', + }); + + process.env.DATABASE_URL = databaseUrl; + process.env.PLATFORM_PAYER_SECRET = PAYER_SECRET; + process.env.PLATFORM_RELEASER_SECRET = RELEASER_SECRET; + process.env.ESCROW_CONTRACT_ID = 'CTESTCONTRACT'; + process.env.ESCROW_TOKEN_ID = 'CTESTTOKEN'; + process.env.IDEMPOTENCY_MAX_CACHED_BODY_BYTES = String(MAX_CACHED_BODY_BYTES); + + const appModule = await import('../../src/app.js'); + const dbModule = await import('../../src/db/prisma-client.js'); + const authServices = await import('../../src/modules/auth/services/index.js'); + const paymentsServices = await import('../../src/modules/payments/services/index.js'); + idempotency = await import('../../src/middleware/idempotency.js'); + errorHandler = (await import('../../src/middleware/error-handler.js')).errorHandler; + app = appModule.createApp(); + prisma = dbModule.prisma; + + authServices.setEmailSender(async (message) => { + const match = /[?&]token=([^&\s]+)/.exec(message.text); + if (match) emailedTokens.set(message.to, decodeURIComponent(match[1])); + }); + + class FakeEscrowClient { + async createEscrow() { + sdkCalls.createEscrow += 1; + return { escrowId: BigInt(1000 + sdkCalls.createEscrow), txHash: TX_HASH }; + } + async release() { + sdkCalls.release += 1; + } + async refund() { + sdkCalls.refund += 1; + } + } + paymentsServices.setEscrowSdkForTests({ + Keypair: { fromSecret: () => ({ publicKey: () => 'GPLATFORMRELEASERPUBLICKEY' }) }, + EscrowClient: FakeEscrowClient, + } as unknown as import('../../src/modules/payments/services/index.js').EscrowSdkModule); + + const adminUser = await prisma.user.create({ + data: { email: `admin-${runId}@example.com`, passwordHash: 'x', role: 'admin', emailVerifiedAt: new Date() }, + }); + admin = { id: adminUser.id, token: jwt.sign({ sub: adminUser.id, role: 'admin' }, 'test-access-secret', { expiresIn: '15m' }) }; + + const doctorUser = await signup('doctor', 'idem-doctor'); + doctor = { ...doctorUser, profileId: (await prisma.doctorProfile.findUniqueOrThrow({ where: { userId: doctorUser.userId } })).id }; + patients = await Promise.all(Array.from({ length: 3 }, (_, i) => signup('patient', `idem-patient-${i}`))); + patientProfileId = (await prisma.patientProfile.findFirstOrThrow({ where: { userId: patients[0]!.userId } })).id; + + const tomorrow = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString().slice(0, 10); + await request(app) + .post(`/availability/doctors/${doctor.profileId}/slots/generate`) + .set('Authorization', `Bearer ${doctor.token}`) + .send({ date: tomorrow, startHour: 9, endHour: 14, slotLengthMinutes: 30 }); + slots = ((await request(app).get(`/availability/doctors/${doctor.profileId}/slots`)).body as { id: string }[]).map((s) => s.id); + expect(slots.length).toBeGreaterThanOrEqual(8); + + const facilityUser = await prisma.user.create({ data: { email: `lab-${runId}@example.com`, passwordHash: 'x', role: 'facility' } }); + facilityId = (await prisma.facilityProfile.create({ data: { userId: facilityUser.id, name: 'Idempotency Lab', type: 'lab' } })).id; + await prisma.walletLink.create({ data: { facilityId, stellarPublicKey: `G${'B'.repeat(55)}` } }); + }, 180_000); + + afterAll(async () => { + const authServices = await import('../../src/modules/auth/services/index.js'); + const paymentsServices = await import('../../src/modules/payments/services/index.js'); + authServices.resetEmailSender(); + paymentsServices.setEscrowSdkForTests(undefined); + await prisma?.$disconnect(); + await pg.stop(); + fs.rmSync(dataDir, { recursive: true, force: true }); + }); + + beforeEach(async () => { + // Only the idempotency bookkeeping is reset between tests — appointments, + // intents and slots accumulate, and every assertion is scoped to the row it + // is about. + await prisma.idempotencyKey.deleteMany(); + sdkCalls.createEscrow = 0; + sdkCalls.release = 0; + sdkCalls.refund = 0; + }); + + describe('POST /appointments', () => { + it('replays the stored response to a retry instead of booking twice', async () => { + const key = `book-replay-${runId}`; + + const first = await bookSlot(slots[0]!, key); + expect(first.status).toBe(201); + expect(first.headers['idempotency-replayed']).toBeUndefined(); + + // Same key, same body — the patient tapping "retry" after a timeout. + const retry = await bookSlot(slots[0]!, key); + expect(retry.status).toBe(201); + expect(retry.headers['idempotency-replayed']).toBe('true'); + expect(retry.body).toEqual(first.body); + + // One request ran, not two: one Appointment, one slot claim, one audit + // event. A replay is a read, never a second execution. + expect(await appointmentCount(slots[0]!)).toBe(1); + expect(await prisma.auditEvent.count({ where: { entityType: 'Appointment', entityId: first.body.id } })).toBe(1); + + const row = await rowFor(patients[0]!.userId, key); + expect(row).toMatchObject({ userId: patients[0]!.userId, key, method: 'POST', route: '/appointments' }); + expect(row!.responseStatus).toBe(201); + expect(row!.responseBody).toEqual(first.body); + expect(row!.requestHash).toMatch(/^[0-9a-f]{64}$/); + expect(row!.expiresAt.getTime()).toBeGreaterThan(Date.now()); + }); + + it('refuses the same key with a different body (422) and runs nothing', async () => { + const key = `book-mismatch-${runId}`; + expect((await bookSlot(slots[1]!, key)).status).toBe(201); + + const mismatch = await bookSlot(slots[2]!, key); + expect(mismatch.status).toBe(422); + expect(mismatch.body.code).toBe('IDEMPOTENCY_KEY_REUSED'); + + // The refused request never reached the handler: slot 2 is still open and + // holds no appointment. + expect(await appointmentCount(slots[2]!)).toBe(0); + expect((await prisma.availabilitySlot.findUniqueOrThrow({ where: { id: slots[2]! } })).status).toBe('open'); + }); + + it('rejects a malformed key with 400 before anything runs', async () => { + const bad = await bookSlot(slots[3]!, 'has a space in it'); + expect(bad.status).toBe(400); + expect(bad.body.code).toBe('INVALID_IDEMPOTENCY_KEY'); + + expect(await appointmentCount(slots[3]!)).toBe(0); + expect(await prisma.idempotencyKey.count()).toBe(0); + }); + + it('does not touch a route that has not opted in', async () => { + const res = await request(app) + .get('/appointments/mine') + .set(asPatient(0)) + .set('Idempotency-Key', `not-opted-in-${runId}`); + expect(res.status).toBe(200); + expect(await prisma.idempotencyKey.count()).toBe(0); + }); + }); + + describe('simultaneous requests', () => { + it('runs one of 8 identical booking requests and refuses or replays the other 7', async () => { + const key = `book-race-${runId}`; + + const responses = await Promise.all(Array.from({ length: 8 }, () => bookSlot(slots[4]!, key))); + + // Exactly one attempt was executed: one idempotency row, and the slot + // was claimed exactly once. + expect(await prisma.idempotencyKey.count()).toBe(1); + expect(await appointmentCount(slots[4]!)).toBe(1); + + const created = responses.filter((res) => res.status === 201); + expect(created.length).toBeGreaterThanOrEqual(1); + + // Every loser is either a 409 that says why, or a replay of the winner's + // exact response. Nothing else is an acceptable outcome, and no two + // distinct appointments were ever handed to the client. + for (const res of responses) { + if (res.status === 409) { + expect(res.body.code).toBe('IN_PROGRESS'); + expect(Number(res.headers['retry-after'])).toBeGreaterThan(0); + } else { + expect(res.status).toBe(201); + expect(res.body).toEqual(created[0]!.body); + } + } + expect(new Set(created.map((res) => res.body.id)).size).toBe(1); + }); + + it('creates one payment intent out of 8 simultaneous identical requests', async () => { + const key = `intent-race-${runId}`; + const orderId = `race-order-${runId}`; + const attempt = () => postIntent(intentBody(orderId), { 'Idempotency-Key': key }); + + const responses = await Promise.all(Array.from({ length: 8 }, () => attempt())); + + // A payment intent has no natural uniqueness of its own — without the + // key these eight requests would create eight intents, which is the + // duplicate charge issue #48 exists to stop. That makes it the sharpest + // available proof that exactly one request executed. + expect(await prisma.idempotencyKey.count()).toBe(1); + expect(await intentCount(orderId)).toBe(1); + + const created = responses.filter((res) => res.status === 201); + expect(created.length).toBeGreaterThanOrEqual(1); + for (const res of responses) { + if (res.status === 409) expect(res.body.code).toBe('IN_PROGRESS'); + else { + expect(res.status).toBe(201); + expect(res.body).toEqual(created[0]!.body); + } + } + expect(new Set(created.map((res) => res.body.id)).size).toBe(1); + }); + }); + + describe('per-user scoping', () => { + it('gives each user their own attempt under the same key string', async () => { + // Two different patients pick the *same* key value. A key is not a + // credential, and one account must never be able to replay (or read) + // another account's stored response. + const key = `shared-key-${runId}`; + + const a = await bookSlot(slots[5]!, key, 0); + const b = await bookSlot(slots[6]!, key, 1); + expect(a.status).toBe(201); + expect(b.status).toBe(201); + expect(a.body.id).not.toBe(b.body.id); + + // Two rows, one per user — the unique index is on (userId, key), not key. + expect(await prisma.idempotencyKey.count({ where: { key } })).toBe(2); + + // Each user's retry replays *their own* response, never the other's. + const aRetry = await bookSlot(slots[5]!, key, 0); + const bRetry = await bookSlot(slots[6]!, key, 1); + expect(aRetry.body).toEqual(a.body); + expect(bRetry.body).toEqual(b.body); + expect(JSON.stringify(aRetry.body)).not.toContain(b.body.id); + expect(JSON.stringify(bRetry.body)).not.toContain(a.body.id); + }); + + it('never hands one user a stored response that belongs to another', async () => { + const key = `cross-user-${runId}`; + const orderId = `cross-user-order-${runId}`; + + // Patient 0 books under this key... + const booking = await bookSlot(slots[7]!, key, 0); + expect(booking.status).toBe(201); + + // ...and an admin reuses the same key string on a different route. It is + // a different attempt, not a replay, and certainly not patient 0's body. + const adminAttempt = await postIntent(intentBody(orderId), { 'Idempotency-Key': key }); + expect(adminAttempt.status).toBe(201); + expect(JSON.stringify(adminAttempt.body)).not.toContain(booking.body.id); + + const adminRetry = await postIntent(intentBody(orderId), { 'Idempotency-Key': key }); + expect(adminRetry.status).toBe(201); + expect(adminRetry.headers['idempotency-replayed']).toBe('true'); + expect(adminRetry.body).toEqual(adminAttempt.body); + + // The patient's row still holds the patient's response, untouched. + const patientRow = await rowFor(patients[0]!.userId, key); + expect(patientRow!.responseBody).toEqual(booking.body); + expect(await prisma.idempotencyKey.count({ where: { key } })).toBe(2); + }); + }); + + describe('payments', () => { + async function createIntent(orderId: string) { + const res = await postIntent(intentBody(orderId), {}); + expect(res.status).toBe(201); + return res.body as { id: string; status: string }; + } + + it('replays intent creation', async () => { + const key = `intent-replay-${runId}`; + const orderId = `intent-replay-order-${runId}`; + + const first = await postIntent(intentBody(orderId), { 'Idempotency-Key': key }); + expect(first.status).toBe(201); + const retry = await postIntent(intentBody(orderId), { 'Idempotency-Key': key }); + + expect(retry.status).toBe(201); + expect(retry.headers['idempotency-replayed']).toBe('true'); + expect(retry.body).toEqual(first.body); + expect(await intentCount(orderId)).toBe(1); + }); + + it('treats the same key against a different intent id as a different request', async () => { + const first = await createIntent(`cross-target-a-${runId}`); + const second = await createIntent(`cross-target-b-${runId}`); + const key = `fund-cross-target-${runId}`; + + expect((await postPaymentAction(first.id, 'fund', { 'Idempotency-Key': key })).status).toBe(200); + + // Same key, same (empty) body, different :id. Replaying the first + // intent's response here would be a lie about a different record. + const wrongTarget = await postPaymentAction(second.id, 'fund', { 'Idempotency-Key': key }); + expect(wrongTarget.status).toBe(422); + expect(wrongTarget.body.code).toBe('IDEMPOTENCY_KEY_REUSED'); + + // Refused before any chain call: the second intent is untouched. + expect((await prisma.paymentIntent.findUniqueOrThrow({ where: { id: second.id } })).status).toBe('created'); + expect(sdkCalls.createEscrow).toBe(1); + }); + + it('replays fund/release/refund without calling the chain twice', async () => { + const funded = await createIntent(`escrow-order-${runId}`); + + const fundKey = `fund-${runId}`; + const firstFund = await postPaymentAction(funded.id, 'fund', { 'Idempotency-Key': fundKey }); + expect(firstFund.status).toBe(200); + expect(firstFund.body.status).toBe('escrowed'); + expect(sdkCalls.createEscrow).toBe(1); + + // The retry a dropped connection causes must not push a second Stellar + // transaction — exactly the mess issue #24's reconcile job would have to + // clean up afterwards. + const fundRetry = await postPaymentAction(funded.id, 'fund', { 'Idempotency-Key': fundKey }); + expect(fundRetry.status).toBe(200); + expect(fundRetry.headers['idempotency-replayed']).toBe('true'); + expect(fundRetry.body).toEqual(firstFund.body); + expect(sdkCalls.createEscrow).toBe(1); + + const releaseKey = `release-${runId}`; + const firstRelease = await postPaymentAction(funded.id, 'release', { 'Idempotency-Key': releaseKey }); + expect(firstRelease.status).toBe(200); + expect(firstRelease.body.status).toBe('released'); + expect(sdkCalls.release).toBe(1); + + const releaseRetry = await postPaymentAction(funded.id, 'release', { 'Idempotency-Key': releaseKey }); + expect(releaseRetry.status).toBe(200); + expect(releaseRetry.headers['idempotency-replayed']).toBe('true'); + expect(releaseRetry.body).toEqual(firstRelease.body); + expect(sdkCalls.release).toBe(1); + + // Refund runs against its own intent, so release's replay can't leak into + // it — and the refund's own retry is likewise exactly one chain call. + const refundable = await createIntent(`refund-order-${runId}`); + expect((await postPaymentAction(refundable.id, 'fund')).status).toBe(200); + + const refundKey = `refund-${runId}`; + const firstRefund = await postPaymentAction(refundable.id, 'refund', { 'Idempotency-Key': refundKey }); + expect(firstRefund.status).toBe(200); + expect(firstRefund.body.status).toBe('refunded'); + + const refundRetry = await postPaymentAction(refundable.id, 'refund', { 'Idempotency-Key': refundKey }); + expect(refundRetry.status).toBe(200); + expect(refundRetry.body).toEqual(firstRefund.body); + expect(sdkCalls.refund).toBe(1); + }); + + it('caches a deterministic 4xx so a retry gets the same answer', async () => { + const intent = await createIntent(`four-oh-four-order-${runId}`); + // Fund it once (no key), then fund it again through the route: the 409 is + // this key's answer, and replaying it is the useful behaviour. + expect((await postPaymentAction(intent.id, 'fund')).status).toBe(200); + expect(sdkCalls.createEscrow).toBe(1); + + const key = `already-funded-${runId}`; + const first = await postPaymentAction(intent.id, 'fund', { 'Idempotency-Key': key }); + expect(first.status).toBe(409); + + const retry = await postPaymentAction(intent.id, 'fund', { 'Idempotency-Key': key }); + expect(retry.status).toBe(409); + expect(retry.headers['idempotency-replayed']).toBe('true'); + expect(retry.body).toEqual(first.body); + // Replaying an answer must never reach the chain. + expect(sdkCalls.createEscrow).toBe(1); + }); + }); + + // A gated handler makes "one request is running" a fact this suite controls, + // instead of a race it hopes to win. Same middleware, same database rows. + describe('in-flight, failures and crash recovery', () => { + let mini: express.Express; + let userId: string; + let handlerCalls: { slow: number; boom: number; huge: number }; + let hold: boolean; + let release: () => void; + + beforeAll(() => { + handlerCalls = { slow: 0, boom: 0, huge: 0 }; + hold = false; + release = () => {}; + + const miniApp = express(); + miniApp.use(express.json()); + // Stands in for `authenticate`: the middleware scopes keys by user, so + // these tests need a user without needing a JWT. + miniApp.use((req, _res, next) => { + req.user = { id: userId, role: 'patient' }; + next(); + }); + + miniApp.post('/slow/:id', idempotency.idempotency(), async (_req, res) => { + handlerCalls.slow += 1; + if (hold) await new Promise((resolve) => (release = resolve)); + res.status(201).json({ created: true, at: handlerCalls.slow }); + }); + + miniApp.post('/boom', idempotency.idempotency(), () => { + handlerCalls.boom += 1; + // Reaches the shared error handler: a 5xx, not a 4xx. + throw new Error('simulated server error'); + }); + + miniApp.post('/huge', idempotency.idempotency(), (_req, res) => { + handlerCalls.huge += 1; + res.status(200).json({ blob: 'x'.repeat(MAX_CACHED_BODY_BYTES * 2) }); + }); + + miniApp.use(errorHandler); + mini = miniApp; + }); + + beforeEach(async () => { + await prisma.idempotencyKey.deleteMany(); + handlerCalls = { slow: 0, boom: 0, huge: 0 }; + hold = false; + release = () => {}; + userId = patients[0]!.userId; + }); + + const slow = (body: unknown, key: string) => request(mini).post('/slow/1').set('Idempotency-Key', key).send(body as object); + + it('answers a concurrent duplicate with 409 IN_PROGRESS, then replays once it finishes', async () => { + const key = `gate-${runId}`; + hold = true; + // `.then()` on a supertest Test is what actually starts the request, so + // the gate has to be awaited through the returned promise, not the Test. + const first = slow({ amount: 10 }, key).then((res) => res); + + // Wait until the winner is genuinely inside the handler. + await vi.waitFor(() => expect(handlerCalls.slow).toBe(1)); + + const loser = await slow({ amount: 10 }, key); + expect(loser.status).toBe(409); + expect(loser.body.code).toBe('IN_PROGRESS'); + expect(Number(loser.headers['retry-after'])).toBeGreaterThan(0); + + // A different body under the same key is refused even while the first is + // in flight, so a colliding key can never redirect the running request. + const mismatch = await slow({ amount: 11 }, key); + expect(mismatch.status).toBe(422); + expect(mismatch.body.code).toBe('IDEMPOTENCY_KEY_REUSED'); + + release(); + const finished = await first; + expect(finished.status).toBe(201); + + // Once it has finished, the same request replays instead of re-running. + const replay = await slow({ amount: 10 }, key); + expect(replay.status).toBe(201); + expect(replay.headers['idempotency-replayed']).toBe('true'); + expect(replay.body).toEqual(finished.body); + expect(handlerCalls.slow).toBe(1); + }); + + it('never caches a 5xx, so the client can retry the same key', async () => { + const key = `boom-${runId}`; + const boom = () => request(mini).post('/boom').set('Idempotency-Key', key).send({}); + + expect((await boom()).status).toBe(500); + // A server error is not a result: the row is deleted, not cached. + expect(await rowFor(userId, key)).toBeNull(); + + // The client retries with the same key and the request runs again. + expect((await boom()).status).toBe(500); + expect(handlerCalls.boom).toBe(2); + expect(await rowFor(userId, key)).toBeNull(); + }); + + it('does not cache a response too large to replay', async () => { + const key = `huge-${runId}`; + const huge = () => request(mini).post('/huge').set('Idempotency-Key', key).send({}); + + expect((await huge()).status).toBe(200); + expect(await rowFor(userId, key)).toBeNull(); + + // The retry re-executes rather than getting a body back that was dropped. + expect((await huge()).status).toBe(200); + expect(handlerCalls.huge).toBe(2); + }); + + it('takes over an attempt abandoned by a dead process — but only for the same request', async () => { + const key = `stale-${runId}`; + const body = { amount: 10 }; + const requestHash = idempotency.hashRequest('POST', '/slow/1', body); + + // What a process that died mid-request leaves behind: a claimed key with + // no response, aged well past the crash-recovery window. + await prisma.idempotencyKey.create({ + data: { + userId, + key, + method: 'POST', + route: '/slow/:id', + requestHash, + createdAt: new Date(Date.now() - 60 * 60 * 1000), + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + + // A different request under that key is still refused — a stale row must + // never be re-pointed at a new operation. + const mismatch = await slow({ amount: 99 }, key); + expect(mismatch.status).toBe(422); + expect(await rowFor(userId, key)).not.toBeNull(); + + // The genuine retry runs and finishes the attempt. + const takeover = await slow(body, key); + expect(takeover.status).toBe(201); + expect(handlerCalls.slow).toBe(1); + + const row = await rowFor(userId, key); + expect(row!.responseStatus).toBe(201); + expect(row!.requestHash).toBe(requestHash); + }); + + it('frees a key whose TTL has passed — a day-old answer is not replayed', async () => { + const key = `expired-${runId}`; + const body = { amount: 10 }; + await prisma.idempotencyKey.create({ + data: { + userId, + key, + method: 'POST', + route: '/slow/:id', + requestHash: idempotency.hashRequest('POST', '/slow/1', body), + responseStatus: 201, + responseBody: { created: true, at: 'ancient' }, + createdAt: new Date(Date.now() - 48 * 60 * 60 * 1000), + expiresAt: new Date(Date.now() - 60 * 1000), + }, + }); + + const res = await slow(body, key); + expect(res.status).toBe(201); + // Executed, not replayed — the stale response is gone. + expect(res.body.at).toBe(1); + expect(handlerCalls.slow).toBe(1); + expect((await rowFor(userId, key))!.createdAt.getTime()).toBeGreaterThan(Date.now() - 60_000); + }); + }); + + describe('retention', () => { + it('prunes only rows past their TTL', async () => { + const keep = `keep-${runId}`; + const drop = `drop-${runId}`; + const base = { userId: admin.id, method: 'POST', route: '/payments/intents', requestHash: 'a'.repeat(64) }; + await prisma.idempotencyKey.create({ + data: { ...base, key: keep, responseStatus: 201, createdAt: new Date(), expiresAt: new Date(Date.now() + 60 * 60 * 1000) }, + }); + await prisma.idempotencyKey.create({ + data: { ...base, key: drop, responseStatus: 201, createdAt: new Date(Date.now() - 48 * 60 * 60 * 1000), expiresAt: new Date(Date.now() - 60 * 1000) }, + }); + + expect(await idempotency.pruneIdempotencyKeys()).toBe(1); + expect((await prisma.idempotencyKey.findMany()).map((row) => row.key)).toEqual([keep]); + }); + }); +}); diff --git a/backend/tests/unit/idempotency.test.ts b/backend/tests/unit/idempotency.test.ts new file mode 100644 index 00000000..f65d5576 --- /dev/null +++ b/backend/tests/unit/idempotency.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from 'vitest'; +import { canonicalJson, hashRequest, isValidIdempotencyKey } from '../../src/middleware/idempotency'; + +// The pure parts of the middleware (Phase 8, issue #48). Everything that +// touches the database is proven against a real Postgres in +// tests/integration/idempotency.test.mts — these are the pieces where a wrong +// answer would silently become a 422-against-yourself or a cross-user replay, +// and they are cheap to pin down exactly. +const NUL_KEY = `nul${String.fromCharCode(0)}byte`; + +describe('idempotency key + request hash (issue #48)', () => { + it('accepts ordinary client-generated keys and rejects unusable ones', () => { + const accepted = [ + 'a', + 'x'.repeat(255), + '9f1c-4d2e-4a7b', + 'order_42/attempt~1+2', + '550e8400-e29b-41d4-a716-446655440000', + 'key:with:colons', + ]; + for (const key of accepted) { + expect(isValidIdempotencyKey(key), key).toBe(true); + } + + // Empty, whitespace, non-ASCII, control characters and anything long + // enough to be a storage/DoS vector are refused. + const refused = ['', ' ', 'x'.repeat(256), 'has space', `tab${String.fromCharCode(9)}here`, `nl${String.fromCharCode(10)}here`, 'emoji🔑', NUL_KEY]; + for (const key of refused) { + expect(isValidIdempotencyKey(key), JSON.stringify(key)).toBe(false); + } + }); + + it('serialises a body canonically, independent of key order', () => { + expect(canonicalJson({ b: 1, a: 2 })).toBe('{"a":2,"b":1}'); + expect(canonicalJson({ a: { z: 1, y: [3, { q: 1, p: 2 }] }, b: 'x' })).toBe( + '{"a":{"y":[3,{"p":2,"q":1}],"z":1},"b":"x"}', + ); + expect(canonicalJson(undefined)).toBe('null'); + expect(canonicalJson([1, 'two', null])).toBe('[1,"two",null]'); + + // Two shapes of the same logical body must produce one string, or a retry + // of a request that never got an answer 422s against itself. + expect(canonicalJson({ availabilitySlotId: 'slot-1', note: 'a' })).toBe( + canonicalJson({ note: 'a', availabilitySlotId: 'slot-1' }), + ); + + // An absent key and an explicitly-null one are different bodies: `undefined` + // is dropped (as JSON.stringify does), `null` is a value. + expect(canonicalJson({ a: 1, b: undefined })).toBe(canonicalJson({ a: 1 })); + expect(canonicalJson({ a: 1, b: null })).not.toBe(canonicalJson({ a: 1 })); + + // Order inside an array IS significant — it is data, not a set. + expect(canonicalJson([1, 2])).not.toBe(canonicalJson([2, 1])); + }); + + it('hashes the same request identically and different requests differently', () => { + const hash = hashRequest('POST', '/appointments', { availabilitySlotId: 'slot-1' }); + + // Method case and key order are not part of the request. + expect(hashRequest('post', '/appointments', { availabilitySlotId: 'slot-1' })).toBe(hash); + expect(hashRequest('POST', '/appointments', { note: 'a', availabilitySlotId: 'slot-1' })).toBe( + hashRequest('POST', '/appointments', { availabilitySlotId: 'slot-1', note: 'a' }), + ); + + // Different bodies, different target ids, different methods, different + // routes — all different requests under the same key. + expect(hashRequest('POST', '/appointments', { availabilitySlotId: 'slot-1', note: null })).not.toBe(hash); + expect(hashRequest('POST', '/appointments', { availabilitySlotId: 'slot-2' })).not.toBe(hash); + expect(hashRequest('POST', '/payments/intents/abc/fund', {})).not.toBe( + hashRequest('POST', '/payments/intents/xyz/fund', {}), + ); + expect(hashRequest('POST', '/payments/intents', {})).not.toBe(hashRequest('GET', '/payments/intents', {})); + expect(hashRequest('DELETE', '/appointments', { availabilitySlotId: 'slot-1' })).not.toBe(hash); + + // SHA-256 hex — no length or encoding surprises for a fixed-width column. + expect(hash).toMatch(/^[0-9a-f]{64}$/); + }); +}); diff --git a/docs/api/README.md b/docs/api/README.md index 1fcc4633..367f88b8 100644 --- a/docs/api/README.md +++ b/docs/api/README.md @@ -240,6 +240,99 @@ design, not the final one. Verified doctors can create a consultation with `POST /orders/consultations` (`appointmentId`, optional `outcome` and `notes`) — the same write as `POST /consultations` plus the verified-doctor gate, so all of its one-per-appointment/encryption/audit rules apply. Issue a prescription with `POST /orders/consultations/:consultationId/prescriptions` (`facilityProfileId`, positive integer `amountStroops`, and `items`), or a lab order with `POST /orders/consultations/:consultationId/lab-orders` (`facilityProfileId`, `amountStroops`, `testType`, optional `urgency` and `notes`). Issuance creates its payment intent in the same transaction. Corrections create superseding immutable orders: a created intent moves to the correction; an escrowed intent remains linked to the original and is handled by the existing admin refund action. +## Idempotency-Key (`POST` mutations) — `backend/src/middleware/idempotency.ts` + +Phase 8, issue #48. A patient on a mobile connection loses signal mid-request, +sees a timeout and taps retry. Without this, the backend cannot tell that retry +from a deliberate second attempt, and the same tap books twice or funds twice — +and no amount of "check then write" fixes it, because two retries 20 ms apart +both pass the check. This is the same problem §5.1 solves for slots, one layer +up. + +**Send `Idempotency-Key: ` on these routes:** + +| Route | +|---| +| `POST /appointments` | +| `POST /payments/intents` | +| `POST /payments/intents/:id/fund` | +| `POST /payments/intents/:id/release` | +| `POST /payments/intents/:id/refund` | + +The header is **optional** — a request without one behaves exactly as before, +so nothing breaks if a client doesn't send it. But any client that retries +should send one, and should generate a fresh value per *logical* operation +(a UUID, or `crypto.randomUUID()`), not per HTTP attempt. + +What the header buys you: + +| Situation | Response | +|---|---| +| Same key, **same** request, first attempt | The normal response (`201`/`200`), stored for 24 h | +| Same key, **same** request, retried within 24 h | The **stored** status and body, byte-for-byte, plus `Idempotency-Replayed: true`. The handler does not run again — no second booking, no second Stellar transaction | +| Same key, **different** body / route / target `:id` | `422` `{ message, code: 'IDEMPOTENCY_KEY_REUSED' }`. Nothing is executed | +| Same key, while the first request is **still running** | `409` `{ message, code: 'IN_PROGRESS' }` + `Retry-After: 1`. Retry shortly with the *same* key — it will then replay | +| Same key after the stored response is older than 24 h | Treated as a new request and executed | +| Malformed key (empty, whitespace, non-ASCII, > 255 chars) | `400` `{ message, code: 'INVALID_IDEMPOTENCY_KEY' }`. Nothing is executed | +| The attempt fails with **5xx** | Nothing is cached and no row is kept — **retry with the same key and the request runs again.** A server error is not a result; caching one would turn a transient failure into a permanent answer for that key | + +`4xx` responses *are* cached: the request ran, and its answer is the answer, so +a retry gets the same `409` rather than a different outcome. + +**Client requirements (docforum-web):** + +- Generate the key when the user *initiates* the action (not inside the retry + loop) and reuse it for every retry of that action. +- Treat `409 IN_PROGRESS` as "retry in `Retry-After` seconds", not as a + failure — a concurrent duplicate of the same tap lands here. +- Treat `422 IDEMPOTENCY_KEY_REUSED` as a client bug: two different operations + were sent under one key. Generate a new key for the second one. +- A replayed response is identical to the original, so a client must not + expect a fresh object id. + +**Why it isn't an in-memory lock:** ownership is decided by a unique constraint +on `(userId, key)` in Postgres (`IdempotencyKey`, `ARCHITECTURE.md` §4.12). +Whichever request wins the INSERT runs the handler; every other one reads the +row. A `Map` of pending keys would be correct on one instance and silently +wrong on two — and would be emptied by exactly the restart that a retry storm +follows. Keys are namespaced **per user**: one account can never replay, read +or shadow another account's stored response, because every lookup is filtered +by user id. An `Idempotency-Key` is a guessable string and is not a credential. + +Env knobs: `IDEMPOTENCY_RECORD_TTL_HOURS` (default 24), +`IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS` (default 300 — how long an attempt +abandoned by a crashed process stays stuck before an identical retry may take +it over), `IDEMPOTENCY_MAX_CACHED_BODY_BYTES` (default 256 KiB; a larger +response is executed but not cached, so a retry re-runs it), +`IDEMPOTENCY_PRUNE_INTERVAL_SECONDS` (cleanup interval in `src/server.ts`). + +Stored responses are not patient data today and are not part of the +patient-export bundle; see `ARCHITECTURE.md` §6.2.6 for the condition that +would change that. + +## Payments (`/payments`) — `backend/src/modules/payments` + +Custodial v1 (`docs/adr/0004-custodial-payments-v1.md`): this repo holds the +platform payer/releaser identities and calls `docforum-escrow`'s SDK. Every +intent route requires a verified address, and the intent routes are +admin-triggered in v1 because Phase 4/5 order issuance doesn't exist yet to +trigger them. + +| Method & path | Auth | Body | Notes | +|---|---|---|---| +| `POST /payments/wallet-link` | facility | `{ stellarPublicKey }` | Links the **facility's** payout wallet (the on-chain payee — never the patient's). | +| `GET /payments/wallet-link` | facility | — | The caller's own link, or `404`. | +| `POST /payments/intents` | admin | `{ orderType, orderId, patientProfileId, facilityId, amountStroops }` | `amountStroops` is a positive integer **string** (a JSON number can't carry bigint precision). `orderType`/`orderId` are opaque — `Prescription`/`LabOrder` don't exist yet (Phase 4). Honours `Idempotency-Key`. | +| `GET /payments/intents/:id` | admin | — | Single intent; `404` if unknown. | +| `POST /payments/intents/:id/fund` | admin | — | `created` → `escrowed`, moves funds from the platform payer. `409` if the intent isn't `created`, or its facility has no linked wallet. Honours `Idempotency-Key`. | +| `POST /payments/intents/:id/release` | admin | — | `escrowed` → `released`, pays the facility. Honours `Idempotency-Key`. | +| `POST /payments/intents/:id/refund` | admin | — | `escrowed` → `refunded`. Honours `Idempotency-Key`. | + +A retried fund/release/refund replays its first response instead of pushing a +second Stellar transaction. That is the belt to §4.10's braces, not a +replacement for them: two *different* keys racing the same transition still +resolve through the conditional status write, and the loser gets `409`. + ## Audit trail (`/admin/audit-events`) — `backend/src/modules/audit` Phase 8, issue #43. Every response carries an `X-Request-Id` header: a