Skip to content

fix: keep tenant identity out of public write payloads - #2088

Open
richardfogaca wants to merge 2 commits into
mainfrom
fix/tenant-write-boundary
Open

fix: keep tenant identity out of public write payloads#2088
richardfogaca wants to merge 2 commits into
mainfrom
fix/tenant-write-boundary

Conversation

@richardfogaca

@richardfogaca richardfogaca commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Linked issue

Related: #2076

Summary

  • Reject caller-supplied tenant_id at one provider-aware boundary for external create, update, and patch operations across every tenant-owned service.
  • Keep valid public DTOs unchanged while trusted tenant identity stays in request params and ambient database context.
  • Remove root-hook DTO stamping/stripping and stripTenantIdFromMutation; keep database-wrapper ownership materialization and PostgreSQL RLS behavior unchanged.

Why / context

Tenant identity is ambient, not a public write field. The root tenant hook previously mixed those responsibilities by adding tenant_id to create DTOs and silently stripping it from update/patch DTOs. Stamping caused strict public-field validation to reject valid schedule creates, while stripping hid invalid ownership input instead of rejecting it.

The corrected boundary is:

external write → reject tenant_id → validate unchanged DTO → service write → database wrapper materializes/preserves ownership → RLS enforces visibility

Implementation notes

  • Register the same provider-aware tenant_id rejection for create, update, and patch on all tenant-owned services in both SQLite and PostgreSQL modes. Internal provider-less calls remain outside this external boundary.
  • Keep resolved tenant identity in trusted params and ambient context; request hooks no longer stamp or strip ordinary DTOs.
  • Preserve the existing database-wrapper insert materialization and RLS transaction scope. Ordinary updates continue to preserve stored ownership.
  • Keep service-specific public DTO validation intact. Nested domain data is not reinterpreted as tenant identity.
  • Schedules intentionally do not expose update; update rejection was proven on repos.

Validation / test plan

Automated and review evidence

  • Focused tenant/write-boundary tests: 90 passed
  • Full daemon suite: 2,036 passed, 31 skipped
  • Daemon typecheck
  • Biome
  • Multitenancy-boundary check
  • Diff check
  • Focused review of the owned changes

PostgreSQL boundary proof

Using the non-bypass application role with forced RLS:

  • A valid schedule create persisted with tenant_id = default.
  • Schedule create and patch payloads containing tenant_id returned 400, with no write.
  • A repo update containing tenant_id returned 400, while a control request reached update.
  • A cards create containing tenant_id returned 400.
  • A valid patch preserved existing ownership.
  • The created schedule's visibility changed 1 → 0 → 1 when tenant context changed default → attacker-tenant → default.

Fixtures were removed after validation.

Screenshots / demos

Not applicable; this is a backend write-boundary change.

Risks / rollout / rollback

The change is centralized across tenant-owned writes, so the main risk is rejecting a caller that incorrectly sends tenant_id. That rejection is intentional and returns 400. Coverage spans both database modes, representative services and methods, valid controls, stored ownership, and forced-RLS visibility. Rollback is a normal revert of the commit.

Out of scope / follow-ups

  • No schema, migration, UI, client, or database-wrapper semantic changes.
  • The retained DrizzleService.stripTenantMutation is outside this compatibility audit.

@richardfogaca
richardfogaca marked this pull request as ready for review July 29, 2026 20:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mistercrunch

mistercrunch commented Jul 29, 2026

Copy link
Copy Markdown
Member

Wondering why stripTenantIdFromMutation is needed. Seems tenant_id should be ambient and unspecified in most places in the app (?)

Ideally we don't need things like stripTenantIdFromMutation across the codebase. Wondering why tenant_id leaked into the caller (?)

@richardfogaca

richardfogaca commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@mistercrunch Removed stripTenantIdFromMutation because tenant identity is now ambient rather than part of caller payloads.

New write boundary

  1. External tenant-owned create, update, or patch supplies tenant_id → return 400.
  2. Valid DTOs continue unchanged through public validation.
  3. The database wrapper materializes tenant ownership for inserts.
  4. PostgreSQL RLS remains the final isolation boundary.

Validated against PostgreSQL

  • A valid schedule persisted with tenant_id = default.
  • Tenant-bearing schedule, card, and repo writes returned 400 without changing ownership.
  • Forced-RLS visibility remained 1 → 0 → 1 across tenant context changes.

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.

3 participants