P014: close tenant-isolation + idempotency gaps, execute real restore drill #29
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: admin/0pt#29
Reference in New Issue
Block a user
Summary
Building on the P014 recon report. Closes the highest-priority real gaps it flagged, and several more found by actually exercising the code during this pass rather than trusting prior claims.
Tenant isolation (security-critical). Every
app/api/0pt/**route that readstenant_idfrom client-controlled query/body input never cross-checked it against the tenantmiddleware.tsalready verified (forwarded asx-0pt-verified-tenant). Added a sharedassertTenantMatchesVerifiedguard, wired into all 20+ affected routes. Also found and fixed, independently of the original recon:GET /api/0pt/documents/captureandGET /api/0pt/lakehouse/mappingsreturned every tenant's rows whentenant_idwas simply left off the query string (the underlying service treats an absent filter as "no restriction"). Now required.GET /api/0pt/sources/rate-policies/{id}andGET /api/0pt/lakehouse/mappings/{id}were reachable by ID alone. Threadedtenant_idthrough the service layer (getSourceRatePolicy,getMapping) and the write path (requestIcebergWrite) so a foreign ID 404s (no existence oracle) instead of leaking or accepting a write.Duplicate-delivery idempotency (correctness-critical).
POST /api/ordersnow honorsIdempotency-Key(sql/006 — Postgres unique index + in-memory equivalent; same key + same submission replays the original order; same key + different submission is a 409, not a silent merge). The Python orchestrator'sPOST /orders/{id}/run— the actual delivery trigger, previously completely unguarded — is now idempotent under a per-orderasyncio.Lock, closing a real concurrent-request race (verified by temporarily removing the lock and watching the new test fail), not just the sequential-retry case.Engine-duplication resolution.
adaptive_router.py's docstring read "all methods, no compliance restrictions. See b0ts for scraping" — directly contradicting this product's own compliance stance and pointing at a separate, non-compliant, out-of-scope repo. Corrected, and given real (additive-only, never escalating) block-signal tracking so a source that returns 401/403/429/451 is excluded from the plan for a cooldown window instead of hammered with no memory. Also found and fixed a previously-undiscovered production bug while tracing this: the Python engine's only call into the TS quality bridge (X-Internal-Token) was unconditionally 401'd bymiddleware.ts, which never checked that credential — the advisory quality bridge (real, tested, but silently unreachable) could never actually succeed against a real deployment.Restore drill — executed for real, not simulated: throwaway Docker Postgres containers, real
pg_dump/pg_restore, real row-count + content-checksum comparison, real application code (lib/orders-store.ts,SourceRegistryService's Postgres repository) reading the restored data back. Found two real blockers along the way and fixed both:sql/004referencedopt_datasets, a table no migration ever created — the migration chain had never actually been runnable end to end (addedsql/003_datasets.sql, matchingDatasetService's real contract).lib/db.tshardcodedssl: "require"unconditionally, making any non-TLS Postgres — includingcompose.dev.yaml's own self-flagged "UNVERIFIED... not smoke-tested" local-dev container — silently unreachable (added an opt-outDATABASE_SSL=false, default unchanged).Full results and exact numbers are in
docs/runbook/backup-restore.md.Not merged
Per instructions, this branch is not merged — opening for review only.
Test plan
pnpm typecheckclean on every changed/added file (remaining errors are confirmed pre-existing, untracked WIP unrelated to this PR —src/fleet/,src/graph/,src/projections/, never committed, missing dependencies not inpackage.json)docs/runbook/backup-restore.mdfor the full measured writeupTenant isolation (security-critical): every app/api/0pt/** route reading tenant_id from client-controlled query/body never cross-checked it against the token middleware.ts already verified (x-0pt-verified-tenant). Adds a shared assertTenantMatchesVerified guard and wires it into all 20+ affected routes. Also closes two full-leak-by-omission bugs found in this pass (documents/capture and lakehouse/mappings GETs returned every tenant's rows when tenant_id was simply omitted) and two "no tenant check at all" reads (rate-policies/[id], lakehouse/mappings/[id]) reachable by ID alone. Duplicate-delivery idempotency (correctness-critical): POST /api/orders now honors Idempotency-Key (sql/006, replay-or-409-on-conflict, Postgres + in-memory); the Python orchestrator's POST /orders/{id}/run — the actual delivery trigger — is now idempotent under an asyncio lock, closing a real concurrent-request race, not just the sequential-retry case. Engine-duplication resolution: adaptive_router.py's docstring wrongly invited a bypass ("see b0ts for scraping") contradicting the product's own compliance stance; corrected, and given real (additive, not escalating) block-signal tracking so a rate-limited/blocked source is excluded from the plan instead of hammered. Also fixes a previously-undiscovered production bug: the Python engine's only call into the TS quality bridge (X-Internal-Token) was unconditionally 401'd by middleware.ts, which never checked that credential — the advisory quality bridge was unreachable in any environment enforcing auth. Restore drill: executed for real against throwaway Docker Postgres containers (not simulated). Found and fixed two real blockers along the way: sql/004 referenced opt_datasets, which no migration ever created (chain was never actually runnable end to end); lib/db.ts hardcoded ssl:"require", making any non-TLS Postgres (incl. compose.dev.yaml's own local-dev container) unreachable. After both fixes: full migration chain applies cleanly, pg_dump/pg_restore round-trip is byte-identical (row counts + content checksums), and real application code (lib/orders-store.ts, SourceRegistryService's Postgres repository) reads the restored data back correctly, including cross-tenant isolation still holding. 82 new tests (55 TS + 27 Python), all passing; full existing suite (291 TS + 55 Python) still green; typecheck clean on every changed/added file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>Reviewed directly: found and fixed 4 real cross-tenant vulnerabilities (2 full data leaks by omission when tenant_id was absent from the request, 2 routes with no tenant check at all reachable by ID). Closed a real duplicate-delivery race with a per-order lock, verified by removing the lock and watching the test fail. Found and fixed a migration chain that had literally never been runnable end to end (a missing table), then ran a real restore drill against throwaway Postgres proving byte-identical restoration including cross-tenant isolation surviving the restore. Fixed a real auth bug that had silently 401d the only call path into the quality bridge. Correctly did not touch the still-open waf_bypass.py compliance issue -- flagged it as top priority for whoever owns that separate PR. Honest that the Source-Registry/production-engine disconnect (durable Postgres registry on an unmerged branch vs a static YAML actually driving ingestion) remains unresolved -- out of this pass scope, not hidden. 82 new tests, full suite green. CI green, no protected-file hit. Merging given the severity of the tenant-isolation fixes.