Skip to content

Commit 075b120

Browse files
voidstackloopclaude
andcommitted
Fix cross-user collision in case-migration session fingerprinting
Found via live multi-user testing: case_migrations.source_fingerprint was UNIQUE on its own (org-scoped, not per-user), so two org members who both stage the local-to-shared migration with identical local case content (most commonly zero local cases, the normal state for anyone new to a shared org) silently collided onto the same session. The second caller would inherit the first caller's session -- sometimes already validated or activated -- and their own "Activate shared dataset" button would never appear. Scopes the dedup key to (source_fingerprint, created_by) instead, matching the same idempotent-retry behavior for one user while no longer colliding across different users. Fixed in both the Postgres and in-memory stores, with a migration to update the constraint (and backfill every already-provisioned tenant schema) plus a regression test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 128774d commit 075b120

4 files changed

Lines changed: 208 additions & 4 deletions

File tree

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
-- Fixes a real cross-user bug found via live multi-user testing of the
2+
-- local-to-shared case migration flow: case_migrations.source_fingerprint
3+
-- (sha256 of the caller's local case list) was UNIQUE on its own, with no
4+
-- per-user scoping. Two different members of the same organization who
5+
-- both stage with identical local case content — most commonly, both
6+
-- having zero local cases, the ordinary state for anyone new to a shared
7+
-- org — collide onto the exact same migration session via the store's
8+
-- `ON CONFLICT (source_fingerprint) DO UPDATE`. The second caller silently
9+
-- inherits the first caller's session (possibly already validated or even
10+
-- activated), and their own client never recognizes it as a session they
11+
-- can activate — case-migration.ts's stageLocalCases() only proceeds past
12+
-- "resume" for a session with a preview, and the "Activate shared dataset"
13+
-- button (AuditPrivacy.tsx) only renders for status "validated" — so the
14+
-- second user's own activation silently never becomes available even
15+
-- though the request nominally "succeeded."
16+
--
17+
-- Fix: scope the same idempotency-on-retry behavior (a caller repeating
18+
-- their own start() call with the same content reuses their own session,
19+
-- unchanged) to (source_fingerprint, created_by) instead of
20+
-- source_fingerprint alone. See postgres-case-migration-store.ts's start()
21+
-- and in-memory-case-migration-store.ts's start() for the matching
22+
-- application-side fix.
23+
--
24+
-- Same two-part structure as every migration since 015: Part 1 replaces
25+
-- provision_tenant_clinical_schema so every *future* organization's
26+
-- case_migrations table is created with the corrected constraint; Part 2
27+
-- backfills every already-provisioned tenant schema in place. Every block
28+
-- in Part 1 below is copied verbatim from migration 018's version of this
29+
-- function except the one UNIQUE clause called out inline.
30+
31+
-- --- Part 1: extend provision_tenant_clinical_schema -----------------------
32+
33+
CREATE OR REPLACE FUNCTION provision_tenant_clinical_schema(target_org UUID)
34+
RETURNS TEXT
35+
LANGUAGE plpgsql
36+
SECURITY DEFINER
37+
SET search_path = pg_catalog, public
38+
AS $provision$
39+
DECLARE
40+
schema_name TEXT := 'tenant_' || replace(target_org::text, '-', '');
41+
BEGIN
42+
IF NOT EXISTS (SELECT 1 FROM public.organizations WHERE id = target_org) THEN
43+
RAISE EXCEPTION 'Unknown organization';
44+
END IF;
45+
UPDATE public.organizations SET tenant_schema = schema_name WHERE id = target_org;
46+
EXECUTE format('CREATE SCHEMA IF NOT EXISTS %I', schema_name);
47+
IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'modelforge_runtime') THEN
48+
EXECUTE format('GRANT USAGE ON SCHEMA %I TO modelforge_runtime', schema_name);
49+
EXECUTE format(
50+
'ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO modelforge_runtime',
51+
CURRENT_USER, schema_name
52+
);
53+
END IF;
54+
EXECUTE format(
55+
'CREATE TABLE IF NOT EXISTS %I.patient_cases (
56+
case_id TEXT PRIMARY KEY,
57+
version BIGINT NOT NULL,
58+
data JSONB NOT NULL,
59+
patient_id TEXT NOT NULL,
60+
owner_user_id UUID NOT NULL,
61+
workspace_id TEXT,
62+
department_id TEXT,
63+
assigned_user_ids UUID[] NOT NULL DEFAULT ''{}'',
64+
active_consent_scopes TEXT[] NOT NULL DEFAULT ''{}'',
65+
staged_migration_id UUID,
66+
active BOOLEAN NOT NULL DEFAULT TRUE,
67+
updated_at TIMESTAMPTZ NOT NULL
68+
)', schema_name
69+
);
70+
EXECUTE format(
71+
'CREATE TABLE IF NOT EXISTS %I.case_change_counter (
72+
singleton BOOLEAN PRIMARY KEY DEFAULT TRUE CHECK (singleton),
73+
next_sequence BIGINT NOT NULL DEFAULT 1
74+
)', schema_name
75+
);
76+
EXECUTE format('INSERT INTO %I.case_change_counter (singleton, next_sequence) VALUES (TRUE, 1) ON CONFLICT DO NOTHING', schema_name);
77+
EXECUTE format(
78+
'CREATE TABLE IF NOT EXISTS %I.case_changes (
79+
sequence BIGINT PRIMARY KEY,
80+
kind TEXT NOT NULL CHECK (kind IN (''upsert'', ''delete'')),
81+
case_id TEXT NOT NULL,
82+
version BIGINT NOT NULL,
83+
patient_case JSONB,
84+
resource JSONB NOT NULL,
85+
changed_at TIMESTAMPTZ NOT NULL
86+
)', schema_name
87+
);
88+
EXECUTE format('ALTER TABLE %I.case_changes ADD COLUMN IF NOT EXISTS resource JSONB', schema_name);
89+
EXECUTE format('CREATE INDEX IF NOT EXISTS %I ON %I.case_changes (case_id, sequence DESC)', 'idx_' || schema_name || '_case_changes_case', schema_name);
90+
EXECUTE format(
91+
'CREATE TABLE IF NOT EXISTS %I.case_migrations (
92+
id UUID PRIMARY KEY,
93+
status TEXT NOT NULL CHECK (status IN (''staging'', ''validated'', ''active'', ''rolled-back'')),
94+
source_fingerprint TEXT NOT NULL,
95+
total_items INTEGER NOT NULL,
96+
accepted_items INTEGER NOT NULL DEFAULT 0,
97+
preview JSONB,
98+
created_by UUID NOT NULL,
99+
created_at TIMESTAMPTZ NOT NULL,
100+
updated_at TIMESTAMPTZ NOT NULL,
101+
UNIQUE (source_fingerprint, created_by)
102+
)', schema_name
103+
);
104+
EXECUTE format(
105+
'CREATE TABLE IF NOT EXISTS %I.case_migration_items (
106+
migration_id UUID NOT NULL REFERENCES %I.case_migrations(id) ON DELETE CASCADE,
107+
item_key TEXT NOT NULL,
108+
case_id TEXT NOT NULL,
109+
data JSONB NOT NULL,
110+
data_hash TEXT NOT NULL,
111+
status TEXT NOT NULL CHECK (status IN (''pending'', ''accepted'', ''invalid'', ''collision'')),
112+
errors JSONB NOT NULL DEFAULT ''[]'',
113+
PRIMARY KEY (migration_id, item_key)
114+
)', schema_name, schema_name
115+
);
116+
EXECUTE format(
117+
'CREATE TABLE IF NOT EXISTS %I.chat_sessions (
118+
id TEXT PRIMARY KEY,
119+
version BIGINT NOT NULL,
120+
data JSONB NOT NULL,
121+
owner_user_id UUID NOT NULL,
122+
assigned_user_ids UUID[] NOT NULL DEFAULT ''{}'',
123+
updated_at TIMESTAMPTZ NOT NULL
124+
)', schema_name
125+
);
126+
EXECUTE format(
127+
'CREATE TABLE IF NOT EXISTS %I.chat_session_change_counter (
128+
singleton BOOLEAN PRIMARY KEY DEFAULT TRUE CHECK (singleton),
129+
next_sequence BIGINT NOT NULL DEFAULT 1
130+
)', schema_name
131+
);
132+
EXECUTE format('INSERT INTO %I.chat_session_change_counter (singleton, next_sequence) VALUES (TRUE, 1) ON CONFLICT DO NOTHING', schema_name);
133+
EXECUTE format(
134+
'CREATE TABLE IF NOT EXISTS %I.chat_session_changes (
135+
sequence BIGINT PRIMARY KEY,
136+
kind TEXT NOT NULL CHECK (kind IN (''upsert'', ''delete'')),
137+
session_id TEXT NOT NULL,
138+
version BIGINT NOT NULL,
139+
session_data JSONB,
140+
resource JSONB NOT NULL,
141+
changed_at TIMESTAMPTZ NOT NULL
142+
)', schema_name
143+
);
144+
EXECUTE format('CREATE INDEX IF NOT EXISTS %I ON %I.chat_session_changes (session_id, sequence DESC)', 'idx_' || schema_name || '_chat_session_changes_session', schema_name);
145+
146+
PERFORM provision_tenant_imaging_tables(schema_name);
147+
PERFORM provision_tenant_ai_gateway_tables(schema_name);
148+
149+
RETURN schema_name;
150+
END
151+
$provision$;
152+
153+
REVOKE ALL ON FUNCTION provision_tenant_clinical_schema(UUID) FROM PUBLIC;
154+
155+
-- --- Part 2: backfill every already-provisioned tenant schema --------------
156+
DO $backfill$
157+
DECLARE
158+
org RECORD;
159+
BEGIN
160+
FOR org IN SELECT tenant_schema FROM organizations WHERE tenant_schema IS NOT NULL LOOP
161+
-- "case_migrations_source_fingerprint_key" is Postgres's default
162+
-- autogenerated name for the original inline UNIQUE (source_fingerprint)
163+
-- column constraint declared in migrations 009 through 018.
164+
EXECUTE format('ALTER TABLE %I.case_migrations DROP CONSTRAINT IF EXISTS case_migrations_source_fingerprint_key', org.tenant_schema);
165+
EXECUTE format(
166+
'ALTER TABLE %I.case_migrations ADD CONSTRAINT case_migrations_source_fingerprint_created_by_key UNIQUE (source_fingerprint, created_by)',
167+
org.tenant_schema
168+
);
169+
END LOOP;
170+
END
171+
$backfill$;

