Skip to content

Fix cross-tenant id collision clobbering memories/tasks rows - #4

Open
VamikaSinghal wants to merge 7 commits into
mainfrom
fix/cross-tenant-memory-task-id-collision-clean
Open

Fix cross-tenant id collision clobbering memories/tasks rows#4
VamikaSinghal wants to merge 7 commits into
mainfrom
fix/cross-tenant-memory-task-id-collision-clean

Conversation

@VamikaSinghal

@VamikaSinghal VamikaSinghal commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

memories.id and tasks.id are plain TEXT PRIMARY KEY (not composite with user_id), and on the default local/CLI path an id is content-derived or caller-supplied with no user salt. INSERT OR REPLACE resolves conflicts purely on that key, so a second tenant writing the same id silently deletes and replaces the first tenant's row — not a duplicate error, silent cross-tenant data loss. Reachable in shared-DB bucket shard mode (sharding.py), where multiple users share one SQLite file.

Four production write paths were affected, all now guarded the same way save_capture's capture_id_override already guards captures.id — check whether the id belongs to a different user, and only re-salt with user_id in that collision case, so the ordinary non-colliding path keeps ids byte-identical:

  • _save_memory
  • _save_task
  • both loops in rebuild_index_from_vault (re-inserts from vault markdown frontmatter; the vault root is shared across users in bucket mode)

A fifth path, the portable-memory bundle import, was audited and is already safe — its id is derived with user_id baked in.

Anti-resurrection follow-up (88a2e41)

The guard as first written re-salted the id before the tombstone check, so a memory tombstoned under its plain derived id was missed once another tenant took that id: tenant A forgets "m1", tenant B later saves its own memory as "m1", and A's next resync salts away from B's row, finds no tombstone under the salted id, and silently resurrects the forgotten memory. Both the pre- and post-salt id are now checked. This gap was found independently on audit/prod-db-concurrency (343f0ca) and ported here.

Second-order collision: the salt itself could clobber a third tenant (fccfe69)

Re-salting once was not enough. The salted id can itself be owned by a third tenant, so the write would delete their row instead — the same bug one level down. Verified against a real database: tenant C holding stable_id("mem_", "tenant-b:collide_mem") was silently erased when tenant B collided with tenant A and salted into it.

The four inline single-hop guards are replaced by _tenant_unique_id(), which chains deterministically until the id is free — bounded, then raises rather than falling through and clobbering. The first hop is unchanged so ids already written stay stable, and the user_id != ? predicate ignores the caller's own row so repeated saves stay idempotent instead of chaining further and accumulating duplicates.

Forensics for databases already damaged (7ff8aac)

The guards prevent future clobbers but cannot un-lose data a deployment already destroyed, and there was no way to tell whether a given database was ever hit. backend/app/tenant_forensics.py is a read-only scan that answers exactly that:

python -m backend.app.tenant_forensics /path/to/index.sqlite

A clobber only rewrote the memories/tasks row itself — sibling rows carry their own user_id, and memory_events is append-only and was never rewritten, so a row whose user_id disagrees with its memory's owner is physical evidence the memory changed hands. Eight checks, each reported with its own confidence rather than summed into one score. Exits 1 on high-confidence evidence so it can gate a deploy check.

Detection is deliberately not gated on the surviving tenant count: a clobber that took over a tenant's only memory removes them from memories entirely, so that gate would report a total wipe as a harmless single-tenant database. Caught by testing the tool against a database produced by actually running the pre-fix code.

Why not a composite primary key (f57a62a)

The obvious fix — migrating memories.id to PRIMARY KEY(user_id, id) like entities — is wrong and breaks at runtime. SQLite requires an FK's parent columns to be a PRIMARY KEY or UNIQUE, and five single-column child FKs point at memories(id): memory_entities, memory_topics, memory_relations (both source_memory_id and target_memory_id), and memory_vec_map (whose memory_id is itself independently UNIQUE). Verified empirically:

