fix(db): enforce tenant ownership in session_pins and billing spend queries (#796)

[rohith500]

Fixes #796.

Summary

Four SQL ownership gaps where tenant isolation was enforced only by the
application layer always passing trusted, same-tenant IDs, not by the SQL
itself. Same pattern as #536 (closed, fixed for
SoftDeleteModelRouterAPIKey), found here on four additional call sites:
session_pins (reads and writes), and three billing queries (key_spend
and user_month_spend in DebitOrgCredits, plus
GetUserMonthlySpendAndLimit). The fourth (user_month_spend) was found
during the fix-design investigation, the same class of gap as the
GetUserMonthlySpendAndLimit read-side finding, just on the write side of
the same query, included here since it's the same pattern and the same
file already being touched for key_spend.

Confirmed in #796 (independently verified twice before filing: an
independent investigation pass, and manual live SQL reproduction on local
Postgres) that none of these are reachable via any current authenticated
/v1/* request. This is a hardening fix, not a response to an active
exploit.

Fix

session_pins: added AND installation_id = @installation_id::uuid to
GetSessionPin, UpdateSessionPinUsage,
IncrementSessionPinUpstreamErrors, and ResetSessionPinUpstreamErrors.
UpsertSessionPin's ON CONFLICT DO UPDATE gained WHERE router.session_pins.installation_id = EXCLUDED.installation_id, so a
mismatched caller's upsert silently no-ops rather than overwriting another
tenant's pin, without needing a PK migration. installation_id threaded
through the sessionpin.Store interface (Get, UpdateUsage,
IncrementUpstreamErrors, ResetUpstreamErrors) and every call site
(loadPin, setForceModelPin, maybeRepinOnRefusal, loop-escalation
handling, recordTurnUsage, maybeEvictPinAfterUpstreamErr,
cyber-refusal re-pin, the feedback pin lookup). SweepExpiredSessionPins
is intentionally untouched, it's global GC by expires_at, not
tenant-scoped.

key_spend and user_month_spend CTEs (DebitOrgCredits): both now join
api_key_id/router_user_id through to
model_router_installations.external_id and confirm it matches the
organization_id being debited, before bumping the key's or user's spend
counter. A mismatched ID silently no-ops that one CTE, the org debit and
ledger row still proceed correctly, same no-op pattern the query already
uses for a missing key or deleted user.

GetUserMonthlySpendAndLimit: the has_override,
override_limit_usd_micros, and spent_usd_micros subqueries now join
through model_router_users to model_router_installations.external_id
and require it to match the organization_id parameter.
org_default_limit_usd_micros is correctly left unjoined, it's
org-scoped only, with no user dimension.

All four: mismatch means a silent miss or no-op (0 spend, no override,
pin treated as not-found), not a new error path, matching how each of
these already handles a missing row today.

Commits

1. test: failing regression tests reproducing all four gaps against real
   Postgres and real repo/service code
2. fix(db): session_pins ownership, SQL + Store interface + every call
   site
3. fix(db): key_spend, user_month_spend, and GetUserMonthlySpendAndLimit
   ownership joins

Testing

• Four permanent regression tests, one per finding, confirmed failing
  against pre-fix code for the right reason (the actual cross-tenant leak
  occurring, not a setup error), then passing after each corresponding
  fix commit.
• go test ./internal/proxy/... ./internal/router/sessionpin/... -count=1
  after the session_pins commit, clean.
• go test ./internal/billing/... ./internal/postgres/... -v -count=1
  after the billing commit, clean.
• go test ./... -count=1, zero regressions anywhere in the repo,
  including after a rebase onto origin/main (which had landed unrelated
  changes touching internal/proxy/service.go and turnloop.go, fix(proxy): bypass engages through installation excluded_models #797/fix: preserve Anthropic context management fidelity #798
  no file overlap with session_pins/billing queries; full suite +
  make check re-run clean post-rebase).
• make check, clean.
• Manual before/after reproduction (real Go code, test data cleaned up
  after):

Finding	Before	After
session_pins	install=A, model overwritten to model-from-B, turn_count=2	install=A, model stays model-from-A, turn_count=1; a Get as B correctly misses
key_spend	org A debited correctly, but B's key spent_usd_micros also bumped to $0.50	org A debited correctly, B's key stays at $0.00
user_month_spend	B's monthly spend row created (count=1) from an org A debit	no row created for B (count=0)
GetUserMonthlySpendAndLimit	org A + B's user id returned B's real spend ($0.42) under org A's limit	returns $0.00 spend, org A's limit unaffected

No migration required

All four fixes are query-level (plus sqlc regeneration); every column
referenced already exists.

Related

#796 (this PR fixes it), #536 (closed, same pattern precedent, different
table).

[greptile-apps]

PR author is not in the allowed authors list.

[devin-ai-integration]

Thank you so much for this, @rohith500 — this is a genuinely valuable hardening fix. You found four real tenant-ownership gaps (the same class as #536), the SQL is clean, the no-op-on-mismatch semantics are exactly right, and the before/after reproduction table in the description is excellent. The production code needed no changes at all.

I've opened #806 as a convention-compliant version so we can land it smoothly, with you credited as co-author. The only thing moved is where the regression tests live — nothing about your logic changed.

What changed and why: the four regression tests were added as a live-Postgres, DB-backed test (internal/postgres/tenant_isolation_796_test.go). Our root AGENTS.md has a hard rule under Tests:

No DB-backed integration tests in internal/. If need real Postgres, docker compose stack is runtime fixture; write scripts under scripts/ rather than *_test.go.

So in #806 those exact four scenarios were ported verbatim (real SessionPinRepo / BillingRepo, no mocks) into scripts/tenant_isolation_check/, gated on ROUTER_TEST_DATABASE_URL, following the existing scripts/feedback_integration_check/ pattern. This keeps go test ./... from ever touching Postgres in CI.

Please don't read this as anything you did wrong — this convention is internal, still evolving, and not at all obvious from outside the repo. Your investigation and the fix itself were spot on, and it's landing because of your work. Thanks again! 🙏