From b1bc454a62596bd907bf34664c8367f00ed993ae Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:53:31 +0200 Subject: [PATCH] fix(cat): the two prune deletes named the row but not its owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both prune paths in the Cat memory service deleted purely by `id`: .from(CAT_MEMORIES).delete().in('id', ids) Safe today, and only conditionally so — the ids come from a query already filtered to this user, run through an RLS-scoped client. Neither condition is stated by the code doing 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. Every other delete in these files already filters on user_id — including the suppression-lifting one twenty lines above pruneIfNeeded. Both prunes now do too. Finding 13 named one of them. The second, in recordForgottenFacts, says "same pattern as memory pruning" in its own comment and had inherited the gap along with the pattern — which is why this adds check:user-scoped-deletes to verify rather than making the same edit a third time later. The gate is deliberately narrow: the Cat memory service, where every table is strictly per-user. Widening it needs a per-table notion of ownership that does not exist yet, and a gate that has to guess is a gate that gets muted. Proven to fail before shipping: reverting either prune exits 1 naming the file and line; the clean tree exits 0 over 6 deletes. bitbaum/orangecat#563 finding 13. Verified latent, not live — the only callers of both prune paths pass the request-scoped client. 73 Cat suites (1031 tests) and type-check green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb --- package.json | 3 +- scripts/check-user-scoped-deletes.mjs | 79 +++++++++++++++++++++++++++ src/services/cat/memory.ts | 29 +++++++++- 3 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 scripts/check-user-scoped-deletes.mjs 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); } }