Repository navigation
fix(api): close five real authorization gaps, and skip the 48-site sweep that would have closed none - #254
Merged
Conversation
…ng it The membership join that gates every project-scoped queryset is held in string literals that nothing reads: go build says nothing about a string, and the queries themselves only run under a database the suite skips unless one of the seven *_TEST_DATABASE_URL gates is set. That makes any rewrite of those literals unreviewable, so these tests render the statements first and read the fragment back out of them. They render rather than reproduce. A gorm session opened with DryRun and DisableAutomaticPing builds a statement without dialling the DSN, so each scope builder can be asked what SQL it would send, with the driver's numbered bind markers put back as question marks. Twelve of the call sites are reachable that way — six scope builders in the session API and six in the external API; the rest are assembled inside a handler body that also executes, and refactoring a handler to make it testable is a larger change than the one these tests exist to guard. What they pin is a split that reads like drift and is not. `pm.is_active = TRUE` is the membership test and is written everywhere. `pm.deleted_at IS NULL` is the membership row's own soft delete, which member removal never sets — only the worker cascade does, when the parent project or workspace is soft-deleted — and it is written only where the queryset reads project_members through its own manager. A join traversed through another model's queryset does not apply the joined model's manager, which is the rule already stated at issue_ordering.go:28 and already asserted for the analytics scope by advance_analytics_test.go. So each of these twelve leaves it out on purpose, and the assertion that it stays out is as load-bearing as the assertion that is_active stays in.
… soft-delete rule as an argument The join that decides whether a caller may read a project's rows was written out as a string literal at thirty-six call sites across internal/project, internal/externalapi and internal/worker. Thirty-one of them end at `pm.is_active = TRUE` and five go on to `AND pm.deleted_at IS NULL`, and nothing at any of those sites says which group it is in or why — the difference is a few characters inside otherwise identical strings, and reading one site tells you nothing about the rule. That is how the same predicate came to diverge in the workspace-role lookups, and it is why the omission keeps being read as an oversight. access.MemberJoin writes the join now and its package comment is where the two columns are explained: is_active is membership, written synchronously by member removal, workspace member removal and account deactivation; deleted_at on the membership row is the worker cascade's mark that the parent project or workspace was soft-deleted, written after the fact and never by member removal. The five sites whose queryset reads project_members through its own manager pass WithMembershipSoftDelete(), so the exception is an argument a reader can see rather than a substring they have to notice. No SQL changed. Thirty-two of the thirty-six render byte for byte what the literal they replaced held; the four in workspace_aggregate_handler.go were backtick strings wrapped across two lines, so their newline and indentation collapse to the single space that was already there logically — same tokens, same order, same bind argument. The join predicates the twelve renderable scope builders send are asserted character for character by the tests added in the previous commit, and they pass unchanged. The remaining hand-written copies are deliberately untouched: the EXISTS fragments and correlated subqueries are embedded in Select strings, the five in workspace_user_handler.go split the predicate between a Joins and a Where, and the assignee joins in the annotations are not authorization at all. The guard in internal/server keeps the literal from regrowing, because one place for this predicate is only worth having if it stays the only place.
…f three handler.workspaceRole, workspace_user_handler.activeWorkspaceRole and member_handler.workspaceMemberRole ran the same query — same table, same join, same predicate `w.slug = ? AND wm.member_id = ? AND wm.is_active = TRUE AND wm.deleted_at IS NULL` — and differed only in how they reported a miss and whether they read the role through Scan or Pluck. Three copies of an authorization predicate in one package is how the fourth copy, in internal/externalapi, came to be missing two filters without anyone noticing, so the next change to this predicate should have one place to happen. workspaceMemberRole is now the only implementation. activeWorkspaceRole is gone and its single caller reads workspaceMemberRole directly, which is an exact swap: both returned the role with a found flag, both returned (0, false, nil) on a miss and both surfaced the error as (0, false, err). workspaceRole keeps its signature and its five callers, as a two-line wrapper — it reported a miss as role 0, and role 0 is not one of the three roles, so discarding the found flag reproduces it including the error path. The only difference between the bodies that is not visible in the result is Pluck into []int against Scan into *int, and workspace_members.role is NOT NULL (migrate/testdata/schema.tsv:1412), so no row can render the two differently. The predicate itself is unchanged. Whether package project's copy should also filter `w.deleted_at IS NULL`, as internal/workspace's does, is a separate question from removing the duplication, and this commit deliberately does not answer it.
…leted workspaces requireProjectBase is ProjectBasePermission, and the workspace-role lookup behind it is the entire gate on the external API's project list, create, update and delete. Its predicate was `w.slug = ? AND wm.member_id = ? AND wm.is_active = TRUE`: alone among the five copies of this lookup in the tree it filtered neither the workspace's own soft delete nor the membership's, so a member of a deleted workspace kept their role there, and every route behind this gate kept answering for them. The exposure is narrow but real. workspaceDelete stamps the workspace's deleted_at and renames its slug to `<slug>__<epoch>` in one transaction, so the caller has to know the mangled slug to get this far — which anyone who saw the workspace before it was deleted can compute, and a client that cached the rename cannot avoid sending. Nothing downstream compensates: the project lookups behind this gate are keyed on the slug and the project id and do not filter the workspace either. The fix is internal/workspace's predicate verbatim, which is the strictest copy: `w.slug = ? AND w.deleted_at IS NULL AND wm.member_id = ? AND wm.is_active = TRUE AND wm.deleted_at IS NULL`. The workspace filter is the load-bearing half — deleted_at on the workspace row is written synchronously, inside the delete's transaction, so the window closes at once rather than when the worker drains the cascade that eventually stamps the membership. It can only narrow: a live workspace has a null deleted_at, and a member of one has a null deleted_at on their membership. The test that comes with it runs the gate against a session that records statements instead of sending them, so the predicate can be asserted without a database — this package has no database-backed test at all, and its gates cover sixty-odd routes. It asserts both halves: which filters the statement carries, and that the gate answers 403 rather than admitting the caller when the lookup matches nothing. Two sibling gates in this package have the same omission and are deliberately left alone here: requireWorkspaceAdmin in project_handler.go and requireWorkspaceUser in sticky_handler.go both filter `wm.is_active = TRUE` without either deleted_at. They guard different routes and belong in their own change rather than inside this one.
…ted project These four queries are the only ones in the tree whose whole subject is the membership row and which carry no other liveness condition at all — no project deleted_at, no workspace deleted_at, no base row to filter: externalapi's requireProjectMember (ProjectEntityPermission, the gate on sixty-odd routes including every write), externalapi's requireProjectAdmin (the three member write routes), externalapi's projectRole (which requireProjectBase asks for updates and deletes), and internal/workspace's requireProjectMember (the project asset routes). With nothing in them about the project, they answered with the caller's stored role forever after the project was deleted, and the handlers behind them do not compensate: the external API's state create, for one, looks its project up on `w.slug = ? AND p.id = ?` with no deleted_at and inserts. Each now filters `pm.deleted_at IS NULL`. That is the parity-correct spelling as well as the narrowing one — in Django these four read ProjectMember's own manager, which is why the other six queries of this shape in the tree already carry it, and why the thirty-one join sites that traverse the membership from another model's queryset deliberately do not. It cannot refuse a live member: a membership row of a live project has a null deleted_at, and nothing but the cascade ever writes one. What it does not do is close the window. The membership's deleted_at is stamped by the worker draining the soft-delete cascade, after the delete has already answered 204 and after the project's own deleted_at is set, so between those two moments these gates still admit the caller. Closing it immediately takes joining projects and filtering p.deleted_at, which is a stronger predicate than the one Django writes for these permissions; that is a parity decision rather than a bug fix, and it is left for its own change. Adding the filter these four were missing is the fail-closed half, and it is the half that needs no decision. view_handler.go's guest count is the fifth query of this shape and is deliberately not touched. It counts guest memberships to decide whether a caller is restricted to their own views, so fewer matching rows means fewer restrictions: adding the filter there loosens access rather than tightening it, and it has no business inside this commit. The tests run each gate against a session that records statements instead of sending them, which is the only way to check a permission query in packages whose database-backed tests are env-gated — internal/externalapi has no such test at all. Each asserts both what the statement filters and that the gate answers 403 when the lookup matches nothing.
…es that asked for it Five queries in four packages asked the same question — what is this person's role in this project, if they are still in it — in four different shapes: Pluck into a slice and read element zero, Count and compare with zero, Count with the role in the predicate and compare with zero, and the same again with the member and project arguments in the other order. access.ProjectRole is that question now, and it is the place a future change to this predicate has to be made once instead of five times. Two of the five counted rows matching a role rather than reading the role back, and turning `COUNT(role = admin) > 0` into `role == admin` is only sound because at most one row can match: project_members carries a unique index on (project_id, member_id) where deleted_at is null (migrate/testdata/schema.tsv:2697), and the workspace is joined on its primary key. That is also why this comes after the commit that added `pm.deleted_at IS NULL` to the gates and not before — without that filter a member of a soft-deleted project can have several cascaded rows with different roles, and the two forms are then not the same question. The predicate access.ProjectRole writes is the one all five now carry: `w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL`. The only textual change is in internal/project's workspaceOrProjectAdmin, which wrote the same conditions with the member before the project. view_handler.go's guest count is the sixth query of this shape and stays where it is. It counts guest memberships to decide whether a caller may only see their own views, so it is the one site where filtering the membership's soft delete widens what the caller can reach — folding it into a helper that filters would quietly change a permission in the direction nobody wants.
houko
force-pushed
the
p3/membership
branch
from
September 19, 2026 12:50
52f2892 to
5c6c508
Compare
houko
added a commit
that referenced
this pull request
Sep 19, 2026
`TestAChangeOnOneServerReachesTheOther` fails on CI roughly one run in eight, always as `0 sync frame(s) arrived in ten seconds`. It hit #254 and #258 during the P3 batch; neither touches `internal/live`, and #258 does not touch Go at all. `Relay.Subscribe` records a subscription in `r.subscriptions` and only afterwards waits for Redis to confirm it, because `client.Subscribe` merely queues the command — `Receive` is what sends it. `waitForSubscription` polled that map, so it returned during the window between those two statements. Pub/sub keeps no backlog, so the change published in that window is not delayed, it is dropped, and the test waits out its full ten seconds for a frame that no longer exists. The relay itself is unchanged: its own ordering is correct, it confirms before it publishes. The test now asks Redis with `PUBSUB NUMSUB`, which only counts a subscriber once the `SUBSCRIBE` has landed. Both servers share one Redis, so one question covers the pair. `0 frames` is what points here rather than at a slow machine: every server publishes SyncStep1 and QueryAwareness from `Subscribe` itself, so a server that were subscribed would have received those too. Receiving nothing means it was not subscribed when the other side published. Verified: the `internal/live` suite passes, including under CPU contention and `-race`. Not verified: the original failure did not reproduce locally in 60 runs under load, on either the old or the new code. This closes a window that is certain from reading the code, not one demonstrated by a local repro. If the test flakes again after this, the root cause is elsewhere and this reasoning should be discarded rather than patched over.
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.
Description
One place for the project-membership join, one workspace-role lookup per package, and two real fail-closed fixes. It deliberately does not do the sweep the audit asked for, because the audit had the direction backwards.
pm.deleted_atis never set by member removal. It is written only by the async worker cascade, and only when a parent project or workspace is soft-deleted (internal/worker/soft_delete.go; the only publishers are project delete and workspace delete). Member removal setsis_active = falseand nothing else. So the ~48 queries that omitpm.deleted_at IS NULLare not observing the weaker of two membership signals — they are declining to observe a lagging duplicate of a parent-liveness signal the same query already tests on the parent row. Adding the clause there closes nothing thatp.deleted_at IS NULLdoes not close sooner and more reliably.There is also a passing test that says so:
internal/project/advance_analytics_test.go:99-103fails ifapm.deleted_atis added, and its failure message records why. Unifying every site to one predicate would break a test that documents the intended Django-port semantics.What is real is five one-line gaps, and they are fixed here:
externalapirequireProjectBase— the gate on the external API's project list, create, update and delete — read memberships of soft-deleted workspaces. Its role lookup filtered neitherw.deleted_atnorwm.deleted_at.w.deleted_atis written synchronously with the delete, so this one closes immediately.externalapirequireProjectMember/requireProjectAdmin/projectRole,workspacerequireProjectMember) kept answering for a project whose membership rows had been cascaded.project/view_handler.go:135is deliberately untouched: it is the one site where filtering the membership's soft delete would loosen a permission.Type of Change
Screenshots and Media (if applicable)
N/A — backend only.
Test Scenarios
The first commit is tests only, landed before any production line moved, so every later commit is provably a no-op or a deliberate change. That ordering is the review aid here.
go build ./...,go vet ./...,gofmt -l .clean;go test -count=1 ./...every package ok, no failures.go test -race -count=1on the six touched packages also clean. (Non-Docker path; the seven*_TEST_DATABASE_URL-gated suites skip themselves. CI'sgo-api.ymlruns exactlygo test,go test -raceandgo vet.)DryRun: true, DisableAutomaticPing: true— no database, no dial — and assert the membership fragment character for character, plus the absence ofpm.deleted_at. Twelve sites covered acrossinternal/projectandinternal/externalapi.deleted_atclause was stripped from all four gates and all four tests failed;AND pm.deleted_at IS NULLwas temporarily added toviewScopeand the pinning test failed with its intended message.internal/server/membership_join_test.gofails onmainand passes here, so the hand-written literal cannot regrow.(alias, column, softDelete)it parsed out of that same literal, and a throwaway test fed those back through the realMemberJoinand compared. 32 of 36 byte-identical; the other 4 areworkspace_aggregate_handler.gosites whose predicate was a two-line backtick string, so a newline and three tabs collapse to one space — identical token stream, order and bind argument. Called out in the commit message rather than normalised silently.References
Three shape changes that keep the same answer:
requireProjectAdminandworkspaceOrProjectAdminnow read the single membership row's role and compare in Go instead ofSELECT count(*)with the role in the predicate, andworkspace requireProjectMemberreads the role instead of counting. Sound becauseproject_membershas a unique index on(project_id, member_id) WHERE deleted_at IS NULLand the workspace is joined on its primary key.Corrections to the plan this branch was built from, all verified: shape A is 36 sites in 25 files, not 32 in 24.
workspaceRolehas 5 callers, not 2. The proposedMemberJoin(..., memberID string, ...) (string, []any)signature was a trap —Joins()accepts it and would bind the argument slice as a single parameter, producing a wrong query that only fails at runtime; it returns just the string.DryRun: truealone still dials the DSN.Not done, deliberately:
externalapiworkspace gates have the same defect:requireWorkspaceAdmin(project_handler.go:324) andrequireWorkspaceUser(sticky_handler.go:248) filterwm.is_activeand neitherdeleted_at. Same one-line fix, different routes, so they are not in this branch.externalapi/state_handler.golooks the project up with nodeleted_at. Closing it means joiningprojectsand filteringp.deleted_at, which departs from Django's predicate for these permissions. That is a decision, not a cleanup.internal/externalapihas no database-backed test. The one worth adding, gated on a newEXTERNALAPI_TEST_DATABASE_URL: soft-delete a project, run the cascade, assert each gate answers 403. These tests assert the predicate and the fail-closed path, which is not the same thing.project's workspace-role predicate still lacksw.deleted_at IS NULLwhereinternal/workspace's has it. Now that there is one copy instead of three it is a one-line change across ~21 call sites — deliberate, so not here.Merge order: independent of #250/#251/#252/#253.
git merge-treeagainst #250 is clean — step 3 does not touchmember_handler.goat all, contrary to what the plan predicted.