Skip to content

Add PGlite-backed cross-tenant export isolation test - #232

Merged
grimicorn merged 3 commits into
mainfrom
agent/export-isolation-test
Aug 29, 2026
Merged

Add PGlite-backed cross-tenant export isolation test#232
grimicorn merged 3 commits into
mainfrom
agent/export-isolation-test

Conversation

@grimicorn-agent

Copy link
Copy Markdown
Collaborator

What

Adds a real Postgres (PGlite, in-process) integration test for the account export handler (server/api/account/export.get.ts). It seeds two tenants, each with saved items, a feed, an integration, and settings, then invokes the handler as each tenant and asserts the export returns only that tenant's rows.

Test file: tests/server/api/account/export.get.isolation.test.ts (4 tests).

Why

The existing export.get.test.ts mocks useDb and pins the drizzle clauses (the query-builder layer). That proves the query is built correctly but can't prove it behaves correctly against a real database. This test closes that gap: two tenants share one real Postgres, so cross-tenant isolation is proven at the enforcing (handler) layer by actual row exclusion, not by asserting the query AST.

Each ownership guard in the handler is genuinely exercised — dropping any one fails at least one assertion (verified locally):

  • feeds subquery feeding inArray (saved-items ownership)
  • per-user feeds filter (sources)
  • per-user integrations filter
  • per-user userSettings findFirst (settings theme asserted in both directions, since findFirst has no defined order)

Coverage details:

  • Saved items are asserted as an exact, ordered array — proving the set is precisely the requesting tenant's saved+starred items (no other tenant's, no unsaved item) and exercising the handler's publishedAt DESC NULLS LAST plus desc(id) tiebreaker ordering.
  • Reverse direction (tenant B) also asserts sources/integrations, so a "always returns the first-seeded tenant" bug can't slip through.
  • A false-pass guard counts the other tenant's saved rows in the same DB, so exclusion can only be the ownership filter, never empty data.
  • A 401 test confirms the auth guard runs before any DB query (useDb spy asserted untouched).

