diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b9421fef..f9c5b4ab 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -182,11 +182,24 @@ DoctorSpecialty (join table) id, doctor_profile_id, specialty_id FacilityProfile (1:1 User) - id, user_id, name, type (enum: lab|pharmacy), capabilities: FacilityCapability[] + id, user_id, name, type (enum: lab|pharmacy), version (int, default 1 — issue #49, §5.4) + capabilities: FacilityCapability[] FacilityCapability id, facility_profile_id, capability_type (enum: lab_test|medication_dispense), detail (e.g. test category) + version (int, default 1 — issue #49, §5.4) ``` +**`version` columns (Phase 8, issue #49):** added by +`backend/prisma/migrations/20261019090000_add_optimistic_concurrency/` as +`INTEGER NOT NULL DEFAULT 1` on exactly the mutable records above plus +`Appointment` (§4.2) and `PaymentIntent`. They are the storage half of the +`ETag`/`If-Match` convention in §5.4 and carry no meaning on their own — +nothing may treat a higher `version` as "more correct", and no code may +compare-then-write instead of guarding the UPDATE itself. Tables that are +append-only (`Prescription`, `LabOrder`) or immutable +(`Specialty`, `WalletLink`, `AuditEvent`, the export tables) deliberately do +**not** have one; the integration test asserts that set so a later change +can't quietly widen it. ### 4.2 Availability & Appointments ``` @@ -194,12 +207,16 @@ AvailabilitySlot id, doctor_profile_id, start_time, end_time, status (enum: open|held|booked) # Uniqueness/exclusion constraint at DB level on (doctor_profile_id, start_time, end_time) # to make double-booking a constraint violation, not an app-logic bug. + # No `version`: a slot is only ever moved by the conditional claim in §5.1, + not edited. Appointment id, patient_profile_id, doctor_profile_id, availability_slot_id (FK, unique, NULLABLE) intake_id (FK -> IntakeForm — still not a column, Phase 2 issue #1/#2) status (enum: scheduled|completed|cancelled|no_show) origin (enum: direct_booking|referral), referral_id (nullable FK -> Referral) + version (int, default 1 — issue #49, §5.4; no If-Match-guarded route exists + for appointments yet, but every future status write must move it) created_at, updated_at # availability_slot_id is nullable (Phase 3): a cancelled appointment # releases its slot back to `open` and drops the unique link, so the @@ -598,6 +615,48 @@ T5, enforced as a curated config policy. 4. Facility updates its own `FulfillmentRecord` status; for labs, attaches `LabResult`. 5. Order's own `status` field is updated to `fulfilled` only via this facility-confirmed path — patient/doctor cannot self-mark it. +### 5.4 Mutable-record optimistic concurrency (Phase 8, issue #49) +Slot booking (§5.1) is protected by a conditional update, but nothing else was: a +doctor editing their profile in one tab and an admin in another, or two tabs +updating the same appointment, silently overwrote each other — last write wins, +and the loser never finds out. This is the convention that closes that whole +class of bug, and **new mutable-resource routes must follow it.** + +1. An individual mutable-resource read returns a strong `ETag` carrying the row's + integer `version`, e.g. `ETag: "4"` (also mirrored as `version` in the JSON + body). Collection reads deliberately do not get one: a client fetches the + individual resource before editing. +2. `PATCH`/`PUT` sends that exact value back in `If-Match`. Missing header → + `428 Precondition Required` (`code: 'IF_MATCH_REQUIRED'`); weak validator, a + validator list, or anything else that isn't one strong `""` → `400` + (`code: 'INVALID_IF_MATCH'`) rather than a guess at which member was meant; + a version that no longer matches → `412 Precondition Failed` + (`code: 'VERSION_CONFLICT'`). +3. The repository performs **one** conditional update — `UPDATE ... WHERE id = ? + AND version = ?` with `version = version + 1` in the same statement — and + returns the row that statement wrote. A zero-row result is a stale write. + Read-compare-write is forbidden: it reopens the exact race it closes, because + two callers can both read version 4 and both pass a check made before the + write. In Prisma that is `updateManyAndReturn` / `updateMany` with + `count === 1`, never `findUnique` → compare → `update`. +4. Everything else keeps working the way it did. Endpoints whose state + transition is *already* conditional (booking's slot claim, payment + fund/release/refund, doctor verification, token redemption) keep that status + guard and are **not** additionally `If-Match`-gated — two guards on one write + produce two different failure modes and the second one teaches clients + nothing. They do still increment `version`, and still publish the new `ETag`, + so a client can precondition whatever it does next. Every other write to a + versioned row increments it too, including system writes (email verification). +5. Applies to `User`, `PatientProfile`, `DoctorProfile`, `FacilityProfile`, + `FacilityCapability`, `Appointment` and `PaymentIntent` (columns documented in + §4.1/§4.2). Append-only `Prescription`/`LabOrder` are excluded by design — + a correction is a new row (`supersedes_*`), so there is nothing to lose. +6. Shared pieces live in `backend/src/middleware/optimistic-concurrency.ts` + (`requireIfMatch`, `expectedVersion`, `setVersionEtag`, `versionConflict`) so + every module answers with the same status, message and code. Proof: + `backend/tests/integration/optimistic-concurrency.test.mts`, including eight + simultaneous writes on one version where exactly one succeeds. + ## 6. Hard questions (architecture-level — see PRD §8 for product-level) ### 6.1 What do we think would break? @@ -607,6 +666,8 @@ T5, enforced as a curated config policy. 4. JWT refresh rotation without a revocation list — a stolen refresh token has no clean kill switch until this is added. 5. **Discovered 2026-10-15 while building email verification (Phase 8, issue #34):** the email/SMS provider is still undecided (Phase 6, issue #17), so verification links can't actually be delivered in a hosted environment yet — `backend/src/modules/auth/services/email-dispatcher.ts` is a no-op seam until a provider is integrated. Until then `backend/scripts/mark-email-verified.mjs` is the documented out-of-band path for an operator with DB access. Not a reason to skip the guard: the guard is what makes the flow safe once delivery exists, and shipping it locked-off would just mean shipping it untested. ++6. **Resolved 2026-10-19 (issue #49):** silent last-write-wins on every mutable record that wasn't the slot claim — a doctor in one tab and an admin in another, or two tabs editing one appointment, each wrote and the second erased the first with no signal to either. §5.4 now makes the compare and the write one conditional statement behind an `If-Match` precondition. **Still open:** the convention is only *enforced* where a route opts in, so a future mutable route that forgets `requireIfMatch` silently opts out — there is no lint or schema-level way to catch that yet, and ESLint itself is still untracked (issue #28). + 6. **Discovered 2026-10-17 (issue #41):** after `DELETE /patients/me`, the caller's access token (stateless JWT, 15-minute TTL) still passes `authenticate` until it expires — there's no revocation list (item 4 above, issue #18). It can't reach anything: every patient route resolves the caller through `getMyPatientProfile`, which refuses an anonymised profile; booking/payments refuse it via `requireVerifiedEmail` (verification is cleared); refresh refuses `deleted_at`. Closing the window itself is #18's job. Also: Postgres reports `ON DELETE RESTRICT` as SQLSTATE 23001, which Prisma surfaces as an *unknown* request error, not `P2003` — `patients/services` `isForeignKeyRefusal` handles both; `middleware/error-handler.ts` currently maps neither to a 4xx. ### 6.2 What edge cases are we missing (architecture-level)? @@ -615,6 +676,7 @@ T5, enforced as a curated config policy. 3. No schema support yet for a doctor working across multiple facilities/locations (PRD edge case 6) — `AvailabilitySlot` currently assumes one calendar per doctor. 4. ~~No audit-log table~~ **Foundation resolved 2026-10-17 (issue #43):** `AuditEvent` (§4.10) + `audit.record()` in the same transaction as the change, append-only at the DB level, wired into booking (both booking paths), doctor verification, payment fund/release/refund, the patient's referral accept/decline and the referral admin actions (`referral.reassign`, `referral.cancel` — Phase 3, issue #68, written as `entity_type = 'Referral'` rows in the same table). **Still open:** the referral's creation and the cascade-driven `completed`/`expired` transitions (no actor to attribute them to yet), the remaining Phase 4 transitions (prescriptions, lab orders, fulfillment), the admin login-throttle clear (`/auth/login-attempts/clear`) still only logs to the console; the app connects as the database owner, which could disable the triggers — enforcing append-only against the owner needs a separate least-privilege DB role (deployment work, issue #22). PRD §7 auditability is therefore "a real mechanism exists" rather than "auditability is done"; remaining coverage stays an open gap. 5. CSRF on the cookie-authenticated routes — the refresh cookie has to be `SameSite=None; Secure` (the Pages frontend is a different site — see `docs/adr/0003`), which means any site the patient visits can make their browser send it to `/auth/refresh` and `/auth/logout`. **Resolved 2026-10-15 (issue #37):** a per-session CSRF token is bound into the refresh token and required in the `X-CSRF-Token` header, the `Origin` header is validated against an explicit allowlist, and CORS is now a per-environment allowlist (`CORS_ORIGINS`) instead of one configured string. The double-submit cookie pattern — the usual answer — is *not* usable here: a cookie set by this API is a third-party cookie to `docforum.github.io`, so the frontend's JavaScript can never read it; the token travels in the login/refresh response body instead. A request with no `Origin` (curl, server-to-server) is allowed through the Origin check but still needs the token. See `backend/src/middleware/csrf.ts`, and `docs/api/README.md` for what the client must send. Consequence: refresh cookies issued before this change carry no `csrf` claim and are rejected, i.e. existing sessions need one re-login. +6. **Added 2026-10-19 (issue #48):** an `IdempotencyKey` row caches a *copy of a response body* for 24 hours (§4.12). Today every opted-in route returns non-PHI data (an appointment id and status, a payment intent), so this is transport bookkeeping and not a patient-data surface — it is not part of the patient-export bundle (§3.1) and no endpoint lists it. **The condition that would change that:** if a route returning clinical free text (consultation notes, intake payloads, lab results) ever opts in, each row becomes a temporary PHI copy subject to right-of-access and retention, and the export registry in §3.1 has to gain a section for it before that route is allowed to opt in. Until then, adding a route to the opted-in list is a deliberate decision, not a default. 6. **Discovered 2026-10-24 (issue #16):** lab result files have two read gaps. (a) The right-of-access export (§4.11) lists the `LabResult` row, but the file bytes are not in the bundle; the patient downloads them separately through `GET /orders/lab-orders/:labOrderId/result`. (b) Read access mirrors consultation notes exactly: the patient's own account, the ordering doctor, and the fulfilling facility. So an active guardian of a dependent (ADR 0011) can't read a dependent's result, and neither can a doctor further along the referral thread. Widening either needs a deliberate decision, not a quiet one. @@ -629,6 +691,7 @@ T5, enforced as a curated config policy. - **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 17444042..a35b4231 100644 --- a/ARCHITECTURE_ESSENTIALS.md +++ b/ARCHITECTURE_ESSENTIALS.md @@ -19,6 +19,7 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc 4. **Cross-module access only through a module's service interface.** Never import another module's `repositories/*` directly. 5. **Facilities write to their own `FulfillmentRecord` only** — never directly to `Prescription.status` / `LabOrder.status`. 6. Every module = `routes/ controllers/ services/ repositories/`. Keep that shape. +7. **Mutable-record edits use optimistic concurrency (Phase 8, issue #49).** Individual-resource read returns a strong version `ETag` (`"4"`); PATCH/PUT must send it in `If-Match` — missing is `428`, weak/list/garbage is `400`, stale is `412`. Write it as ONE conditional `UPDATE ... WHERE id = ? AND version = ?` that increments `version` (Prisma: `updateManyAndReturn`); read-compare-write is forbidden. Status transitions that are *already* conditional (booking, payment status, verification) keep their own guard, don't get a second `If-Match` one, but still bump + republish the version. Append-only `Prescription`/`LabOrder` are excluded. See ARCHITECTURE.md §5.4. ## Data model map (see ARCHITECTURE.md §4 for full field lists) `User` (including `locale`, default `en`, Phase 9 #133) → `PatientProfile | DoctorProfile | FacilityProfile`; `User` → `AdminInvite` (Phase 8, issue #39 — hashed/expiring/single-use; `User.deactivatedAt` soft-deactivates an account, refusing login/refresh/access tokens without deleting anything) @@ -37,6 +38,8 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc Account deletion (Phase 8, issue #41 — docs/adr/0010): anonymise in place (`User.deletedAt`, `PatientProfile.anonymizedAt`), never cascade. `PatientProfile.userId` is ON DELETE RESTRICT; new patient-linked clinical tables must be RESTRICT too **and** register a retention hold in `src/retention-holds.ts`. `FacilityProfile` → `WalletLink` (payout address) ; `PatientProfile` → `PatientWalletLink` (payer address, ADR 0005) ; `Prescription` or `LabOrder` → exactly one typed `PaymentIntent` FK (issue #85), whose `amount` is in the smallest unit of a per-environment allowlisted `assetCode` (issue #79) +Mutable records above (plus `Appointment` and `PaymentIntent`) carry a `version` int for the `ETag`/`If-Match` convention — hard rule 7, ARCHITECTURE.md §5.4. Mutable profile fields a new route exposes need a conditional write, not just a `.update()`. + ## Payments (Phase 5.5 — see docs/adr/0004-custodial-payments-v1.md) - **Custodial v1**: `docforum-core` holds its own Stellar payer + releaser identities (`PLATFORM_PAYER_SECRET`/`PLATFORM_RELEASER_SECRET` env vars). Patients never hold or sign with a wallet — the only reason this is possible without violating `docforum-web`'s "no direct Stellar calls" hard rule. - `WalletLink` is scoped to the **facility** (the on-chain payee), not the patient. diff --git a/ROADMAP.md b/ROADMAP.md index 9035f6dd..4bb098af 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -121,7 +121,7 @@ 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] Admin success-metrics endpoint for available booking lead-time and no-show measures; unsupported PRD metrics are explicitly marked unavailable. Definitions and suppression policy: `docs/success-metrics.md`. Tracked as [issue #124](https://github.com/DocForum/docforum-core/issues/124). diff --git a/backend/.env.example b/backend/.env.example index 8b2ba53d..b995501f 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -35,6 +35,11 @@ LOGIN_THROTTLE_PRUNE_AFTER_SECONDS=86400 # than this many days are deleted by the prune job in src/server.ts. ACCESS_LOG_RETENTION_DAYS=365 +# Idempotent retries (Phase 8, issue #48). Defaults are in src/config/index.ts. +IDEMPOTENCY_RECORD_TTL_HOURS=24 +IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS=30 +IDEMPOTENCY_MAX_CACHED_BODY_BYTES=1048576 + # Patient data export (Phase 8, issue #40). Defaults are in src/config/index.ts. PATIENT_EXPORT_TTL_HOURS=24 PATIENT_EXPORT_PAGE_SIZE=200 diff --git a/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql b/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql new file mode 100644 index 00000000..13f0dba6 --- /dev/null +++ b/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql @@ -0,0 +1,31 @@ +-- Optimistic concurrency for mutable records (Phase 8, issue #49). +-- +-- One integer per row, defaulting to 1 so existing rows are valid at their +-- current state without a backfill. It is the storage half of the ETag/If-Match +-- convention in ARCHITECTURE.md §5.4 and is meaningless on its own: nothing may +-- compare-then-write, the compare has to live in the UPDATE's WHERE clause. +-- +-- Deliberately NOT added to Prescription/LabOrder (append-only — a correction is +-- a new row, so there is no update to lose) or to immutable tables +-- (AvailabilitySlot, Specialty, WalletLink, AuditEvent, the export tables). + +-- AlterTable +ALTER TABLE "User" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "PatientProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "DoctorProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "FacilityProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "FacilityCapability" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "Appointment" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "PaymentIntent" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; \ No newline at end of file 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 89501909..5bf344ca 100644 --- a/backend/prisma/schema.prisma +++ b/backend/prisma/schema.prisma @@ -131,6 +131,7 @@ enum Urgency { model User { id String @id @default(uuid()) + version Int @default(1) email String @unique phone String? passwordHash String @@ -301,6 +302,7 @@ model RefreshSession { model PatientProfile { id String @id @default(uuid()) + version Int @default(1) // Null for a dependent (Phase 8, issue #42, docs/adr/0011): a // guardian-owned profile with no login of its own. Still unique, so a // login owns at most one profile. A DB CHECK (see the @@ -439,6 +441,7 @@ model Guardianship { model DoctorProfile { id String @id @default(uuid()) + version Int @default(1) userId String @unique fullName String verificationStatus VerificationStatus @default(pending) @@ -489,6 +492,7 @@ model DoctorSpecialty { model FacilityProfile { id String @id @default(uuid()) + version Int @default(1) userId String @unique name String type FacilityType @@ -515,6 +519,7 @@ model FacilityProfile { model FacilityCapability { waitlistEntries WaitlistEntry[] id String @id @default(uuid()) + version Int @default(1) facilityProfileId String capabilityType CapabilityType // e.g. a lab test category, or a medication class — free text on purpose, @@ -578,6 +583,7 @@ model AvailabilitySlot { model Appointment { id String @id @default(uuid()) + version Int @default(1) patientProfileId String doctorProfileId String // Nullable because a CANCELLED appointment releases its slot back to @@ -1057,6 +1063,7 @@ enum PaymentIntentStatus { model PaymentIntent { id String @id @default(uuid()) + version Int @default(1) prescriptionId String? @unique labOrderId String? @unique status PaymentIntentStatus @default(created) @@ -1349,3 +1356,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 5ebd276f..7391cd10 100644 --- a/backend/src/config/index.ts +++ b/backend/src/config/index.ts @@ -266,6 +266,14 @@ export const config = { accessLog: { retentionDays: Number(process.env.ACCESS_LOG_RETENTION_DAYS ?? 365), }, + // Idempotent retries (Phase 8, issue #48). Env-configurable so tests can + // use short windows and deployments can tune retention without a code + // change. + idempotency: { + recordTtlHours: Number(process.env.IDEMPOTENCY_RECORD_TTL_HOURS ?? 24), + inProgressTimeoutSeconds: Number(process.env.IDEMPOTENCY_IN_PROGRESS_TIMEOUT_SECONDS ?? 30), + maxCachedBodyBytes: Number(process.env.IDEMPOTENCY_MAX_CACHED_BODY_BYTES ?? 1024 * 1024), + }, // 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/middleware/optimistic-concurrency.ts b/backend/src/middleware/optimistic-concurrency.ts new file mode 100644 index 00000000..d4929a04 --- /dev/null +++ b/backend/src/middleware/optimistic-concurrency.ts @@ -0,0 +1,84 @@ +// Optimistic concurrency for mutable records (Phase 8, issue #49). +// See ARCHITECTURE.md §5.4 for the convention every new module must follow. +// +// The whole point is that the compare and the write are ONE statement +// (`UPDATE ... WHERE id = ? AND version = ?` + `version = version + 1`). +// A read-compare-write would reopen the exact race this closes: two callers +// can both read version 4, both pass the check, and the second write +// silently destroys the first. +import type { NextFunction, Request, Response } from 'express'; +import { HttpError } from '../http-error'; + +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace + namespace Express { + interface Request { + expectedVersion?: number; + } + } +} + +/** + * A weak validator (`W/"4"`) or a list (`"4", "7"`) is refused rather than + * parsed: this API only ever issues one strong validator per resource, and + * guessing which member of a list the caller meant is how silent overwrites + * come back. + */ +const VERSION_ETAG = /^"([1-9]\d*)"$/; + +/** Strong `ETag` carrying the row's integer version, e.g. `ETag: "4"`. + * Call it on the individual-resource read; collection reads deliberately + * don't get one (§5.4.5). */ +export function setVersionEtag(res: Response, version: number) { + res.set('ETag', `"${version}"`); +} + +/** Rejects a mutable-resource write that doesn't carry the version it read. + * Must run after `authenticate`/`requireRole` so an unauthorised caller gets + * 401/403 rather than being told the resource exists. */ +export function requireIfMatch(req: Request, _res: Response, next: NextFunction) { + const value = req.get('If-Match'); + if (!value) { + throw new HttpError( + 428, + 'If-Match is required. Read the resource first and retry with its ETag in the If-Match header.', + 'IF_MATCH_REQUIRED', + ); + } + + const match = VERSION_ETAG.exec(value.trim()); + const version = match ? Number(match[1]) : Number.NaN; + if (!Number.isSafeInteger(version)) { + throw new HttpError( + 400, + 'If-Match must be the strong version ETag returned by this API, e.g. If-Match: "4".', + 'INVALID_IF_MATCH', + ); + } + + req.expectedVersion = version; + next(); +} + +/** The version the caller claimed. Read by a controller after + * `requireIfMatch` has run; throws rather than defaulting to 0, because a + * missing precondition silently skipping the guard would reintroduce the + * lost-update bug this exists to prevent. */ +export function expectedVersion(req: Request): number { + const version = req.expectedVersion; + if (typeof version !== 'number') { + throw new Error('requireIfMatch must run before expectedVersion().'); + } + return version; +} + +/** The error a repository's zero-row conditional update means: someone else + * moved the record since the caller read it. Shared so every module returns + * the same status, message and code. */ +export function versionConflict() { + return new HttpError( + 412, + 'This record changed since you read it. Fetch it again and reapply your change.', + 'VERSION_CONFLICT', + ); +} \ 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 89627369..394e3872 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/auth/repositories/index.ts b/backend/src/modules/auth/repositories/index.ts index 494a0aed..59981aa5 100644 --- a/backend/src/modules/auth/repositories/index.ts +++ b/backend/src/modules/auth/repositories/index.ts @@ -143,7 +143,7 @@ export async function consumeVerificationToken(tokenId: string, consumedAt: Date } export function markUserEmailVerified(userId: string, verifiedAt: Date) { - return prisma.user.update({ where: { id: userId }, data: { emailVerifiedAt: verifiedAt } }); + return prisma.user.update({ where: { id: userId }, data: { emailVerifiedAt: verifiedAt, version: { increment: 1 } } }); } // --- Login throttling (Phase 8, issue #35) ------------------------------- diff --git a/backend/src/modules/doctors/controllers/index.ts b/backend/src/modules/doctors/controllers/index.ts index b56e0c7c..d4628a89 100644 --- a/backend/src/modules/doctors/controllers/index.ts +++ b/backend/src/modules/doctors/controllers/index.ts @@ -1,13 +1,23 @@ import type { Request, Response } from 'express'; import * as doctorsService from '../services'; -import { publicDoctorListQuerySchema, setDoctorSpecialtyVerificationSchema, setMySpecialtiesSchema, setVerificationStatusSchema } from '../validation'; +import { publicDoctorListQuerySchema, setDoctorSpecialtyVerificationSchema, setMySpecialtiesSchema, setVerificationStatusSchema, updateDoctorProfileSchema } from '../validation'; import { HttpError } from '../../../http-error'; import { getParam } from '../../../request-params'; import { auditActor } from '../../../middleware/request-id'; +import { expectedVersion, setVersionEtag } from '../../../middleware/optimistic-concurrency'; export async function getMe(req: Request, res: Response) { if (!req.user) throw new HttpError(401, 'Authentication required.'); const profile = await doctorsService.getMyDoctorProfile(req.user.id); + setVersionEtag(res, profile.version); + res.status(200).json(profile); +} + +export async function updateMe(req: Request, res: Response) { + if (!req.user) throw new HttpError(401, 'Authentication required.'); + const { fullName } = updateDoctorProfileSchema.parse(req.body); + const profile = await doctorsService.updateMyDoctorProfile(req.user.id, expectedVersion(req), fullName); + setVersionEtag(res, profile.version); res.status(200).json(profile); } diff --git a/backend/src/modules/doctors/repositories/index.ts b/backend/src/modules/doctors/repositories/index.ts index b2cf14c6..8beef3c4 100644 --- a/backend/src/modules/doctors/repositories/index.ts +++ b/backend/src/modules/doctors/repositories/index.ts @@ -128,6 +128,15 @@ export function findDoctorProfileById(id: string, db: DbClient = prisma) { return db.doctorProfile.findUnique({ where: { id } }); } +export function updateDoctorProfile(id: string, version: number, fullName: string) { + return prisma.doctorProfile + .updateManyAndReturn({ + where: { id, version }, + data: { fullName, version: { increment: 1 } }, + }) + .then((rows) => rows[0] ?? null); +} + export function findDoctorSummariesByIds(ids: string[]) { return prisma.doctorProfile.findMany({ where: { id: { in: ids } }, select: { id: true, fullName: true } }); } @@ -146,7 +155,7 @@ export async function lockVerificationStatus( } export function updateVerificationStatus(db: DbClient, id: string, status: VerificationStatus) { - return db.doctorProfile.update({ where: { id }, data: { verificationStatus: status } }); + return db.doctorProfile.update({ where: { id }, data: { verificationStatus: status, version: { increment: 1 } } }); } /** diff --git a/backend/src/modules/doctors/routes/index.ts b/backend/src/modules/doctors/routes/index.ts index b8db553c..e7538a2b 100644 --- a/backend/src/modules/doctors/routes/index.ts +++ b/backend/src/modules/doctors/routes/index.ts @@ -1,11 +1,13 @@ import { Router } from 'express'; import { authenticate, requireRole } from '../../../middleware/auth'; +import { requireIfMatch } from '../../../middleware/optimistic-concurrency'; import * as doctorsController from '../controllers'; export const doctorsRouter = Router(); doctorsRouter.get('/', doctorsController.listPublic); doctorsRouter.get('/me', authenticate, requireRole('doctor'), doctorsController.getMe); +doctorsRouter.patch('/me', authenticate, requireRole('doctor'), requireIfMatch, doctorsController.updateMe); doctorsRouter.put('/me/specialties', authenticate, requireRole('doctor'), doctorsController.setMySpecialties); doctorsRouter.patch( '/:doctorProfileId/verification', diff --git a/backend/src/modules/doctors/services/index.ts b/backend/src/modules/doctors/services/index.ts index fe3765bf..17e771c6 100644 --- a/backend/src/modules/doctors/services/index.ts +++ b/backend/src/modules/doctors/services/index.ts @@ -1,4 +1,5 @@ import { HttpError } from '../../../http-error'; +import { versionConflict } from '../../../middleware/optimistic-concurrency'; import { prisma } from '../../../db/prisma-client'; import * as auditService from '../../audit/services'; import * as doctorsRepo from '../repositories'; @@ -10,6 +11,13 @@ export async function getMyDoctorProfile(userId: string) { return profile; } +export async function updateMyDoctorProfile(userId: string, version: number, fullName: string) { + const current = await getMyDoctorProfile(userId); + const profile = await doctorsRepo.updateDoctorProfile(current.id, version, fullName); + if (!profile) throw versionConflict(); + return profile; +} + export function listSpecialties() { return doctorsRepo.findSpecialties(); } diff --git a/backend/src/modules/doctors/validation.ts b/backend/src/modules/doctors/validation.ts index 7a8718d3..b7859d99 100644 --- a/backend/src/modules/doctors/validation.ts +++ b/backend/src/modules/doctors/validation.ts @@ -15,6 +15,10 @@ export const publicDoctorListQuerySchema = z.object({ cursor: z.string().uuid().optional(), }).strict(); +export const updateDoctorProfileSchema = z.object({ + fullName: z.string().trim().min(1).max(200), +}).strict(); + export const setDoctorSpecialtyVerificationSchema = z.object({ status: z.enum(['pending', 'verified', 'rejected']), reviewer: z.string().min(1), diff --git a/backend/src/modules/payments/controllers/index.ts b/backend/src/modules/payments/controllers/index.ts index 73652a66..181fa064 100644 --- a/backend/src/modules/payments/controllers/index.ts +++ b/backend/src/modules/payments/controllers/index.ts @@ -4,6 +4,7 @@ import { walletLinkSchema, paymentHistoryQuerySchema } from '../validation'; import { HttpError } from '../../../http-error'; import { getParam } from '../../../request-params'; import { auditActor } from '../../../middleware/request-id'; +import { setVersionEtag } from '../../../middleware/optimistic-concurrency'; // BigInt doesn't serialize via JSON.stringify by default — every response // here that carries one converts to a string explicitly. The prepared XDR @@ -22,6 +23,7 @@ function serializeIntent(intent: { escrowId: bigint | null; stellarTxHash: string | null; sorobanContractId: string | null; + version: number; preparedTxHash: string | null; preparedExpiryLedger: number | null; preparedExpiresAt: Date | null; @@ -62,12 +64,14 @@ export async function getOwnWallet(req: Request, res: Response) { export async function getIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.getPaymentIntent(id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } export async function prepareIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const prepared = await paymentsService.preparePaymentIntent(auditActor(req), id); + setVersionEtag(res, prepared.intent.version); res.status(200).json({ ...serializeIntent(prepared.intent), // ADR 0005 step 2: unsigned XDR + the ledger after which the @@ -81,18 +85,21 @@ export async function prepareIntent(req: Request, res: Response) { export async function fundIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.fundPaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } export async function releaseIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.releasePaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } export async function refundIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.refundPaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } diff --git a/backend/src/modules/payments/repositories/index.ts b/backend/src/modules/payments/repositories/index.ts index fb4f1bd3..58516a54 100644 --- a/backend/src/modules/payments/repositories/index.ts +++ b/backend/src/modules/payments/repositories/index.ts @@ -183,7 +183,7 @@ export async function markEscrowed( ) { const result = await tx.paymentIntent.updateMany({ where: { id, status: 'created' }, - data: { status: 'escrowed', ...input }, + data: { status: 'escrowed', ...input, version: { increment: 1 } }, }); return result.count === 1 ? tx.paymentIntent.findUniqueOrThrow({ where: { id } }) : null; } @@ -248,7 +248,10 @@ export async function markStatus( from: PaymentIntentStatus, to: PaymentIntentStatus, ) { - const result = await tx.paymentIntent.updateMany({ where: { id, status: from }, data: { status: to } }); + const result = await tx.paymentIntent.updateMany({ + where: { id, status: from }, + data: { status: to, version: { increment: 1 } }, + }); return result.count === 1 ? tx.paymentIntent.findUniqueOrThrow({ where: { id } }) : null; } diff --git a/backend/src/modules/payments/routes/index.ts b/backend/src/modules/payments/routes/index.ts index 1fb33aa5..e3012e13 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(); @@ -41,6 +42,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.fundIntent, ); paymentsRouter.post( @@ -48,6 +50,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.releaseIntent, ); paymentsRouter.post( @@ -55,6 +58,7 @@ paymentsRouter.post( authenticate, requireRole('admin'), requireVerifiedEmail, + idempotency(), paymentsController.refundIntent, ); diff --git a/backend/src/server.ts b/backend/src/server.ts index ceecc1df..7ca82428 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 { pruneOldAccessLogs } from './modules/audit/services'; import { startPatientExportJobs } from './jobs/patient-exports'; import { startEscrowEventPoller } from './jobs/escrow-events'; @@ -110,6 +111,21 @@ const pruneTimer = setInterval(() => { // eslint-disable-next-line no-console console.error('[jobs] access-log prune failed:', err); }); + + // Idempotency-key retention (Phase 8, issue #48): drop rows whose TTL has + // passed. Correctness does not depend on it, but pruning keeps the table + // bounded. + 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-key prune failed:', err); + }); }, config.loginThrottle.pruneIntervalSeconds * 1000); pruneTimer.unref(); diff --git a/backend/tests/integration/idempotency.test.mts b/backend/tests/integration/idempotency.test.mts new file mode 100644 index 00000000..7c1c7523 --- /dev/null +++ b/backend/tests/integration/idempotency.test.mts @@ -0,0 +1,629 @@ +// 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'; +import { createOrderFixture, createPaymentIntentForFixture } from './order-fixtures.mjs'; + +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 } }); + + 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 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: { ownerType: 'facility', ownerId: facilityId, stellarPublicKey: `G${'B'.repeat(55)}`, verificationMethod: 'sep10', activeOwnerKey: `facility:${facilityId}` } }); + }, 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); + }); + + }); + + 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}`; + // 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 adminOrder = await createOrderFixture(prisma, { patientProfileId, facilityProfileId: facilityId, kind: 'lab_order', amountStroops: 5000n, tag: `cross-user-order-${runId}` }); + const adminIntent = await createPaymentIntentForFixture(prisma, adminOrder, 'created'); + const adminAttempt = await postPaymentAction(adminIntent.id, 'fund', { 'Idempotency-Key': key }); + expect(adminAttempt.status).toBe(200); + expect(JSON.stringify(adminAttempt.body)).not.toContain(booking.body.id); + + const adminRetry = await postPaymentAction(adminIntent.id, 'fund', { 'Idempotency-Key': key }); + expect(adminRetry.status).toBe(200); + 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 order = await createOrderFixture(prisma, { + patientProfileId, + facilityProfileId: facilityId, + kind: 'lab_order', + amountStroops: 5000n, + tag: orderId, + }); + const intent = await createPaymentIntentForFixture(prisma, order, 'created'); + return { id: intent.id, status: intent.status } as { id: string; status: string }; + } + + 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/integration/optimistic-concurrency.test.mts b/backend/tests/integration/optimistic-concurrency.test.mts new file mode 100644 index 00000000..defc681f --- /dev/null +++ b/backend/tests/integration/optimistic-concurrency.test.mts @@ -0,0 +1,359 @@ +// Integration tests for optimistic concurrency on mutable records +// (Phase 8, issue #49; convention documented in ARCHITECTURE.md §5.4). +// +// This is the lost-update proof: two actors read the same record, both +// decide to write, and the API must let exactly one of them through and +// answer the other with 412 instead of silently discarding their change. +// Self-contained, same embedded-Postgres pattern as booking.test.mts, on its +// own port (5443). 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, describe, expect, it } from 'vitest'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import EmbeddedPostgres from 'embedded-postgres'; +import { createOrderFixture, createPaymentIntentForFixture } from './order-fixtures.mjs'; + +const BACKEND_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const PORT = 5443; +const runId = Date.now(); +const TX_HASH = 'd'.repeat(64); + +const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'docforum-concurrency-pg-')); +const pg = new EmbeddedPostgres({ + databaseDir: dataDir, + user: 'docforum', + password: 'docforum', + port: PORT, + persistent: false, +}); +const databaseUrl = `postgresql://docforum:docforum@localhost:${PORT}/docforum_concurrency_test`; + +type App = ReturnType; +type Prisma = typeof import('../../src/db/prisma-client.js').prisma; + +const emailedTokens = new Map(); + +describe('optimistic concurrency (version + If-Match)', () => { + let app: App; + let prisma: Prisma; + let admin: { id: string; token: string }; + let doctor: { userId: string; profileId: string; token: string }; + /** A second doctor, so the admin verification test can't move the version + * the PATCH /doctors/me assertions are built on. */ + let otherDoctor: { userId: string; profileId: string; token: string }; + let patient: { userId: string; profileId: string; token: string }; + let facilityId: string; + let slotId: string; + + const adminAuth = () => ({ Authorization: `Bearer ${admin.token}` }); + const asUser = (d: { token: string }) => ({ Authorization: `Bearer ${d.token}` }); + + 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); + expect((await request(app).post('/auth/verify-email').send({ token: emailedTokens.get(email) })).status).toBe(200); + return { userId: res.body.user.id as string, token: res.body.accessToken as string, email }; + } + + async function createIntent() { + const order = await createOrderFixture(prisma, { + patientProfileId: patient.profileId, + facilityProfileId: facilityId, + kind: 'lab_order', + amountStroops: 5000n, + tag: `occ-${Math.random().toString(36).slice(2)}`, + }); + return createPaymentIntentForFixture(prisma, order, 'created'); + } + + beforeAll(async () => { + await pg.initialise(); + await pg.start(); + await pg.createDatabase('docforum_concurrency_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 = `CCONCPPAYERSENTINEL${runId}`; + process.env.PLATFORM_RELEASER_SECRET = `CCONCPRELEASERSENTINEL${runId}`; + process.env.ESCROW_CONTRACT_ID = 'CTESTCONTRACT'; + process.env.ESCROW_TOKEN_ID = 'CTESTTOKEN'; + + 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'); + 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() { + return { escrowId: 501n, txHash: TX_HASH }; + } + async release() {} + async refund() {} + } + 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', 'cc-doctor'); + doctor = { ...doctorUser, profileId: (await prisma.doctorProfile.findUniqueOrThrow({ where: { userId: doctorUser.userId } })).id }; + const otherDoctorUser = await signup('doctor', 'cc-doctor-other'); + otherDoctor = { + ...otherDoctorUser, + profileId: (await prisma.doctorProfile.findUniqueOrThrow({ where: { userId: otherDoctorUser.userId } })).id, + }; + const patientUser = await signup('patient', 'cc-patient'); + patient = { + ...patientUser, + profileId: (await prisma.patientProfile.findUniqueOrThrow({ where: { userId: patientUser.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(asUser(doctor)) + .send({ date: tomorrow, startHour: 9, endHour: 12, slotLengthMinutes: 30 }); + slotId = ((await request(app).get(`/availability/doctors/${doctor.profileId}/slots`)).body as { id: string }[])[0]!.id; + + 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: 'Concurrency Lab', type: 'lab' } })).id; + await prisma.walletLink.create({ data: { ownerType: 'facility', ownerId: facilityId, stellarPublicKey: `G${'B'.repeat(55)}`, verificationMethod: 'sep10', activeOwnerKey: `facility:${facilityId}` } }); + }, 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 }); + }); + + describe('doctor profile — PATCH /doctors/me', () => { + it('publishes a strong version ETag on the individual resource read', async () => { + const res = await request(app).get('/doctors/me').set(asUser(doctor)); + expect(res.status).toBe(200); + expect(res.body.version).toBe(1); + expect(res.headers.etag).toBe('"1"'); + }); + + it('refuses a write with no If-Match and says how to recover (428)', async () => { + const res = await request(app).patch('/doctors/me').set(asUser(doctor)).send({ fullName: 'No Precondition' }); + expect(res.status).toBe(428); + expect(res.body.code).toBe('IF_MATCH_REQUIRED'); + expect(res.body.message).toMatch(/If-Match/); + // Nothing changed: a refused precondition is not a partial write. + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).fullName).not.toBe( + 'No Precondition', + ); + }); + + it('rejects a weak validator, a list and garbage with 400 rather than guessing', async () => { + for (const value of ['W/"2"', '"2", "3"', '2', 'abc', '"-1"', '"0"']) { + const res = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', value).send({ fullName: 'Bad Tag' }); + expect(res.status, `If-Match: ${value}`).toBe(400); + expect(res.body.code).toBe('INVALID_IF_MATCH'); + } + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe(1); + }); + + it('applies the write and increments the version in the same response', async () => { + const current = await request(app).get('/doctors/me').set(asUser(doctor)); + const res = await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', current.headers.etag) + .send({ fullName: 'Renamed Doctor' }); + expect(res.status).toBe(200); + expect(res.body.fullName).toBe('Renamed Doctor'); + expect(res.body.version).toBe(current.body.version + 1); + expect(res.headers.etag).toBe(`"${res.body.version}"`); + // The database agrees — the increment happened in the guarded UPDATE. + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe( + res.body.version, + ); + }); + + it('rejects a stale write with 412 and leaves the winning value in place', async () => { + const stale = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const first = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', stale).send({ fullName: 'First Writer' }); + expect(first.status).toBe(200); + + const loser = await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', stale) + .send({ fullName: 'Second Writer' }); + expect(loser.status).toBe(412); + expect(loser.body.code).toBe('VERSION_CONFLICT'); + + // The classic lost-update check: the first writer's value survived, and + // the version moved exactly once — not twice. + const stored = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } }); + expect(stored.fullName).toBe('First Writer'); + expect(stored.version).toBe(first.body.version); + + // And re-reading then retrying the loser's intent succeeds. + const retry = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', `"${stored.version}"`).send({ + fullName: 'Second Writer', + }); + expect(retry.status).toBe(200); + expect(retry.body.fullName).toBe('Second Writer'); + }); + + it('lets exactly one of 8 simultaneous writes on the same version through', async () => { + const etag = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const versionBefore = (await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version; + + const responses = await Promise.all( + Array.from({ length: 8 }, (_, i) => + request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', etag).send({ fullName: `Racer ${i}` }), + ), + ); + + expect(responses.filter((r) => r.status === 200)).toHaveLength(1); + expect(responses.filter((r) => r.status === 412)).toHaveLength(7); + for (const loser of responses.filter((r) => r.status === 412)) expect(loser.body.code).toBe('VERSION_CONFLICT'); + + const stored = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } }); + expect(stored.version).toBe(versionBefore + 1); + expect(stored.fullName).toBe(responses.find((r) => r.status === 200)!.body.fullName); + }); + + it('authorises before checking the precondition, and validates the body', async () => { + // No token -> 401, not a hint that this route has a precondition. + expect((await request(app).patch('/doctors/me').send({ fullName: 'Anon' })).status).toBe(401); + // Wrong role -> 403, likewise. + expect((await request(app).patch('/doctors/me').set(asUser(patient)).send({ fullName: 'Patient' })).status).toBe( + 403, + ); + + const etag = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const before = (await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version; + expect( + (await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', etag).send({ fullName: '' })).status, + ).toBe(400); + expect( + ( + await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', etag) + .send({ fullName: 'Sneaky', verificationStatus: 'verified' }) + ).status, + ).toBe(400); + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe(before); + }); + }); + + describe('already-conditional transitions are not given a second guard', () => { + it('books without If-Match — the slot claim is already the guard', async () => { + const res = await request(app) + .post('/appointments') + .set({ Authorization: `Bearer ${patient.token}` }) + .send({ availabilitySlotId: slotId }); + expect(res.status).toBe(201); + expect((await prisma.appointment.findUniqueOrThrow({ where: { id: res.body.id } })).version).toBe(1); + }); + + it('flips doctor verification without If-Match and still bumps the version', async () => { + const before = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: otherDoctor.profileId } }); + const res = await request(app) + .patch(`/doctors/${otherDoctor.profileId}/verification`) + .set(adminAuth()) + .send({ status: 'verified' }); + expect(res.status).toBe(200); + expect(res.body.verificationStatus).toBe('verified'); + expect(res.body.version).toBe(before.version + 1); + }); + + it('funds and releases without If-Match, advancing the version each time', async () => { + const intent = await createIntent(); + expect(intent.version).toBe(1); + + const read = await request(app).get(`/payments/intents/${intent.id}`).set(adminAuth()); + expect(read.status).toBe(200); + expect(read.headers.etag).toBe('"1"'); + expect(read.body.version).toBe(1); + + const funded = await request(app).post(`/payments/intents/${intent.id}/fund`).set(adminAuth()); + expect(funded.status).toBe(200); + expect(funded.body.version).toBe(2); + expect(funded.headers.etag).toBe('"2"'); + + const released = await request(app).post(`/payments/intents/${intent.id}/release`).set(adminAuth()); + expect(released.status).toBe(200); + expect(released.body.version).toBe(3); + + expect((await prisma.paymentIntent.findUniqueOrThrow({ where: { id: intent.id } })).version).toBe(3); + + // The status transition's own conditional write is untouched: a second + // release still loses on status (409), not on the version. + expect((await request(app).post(`/payments/intents/${intent.id}/release`).set(adminAuth())).status).toBe(409); + expect((await prisma.paymentIntent.findUniqueOrThrow({ where: { id: intent.id } })).version).toBe(3); + }); + }); + + describe('schema', () => { + it('puts a version column on the mutable models and nothing else', async () => { + const rows = await prisma.$queryRaw<{ table_name: string }[]>` + SELECT table_name FROM information_schema.columns + WHERE table_schema = 'public' AND column_name = 'version' + ORDER BY table_name + `; + // Append-only Prescription/LabOrder and every immutable record + // (Specialty, WalletLink, AuditEvent, the export tables) are absent by + // design — see ARCHITECTURE.md §5.4.4. + expect(rows.map((r) => r.table_name)).toEqual([ + 'Appointment', + 'DoctorProfile', + 'FacilityCapability', + 'FacilityProfile', + 'PatientProfile', + 'PaymentIntent', + 'User', + ]); + }); + + it('bumps the User version when the address is verified', async () => { + const email = `cc-verify-${runId}@example.com`; + const signupRes = await request(app) + .post('/auth/signup') + .send({ role: 'patient', email, password: 'longenough123', fullName: 'Verify Me' }); + expect(signupRes.status).toBe(200); + const userId = signupRes.body.user.id as string; + expect((await prisma.user.findUniqueOrThrow({ where: { id: userId } })).version).toBe(1); + + expect((await request(app).post('/auth/verify-email').send({ token: emailedTokens.get(email) })).status).toBe(200); + expect((await prisma.user.findUniqueOrThrow({ where: { id: userId } })).version).toBe(2); + }); + }); +}); \ No newline at end of file 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/changelog.d/48-49-idempotency-and-optimistic-concurrency.md b/changelog.d/48-49-idempotency-and-optimistic-concurrency.md new file mode 100644 index 00000000..3fb6f3bf --- /dev/null +++ b/changelog.d/48-49-idempotency-and-optimistic-concurrency.md @@ -0,0 +1,9 @@ +Consolidate idempotency-Key support (issue #48) and optimistic concurrency with +version fields and `If-Match` (issue #49) into one PR. Adds the +`IdempotencyKey` model with a unique `(userId, key)` constraint and the shared +`optimistic-concurrency` middleware (`setVersionEtag`, `requireIfMatch`, +`expectedVersion`, `versionConflict`), plus `version Int @default(1)` on `User`, +`PatientProfile`, `DoctorProfile`, `FacilityProfile`, `FacilityCapability`, +`Appointment` and `PaymentIntent`. `PATCH /doctors/me` is the reference +implementation; system-driven updates (email verification, payment transitions) +also bump the version. Existing status-conflict `409` behavior is preserved.