amadan
console: the deployment-route 409 must be unreachable-alike, not just byte-alike
Review finding. The refusal's bytes matched the pre-existing "someone else
holds it" 409, but its POSITION did not: the check ran above the branch
split, before resolveAliasRoute and before AttachClaim, while the old refusal
was reachable only after both. So a prober told them apart by deliberately
failing the LATER gate and reading which refusal came back first —
{"host":"<probe>","route":"!!"} answers 409 for a live non-alias route and
400 "invalid route hostname" otherwise, and a routeless attach from an app
with no instance records does the same against "specify -route". Both are
side-effect-free and enumerate the fleet's whole non-alias route set one
hostname at a time, inside the ceiling.
Byte-equality is necessary, not sufficient. Reachability is the other half of
the neutrality, and a body-comparison test structurally cannot see it.
The guard is now one per branch instead of one above the split: on the alias
branch it sits immediately before AttachClaim, strictly after
resolveAliasRoute has had its chance to refuse; the kind=static branch names
no route, so nothing earlier can run there. The comment at the alias site
records both probe strings and says anything added later belongs above that
line.
The neutrality test now asserts both halves. It keeps the valid-route
byte-comparison and adds the two probe shapes from the review, each asserting
that a deployment-route host and a never-heard-of host answer identically —
and that neither answers 409 at all, since a request that cannot name a route
must never get as far as deciding whether the hostname is free. The previous
test used a valid route on both sides and could not see this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+106
−30
internal/console/apidomains.go+41 −15internal/console/apidomains_test.go+65 −15
diff --git a/internal/console/apidomains.go b/internal/console/apidomains.go| index 3057928d2..849736453 100644 |
| --- a/internal/console/apidomains.go |
| +++ b/internal/console/apidomains.go |
| @@ -252,22 +252,22 @@ func (s *Server) handleAPIDomainAttach(w http.ResponseWriter, r *http.Request) { |
| } |
| ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| - // Defence in depth for the fleet-wide route collision (see |
| - // deploymentRouteTaken): the authoritative refusal is the box-side |
| - // reconcile pass, which is the only place that can retro-clean a claim |
| - // written before this rule existed. This one just stops the claim being |
| - // written in the first place, so the tenant hears "no" at the door |
| - // instead of watching a hostname sit permanently un-materialized. |
| - // |
| - // Byte-identical to the "held by someone else" 409 below, on purpose: a |
| - // distinct status or message here would make the door an existence |
| - // oracle for operator-minted routes, which is precisely the neutrality |
| - // this file's header commits to. |
| - if s.deploymentRouteTaken(ctx, host, acct.Sqid, app) { |
| - jsonError(w, http.StatusConflict, hostUnavailableMsg) |
| - return |
| - } |
| if in.Kind == "static" { |
| + // Defence in depth for the fleet-wide route collision (see |
| + // deploymentRouteTaken): the authoritative refusal is the box-side |
| + // reconcile pass, which is the only place that can retro-clean a |
| + // claim written before this rule existed. This one just stops the |
| + // claim being written in the first place, so the tenant hears "no" |
| + // at the door instead of watching a hostname sit permanently |
| + // un-materialized. |
| + // |
| + // The first-route branch names no route, so resolveAliasRoute (the |
| + // gate the alias branch below has to sit AFTER) never runs here and |
| + // there is nothing earlier to order this against. |
| + if s.deploymentRouteTaken(ctx, host, acct.Sqid, app) { |
| + jsonError(w, http.StatusConflict, hostUnavailableMsg) |
| + return |
| + } |
| // The shared core decides alias-vs-first-route itself (readyRouteHost |
| // is ITS question — no pre-read here): a ready route makes this an |
| // ordinary alias claim naming it, none makes the host the app's |
| @@ -291,6 +291,32 @@ func (s *Server) handleAPIDomainAttach(w http.ResponseWriter, r *http.Request) { |
| if !ok { |
| return |
| } |
| + // The deployment-route refusal sits HERE, immediately before the claim |
| + // write and strictly AFTER resolveAliasRoute — never above it. |
| + // |
| + // Byte-identical bodies are not enough on their own; ORDER is half of |
| + // the neutrality (Task 5 review). When this check ran before |
| + // resolveAliasRoute, the pre-existing "someone else holds it" 409 was |
| + // reachable only after it, so a prober could tell the two apart by |
| + // deliberately failing the LATER gate and reading which refusal came |
| + // back first: |
| + // |
| + // {"host":"<probe>","route":"!!"} -> 409 if <probe> is a live route, |
| + // 400 "invalid route hostname" if not |
| + // {"host":"<probe>"} from an app with no instance records |
| + // -> 409 if <probe> is a live route, |
| + // 400 "specify -route: ..." if not |
| + // |
| + // Both are side-effect-free and enumerate the deployment's non-alias |
| + // route set one hostname at a time, inside the ceiling — exactly the |
| + // existence oracle this file's header forbids. Behind every gate that |
| + // can refuse first, the two 409s are indistinguishable in status, body |
| + // AND reachability. Anything added to this branch later belongs above |
| + // this line, not below it. |
| + if s.deploymentRouteTaken(ctx, host, acct.Sqid, app) { |
| + jsonError(w, http.StatusConflict, hostUnavailableMsg) |
| + return |
| + } |
| created, changed, err := domains.AttachClaim(ctx, s.cfg.Releases, domains.Claim{ |
| Host: host, Account: acct.Sqid, App: app, Route: route, |
| // UTC, like every other timestamp written to the bucket: these claims |
diff --git a/internal/console/apidomains_test.go b/internal/console/apidomains_test.go| index f6f809042..42f4aecc0 100644 |
| --- a/internal/console/apidomains_test.go |
| +++ b/internal/console/apidomains_test.go |
| @@ -1292,12 +1292,25 @@ func TestLegacyAppTokensHaveSeparateWildFailBudgets(t *testing.T) { |
| // publish fleet-wide, such a claim captures that traffic on every remote |
| // edge, so the door refuses it. |
| // |
| -// The refusal must be byte-identical to the "someone else holds it" 409 |
| -// asserted in TestAPIDomainAttachConflictNeutral: a distinct status or body |
| -// here would turn the door into an existence oracle for operator routes, |
| -// which is exactly the neutrality this file exists to keep. Both refusals |
| -// are compared against each other, not against a literal, so they cannot |
| -// drift apart without this failing. |
| +// The refusal must be indistinguishable from the "someone else holds it" |
| +// 409 in TWO ways, and this test asserts both: |
| +// |
| +// 1. SAME BYTES. Status and body are compared against the pre-existing |
| +// refusal itself, not against a literal, so the two cannot drift apart. |
| +// |
| +// 2. SAME REACHABILITY. Byte-identical bodies are worthless if the new |
| +// check runs EARLIER than the old one, because a prober can then |
| +// deliberately fail a later gate and read the two apart by which |
| +// refusal comes back first (Task 5 review). So the probes below use a |
| +// BAD route and an ABSENT route — request shapes that must be refused |
| +// by resolveAliasRoute before the hostname is ever considered — and |
| +// assert a deployment-route host and a never-heard-of host answer |
| +// IDENTICALLY. With the guard ordered above resolveAliasRoute these |
| +// read 409 vs 400 and the deployment's whole non-alias route set can be |
| +// enumerated one host at a time, side-effect-free, inside the ceiling. |
| +// |
| +// A test that only probes with a VALID route (as the first version of this |
| +// one did) cannot see failure mode 2 at all. |
| func TestAPIDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| ctx := context.Background() |
| fx, member := instanceFixture(t, "code-dom-deproute") |
| @@ -1312,8 +1325,8 @@ func TestAPIDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| if err := status.WriteBox(ctx, fx.rel, status.Box{Label: "annie", GeneratedAt: time.Now()}); err != nil { |
| t.Fatal(err) |
| } |
| - // A hostname held by a different account: the existing refusal, and the |
| - // yardstick the new one is measured against. |
| + // A hostname held by a different account: the pre-existing refusal, and |
| + // the yardstick the new one is measured against. |
| if _, _, err := domains.AttachClaim(ctx, fx.rel, domains.Claim{ |
| Host: "claimed-by-someone-else.com", Account: "cdef", App: "widgets", |
| Route: "widgets.example.net", CreatedAt: time.Now().UTC().Format(time.RFC3339), |
| @@ -1321,20 +1334,57 @@ func TestAPIDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| t.Fatal(err) |
| } |
| - takenCode, _, takenBody := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), member, |
| - attachBody("go.tito.io", "two.go.tito.io")) |
| - unavailCode, _, unavailBody := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), member, |
| - attachBody("claimed-by-someone-else.com", "two.go.tito.io")) |
| + attach := func(body string) (int, string) { |
| + t.Helper() |
| + code, _, out := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), member, body) |
| + return code, strings.TrimSpace(out) |
| + } |
| + // (1) Same bytes: with a perfectly valid route, both refused hostnames |
| + // answer the identical neutral 409. |
| + takenCode, takenBody := attach(attachBody("go.tito.io", "two.go.tito.io")) |
| + heldCode, heldBody := attach(attachBody("claimed-by-someone-else.com", "two.go.tito.io")) |
| if takenCode != http.StatusConflict { |
| t.Fatalf("attach of another box's route = %d %s, want 409", takenCode, takenBody) |
| } |
| - if takenCode != unavailCode || strings.TrimSpace(takenBody) != strings.TrimSpace(unavailBody) { |
| + if takenCode != heldCode || takenBody != heldBody { |
| t.Fatalf("refusal %d %q differs from the existing unavailable %d %q — that is an existence oracle", |
| - takenCode, takenBody, unavailCode, unavailBody) |
| + takenCode, takenBody, heldCode, heldBody) |
| + } |
| + claimAbsent(t, fx, "go.tito.io") |
| + |
| + // (2) Same reachability. "never-heard-of.example" is not a deployment |
| + // route, is claimed by nobody, and is not denied — the ONLY thing that |
| + // differs between the two probes in each pair is whether a live box |
| + // serves the hostname. That difference must not be observable. |
| + for _, probe := range []struct { |
| + what string |
| + body func(host string) string |
| + }{ |
| + // A malformed route: resolveAliasRoute's 400 must come first. |
| + {"a malformed route", func(host string) string { return attachBody(host, "!!") }}, |
| + // No route at all, from an app with zero instance records: the |
| + // "specify -route" 400 must come first. Needs no malformed input at |
| + // all, which is what makes it the cheap version of this probe. |
| + {"an absent route", func(host string) string { return attachBody(host, "") }}, |
| + } { |
| + routeCode, routeBody := attach(probe.body("go.tito.io")) |
| + unknownCode, unknownBody := attach(probe.body("never-heard-of.example")) |
| + if routeCode != unknownCode || routeBody != unknownBody { |
| + t.Errorf("with %s, a deployment-route host answered %d %q but an unknown host answered %d %q — "+ |
| + "the deployment-route check is ordered ABOVE the gate that should refuse first, "+ |
| + "so this door enumerates every non-alias route in the fleet", |
| + probe.what, routeCode, routeBody, unknownCode, unknownBody) |
| + } |
| + if routeCode == http.StatusConflict { |
| + t.Errorf("with %s, the request was refused as a conflict (%d %q) — a request that "+ |
| + "cannot name a route must never get as far as deciding whether the hostname is free", |
| + probe.what, routeCode, routeBody) |
| + } |
| } |
| - // And nothing was written for the refused host. |
| + // Nothing was written by any probe. |
| claimAbsent(t, fx, "go.tito.io") |
| + claimAbsent(t, fx, "never-heard-of.example") |
| } |
| // A hostname this very app ALREADY holds the claim on stays attachable. This |