amadan
console/release: fix version-target review findings
Three fixes from task review:
- handleAPITagGet shape-checks {version} with release.ParseSemVer
before it reaches the key builder, matching apiinstances.go's
discipline for {host}: a path-traversal segment (or any non-semver
string) is refused as the same 404 a real miss gets, on every
backend, before GetTag ever runs.
- Every 502 branch in apiversion.go now logs the underlying error
(console: api <door> <acct>/<app>: err) before answering the fixed
caller-facing sentence, matching every other bucket-failure handler
in this package.
- release.SetTarget's not-above-tags refusal is now a wrapped sentinel
(release.ErrTargetNotAboveTags) rather than a bare string. The PUT
handler recognises it (or release.ErrCASConflict) via errors.Is for
409, and treats anything else SetTarget can return as a store
failure: logged, and answered with a fixed 502 rather than the raw
error text at 409 — a transient bucket outage no longer reads as
"your input was wrong" or leaks storage-layer wording to a caller.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+108
−9
internal/console/apiversion.go+41 −6internal/console/apiversion_test.go+55 −2internal/release/target.go+12 −1
diff --git a/internal/console/apiversion.go b/internal/console/apiversion.go| index 8a335dcb1..7ea793304 100644 |
| --- a/internal/console/apiversion.go |
| +++ b/internal/console/apiversion.go |
| @@ -9,6 +9,7 @@ package console |
| import ( |
| "context" |
| "errors" |
| + "log" |
| "net/http" |
| "os" |
| "strings" |
| @@ -29,6 +30,7 @@ func (s *Server) handleAPIVersionTargetGet(w http.ResponseWriter, r *http.Reques |
| defer cancel() |
| vt, _, err := release.GetTarget(ctx, s.cfg.Releases, acct.Sqid, app) |
| if err != nil { |
| + log.Printf("console: api version-target get %s/%s: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not read the version target") |
| return |
| } |
| @@ -47,8 +49,16 @@ func (s *Server) handleAPIVersionTargetGet(w http.ResponseWriter, r *http.Reques |
| // release.SetTarget carries its own two refusals — not a bare semver, and |
| // not above every existing tag — and this handler surfaces both rather than |
| // re-deriving them: a 400 for the shape (this caller's mistake, fixable by |
| -// retyping) and a 409 for the ordering (a fact about the app's history that |
| -// SetTarget's own message names). |
| +// retyping, checked here before SetTarget is even called) and a 409 for the |
| +// ordering (a fact about the app's history that SetTarget's own message |
| +// names, recognised below via errors.Is(release.ErrTargetNotAboveTags) |
| +// rather than a string match). Anything else SetTarget can return — a |
| +// store outage, a lost CAS race against a concurrent writer — is the |
| +// STORE's fault, not the caller's: surfacing it as a 409 with the raw |
| +// error text would tell a caller retrying a good request that their INPUT |
| +// was the problem, and would put storage-layer wording (key names, request |
| +// ids) in front of a terminal. Those get logged and answered with the same |
| +// fixed 502 every other bucket failure in this file gets. |
| func (s *Server) handleAPIVersionTargetSet(w http.ResponseWriter, r *http.Request) { |
| addr, acct, app, ok := s.apiAccountApp(w, r) |
| if !ok { |
| @@ -75,10 +85,18 @@ func (s *Server) handleAPIVersionTargetSet(w http.ResponseWriter, r *http.Reques |
| return |
| } |
| if err := release.SetTarget(ctx, s.cfg.Releases, acct.Sqid, app, target, addr, time.Now().UTC()); err != nil { |
| - // SetTarget's own message names the offending tag when the target is |
| - // not above every release already cut — as useful over the API as it |
| - // is on the command line, so it passes through verbatim. |
| - jsonError(w, http.StatusConflict, err.Error()) |
| + if errors.Is(err, release.ErrTargetNotAboveTags) || errors.Is(err, release.ErrCASConflict) { |
| + // SetTarget's own message names the offending tag when the |
| + // target is not above every release already cut — as useful |
| + // over the API as it is on the command line, so it passes |
| + // through verbatim. A lost CAS race gets the same status: a |
| + // concurrent write beat this one, which is the caller's cue to |
| + // re-read and retry, exactly like the 409 above. |
| + jsonError(w, http.StatusConflict, err.Error()) |
| + return |
| + } |
| + log.Printf("console: api version-target set %s/%s: %v", acct.Sqid, app, err) |
| + jsonError(w, http.StatusBadGateway, "could not write the version target") |
| return |
| } |
| s.audit(acct, app, addr, "version-target", "version target set to "+target+" (cli)") |
| @@ -112,6 +130,7 @@ func (s *Server) handleAPITagsList(w http.ResponseWriter, r *http.Request) { |
| // refusing over, not hiding" call handleAPIChannels' per-row error |
| // makes differently, because there a partial answer is still useful |
| // and here list order already depends on parsing every row. |
| + log.Printf("console: api tags list %s/%s: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not read the tags") |
| return |
| } |
| @@ -134,6 +153,19 @@ func (s *Server) handleAPITagGet(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| version := r.PathValue("version") |
| + // Shape-checked before the key builder, not after — the same discipline |
| + // apiinstances.go's handleAPIInstanceGet applies to a {host} segment. |
| + // A {version} segment arrives percent-decoded, so "..%2Fversion-target" |
| + // reaches this handler as a path with a separator in it, and TagKey |
| + // would happily join it into a key naming a DIFFERENT object under this |
| + // app's prefix. ListTags already only ever produces semver-parsable tag |
| + // names (tag.go), so a version that fails ParseSemVer can never name a |
| + // real tag regardless of what the store holds — this 404 is correct on |
| + // every backend, not just the ones where the traversal itself would fail. |
| + if _, ok := release.ParseSemVer(version); !ok { |
| + jsonError(w, http.StatusNotFound, "no such tag "+version) |
| + return |
| + } |
| ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second) |
| defer cancel() |
| tag, err := release.GetTag(ctx, s.cfg.Releases, acct.Sqid, app, version) |
| @@ -142,6 +174,7 @@ func (s *Server) handleAPITagGet(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusNotFound, "no such tag "+version) |
| return |
| } |
| + log.Printf("console: api tag get %s/%s %s: %v", acct.Sqid, app, version, err) |
| jsonError(w, http.StatusBadGateway, "could not read tag "+version) |
| return |
| } |
| @@ -166,11 +199,13 @@ func (s *Server) handleAPIReleasesList(w http.ResponseWriter, r *http.Request) { |
| defer cancel() |
| versions, err := Versions(s.cfg.Releases, acct.Sqid, app) |
| if err != nil { |
| + log.Printf("console: api releases list %s/%s: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not read the releases") |
| return |
| } |
| tags, err := release.ListTags(ctx, s.cfg.Releases, acct.Sqid, app) |
| if err != nil { |
| + log.Printf("console: api releases list %s/%s: tags: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not read the tags") |
| return |
| } |
diff --git a/internal/console/apiversion_test.go b/internal/console/apiversion_test.go| index 0eb4dbbd1..d4364a256 100644 |
| --- a/internal/console/apiversion_test.go |
| +++ b/internal/console/apiversion_test.go |
| @@ -8,7 +8,9 @@ package console |
| import ( |
| "context" |
| + "errors" |
| "net/http" |
| + "strings" |
| "testing" |
| "time" |
| @@ -89,8 +91,8 @@ func TestAPIVersionTargetGetSet(t *testing.T) { |
| if status != http.StatusConflict { |
| t.Fatalf("put not-above-tags = %d %v, want 409", status, out) |
| } |
| - if msg := str(t, out, "error"); msg == "" { |
| - t.Fatal("409 body carries no error message") |
| + if msg := str(t, out, "error"); !strings.Contains(msg, "already released as tag") { |
| + t.Fatalf("409 body = %q, want it to contain %q", msg, "already released as tag") |
| } |
| // Non-member: byte-identical 404 to an unknown app. |
| @@ -105,6 +107,44 @@ func TestAPIVersionTargetGetSet(t *testing.T) { |
| } |
| } |
| +// brokenTargetStore fails every write to the version-target object — a |
| +// bucket outage, not a bad request. Everything else passes through |
| +// unchanged, so a GET (which reads a different key) is unaffected. |
| +type brokenTargetStore struct{ release.Store } |
| + |
| +const brokenTargetStoreErr = "AccessDenied: s3://carlos-prod-secret-bucket/version-target request id ABC123" |
| + |
| +func (b brokenTargetStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) { |
| + if strings.HasSuffix(key, "version-target.json") { |
| + return "", errors.New(brokenTargetStoreErr) |
| + } |
| + return b.Store.Put(ctx, key, body, ifMatch) |
| +} |
| + |
| +// TestAPIVersionTargetSetStoreFailure: a write that fails because the STORE |
| +// is broken — not because the caller sent anything wrong — must not be |
| +// reported as a 409 (that status says "your input conflicts with reality", |
| +// which is false here), and must not echo the store's own error text (bucket |
| +// names, request ids) to the caller. |
| +func TestAPIVersionTargetSetStoreFailure(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "km", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-version-target-storefail") |
| + fx.srv.cfg.Releases = brokenTargetStore{fx.rel} |
| + |
| + status, out := putJSON(t, fx.ts.URL+"/api/cli/apps/bab/km/version-target", |
| + map[string]string{"target": "0.5.0"}, token) |
| + if status != http.StatusBadGateway { |
| + t.Fatalf("put on a broken store = %d %v, want 502", status, out) |
| + } |
| + if msg := str(t, out, "error"); strings.Contains(msg, "AccessDenied") || strings.Contains(msg, "carlos-prod-secret-bucket") { |
| + t.Fatalf("502 body leaks the store's own error text: %q", msg) |
| + } |
| +} |
| + |
| // TestAPITagsListAndGet: the tags list omits the changelog, semver-descending |
| // (0.10.0 above 0.6.0, which string order gets wrong); the single-tag door |
| // carries the full changelog; a version never tagged is a 404. |
| @@ -165,6 +205,19 @@ func TestAPITagsListAndGet(t *testing.T) { |
| if status != http.StatusNotFound { |
| t.Fatalf("get missing tag = %d %v, want 404", status, missOut) |
| } |
| + |
| + // A version that isn't a bare semver at all — including a path-traversal |
| + // attempt — is refused before the key builder ever runs: ListTags only |
| + // ever produces semver-parsable names, so neither string could ever name |
| + // a real tag, on any backend. |
| + status, traversalOut := getRaw(t, fx.ts.URL+"/api/cli/apps/bab/km/tags/..%2Fversion-target", token) |
| + if status != http.StatusNotFound { |
| + t.Fatalf("get tags/..%%2Fversion-target = %d %v, want 404", status, traversalOut) |
| + } |
| + status, nonSemverOut := getJSON(t, fx.ts.URL+"/api/cli/apps/bab/km/tags/not-semver", token) |
| + if status != http.StatusNotFound { |
| + t.Fatalf("get tags/not-semver = %d %v, want 404", status, nonSemverOut) |
| + } |
| } |
| // TestAPIReleasesList: Versions() joined against ListTags — the release a |
diff --git a/internal/release/target.go b/internal/release/target.go| index 0140c10d4..3ab77cfe8 100644 |
| --- a/internal/release/target.go |
| +++ b/internal/release/target.go |
| @@ -15,6 +15,17 @@ import ( |
| "time" |
| ) |
| +// ErrTargetNotAboveTags is SetTarget's ordering refusal, wrapped (not |
| +// returned bare) so a caller can tell it apart from every other way |
| +// SetTarget can fail — a store outage, say — with errors.Is, while the |
| +// human-facing message (which names the colliding tag) still comes through |
| +// on Error(). The console API is exactly that caller: SetTarget's shape |
| +// refusal (400, the caller mistyped) and this ordering refusal (409, a fact |
| +// about the app's own history) are a caller's fault either way, but |
| +// anything else SetTarget can return is the STORE's fault and must not |
| +// share a status code with either. |
| +var ErrTargetNotAboveTags = errors.New("target must be greater than every existing tag") |
| + |
| // VersionTarget is the bucket object at TargetKey. |
| type VersionTarget struct { |
| Schema int `json:"schema"` // 1 |
| @@ -66,7 +77,7 @@ func SetTarget(ctx context.Context, s Store, acct, app, target, actor string, no |
| } |
| for _, tag := range tags { |
| if tv, ok := ParseSemVer(tag.Version); ok && t.Compare(tv) <= 0 { |
| - return fmt.Errorf("version target %s: already released as tag %s — the target must be greater", target, tag.Version) |
| + return fmt.Errorf("version target %s: already released as tag %s — %w", target, tag.Version, ErrTargetNotAboveTags) |
| } |
| } |
| cur, etag, err := GetTarget(ctx, s, acct, app) |