Implementation notes

  • Follows the existing PGlite pattern from tests/server/db/sourceCap.test.ts: minimal local DDL for the five tables the export reads, rather than replaying the migration chain (earlier migrations use GIN/tsvector triggers the in-memory harness doesn't reliably support). Column names mirror server/db/schema.ts; a drifted column fails loudly with the column named.
  • Test-only change. The handler is not modified.

Viewable

Not a UI change — runs via npm run test:ci (tests/server/api/account/export.get.isolation.test.ts).

Closes #226

Seeds two tenants into a real in-process Postgres (PGlite) and invokes the
account export handler as each, asserting the export returns only the
requesting tenant's saved items, sources, integrations, and settings. Proves
isolation at the enforcing (handler) layer rather than the query-builder layer.

Closes #226
@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail

Ran the independent reviewer (fresh Opus, diff on stdin) over multiple rounds. The core guarantee was confirmed solid every round — dropping any of the four ownership filters in the handler fails at least one exact-array assertion. Summary of what was flagged and done:

Fixed

  • Replaced mock-style AST assertions with real-DB row exclusion (the whole point of the test).
  • Made the false-pass guard count the other tenant's saved rows honestly (was counting all rows).
  • Made the 401 test assert useDb was never called (auth runs before any query).
  • Seeded + asserted integrations and settings isolation, not just saved items (integrations carry connected-account identity; settings asserted in both directions since findFirst has no defined order and an unscoped lookup returns one arbitrary row).
  • Exact ordered toEqual for saved items (replaced .sort() and a vacuous .every()), and added a second null-published starred item so the handler's desc(id) tiebreaker is genuinely exercised (not just NULLS LAST).
  • Added symmetric reverse-direction (tenant B) assertions for sources/integrations so an "always returns first-seeded tenant" bug can't pass.
  • Switched to a static handler import matching the sibling export.get.test.ts; added afterAll(vi.unstubAllGlobals()).
  • Used the shared SYNC_STATUS.OK constant in the DDL; corrected an integrations.provider value that had been wrongly tied to the feed-source BLUESKY_SOURCE vocabulary (the app writes a plain literal).
  • Tightened comments to match reality (DDL keeps NOT NULL/DEFAULT because fixtures rely on them; account.providerId is a serializer passthrough, not isolation coverage).

Skipped (with reason)

  • eventFor typed as unknown rather than cast to H3Event: matches the sibling export.get.test.ts verbatim, and there is no tsc/vue-tsc gate in CI (lint:ci = prettier + eslint + fallow; test:ci = vitest). Casting would diverge from the established pattern.
  • Generate the DDL via drizzle-kit/api instead of hand-writing: that module isn't resolvable in this repo's drizzle-kit@0.31. Hand-written minimal DDL matches the accepted tests/server/db/sourceCap.test.ts convention; column types match schema.ts and a drifted column fails loudly with the column named.
  • beforeAll + TRUNCATE instead of per-test PGlite boot: the per-test lifecycle matches sourceCap.test.ts; the file runs in ~2s.
  • Token-column-exclusion test: removed as out-of-scope for cross-tenant isolation and already covered by export.get.test.ts; the serializer whitelists non-secret fields, so payload-level assertions are vacuous and query-arg assertions duplicate the sibling test.
  • Extra edge cases (empty export, null-settings tenant, response headers): already covered by export.get.test.ts at the handler layer.

grimicorn and others added 2 commits August 27, 2026 21:16
Address independent review: add an empty-account tenant (proves empty
collections/null settings, no cross-tenant findFirst leak), assert token
columns and their values never reach the export, and harden spy/teardown.
@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Merged main + independent review trail

Merged origin/main into the branch — no conflicts (fast-forward of unrelated files; this PR's single test file was untouched). Verified the PR still has a real, non-dead diff vs origin/main (the isolation test is not on main; the target handler server/api/account/export.get.ts exists) — not superseded. Full unit suite green after merge (155 files, 1890 tests).

Independent Opus code review ran 3 rounds against the diff.

Round 1 → applied:

  • Added a strict assertion that token columns never reach the export (real accessToken/refreshToken/tokenSecret seeded, then asserted absent) — the one leak a mocked useDb can't catch.
  • Added an empty-data tenant test (owns no feeds/items/integrations/settings): proves empty collections + null settings, no cross-tenant findFirst leak.

Round 2 → applied:

  • Fixed the token assertion: the initial expect.not.arrayContaining([...]) form passed on a single-column leak. Now filters each excluded column individually, failing (and naming) any that appears.
  • Moved useDbSpy.mockClear() from the 401 test body into beforeEach so the 'never queried before 401' assertion is order-independent.

Round 3 → applied:

  • Strengthened the secret check with a value-level guard: no seeded token string appears anywhere in the serialized export (catches a token copied into a non-token field, not just its own column).
  • Hardened teardown: client?.close() so a failed beforeEach can't mask the real error.

Skipped, with reasons:

  • eventFor should cast to H3Event / would fail typecheck — no typecheck script and no CI tsc step; eslint on the file is clean; the sibling export.get.test.ts uses the identical { context: { user } } shape. A cast would break the match-existing-patterns convention.
  • Replay real migrations instead of local DDL — deliberate, documented choice mirroring tests/server/db/sourceCap.test.ts; starred is genuinely nullable in schema.ts, so the DDL matches. Replaying the chain reintroduces the GIN/tsvector-trigger instability the author avoided (scope expansion).
  • beforeAll instead of beforeEach for perf — per-test PGlite is the CI-proven pattern in sourceCap.test.ts; boot is cheap.
  • Assign explicit fixture ids for the tiebreaker — reliance on ascending-serial insert order is stable Postgres behavior and is explicitly documented at the fixtures.
  • Comment volume — the comments document non-obvious isolation-test design decisions; tests + lint are green and the reviewer conceded several 'earn their space'. Trimming another author's file risks dropping useful context.

lint (prettier + eslint + fallow audit) clean on the changed file; isolation suite 5/5, full suite 1890/1890.

@grimicorn
grimicorn merged commit 7f1ecbf into main Aug 29, 2026
16 checks passed
@grimicorn
grimicorn deleted the agent/export-isolation-test branch August 29, 2026 01:17
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.

PGlite-backed cross-tenant export isolation test

2 participants