carlos / platform Public

console: raise the ship meta budget for notes, share label/notes validation

The meta part's JSON decoder was still capped at a flat 8KiB (apicli.go),
so notes anywhere between 8KiB and the documented MaxNotesLen (16KiB)
truncated mid-JSON and 400'd as "malformed meta part" instead of the
notes-specific refusal — the len(meta.Notes) > MaxNotesLen check was
unreachable for exactly the range it exists to guard. apiShipMetaMaxBytes
(release.MaxNotesLen + 8KiB headroom) replaces the flat limit.

The label/notes bounds and their message text were duplicated between
handleAPIShip and release.validateManifest; both now call one shared
release.ValidateLabelNotes. The label/notes check also moved ahead of the
version mint-or-validate block (still after the kind check, still inside
the ship semaphore) so a label/notes refusal no longer pays for the
mint path's bucket List first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 5b000ee3fdae793b9bd398672293186075c79367 parent 7fcff05

3 files changed, +91 −23

internal/console/apicli.go +29 −15
internal/console/apicli_test.go +43 −3
internal/release/release.go +19 −5

diff --git a/internal/console/apicli.go b/internal/console/apicli.go

index d2da14044..eeb1cc9ec 100644

--- a/internal/console/apicli.go

+++ b/internal/console/apicli.go

@@ -640,6 +640,15 @@ const (

apiShipMaxPartsBinary = 8

)

+// apiShipMetaMaxBytes bounds the "meta" part's own JSON — separately from

+// apiShipMaxBytes, which bounds the whole request (meta plus every

+// artifact). release.MaxNotesLen is the biggest single field meta carries;

+// the extra 8KiB is headroom for everything else in the same object

+// (version, kind, label, and a capability block). Still tiny next to

+// apiShipMaxBytes, and still read inside the same semaphore-bounded

+// handler, so this is a decode-shape bound, not a new resource exposure.

+const apiShipMetaMaxBytes = release.MaxNotesLen + 8<<10

// targetRe validates a binary artifact's part NAME before it is used to

// build a bucket key: release.ArtifactKey joins it straight into the object

// path, so this check is path-traversal defense, not tidiness — a part

@@ -863,12 +872,29 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) {

// specific, actionable thing to be told.

Capabilities json.RawMessage `json:"capabilities,omitempty"`

}

- // The 8KiB limit stands unchanged: a capability block is a few hundred

- // bytes, and the CLI sends the canonical re-marshal, not a file verbatim.

