fix(admin): KeyViz fan-out recursion guard + cookie whitelist (PR #692 r1)

Three items from round-1 review on PR #692:

- Claude bot P1 / recursion storm: with cookie forwarding now
  letting peer calls succeed, a symmetric --keyvizFanoutNodes
  configuration (every node lists every other) would generate
  O(N^2) HTTP calls per browser poll as each peer recursively
  fanned out. Add a marker header X-Admin-Fanout-Peer that the
  fetcher sets on every peer request; KeyVizHandler.ServeHTTP
  short-circuits its own fan-out when the header is present.
  TestKeyVizHandlerSkipsFanoutForPeerCall pins this with a peer
  stub that fails the test if dialled.

- Gemini security-medium / cookie whitelist: forward only
  admin_session and admin_csrf to peers, not every inbound cookie.
  Operators may have unrelated cookies on the same domain
  (analytics, feature flags, other-app sessions); blasting those
  across the internal network needlessly widens the data exposure.
  attachAdminCookies helper centralises the whitelist.
  TestKeyVizFanoutRunForwardsCookies extended to assert
  unrelated_app_session is dropped while admin_session and
  admin_csrf pass through verbatim.

- Claude bot P2 / nil-guard the test: TestKeyVizFanoutRunForwardsCookies
  now require.NotNil(merged.Fanout) and require.Len(Nodes, 2)
  before indexing Nodes[1], so a regression that produced a nil
  Fanout block surfaces with a precise assertion failure rather
  than a generic nil-pointer panic.

Design doc 3 updated with the whitelist + recursion-guard
behavior; the Phase 2-C MVP now matches what the code actually
does.

Author: bootjp

docs/design/2026_04_27_proposed_keyviz_cluster_fanout.md

@@ -132,12 +132,22 @@ elastickv \
   network); the HTTP client uses `http://`. A follow-up will
   introduce `--keyvizFanoutTLS` once the rest of the admin path
   has TLS too.
-- **Auth (Phase 2-C MVP)**: the aggregator forwards the inbound
-  user's `admin_session` cookie (and any other cookies the request
-  carries) on every peer call. The peer's `SessionAuth` middleware
-  verifies the cookie against its own `--adminSessionSigningKey`;
+- **Auth (Phase 2-C MVP)**: the aggregator forwards a whitelist of
+  the inbound user's cookies (`admin_session` + `admin_csrf` only)
+  on every peer call. The peer's `SessionAuth` middleware verifies
+  `admin_session` against its own `--adminSessionSigningKey`;
   cluster nodes already share that key for HA, so a cookie minted
   on node A is verifiable on node B without any new infrastructure.
