amadan

carlos / platform Public

console: fix CLI API account resolution, error-row coverage, rung contract

Review fixes on the CLI API read endpoints (apicli.go):

- apiAccountApp now resolves the {account} segment the same way the CLI's
  own resolveAccount does: an exact sqid match wins outright, a name match
  only applies when no sqid matched, is case-insensitive (EqualFold), and
  an ambiguous name (two of the caller's own accounts sharing it) is
  refused with 409 naming both candidate sqids rather than picked.
- A HasAppIn store error is now logged before folding into the 404 (the
  404 itself is unchanged — gate hygiene stays byte-identical either way).
- handleAPIChannels always emits "rung" (empty string when the channel
  isn't one of the ladder's rungs) instead of omitting the key, so a typed
  client has one shape to parse rather than two.

Tests added: sqid-beats-name, ambiguous-name-refused, case-insensitive-name,
and a channels error-row test (a garbage pointer alongside a good one comes
back as one erroring row, not a failed response). Fixed the vacuous "no
leak" assertion in TestAPIAppsListsMemberships (it checked for the account
sqid "bac" appearing in an app list, where no app is ever named that) to
seed a real app in the other account and check for that name instead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 1d99f8f38a5e9700f375573d1b1345b61149bf25 parent 5856dc5
2 files changed, +222 −24
  • internal/console/apicli.go +44 −8
  • internal/console/apicli_test.go +178 −16