- if err := json.NewDecoder(io.LimitReader(first, 8<<10)).Decode(&meta); err != nil {

+ // The budget is release.MaxNotesLen — the biggest single field the meta

+ // part now carries — plus 8KiB of headroom for everything else in it:

+ // version, kind, label, and a capability block (a few hundred bytes, the

+ // CLI's canonical re-marshal rather than a file verbatim). Before Notes

+ // existed this reader was a flat 8KiB; capping it at that today would

+ // truncate notes anywhere above ~8KiB mid-JSON, so a caller within

+ // release.MaxNotesLen would meet "malformed meta part" instead of the

+ // notes-specific refusal below.

+ if err := json.NewDecoder(io.LimitReader(first, apiShipMetaMaxBytes)).Decode(&meta); err != nil {

jsonError(w, http.StatusBadRequest, "malformed meta part")

return

}

+ if meta.Kind != "binary" && meta.Kind != "static" {

+ jsonError(w, http.StatusBadRequest, `invalid kind (want "binary" or "static")`)

+ return

+ }

+ // Checked before the version is minted or validated: NextIteration below

+ // lists the app's whole releases prefix, and a label/notes refusal should

+ // not pay for that bucket round trip first.

+ if err := release.ValidateLabelNotes(meta.Label, meta.Notes); err != nil {

+ jsonError(w, http.StatusBadRequest, err.Error())

+ return

+ }

ver := strings.TrimSpace(meta.Version)

if ver == "" {

// Mint from the app's version target (release versioning spec): no

@@ -899,18 +925,6 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) {

jsonError(w, http.StatusBadRequest, "invalid version")

return

}

- if meta.Kind != "binary" && meta.Kind != "static" {

- jsonError(w, http.StatusBadRequest, `invalid kind (want "binary" or "static")`)

- return

- }

- if len(meta.Label) > release.MaxLabelLen || strings.ContainsAny(meta.Label, "\r\n") {

- jsonError(w, http.StatusBadRequest, fmt.Sprintf("label: one line, at most %d characters", release.MaxLabelLen))

- return

- }

- if len(meta.Notes) > release.MaxNotesLen {

- jsonError(w, http.StatusBadRequest, fmt.Sprintf("notes: at most %d bytes", release.MaxNotesLen))

- return

- }

// Decoded and validated HERE — before a single artifact part is read, let

// alone buffered — so a bad declaration costs the caller nothing but the

// meta part. release.Ship would reach the same verdict through

diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go

index f73dab91a..1cbc49faa 100644

--- a/internal/console/apicli_test.go

+++ b/internal/console/apicli_test.go

@@ -1662,10 +1662,16 @@ func TestAPIShipLabelLimits(t *testing.T) {

version string

label string

notes string

+ wantErr string // substring the refusal must contain

}{

- {"label too long", "lbl0001", strings.Repeat("x", release.MaxLabelLen+1), ""},

- {"label multi-line", "lbl0002", "line one\nline two", ""},

- {"notes too big", "lbl0003", "", strings.Repeat("x", release.MaxNotesLen+1)},

+ {"label too long", "lbl0001", strings.Repeat("x", release.MaxLabelLen+1), "", "label: one line"},

+ {"label multi-line", "lbl0002", "line one\nline two", "", "label: one line"},

+ // One byte over MaxNotesLen — well inside apiShipMetaMaxBytes, so this

+ // must reach release.ValidateLabelNotes's own refusal rather than

+ // tripping the meta part's outer size limit first (the bug the

+ // apiShipMetaMaxBytes budget exists to avoid: a truncated-JSON

+ // "malformed meta part" instead of the notes-specific message).

+ {"notes too big", "lbl0003", "", strings.Repeat("x", release.MaxNotesLen+1), "notes: at most"},

}

for _, c := range cases {

t.Run(c.name, func(t *testing.T) {

@@ -1674,6 +1680,10 @@ func TestAPIShipLabelLimits(t *testing.T) {

if status != http.StatusBadRequest {

t.Fatalf("%s = %d %v, want 400", c.name, status, out)

}

+ if !strings.Contains(str(t, out, "error"), c.wantErr) {

+ t.Fatalf("%s: error = %q, want it to contain %q (proves the real check was reached, not a truncated meta part)",

+ c.name, str(t, out, "error"), c.wantErr)

+ }

if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "titogo", c.version); err == nil {

t.Fatalf("%s: a manifest was stored despite the refusal", c.name)

}

@@ -1681,6 +1691,36 @@ func TestAPIShipLabelLimits(t *testing.T) {

}

+// TestAPIShipLargeNotesSucceed: notes comfortably within release.MaxNotesLen

+// but well past the meta part's OLD flat 8KiB reader limit must still ship —

+// apiShipMetaMaxBytes exists precisely so a caller inside the documented

+// ceiling never meets "malformed meta part" for a value the spec calls

+// valid.

+func TestAPIShipLargeNotesSucceed(t *testing.T) {

+ fx := newConsoleFixture(t)

+ if _, err := fx.st.CreateAppIn(OpsAccountID, "titogo", "paul@keymail.dev", time.Now()); err != nil {

+ t.Fatal(err)

+ }

+ token := cliToken(t, fx, "paul@keymail.dev", "code-ship-bignotes")

+ notes := strings.Repeat("- fixed a thing\n", 640) // ~10KiB, well past the old 8KiB cap

+ if len(notes) <= 8<<10 || len(notes) > release.MaxNotesLen {

+ t.Fatalf("test fixture notes = %d bytes, want >8KiB and <= MaxNotesLen(%d)", len(notes), release.MaxNotesLen)

+ }

+ status, out := postShip(t, fx.ts.URL+"/api/cli/apps/bab/titogo/releases", token,

+ []shipPart{metaPartFull(t, "big0001", "binary", "", notes), {name: "linux-arm64", body: randomBytes(t, 64)}})

+ if status != http.StatusCreated {

+ t.Fatalf("~10KiB notes ship = %d %v, want 201", status, out)

+ }

+ m, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "titogo", "big0001")

+ if err != nil {

+ t.Fatalf("manifest: %v", err)

+ }

+ if m.Notes != notes {

+ t.Fatalf("stored notes length = %d, want %d (bytes must survive intact)", len(m.Notes), len(notes))

+ }

// TestAPIShipExplicitVersionStillWorks: a meta part naming an explicit

// version and no label/notes ships exactly as it did before this change —

// the mint-or-validate branch must not disturb the existing explicit-version

diff --git a/internal/release/release.go b/internal/release/release.go

index d159061ca..3fc8bee63 100644

--- a/internal/release/release.go

+++ b/internal/release/release.go

@@ -164,6 +164,23 @@ const (

MaxNotesLen = 16 << 10

)

+// ValidateLabelNotes is the one home for the label/notes bounds and their

+// refusal text — called from validateManifest (Ship and every GetManifest

+// read) AND from the console API's handleAPIShip, which must reach the same

+// verdict before a single artifact byte crosses the wire rather than only

+// after release.Ship re-derives it. Keeping the messages here means a future

+// change to either limit, or its wording, has one call site to edit rather

+// than two copies that can drift.

+func ValidateLabelNotes(label, notes string) error {

+ if len(label) > MaxLabelLen || strings.ContainsAny(label, "\r\n") {

+ return fmt.Errorf("label: one line, at most %d characters", MaxLabelLen)

+ }

+ if len(notes) > MaxNotesLen {

+ return fmt.Errorf("notes: at most %d bytes", MaxNotesLen)

+ }

+ return nil

// Bucket keys, account-scoped: app names are unique WITHIN an account, so the

// account sqid is part of every key and the key path itself carries the

// app→account linkage (there is no separate linkage manifest). Channel names

@@ -280,11 +297,8 @@ func validateManifest(m *Manifest) error {

return fmt.Errorf("artifact %s: negative size", k)

}

- if len(m.Label) > MaxLabelLen || strings.ContainsAny(m.Label, "\r\n") {

- return fmt.Errorf("label: one line, at most %d characters", MaxLabelLen)

- }

- if len(m.Notes) > MaxNotesLen {

- return fmt.Errorf("notes: at most %d bytes", MaxNotesLen)

+ if err := ValidateLabelNotes(m.Label, m.Notes); err != nil {

+ return err

}

// LAST, and deliberately the same call on both sides of the trust chain:

// Ship runs validateManifest before writing a byte (a human is present to