From 9df6c5d41757b85d6c0eef36a8219b64692d7c3d Mon Sep 17 00:00:00 2001 From: Ibrahim Qozeem Date: Mon, 19 Oct 2026 11:46:10 +0000 Subject: [PATCH] feat(api): idempotency-Key support for booking and payment mutations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A patient on a flaky mobile connection times out mid-request and taps retry. The backend cannot tell that retry from a deliberate second attempt, so the same tap books twice or funds twice. "Check then write" cannot fix it either — two retries 20 ms apart both pass the check. This is the same problem ARCHITECTURE.md §5.1 already solves for slots, one layer up. The mechanism is the boring one: the unique constraint on (userId, key) IS the lock. Whoever wins the INSERT ... ON CONFLICT DO NOTHING owns the attempt; everyone else reads the row and either replays the stored response, is told one is in flight, or is refused for reusing the key on a different request. No in-memory lock anywhere — a Map of pending keys is correct on one instance, silently broken on two, and emptied by exactly the restart a retry storm follows. Schema: IdempotencyKey (userId, key, method, route, requestHash, responseStatus/body/headers, createdAt, expiresAt), unique (userId, key) plus an expiresAt index. Generated with scripts/gen-migration.mjs (real diff, not hand-written); scripts/check-migration-drift.mjs reports no drift. Middleware (src/middleware/idempotency.ts), opt-in per route: - same key + same request within 24h -> stored status/body replayed, with Idempotency-Replayed: true; the handler does not run again - same key + different body/route/target :id -> 422 IDEMPOTENCY_KEY_REUSED - collides with a request still in flight -> 409 IN_PROGRESS + Retry-After - malformed key -> 400 INVALID_IDEMPOTENCY_KEY, before anything runs - 5xx is NEVER cached and the row is deleted: a server error is not a result, so the client retries the same key and the request runs again - an attempt left unfinished by a crashed process is taken over, but only by an identical request — a stale row is never re-pointed at a new operation - keys are namespaced per user: one account can never replay or read another's stored body Opted in: POST /appointments, POST /payments/intents, and POST /payments/intents/:id/{fund,release,refund}. The header stays optional, so existing clients are unaffected. This complements rather than replaces the conditional status write in commitTransition — two different keys racing one transition still resolve to a single winner and a 409. Tests: 18 integration tests against a real Postgres (own embedded cluster, port 5443) plus 3 unit tests. A gated handler makes "in progress" a fact the test controls rather than a race it hopes to win; a genuine 8-way concurrent burst proves the unique index decides ownership; the fake escrow SDK counts calls so a retried fund/release/refund is proven to make exactly one chain call; per-user scoping is proven by two accounts reusing one key string. Docs: ARCHITECTURE.md §4.12 + §7, new §6.2 hard question (a route returning clinical free text must not opt in until the export registry can account for the cached copy), ARCHITECTURE_ESSENTIALS.md data-model map + known gaps, docs/api/README.md (client requirements, status table, env knobs), ROADMAP.md item + changelog, backend/.env.example. Closes #48 --- ARCHITECTURE.md | 42 ++ ARCHITECTURE_ESSENTIALS.md | 2 + ROADMAP.md | 26 +- backend/.env.example | 9 + .../migration.sql | 41 ++ backend/prisma/schema.prisma | 45 ++ backend/src/config/index.ts | 25 + backend/src/middleware/idempotency.ts | 398 +++++++++++ .../src/modules/appointments/routes/index.ts | 9 + backend/src/modules/payments/routes/index.ts | 15 +- backend/src/server.ts | 19 + .../tests/integration/idempotency.test.mts | 666 ++++++++++++++++++ backend/tests/unit/idempotency.test.ts | 78 ++ docs/api/README.md | 101 ++- 14 files changed, 1470 insertions(+), 6 deletions(-) create mode 100644 backend/prisma/migrations/20261019113906_add_idempotency_key/migration.sql create mode 100644 backend/src/middleware/idempotency.ts create mode 100644 backend/tests/integration/idempotency.test.mts create mode 100644 backend/tests/unit/idempotency.test.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d7592346..d4ff1bc8 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -292,6 +292,46 @@ Bundle format (schemaVersion 1): `{ format: "docforum.patient-export", schemaVersion, exportId, generatedAt, sections: { : { version, items } } }` — see §3.1 for where sections come from. +### 4.12 Idempotency keys (Phase 8, issue #48) +``` +IdempotencyKey + id, user_id, key, method, route, request_hash + response_status (nullable int), response_body (jsonb, nullable), + response_headers (jsonb, nullable — `Location` only, never Set-Cookie) + created_at, expires_at + # unique (user_id, key) — this constraint IS the concurrency mechanism, the + same way §5.1's conditional slot claim is. Whichever request wins the + INSERT owns the attempt; every other one reads the row and either replays + the stored response, is told one is in flight, or is refused for reusing + the key on a different request. There is no in-memory lock anywhere, so + this behaves identically on one instance and on ten. + # Scoped per user on purpose: an Idempotency-Key is a guessable string, so + it must never be usable as a bearer credential for another account's + stored body. Every read filters on user_id. + # route is the route *pattern* ('/payments/intents/:id/fund'), request_hash + is SHA-256 over method + concrete path + canonical JSON body. The concrete + path is in the hash so one key aimed at two different :ids is a mismatch, + not a replay of the first record's response. + # response_* are NULL while the attempt is in flight. That gap is what a + concurrent duplicate sees, and what it waits on. + # NOT an AuditEvent: a 5xx deletes the row outright (a server error is not + a result — the client retries with the same key and it runs again), and an + attempt abandoned by a crashed process is taken over once it is older than + IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS. A takeover is only ever by an + identical request, so a stale row can't be re-pointed at a new operation. + # Retained for IDEMPOTENCY_RECORD_TTL_HOURS (default 24) and pruned by an + interval in src/server.ts. Reads filter on expires_at first, so a missed + prune costs storage, not correctness. + # Holds a copy of a response the requesting user already received. Not + patient data today, and deliberately not exported or listed — see §6.2.6 + for the condition that would change that. +``` +Opted into by middleware (`src/middleware/idempotency.ts`), per route, not +globally: `POST /appointments`, `POST /payments/intents`, and +`POST /payments/intents/:id/{fund,release,refund}`. The header is optional +(a request without one passes through untouched, so existing clients keep +working), but a client that retries should always send one. + ## 5. Key flows (sequence-level, not code) ### 5.1 Booking (concurrency-safe) @@ -327,6 +367,7 @@ schemaVersion, exportId, generatedAt, sections: { : { version, items } } } 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 — every FR around auditability (PRD §7) currently relies on `created_at/updated_at` timestamps only, which is not sufficient for "who changed what." A dedicated `AuditEvent` table is likely needed before this ships to real patients. **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, doctor verification and payment fund/release/refund. **Still open:** Phase 3/4 transitions (referrals, prescriptions, lab orders, fulfillment) must be wired as they're built; 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). 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. @@ -338,6 +379,7 @@ schemaVersion, exportId, generatedAt, sections: { : { version, items } } } - **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/ARCHITECTURE_ESSENTIALS.md b/ARCHITECTURE_ESSENTIALS.md index bdbb1d42..e8afab36 100644 --- a/ARCHITECTURE_ESSENTIALS.md +++ b/ARCHITECTURE_ESSENTIALS.md @@ -29,6 +29,7 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc `User` → `EmailVerificationToken` (Phase 8, issue #34 — hashed/expiring, single-use; `User.emailVerifiedAt` gates booking + payment mutations, never login/browsing). `LoginAttempt` (Phase 8, issue #35 — Postgres-backed login throttle buckets, one per normalised email and per client IP; counters move only via a conditional upsert). `PatientProfile` → `PatientExport` → `PatientExportChunk` (Phase 8, issue #40 — right-of-access export jobs + their chunked JSON bundle; each module contributes a section via its own service, registered in `src/export-sections.ts` — see ARCHITECTURE.md §3.1). +`User` → `IdempotencyKey` (Phase 8, issue #48 — `Idempotency-Key` replay protection for booking + payment mutations; the unique `(userId, key)` index *is* the lock, so there is no in-memory lock; middleware in `src/middleware/idempotency.ts`, per-route opt-in, see §4.12). `FacilityProfile` → `WalletLink` (payout address) ; `PatientProfile`/`FacilityProfile` → `PaymentIntent` (opaque `orderType`/`orderId` — see docs/adr/0004) ## Payments (Phase 5.5 — see docs/adr/0004-custodial-payments-v1.md) @@ -42,6 +43,7 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc - No routing rule yet for a referral with no specific target doctor (which specialist gets it?). - No expiry/TTL job for held slots or pending referrals — currently just an enum value with no worker. - Polymorphic `FulfillmentRecord.order_id` is a sketch, not final — may need splitting into two tables. +- `IdempotencyKey.response_body` (Phase 8, issue #48 — ARCHITECTURE.md §4.12) is a temporary copy of a response, kept 24 h. Non-PHI today; **a route that returns clinical free text must not opt in** until the export registry (ARCHITECTURE.md §3.1) can account for it. - `AuditEvent` foundation exists (Phase 8, issue #43 — ARCHITECTURE.md §4.10): append-only (DB triggers), written by `auditService.record(tx, …)` **inside the same transaction** as the change. Wired for booking, doctor verification and payment fund/release/refund only — **Phase 3/4 transitions must call it when they're built.** Never put PHI in `metadata`. - No email/SMS provider integrated yet (Phase 6, issue #17) — email verification (issue #34) therefore can't deliver its link on a hosted deployment; `backend/scripts/mark-email-verified.mjs` is the interim operator path. - One doctor = one calendar assumed; multi-facility doctors not modeled. diff --git a/ROADMAP.md b/ROADMAP.md index dd2c6462..6fc67976 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1,6 +1,6 @@ # ROADMAP.md — DocForum -Last updated: 2026-10-17 +Last updated: 2026-10-19 Status: Phase 0, Phase 1, and Phase 5.5 (Payments, custodial v1 — see `docs/adr/0004-custodial-payments-v1.md`) built and tested. Phases 2-6 and 8-9 scoped into GitHub issues (#1-#22, #21 closed); Phase 7 is @@ -107,7 +107,7 @@ 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. @@ -115,6 +115,7 @@ for status and issue links. - [ ] Admin account lifecycle: secure bootstrap CLI, admin invites, and account deactivation. Tracked as [issue #39](https://github.com/DocForum/docforum-core/issues/39) (High, 200 pts) — **in progress**. - [ ] Patient data export (right of access) as a complete, machine-readable bundle. `POST /patients/me/export` + `GET /patients/me/export/:id` (+ `/download`), owner-only; an exporter registry where each module contributes its own section through its service (ARCHITECTURE.md §3.1 — `account`, `profile`, `appointments`, `payments` today); versioned JSON with `generatedAt`; generated by an interval worker in `src/jobs/patient-exports.ts` and stored as bounded chunks in Postgres (`PatientExport`/`PatientExportChunk`, §4.11 — no object-storage provider exists yet, see §6.3.4); expiry + cleanup after `PATIENT_EXPORT_TTL_HOURS`. Tracked as [issue #40](https://github.com/DocForum/docforum-core/issues/40) (High, 200 pts) — **in review**, branch `feat/40-patient-data-export`. Later modules (referrals, prescriptions, lab orders) must register their own section when they land; a verified guardian exporting a dependent's data is left to issue #42's model. - [ ] `AuditEvent` table and write path — **foundation done, Phase 3/4 wiring pending.** Append-only `AuditEvent` (DB triggers reject UPDATE/DELETE/TRUNCATE), `auditService.record()` that must run inside the state change's own transaction, request ids on every response, and `GET /admin/audit-events` (entity/actor/date filters, keyset cursor). Wired into appointment booking, doctor verification and payment fund/release/refund. Referrals, prescriptions, lab orders and fulfillment (Phases 3–5) must call `record()` when they land; the login-throttle admin clear still logs to console only. Tracked as [issue #43](https://github.com/DocForum/docforum-core/issues/43) (High, 200 pts) — **in review**, branch `feat/43-auditevent-foundation`. +- [ ] Idempotency-Key support for booking and payment mutations. `POST /appointments`, `POST /payments/intents` and `POST /payments/intents/:id/{fund,release,refund}` honour an `Idempotency-Key` header: a repeat within 24 h replays the stored response instead of running the mutation again, the same key with a different body/route/target is `422`, and a second request arriving while the first is still running is `409 IN_PROGRESS` + `Retry-After`. Concurrency is decided by a unique `(userId, key)` constraint on a new `IdempotencyKey` table (ARCHITECTURE.md §4.12) — deliberately not an in-memory lock, which would be correct on one instance and silently broken on two. Keys are namespaced per user (one account can never replay or read another's stored body), 5xx is never cached so a client can retry the same key, and a crashed attempt is taken over by an identical retry after `IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS`. Tracked as [issue #48](https://github.com/DocForum/docforum-core/issues/48) (High, 200 pts) — **in review**, branch `feat/idempotency-key-support-for-booking-and-payment`. - [ ] JWT refresh revocation list (ARCHITECTURE.md §6.1.4). Tracked as [issue #18](https://github.com/DocForum/docforum-core/issues/18) (Medium, 150 pts) — unblocked now, operates on Phase 1's auth module. - [ ] Held-slot TTL/release job (ARCHITECTURE.md §6.2.1). Tracked as [issue #19](https://github.com/DocForum/docforum-core/issues/19) (Medium, 150 pts) — unblocked now, operates on Phase 1's `AvailabilitySlot`. - [x] Access-control test suite: who may read intake, consultation and payment data. Integration tests (`tests/integration/access-control.test.mts`) establishing the test harness and access-control patterns for the three PHI data categories; intake/consultation test cases are scaffolded as placeholders until Phase 2/3 routes exist, payment-intent access cases exercise the live admin-gated routes. Tracked as [issue #44](https://github.com/DocForum/docforum-core/issues/44) — **Done**. @@ -405,3 +406,24 @@ for status and issue links. observed state, so a racing second action gets 409 instead of silently overwriting). Rollback coupling proven with a real injected DB failure, not a mock. Phase 3/4 wiring explicitly left pending. +- 2026-10-19 — Issue #48 (`Idempotency-Key` for booking and payment + mutations) opened as a PR: an `IdempotencyKey` table whose unique + `(userId, key)` index *is* the concurrency mechanism — the winner of the + INSERT runs the handler, everyone else reads the row and replays, waits or + is refused. Deliberately not an in-memory lock (correct on one instance, + silently broken on two, and emptied by the restart a retry storm follows), + which makes this the §5.1 booking guarantee applied one layer up. Opted in + per route on `POST /appointments`, `POST /payments/intents` and + `POST /payments/intents/:id/{fund,release,refund}`; the header stays + optional so no existing client breaks. A retry within 24 h replays the + stored response (proven for payments by counting escrow-SDK calls: a retried + fund makes exactly one chain call), the same key with a different body, + route or target `:id` is `422`, and a request that collides with one still + in flight is `409 IN_PROGRESS` + `Retry-After` — proven both deterministically + (handler held open by a gate) and under a genuine 8-way concurrent burst. + Keys are scoped per user so one account can never replay or read another's + stored body; 5xx is never cached so a client can retry the same key and have + it actually run; an attempt left unfinished by a crashed process is taken over + by an *identical* retry only. Recorded as a new hard question in + ARCHITECTURE.md §6.2.6: a route returning clinical free text must not opt in + until the export registry can account for the cached copy. diff --git a/backend/.env.example b/backend/.env.example index e815beaa..2ad35236 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -40,6 +40,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 7cfb28b6..6edbc1aa 100644 --- a/backend/prisma/schema.prisma +++ b/backend/prisma/schema.prisma @@ -563,3 +563,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/config/index.ts b/backend/src/config/index.ts index 9a01e58f..30fa24eb 100644 --- a/backend/src/config/index.ts +++ b/backend/src/config/index.ts @@ -86,6 +86,31 @@ export const config = { pruneIntervalSeconds: Number(process.env.LOGIN_THROTTLE_PRUNE_INTERVAL_SECONDS ?? 60 * 60), pruneAfterSeconds: Number(process.env.LOGIN_THROTTLE_PRUNE_AFTER_SECONDS ?? 24 * 60 * 60), }, + // `Idempotency-Key` replay protection (Phase 8, issue #48). Env-configurable + // for the same reason as the knobs above: retention and the crash-recovery + // window are deployment decisions, and tests need them short. + idempotency: { + // How long a completed response stays replayable. The issue fixes this at + // 24h: long enough to cover a client that was offline for a day, short + // enough that the table doesn't accumulate a copy of every mutation's + // response body forever. + recordTtlHours: Number(process.env.IDEMPOTENCY_RECORD_TTL_HOURS ?? 24), + // How long an attempt may sit unfinished before another request with the + // same key and the same body may take it over. This exists only to recover + // from a process that died mid-request and left a row with no response — + // it is NOT a "wait this long then assume it worked" timer, because that + // would be a duplicate-write bug. Set it well above the slowest real + // request (an escrow call over the network), never below it. + inProgressTimeoutSeconds: Number(process.env.IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS ?? 300), + // Responses larger than this are executed but not cached: the row is + // dropped instead, so a retry re-runs the request rather than replaying a + // truncated body. Keeps one row from becoming an unbounded blob. + maxCachedBodyBytes: Number(process.env.IDEMPOTENCY_MAX_CACHED_BODY_BYTES ?? 256 * 1024), + // Cleanup job (src/server.ts): how often to drop rows whose TTL has + // passed. Reads always filter on `expiresAt` first, so a missed run is a + // storage cost, not a correctness problem. + pruneIntervalSeconds: Number(process.env.IDEMPOTENCY_PRUNE_INTERVAL_SECONDS ?? 60 * 60), + }, // Patient data export (Phase 8, issue #40). Env-configurable so a // deployment can tune retention and memory bounds without a code change. patientExport: { 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 04ecd694..2b84129b 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 7f80503b..2f41096c 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,13 +24,23 @@ paymentsRouter.get('/wallet-link', authenticate, requireRole('facility'), paymen // #34) — including the admin-only ones, since an admin account that can't // receive mail can't complete the account lifecycle (invites, deactivation // notices) this phase adds either. -paymentsRouter.post('/intents', authenticate, requireRole('admin'), requireVerifiedEmail, paymentsController.createIntent); +// +// The four intent mutations additionally opt into `idempotency()` (Phase 8, +// issue #48), after authentication so the key can be namespaced per user. +// This is the belt to §4.10's braces: a retried `fund`/`release`/`refund` +// replays its first answer instead of pushing a second Stellar transaction +// (the reconcile job that would have to clean that up is issue #24, still +// open). It does not replace the conditional status write in +// `commitTransition` — two *different* keys racing the same transition still +// get exactly one winner and a 409 from the row-level guarantee. +paymentsRouter.post('/intents', authenticate, requireRole('admin'), requireVerifiedEmail, idempotency(), paymentsController.createIntent); paymentsRouter.get('/intents/:id', authenticate, requireRole('admin'), paymentsController.getIntent); paymentsRouter.post( '/intents/:id/fund', authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.fundIntent, ); paymentsRouter.post( @@ -37,6 +48,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.releaseIntent, ); paymentsRouter.post( @@ -44,5 +56,6 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.refundIntent, ); diff --git a/backend/src/server.ts b/backend/src/server.ts index 7030fd12..5edafa81 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -1,6 +1,7 @@ import { createApp } from './app'; import { config } from './config'; import { pruneLoginAttempts } from './modules/auth/services'; +import { pruneIdempotencyKeys } from './middleware/idempotency'; import { startPatientExportJobs } from './jobs/patient-exports'; const app = createApp(); @@ -30,5 +31,23 @@ 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(); 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 5e67678a..ba789d28 100644 --- a/docs/api/README.md +++ b/docs/api/README.md @@ -148,9 +148,102 @@ drift apart. See `docs/adr/0003` for why production needs `SameSite=None`. | Method & path | Auth | Body | Notes | |---|---|---|---| -| `POST /appointments` | patient | `{ availabilitySlotId }` | **The concurrency-critical endpoint** — see `ARCHITECTURE.md` §5.1 and `backend/tests/integration/booking.test.mts`. `409` if the slot is no longer open. | +| `POST /appointments` | patient | `{ availabilitySlotId }` | **The concurrency-critical endpoint** — see `ARCHITECTURE.md` §5.1 and `backend/tests/integration/booking.test.mts`. `409` if the slot is no longer open. Honours `Idempotency-Key` (below). | | `GET /appointments/mine` | patient | — | Caller's own appointments, most recent first. | +## 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 @@ -242,5 +335,7 @@ the cleanup job deletes the stored bundle. Other knobs: `PATIENT_EXPORT_PAGE_SIZ Everything else in `ARCHITECTURE.md` §3's module list (`facilities`, `consultations`, `referrals`, `prescriptions`, `lab-orders`, -`pharmacy-orders`, `payments`, `notifications`) has no routes yet — Phase -2 onward, see `ROADMAP.md`. +`pharmacy-orders`, `notifications`) has no routes yet — Phase +2 onward, see `ROADMAP.md`. (`payments` *does* have routes — see its section +above; Phase 5.5 shipped it ahead of the order modules it will eventually be +triggered by.)