Fix mutation reliability, close RLS holes, add e2e suite and CI gating#30
Merged
Conversation
Three problems found while testing permissions against a real database: The profiles INSERT policy read `not exists(select * from profiles where orgid = id)`, where both columns resolved to the subquery's own table. Comparing a profile's org id to its own primary key is never true, so the clause was always true and the policy collapsed to `isAdmin(orgid) or true` -- any caller, including anon, could insert an admin profile into any organization. The bootstrap case it was reaching for is unreachable anyway, since create_org is security definer and creates the first profile itself. The profiles UPDATE policy gates the row rather than its columns. Members need it to edit their own name and bio, but it also let them set admin on themselves. RLS cannot restrict columns, so a trigger now guards admin and supervisor. Commenting required permission to UPDATE the thing being commented on, because addComment appended to the parent's comments array directly. A member commenting on someone else's change inserted the comment but matched zero rows on the link, which PostgREST does not report as an error, so the comment was silently orphaned. Commenting and deleting now go through security definer functions that check membership and act atomically. Also fixes profile/person linking to be case insensitive, tightens role visibility to match teams, and corrects `accountable = NULL` to `IS NULL` in the process delete policy, which had made that clause dead code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two bugs in the quote branch of the parser. The loop parsed the line captured before it started rather than the current line, so every line of a multi-line quote was a copy of the first: `"one"` followed by `"two"` produced Quote[Text[one], Text[one]]. It also advanced the index once more after the loop had already advanced past the last quoted line, which swallowed whatever block followed a quote. The existing test for that case was failing; the multi-line case had no coverage at all, so tests are added for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mutations were fire-and-forget, relying on a realtime notification to make
the change visible. When realtime was delayed or disconnected, the write
succeeded but the UI silently kept showing stale data -- which is what
"added a person and they didn't appear" turned out to be.
Realtime itself was also broken. Organization.channels was never written to,
so the dedupe guard was dead and ignore() never removed a channel. Since
supabase.channel() returns the existing channel for a topic and .on() after
subscribe() throws, the second visit to an organization threw and realtime
stayed dead for the rest of the session. Subscribing without a status
callback hid it. The db instance was also rebuilt on every invalidation,
discarding its listeners.
The deeper reason mutations went unwrapped is that queryOrError only
accepted PostgrestError | null, while the methods that create things
returned {data, error} or {error, id} -- so the wrapper did not fit them and
roughly two dozen call sites did without. Every mutation now returns a
single MutationResult shape and goes through mutate(), which reports the
error and reloads on success. Reloads coalesce, so unchecking a task with
many subtasks reloads once rather than once per subtask.
Fixes found along the way: uncheckAll never returned its promises, so
Promise.all awaited nothing; updateProcessConcern, updateChangeStatus and
updateOrgName were missing awaits on addComment; updateTeamName recorded its
comment against 'orgs' instead of 'teams', so renames vanished from history;
and inserting a how focused the new node even when creation failed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
206 tests running the real Organization API against a real local database, so they cover PostgREST, row level security, and the profile/person linking triggers. Every bug fixed in the preceding commits lived in that layer and was invisible to TypeScript. Each test file gets an organization built through the real create_org RPC plus three actors -- an admin, a plain member, and an outsider -- so every mutating operation can assert what each is actually permitted to do. Assertions read state back through a service-role client rather than trusting error === null, because a write filtered out by RLS returns no error at all. Two guards keep the suite honest as the code moves. coverage.test.ts fails naming any Organization method no test exercises, and mutations.test.ts fails naming any component that calls a mutating method without mutate(), deriving the method list from the MutationResult return type so new mutations are covered the moment they are written. Both keep an allowlist for genuine exceptions plus a check that fails if an allowlist entry names something that no longer exists. test:unit ran vitest in watch mode, which is wrong for CI; it now uses `vitest run`, with test:watch for the old behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
seed.sql was empty, so a fresh `supabase db reset` left nothing to click on and every local check started with hand-building an organization. Creates one bakery with enough shape to exercise every mutation: roles in and out of teams, a role nobody holds, a nested how tree with mixed completion and RCI assignments, processes in all three states, a change per status, comments on each kind of parent, and a process with no accountable role so that delete policy branch is reachable. Sign in as test@test.com; the app uses magic links, so the message arrives in Mailpit. sam@test.com is invited but has no account, so signing in as them exercises the linking trigger. Ids are fixed rather than generated, so a reset always produces the same data and links stay valid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nothing gated a production deploy. ci.yaml ran on pull requests but executed no tests at all -- its one step regenerated types into types.gen.ts and diffed that path, while the committed file is src/database/database.types.ts, so git diff compared nothing and the check could never fail. production.yaml pushed migrations to production on every push to main with no test gate, and the app deployed through Vercel's Git integration, which no workflow here could block. That left migrations and the app shipping through independent pipelines with no ordering. Concretely: the app now calls add_comment and delete_comment, which only exist after the migrations, so losing that race breaks commenting. The suite now lives in a reusable workflow that both pipelines call, so the checks gating a deploy are the same ones that ran on the pull request. On main it is test, then migrate, then deploy, with concurrency set so a cancelled run cannot leave migrations applied with no matching app. The Vercel CLI does the deploy, and vercel.json disables Git auto-deploy for main so the two cannot race. CI uses the Supabase CLI from devDependencies rather than setup-cli: generated type output varies between CLI releases, so a floating version would fail the drift check on unrelated changes. .env.local was tracked in this public repository, and the Vercel CLI appends a VERCEL_OIDC_TOKEN to it. gitignore listed it but has no effect on tracked files, so it is untracked here and its local defaults move to .env.example. The existing !.env.example negation sat before the broader .env* rule and was silently overridden, since gitignore applies the last matching pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Supabase has removed pgsodium from their Postgres images, so replaying the migration history on a current image failed with 'schema "pgsodium" does not exist'. That broke CI, and would break any fresh local setup too -- existing machines only worked because they had an older image cached. The extension came from the original schema dump and is referenced nowhere else in the project, so it is now created only where it is actually available. Editing an applied migration is safe here: production already records this version, so db push will not re-run it, and the change only affects databases built from scratch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous attempt guarded on the extension being available, but that was not the problem: pgsodium is still listed as available, and it is the schema that current Supabase Postgres images no longer pre-create. CREATE EXTENSION ... WITH SCHEMA requires the schema to already exist, so the migration still failed to replay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Current Supabase Postgres images no longer let the migration role create objects in the auth schema, so replaying the history failed with 'permission denied for schema auth'. Supabase provides auth.email() itself, and nothing in this project ever referenced it -- including the migration that defined it -- so the redefinition is removed rather than worked around. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
svelte-check failed on a fresh checkout two ways, both hidden locally by a working copy that already had the generated files. Without svelte-kit sync there is no .svelte-kit/tsconfig.json, so nothing resolves. And sync generates the $env/static/public module from the environment it can see, so without a .env the PUBLIC_SUPABASE_* exports are missing and every file importing them fails to check. .env is now written from the running stack rather than copied from .env.example, so a port change cannot make CI disagree with reality, and the build reads it instead of being handed the values separately. Verified in a clean clone rather than the working copy, which is what let both of these through in the first place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A bad or expired token previously failed at `vercel pull` in the deploy job, which runs after migrations have already been applied -- and `supabase db push` cannot be rolled back, so that left the database ahead of an app that never shipped. The credentials are now checked in their own job before anything touches production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three of the five names in the exclude list were not services the CLI knows about -- storage-api, logflare and supavisor, rather than storage, analytics and nothing. Unrecognised names are ignored silently, so those containers started and their images were pulled anyway. CI runs had already failed on Docker Hub rate limits. CI now excludes everything the tests do not touch, which skips those pulls: nothing in the suite exercises realtime, email, storage or edge functions, and supabase/functions is empty. Locally the list is narrower, since Studio, Mailpit and realtime are all wanted during development -- magic link sign in needs Mailpit, and realtime is what the app uses to stay current. Verified by running the full suite against the reduced set. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Started from "I added a person by email and they didn't appear in the table." That turned out to be a dead realtime subscription with no fallback, and pulling on it surfaced several security problems underneath.
Six commits, each reviewable on its own. The first one is a security fix and deserves the closest look.
What was wrong
Any caller, including an unauthenticated one, could make themselves an admin of any organization. The profiles INSERT policy compared a profile's
orgidto its ownidinside a subquery, which is never true, so the whole policy collapsed to always-true. I confirmed the exploit against a live database before fixing it, then confirmed the same script is rejected afterward.Separately, any member could promote themselves to admin, because the UPDATE policy gates the row rather than its columns.
Comments were being silently lost. Commenting required permission to UPDATE the thing being commented on, so a member commenting on someone else's change inserted the comment but linked it to nothing. PostgREST doesn't treat a zero-row update as an error, so the caller saw success.
Why mutations went stale
queryOrErroronly acceptedPostgrestError | null, while the methods that create things returned{data, error}or{error, id}. The wrapper didn't fit them, so ~24 call sites did without it and depended entirely on realtime. Every mutation now returns oneMutationResultshape and goes throughmutate().Tests
206 e2e tests run the real API against a real database — the layer where all of these bugs lived and where TypeScript can't help. Two guards keep it honest: one fails if any method is untested, another fails if any component calls a mutation without
mutate().Both security fixes were mutation-tested: I reverted the mechanism and confirmed the tests fail, so they're load-bearing rather than decorative.
CI
CI ran no tests — its only check regenerated types to a path that isn't the committed one, so
git diffcompared nothing and it could never fail. Meanwhile migrations pushed to production on every merge, and Vercel deployed independently, so the app could land before the migrations it depends on.Now: test → migrate → deploy, in that order, with the same suite gating both PRs and main.
Before merging
vercel.jsondoes this too, but it may not apply to the commit that introduces it, and this merge would otherwise race.Not addressed
addCommentcan still be called on a change whose comments a member can't otherwise touch — worth deciding whether members should be able to comment on changes they didn't author, which is a product question rather than a bug.🤖 Generated with Claude Code