carlos / platform Public

console: fleet page honesty on read failure; no-store on the token page

Opus review fixes (2 Importants) on the fleet page (Task 9):

1. A heartbeat read that fails (bucket outage) is not the same claim as
   "no box has ever heartbeated" — loadFleetPage's LoadBoxes error branch
   only logged, so every box on the page silently rendered "no heartbeat
   yet" during an outage, an honest-sounding but false positive claim.
   Now sets the flash warning (first-failure-wins, same as the existing
   box-list warn) so the page says "could not read", not "never happened".

2. The rotate response's body carries a bearer token in plaintext, shown
   exactly once — the same shape writeJSON (authcli.go) already protects
   with Cache-Control: no-store on every JSON token response. The HTML
   twin needed the same header: a cached or bfcache/session-restored copy
   of the rendered page is a second life a JSON response never gets.

Tests: TestFleetPageDegradesOnHeartbeatReadFailure (new — listFailStore
swapped in after fleet/box creation, page still renders with the boxes
list intact and the new flash banner present); extended
TestFleetPageRotateShowsTokenOnceWithWarning to assert the no-store header
via a new postFormHeaders test helper.

Paul Campbell 2 months ago 3e11df019bcb5dd7c7e2d91cfb808e656c4b6383 parent 8532a8b

2 files changed, +82 −1

internal/console/fleets.go +17 −0
internal/console/fleets_test.go +65 −1

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

index af307a4dc..7d3a910fa 100644

--- a/internal/console/fleets.go

+++ b/internal/console/fleets.go