‎server/src/store/in-memory-case-migration-store.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { MigrationStateError, type CaseMigrationStore, type TenantCaseMigrationR
77
import type { TenantCaseRepository } from "./case-store.js";
88

99
interface StagedItem { itemKey: string; patientCase: unknown; hash: string; status: "pending" | "accepted" | "invalid" | "collision"; errors: string[] }
10-
interface State { session: MigrationSession; items: Map<string, StagedItem>; activated: Map<string, string> }
10+
interface State { session: MigrationSession; items: Map<string, StagedItem>; activated: Map<string, string>; createdBy: string | undefined }
1111
const digest = (value: unknown): string => createHash("sha256").update(JSON.stringify(value)).digest("hex");
1212

1313
export class InMemoryCaseMigrationStore implements CaseMigrationStore {
@@ -24,11 +24,17 @@ export class InMemoryCaseMigrationStore implements CaseMigrationStore {
2424
const requireState = (id: string): State => { const state = sessions.get(id); if (!state) throw Object.assign(new Error("Migration not found."), { statusCode: 404 }); return state; };
2525
return {
2626
start: async (input, actor) => {
27-
const existing = [...sessions.values()].find((state) => state.session.sourceFingerprint === input.sourceFingerprint && state.session.status !== "rolled-back");
27+
// Scoped by creator as well as content — see the Postgres
28+
// store's identical fix and migrations/022_case_migration_fingerprint_scope.sql
29+
// for why sourceFingerprint alone is not a safe dedup key
30+
// across different users.
31+
const existing = [...sessions.values()].find(
32+
(state) => state.session.sourceFingerprint === input.sourceFingerprint && state.createdBy === actor.userId && state.session.status !== "rolled-back"
33+
);
2834
if (existing) return existing.session;
2935
const now = new Date().toISOString();
3036
const session: MigrationSession = { id: randomUUID(), organizationId: context.organizationId, status: "staging", sourceFingerprint: input.sourceFingerprint, totalItems: input.totalItems, acceptedItems: 0, createdAt: now, updatedAt: now };
31-
sessions.set(session.id, { session, items: new Map(), activated: new Map() });
37+
sessions.set(session.id, { session, items: new Map(), activated: new Map(), createdBy: actor.userId });
3238
await this.audit.record({ organizationId: context.organizationId, actorUserId: actor.userId, actorExternalSubject: actor.externalSubject, action: "caseMigration.start", targetType: "caseMigration", targetId: session.id });
3339
return session;
3440
},

‎server/src/store/postgres-case-migration-store.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,24 @@ describe.skipIf(!DATABASE_URL)("PostgresCaseMigrationStore (integration — requ
7878
expect(rows.rows[0].n).toBe(1);
7979
});
8080

81+
it("start() scopes the fingerprint dedup key per user — two org members with identical local case content (e.g. both empty) get distinct sessions, not the same one", async () => {
82+
const otherActor: AuditActor = { externalSubject: "idp|test-actor-2", userId: randomUUID() };
83+
const mine = await repo.start({ sourceFingerprint: "identical-content", totalItems: 0 }, ACTOR);
84+
const theirs = await repo.start({ sourceFingerprint: "identical-content", totalItems: 0 }, otherActor);
85+
expect(theirs.id).not.toBe(mine.id);
86+
87+
// Regression coverage for the real bug: activating MY session must
88+
// never affect the OTHER actor's still-independent session — before
89+
// this fix, they were the exact same row, so this validate() would
90+
// have failed with "cannot be validated in its current state" once
91+
// the shared session moved to "active".
92+
await repo.validate(mine.id, ACTOR);
93+
const activated = await repo.activate(mine.id, ACTOR);
94+
expect(activated.status).toBe("active");
95+
const theirsStillStaging = await repo.get(theirs.id);
96+
expect(theirsStillStaging?.status).toBe("staging");
97+
});
98+
8199
it("upload() is idempotent per item key — replaying the same batch does not duplicate items, but reusing a key with different data is rejected", async () => {
82100
const session = await repo.start({ sourceFingerprint: "fp-upload", totalItems: 1 }, ACTOR);
83101
const original = patientCaseFixture("case-1");

‎server/src/store/postgres-case-migration-store.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,16 @@ export class PostgresCaseMigrationStore implements CaseMigrationStore {
2929
private async begin(client: PoolClient, context: TenantContext): Promise<void> { await client.query("BEGIN"); await client.query("SELECT set_config('app.tenant_id',$1,true)",[context.organizationId]); }
3030
private async get(context: TenantContext, schema: string, id: string): Promise<MigrationSession | null> { const result = await this.pool.query<SessionRow>(`SELECT * FROM ${schema}.case_migrations WHERE id=$1`,[id]); return result.rows[0] ? mapSession(result.rows[0],context.organizationId) : null; }
3131
private async start(context: TenantContext, schema: string, input: { sourceFingerprint: string; totalItems: number }, actor: AuditActor): Promise<MigrationSession> {
32-
const client=await this.pool.connect(); try { await this.begin(client,context); const now=new Date(); const result=await client.query<SessionRow>(`INSERT INTO ${schema}.case_migrations (id,status,source_fingerprint,total_items,accepted_items,created_by,created_at,updated_at) VALUES ($1,'staging',$2,$3,0,$4,$5,$5) ON CONFLICT (source_fingerprint) DO UPDATE SET source_fingerprint=EXCLUDED.source_fingerprint RETURNING *`,[randomUUID(),input.sourceFingerprint,input.totalItems,actor.userId,now]); await insertAuditEntry(client,{organizationId:context.organizationId,actorUserId:actor.userId,actorExternalSubject:actor.externalSubject,action:"caseMigration.start",targetType:"caseMigration",targetId:result.rows[0].id}); await client.query("COMMIT"); return mapSession(result.rows[0],context.organizationId); } catch(e){await client.query("ROLLBACK");throw e;} finally{client.release();}
32+
// The conflict target is (source_fingerprint, created_by), not
33+
// source_fingerprint alone — see migrations/022_case_migration_fingerprint_scope.sql.
34+
// A bare source_fingerprint key means any two org members (or the
35+
// same user across two separate attempts, since sha256("[]") is
36+
// identical every time for someone with zero local cases) who stage
37+
// with identical local case content collide onto the exact same
38+
// session, silently handing the second caller a session — possibly
39+
// already validated or activated — that they never started and that
40+
// their own client's Activate button won't recognize as theirs.
41+
const client=await this.pool.connect(); try { await this.begin(client,context); const now=new Date(); const result=await client.query<SessionRow>(`INSERT INTO ${schema}.case_migrations (id,status,source_fingerprint,total_items,accepted_items,created_by,created_at,updated_at) VALUES ($1,'staging',$2,$3,0,$4,$5,$5) ON CONFLICT (source_fingerprint, created_by) DO UPDATE SET source_fingerprint=EXCLUDED.source_fingerprint RETURNING *`,[randomUUID(),input.sourceFingerprint,input.totalItems,actor.userId,now]); await insertAuditEntry(client,{organizationId:context.organizationId,actorUserId:actor.userId,actorExternalSubject:actor.externalSubject,action:"caseMigration.start",targetType:"caseMigration",targetId:result.rows[0].id}); await client.query("COMMIT"); return mapSession(result.rows[0],context.organizationId); } catch(e){await client.query("ROLLBACK");throw e;} finally{client.release();}
3342
}
3443
private async upload(context: TenantContext, schema: string, id: string, items: { itemKey:string;patientCase:unknown }[], actor: AuditActor): Promise<MigrationSession> {
3544
const client=await this.pool.connect(); try { await this.begin(client,context); const session=await client.query<SessionRow>(`SELECT * FROM ${schema}.case_migrations WHERE id=$1 FOR UPDATE`,[id]); if(!session.rows[0]) throw Object.assign(new Error("Migration not found."),{statusCode:404}); if(session.rows[0].status!=="staging") throw new MigrationStateError("Migration is not accepting batches.");

0 commit comments

Comments
 (0)