diff --git a/package.json b/package.json index 1c3804dbd..69b4f0c70 100644 --- a/package.json +++ b/package.json @@ -39,9 +39,10 @@ "check:rpc-exists": "node scripts/check-rpc-exists.mjs", "check:one-current-user": "node scripts/check-one-current-user.mjs", "check:client-ip": "node scripts/check-client-ip.mjs", + "check:user-scoped-deletes": "node scripts/check-user-scoped-deletes.mjs", "check:ai-models": "node scripts/check-ai-models.mjs", "check:mdx": "node scripts/check-mdx.mjs", - "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:one-current-user && npm run check:client-ip && npm run check:mdx && npm run test:unit -- --watchAll=false", + "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:one-current-user && npm run check:client-ip && npm run check:user-scoped-deletes && npm run check:mdx && npm run test:unit -- --watchAll=false", "audit:schema": "node scripts/db/audit-schema-drift.mjs", "audit:routes": "node scripts/audit-routes.mjs", "gen:types": "bash scripts/db/gen-types.sh", diff --git a/scripts/check-user-scoped-deletes.mjs b/scripts/check-user-scoped-deletes.mjs new file mode 100644 index 000000000..1ceda3b14 --- /dev/null +++ b/scripts/check-user-scoped-deletes.mjs @@ -0,0 +1,79 @@ +#!/usr/bin/env node +/* eslint-disable no-console */ +/** + * check-user-scoped-deletes.mjs — every DELETE on a per-user Cat table names + * the user. + * + * Cat's memory tables hold one row per user with no other tenancy boundary in + * the query itself. Six deletes in `src/services/cat/memory.ts` remove rows; + * four already filtered on `user_id`, and the two PRUNE paths did not — they + * deleted purely by `id`, on ids fetched moments earlier by a user-scoped + * query. + * + * That was safe, and only conditionally: the ids were right, and the client was + * RLS-scoped. Neither is guaranteed by the code that does the deleting. Three + * other paths in this service already use `getAdminClient()`, where RLS is not + * a backstop at all — hand one of those to a prune and it deletes across users, + * silently, from a function whose job is routine cleanup nobody watches. + * + * The second instance is the reason this gate exists rather than a third fix: + * `recordForgottenFacts` says "same pattern as memory pruning" in its own + * comment, and inherited the gap along with the pattern. bitbaum/orangecat#563 + * finding 13. + * + * Deliberately narrow: it checks the Cat memory service, where the invariant is + * uniform and the tables are strictly per-user. Widening it to every table in + * the app would need a per-table notion of ownership that does not exist yet, + * and a gate that has to guess is a gate that gets muted. + */ + +import { readFileSync } from 'node:fs'; + +const FILES = ['src/services/cat/memory.ts', 'src/services/cat/economic-profile.ts']; + +/** + * A delete and the statement that follows it, up to the terminating `;`. + * Chains here are short and always end in one, so this needs no JS parser. + */ +function deleteStatements(source) { + const found = []; + const re = /\.delete\(\)/g; + let m; + while ((m = re.exec(source)) !== null) { + const end = source.indexOf(';', m.index); + const line = source.slice(0, m.index).split('\n').length; + found.push({ line, text: source.slice(m.index, end === -1 ? source.length : end) }); + } + return found; +} + +let offenders = 0; +let checked = 0; + +for (const file of FILES) { + let source; + try { + source = readFileSync(file, 'utf8'); + } catch { + continue; + } + for (const stmt of deleteStatements(source)) { + checked += 1; + if (!/\.eq\(\s*['"]user_id['"]/.test(stmt.text)) { + offenders += 1; + console.error(`✗ ${file}:${stmt.line} — DELETE does not filter on user_id`); + console.error(` ${stmt.text.replace(/\s+/g, ' ').slice(0, 100)}`); + } + } +} + +if (offenders > 0) { + console.error(''); + console.error(' These tables are strictly per-user, and the delete is the last place'); + console.error(' that can say so. RLS is a backstop, not the rule: three paths in this'); + console.error(' service already run under getAdminClient(), where there is no backstop.'); + console.error(' Add .eq(\'user_id\', userId) — the other deletes in these files all do.'); + process.exit(1); +} + +console.log(`✓ user-scoped deletes: ${checked} delete(s) checked, all filter on user_id`); diff --git a/src/services/cat/memory.ts b/src/services/cat/memory.ts index d4e75f95b..ec27e6e56 100644 --- a/src/services/cat/memory.ts +++ b/src/services/cat/memory.ts @@ -402,7 +402,13 @@ async function recordForgottenFacts( .limit(count - MAX_FORGOTTEN_PER_USER); const ids = (oldest as Array<{ id: string }> | null)?.map(r => r.id) ?? []; if (ids.length > 0) { - await supabase.from(DATABASE_TABLES.CAT_FORGOTTEN_FACTS).delete().in('id', ids); + // Scoped by user_id as well as id, for the same reason pruneIfNeeded is + // — this inherited the pattern, and the gap with it. + await supabase + .from(DATABASE_TABLES.CAT_FORGOTTEN_FACTS) + .delete() + .eq('user_id', userId) + .in('id', ids); } } } catch (err) { @@ -820,7 +826,20 @@ export async function extractAndStoreMemories( } } -/** Keep the corpus bounded: delete the oldest memories beyond the per-user cap. */ +/** + * Keep the corpus bounded: delete the oldest memories beyond the per-user cap. + * + * The delete is scoped by user_id as well as by id. That is redundant today — + * the ids come from a query already filtered to this user, run through an + * RLS-scoped client — and it is exactly the redundancy worth having: this is an + * unconditional DELETE of rows the user never asked to remove, and the only + * thing standing between it and someone else's memories is that both of those + * conditions keep holding. Hand it a service-role client one day, as three + * other paths in this service already use, and RLS stops being the backstop. + * + * Every other delete in this file is written that way, including the + * suppression-lifting one twenty lines above. bitbaum/orangecat#563 finding 13. + */ async function pruneIfNeeded(supabase: AnySupabaseClient, userId: string): Promise { const { count } = await supabase .from(DATABASE_TABLES.CAT_MEMORIES) @@ -837,7 +856,11 @@ async function pruneIfNeeded(supabase: AnySupabaseClient, userId: string): Promi .limit(count - MAX_MEMORIES_PER_USER); const ids = (oldest as Array<{ id: string }> | null)?.map(r => r.id) ?? []; if (ids.length > 0) { - await supabase.from(DATABASE_TABLES.CAT_MEMORIES).delete().in('id', ids); + await supabase + .from(DATABASE_TABLES.CAT_MEMORIES) + .delete() + .eq('user_id', userId) + .in('id', ids); } }