amadan
console: don't leak the Go decoder at the capabilities gate; close two test gaps
Review minors on 9e872a6 + 474734e, no behaviour change to the gate itself.
capsRefusal replaces the raw error on the capabilities 400. caps writes its
own refusals for a human and every one opens with "capabilities: " — those
pass through verbatim, because naming the offending capability and the fix is
the whole point of them. What was leaking was encoding/json's message for a
value that is not even an object ("json: cannot unmarshal number into Go value
of type caps.Set"): an internal type name, no prefix, nothing a caller can act
on. Those become one fixed line in the same register as this handler's other
400s.
shipCapabilitiesMatch's doc overclaimed. It said two blocks "say the same
thing"; list ORDER is significant, so ports [{25},{587}] and [{587},{25}] do
not match and a re-ship between them is a 409. That order-sensitivity is the
defensible behaviour under immutability — sorting would let this function
decide that two manifests an operator can see differ are the same release — so
the comment is corrected to "the same declaration" and the behaviour is now
pinned by a test rather than left to be "helpfully" sorted away later.
Two test gaps:
- The adopt fail-closed case only covered a DECODER refusal, where
json.Unmarshal never completes. Added the semantic one: a block that decodes
perfectly (every name known, v is 1) and is caught only by
ValidCapabilities, on a port that is not a capability. It is reached because
validateManifest runs on every GetManifest, not only at ship — delete that
line and the new test goes red while the old one stays green. Both now run
through one adoptRefusesManifest helper asserting the same four things.
- Gate ORDER at handleAPIShip is now pinned: with the ship semaphore
exhausted, a request carrying a bad capability block still gets 429, not
400. The capability decode was added inside the handler, after the member
gate, the rate limiter and the semaphore, and it has to stay there — a
validation step ahead of them would do decode work for callers those gates
exist to refuse.
The non-object cases got their own test with their own token: ship's limiter
is cliShipBurst (10) per token per hour, and an eleventh case in the existing
table was answered 429 and pinned nothing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+167
−29
internal/console/apicli.go+36 −10internal/console/apicli_test.go+80 −3internal/release/adopt_test.go+51 −16
diff --git a/internal/console/apicli.go b/internal/console/apicli.go| index 1da3642cc..0634686ee 100644 |
| --- a/internal/console/apicli.go |
| +++ b/internal/console/apicli.go |
| @@ -429,15 +429,41 @@ func shipArtifactsMatch(a, b map[string]release.Artifact) bool { |
| return true |
| } |
| -// shipCapabilitiesMatch reports whether two capability blocks say the same |
| -// thing, comparing their CANONICAL JSON ENCODINGS rather than the structs. |
| +// capsRefusal turns a caps error into the caller-facing 400 body. |
| // |
| -// That is not a stylistic choice. caps.Set carries an unexported notes slice |
| -// (the unknown-field-inside-a-known-capability lines its decoder collected), |
| -// so a decoded Set is neither ==-comparable nor reflect.DeepEqual-safe |
| -// against one built any other way once those notes are non-empty — two blocks |
| -// that mean identically the same thing would compare unequal because one of |
| -// them was written with a field this binary ignored. The JSON encoding |
| +// caps writes its own refusals for a human, and every one of them opens with |
| +// "capabilities: " — those pass through verbatim, because naming the offending |
| +// capability and the fix is the whole point of them. What must NOT pass |
| +// through is encoding/json's own message for a value that is not even an |
| +// object ("json: cannot unmarshal number into Go value of type caps.Set"): |
| +// it leaks an internal type name, carries no prefix, and tells the caller |
| +// nothing they can act on. Those become one fixed line, in the same register |
| +// as this handler's other 400s ("malformed meta part"). |
| +func capsRefusal(err error) string { |
| + if msg := err.Error(); strings.HasPrefix(msg, "capabilities: ") { |
| + return msg |
| + } |
| + return "malformed capabilities block — want the manifest's capabilities object" |
| +} |
| + |
| +// shipCapabilitiesMatch reports whether two capability blocks are the SAME |
| +// DECLARATION, comparing their CANONICAL JSON ENCODINGS rather than the |
| +// structs. |
| +// |
| +// Same declaration, not "same meaning": list ORDER is significant, so |
| +// ports [{25},{587}] and [{587},{25}] do not match and a re-ship between them |
| +// is a 409. That is deliberate and it is the conservative direction — under |
| +// immutability the question being asked is "are these the same bytes of |
| +// declaration", and sorting here would be this function quietly deciding that |
| +// two manifests an operator can see differ are the same release. |
| +// |
| +// Comparing ENCODINGS rather than structs is not a stylistic choice either. |
| +// caps.Set carries an unexported notes slice (the |
| +// unknown-field-inside-a-known-capability lines its decoder collected), so a |
| +// decoded Set is neither ==-comparable nor reflect.DeepEqual-safe against one |
| +// built any other way once those notes are non-empty — two blocks that reach |
| +// the manifest as identical bytes would compare unequal because one of them |
| +// was written with a field this binary ignored. The JSON encoding |
| // carries exactly the fields that reach the manifest and nothing else, and |
| // Set has no custom marshaller, so the encoding is deterministic (struct |
| // field order) and total. A nil block encodes as "null" on both sides, which |
| @@ -576,7 +602,7 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) { |
| var declared *caps.Set |
| if len(meta.Capabilities) > 0 { |
| if err := json.Unmarshal(meta.Capabilities, &declared); err != nil { |
| - jsonError(w, http.StatusBadRequest, err.Error()) |
| + jsonError(w, http.StatusBadRequest, capsRefusal(err)) |
| return |
| } |
| if declared == nil { |
| @@ -584,7 +610,7 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| if err := caps.ValidCapabilities(app, declared); err != nil { |
| - jsonError(w, http.StatusBadRequest, err.Error()) |
| + jsonError(w, http.StatusBadRequest, capsRefusal(err)) |
| return |
| } |
| } |
diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go| index 497ef6d02..d22bef67b 100644 |
| --- a/internal/console/apicli_test.go |
| +++ b/internal/console/apicli_test.go |
| @@ -1115,6 +1115,21 @@ func TestAPIShipConcurrencyLimit(t *testing.T) { |
| t.Fatalf("third concurrent ship error = %q, want it to name the concurrency limit", str(t, out, "error")) |
| } |
| + // Gate ORDER, pinned while the semaphore is still exhausted: a request |
| + // whose capability block is garbage must still be refused 429, not 400. |
| + // The capability decode was added inside the handler, AFTER the member |
| + // gate, the rate limiter and this semaphore — and it has to stay there. A |
| + // validation step that ran before them would let an unauthenticated or |
| + // over-budget caller learn things about this server's capability |
| + // vocabulary by feeding it blocks, and would do the work of decoding |
| + // them, which is precisely what the resource gates exist to refuse. |
| + status, out = postShip(t, fx.ts.URL+"/api/cli/apps/bab/titogo/releases", token, |
| + []shipPart{metaPartCaps(t, "abc1234", "binary", `{"v":1,"daemon":true,"gpu":true}`), |
| + {name: "linux-arm64", body: randomBytes(t, 64)}}) |
| + if status != http.StatusTooManyRequests { |
| + t.Fatalf("bad-capabilities ship under an exhausted semaphore = %d %v, want 429 — the caps decode must sit AFTER the resource gates", status, out) |
| + } |
| + |
| // Unblock the two held-open requests (each now sees an immediately-EOF |
| // body and is refused as an empty upload) so the test doesn't leak |
| // goroutines, and confirm they actually finish. |
| @@ -1201,7 +1216,6 @@ func TestAPIShipRefusesABadCapabilityBlock(t *testing.T) { |
| if _, err := fx.st.CreateAppIn(OpsAccountID, "titogo", "paul@keymail.dev", now); err != nil { |
| t.Fatal(err) |
| } |
| - token := cliToken(t, fx, "paul@keymail.dev", "code-ship-badcaps") |
| payload := randomBytes(t, 128) |
| cases := []struct { |
| @@ -1221,6 +1235,11 @@ func TestAPIShipRefusesABadCapabilityBlock(t *testing.T) { |
| {"db name with a separator", "cap10009", `{"v":1,"data":{"replicate":"platform","dbs":["a/b.db"]}}`, "a/b.db"}, |
| {"explicit null", "cap10010", `null`, "null"}, |
| } |
| + // One token, and the table is kept at or under cliShipBurst (10 ships per |
| + // token per hour) on purpose — an eleventh case would be answered 429 and |
| + // would pin nothing. The non-object shapes live in their own test below, |
| + // with their own token, for exactly that reason. |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-ship-badcaps") |
| for _, c := range cases { |
| t.Run(c.name, func(t *testing.T) { |
| status, out := postShip(t, fx.ts.URL+"/api/cli/apps/bab/titogo/releases", token, |
| @@ -1228,9 +1247,57 @@ func TestAPIShipRefusesABadCapabilityBlock(t *testing.T) { |
| if status != http.StatusBadRequest { |
| t.Fatalf("%s = %d %v, want 400", c.name, status, out) |
| } |
| - if msg := str(t, out, "error"); !strings.Contains(msg, c.names) { |
| + msg := str(t, out, "error") |
| + if !strings.Contains(msg, c.names) { |
| t.Fatalf("%s: refusal %q does not contain %q", c.name, msg, c.names) |
| } |
| + // No refusal on this path may leak an internal Go type name — a |
| + // caller cannot act on one, and it is not this API's vocabulary. |
| + if strings.Contains(msg, "caps.Set") || strings.Contains(msg, "json:") { |
| + t.Fatalf("%s: refusal %q leaks the Go decoder's own message", c.name, msg) |
| + } |
| + if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "titogo", c.version); err == nil { |
| + t.Fatalf("%s: a manifest exists for %s — release.Ship must never have been reached", c.name, c.version) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestAPIShipRefusesANonObjectCapabilityBlock: a "capabilities" value that is |
| +// not an object at all never reaches caps's own vocabulary — encoding/json |
| +// refuses it first, and ITS message names an internal Go type ("json: cannot |
| +// unmarshal number into Go value of type caps.Set"), carries no prefix, and |
| +// tells the caller nothing they can act on. It is replaced with one fixed |
| +// line in the same register as this handler's other 400s ("malformed meta |
| +// part"). Pinned so the raw decoder error can never leak back into the API's |
| +// vocabulary. |
| +func TestAPIShipRefusesANonObjectCapabilityBlock(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "titogo", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-ship-nonobject") |
| + payload := randomBytes(t, 128) |
| + |
| + for _, c := range []struct{ name, version, block string }{ |
| + {"a number", "cap20001", `7`}, |
| + {"a string", "cap20002", `"daemon"`}, |
| + {"an array", "cap20003", `[{"v":1}]`}, |
| + } { |
| + t.Run(c.name, func(t *testing.T) { |
| + status, out := postShip(t, fx.ts.URL+"/api/cli/apps/bab/titogo/releases", token, |
| + []shipPart{metaPartCaps(t, c.version, "binary", c.block), {name: "linux-arm64", body: payload}}) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf("%s = %d %v, want 400", c.name, status, out) |
| + } |
| + msg := str(t, out, "error") |
| + if !strings.Contains(msg, "malformed capabilities block") { |
| + t.Fatalf("%s: refusal = %q, want the fixed malformed-block line", c.name, msg) |
| + } |
| + if strings.Contains(msg, "caps.Set") || strings.Contains(msg, "json:") { |
| + t.Fatalf("%s: refusal %q leaks the Go decoder's own message", c.name, msg) |
| + } |
| if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "titogo", c.version); err == nil { |
| t.Fatalf("%s: a manifest exists for %s — release.Ship must never have been reached", c.name, c.version) |
| } |
| @@ -1341,7 +1408,17 @@ func TestShipCapabilitiesMatchComparesEncodingsNotStructs(t *testing.T) { |
| t.Fatal("the two Sets are DeepEqual — the unexported notes hazard is gone and this test can be simplified") |
| } |
| if !shipCapabilitiesMatch(withNote, plain) { |
| - t.Fatal("shipCapabilitiesMatch called two identical-meaning blocks different — an honest re-ship would 409") |
| + t.Fatal("shipCapabilitiesMatch called two blocks that reach the manifest as identical bytes different — an honest re-ship would 409") |
| + } |
| + // List ORDER is significant, and that is the documented, deliberate |
| + // behaviour: the question under immutability is "is this the same |
| + // declaration", and sorting here would let this function decide that two |
| + // manifests an operator can see differ are the same release. Pinned so a |
| + // future "helpful" sort is a red test rather than a silent loosening. |
| + if shipCapabilitiesMatch( |
| + decode(`{"v":1,"ports":[{"port":25},{"port":587}]}`), |
| + decode(`{"v":1,"ports":[{"port":587},{"port":25}]}`)) { |
| + t.Fatal("reordered ports matched — the comparison must stay order-sensitive") |
| } |
| if !shipCapabilitiesMatch(nil, nil) { |
| t.Fatal("two capability-free manifests must match") |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index 63f6cab48..299463bdc 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -624,19 +624,21 @@ func withCapturedStdout(t *testing.T, buf *bytes.Buffer, fn func()) { |
| <-done |
| } |
| -// TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName is the |
| -// fail-closed direction, and it is the test that matters most in this task. |
| +// adoptRefusesManifest runs the whole fail-closed scenario for a stored |
| +// manifest whose capability block this binary will not accept: a box already |
| +// serving v1, a channel that has moved to v2, and v2's manifest bytes written |
| +// DIRECTLY to the store (bypassing Ship, which is exactly what a newer |
| +// platform binary would leave behind). It asserts the four things that make |
| +// the refusal safe — the live symlink unmoved, nothing staged, no restart, |
| +// the registry unchanged — and returns what the pass printed. |
| // |
| -// The manifest bytes are written DIRECTLY to the store, bypassing Ship — that |
| -// is exactly what a newer platform binary would leave behind after shipping a |
| -// release declaring a capability this binary has never heard of. The box's |
| -// answer must be to refuse the VERSION: nothing staged, the live symlink |
| -// unmoved, one printed line naming the capability and the fix. The box keeps |
| -// serving what it has, which for a daemon is the safe direction; the |
| -// alternative — materializing a unit that is missing something the app says |
| -// it needs, and looking healthy — is the config-that-lies failure mode this |
| -// whole design exists to avoid. |
| -func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) { |
| +// Shared by the two tests below because the two refusals arrive by DIFFERENT |
| +// routes and both have to land here: one from caps.Set's strict decoder |
| +// (json.Unmarshal fails), one from ValidCapabilities (the block decodes |
| +// perfectly and is only then refused). A gate that caught the first and not |
| +// the second would look correct in a single test. |
| +func adoptRefusesManifest(t *testing.T, capsJSON string) string { |
| + t.Helper() |
| s := NewDirStore(t.TempDir()) |
| ctx := context.Background() |
| priv, pub := testKeys(t) |
| @@ -661,8 +663,7 @@ func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) |
| t.Fatalf("Promote v2: %v", err) |
| } |
| // ...and is then replaced by the bytes a NEWER binary would have written: |
| - // same artifacts, same sha, plus a capability from a vocabulary this |
| - // binary does not have. |
| + // same artifacts, same sha, plus a capability block this one refuses. |
| key := ManifestKey("bab", "myapp", "v2") |
| _, etag, err := s.Get(ctx, key) |
| if err != nil { |
| @@ -674,7 +675,7 @@ func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) |
| "version": "v2", |
| "kind": "binary", |
| "artifacts": { "` + adoptPlatform + `": { "file": "` + adoptPlatform + `", "sha256": "` + etagOf(body) + `", "size": ` + strconv.Itoa(len(body)) + ` } }, |
| - "capabilities": { "v": 1, "daemon": true, "gpu": true } |
| + "capabilities": ` + capsJSON + ` |
| }`) |
| if _, err := s.Put(ctx, key, future, etag); err != nil { |
| t.Fatalf("replace v2 manifest: %v", err) |
| @@ -703,7 +704,24 @@ func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) |
| if rt, _ := reg.Get("h.example.com"); rt == nil || rt.Version == "v2" { |
| t.Fatalf("registry recorded v2 as adopted: %+v", rt) |
| } |
| - line := out.String() |
| + return out.String() |
| +} |
| + |
| +// TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName is the |
| +// fail-closed direction, and it is the test that matters most in this task. |
| +// |
| +// The box's answer to a capability name from a vocabulary it does not have |
| +// must be to refuse the VERSION: nothing staged, the live symlink unmoved, |
| +// one printed line naming the capability and the fix. The box keeps serving |
| +// what it has, which for a daemon is the safe direction; the alternative — |
| +// materializing a unit that is missing something the app says it needs, and |
| +// looking healthy — is the config-that-lies failure mode this whole design |
| +// exists to avoid. |
| +// |
| +// This refusal comes from caps.Set's strict DECODER: json.Unmarshal never |
| +// completes, so the manifest never becomes a value at all. |
| +func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) { |
| + line := adoptRefusesManifest(t, `{ "v": 1, "daemon": true, "gpu": true }`) |
| if !strings.Contains(line, "gpu") { |
| t.Fatalf("printed %q, want a line naming the capability", line) |
| } |
| @@ -711,3 +729,20 @@ func TestAdoptRefusesTheVersionWhenAManifestDeclaresAnUnknownName(t *testing.T) |
| t.Fatalf("printed %q, want a line naming the fix", line) |
| } |
| } |
| + |
| +// TestAdoptRefusesTheVersionWhenAManifestIsSemanticallyInvalid closes the |
| +// other half of the gate, and it is the half a decoder-only test would miss: |
| +// this block decodes PERFECTLY — every name is known, "v" is 1, the shape is |
| +// right — and is refused only by ValidCapabilities, on a port that is not a |
| +// capability at all. It is reached because validateManifest runs on every |
| +// GetManifest, not only at ship; delete that one line and this test goes red |
| +// while the unknown-name test above stays green. |
| +func TestAdoptRefusesTheVersionWhenAManifestIsSemanticallyInvalid(t *testing.T) { |
| + line := adoptRefusesManifest(t, `{ "v": 1, "daemon": true, "ports": [ { "port": 8080 } ] }`) |
| + if !strings.Contains(line, "8080") { |
| + t.Fatalf("printed %q, want a line naming the offending port", line) |
| + } |
| + if !strings.Contains(line, "capabilities") { |
| + t.Fatalf("printed %q, want a line naming what was refused", line) |
| + } |
| +} |