fix(consent): review follow-ups — preserve replay expiry, align docs/specs, harden tests

- replay re-render keeps the ORIGINAL ExpiresAt (closes the
  indefinite keep-alive primitive a refreshed TTL would hand a
  captured consent blob); pinned by TestConsent_Replay_DoesNotExtendExpiry
- adapt keycloak_e2e tests to the interstitial (build tag hid the
  302 expectations from the default suite)
- specs.md POST /consent block rewritten for the interstitial +
  replay re-render contract; consent_replay error code removed
- replay_detected{kind=consent} semantic drift documented in
  specs/README/configuration/redis-production (counts benign
  double-submits; alert on rate, not ticks)
- stale comments fixed: securityHeaders middleware, Consent godoc,
  consentTmpl CSP attribution, CanonicalCSPHostSource
- consent/interstitial CSP literals share one base (only
  form-action differs); replayNotice + meta-refresh ;-safety
  documented
- new tests: NavError parse-failure fallback, interstitial URL
  escaping round-trip, deny→replay→approve cross-action chain;
  consent-token extraction deduped onto one helper

Author: babs

README.md

@@ -234,7 +234,9 @@ ordering) live in [`specs.md`](./specs.md).
   - `mcp_auth_consent_decisions_total{decision}` — funnel approve/deny
   - `mcp_auth_access_denied_total{reason}` — every denial bucket
   - `mcp_auth_replay_detected_total{kind}` — `code` / `refresh` /
-    `consent` / `callback_state`
+    `consent` / `callback_state`. `consent` also counts benign
+    double-submits (replay answers with a re-rendered consent page,
+    not a 4xx) — alert on sustained rate, not single ticks
   - `mcp_auth_rate_limited_total{endpoint}` — pre-auth httprate 429s
   - `mcp_auth_idp_exchange_throttled_total` — outbound bucket denials
   - `mcp_auth_clients_registered_total`, `mcp_auth_token_seals_total{purpose}`,

config/config.go

@@ -711,10 +711,16 @@ var cspHostSourceHostPort = regexp.MustCompile(
 )
 
 // CanonicalCSPHostSource validates an operator-supplied CSP host-source
-// (scheme://host[:port]) and returns the lower-cased canonical form
-// suitable for emission into the consent page's CSP form-action
-// directive. Returns an error with the offending input quoted on any
-// validation failure.
+// (scheme://host[:port]) and returns the lower-cased canonical form.
+// Returns an error with the offending input quoted on any validation
+// failure.
+//
+// Since the consent interstitial removed form-action origin
+// enumeration, the only caller is the deprecated
+// CSP_FORM_ACTION_EXTRA parse path: the result is validated but
+// discarded (nothing emits it into a header anymore). Kept so a
+// malformed value still fails startup loud instead of silently
+// changing meaning; delete together with CSP_FORM_ACTION_EXTRA.
 //
 // Canonicalisation: scheme and host are ASCII-lower-cased per CSP3
 // §6.7.2.5 (host-source matching is case-insensitive); operators

docs/configuration.md

@@ -160,7 +160,9 @@ sum(mcp_auth_consent_decisions_total{decision="approved"})
     refresh-rotation outcomes.
 - `mcp_auth_replay_detected_total{kind}` — `code` / `refresh` /
   `consent` / `callback_state` replays caught by the Redis-backed
-  store.
+  store. The `consent` kind answers with a re-rendered consent page
+  (200), so it also counts benign double-submits / back-button
+  re-POSTs — not a pure attack signal.
 - `mcp_auth_groups_claim_shape_mismatch_total` — id_token `groups`
   claim failed to decode as `[]string`. **No denial occurs** — user
   is admitted with empty groups; the counter surfaces an IdP schema

docs/redis-production.md

@@ -143,7 +143,9 @@ The proxy's Prom counters surface Redis-related behavior:
   counts 503s returned because Redis errored.
 - `mcp_auth_replay_detected_total{kind="code"|"refresh"}` — counts
   legitimate replay-detection events. A spike may be an attack, or a
