security(db): enforce workspace_id on protocol tables#354
Open
glassBead-tc wants to merge 5 commits into
Open
Conversation
Production is live multi-tenant (16 workspaces). Several RLS policies granted unrestricted cross-workspace access to the public/anon roles, and one service_role query path was unscoped: - api_keys: drop api_keys_anon_validate, which let the anon role SELECT every workspace's key metadata (workspace_id, name, prefix, key_hash, created_by_user_id). Key validation already uses the service_role admin client (src/auth/api-key.ts), so anon never needed read access. - protocol_sessions: replace the public/USING(true) policy (mis-named service_role_all) with a real service_role policy plus a workspace-scoped authenticated SELECT. - protocol_audits/history/scope/visas: restrict the public/USING(true) policy to service_role only (server-internal protocol-enforcement state). - handler.ts ulyssesStatus(): scope the "last completed session" lookup to this.workspaceId so one tenant cannot see another tenant's session metadata. Migration is idempotent (DROP POLICY IF EXISTS before each CREATE). Applied to production via apply_migration after a transaction-rollback dry-run against the live schema; lands here so staging picks it up via staging-deploy.yml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses PR review: the previous ulyssesStatus() guard only added the workspace_id filter when this.workspaceId was set, so a handler used before setProject() would fall back to an unscoped, cross-tenant "last session" read. Skip the lookup entirely when no workspace is set (last := null) instead of degrading to a global query. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Defense-in-depth follow-up to the protocol RLS hardening. The protocol- enforcement tables had weak tenant isolation: protocol_sessions.workspace_id was text (mixing real tenant UUIDs, NULLs, and dev project strings like "thoughtbox-staging"), and the child tables (audits/history/scope/visas) had no workspace_id at all, relying solely on the session_id FK chain. - Delete 76 legacy/dev sessions (51 NULL + 25 non-tenant text), keeping the 12 real tenant sessions; children cascade. - Normalize protocol_sessions.workspace_id to uuid + NOT NULL + workspaces FK. - Add workspace_id (uuid, NOT NULL, workspaces FK, indexed) to the four child tables, backfilled from the parent session. - Add a BEFORE INSERT trigger (set_protocol_child_workspace_id) that copies workspace_id from the parent session, so app inserts cannot omit it. This follows the repo's trigger conventions and avoids scattering the assignment across ~13 insert sites. Validated via transaction-rollback dry-run against the live prod schema, then applied via apply_migration; lands here so staging picks it up via staging-deploy.yml. database.types.ts is intentionally not regenerated: the child workspace_id is trigger-managed and unreferenced in code, and the typed client would otherwise treat it as a required Insert field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (fix/protocol-workspace-isolation) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a371a8dcce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+80
to
+82
| IF NEW.workspace_id IS NULL THEN | ||
| SELECT workspace_id INTO NEW.workspace_id | ||
| FROM public.protocol_sessions WHERE id = NEW.session_id; |
There was a problem hiding this comment.
When any insert path supplies workspace_id explicitly, this trigger leaves that value untouched even if it belongs to a different workspace than NEW.session_id; the independent FK to workspaces still passes, so the child row can claim a tenant that disagrees with its parent protocol session. In the contexts I inspected, current code omits the new child column, but regenerated types or future explicit inserts would defeat the intended defense-in-depth tenant identity unless the trigger always assigns the parent's workspace or rejects mismatches.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis migration hardens tenant isolation on the protocol-enforcement tables by deleting legacy/dev sessions, converting
Confidence Score: 5/5Safe to merge; all findings are non-blocking quality improvements with no current incorrect behavior introduced. The migration has already been applied to production via a dry-run-verified apply_migration. The structural changes — type conversion, NOT NULL enforcement, FK constraints, backfill, and trigger — are sound. The two open gaps (INSERT-only trigger and a missing index on protocol_sessions.workspace_id) don't compromise correctness today: the child tables are service_role-only so the trigger mismatch path requires an internal application UPDATE of session_id, and the missing index is a latency concern rather than an isolation failure. The single changed file, supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql, warrants a follow-up to add the protocol_sessions.workspace_id index and extend the trigger to cover session_id updates. Important Files Changed
Entity Relationship Diagram%%{init: {'theme': 'neutral'}}%%
erDiagram
workspaces {
uuid id PK
}
protocol_sessions {
uuid id PK
uuid workspace_id FK "NOT NULL (was text)"
text protocol
text status
}
protocol_audits {
uuid id PK
uuid session_id FK
uuid workspace_id FK "NEW: NOT NULL, backfilled"
}
protocol_history {
uuid id PK
uuid session_id FK
uuid workspace_id FK "NEW: NOT NULL, backfilled"
}
protocol_scope {
uuid id PK
uuid session_id FK
uuid workspace_id FK "NEW: NOT NULL, backfilled"
}
protocol_visas {
uuid id PK
uuid session_id FK
uuid workspace_id FK "NEW: NOT NULL, backfilled"
}
workspaces ||--o{ protocol_sessions : "workspace_id FK (NEW)"
workspaces ||--o{ protocol_audits : "workspace_id FK (NEW)"
workspaces ||--o{ protocol_history : "workspace_id FK (NEW)"
workspaces ||--o{ protocol_scope : "workspace_id FK (NEW)"
workspaces ||--o{ protocol_visas : "workspace_id FK (NEW)"
protocol_sessions ||--o{ protocol_audits : "session_id ON DELETE CASCADE"
protocol_sessions ||--o{ protocol_history : "session_id ON DELETE CASCADE"
protocol_sessions ||--o{ protocol_scope : "session_id ON DELETE CASCADE"
protocol_sessions ||--o{ protocol_visas : "session_id ON DELETE CASCADE"
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql:88-91
**Trigger covers INSERT only, not UPDATE of `session_id`**
The trigger comment says "Keep child `workspace_id` in sync with the parent session automatically," but it is wired as `BEFORE INSERT` only. If a child row's `session_id` is ever updated (e.g., reassigned to a different session belonging to a different workspace), `workspace_id` silently stays pinned to the original workspace, breaking the invariant the column is meant to enforce. Adding a `BEFORE UPDATE WHEN (OLD.session_id IS DISTINCT FROM NEW.session_id)` counterpart to the trigger body, or using `FOR EACH ROW EXECUTE FUNCTION … BEFORE INSERT OR UPDATE`, would cover this gap.
### Issue 2 of 2
supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql:72
**Missing index on `protocol_sessions.workspace_id`**
The migration adds `workspace_id` indexes on all four child tables, but not on `protocol_sessions` itself. PostgreSQL does not create an index automatically for a foreign key source column, and the new RLS policy (step 4) evaluates `workspace_id IN (SELECT ...)` on every authenticated `SELECT` against `protocol_sessions`. Without an index, that filter will scan the whole table. Given that child-table indexes were added for exactly the same reason, adding one here keeps the pattern consistent.
```suggestion
CREATE INDEX IF NOT EXISTS idx_protocol_sessions_workspace ON public.protocol_sessions(workspace_id);
-- 8. Keep child workspace_id in sync with the parent session automatically,
```
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/protocol-wo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+15
to
+91
| DELETE FROM public.protocol_sessions ps | ||
| WHERE ps.workspace_id IS NULL | ||
| OR ps.workspace_id !~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | ||
| OR NOT EXISTS (SELECT 1 FROM public.workspaces w WHERE w.id::text = ps.workspace_id); | ||
|
|
||
| -- 2. Drop the workspace-scoped policy before altering the column type | ||
| -- (a policy referencing the column blocks ALTER COLUMN TYPE). | ||
| DROP POLICY IF EXISTS tenant_member_read ON public.protocol_sessions; | ||
|
|
||
| -- 3. Normalize workspace_id to uuid + NOT NULL, and add the workspaces FK. | ||
| ALTER TABLE public.protocol_sessions | ||
| ALTER COLUMN workspace_id TYPE uuid USING workspace_id::uuid, | ||
| ALTER COLUMN workspace_id SET NOT NULL; | ||
|
|
||
| ALTER TABLE public.protocol_sessions | ||
| ADD CONSTRAINT protocol_sessions_workspace_id_fkey | ||
| FOREIGN KEY (workspace_id) REFERENCES public.workspaces(id) ON DELETE CASCADE; | ||
|
|
||
| -- 4. Recreate the policy without the text cast (now uuid = uuid). | ||
| CREATE POLICY tenant_member_read ON public.protocol_sessions | ||
| FOR SELECT TO authenticated | ||
| USING ( | ||
| workspace_id IN ( | ||
| SELECT wm.workspace_id FROM public.workspace_memberships wm | ||
| WHERE wm.user_id = auth.uid() | ||
| ) | ||
| ); | ||
|
|
||
| -- 5. Add workspace_id to the child tables (nullable for backfill). | ||
| ALTER TABLE public.protocol_audits ADD COLUMN IF NOT EXISTS workspace_id uuid; | ||
| ALTER TABLE public.protocol_history ADD COLUMN IF NOT EXISTS workspace_id uuid; | ||
| ALTER TABLE public.protocol_scope ADD COLUMN IF NOT EXISTS workspace_id uuid; | ||
| ALTER TABLE public.protocol_visas ADD COLUMN IF NOT EXISTS workspace_id uuid; | ||
|
|
||
| -- 6. Backfill from the parent session. | ||
| UPDATE public.protocol_audits c SET workspace_id = s.workspace_id FROM public.protocol_sessions s WHERE s.id = c.session_id; | ||
| UPDATE public.protocol_history c SET workspace_id = s.workspace_id FROM public.protocol_sessions s WHERE s.id = c.session_id; | ||
| UPDATE public.protocol_scope c SET workspace_id = s.workspace_id FROM public.protocol_sessions s WHERE s.id = c.session_id; | ||
| UPDATE public.protocol_visas c SET workspace_id = s.workspace_id FROM public.protocol_sessions s WHERE s.id = c.session_id; | ||
|
|
||
| -- 7. Enforce NOT NULL + workspaces FK + index on each child table. | ||
| ALTER TABLE public.protocol_audits ALTER COLUMN workspace_id SET NOT NULL; | ||
| ALTER TABLE public.protocol_audits ADD CONSTRAINT protocol_audits_workspace_id_fkey FOREIGN KEY (workspace_id) REFERENCES public.workspaces(id) ON DELETE CASCADE; | ||
| CREATE INDEX IF NOT EXISTS idx_protocol_audits_workspace ON public.protocol_audits(workspace_id); | ||
|
|
||
| ALTER TABLE public.protocol_history ALTER COLUMN workspace_id SET NOT NULL; | ||
| ALTER TABLE public.protocol_history ADD CONSTRAINT protocol_history_workspace_id_fkey FOREIGN KEY (workspace_id) REFERENCES public.workspaces(id) ON DELETE CASCADE; | ||
| CREATE INDEX IF NOT EXISTS idx_protocol_history_workspace ON public.protocol_history(workspace_id); | ||
|
|
||
| ALTER TABLE public.protocol_scope ALTER COLUMN workspace_id SET NOT NULL; | ||
| ALTER TABLE public.protocol_scope ADD CONSTRAINT protocol_scope_workspace_id_fkey FOREIGN KEY (workspace_id) REFERENCES public.workspaces(id) ON DELETE CASCADE; | ||
| CREATE INDEX IF NOT EXISTS idx_protocol_scope_workspace ON public.protocol_scope(workspace_id); | ||
|
|
||
| ALTER TABLE public.protocol_visas ALTER COLUMN workspace_id SET NOT NULL; | ||
| ALTER TABLE public.protocol_visas ADD CONSTRAINT protocol_visas_workspace_id_fkey FOREIGN KEY (workspace_id) REFERENCES public.workspaces(id) ON DELETE CASCADE; | ||
| CREATE INDEX IF NOT EXISTS idx_protocol_visas_workspace ON public.protocol_visas(workspace_id); | ||
|
|
||
| -- 8. Keep child workspace_id in sync with the parent session automatically, | ||
| -- so app inserts cannot omit or mis-set it. Explicit values are respected. | ||
| CREATE OR REPLACE FUNCTION public.set_protocol_child_workspace_id() | ||
| RETURNS trigger | ||
| LANGUAGE plpgsql | ||
| SET search_path = pg_catalog, public | ||
| AS $$ | ||
| BEGIN | ||
| IF NEW.workspace_id IS NULL THEN | ||
| SELECT workspace_id INTO NEW.workspace_id | ||
| FROM public.protocol_sessions WHERE id = NEW.session_id; | ||
| END IF; | ||
| RETURN NEW; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE TRIGGER trg_set_workspace_id BEFORE INSERT ON public.protocol_audits FOR EACH ROW EXECUTE FUNCTION public.set_protocol_child_workspace_id(); | ||
| CREATE TRIGGER trg_set_workspace_id BEFORE INSERT ON public.protocol_history FOR EACH ROW EXECUTE FUNCTION public.set_protocol_child_workspace_id(); | ||
| CREATE TRIGGER trg_set_workspace_id BEFORE INSERT ON public.protocol_scope FOR EACH ROW EXECUTE FUNCTION public.set_protocol_child_workspace_id(); | ||
| CREATE TRIGGER trg_set_workspace_id BEFORE INSERT ON public.protocol_visas FOR EACH ROW EXECUTE FUNCTION public.set_protocol_child_workspace_id(); |
Contributor
There was a problem hiding this comment.
This 9-step migration runs as discrete auto-committed DDL statements. If any step fails on staging — for example, SET NOT NULL on a child table failing because orphaned rows survived — the database is left in a partially-applied state with no rollback path. Some tables would have workspace_id uuid (nullable) while others have it NOT NULL with the FK, and re-running the migration would fail on ADD CONSTRAINT and CREATE TRIGGER statements that already committed. Wrapping the entire migration in BEGIN; ... COMMIT; gives all-or-nothing semantics.
Prompt To Fix With AI
This is a comment left during a code review.
Path: supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql
Line: 15-91
Comment:
**Migration is not wrapped in a transaction**
This 9-step migration runs as discrete auto-committed DDL statements. If any step fails on staging — for example, `SET NOT NULL` on a child table failing because orphaned rows survived — the database is left in a partially-applied state with no rollback path. Some tables would have `workspace_id uuid` (nullable) while others have it `NOT NULL` with the FK, and re-running the migration would fail on `ADD CONSTRAINT` and `CREATE TRIGGER` statements that already committed. Wrapping the entire migration in `BEGIN; ... COMMIT;` gives all-or-nothing semantics.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+79
to
+85
| BEGIN | ||
| IF NEW.workspace_id IS NULL THEN | ||
| SELECT workspace_id INTO NEW.workspace_id | ||
| FROM public.protocol_sessions WHERE id = NEW.session_id; | ||
| END IF; | ||
| RETURN NEW; | ||
| END; |
Contributor
There was a problem hiding this comment.
workspace_id to be accepted
The trigger only populates workspace_id when NULL. If a caller provides an explicit workspace_id that doesn't match the parent session's workspace (but is a valid UUID in workspaces), the FK is satisfied and the row is accepted — creating a child row whose workspace_id disagrees with its session_id's workspace. The migration comment states the trigger ensures "app inserts can't omit or mis-set it" but only the omission case is enforced. Unconditionally deriving workspace_id from the parent session removes the gap.
Suggested change
| BEGIN | |
| IF NEW.workspace_id IS NULL THEN | |
| SELECT workspace_id INTO NEW.workspace_id | |
| FROM public.protocol_sessions WHERE id = NEW.session_id; | |
| END IF; | |
| RETURN NEW; | |
| END; | |
| BEGIN | |
| -- Always derive workspace_id from the parent session to prevent mismatches. | |
| SELECT workspace_id INTO NEW.workspace_id | |
| FROM public.protocol_sessions WHERE id = NEW.session_id; | |
| RETURN NEW; | |
| END; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql
Line: 79-85
Comment:
**Trigger allows explicitly mismatched `workspace_id` to be accepted**
The trigger only populates `workspace_id` when `NULL`. If a caller provides an explicit `workspace_id` that doesn't match the parent session's workspace (but is a valid UUID in `workspaces`), the FK is satisfied and the row is accepted — creating a child row whose `workspace_id` disagrees with its `session_id`'s workspace. The migration comment states the trigger ensures "app inserts can't omit or mis-set it" but only the omission case is enforced. Unconditionally deriving `workspace_id` from the parent session removes the gap.
```suggestion
BEGIN
-- Always derive workspace_id from the parent session to prevent mismatches.
SELECT workspace_id INTO NEW.workspace_id
FROM public.protocol_sessions WHERE id = NEW.session_id;
RETURN NEW;
END;
```
How can I resolve this? If you propose a fix, please make it concise.| DELETE FROM public.protocol_sessions ps | ||
| WHERE ps.workspace_id IS NULL | ||
| OR ps.workspace_id !~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | ||
| OR NOT EXISTS (SELECT 1 FROM public.workspaces w WHERE w.id::text = ps.workspace_id); |
Contributor
There was a problem hiding this comment.
w.id::text = ps.workspace_id is a case-sensitive text comparison. PostgreSQL's UUID type always serialises to lowercase, but if any real session stored its workspace_id as uppercase (via a direct insert or client library), NOT EXISTS would return true and that session would be incorrectly deleted. Casting ps.workspace_id::uuid for the join is safer and more idiomatic.
Suggested change
| OR NOT EXISTS (SELECT 1 FROM public.workspaces w WHERE w.id::text = ps.workspace_id); | |
| OR NOT EXISTS (SELECT 1 FROM public.workspaces w WHERE w.id = ps.workspace_id::uuid); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: supabase/migrations/20260608000001_protocol_workspace_defense_in_depth.sql
Line: 18
Comment:
**Case-sensitive UUID comparison may delete valid sessions**
`w.id::text = ps.workspace_id` is a case-sensitive text comparison. PostgreSQL's UUID type always serialises to lowercase, but if any real session stored its `workspace_id` as uppercase (via a direct insert or client library), `NOT EXISTS` would return `true` and that session would be incorrectly deleted. Casting `ps.workspace_id::uuid` for the join is safer and more idiomatic.
```suggestion
OR NOT EXISTS (SELECT 1 FROM public.workspaces w WHERE w.id = ps.workspace_id::uuid);
```
How can I resolve this? If you propose a fix, please make it concise.
goncalovelosa
pushed a commit
to goncalovelosa/thoughtbox
that referenced
this pull request
Jun 15, 2026
Prod's migration ledger references versions whose files never reached main, so the Supabase branching integration aborts every run with 'Remote migration versions not found in local migrations directory' — the default branch has been stuck in MIGRATIONS_FAILED and no new migration (including 20260602214044) can reach prod through it. Restore the two orphaned files verbatim from their source branches: - 20260513090000_allow_ulysses_validator_history_events.sql (origin/fix/ulysses-fixes; applied to prod under this exact version) - 20260608000001_protocol_workspace_defense_in_depth.sql (origin/fix/protocol-workspace-isolation, PR Kastalien-Research#354; applied to prod under re-stamped version 20260608050836 and to staging under this version. When Kastalien-Research#354 rebases, this file diff disappears.) Follow-up (documented one-off, SPEC-V1-INITIATIVE:c2): repair prod's re-stamped ledger entries and apply the missing 20260602214044, after which the branching integration goes green. Refs SPEC-V1-INITIATIVE:c2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #353 (base =
fix/tenant-isolation-leaks; retarget tomainafter #353 merges).What
Defense-in-depth follow-up to the protocol RLS hardening in #353. Gives the protocol-enforcement tables real row-level tenant identity instead of relying solely on the
session_idFK chain.Migration
20260608000001_protocol_workspace_defense_in_depth.sql:protocol_sessions(51 with NULL workspace + 25 with non-tenant text likethoughtbox-staging/thoughtbox-webpage-2026, all from the 2026-03-22→04-10 pre-multi-tenancy dev window). Keeps the 12 real tenant sessions (Apr 23–Jun 6). Children cascade.protocol_sessions.workspace_idtext → uuid,NOT NULL, +workspacesFK (it previously had none — text couldn't FK uuid).workspace_id(uuid,NOT NULL,workspacesFK, indexed) toprotocol_audits/history/scope/visas, backfilled from the parent session.BEFORE INSERTtriggerset_protocol_child_workspace_idcopiesworkspace_idfrom the parent session so app inserts can't omit it — matches the repo's trigger conventions (handle_new_user,set_updated_at) and avoids editing ~13 scattered insert sites.Validation
uuid/NOT NULL, 4 triggers, 5 FKs), then applied viaapply_migration.staging-deploy.yml.db diff --linked(CI schema-drift check) is green because prod matches the migration files.Notes
database.types.tsintentionally not regenerated: childworkspace_idis trigger-managed and unreferenced in code; with the typed client, regenerating would make it a required Insert field and break the build. If the team prefers explicit inserts + regenerated types over the trigger, that's a clean follow-up.🤖 Generated with Claude Code