carlos / platform Public

console: tier door fix round — AccountBySqid, fixed rejection message, CSRF/signed-out-POST test coverage

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

Paul Campbell 2 months ago ef659cd620a55711336625e229030b07da2358f9 parent 90428b8

2 files changed, +47 −14

internal/console/economicspass.go +16 −6
internal/console/economicspass_test.go +31 −8

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

index 4706e641b..9e400d3b1 100644

--- a/internal/console/economicspass.go

+++ b/internal/console/economicspass.go

@@ -1245,10 +1245,15 @@ func (s *Server) handleEconomicsBillSave(w http.ResponseWriter, r *http.Request)

// Registered without a method prefix (server.go), same reasoning as

// handleEconomicsScenarioSave: the method check comes FIRST, before the

// session/operator gate, so a wrong-method request is byte-identical to an

-// unknown route. Resolves the target account by sqid regardless of viewer

-// (AccountAnyBySqid) — an operator must be able to set the tier on ANY

-// account, including one the operator does not belong to, unlike every

-// member-facing door which resolves through the caller's own membership.

+// unknown route. Resolves the target account by sqid via AccountBySqid, NOT

+// AccountAnyBySqid: AccountBySqid is already viewer-independent — it

+// filters deleted_at, not membership, so it satisfies "an operator can set

+// the tier on ANY account, including one they don't belong to" just fine —

+// and AccountAnyBySqid's doc comment (store.go) reserves it for

+// materialization's one caller alone. Using AccountBySqid also means a

+// tombstoned account's sqid correctly reports "No such account." instead of

+// a false success that writes an inert tier onto a dead row (review, fix

+// round 1).

func (s *Server) handleEconomicsTierSave(w http.ResponseWriter, r *http.Request) {

if r.Method != http.MethodPost {

http.NotFound(w, r)

@@ -1280,7 +1285,7 @@ func (s *Server) handleEconomicsTierSave(w http.ResponseWriter, r *http.Request)

}

sqid := strings.TrimSpace(r.FormValue("account"))

- acct, _, err := s.store.AccountAnyBySqid(sqid)

+ acct, err := s.store.AccountBySqid(sqid)

if err != nil {

log.Printf("console: economics tier save: lookup %s: %v", sqid, err)

s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not look up the account. Please try again.")

@@ -1293,7 +1298,12 @@ func (s *Server) handleEconomicsTierSave(w http.ResponseWriter, r *http.Request)

tier := r.FormValue("tier")

if err := s.store.SetAccountTier(acct.ID, tier); err != nil {

- back("err", "Invalid tier: "+err.Error())

+ // A fixed message, not err.Error(): sibling doors use human wording,

+ // and echoing arbitrary form input into a Set-Cookie flash risks

+ // exceeding the cookie's size limit and silently dropping the flash

+ // (review, fix round 1). The rejected value still reaches the logs.

+ log.Printf("console: economics tier save: rejected tier %q for %s: %v", tier, sqid, err)

+ back("err", "Tier must be one of: try, pool, hosted, fleet, dedicated — or blank to unset.")

return

}

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

index a1a0134c7..5ceaf12fa 100644

--- a/internal/console/economicspass_test.go

+++ b/internal/console/economicspass_test.go

@@ -619,9 +619,9 @@ func TestAccountTierStore(t *testing.T) {

// TestEconomicsWriteDoorsRejectNonPOST — a wrong-method request must be

// byte-identical to an unknown route, both signed out and signed in as a

// non-operator, before the operator gate, before CSRF. An operator POST

-// resolves the account by sqid (regardless of viewer, via

-// AccountAnyBySqid), sets its tier and redirects to

-// /fleet/economics/accounts.

+// resolves the account by sqid (via AccountBySqid — already

+// viewer-independent, since it filters deleted_at rather than membership),

+// sets its tier and redirects to /fleet/economics/accounts.

func TestEconomicsTierDoor(t *testing.T) {

fx := newConsoleFixture(t)

out := newBrowser(t)

@@ -629,10 +629,18 @@ func TestEconomicsTierDoor(t *testing.T) {

const path = "/fleet/economics/tier"

- // Signed out: GET and POST alike must be the ghost 404.

+ // Signed out: GET is the ghost 404.

if status, _, body := get(t, out, fx.ts.URL+path); status != ghostStatus || body != ghostBody {

t.Fatalf("signed-out GET %s = %d %q, want the same 404 as a missing route (%d %q)", path, status, body, ghostStatus, ghostBody)

}

+ // Signed out: POST is the same ghost 404 too (method check runs before

+ // the session gate, but a signed-out POST must still be indistinguishable

+ // from an unknown route — no session cookie means no CSRF token to send

+ // at all).

+ signedOutForm := url.Values{"csrf": {""}, "account": {fx.sqid}, "tier": {"pool"}}

+ if status, _, body := postForm(t, out, fx.ts.URL+path, signedOutForm); status != ghostStatus || body != ghostBody {

+ t.Fatalf("signed-out POST %s = %d %q, want the same 404 as a missing route (%d %q)", path, status, body, ghostStatus, ghostBody)

+ }

c := signInPaul(t, fx)

csrf := sessionToken(t, fx, c)

@@ -644,8 +652,8 @@ func TestEconomicsTierDoor(t *testing.T) {

// Signed in, not an operator: POST is the same ghost 404 (gate before

// CSRF).

- form := url.Values{"csrf": {csrf}, "account": {fx.sqid}, "tier": {"pool"}}

- if status, _, body := postForm(t, c, fx.ts.URL+path, form); status != ghostStatus || body != ghostBody {

+ opForm := url.Values{"csrf": {csrf}, "account": {fx.sqid}, "tier": {"pool"}}

+ if status, _, body := postForm(t, c, fx.ts.URL+path, opForm); status != ghostStatus || body != ghostBody {

t.Fatalf("non-operator POST %s = %d %q, want the same 404 as a missing route (%d %q)", path, status, body, ghostStatus, ghostBody)

}

@@ -658,12 +666,27 @@ func TestEconomicsTierDoor(t *testing.T) {

t.Fatalf("operator GET %s = %d %q, want the same 404 as a missing route (%d %q)", path, status, body, ghostStatus, ghostBody)

}

+ // Operator POST, wrong CSRF: 403, same shape TestScenarioSaveGateAndValidation

+ // pins for the sibling doors — the gate runs AFTER the operator check,

+ // so this is a real 403, not the ghost 404.

+ badCSRFForm := url.Values{"csrf": {"not-the-real-token"}, "account": {fx.sqid}, "tier": {"pool"}}

+ if status, _, _ := postForm(t, c, fx.ts.URL+path, badCSRFForm); status != http.StatusForbidden {

+ t.Fatalf("operator POST with wrong CSRF = %d, want %d", status, http.StatusForbidden)

+ }

+ tiers, err := fx.st.AccountTiers()

+ if err != nil {

+ t.Fatal(err)

+ }

+ if got := tiers[fx.sqid]; got != "" {

+ t.Fatalf("account tier after a wrong-CSRF POST = %q, want unchanged %q", got, "")

+ }

// Operator POST, happy path: sets the tier and redirects.

- status, loc, _ := postForm(t, c, fx.ts.URL+path, form)

+ status, loc, _ := postForm(t, c, fx.ts.URL+path, opForm)

if status != http.StatusSeeOther || loc != "/fleet/economics/accounts" {

t.Fatalf("operator tier POST = %d -> %q, want 303 -> /fleet/economics/accounts", status, loc)

}

- tiers, err := fx.st.AccountTiers()

+ tiers, err = fx.st.AccountTiers()

if err != nil {

t.Fatal(err)

}