amadan
carlos deploy: re-check the echoed probe, and state the body contract
Fix round 1 (three Minors, all promoted).
The deploy path took the console's echoed build_probe on trust and
concatenated it onto an origin. Both grammar gates live at the WRITE door;
this value arrives from a READ. A probe not starting with "/" is the shape
that matters: "https://" + host + "@evil.example/x" parses host as USERINFO
and evil.example as the HOST, so the watch would poll a machine of someone
else's choosing and report ITS build as this deploy's — exactly what
validBuildProbe's own comment says it exists to prevent. Re-checked with that
same mirror, and degraded (never fatal: ship and promote already landed).
`-build-probe`'s help now states the body contract an app author has to meet:
the build must appear as a whole whitespace-delimited word. {"build":"abc1234"}
satisfied the old wording and then watched forever.
And the ship door's degradation branch — read the flags, fail, log, answer
201 anyway — is pinned. It is the whole premise of "a metadata read must
never fail a ship whose bytes are already stored", and nothing tested it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+135
−4
apps.go+2 −1deploy.go+24 −3deploy_test.go+68 −0internal/console/apicli_test.go+41 −0
diff --git a/apps.go b/apps.go| index a76c24333..9ac32f9a5 100644 |
| --- a/apps.go |
| +++ b/apps.go |
| @@ -416,7 +416,8 @@ func runAppsCreate(args []string) { |
| buildProbe := fs.String("build-probe", "", |
| `path whose response body carries the running build, e.g. "/healthz" — `+ |
| "`carlos deploy` polls it instead of the X-Carlos-Version header, which the edge "+ |
| - "stamps from the route row and which therefore lies until the process restarts") |
| + "stamps from the route row and which therefore lies until the process restarts; "+ |
| + `the build must appear as a whole whitespace-delimited word in the body, e.g. "ok abc1234"`) |
| // Not accountFlags: that helper also registers -registry for direct-mode |
| // resolution, and this verb never resolves locally — the console resolves |
| // the account against the caller's own memberships. |
diff --git a/deploy.go b/deploy.go| index 2ec49d980..1fd40ee14 100644 |
| --- a/deploy.go |
| +++ b/deploy.go |
| @@ -236,12 +236,33 @@ func runDeploy(args []string) { |
| // before, and whether or not, the process running the old binary |
| // restarts. Empty (no probe declared, or a console too old to echo the |
| // field) leaves the header watch exactly as it was. |
| - if shipRes.BuildProbe != "" { |
| - mutedf(stdout, "watching %s for the build %s reports itself\n", shipRes.BuildProbe, *app) |
| + // |
| + // Re-checked HERE against the same grammar the create door enforces |
| + // (validBuildProbe, apps.go — the byte-identical mirror of the console's |
| + // own). Both existing gates are at the WRITE door; this value arrives |
| + // from a READ, and the watch CONCATENATES it onto an origin. A probe not |
| + // starting with "/" is the shape that matters: "https://" + host + |
| + // "@evil.example/x" parses `host` as userinfo and evil.example as the |
| + // HOST, so the watch would poll a machine of someone else's choosing and |
| + // report ITS build as this deploy's — precisely the failure |
| + // validBuildProbe exists to prevent. Only a pre-existing row, a future |
| + // migration that forgets, or a hostile console can produce it; the cost |
| + // of refusing to depend on that is four lines. Degrade, never fatal: the |
| + // ship and promote already landed, so the honest outcome is the header |
| + // watch plus a line saying why. |
| + probe := shipRes.BuildProbe |
| + if probe != "" { |
| + if err := validBuildProbe(probe); err != nil { |
| + mutedf(stdout, "ignoring the console's build probe: %v — watching the version header instead\n", err) |
| + probe = "" |
| + } |
| + } |
| + if probe != "" { |
| + mutedf(stdout, "watching %s for the build %s reports itself\n", probe, *app) |
| } |
| targets := make([]watchTarget, 0, len(hosts)) |
| for _, h := range hosts { |
| - targets = append(targets, watchTarget{Host: h, Origin: watchOrigin(h), Probe: shipRes.BuildProbe}) |
| + targets = append(targets, watchTarget{Host: h, Origin: watchOrigin(h), Probe: probe}) |
| } |
| diag := &watchDiagnostics{ |
| // A timed-out host's wake reason, from the platform log stream — |
diff --git a/deploy_test.go b/deploy_test.go| index 47936e120..3ee25ee4d 100644 |
| --- a/deploy_test.go |
| +++ b/deploy_test.go |
| @@ -1631,3 +1631,71 @@ func TestDeployWithNoProbeStillWatchesTheHeader(t *testing.T) { |
| t.Fatalf("header watch did not complete:\n%s", out) |
| } |
| } |
| + |
| +// TestDeployRefusesAMalformedProbeAndFallsBackToTheHeader pins the re-check |
| +// on the READ side. Both grammar gates live at the write door; this value |
| +// arrives from a response and is CONCATENATED onto an origin, so a probe |
| +// that does not start with "/" is a redirection primitive: |
| +// "https://" + host + "@evil.example/x" parses host as USERINFO and |
| +// evil.example as the host, and the watch would report a machine of someone |
| +// else's choosing as this deploy's build. Degrade to the header watch, say |
| +// why, and never dial the named host at all. |
| +func TestDeployRefusesAMalformedProbeAndFallsBackToTheHeader(t *testing.T) { |
| + useTempCredentials(t) |
| + clearDeploymentEnv(t) |
| + |
| + const ( |
| + app = "console" |
| + host = "console.example.test" |
| + channel = "canary/dev" |
| + ) |
| + fc := newDeployFakeConsole(t, "sha0002") |
| + fc.setShipProbe("@evil.example/healthz") // no leading slash: the userinfo trick |
| + fc.setInstances([]apiInstanceRecord{{Host: host, Channel: channel}}) |
| + credFile, err := loadCredentials() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + credFile.put(credential{Console: fc.ts.URL, Address: "paul@keymail.dev", Token: "cli-token", |
| + ExpiresAt: time.Now().Add(time.Hour)}) |
| + if err := credFile.save(); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + var paths sync.Map |
| + edgeSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + paths.Store(r.URL.Path, true) |
| + w.Header().Set("X-Carlos-Version", "sha0002") |
| + w.WriteHeader(http.StatusOK) |
| + })) |
| + defer edgeSrv.Close() |
| + origOrigin := watchOrigin |
| + watchOrigin = func(h string) string { return edgeSrv.URL } |
| + defer func() { watchOrigin = origOrigin }() |
| + |
| + bin := filepath.Join(t.TempDir(), "console") |
| + if err := os.WriteFile(bin, []byte("ELF-ish"), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + buf := captureStdout(t) |
| + runDeploy([]string{"-app", app, "-account", "ops", "-version", "sha0002", |
| + "-channel", channel, "-timeout", "5s", bin}) |
| + out := buf.String() |
| + |
| + if !strings.Contains(out, "ignoring the console's build probe") { |
| + t.Fatalf("a malformed probe must be named and dropped, not silently used:\n%s", out) |
| + } |
| + paths.Range(func(k, _ any) bool { |
| + if k.(string) != "/" { |
| + t.Errorf("a refused probe was still dialled: %q", k) |
| + } |
| + return true |
| + }) |
| + if _, dialled := paths.Load("/"); !dialled { |
| + t.Fatalf("the header watch never ran:\n%s", out) |
| + } |
| + if !strings.Contains(out, "live https://"+host) { |
| + t.Fatalf("deploy must still complete on the header watch:\n%s", out) |
| + } |
| +} |
diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go| index cfb920fec..8d5158098 100644 |
| --- a/internal/console/apicli_test.go |
| +++ b/internal/console/apicli_test.go |
| @@ -2570,3 +2570,44 @@ func TestAPIShipOmitsTheBuildProbeWhenNoneIsDeclared(t *testing.T) { |
| t.Fatalf("ship body carries build_probe for an app that declares none: %v", out) |
| } |
| } |
| + |
| +// TestAPIShipSurvivesAnUnreadableBuildProbe pins the degradation the ruling |
| +// rests on: the bytes are STORED by the time addBuildProbe runs, so a failed |
| +// metadata read must log and leave the response alone — never turn a |
| +// successful upload into an error. The CLI then sees no key, reads "", and |
| +// keeps the header watch, which is the same outcome as talking to a console |
| +// too old to echo the field at all. |
| +// |
| +// The column is dropped rather than the database closed, the same technique |
| +// (and for the same reason) as TestControlPlaneLookupFailureRefuses: closing |
| +// it would fail the token lookup and the app 404 long before the ship handler |
| +// reached this read. Dropping ONE column leaves every earlier gate working |
| +// and fails exactly AppFlags. |
| +func TestAPIShipSurvivesAnUnreadableBuildProbe(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "console", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.SetAppBuildProbe(OpsAccountID, "console", "/healthz"); err != nil { |
| + t.Fatal(err) |
| + } |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-ship-probe-blip") |
| + if _, err := fx.st.db.Exec(`ALTER TABLE apps DROP COLUMN build_probe`); err != nil { |
| + t.Fatalf("drop the probe column: %v", err) |
| + } |
| + |
| + payload := randomBytes(t, 256) |
| + status, out := postShip(t, fx.ts.URL+"/api/cli/apps/bab/console/releases", token, |
| + []shipPart{metaPart(t, "abc1234", "binary"), {name: "linux-arm64", body: payload}}) |
| + if status != http.StatusCreated { |
| + t.Fatalf("ship with an unreadable build_probe = %d %v, want 201 — the bytes are already stored", status, out) |
| + } |
| + if _, present := out["build_probe"]; present { |
| + t.Fatalf("an unreadable probe must leave the key off, not guess: %v", out) |
| + } |
| + // The release really landed — the point is that only the ECHO degraded. |
| + if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "console", "abc1234"); err != nil { |
| + t.Fatalf("manifest after a degraded echo: %v", err) |
| + } |
| +} |