amadan
console: the deployment-route guard belongs in the shared attach core
Two residuals, one move. The kind=static branch still carried the round-1
ordering leak: "nothing earlier to order against" was true about what came
before it and silent about what came after, and attachDomain has its own
later gates a prober can deliberately fail —
{"host":"<probe>","kind":"static","channel":"!!"} answered 409 for a live
non-alias route and 422 "invalid channel name" otherwise. Separately, the
browser attach door had no guard at all.
Both doors share attachDomain, which holds exactly two AttachClaim calls, so
putting the guard immediately before each of them closes both. The redundant
check above the CLI's kind=static branch goes away; the CLI's alias branch
keeps its own, since that path calls AttachClaim directly and does not go
through the core.
The two core sites are deliberately duplicated rather than hoisted to the top
of the function — hoisting is what creates the leak. The doc comment records
that rule, the probe that exposes it, and that anything added later belongs
above the guard, never between it and the write. Both refuse through
claimRefusal(host, domains.ErrHostUnavailable), so they ARE the existing
refusal path rather than a second one that matches it today.
Tests cover both newly-guarded paths. The browser door was reachable from the
existing fixtures, so it gets a real page-against-page neutrality assertion
plus the ownership exemption. The static branch gets an ordering probe AND a
valid-channel positive case: with a malformed channel attachChannel refuses
both hosts identically, so the ordering probe stayed green with the
first-route guard deleted. An ordering test is not an existence test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 files changed,
+154
−15
internal/console/apidomains.go+6 −15internal/console/apidomains_test.go+41 −0internal/console/appdomains_test.go+83 −0internal/console/domainattach.go+24 −0
diff --git a/internal/console/apidomains.go b/internal/console/apidomains.go| index 849736453..ef6bfa575 100644 |
| --- a/internal/console/apidomains.go |
| +++ b/internal/console/apidomains.go |
| @@ -253,21 +253,12 @@ func (s *Server) handleAPIDomainAttach(w http.ResponseWriter, r *http.Request) { |
| ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| 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 |
| - } |
| + // No deployment-route check here: this branch delegates to |
| + // attachDomain, which carries the guard immediately before each of |
| + // its own AttachClaim calls — below attachChannel and every other |
| + // gate that can refuse first. One placed HERE would sit above them |
| + // and be read apart from the ordinary 409 by a prober sending a |
| + // deliberately malformed channel (Task 5 review round 2). |
| // 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 |
diff --git a/internal/console/apidomains_test.go b/internal/console/apidomains_test.go| index 42f4aecc0..dc4562297 100644 |
| --- a/internal/console/apidomains_test.go |
| +++ b/internal/console/apidomains_test.go |
| @@ -1367,6 +1367,20 @@ func TestAPIDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| // "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, "") }}, |
| + // The kind=static branch, which takes no route and so has none of |
| + // the gates above — its later gate is attachChannel, inside the |
| + // shared attachDomain core. Round 1 left the guard at the top of |
| + // this branch on the reasoning that there was nothing earlier to |
| + // order against; there was nothing earlier, but there was something |
| + // LATER, and that is just as readable. Round 2 moved the guard down |
| + // into the core, immediately before each AttachClaim. |
| + {"a malformed channel on the static branch", func(host string) string { |
| + b, err := json.Marshal(map[string]string{"host": host, "kind": "static", "channel": "!!"}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return string(b) |
| + }}, |
| } { |
| routeCode, routeBody := attach(probe.body("go.tito.io")) |
| unknownCode, unknownBody := attach(probe.body("never-heard-of.example")) |
| @@ -1382,6 +1396,33 @@ func TestAPIDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| probe.what, routeCode, routeBody) |
| } |
| } |
| + |
| + // (3) The kind=static branch actually REFUSES, not merely refuses in the |
| + // right order. The ordering probes above cannot see this: with a |
| + // malformed channel, attachChannel refuses BOTH hosts identically and |
| + // the pair agrees whether or not the guard behind it exists at all. So |
| + // probe once more with a VALID channel, where nothing stands between the |
| + // guard and the claim write, and compare against the held hostname on |
| + // the same branch. |
| + staticBody := func(host string) string { |
| + b, err := json.Marshal(map[string]string{"host": host, "kind": "static", "channel": "stable"}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return string(b) |
| + } |
| + staticRouteCode, staticRouteBody := attach(staticBody("go.tito.io")) |
| + staticHeldCode, staticHeldBody := attach(staticBody("claimed-by-someone-else.com")) |
| + if staticRouteCode != http.StatusConflict { |
| + t.Errorf("kind=static attach of another box's route = %d %q, want 409 — the first-route "+ |
| + "branch reaches AttachClaim with no deployment-route guard in front of it", |
| + staticRouteCode, staticRouteBody) |
| + } |
| + if staticRouteCode != staticHeldCode || staticRouteBody != staticHeldBody { |
| + t.Errorf("kind=static refusal %d %q differs from the held-hostname refusal %d %q — that is an existence oracle", |
| + staticRouteCode, staticRouteBody, staticHeldCode, staticHeldBody) |
| + } |
| + |
| // Nothing was written by any probe. |
| claimAbsent(t, fx, "go.tito.io") |
| claimAbsent(t, fx, "never-heard-of.example") |
diff --git a/internal/console/appdomains_test.go b/internal/console/appdomains_test.go| index 02ab75a0f..2ddc2a2f0 100644 |
| --- a/internal/console/appdomains_test.go |
| +++ b/internal/console/appdomains_test.go |
| @@ -15,6 +15,8 @@ import ( |
| "time" |
| "github.com/carlosframework/platform/internal/domains" |
| + "github.com/carlosframework/platform/internal/edgesfile" |
| + "github.com/carlosframework/platform/internal/status" |
| ) |
| // TestAppDomainAttachHappyPathAliases exercises the default fixture's kass |
| @@ -211,3 +213,84 @@ func TestAppDomainDetachIsOwnerOnly(t *testing.T) { |
| t.Errorf("claim after a member's refused detach: %v, want it untouched", err) |
| } |
| } |
| + |
| +// TestAppDomainAttachRefusesADeploymentRouteNeutrally: the BROWSER door's |
| +// half of the fleet-wide route collision (Task 5 review round 2). Both doors |
| +// share attachDomain, which is where the guard now lives — immediately |
| +// before each of its two AttachClaim calls — so this door is covered by the |
| +// same move that closed the CLI's kind=static branch. This test is what |
| +// makes that coverage a fact rather than an inference. |
| +// |
| +// The property is the one the CLI door keeps: a hostname a live box already |
| +// serves as a REAL route is refused with the refusal a hostname held by |
| +// another account gets, and the two are indistinguishable. Compared page |
| +// against page, not against a literal, so they cannot drift apart. |
| +func TestAppDomainAttachRefusesADeploymentRouteNeutrally(t *testing.T) { |
| + ctx := context.Background() |
| + fx := newConsoleFixture(t) |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + |
| + // go.tito.io is a real, NON-ALIAS route published by a live box. Both |
| + // halves are required: DeploymentRoutes drops any edges file whose box |
| + // has no fresh status snapshot. |
| + if _, err := fx.rel.Put(ctx, edgesfile.Key("annie"), |
| + []byte(`{"v":1,"box":"annie","eip":"198.51.100.9","hosts":{"go.tito.io":{"role":"home"}}}`), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := status.WriteBox(ctx, fx.rel, status.Box{Label: "annie", GeneratedAt: time.Now()}); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The yardstick: a hostname held by a different account entirely. |
| + 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), |
| + }); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + routeStatus, _, routeBody := postForm(t, c, fx.u("/apps/kass/domains/attach"), |
| + url.Values{"csrf": {csrf}, "host": {"go.tito.io"}}) |
| + heldStatus, _, heldBody := postForm(t, c, fx.u("/apps/kass/domains/attach"), |
| + url.Values{"csrf": {csrf}, "host": {"claimed-by-someone-else.com"}}) |
| + |
| + if routeStatus != http.StatusConflict { |
| + t.Fatalf("browser attach of another box's route = %d, want 409:\n%s", routeStatus, routeBody) |
| + } |
| + if routeStatus != heldStatus || routeBody != heldBody { |
| + t.Fatalf("browser refusal %d differs from the held-hostname refusal %d — that is an existence oracle:\n--- route ---\n%s\n--- held ---\n%s", |
| + routeStatus, heldStatus, routeBody, heldBody) |
| + } |
| + claimAbsent(t, fx, "go.tito.io") |
| +} |
| + |
| +// The browser door's own version of the ownership exemption: a hostname this |
| +// app ALREADY holds the claim on stays attachable, because a first-route |
| +// attach makes the host a real route and the fold would otherwise report the |
| +// caller's own route back at them forever. |
| +func TestAppDomainAttachStillConvergesTheCallersOwnDeploymentRoute(t *testing.T) { |
| + ctx := context.Background() |
| + fx := newConsoleFixture(t) |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + |
| + if _, err := fx.rel.Put(ctx, edgesfile.Key("annie"), |
| + []byte(`{"v":1,"box":"annie","eip":"198.51.100.9","hosts":{"mine.example.com":{"role":"home"}}}`), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := status.WriteBox(ctx, fx.rel, status.Box{Label: "annie", GeneratedAt: time.Now()}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := domains.AttachClaim(ctx, fx.rel, domains.Claim{ |
| + Host: "mine.example.com", Account: fx.sqid, App: "kass", |
| + CreatedAt: time.Now().UTC().Format(time.RFC3339), |
| + }); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + st, loc, body := postForm(t, c, fx.u("/apps/kass/domains/attach"), |
| + url.Values{"csrf": {csrf}, "host": {"mine.example.com"}}) |
| + if st != http.StatusSeeOther || loc != fx.path("/apps/kass/settings") { |
| + t.Fatalf("re-attach of the caller's own hostname = %d -> %q, want 303 -> settings:\n%s", st, loc, body) |
| + } |
| +} |
diff --git a/internal/console/domainattach.go b/internal/console/domainattach.go| index c0f57f5f4..baff49f74 100644 |
| --- a/internal/console/domainattach.go |
| +++ b/internal/console/domainattach.go |
| @@ -24,6 +24,24 @@ |
| // would let two accounts each declare a record for one host and the loser |
| // find out from the fleet. |
| // |
| +// The fleet-wide deployment-route refusal lives in here for the same reason, |
| +// and with one extra rule: it sits IMMEDIATELY BEFORE each AttachClaim, below |
| +// every gate that could refuse for another reason. Byte-identical refusals |
| +// are only half of the neutrality — ORDER is the other half. A check placed |
| +// above attachChannel, say, is read apart from the ordinary "held by someone |
| +// else" 409 by a prober who deliberately sends a malformed channel: a |
| +// hostname a live box serves comes back 409, anything else comes back the |
| +// channel refusal, and the deployment's whole non-alias route set enumerates |
| +// one hostname at a time. So the two calls below are deliberately duplicated |
| +// rather than hoisted to the top of this function, and anything added to |
| +// either branch later belongs ABOVE them, never between them and the write. |
| +// (Task 5 review, rounds 1 and 2; the CLI door's own direct-AttachClaim path |
| +// carries the same guard under the same rule — apidomains.go.) |
| +// |
| +// Both refusals go through claimRefusal with domains.ErrHostUnavailable |
| +// rather than constructing a doorRefusal here, so they ARE the existing |
| +// refusal path rather than a second one that merely matches it today. |
| +// |
| // audit + bumpSerial live in here rather than in the doors, so both doors |
| // get them, in the right place, for free: strictly after the write they |
| // announce — and gated on an ACTUAL write (#151's changed-gating rule): a |
| @@ -106,6 +124,9 @@ func (s *Server) attachDomain(ctx context.Context, acct *Account, app, host, kin |
| // --- alias attach: the app already has a route to serve from --------- |
| if route != "" { |
| + if s.deploymentRouteTaken(ctx, host, acct.Sqid, app) { |
| + return attachResult{}, claimRefusal(host, domains.ErrHostUnavailable) |
| + } |
| created, changed, err := domains.AttachClaim(ctx, s.cfg.Releases, claim) |
| if err != nil { |
| return attachResult{}, claimRefusal(host, err) |
| @@ -126,6 +147,9 @@ func (s *Server) attachDomain(ctx context.Context, acct *Account, app, host, kin |
| if ref != nil { |
| return attachResult{}, ref |
| } |
| + if s.deploymentRouteTaken(ctx, host, acct.Sqid, app) { |
| + return attachResult{}, claimRefusal(host, domains.ErrHostUnavailable) |
| + } |
| created, changed, err := domains.AttachClaim(ctx, s.cfg.Releases, claim) |
| if err != nil { |
| return attachResult{}, claimRefusal(host, err) |