auth: fail closed when a jurisdiction's clusters disagree on apiUrl - #2633
Draft
computermode wants to merge 1 commit into
Draft
computermode wants to merge 1 commit into
computermode wants to merge 1 commit into
Conversation
entiredb#4129 (COR-1896) made Cluster.apiUrl per-cell, so two clusters in one jurisdiction can advertise different values. resolveCellAPIBaseURL fell back to the first match when no default cluster matched, which would route /me reads to an arbitrary cell with no error. It now resolves the default cluster's value or a value every cluster agrees on, and errors otherwise — without wrapping ErrNoCellForJurisdiction, so activity/recap do not treat ambiguity as an unserved region and fall back. Part of COR-1896. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Blank-URL cluster rows are excluded from consensus and can still cause silent wrong-cell routing.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates home-cell routing to reject ambiguous multi-cell jurisdiction catalogs.
Changes:
- Prefer the jurisdiction’s default cluster.
- Require non-default clusters to agree on
apiUrl. - Add multi-cell routing tests.
| File | Description |
|---|---|
cmd/entire/cli/auth/cell_data_api.go |
Implements fail-closed cell selection. |
cmd/entire/cli/auth/cell_data_api_test.go |
Tests default, ambiguous, and unanimous catalogs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+603
to
+608
| // The default cluster marks the jurisdiction's home cell. Without one, | ||
| // clusters agreeing on one apiUrl are still unambiguous; distinct values | ||
| // fail closed rather than routing /me reads to an arbitrary cell. | ||
| for _, row := range matches { | ||
| if row.IsDefault { | ||
| chosen = row | ||
| break | ||
| return strings.TrimRight(row.APIURL, "/"), nil |
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
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.
https://entire.io/gh/entireio/cli/trails/1459
Why
entiredb#4129 (COR-1896) derives
Cluster.apiUrlper cell, so two clusters in one jurisdiction can advertise different values.resolveCellAPIBaseURL— the home-cell pick behind/me/*routing and--jurisdiction— assumed they never differ and fell back to the first match when no default cluster matched, which would route home-scoped reads to an arbitrary cell with no error. Today the default-cluster preference masks it (the default sits in the home cell); moving the default, or the default row losing itsapiUrl, would misroute silently. Same fix as entire.io#4889 makes in the BFF'spickJurisdictionApi.What
resolveCellAPIBaseURLresolves the default cluster's value, else a value every matching cluster agrees on, and otherwise errors — consistent with this file's existing rule that a wrong-region "success" is worse than a command failure.ErrNoCellForJurisdiction: callers with a data-API fallback treat that as "entire-api isn't serving this region", which an ambiguous catalog is not.No behavior change for any current topology: prod is one cell per jurisdiction, and staging US's default (
royalcanin) carries anapiUrl.Verification
TestCellClientFactory_MultiCellJurisdictionHomePick: default wins over a sibling cell; no default with distinct cells fails closed (and does not unwrap toErrNoCellForJurisdiction); no default with one agreed value resolves.mise run check(format, lint, unit + integration + canary) passes.Part of COR-1896.
🤖 Generated with Claude Code
Note
Medium Risk
Changes home-jurisdiction cell routing for
/meand--jurisdiction; wrong routing would read another cell’s data, but current prod/staging topologies are unchanged and ambiguous catalogs now fail loudly.Overview
resolveCellAPIBaseURLno longer falls back to the first matching cluster when several US (or other) clusters advertise differentapiUrlvalues. It now returns the default cluster’s cell immediately, accepts a single agreedapiUrlwhen there is no default, and errors if cells disagree—avoiding silent/mereads against the wrong cell (entiredb#4129 / COR-1896).The new ambiguity error is not wrapped in
ErrNoCellForJurisdiction, so callers that treat that sentinel as “region not served” and fall back to the data API do not mis-handle an ambiguous catalog.TestCellClientFactory_MultiCellJurisdictionHomePicklocks in default-wins, fail-closed on distinct cells without a default, and unanimous-apiUrlresolution. Comments incell_data_api.godocument the multi-cell-per-jurisdiction model.Reviewed by Cursor Bugbot for commit 3c85812. Configure here.