OperationalError: foreign key mismatch - "memory_entities" referencing "memories"

The only escapes are rewriting every child table to carry user_id, or keeping a standalone UNIQUE index on memories.id — which re-imposes the exact global uniqueness the composite key was meant to relax. entities could go composite precisely because nothing FKs to entities(id). So the invariant this schema needs is "memory/task ids are globally unique," enforced on write. Documented at both table definitions so it isn't attempted again.

Test plan

  • Regression tests added (backend/tests/test_sharding.py) in the deployment shape that matters: bucket mode with shard_count=1 routes both tenants into one SQLite file.
    • both tenants' memory and task rows survive a colliding write, each still owning its own content
    • retrieval stays isolated — neither tenant's search surfaces the other's memory
    • the collision salt does not resurrect a forgotten memory
    • a salted id already owned by a third tenant does not clobber them
    • 12 tenants piling onto one id all survive, with distinct ids
    • repeated saves under collision stay idempotent (no duplicate rows)
    • 8 forensics tests, including detection of a total tenant erasure
  • Verified load-bearing, not just green. Reverting storage.py to pristine main leaves a single memories row owned by whichever tenant wrote last and returns an empty search for the other; removing the second tombstone check resurrects the forgotten memory as mem_2fa8523d0a70.
  • Full suite: 2147 passed, 6 skipped, 416 subtests, 2 failed. Both remaining failures were confirmed pre-existing by running them against pristine trace-cortex/main: test_rerank_eval (missing model2vec dependency) and test_macos_ui_quality_contract.

One failure was caused by this change, and is fixed here (90e9ad7)

test_import_diff.py::test_import_diff_route was initially mis-reported as pre-existing. It is not — it passes on pristine main (30/30) and fails with the guard. ImportDiffFastAPIEndpointTests shares one process-wide store across test methods, and two tests seeded the literal id "mem_python" under two different user_ids; unittest runs methods alphabetically, so the second seed clobbered the first. That test only ever passed because of the bug this PR fixes. _seed's memory_id is now parameterized and the second test uses a distinct id. Same root cause and fix as integration/final (638c965).

Merge note: this should land first

audit/prod-db-concurrency also carries these guards plus its own concurrency work, but it is not a peer of this PR and cannot merge to main yet — it is built on the entire taste-exclude feature, which does not exist in main.

this PR audit/prod-db-concurrency
ahead / behind main 5 / 0 30 / 9
taste_excluded refs 0 77 in storage.py, 2 in database.py
taste-exclude commits 0 11 of 30
mergeable to main today yes no — blocked behind the feature

So: land this, then rebase audit/prod-db-concurrency onto updated main (~10 min; conflicts are limited to storage.py and database.py, and since both branches now carry identical anti-resurrection code that region resolves by taking either side — its guard commits become no-ops). The alternative leaves a live cross-tenant data-loss bug in main while this sits blocked behind an unrelated feature.

The anti-resurrection fix has also been ported to the other branches that carried the guards without it (integration/final, audit/prod-security, audit/prod-reliability, audit/prod-backend-arch), so no merge order can reintroduce the resurrection bug.

CI

The 3 failing checks are pre-existing — main fails the identical 3 jobs (Distribution site, Python checks, Security scan). The Python failure is assertIn("hasAppleSignInEntitlement", source) in the macOS UI contract test; the distribution failure is a missing release DMG. This PR introduces no new failures.

memories.id and tasks.id are plain PRIMARY KEY (not composite with
user_id), and on the default local/CLI path ids are content-derived
or caller-supplied with no user salt. INSERT OR REPLACE resolves
conflicts purely on that primary key, so a second tenant writing the
same id silently deletes and replaces the first tenant's row -
exploitable in shared-DB "bucket" deployment mode (sharding.py).

