amadan
console: pin the system gate's refusals on the upload paths and through a store fault (#176 review)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
From the Opus review of b871f959 (no HIGH or MEDIUM). Unlisted callers get the bare 404, not the owner-only 403, on PUT system/upload and on an update naming an upload handle. A Role() failure admits nobody: an unlisted owner gets 404, and a listed operator still gets in as an operator. Two comment fixes: the ops account cannot be deleted, and the file header now says when a 403 is possible.
Agent notes
The store fault is simulated by renaming the memberships table after the tokens are minted. Mutating the gate to admit on a store error fails TestSystemRollerStoreErrorAdmitsNobody.
3 files changed,
+42
−4
internal/console/apisystemoperators_test.go+37 −0internal/console/apisystemupdate.go+3 −2internal/console/server.go+2 −2
CI passed — run details
diff --git a/internal/console/apisystemoperators_test.go b/internal/console/apisystemoperators_test.go| index f36b3c1f1..5215a2b7f 100644 |
| --- a/internal/console/apisystemoperators_test.go |
| +++ b/internal/console/apisystemoperators_test.go |
| @@ -147,9 +147,46 @@ func TestSystemOperatorListGatesWhoGetsIn(t *testing.T) { |
| if st, body := postRaw(t, fx.ts.URL+"/api/cli/system/rollback", map[string]string{"channel": "canary"}, bill); st != http.StatusNotFound || body != want { |
| t.Errorf("rollback with SystemOperators=%q = %d %q, want 404 %q", list, st, body, want) |
| } |
| + // The owner-only 403 is for callers the gate has ADMITTED. Off the |
| + // list, the two upload paths must answer the same 404 as every other |
| + // door, or the 403's sentence confirms the surface exists. |
| + if st, _ := uploadBuild(t, fx, bill); st != http.StatusNotFound { |
| + t.Errorf("upload with SystemOperators=%q = %d, want 404", list, st) |
| + } |
| + if st, body := postRaw(t, fx.ts.URL+"/api/cli/system/update", map[string]string{"channel": "canary", "upload": "deadbeef"}, bill); st != http.StatusNotFound || body != want { |
| + t.Errorf("update naming an upload with SystemOperators=%q = %d %q, want 404 %q", list, st, body, want) |
| + } |
| } |
| fx.srv.cfg.SystemOperators = []string{" Bill@Keymail.dev "} |
| if st, out := getJSON(t, fx.ts.URL+"/api/cli/system/status", bill); st != http.StatusOK { |
| t.Fatalf("status with a padded, mixed-case entry = %d %v, want 200", st, out) |
| } |
| } |
| + |
| +// TestSystemRollerStoreErrorAdmitsNobody: a failed ownership lookup is |
| +// logged and the list is still consulted, so a listed operator keeps working |
| +// through a store fault. The fault itself must never admit anyone: an owner |
| +// the console cannot see is refused like a stranger, and the listed operator |
| +// gets in as an operator, never as an owner. |
| +func TestSystemRollerStoreErrorAdmitsNobody(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + fx.srv.cfg.SystemOperators = []string{"bill@keymail.dev"} |
| + owner := cliToken(t, fx, "paul@keymail.dev", "code-sysop-err-owner") |
| + bill := cliToken(t, fx, "bill@keymail.dev", "code-sysop-err-bill") |
| + _, want := getRaw(t, fx.ts.URL+"/api/cli/system/no-such-door", owner) |
| + if _, err := fx.st.db.Exec(`ALTER TABLE memberships RENAME TO memberships_gone`); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := fx.st.Role(OpsAccountID, "paul@keymail.dev"); err == nil { |
| + t.Fatal("Role still answers; the fault this test needs did not happen") |
| + } |
| + if st, body := getRaw(t, fx.ts.URL+"/api/cli/system/status", owner); st != http.StatusNotFound || body != want { |
| + t.Errorf("status as an unlisted owner during a store fault = %d %q, want 404 %q", st, body, want) |
| + } |
| + if st, out := getJSON(t, fx.ts.URL+"/api/cli/system/status", bill); st != http.StatusOK { |
| + t.Errorf("status as a listed operator during a store fault = %d %v, want 200", st, out) |
| + } |
| + if st, _ := uploadBuild(t, fx, bill); st != http.StatusForbidden { |
| + t.Errorf("upload as a listed operator during a store fault = %d, want 403 (an operator, not an owner)", st) |
| + } |
| +} |
diff --git a/internal/console/apisystemupdate.go b/internal/console/apisystemupdate.go| index 161a60882..9640e7655 100644 |
| --- a/internal/console/apisystemupdate.go |
| +++ b/internal/console/apisystemupdate.go |
| @@ -25,8 +25,9 @@ import ( |
| // The console half of `carlos system` (spec §5). Every door here is for an |
| // OWNER of the ops account or an address on CARLOS_SYSTEM_OPERATORS, on a |
| // PERSON session, checked on every call (apiSystemRoller); uploading a local |
| -// build is the owner's alone. Refusal is a bare 404 like the operator doors, |
| -// so anyone else learns nothing. The console holds the bucket, the signing key and the egress; |
| +// build is the owner's alone, and an admitted operator who asks for it gets a |
| +// 403 (systemUploadOwnerOnly). A caller the gate refuses gets a bare 404 like |
| +// the operator doors, so anyone else learns nothing. The console holds the bucket, the signing key and the egress; |
| // the operator's laptop holds none of them. |
| const ( |
diff --git a/internal/console/server.go b/internal/console/server.go| index e666e1452..4da840494 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -704,8 +704,8 @@ type Config struct { |
| // status`, `update` from the signed release feed, its job, and |
| // `rollback`. Owners of the ops account have all of that already; this |
| // list is for a person who should roll releases without owning the |
| - // account, which would also hand them member removal, ownership transfer |
| - // and account deletion. Its own list rather than StoreOperators', |
| + // account, which would also hand them member removal and ownership |
| + // transfer. Its own list rather than StoreOperators', |
| // because that one reads tenant bytes through grants and scanning, and |
| // rolling releases should not. |
| // |