@@ -721,6 +721,16 @@ func (s *Server) loadFleetPage(ctx context.Context, reg *registry.Registry, acct

hbs, err := status.LoadBoxes(ctx, release.NewPrefixStore(s.cfg.Releases, "fleets/"+fleet.Name+"/"))

if err != nil {

log.Printf("console: fleet page %s/%s: load heartbeats: %v", acct.Sqid, fleet.Name, err)

+ // A heartbeat read that failed is NOT the same as "no box has

+ // ever heartbeated" — the former is "unknown, could not check",

+ // the latter is a positive claim this page has no business

+ // making when the truth is "S3 is having a moment". Every box

+ // below still renders (degrading softly, per this function's

+ // doc comment), but the flash makes the distinction honest

+ // rather than silently collapsing it into "no heartbeat yet".

+ if warn == "" {

+ warn = "Could not read this fleet's heartbeats."

+ }

} else {

for _, b := range hbs {

heartbeats[b.Label] = b

@@ -859,6 +869,13 @@ func (s *Server) handleFleetRotateBoxPage(w http.ResponseWriter, r *http.Request

p.FlashErr = warn

p.RotatedLabel = label

p.RotatedToken = plaintext

+ // This response carries a bearer credential in its body — the same

+ // reason writeJSON (authcli.go) sets this on every JSON response,

+ // including the rotate/add-box token bodies. A cached or

+ // bfcache/session-restored copy of an HTML page is exactly the kind of

+ // second life a JSON response never gets, which makes the header more

+ // load-bearing here, not less.

+ w.Header().Set("Cache-Control", "no-store")

s.render(w, http.StatusOK, "fleetdetail", p)

}

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

index 4c32b7dd2..6de0f03f8 100644

--- a/internal/console/fleets_test.go

+++ b/internal/console/fleets_test.go

@@ -15,6 +15,7 @@ import (

"context"

"crypto/sha256"

"encoding/hex"

+ "io"

"net/http"

"net/url"

"strings"

@@ -491,6 +492,23 @@ func writeFleetHeartbeat(t *testing.T, fx *consoleFixture, fleet string, b statu

}

+// postFormHeaders is postForm (server_test.go) plus the response headers —

+// needed to assert Cache-Control on the rotate response, which postForm's

+// three-value return has no room for.

+func postFormHeaders(t *testing.T, c *http.Client, u string, form url.Values) (status int, headers http.Header, body string) {

+ t.Helper()

+ res, err := c.PostForm(u, form)

+ if err != nil {

+ t.Fatal(err)

+ }

+ defer res.Body.Close()

+ raw, err := io.ReadAll(res.Body)

+ if err != nil {

+ t.Fatal(err)

+ }

+ return res.StatusCode, res.Header, string(raw)

// TestFleetPageOwnerControlsMemberReadOnly: both a member and the owner see

// the fleet's boxes — including a fresh box's raw metrics, which this page

// (unlike the public fleet status page) is allowed to show — but only the

@@ -615,6 +633,46 @@ func TestFleetPageStaleHeartbeatRendersUnreachable(t *testing.T) {

}

+// TestFleetPageDegradesOnHeartbeatReadFailure: a heartbeat read that FAILS

+// (bucket outage, listFailStore below) is not the same claim as "no box has

+// ever heartbeated" — the page still renders (boxes list, placed apps, the

+// registry rows themselves are all untouched by this failure) but with a

+// flash saying the heartbeats specifically could not be read, rather than

+// silently collapsing "unknown" into the same "no heartbeat yet" wording an

+// honestly-never-joined box gets (TestFleetPageUnjoinedRegistryRowRendersSafely).

+func TestFleetPageDegradesOnHeartbeatReadFailure(t *testing.T) {

+ fx, token := fleetFixture(t, "code-fleetpage-hbfail")

+ if st, body := postJSON(t, fleetsURL(fx, fx.sqid)+"/new",

+ map[string]string{"name": "acme", "bucket": "acme-data"}, token); st != http.StatusOK {

+ t.Fatalf("create fleet = %d %v", st, body)

+ }

+ if st, body := postJSON(t, fleetsURL(fx, fx.sqid)+"/acme/boxes",

+ map[string]string{"label": "pi-1"}, token); st != http.StatusOK {

+ t.Fatalf("add-box = %d %v", st, body)

+ }

+ c := newBrowser(t)

+ signUp(t, fx, c, "code-fleetpage-hbfail-browser")

+ // Swapped in AFTER sign-in and box creation, neither of which touches

+ // the release store — only the page's own heartbeat read (status.LoadBoxes,

+ // via release.NewPrefixStore) should ever see this failure.

+ fx.srv.cfg.Releases = listFailStore{fx.rel}

+ st, _, body := get(t, c, fx.u("/fleets/acme"))

+ if st != http.StatusOK {

+ t.Fatalf("GET /fleets/acme with an unreadable heartbeat store = %d, want 200 (degrade, don't 500):\n%s", st, body)

+ }

+ // html/template escapes the apostrophe (') — match around it rather

+ // than typing the escaped entity into the test.

+ if !strings.Contains(body, "Could not read this fleet") || !strings.Contains(body, "heartbeats.") {

+ t.Fatalf("heartbeat read failure did not surface its own flash — page rendered as if nothing were wrong:\n%s", body)

+ }

+ if !strings.Contains(body, "pi-1") {

+ t.Fatalf("the registry's own box row (unaffected by the heartbeat failure) went missing too:\n%s", body)

+ }

// TestFleetPageUnjoinedRegistryRowRendersSafely is the Task 6 KNOWN

// DEGRADATION carry: a registry box row with no heartbeat that ever joined

// it (pi-2 here — never written at all, standing in for a box heartbeating

@@ -695,7 +753,7 @@ func TestFleetPageRotateShowsTokenOnceWithWarning(t *testing.T) {

signUp(t, fx, c, "code-fleetpage-rotate-browser")

csrf := sessionToken(t, fx, c)

- st, _, body := postForm(t, c, fx.u("/fleets/acme/rotate"), url.Values{"csrf": {csrf}, "label": {"pi-1"}})

+ st, hdr, body := postFormHeaders(t, c, fx.u("/fleets/acme/rotate"), url.Values{"csrf": {csrf}, "label": {"pi-1"}})

if st != http.StatusOK {

t.Fatalf("rotate = %d, want 200 (rendered directly):\n%s", st, body)

}

@@ -705,6 +763,12 @@ func TestFleetPageRotateShowsTokenOnceWithWarning(t *testing.T) {

if !strings.Contains(strings.ToUpper(body), "NOT REVOCATION") {

t.Fatalf("rotate response missing the detach-not-rotate warning:\n%s", body)

}

+ // The response body carries a bearer credential — a cached or

+ // bfcache/session-restored copy must never keep it around, the same

+ // reason writeJSON (authcli.go) sets this on every JSON token response.

+ if got := hdr.Get("Cache-Control"); got != "no-store" {

+ t.Fatalf("rotate response Cache-Control = %q, want %q", got, "no-store")

+ }

reg := openTestReg(t, fx)

boxes, err := reg.ListFleetBoxes("acme")