diff --git a/internal/console/apicli.go b/internal/console/apicli.go
index 5ab047c7b..9fcfc3ae4 100644
--- a/internal/console/apicli.go
+++ b/internal/console/apicli.go
@@ -10,6 +10,7 @@ package console
import (
"context"
+ "fmt"
"log"
"net/http"
"strings"
@@ -22,6 +23,13 @@ import (
// segment accepts a sqid or a name, resolved against the caller's OWN
// memberships — resolution and membership proof are deliberately the same
// lookup, so an unknown account and someone else's account are the same 404.
+//
+// Resolution follows the same doctrine as the CLI's own resolveAccount
+// (accounts.go): an exact SQID match wins outright, and only when none
+// matches does a NAME match apply, case-insensitively (EqualFold) — and an
+// ambiguous name (two of the caller's own accounts sharing it) is refused
+// rather than guessed at, because picking the wrong account here would list
+// or promote against the wrong tenant.
func (s *Server) apiAccountApp(w http.ResponseWriter, r *http.Request) (string, *Account, string, bool) {
addr, ok := s.cliUser(w, r)
if !ok {
@@ -36,16 +44,43 @@ func (s *Server) apiAccountApp(w http.ResponseWriter, r *http.Request) (string,
}
var acct *Account
for i := range accts {
- if accts[i].Sqid == seg || accts[i].Name == seg {
+ if accts[i].Sqid == seg {
acct = &accts[i]
break
}
}
+ if acct == nil {
+ var matches []*Account
+ for i := range accts {
+ if strings.EqualFold(accts[i].Name, seg) {
+ matches = append(matches, &accts[i])
+ }
+ }
+ switch len(matches) {
+ case 1:
+ acct = matches[0]
+ case 0:
+ // acct stays nil — the same 404 an unknown or someone-else's
+ // account gets, below.
+ default:
+ sqids := make([]string, len(matches))
+ for i, m := range matches {
+ sqids[i] = m.Sqid
+ }
+ jsonError(w, http.StatusConflict,
+ fmt.Sprintf("account name %q is ambiguous between %s — use a sqid", seg, strings.Join(sqids, ", ")))
+ return "", nil, "", false
+ }
+ }
if acct == nil {
http.NotFound(w, r)
return "", nil, "", false
}
- if has, err := s.store.HasAppIn(acct.ID, app); err != nil || !has {
+ has, err := s.store.HasAppIn(acct.ID, app)
+ if err != nil {
+ log.Printf("console: api has-app %s/%s: %v", acct.Sqid, app, err)
+ }
+ if err != nil || !has {
http.NotFound(w, r)
return "", nil, "", false
}
@@ -113,15 +148,16 @@ func (s *Server) handleAPIChannels(w http.ResponseWriter, r *http.Request) {
out = append(out, map[string]any{"channel": name, "error": err.Error()})
continue
}
- entry := map[string]any{
+ // rung is always present, "" when the channel name isn't one of the
+ // ladder's rungs — a typed client (later tasks) can rely on the key
+ // existing rather than testing for its absence.
+ rung, _ := release.Rung(name)
+ out = append(out, map[string]any{
"channel": name,
"version": cp.Version,
"promoted_at": cp.PromotedAt.UTC().Format(time.RFC3339),
- }
- if rung, ok := release.Rung(name); ok {
- entry["rung"] = rung
- }
- out = append(out, entry)
+ "rung": rung,
+ })
}
writeJSON(w, http.StatusOK, map[string]any{"channels": out})
}
diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go
index 0cda5876a..aa347a4e4 100644
--- a/internal/console/apicli_test.go
+++ b/internal/console/apicli_test.go
@@ -3,15 +3,20 @@ package console
// The CLI's read-only API surface (apicli.go): `carlos apps` and `carlos
// channels` reading through the console instead of the bucket directly. The
// properties that matter, in the order they'd hurt if lost: a token only
-// ever sees ITS OWN memberships, an unknown account and one you don't belong
-// to are the identical 404, and a channel-ladder read mirrors the CLI's own
-// per-row ERROR behaviour rather than failing the whole response.
+// ever sees ITS OWN memberships, account resolution never guesses between
+// candidates, an unknown account and one you don't belong to are the
+// identical 404, and a channel-ladder read mirrors the CLI's own per-row
+// ERROR behaviour rather than failing the whole response.
import (
+ "context"
"net/http"
"strings"
"testing"
"time"
+
+ "github.com/carlosframework/platform/internal/domains"
+ "github.com/carlosframework/platform/internal/release"
)
// getRaw is getJSON's plain-text cousin: some answers here (http.NotFound's
@@ -44,15 +49,18 @@ func getRaw(t *testing.T, u, bearer string) (int, string) {
}
// TestAPIAppsListsMemberships: a token only ever learns about the accounts
-// IT belongs to, and only that account's apps — a second account existing
-// (bac) never leaks into the body.
+// IT belongs to, and only that account's apps — a second account's OWN app
+// (secretapp, claimed in bac) never leaks into bab's list.
func TestAPIAppsListsMemberships(t *testing.T) {
fx := newConsoleFixture(t)
now := time.Now()
- // bac: a second account alice is NOT a member of.
+ // bac: a second account alice is NOT a member of, with its own app.
if err := fx.st.CreateAccount(2, "bac", "widgets", "bob@keymail.dev", now); err != nil {
t.Fatal(err)
}
+ if _, err := fx.st.CreateAppIn(2, "secretapp", "bob@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, now); err != nil {
t.Fatal(err)
}
@@ -79,8 +87,8 @@ func TestAPIAppsListsMemberships(t *testing.T) {
if v == "titogo" {
found = true
}
- if v == "bac" {
- t.Fatalf("apps for bab leaked something from bac: %v", apps)
+ if v == "secretapp" {
+ t.Fatalf("bab's apps leaked bac's app: %v", apps)
}
}
if !found {
@@ -101,8 +109,10 @@ func TestAPIAppsNoToken(t *testing.T) {
}
// TestAPIChannelsHappy: a promoted channel reads back through the API
-// exactly as the CLI's own `carlos channels` reads it — and the account
-// segment works identically whether given as the sqid or the name.
+// exactly as the CLI's own `carlos channels` reads it — the account segment
+// works identically whether given as the sqid or the name, `rung` is always
+// present (empty for a channel outside the ladder's vocabulary), and
+// `promoted_at` is a real RFC3339 timestamp a typed client can parse.
func TestAPIChannelsHappy(t *testing.T) {
fx := newConsoleFixture(t)
now := time.Now()
@@ -111,6 +121,10 @@ func TestAPIChannelsHappy(t *testing.T) {
}
shipVersion(t, fx, "titogo", "abc1234")
seedChannel(t, fx, "titogo", "canary/x", "abc1234")
+ // A second, non-rung channel — "adhoc" matches no ladder rung, so `rung`
+ // must come back "" rather than being omitted.
+ shipVersion(t, fx, "titogo", "def5678")
+ seedChannel(t, fx, "titogo", "adhoc", "def5678")
token := cliToken(t, fx, "paul@keymail.dev", "code-channels-paul")
for _, seg := range []string{"bab", "ops"} {
@@ -119,14 +133,89 @@ func TestAPIChannelsHappy(t *testing.T) {
t.Fatalf("channels (%s) = %d %v", seg, status, out)
}
chans, _ := out["channels"].([]any)
- if len(chans) != 1 {
- t.Fatalf("channels (%s) = %v, want exactly one entry", seg, out["channels"])
+ if len(chans) != 2 {
+ t.Fatalf("channels (%s) = %v, want exactly two entries", seg, out["channels"])
+ }
+ byName := map[string]map[string]any{}
+ for _, c := range chans {
+ m, _ := c.(map[string]any)
+ byName[str(t, m, "channel")] = m
+ }
+ canary, ok := byName["canary/x"]
+ if !ok {
+ t.Fatalf("channels (%s) missing canary/x: %v", seg, chans)
+ }
+ if str(t, canary, "version") != "abc1234" || str(t, canary, "rung") != "canary" {
+ t.Fatalf("canary/x entry (%s) = %v", seg, canary)
+ }
+ if _, err := time.Parse(time.RFC3339, str(t, canary, "promoted_at")); err != nil {
+ t.Fatalf("canary/x promoted_at (%s) = %q, not RFC3339: %v", seg, str(t, canary, "promoted_at"), err)
+ }
+ adhoc, ok := byName["adhoc"]
+ if !ok {
+ t.Fatalf("channels (%s) missing adhoc: %v", seg, chans)
+ }
+ if str(t, adhoc, "version") != "def5678" {
+ t.Fatalf("adhoc entry (%s) = %v", seg, adhoc)
}
- entry, _ := chans[0].(map[string]any)
- if str(t, entry, "channel") != "canary/x" || str(t, entry, "version") != "abc1234" ||
- str(t, entry, "rung") != "canary" || str(t, entry, "promoted_at") == "" {
- t.Fatalf("channels (%s) entry = %v", seg, entry)
+ if v, present := adhoc["rung"]; !present || v != "" {
+ t.Fatalf("adhoc rung (%s) = %v (present=%v), want present and empty", seg, v, present)
}
+ if _, err := time.Parse(time.RFC3339, str(t, adhoc, "promoted_at")); err != nil {
+ t.Fatalf("adhoc promoted_at (%s) = %q, not RFC3339: %v", seg, str(t, adhoc, "promoted_at"), err)
+ }
+ }
+}
+
+// TestAPIChannelsErrorRow: a channel pointer that fails verification becomes
+// one erroring ROW, not a failed response — the good channel alongside it
+// must still read back intact.
+func TestAPIChannelsErrorRow(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)
+ }
+ shipVersion(t, fx, "titogo", "abc1234")
+ seedChannel(t, fx, "titogo", "canary/x", "abc1234")
+ // A garbage pointer at a second channel key — not signed, not even JSON —
+ // planted directly in the bucket the way a corrupted or hand-edited
+ // object would arrive.
+ garbage := []byte("not a valid signed channel pointer")
+ if _, err := fx.rel.Put(context.Background(), release.ChannelKey(release.OpsAccount, "titogo", "broken"), garbage, ""); err != nil {
+ t.Fatal(err)
+ }
+ token := cliToken(t, fx, "paul@keymail.dev", "code-channels-error-row")
+
+ status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/bab/titogo/channels", token)
+ if status != http.StatusOK {
+ t.Fatalf("channels = %d %v, want 200 even with one bad pointer", status, out)
+ }
+ chans, _ := out["channels"].([]any)
+ if len(chans) != 2 {
+ t.Fatalf("channels = %v, want exactly two rows (one good, one broken)", out["channels"])
+ }
+ byName := map[string]map[string]any{}
+ for _, c := range chans {
+ m, _ := c.(map[string]any)
+ byName[str(t, m, "channel")] = m
+ }
+ broken, ok := byName["broken"]
+ if !ok {
+ t.Fatalf("channels missing the broken row: %v", chans)
+ }
+ if str(t, broken, "error") == "" {
+ t.Fatalf("broken row = %v, want a non-empty error", broken)
+ }
+ if _, present := broken["version"]; present {
+ t.Fatalf("broken row = %v, want no version alongside the error", broken)
+ }
+ good, ok := byName["canary/x"]
+ if !ok {
+ t.Fatalf("channels missing the good row: %v", chans)
+ }
+ if str(t, good, "version") != "abc1234" || str(t, good, "rung") != "canary" {
+ t.Fatalf("good row = %v, want it unaffected by the broken one", good)
}
}
@@ -163,3 +252,76 @@ func TestAPIChannelsRefusalMatrix(t *testing.T) {
t.Fatalf("refusal body = %q, want the stdlib http.NotFound output", bodyA)
}
}
+
+// TestAPIAccountSqidBeatsName: a SQID match wins outright, even when the
+// segment given also happens to equal ANOTHER of the caller's accounts'
+// NAME. Picking that other account here would list (or later, act on) the
+// wrong tenant.
+func TestAPIAccountSqidBeatsName(t *testing.T) {
+ fx := newConsoleFixture(t)
+ now := time.Now()
+ // bac's NAME is literally "bab" — the sqid of the ops account. Alice
+ // belongs to both.
+ if err := fx.st.CreateAccount(2, "bac", "bab", "alice@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, now); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := fx.st.CreateAppIn(OpsAccountID, "titogo", "paul@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ token := cliToken(t, fx, "alice@keymail.dev", "code-sqid-beats-name")
+
+ // "bab" as the account segment must resolve to the SQID match (the ops
+ // account, which has titogo) and not the NAME match (bac, which doesn't).
+ status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/bab/titogo/channels", token)
+ if status != http.StatusOK {
+ t.Fatalf("channels = %d %v, want 200 (sqid match, not bac's name match)", status, out)
+ }
+}
+
+// TestAPIAccountAmbiguousNameRefused: two of the caller's own accounts
+// sharing a name is refused with 409 and both candidate sqids named, never
+// silently resolved to one of them.
+func TestAPIAccountAmbiguousNameRefused(t *testing.T) {
+ fx := newConsoleFixture(t)
+ now := time.Now()
+ sqidA, sqidB := domains.Sqid(2), domains.Sqid(3)
+ if err := fx.st.CreateAccount(2, sqidA, "widgets", "alice@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ if err := fx.st.CreateAccount(3, sqidB, "widgets", "alice@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ token := cliToken(t, fx, "alice@keymail.dev", "code-ambiguous-name")
+
+ status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/widgets/anyapp/channels", token)
+ if status != http.StatusConflict {
+ t.Fatalf("ambiguous-name channels = %d %v, want 409", status, out)
+ }
+ msg := str(t, out, "error")
+ if !strings.Contains(msg, sqidA) || !strings.Contains(msg, sqidB) {
+ t.Fatalf("ambiguous-name error = %q, want both %q and %q named", msg, sqidA, sqidB)
+ }
+}
+
+// TestAPIAccountNameMatchIsCaseInsensitive: a unique name match applies
+// EqualFold, matching resolveAccount's own doctrine (accounts.go).
+func TestAPIAccountNameMatchIsCaseInsensitive(t *testing.T) {
+ fx := newConsoleFixture(t)
+ now := time.Now()
+ sqid := domains.Sqid(2)
+ if err := fx.st.CreateAccount(2, sqid, "Widgets", "alice@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := fx.st.CreateAppIn(2, "gizmo", "alice@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ token := cliToken(t, fx, "alice@keymail.dev", "code-case-insensitive-name")
+
+ status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/widgets/gizmo/channels", token)
+ if status != http.StatusOK {
+ t.Fatalf("case-insensitive name match = %d %v, want 200", status, out)
+ }
+}