amadan
apps create: fix build_probe test gaps + close protocol-relative escape
Review round 1 findings, both defects in test code, not the implementation:
- TestCreateAppStampsControlPlaneAndProbe asserted the store but never the
HTTP response body, so a renamed or dropped echo key would pass every
test while breaking every real `carlos apps create -control-plane` at
"did not stick". Now asserts out["control_plane"]/out["build_probe"] too.
- TestBuildProbeMustBeAnAbsolutePath derived app names from the bad value's
first byte, which collapsed two rows ("/healthz?x=1", "/health z") to the
same invalid app name "probe/" — they were passing via claimInvalid (400
for the wrong reason) without ever reaching validBuildProbe. Names are
now indexed, and the 400 body is asserted to name build_probe so the
refusal is pinned to the right gate.
Also closes a real gap validBuildProbe's own doc comment overclaimed:
"//evil.example/healthz" (protocol-relative) and raw control bytes both
passed. Both validBuildProbe copies (internal/console/server.go, apps.go)
now reject a "//" prefix and bound every byte to printable ASCII, kept
byte-identical between server and client. New table rows cover the
protocol-relative escape, a control character, and the server's previously
unexercised 256-byte bound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+85
−18
apps.go+17 −5apps_test.go+9 −3internal/console/apps_claim_test.go+37 −3internal/console/server.go+22 −7
diff --git a/apps.go b/apps.go| index 1693057f8..a76c24333 100644 |
| --- a/apps.go |
| +++ b/apps.go |
| @@ -380,16 +380,28 @@ func validPlaceTarget(target string) error { |
| // BEFORE the API call on the same principle validAppName and |
| // validPlaceTarget already state: a shape the server is certain to refuse |
| // should cost nothing against the create verb's own scarce per-day budget. |
| -// "" is always valid (no probe). The grammar mirrors the server's own |
| -// validBuildProbe (internal/console/server.go) exactly: an absolute path, |
| -// no query, no spaces, no fragment. |
| +// "" is always valid (no probe). The grammar must stay BYTE-IDENTICAL to the |
| +// server's own validBuildProbe (internal/console/server.go): an absolute |
| +// path, not protocol-relative ("//host/..."), no query, no fragment, and |
| +// printable ASCII only (no control characters) — the same rule, checked |
| +// twice, never merely "similar" rules that could quietly drift apart. |
| func validBuildProbe(probe string) error { |
| if probe == "" { |
| return nil |
| } |
| - if !strings.HasPrefix(probe, "/") || strings.ContainsAny(probe, "?# \t") || len(probe) > 256 { |
| + bad := !strings.HasPrefix(probe, "/") || strings.HasPrefix(probe, "//") || |
| + strings.ContainsAny(probe, "?#") || len(probe) > 256 |
| + if !bad { |
| + for i := 0; i < len(probe); i++ { |
| + if probe[i] < 0x21 || probe[i] > 0x7e { |
| + bad = true |
| + break |
| + } |
| + } |
| + } |
| + if bad { |
| return fmt.Errorf(`%q is not a usable build probe — want an absolute URL path with no query, `+ |
| - `e.g. "/healthz"`, probe) |
| + `fragment, or protocol-relative prefix, e.g. "/healthz"`, probe) |
| } |
| return nil |
| } |
diff --git a/apps_test.go b/apps_test.go| index 5fbebf7a1..387bc5507 100644 |
| --- a/apps_test.go |
| +++ b/apps_test.go |
| @@ -272,14 +272,20 @@ func TestPlacementMismatchErr(t *testing.T) { |
| // -build-probe against the same shapes the server's own validBuildProbe |
| // (internal/console/server.go) refuses — a typo here must cost nothing |
| // against the create verb's own scarce per-day budget, so the two gates |
| -// must agree on what's refused. |
| +// must agree on what's refused. "//evil.example/healthz" and the |
| +// control-character case pin the 2026-08-24 review finding: a |
| +// protocol-relative probe would silently point the deploy watch at another |
| +// host, and the doc comment must not promise a protection the code lacks. |
| func TestValidBuildProbeMirrorsTheServer(t *testing.T) { |
| - for _, bad := range []string{"healthz", "http://x/healthz", "/healthz?x=1", "/health z", strings.Repeat("/a", 200)} { |
| + for _, bad := range []string{ |
| + "healthz", "http://x/healthz", "/healthz?x=1", "/health z", |
| + "//evil.example/healthz", "/health\nz", "/" + strings.Repeat("a", 256), |
| + } { |
| if err := validBuildProbe(bad); err == nil { |
| t.Errorf("validBuildProbe(%q) = nil, want an error", bad) |
| } |
| } |
| - for _, good := range []string{"", "/healthz", "/a/b/c"} { |
| + for _, good := range []string{"", "/healthz", "/a/b/c", "/" + strings.Repeat("a", 255)} { |
| if err := validBuildProbe(good); err != nil { |
| t.Errorf("validBuildProbe(%q) = %v, want nil", good, err) |
| } |
diff --git a/internal/console/apps_claim_test.go b/internal/console/apps_claim_test.go| index 0a861650f..880ee4694 100644 |
| --- a/internal/console/apps_claim_test.go |
| +++ b/internal/console/apps_claim_test.go |
| @@ -2,6 +2,7 @@ package console |
| import ( |
| "context" |
| + "fmt" |
| "net/http" |
| "net/url" |
| "strings" |
| @@ -155,6 +156,14 @@ func TestCreateAppStampsControlPlaneAndProbe(t *testing.T) { |
| if status != http.StatusOK { |
| t.Fatalf("create = %d %v, want 200", status, out) |
| } |
| + // The echo is what the CLI (runAppsCreate) hard-fails on if it disagrees |
| + // with what was asked for — assert the WIRE keys here too, not just the |
| + // store, so a renamed or dropped echo field (e.g. "controlPlane" instead |
| + // of "control_plane") fails a test instead of only failing every real |
| + // `carlos apps create -control-plane` at "did not stick". |
| + if out["control_plane"] != true || out["build_probe"] != "/healthz" { |
| + t.Fatalf("echo = %v, want the stamped flags the CLI hard-fails on", out) |
| + } |
| cp, probe, err := fx.st.AppFlags(OpsAccountID, "console") |
| if err != nil { |
| t.Fatal(err) |
| @@ -188,15 +197,40 @@ func TestOnlyAnOwnerSetsControlPlane(t *testing.T) { |
| // TestBuildProbeMustBeAnAbsolutePath: the probe is a URL path the deploy watch |
| // dials. Anything else is a typo that would otherwise surface as a deploy that |
| // times out for reasons nobody can see. |
| +// |
| +// App names are indexed ("probeN"), NOT derived from the bad value's first |
| +// byte: "probe" + bad[:1] collapses "/healthz?x=1" and "/health z" to the |
| +// same app name "probe/", which release.ValidApp itself refuses — those two |
| +// rows were passing as claimInvalid (400 for the WRONG reason, before |
| +// build_probe is even read) rather than exercising validBuildProbe at all. |
| +// Indexing keeps every row's app name valid so every row actually reaches |
| +// the gate under test, and asserting the error body names "build_probe" |
| +// pins the refusal to the right gate. |
| func TestBuildProbeMustBeAnAbsolutePath(t *testing.T) { |
| fx := newConsoleFixture(t) |
| token := cliToken(t, fx, "paul@keymail.dev", "code-create-probe-bad") |
| - for _, bad := range []string{"healthz", "http://x/healthz", "/healthz?x=1", "/health z"} { |
| + bad := []string{ |
| + "healthz", // not absolute |
| + "http://x/healthz", // full URL, not a path |
| + "/healthz?x=1", // query — the watch appends its own |
| + "/health z", // space |
| + "//evil.example/healthz", // protocol-relative — silently points the watch at another host |
| + "/health\nz", // control character (CRLF-style header/request injection shape) |
| + "/" + strings.Repeat("a", 256), // 257 bytes, one over the server's bound |
| + } |
| + for i, b := range bad { |
| + name := fmt.Sprintf("probe%d", i) |
| status, out := postJSON(t, fx.ts.URL+"/api/cli/apps/bab/new", map[string]any{ |
| - "name": "probe" + bad[:1], "build_probe": bad, |
| + "name": name, "build_probe": b, |
| }, token) |
| if status != http.StatusBadRequest { |
| - t.Fatalf("build_probe %q = %d %v, want 400", bad, status, out) |
| + t.Fatalf("build_probe %q = %d %v, want 400", b, status, out) |
| + } |
| + if !strings.Contains(str(t, out, "error"), "build_probe") { |
| + t.Fatalf("build_probe %q error = %v, want it to name build_probe (not some other gate)", b, out) |
| + } |
| + if has, err := fx.st.HasAppIn(OpsAccountID, name); err != nil || has { |
| + t.Fatalf("HasAppIn(%s) after a refused build_probe = %v %v, want false", name, has, err) |
| } |
| } |
| } |
diff --git a/internal/console/server.go b/internal/console/server.go| index de68a4af6..981579f19 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -2395,16 +2395,31 @@ func normalizeAppName(name string) string { |
| } |
| // validBuildProbe bounds the deploy watch's probe path to what it actually |
| -// dials: an absolute path, no query, no spaces. The watch appends its own |
| -// cache-busting query parameter (deploy.go), so a probe carrying one would |
| -// produce a URL with two — and a probe that is a full URL would silently |
| -// point the watch at another host entirely, which is the one mistake here |
| -// that could report a DIFFERENT deployment's build as this one's. |
| +// dials: an absolute path, no query, no fragment, no protocol-relative |
| +// "//host/..." escape, and printable ASCII only (no control characters, no |
| +// bytes a Go net/http client would refuse or mangle). The watch appends its |
| +// own cache-busting query parameter (deploy.go), so a probe carrying one |
| +// would produce a URL with two — and a probe starting "//" (or a full URL) |
| +// would silently point the watch at another host entirely, which is the one |
| +// mistake here that could report a DIFFERENT deployment's build as this |
| +// one's. The client mirror (validBuildProbe, apps.go) must stay |
| +// byte-identical to this rule — a typo the server refuses must cost nothing |
| +// against the create verb's own scarce per-day budget, and a typo the |
| +// server ACCEPTS but the client refuses would just be a confusing false |
| +// refusal. |
| func validBuildProbe(p string) bool { |
| - if !strings.HasPrefix(p, "/") || strings.ContainsAny(p, "?# \t") { |
| + if !strings.HasPrefix(p, "/") || strings.HasPrefix(p, "//") || strings.ContainsAny(p, "?#") { |
| return false |
| } |
| - return len(p) <= 256 |
| + if len(p) > 256 { |
| + return false |
| + } |
| + for i := 0; i < len(p); i++ { |
| + if p[i] < 0x21 || p[i] > 0x7e { |
| + return false |
| + } |
| + } |
| + return true |
| } |
| // claimApp claims an app name for acct — once, ever. A name is refused when |