Guard _save_memory, _save_task, and both loops in
rebuild_index_from_vault (which re-inserts from vault markdown
frontmatter, also reachable in bucket mode) the same way
save_capture's capture_id_override already guards captures.id: check
whether the id belongs to a different user first, and only re-salt
with user_id in that collision case, so the ordinary non-colliding
path keeps ids unchanged.
Pins the fix in the deployment shape where it matters: bucket shard mode
with shard_count=1 routes both tenants into one sqlite file. Verified
load-bearing - reverting storage.py to pre-fix leaves a single memories
row owned by whichever tenant wrote last, and the other tenant's search
returns empty.
The cross-tenant salt guard re-derived memory_id BEFORE the tombstone
check, so a memory tombstoned under its plain derived id was missed once
another tenant took that id: tenant A forgets "m1", tenant B later saves
its own memory as "m1", and A's next resync salts away from B's row,
finds no tombstone under the salted id, and silently resurrects the
forgotten memory. Check both the pre- and post-salt id.

Credit: this gap was found independently on audit/prod-db-concurrency
(343f0ca); ported here with a regression test that reproduces the
resurrection (the memory returns as mem_2fa8523d0a70) without the fix.
ImportDiffFastAPIEndpointTests shares one process-wide store across test
methods and two tests seeded the literal id "mem_python" under different
user_ids. That only ever passed because the second write silently
clobbered the first tenant's row - the exact bug this branch fixes. The
guard correctly refuses the overwrite and re-salts, so test_import_diff_route
no longer saw "mem_python". Parameterized _seed's memory_id and gave the
second test a distinct one.

I had previously mis-reported this failure as pre-existing/unrelated;
verified it passes on pristine trace-cortex/main and fails with the guard,
so it was caused by this change.

Credit: same root cause and fix found on integration/final (638c965).
I originally proposed migrating memories.id to a composite
PRIMARY KEY(user_id, id), mirroring entities. That is the wrong fix and
would break at runtime: SQLite requires an FK's parent columns to be PK
or UNIQUE, and five single-column child FKs point at memories(id)
(memory_entities, memory_topics, memory_relations x2, memory_vec_map -
the last independently UNIQUE). Verified empirically:

  OperationalError: foreign key mismatch - "memory_entities" referencing "memories"

entities could go composite only because nothing FKs to entities(id).
Recorded at both table definitions so the migration isn't attempted
again, pointing at the write-side guards that enforce the invariant.
Re-salting ONCE was not enough. The salted id can itself be owned by a
third tenant, and INSERT OR REPLACE would then delete THEIR row - the
same bug one level down. Verified: tenant C holding
stable_id("mem_", "tenant-b:collide_mem") was silently erased when
tenant B collided with tenant A and salted into it.

Replaces the four inline single-hop guards with _tenant_unique_id(),
which chains deterministically until the id is free (bounded, then
raises rather than falling through and clobbering). The first hop is
unchanged, so ids already written stay stable, and the user_id != ?
predicate ignores the caller's own row so repeated saves stay idempotent
instead of chaining further.

Adds three tests: the third-tenant clobber (verified load-bearing), a
12-tenant pile-up on one id, and an idempotency check.
The guards stop future clobbers but cannot un-lose data already
destroyed by a deployment that ran the old code, and there was no way to
tell whether a given database was ever hit.

A clobber only rewrote the memories/tasks row itself. Sibling rows carry
their own user_id and memory_events is append-only and never rewritten,
so a row whose user_id disagrees with its memory's owner is physical
evidence the memory changed hands. Eight checks, each reported with its
own confidence rather than summed.

Detection is deliberately NOT gated on the surviving tenant count: a
clobber that took over a tenant's only memory removes them from
memories entirely, so that gate would report a total wipe as a harmless
single-tenant database (caught by testing against a real pre-fix DB).
Tenant count is instead taken across memories/captures/memory_events.

Exits 1 on high-confidence evidence so it can gate a deploy check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant