fix(taxonomy): 404 node records on a tenant mismatch (ENG-1887) - #118
Conversation
GET /v1/taxonomy/nodes/{node_id}/records returned 200 with an empty data
array when the node belonged to another tenant, while every sibling
node-scoped operation — runs.retrieve, record-counts, rename, softRemove —
returns 404 for the same mismatch.
Nothing leaked: the tenant is a predicate on the records query, so a foreign
node simply matches no rows. The problem is that the answer was ambiguous.
"Empty" meant three different things at once — the node is yours and holds
no records, the node is someone else's, or the node does not exist — and a
caller could not tell them apart.
ListNodeRecords now resolves ownership before running the records query, the
same shape CountNodeRecords and GetTree already use. The ownership predicate
moves into a shared taxonomyNodeForTenantWhere const so the locking write
path and the new read path cannot drift on what "yours" means.
A soft-removed node of your own tenant now 404s too, where it previously
returned 200-empty. That matches rename and softRemove, and the tree has
already dropped the node, so a caller asking for its records is working from
stale state.
Ownership stays non-enumerable: an unknown node id, a foreign node and a
removed node are all the same 404.
Also documents the 404 on the endpoint in openapi.yaml, which regenerates
the listRecords SDK docstring — it promised only "Tenant-scoped" while
rename/softRemove already promised the 404.
✱ Stainless preview buildsThis PR will update the ✅ hub-typescript studio · code
This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
WalkthroughTaxonomy node record lookup now validates that the node is visible and belongs to the tenant before querying records. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/repository/taxonomy_repository.go`:
- Around line 1049-1055: The node ownership validation and records retrieval
must use one consistent database read so concurrent RemoveNode commits cannot
produce a false empty success. Update the method containing the GetNodeForTenant
guard and recursive records query to combine validation with retrieval in one
SQL statement or use a consistent locking transaction snapshot, preserving the
required 404 behavior when the node is removed or does not belong to the tenant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4250ff75-0073-4f74-9c2a-de354a57840f
📒 Files selected for processing (4)
internal/repository/taxonomy_repository.goopenapi.yamltests/taxonomy_api_test.gotests/taxonomy_persistence_test.go
The ownership guard and the recursive records query ran as two separate statements, each on its own READ COMMITTED snapshot. A RemoveNode committing between them passed the guard and then matched no rows in the CTE anchor (WHERE id = $1 AND removed_at IS NULL), handing back 200 with an empty data array — the exact ambiguity this endpoint's 404 contract exists to remove. The result was not just ambiguous but unlinearizable: soft-remove leaves cluster memberships intact, so the node did hold records at guard time and holds them still. "Empty" described no point in time. Both reads now share one REPEATABLE READ, read-only transaction. Postgres takes the snapshot at the first statement, so the node is either visible to both reads or to neither: a removal landing after the guard still returns the records, one landing before it returns 404. Read-only, so there is no write conflict to serialize against and the deferred rollback is the only exit. GetNodeForTenant becomes the package-level getNodeForTenant taking a queryer, so the caller owns the snapshot. It had a single caller, so nothing else changes; it also no longer needs to be exported.
|
Verified — the finding is valid, and fixed in beeae71. The race, concretely. The guard and the records query ran as two statements, each on its own
So a node removed mid-request came back as the ambiguous 200-empty that this PR exists to eliminate. It was also not merely ambiguous but unlinearizable: soft-remove leaves The fix. Both reads now share one dbTx, err := r.db.BeginTx(ctx, pgx.TxOptions{IsoLevel: pgx.RepeatableRead, AccessMode: pgx.ReadOnly})
if err != nil {
return nil, 0, fmt.Errorf("begin taxonomy node records tx: %w", err)
}
defer rollbackQuietly(ctx, dbTx, "list taxonomy node records: rollback failed")
if _, err := getNodeForTenant(ctx, dbTx, nodeID, tenantID); err != nil {
return nil, 0, err
}
rows, err := dbTx.Query(ctx, `WITH RECURSIVE visible_nodes AS (...)`)Postgres takes the snapshot at the first statement, not at
Either way the 200-empty-for-a-removed-node outcome is gone. I went with the snapshot rather than folding it into a single statement: making one query distinguish "root missing" from "root present but empty" needs a Two notes:
On testing the window itself: I have not added a test for it. Reproducing it needs a removal to commit between the two statements inside
|
2 participants
What does this PR do?
Linear: https://linear.app/formbricks/issue/ENG-1887/triage-drilldown-nodesidrecords-returns-200-empty-not-404-on-tenant
GET /v1/taxonomy/nodes/{node_id}/recordsreturned 200 with an emptydatawhen the node belonged to another tenant, while every sibling node-scoped operation returns 404 for the same mismatch:GET /v1/taxonomy/runs/{run_id}GET /v1/taxonomy/runs/{run_id}/treeGET /v1/taxonomy/runs/{run_id}/record-countsPATCH /v1/taxonomy/nodes/{node_id}(rename)DELETE /v1/taxonomy/nodes/{node_id}(soft remove)GET /v1/taxonomy/nodes/{node_id}/recordsNothing leaked. The tenant is a predicate on the records query (
WHERE tr.tenant_id = $2), so a foreign node simply matches no rows. This is a contract problem, not an isolation one — surfaced by the ENG-1214 tenant-isolation verification, which confirmed zero cross-tenant records.The problem is that the answer was ambiguous. "Empty" meant three different things at once and a caller could not tell them apart:
The change
ListNodeRecordsnow resolves ownership before running the records query — the same shapeCountNodeRecordsandGetTreealready use:getNodeForTenantis the read-only counterpart of the existinggetNodeForUpdate: same ownership predicate, no row lock. The predicate itself moves into a sharedtaxonomyNodeForTenantWhereconst so the locking write path and the read path cannot drift on what "yours" means.Both reads run inside one
REPEATABLE READ, read-only transaction. That matters: on separate statements each read gets its ownREAD COMMITTEDsnapshot, so aRemoveNodecommitting between them would pass the guard and then match no rows in the CTE anchor (WHERE id = $1 AND removed_at IS NULL) — handing back the very 200-empty this contract exists to remove. Worse, it would not even be a state that ever existed: soft-remove leaves cluster memberships intact, so the node held records at guard time and holds them still.Postgres takes the
REPEATABLE READsnapshot at the first statement, so the node is either visible to both reads or to neither — a removal landing after the guard still returns the records, one landing before it returns 404. Read-only, so there is no write conflict to serialize against (could not serialize accesscannot occur here) and the deferredrollbackQuietlyis the only exit.Ownership stays non-enumerable: an unknown node id, a foreign node and a removed node are all the same 404.
openapi.yamldocuments the 404 on the endpoint and drops the now-inaccurate description. That regenerates thelistRecordsSDK docstring, which promised only "Tenant-scoped" whilerename/softRemovealready promised "404 if the node does not belong to the tenant".Behaviour change to be aware of
A soft-removed node of your own tenant now 404s where it previously returned 200-empty. That is deliberate:
renameandsoftRemove, which already 404 on a removed node;If we would rather keep removed nodes readable, it is one line — drop
removed_at IS NULLfrom the shared const.API behaviour: before / after
Request, with
node_idowned by tenantorg-A:Before:
After:
A node that is yours and genuinely holds no records still returns
200 { "data": [], "limit": … }— and now that answer actually means something.Consumers
ListNodeRecordshas exactly one caller — the public endpoint itself. No worker or internal service depends on it, so the blast radius is that one route.On the Formbricks Web side, ENG-1886 (formbricks#8707) already maps a Hub 404 on this call to a 404 of its own. That mapping is currently unreachable because the Hub never sends one; once this merges it starts working with no further change on the web side.
How should this be tested?
Automated (all run locally against
compose.ymlPostgres onPOSTGRES_PORT=5433, migrations at version 20):make build→ both binaries built ✅make tests→ok github.com/formbricks/hub/tests✅go test ./...→ all 17 packages green ✅make fmt+make lint→0 issues✅make lint-openapi→No results with a severity of 'error' found!✅Tests updated — both places that pinned the old behaviour, plus new coverage:
tests/taxonomy_api_test.go→node records 404 for another tenanttests/taxonomy_persistence_test.go→TestTaxonomyRepository_ListNodeRecordsErrNotFound; unknown node id → the sameErrNotFound; soft-removed node →ErrNotFound; surviving root → 200-empty, which is now a meaningful answertests/taxonomy_persistence_test.go→TestTaxonomyRepository_TenantIsolationnode records refuse another tenantsubtest, so the isolation suite covers every tenant-scoped op as its docstring claimsNot covered by a test: the concurrent-removal window itself. Exercising it needs a removal to commit between the two statements inside
ListNodeRecords, which would require a test seam in the method; any goroutine-and-sleep approximation would be flaky. The contract either side of the race is covered by the tests above, and the snapshot change is exercised by the whole suite still passing. Happy to add the seam and a deterministic test if reviewers would rather have it.Manual reproduction against a local Hub:
org-Aand note a node id from the tree.GET /v1/taxonomy/nodes/{node_id}/records?tenant_id=org-A→ 200 with the assigned records.?tenant_id=org-B→ 404application/problem+json(was 200 +"data": []).?tenant_id=org-A→ the same 404, so ownership is not enumerable.DELETEthe node, then re-run step 2 → 404."data": [].Test configuration:
DATABASE_URL=postgres://postgres:postgres@localhost:5433/test_db?sslmode=disable, API key from.env.Checklist
Required
make buildmake tests(integration tests intests/)make fmtandmake lint; no new warningsgit pull origin mainmigrations/with goose annotations and ranmake migrate-validate— n/a, no schema changeAppreciated
make testsor API contract workflow)docs/if changes were necessary — n/a, the endpoint's contract lives inopenapi.yaml, which is updated heremake tests-coveragefor meaningful logic changes