+  Unrelated cookies the browser may carry (analytics, feature
+  flags, other-app sessions on the same domain) are dropped at the
+  fan-out boundary so they are not leaked across the internal
+  network (Gemini security-medium on PR #692).
+- **Recursion guard**: peer requests carry an `X-Admin-Fanout-Peer`
+  header. The receiving handler short-circuits its own fan-out
+  when this header is set; without the guard, a symmetric
+  configuration (every node lists every other node) would generate
+  O(N²) HTTP calls per browser poll as each peer recursively
+  fanned out. (Claude bot P1 on PR #692.)
   - The earlier draft of this paragraph said "anonymous on a
     private network" — that was wrong: the receiving side enforces
     session auth, so anonymous calls are rejected with 401 and the

internal/admin/keyviz_fanout.go

@@ -34,6 +34,14 @@ const keyVizFanoutDefaultTimeout = 2 * time.Second
 // misbehaving peer flood operator logs.
 const keyVizPeerErrorBodyLimit = 512
 
+// keyVizFanoutPeerHeader marks a request as originating from another
+// node's fan-out aggregator. The receiving handler skips its own
+// fan-out step when this header is set, breaking the
+// every-node-fans-to-every-node recursion that would otherwise turn
+// a single browser poll into O(N²) peer HTTP calls in a symmetric
+// cluster (Claude bot P1 on PR #692).
+const keyVizFanoutPeerHeader = "X-Admin-Fanout-Peer"
+
 // keyVizPeerResponseBodyLimit caps the JSON body we are willing to
 // decode from a peer. A misbehaving or compromised peer that streams
 // gigabytes back at us would otherwise pin a goroutine on
@@ -190,6 +198,25 @@ func (f *KeyVizFanout) WithTimeout(d time.Duration) *KeyVizFanout {
 	return f
 }
 
+// attachAdminCookies forwards only the admin session and CSRF
+// cookies to a peer request. We whitelist rather than passing every
+// inbound cookie through: a logged-in operator may have unrelated
+// cookies for other services on the same domain (analytics, feature
+// flags, dev-tooling sessions), and the fan-out should not blast
+// those across the cluster's internal network. The peer's
+// SessionAuth middleware only inspects admin_session, and the CSRF
+// double-submit cookie pairs with the X-Admin-CSRF header (which
+// fan-out doesn't send because all peer calls are GETs); the cookie
+// is forwarded for parity with browser-issued requests but not
+// load-bearing. (Gemini security-medium on PR #692.)
+func attachAdminCookies(req *http.Request, cookies []*http.Cookie) {
+	for _, c := range cookies {
+		if c.Name == sessionCookieName || c.Name == csrfCookieName {
+			req.AddCookie(c)
+		}
+	}
+}
+
 // peerResult is the per-peer outcome the goroutine pool collects
 // before the synchronous merge phase. Either matrix is non-nil or
 // err is non-nil; never both.
@@ -300,18 +327,14 @@ func (f *KeyVizFanout) fetchPeer(ctx context.Context, peer string, params keyViz
 		return nil, pkgerrors.Wrap(err, "new request")
 	}
 	req.Header.Set("Accept", "application/json")
-	// Forward the inbound user's admin session cookie so the peer's
-	// SessionAuth middleware sees a valid principal. Without this
-	// the peer rejects every fan-out request with 401 — the missing
-	// piece in the Phase 2-C MVP design (#685 §3 said "anonymous on
-	// a private network" but the receiving side still enforces
-	// session auth, so anonymous calls don't actually go through).
-	// Forwarding the cookie works because all admin nodes share
-	// --adminSessionSigningKey for HA; a cookie minted by node A is
-	// verifiable on node B.
-	for _, c := range cookies {
-		req.AddCookie(c)
-	}
+	// Mark this request as a peer fan-out call so the receiving
+	// handler does not recursively fan out to every other peer —
+	// without this header, a symmetric cluster (every node lists
+	// every other node) generates O(N²) peer HTTP calls per
+	// browser poll. The check on the receiving side is in
+	// KeyVizHandler.ServeHTTP. (Claude bot P1 on PR #692.)
+	req.Header.Set(keyVizFanoutPeerHeader, "1")
+	attachAdminCookies(req, cookies)
 	resp, err := f.client.Do(req)
 	if err != nil {
 		return nil, pkgerrors.Wrap(err, "peer request")

internal/admin/keyviz_fanout_test.go

@@ -160,17 +160,30 @@ func TestMergeKeyVizMatricesDistinctRowsPreserveOrder(t *testing.T) {
 // this, peers reject every fan-out call with 401 missing-session-
 // cookie and the cluster heatmap collapses to "1 of N nodes
 // responded".
+//
+// Pins both halves of the cookie contract:
+//   - admin_session and admin_csrf are forwarded with their values
+//     verbatim.
+//   - Unrelated cookies present on the inbound request are
+//     **dropped** rather than leaked to peer nodes (Gemini
+//     security-medium on PR #692).
+//   - The peer call carries the X-Admin-Fanout-Peer header so the
+//     receiving handler can short-circuit its own fan-out (Claude
+//     bot P1 on PR #692; the receiving check is in
+//     KeyVizHandler.ServeHTTP and is exercised by
+//     TestKeyVizHandlerSkipsFanoutForPeerCall in
+//     keyviz_handler_test.go).
 func TestKeyVizFanoutRunForwardsCookies(t *testing.T) {
 	t.Parallel()
-	var seenCookies [][]*http.Cookie
+	var seenRequests []*http.Request
 	var seenMu sync.Mutex
 	peer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !strings.HasSuffix(r.URL.Path, "/admin/api/v1/keyviz/matrix") {
 			http.NotFound(w, r)
 			return
 		}
 		seenMu.Lock()
-		seenCookies = append(seenCookies, r.Cookies())
+		seenRequests = append(seenRequests, r.Clone(context.Background()))
 		seenMu.Unlock()
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusOK)
@@ -182,19 +195,27 @@ func TestKeyVizFanoutRunForwardsCookies(t *testing.T) {
 	cookies := []*http.Cookie{
 		{Name: "admin_session", Value: "session-token-abc"},
 		{Name: "admin_csrf", Value: "csrf-token-def"},
+		{Name: "unrelated_app_session", Value: "should-not-leak"},
 	}
 	merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, cookies)
+	require.NotNil(t, merged.Fanout, "fan-out result must be present")
+	require.Len(t, merged.Fanout.Nodes, 2, "self + 1 peer = 2 nodes (Claude bot P2 on PR #692)")
 	require.True(t, merged.Fanout.Nodes[1].OK)
+
 	seenMu.Lock()
 	defer seenMu.Unlock()
-	require.Len(t, seenCookies, 1)
-	got := seenCookies[0]
+	require.Len(t, seenRequests, 1)
+	got := seenRequests[0].Cookies()
 	names := make(map[string]string, len(got))
 	for _, c := range got {
 		names[c.Name] = c.Value
 	}
 	require.Equal(t, "session-token-abc", names["admin_session"], "session cookie must be forwarded verbatim")
-	require.Equal(t, "csrf-token-def", names["admin_csrf"], "all inbound cookies forwarded; CSRF is innocuous on a GET")
+	require.Equal(t, "csrf-token-def", names["admin_csrf"], "csrf cookie forwarded; harmless on a GET but kept for parity")
+	require.NotContains(t, names, "unrelated_app_session",
+		"unrelated cookies must be dropped; only admin_session/admin_csrf are whitelisted")
+	require.Equal(t, "1", seenRequests[0].Header.Get(keyVizFanoutPeerHeader),
+		"peer marker header must be set so the receiver short-circuits its own fan-out")
 }
 
 // TestKeyVizFanoutRunSinglePeerOK exercises the end-to-end happy

internal/admin/keyviz_handler.go

@@ -177,12 +177,17 @@ func (h *KeyVizHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	cols := h.source.Snapshot(params.from, params.to)
 	matrix := pivotKeyVizColumns(cols, params.series, params.rows)
 	matrix.GeneratedAt = h.now()
-	if h.fanout != nil {
+	if h.fanout != nil && r.Header.Get(keyVizFanoutPeerHeader) == "" {
 		// Forward the inbound user's cookies so the peer's SessionAuth
 		// middleware sees a valid principal. Cluster nodes share
 		// --adminSessionSigningKey for HA, so a cookie minted on this
 		// node verifies on any peer. nil cookies make peers 401,
 		// which surfaces as ok=false in the per-node status.
+		//
+		// Skip the fan-out when keyVizFanoutPeerHeader is present:
+		// the request is itself a peer call and recursing would
+		// generate O(N²) HTTP calls per browser poll on a symmetric
+		// cluster (Claude bot P1 on PR #692).
 		matrix = h.fanout.Run(r.Context(), params, matrix, r.Cookies())
 	}
 	w.Header().Set("Content-Type", "application/json; charset=utf-8")

internal/admin/keyviz_handler_test.go

@@ -450,3 +450,51 @@ func TestKeyVizHandlerAggregateFallbackWhenTotalZero(t *testing.T) {
 	require.True(t, matrix.Rows[0].Aggregate)
 	require.Equal(t, uint64(2), matrix.Rows[0].RouteCount, "fallback to len(MemberRoutes)")
 }
+
+// TestKeyVizHandlerSkipsFanoutForPeerCall pins the recursion guard
+// added on PR #692 (Claude bot P1): when the inbound request carries
+// the X-Admin-Fanout-Peer header, the handler must serve the local
+// view without invoking its own KeyVizFanout — otherwise a
+// symmetric cluster (every node lists every peer) would generate
+// O(N²) HTTP calls per browser poll. The test uses a recording
+// fanout that fails the test if Run is ever called.
+func TestKeyVizHandlerSkipsFanoutForPeerCall(t *testing.T) {
+	t.Parallel()
+	src := &fakeKeyVizSource{cols: []keyviz.MatrixColumn{
+		{At: time.Unix(1_700_000_000, 0), Rows: []keyviz.MatrixRow{
+			{RouteID: 1, Start: []byte("a"), End: []byte("b"), Writes: 5},
+		}},
+	}}
+	// A real *KeyVizFanout pointed at a peer URL that, if dialled,
+	// would record the call. We assert the call-count stays at zero
+	// because the handler's recursion guard short-circuits before
+	// Run runs the parallel-fetch step.
+	var peerHits int
+	peer := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
+		peerHits++
+	}))
+	defer peer.Close()
+
+	fanout := NewKeyVizFanout("self", []string{peer.URL}).WithHTTPClient(peer.Client())
+	h := NewKeyVizHandler(src).
+		WithClock(func() time.Time { return time.Unix(1_700_000_000, 0).UTC() }).
+		WithFanout(fanout)
+	srv := httptest.NewServer(h)
+	defer srv.Close()
+
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil)
+	require.NoError(t, err)
+	req.Header.Set(keyVizFanoutPeerHeader, "1")
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	var matrix KeyVizMatrix
+	require.NoError(t, json.NewDecoder(resp.Body).Decode(&matrix))
+	require.Len(t, matrix.Rows, 1, "local matrix served directly")
+	require.Nil(t, matrix.Fanout,
+		"peer-marked request must not produce a Fanout block — fan-out was skipped, no aggregator metadata to surface")
+	require.Equal(t, 0, peerHits,
+		"recursion guard violated: handler dialled a peer despite X-Admin-Fanout-Peer being set")
+}