-  broken client that retries with the same code.
+  broken client that retries with the same code. The `consent` kind
+  is softer: a replayed consent POST is answered with a re-rendered
+  consent page, so it also counts benign double-submits.
 - `/readyz` on the metrics port — probes Redis with a cached
   `Exists`. Feeds K8s readiness.
 

docs/runbooks/consent-denials.md

@@ -33,6 +33,11 @@ stays clean.
 3. **Automated test rig.** A CI job that drove `/authorize` and
    expected a 302 now sees a 200 HTML page; whatever scraped
    the form the wrong way looks like a denial in the logs.
+   Same trap one step later: `POST /consent` no longer 302s
+   either — approve/deny answer with a 200 navigation
+   interstitial whose `meta http-equiv="refresh"` carries the
+   next URL. Rigs must extract that target, not the `Location`
+   header.
 4. **Consent page rendering broken behind the ingress.** Strict
    CSP or content-rewriting at the L7 hop mangles the form;
    user can't click. Symptom: zero approves, zero denies, just

handlers/consent.go

@@ -46,11 +46,12 @@ type consentPageData struct {
 }
 
 // consentTmpl is the proxy-rendered consent page. Plain HTML, no
-// JavaScript, CSP-tight (default-src 'none' from the
-// security-headers middleware) — the only interactivity is the two
-// submit buttons on the embedded form. Keeping the page free of
-// remote subresources also keeps the operator from having to relax
-// CSP just to render consent.
+// JavaScript, CSP-tight (consentPageCSP, set self-contained by
+// renderConsent — it overrides the security-headers middleware
+// baseline) — the only interactivity is the two submit buttons on
+// the embedded form. Keeping the page free of remote subresources
+// also keeps the operator from having to relax CSP just to render
+// consent.
 var consentTmpl = template.Must(template.New("consent").Parse(`<!doctype html>
 <html lang="en">
 <head>
@@ -127,15 +128,28 @@ var consentTmpl = template.Must(template.New("consent").Parse(`<!doctype html>
 </html>
 `))
 
-// consentPageCSP is the static CSP for the consent page. form-action
-// stays 'self'-only: the approve/deny POST is answered with a 200
-// same-origin interstitial (renderNavInterstitial) rather than a
-// redirect, which terminates Chromium's form-action enforcement of
-// the navigation chain. No IdP / client origin enumeration needed —
-// the header is independent of IdP topology and client redirect
-// targets (previously CSP_FORM_ACTION_EXTRA + per-render widening,
-// both obsoleted by the interstitial).
-const consentPageCSP = "default-src 'none'; style-src 'unsafe-inline'; form-action 'self'; frame-ancestors 'none'; base-uri 'none'"
+// Consent-flow CSP headers, sharing one base so the two
+// security-critical literals cannot drift apart — the single
+// intentional difference is the form-action source.
+//
+// consentPageCSP (form-action 'self'): the approve/deny POST is
+// answered with a 200 same-origin interstitial
+// (renderNavInterstitial) rather than a redirect, which terminates
+// Chromium's form-action enforcement of the navigation chain. No
+// IdP / client origin enumeration needed — the header is
+// independent of IdP topology and client redirect targets
+// (previously CSP_FORM_ACTION_EXTRA + per-render widening, both
+// obsoleted by the interstitial).
+//
+// navInterstitialCSP (form-action 'none'): the interstitial carries
+// no form; meta-refresh / anchor navigation is not governed by any
+// fetch or form-action directive, so everything stays locked down.
+const (
+	cspConsentPrefix   = "default-src 'none'; style-src 'unsafe-inline'; form-action "
+	cspConsentSuffix   = "; frame-ancestors 'none'; base-uri 'none'"
+	consentPageCSP     = cspConsentPrefix + "'self'" + cspConsentSuffix
+	navInterstitialCSP = cspConsentPrefix + "'none'" + cspConsentSuffix
+)
 
 // navInterstitialTmpl carries the user from a /consent form POST to
 // the next location: the IdP authorize URL on approve, the client
@@ -156,6 +170,13 @@ const consentPageCSP = "default-src 'none'; style-src 'unsafe-inline'; form-acti
 // enforcement; the meta refresh then starts a regular navigation
 // that form-action does not govern. Meta refresh needs no
 // JavaScript, so script-src stays 'none'.
+//
+// {{.URL}} in the content attribute relies on the upstream
+// builders for `;`-safety: html/template HTML-escapes the
+// attribute (quotes, <, &) but leaves `;` raw, and the meta-refresh
+// parse algorithm takes everything after "url=" as the URL — safe
+// because both producers (oauth2Cfg.AuthCodeURL, authzErrorURL's
+// q.Encode) percent-encode `;` in query values.
 var navInterstitialTmpl = template.Must(template.New("nav").Parse(`<!doctype html>
 <html lang="en">
 <head>
@@ -186,10 +207,7 @@ func renderNavInterstitial(w http.ResponseWriter, logger *zap.Logger, targetURL
 	// the client's error envelope — never serve it from a cache.
 	w.Header().Set("Cache-Control", "no-store")
 	w.Header().Set("Pragma", "no-cache")
-	// No form on this page; meta-refresh / anchor navigation is not
-	// governed by any fetch or form-action directive, so everything
-	// stays locked down.
-	w.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'; form-action 'none'; frame-ancestors 'none'; base-uri 'none'")
+	w.Header().Set("Content-Security-Policy", navInterstitialCSP)
 	w.WriteHeader(http.StatusOK)
 	if err := navInterstitialTmpl.Execute(w, struct{ URL string }{targetURL}); err != nil {
 		// Body already started — log only.
@@ -223,6 +241,10 @@ func consentNavError(w http.ResponseWriter, logger *zap.Logger, redirectURI, sta
 // already trusted at this point in the flow. The interstitial (not
 // a 302) because this renderer also answers the POST /consent
 // replay path, where a cross-origin redirect would trip form-action.
+//
+// replayNotice=true adds the "previous response already processed"
+// banner — set only by the POST /consent replay re-render; the
+// GET /authorize first render passes false.
 func renderConsent(w http.ResponseWriter, tm *token.Manager, logger *zap.Logger, baseURL, resourceName string, consent sealedConsent, replayNotice bool) {
 	consentToken, err := tm.SealJSON(consent, token.PurposeConsent)
 	if err != nil {
@@ -299,15 +321,17 @@ type ConsentConfig struct {
 //
 // Replays /authorize Phase 3 on approval: opens the sealedConsent,
 // mints the upstream OIDC nonce and PKCE verifier, seals a
-// sealedSession, and 302s to the IdP. The original sealedClient is
-// NOT reopened here — the consent blob carries only the inner
-// client_id UUID, not the sealed registration handle, so a
-// re-validation would have nothing to re-validate against. The
-// audience + TTL + AAD-purpose triple binding on the consent blob
-// is the integrity check.
+// sealedSession, and answers with the navigation interstitial
+// targeting the IdP (see renderNavInterstitial for why not a 302).
+// The original sealedClient is NOT reopened here — the consent blob
+// carries only the inner client_id UUID, not the sealed
+// registration handle, so a re-validation would have nothing to
+// re-validate against. The audience + TTL + AAD-purpose triple
+// binding on the consent blob is the integrity check.
 //
-// On deny: redirects 302 to the user's registered redirect_uri
-// with `error=access_denied` per RFC 6749 §4.1.2.1.
+// On deny: answers with the interstitial targeting the user's
+// registered redirect_uri carrying `error=access_denied` per
+// RFC 6749 §4.1.2.1.
 //
 // CSRF: the sealedConsent itself is the CSRF token (audience- and
 // purpose-bound, 5-min TTL). A POST without a valid consent_token
@@ -416,8 +440,12 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o
 					// above), only its JTI is spent. The fresh token
 					// still requires a new explicit click, so the
 					// single-use guarantee on the *decision* holds.
+					// ExpiresAt is deliberately NOT refreshed: the
+					// consentTTL window counts from the original
+					// /authorize render, otherwise replay→re-render
+					// cycles would keep a captured blob alive
+					// indefinitely.
 					consent.JTI = uuid.New().String()
-					consent.ExpiresAt = time.Now().Add(consentTTL)
 					renderConsent(w, tm, logger, baseURL, cfg.ResourceName, consent, true)
 					return
 				}

handlers/consent_test.go

@@ -511,6 +511,89 @@ func registerClientNamed(t *testing.T, tm *token.Manager, redirectURIs []string,
 	return enc, internalUUID
 }
 
+// TestConsent_NavErrorParseFailureFallback pins consentNavError's
+// fallback: when the sealed redirect_uri does not url.Parse (an
+// invariant breach — DCR validates the shape — but the branch must
+// stay fail-safe), the error envelope is served as proxy-hosted
+// JSON 400 instead of an interstitial targeting a garbage URL.
+func TestConsent_NavErrorParseFailureFallback(t *testing.T) {
+	tm := newTestTokenManager(t)
+	// Space in host fails url.Parse.
+	consentToken := mintConsentToken(t, tm, "https://bad host.example.com/cb", "s")
+
+	form := url.Values{
+		"consent_token": {consentToken},
+		"action":        {"deny"},
+	}
+	req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/consent", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	rr := httptest.NewRecorder()
+	Consent(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), ConsentConfig{})(rr, req)
+
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("want 400 JSON fallback, got %d: %s", rr.Code, rr.Body.String())
+	}
+	var oauthErr OAuthError
+	if err := json.NewDecoder(rr.Body).Decode(&oauthErr); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if oauthErr.Error != "access_denied" {
+		t.Errorf("error = %q, want access_denied", oauthErr.Error)
+	}
+}
+
+// TestConsent_InterstitialEscapesTargetURL pins the escaping
+// round-trip the extraction helpers rely on: the target URL is
+// HTML-attribute-escaped in the raw body (& → &) in BOTH the
+// meta-refresh content attribute and the anchor-href fallback, and
+// unescaping restores a parseable multi-param URL. A regression to
+// template.HTML (raw &) or a divergence between the two attributes
+// fails here.
+func TestConsent_InterstitialEscapesTargetURL(t *testing.T) {
+	tm := newTestTokenManager(t)
+	consentToken := mintConsentToken(t, tm, "https://app.example.com/cb", "s")
+
+	form := url.Values{
+		"consent_token": {consentToken},
+		"action":        {"approve"},
+	}
+	req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/consent", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	rr := httptest.NewRecorder()
+	Consent(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), ConsentConfig{})(rr, req)
+
+	body := rr.Body.String()
+	const marker = `content="0;url=`
+	_, rest, found := strings.Cut(body, marker)
+	if !found {
+		t.Fatalf("meta refresh not found:\n%s", body)
+	}
+	rawAttr, _, found := strings.Cut(rest, `"`)
+	if !found {
+		t.Fatalf("meta refresh close-quote not found")
+	}
+	// The IdP authorize URL always carries multiple query params, so
+	// the escaped form MUST contain & and the raw form must not
+	// leak a bare & into the attribute.
+	if !strings.Contains(rawAttr, "&") {
+		t.Errorf("meta refresh attribute not HTML-escaped (no &): %q", rawAttr)
+	}
+	if strings.Contains(html.UnescapeString(rawAttr), "&") {
+		t.Errorf("double-escaped meta refresh attribute: %q", rawAttr)
+	}
+	u, err := url.Parse(html.UnescapeString(rawAttr))
+	if err != nil {
+		t.Fatalf("unescaped target does not parse: %v", err)
+	}
+	if len(u.Query()) < 2 {
+		t.Errorf("expected multi-param IdP URL, got %q", u.String())
+	}
+	// Anchor fallback must carry the identical escaped URL.
+	if !strings.Contains(body, `<a href="`+rawAttr+`"`) {
+		t.Errorf("anchor href does not match the meta refresh target")
+	}
+}
+
 func mintConsentToken(t *testing.T, tm *token.Manager, redirectURI, state string) string {
 	t.Helper()
 	consent := sealedConsent{