From 9bcdae60a1239f7713112473f688eaea17986e23 Mon Sep 17 00:00:00 2001 From: pandeymangg Date: Fri, 31 Jul 2026 14:09:08 +0530 Subject: [PATCH 1/2] fix(taxonomy): 404 node records on a tenant mismatch (ENG-1887) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/repository/taxonomy_repository.go | 49 +++++++++++++++++++--- openapi.yaml | 10 ++++- tests/taxonomy_api_test.go | 9 ++-- tests/taxonomy_persistence_test.go | 28 +++++++++++-- 4 files changed, 81 insertions(+), 15 deletions(-) diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index 39d71cd8..50466205 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -1038,12 +1038,22 @@ func (r *TaxonomyRepository) RemoveNode( } // ListNodeRecords returns feedback records assigned to a visible taxonomy node or descendants. +// The node must be visible and belong to the tenant, otherwise a not-found error is returned — +// the same contract as every other node-scoped operation. func (r *TaxonomyRepository) ListNodeRecords( ctx context.Context, nodeID uuid.UUID, tenantID string, limit int, ) ([]models.FeedbackRecord, int, error) { + // The records query below is already tenant-safe on its own (it filters on the run's tenant, so a + // foreign node simply matches no rows). This guard is about the contract, not isolation: without it + // a foreign or removed node is indistinguishable from one that genuinely holds no records, which is + // what CountNodeRecords, GetTree, RenameNode and RemoveNode all avoid by resolving ownership first. + if _, err := r.GetNodeForTenant(ctx, nodeID, tenantID); err != nil { + return nil, 0, err + } + if limit <= 0 { limit = defaultTaxonomyNodeRecordLimit } @@ -1099,6 +1109,28 @@ func (r *TaxonomyRepository) ListNodeRecords( return records, limit, nil } +// GetNodeForTenant returns a visible taxonomy node addressable by the tenant, or a not-found error. +// The read-only counterpart of getNodeForUpdate: same ownership predicate, no row lock, no transaction. +func (r *TaxonomyRepository) GetNodeForTenant( + ctx context.Context, + nodeID uuid.UUID, + tenantID string, +) (*models.TaxonomyNode, error) { + node, err := queryTaxonomyNode(ctx, r.db, taxonomyNodeSelect+` + FROM taxonomy_nodes`+taxonomyNodeForTenantWhere, + nodeID, tenantID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewNotFoundError("taxonomy_node", "taxonomy node not found") + } + + return nil, fmt.Errorf("get taxonomy node for tenant: %w", err) + } + + return node, nil +} + func (r *TaxonomyRepository) queryRunInputRows( ctx context.Context, run *models.TaxonomyRun, @@ -1342,6 +1374,16 @@ func scanTaxonomyNode(row scanner) (*models.TaxonomyNode, error) { return &node, nil } +// A node is addressable by a tenant when it is visible and its run belongs to that tenant. Shared by +// the locking write path and the read path so the two can never drift apart on what "yours" means. +// $1 is the node id, $2 the tenant id. +const taxonomyNodeForTenantWhere = ` + WHERE id = $1 AND removed_at IS NULL + AND EXISTS ( + SELECT 1 FROM taxonomy_runs + WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 + )` + // getNodeForUpdate takes a tenantWriteTx (not the narrower queryer) so the // compiler enforces that the SELECT ... FOR UPDATE row lock is held for the // life of a transaction; outside one, the lock would release at statement end. @@ -1354,12 +1396,7 @@ func getNodeForUpdate( // The tenant predicate keeps the row lock tenant-scoped: a caller can never // lock another tenant's node row, even transiently. node, err := queryTaxonomyNode(ctx, transaction, taxonomyNodeSelect+` - FROM taxonomy_nodes - WHERE id = $1 AND removed_at IS NULL - AND EXISTS ( - SELECT 1 FROM taxonomy_runs - WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 - ) + FROM taxonomy_nodes`+taxonomyNodeForTenantWhere+` FOR UPDATE`, nodeID, tenantID, ) diff --git a/openapi.yaml b/openapi.yaml index 51212dab..63bfb2aa 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -2080,7 +2080,9 @@ paths: summary: List feedback records for a taxonomy node description: | Returns the feedback records assigned to a node and all of its (visible) descendant nodes, via the - clusters those nodes reference. Tenant-scoped. The `limit` in the response reflects the applied cap. + clusters those nodes reference. Tenant-scoped; returns 404 if the node does not belong to the tenant + or has been removed. An empty `data` therefore means the node genuinely holds no records. The `limit` + in the response reflects the applied cap. operationId: list-taxonomy-node-records parameters: - name: node_id @@ -2129,6 +2131,12 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "404": + description: Not Found – no node with this ID for the tenant. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: diff --git a/tests/taxonomy_api_test.go b/tests/taxonomy_api_test.go index d49361d1..d7f3127f 100644 --- a/tests/taxonomy_api_test.go +++ b/tests/taxonomy_api_test.go @@ -223,12 +223,11 @@ func TestTaxonomyAPI_TenantIsolation(t *testing.T) { taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.BranchID.String(), removeQuery), harness.apiKey, nil, http.StatusNotFound, nil) }) - t.Run("node records return nothing for another tenant", func(t *testing.T) { + t.Run("node records 404 for another tenant", func(t *testing.T) { + // Not 200-with-empty: that would make a foreign node indistinguishable from one of your own + // that holds no records. Same contract as the run, tree, rename and remove cases above. recordsURL := taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.RootID.String()+"/records", otherQuery) - - var resp models.TaxonomyNodeRecordsResponse - requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusOK, &resp) - require.Empty(t, resp.Data) + requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusNotFound, nil) }) } diff --git a/tests/taxonomy_persistence_test.go b/tests/taxonomy_persistence_test.go index 1c908892..67427d15 100644 --- a/tests/taxonomy_persistence_test.go +++ b/tests/taxonomy_persistence_test.go @@ -466,10 +466,27 @@ func TestTaxonomyRepository_ListNodeRecords(t *testing.T) { require.Len(t, records, 1) require.Equal(t, ids.FeedbackRecordID, records[0].ID) - // A different tenant sees nothing for the same node id. - otherTenantRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50) + // A different tenant cannot address the node at all — not even to be told it is empty. + _, _, err = repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50) + require.ErrorIs(t, err, huberrors.ErrNotFound, "node records must be tenant-scoped") + + // An unknown node id is the same not-found, so ownership is never enumerable. + _, _, err = repo.ListNodeRecords(ctx, uuid.New(), scope.TenantID, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + + // A soft-removed node is not addressable either, matching rename and remove: the tree has already + // dropped it, so a caller asking for its records is working from stale state. + _, err = repo.RemoveNode(ctx, ids.LeafID, scope.TenantID, "actor-remove") + require.NoError(t, err) + + _, _, err = repo.ListNodeRecords(ctx, ids.LeafID, scope.TenantID, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + + // The root survives and now genuinely holds no records, because the removed leaf carried the only + // cluster membership. That empty result is only meaningful because the cases above are 404s. + rootRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, scope.TenantID, 50) require.NoError(t, err) - require.Empty(t, otherTenantRecords, "node records must be tenant-scoped") + require.Empty(t, rootRecords) } // TestTaxonomyRepository_TenantIsolation proves every tenant-scoped read and mutation refuses @@ -501,6 +518,11 @@ func TestTaxonomyRepository_TenantIsolation(t *testing.T) { require.ErrorIs(t, err, huberrors.ErrNotFound) }) + t.Run("node records refuse another tenant", func(t *testing.T) { + _, _, err := repo.ListNodeRecords(ctx, ids.RootID, otherTenant, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + }) + t.Run("rename and remove refuse another tenant", func(t *testing.T) { _, err := repo.RenameNode(ctx, ids.BranchID, otherTenant, "attacker", "Hijacked") require.ErrorIs(t, err, huberrors.ErrNotFound) From beeae71b1e666b2595296460f05925b81cbfbb7a Mon Sep 17 00:00:00 2001 From: pandeymangg Date: Fri, 31 Jul 2026 14:49:32 +0530 Subject: [PATCH 2/2] fix(taxonomy): read node ownership and records in one snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/repository/taxonomy_repository.go | 77 +++++++++++++--------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index 50466205..502b91b5 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -1046,19 +1046,33 @@ func (r *TaxonomyRepository) ListNodeRecords( tenantID string, limit int, ) ([]models.FeedbackRecord, int, error) { + if limit <= 0 { + limit = defaultTaxonomyNodeRecordLimit + } + // The records query below is already tenant-safe on its own (it filters on the run's tenant, so a - // foreign node simply matches no rows). This guard is about the contract, not isolation: without it - // a foreign or removed node is indistinguishable from one that genuinely holds no records, which is - // what CountNodeRecords, GetTree, RenameNode and RemoveNode all avoid by resolving ownership first. - if _, err := r.GetNodeForTenant(ctx, nodeID, tenantID); err != nil { - return nil, 0, err + // foreign node simply matches no rows). The ownership guard is about the contract, not isolation: + // without it a foreign or removed node is indistinguishable from one that genuinely holds no + // records, which is what CountNodeRecords, GetTree, RenameNode and RemoveNode all avoid by + // resolving ownership first. + // + // Both reads share one REPEATABLE READ snapshot. On separate statements a RemoveNode committing + // between them would pass the guard and then match no rows in the recursive CTE, handing back the + // ambiguous 200-empty this contract exists to remove. Under one snapshot the node is either + // visible to both reads or to neither, so a removal mid-request lands as a 404. Read-only, so the + // deferred rollback is the only exit. + 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) } - if limit <= 0 { - limit = defaultTaxonomyNodeRecordLimit + 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 := r.db.Query(ctx, ` + rows, err := dbTx.Query(ctx, ` WITH RECURSIVE visible_nodes AS ( SELECT id, run_id, cluster_id FROM taxonomy_nodes @@ -1109,28 +1123,6 @@ func (r *TaxonomyRepository) ListNodeRecords( return records, limit, nil } -// GetNodeForTenant returns a visible taxonomy node addressable by the tenant, or a not-found error. -// The read-only counterpart of getNodeForUpdate: same ownership predicate, no row lock, no transaction. -func (r *TaxonomyRepository) GetNodeForTenant( - ctx context.Context, - nodeID uuid.UUID, - tenantID string, -) (*models.TaxonomyNode, error) { - node, err := queryTaxonomyNode(ctx, r.db, taxonomyNodeSelect+` - FROM taxonomy_nodes`+taxonomyNodeForTenantWhere, - nodeID, tenantID, - ) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, huberrors.NewNotFoundError("taxonomy_node", "taxonomy node not found") - } - - return nil, fmt.Errorf("get taxonomy node for tenant: %w", err) - } - - return node, nil -} - func (r *TaxonomyRepository) queryRunInputRows( ctx context.Context, run *models.TaxonomyRun, @@ -1384,6 +1376,31 @@ const taxonomyNodeForTenantWhere = ` WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 )` +// getNodeForTenant returns a visible taxonomy node addressable by the tenant, or a not-found error. +// The read-only counterpart of getNodeForUpdate: same ownership predicate, no row lock. It takes a +// queryer so the caller decides the snapshot — ListNodeRecords passes its transaction so the check +// and the records read cannot disagree about whether the node is still there. +func getNodeForTenant( + ctx context.Context, + q queryer, + nodeID uuid.UUID, + tenantID string, +) (*models.TaxonomyNode, error) { + node, err := queryTaxonomyNode(ctx, q, taxonomyNodeSelect+` + FROM taxonomy_nodes`+taxonomyNodeForTenantWhere, + nodeID, tenantID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewNotFoundError("taxonomy_node", "taxonomy node not found") + } + + return nil, fmt.Errorf("get taxonomy node for tenant: %w", err) + } + + return node, nil +} + // getNodeForUpdate takes a tenantWriteTx (not the narrower queryer) so the // compiler enforces that the SELECT ... FOR UPDATE row lock is held for the // life of a transaction; outside one, the lock would release at statement end.