diff --git a/README.md b/README.md index 6d083a5..00a1f40 100644 --- a/README.md +++ b/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}`, diff --git a/config/config.go b/config/config.go index bc4b226..6d60fef 100644 --- a/config/config.go +++ b/config/config.go @@ -181,19 +181,14 @@ type Config struct { // env: UPSTREAM_AUTHORIZATION_HEADER. Treat as a secret in // deployment (mount from a Secret, not a ConfigMap). UpstreamAuthorization string - // CSPFormActionExtra lists additional scheme://host[:port] - // origins appended to the consent page's CSP form-action source - // list, alongside 'self' and the discovered upstream authorize - // endpoint origin. Needed for IdP redirect chains that cross the - // authorize host: Entra B2C (*.b2clogin.com), personal Microsoft - // accounts (login.live.com), federated AD FS (customer-owned - // host), sovereign clouds (login.microsoftonline.us / - // .partner.microsoftonline.cn). Each entry is a single fixed - // origin — wildcards are intentionally not supported because the - // directive's role is an explicit allowlist defense-in-depth - // against HTML injection on the consent page. Empty for the - // common one-host-IdP case. env: CSP_FORM_ACTION_EXTRA, - // comma-separated. + // CSPFormActionExtra is DEPRECATED and ignored. The consent POST + // is now answered with a same-origin navigation interstitial + // (handlers.renderNavInterstitial), which ends Chromium's + // form-action redirect-chain enforcement at the proxy — no IdP or + // client origin needs to be listed, so form-action is 'self'-only + // again. The variable is still parsed and validated so existing + // deployments keep starting; a startup warning flags it for + // removal. env: CSP_FORM_ACTION_EXTRA, comma-separated. CSPFormActionExtra []string // secretWeakWarning is non-empty when TOKEN_SIGNING_SECRET matches // an obvious-weakness pattern (all-same byte, or short repeating @@ -716,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 diff --git a/docs/configuration.md b/docs/configuration.md index 27416ac..6920e69 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -92,7 +92,7 @@ control. | `PKCE_REQUIRED` | `true` | Set `false` for legacy clients that omit PKCE (Cursor, MCP Inspector, ChatGPT). Rejected by `PROD_MODE`. | | `COMPAT_ALLOW_STATELESS` | `false` | Synthesize a server-side `state` on `/authorize` when the client omits it. Strict mode refuses the request; counter `mcp_auth_access_denied_total{reason="state_missing"}` fires either way. Rejected by `PROD_MODE`. | | `RENDER_CONSENT_PAGE` | `true` | Render an explicit proxy-side consent page on `/authorize` so the user sees who's asking and where they'll be redirected before the IdP login. Closes the silent-token-issuance path where a malicious DCR client + an active IdP session = tokens issued without any user interaction. Plain HTML, no JavaScript. Set `false` to fall back to the legacy silent-redirect — only when every caller is non-interactive and known-trusted. | -| `CSP_FORM_ACTION_EXTRA` | (empty) | Comma-separated additional `scheme://host[:port]` origins appended to the consent page's CSP `form-action` source list, alongside `'self'`, the discovered OIDC authorize endpoint, and the current request's validated `redirect_uri` origin (added per render so already-authenticated upstream sessions whose redirect chain stays in one navigation — `POST /consent` → IdP authorize → `/callback` → client `redirect_uri` — are not blocked by Chromium's form-action enforcement at the final hop; the redirect_uri origin is itself filtered through the same CSP3 host-source check below, with a `consent_csp_redirect_uri_skipped` warn when a registered client's host fails the check). Needed when the IdP redirect chain crosses the authorize host: Entra B2C (`tenant.b2clogin.com`), personal Microsoft accounts (`login.live.com`), federated AD FS (customer host), sovereign clouds (`login.microsoftonline.us` / `login.partner.microsoftonline.cn`). Each entry is validated at startup against the CSP3 §2.4 host-source ABNF (stricter than RFC 3986 reg-name — only `ALPHA`/`DIGIT`/`-`/`.` in hostname labels, or `[IPv6]`); paths, queries, fragments, userinfo, wildcards, and sub-delim host characters (`;`, `,`, `&`, `_`, …) are rejected loud so a misconfigured allowlist cannot silently weaken the emitted header. Scheme and host are ASCII-lower-cased on the way in (per CSP3 §6.7.2.5) so the emitted header is greppable in the form an operator typed. Empty for the common one-host-IdP case. | +| `CSP_FORM_ACTION_EXTRA` | (empty) | **Deprecated, ignored.** `POST /consent` is now answered with a same-origin navigation interstitial (200 + meta refresh) instead of a 302, which terminates Chromium's form-action redirect-chain enforcement at the proxy — the consent page's `form-action` is `'self'`-only and no IdP / client origin needs to be enumerated. Still parsed and validated so existing deployments keep starting; a `csp_form_action_extra_deprecated` startup warning fires when set. Remove it from your deployment. | | `OIDC_ALLOW_INSECURE_HTTP` | `false` | Dev-only escape hatch for cleartext `http://` OIDC issuers (Docker Compose Keycloak demo). Rejected when `PROD_MODE=true`. | ## Logging and observability @@ -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 diff --git a/docs/redis-production.md b/docs/redis-production.md index 729f61c..1ad2888 100644 --- a/docs/redis-production.md +++ b/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. diff --git a/docs/runbooks/consent-denials.md b/docs/runbooks/consent-denials.md index 7a788df..78bdce0 100644 --- a/docs/runbooks/consent-denials.md +++ b/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 diff --git a/docs/threat-model.md b/docs/threat-model.md index b0dedbc..13071e5 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -46,13 +46,12 @@ rather than assuming they're already covered. - **Browser-side XSS in the consent page.** The page is JS-free and CSP-locked (`default-src 'none'`, `style-src 'unsafe-inline'`, `script-src` defaults to none, `frame-ancestors 'none'`, - `base-uri 'none'`). `form-action` is widened only to the upstream - IdP origin (from OIDC discovery), any `CSP_FORM_ACTION_EXTRA` - entries, and the current request's validated `redirect_uri` origin - — each of the three filtered through the CSP3 §2.4 host-source - ABNF check so a DCR-registered redirect_uri whose host smuggles a - sub-delim cannot break out of the directive. A browser-engine bug - that escapes contextual HTML escaping is not separately mitigated. + `base-uri 'none'`, `form-action 'self'`). The consent POST is + answered with a same-origin navigation interstitial (200 + meta + refresh), so `form-action` never needs to name the IdP or client + origins — an injected form cannot point anywhere but the proxy + itself. A browser-engine bug that escapes contextual HTML + escaping is not separately mitigated. - **Network-level MITM between proxy and IdP.** TLS verification is on by default in the `oauth2` library; an operator who disables it (or a CA compromise) lets a MITM observe the upstream code diff --git a/handlers/authorize.go b/handlers/authorize.go index d7a61f9..450264c 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -55,14 +55,6 @@ type AuthorizeConfig struct { // name via MCP_RESOURCE_NAME. Falls back to CanonicalResource // when empty. ResourceName string - // CSPFormActionExtra is an operator-supplied list of additional - // scheme://host[:port] origins appended to the consent page's - // form-action source list (alongside 'self' and the discovered - // upstream authorize endpoint origin). Needed for IdP redirect - // chains that cross the authorize host (Entra B2C, federated - // AD FS, personal MS accounts, sovereign clouds). Validated at - // config.Load time. Empty for the common case. - CSPFormActionExtra []string } // Authorize handles GET /authorize (OAuth 2.1 PKCE authorization request). @@ -78,31 +70,6 @@ type AuthorizeConfig struct { // front-loaded above the response_type / resource / PKCE / state // checks. func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *oauth2.Config, authzCfg AuthorizeConfig) http.HandlerFunc { - // Precompute the static form-action source list once at startup. - // The IdP origin in form-action must match the upstream AuthURL - // so the consent POST's 302 to the IdP is not blocked by - // Chromium's form-action redirect-chain enforcement. Extra - // operator-supplied origins (CSP_FORM_ACTION_EXTRA) cover IdP - // topologies whose redirect chain leaves the authorize host - // (Entra B2C, federated AD FS, personal MS accounts, sovereign - // clouds). The client's redirect_uri origin is appended per - // render — see formatConsentCSP. - // - // Skipped entirely when RenderConsentPage is false: the silent - // fork bypasses renderConsent, the precomputed sources are - // unused, and a "consent CSP misconfigured" warn on a deployment - // that doesn't render the consent page would be misleading noise. - var consentCSPSources []string - if authzCfg.RenderConsentPage { - var idpOriginOK bool - consentCSPSources, idpOriginOK = buildConsentCSPSources(oauth2Cfg.Endpoint.AuthURL, authzCfg.CSPFormActionExtra) - if !idpOriginOK { - logger.Warn("consent_csp_idp_origin_missing", - zap.String("auth_url", oauth2Cfg.Endpoint.AuthURL), - zap.String("hint", "consent submit will be blocked in Chromium browsers; verify OIDC discovery"), - ) - } - } return func(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() if rejectRepeatedParams(w, q, @@ -266,7 +233,7 @@ func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg // redirect) replays from POST /consent on approval. if authzCfg.RenderConsentPage { metrics.AuthorizeInitiated.WithLabelValues("consent").Inc() - renderConsent(w, r, tm, logger, baseURL, authzCfg.ResourceName, consentCSPSources, sealedConsent{ + renderConsent(w, tm, logger, baseURL, authzCfg.ResourceName, sealedConsent{ // Per-render JTI: a fresh id every GET /authorize so // back-button = re-consent (each render gets its own // single-use claim slot) rather than dead-state errors. @@ -281,7 +248,7 @@ func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg Typ: token.PurposeConsent, Audience: baseURL, ExpiresAt: time.Now().Add(consentTTL), - }) + }, false) return } diff --git a/handlers/callback.go b/handlers/callback.go index 43bab65..367e310 100644 --- a/handlers/callback.go +++ b/handlers/callback.go @@ -499,11 +499,25 @@ func callbackHandler(tm *token.Manager, logger *zap.Logger, audience string, oau // upstream, so a parse error here is an invariant violation rather // than attacker-controlled input. func redirectAuthzError(w http.ResponseWriter, r *http.Request, redirectURI, state, errCode, errDesc, audience string) { - u, err := url.Parse(redirectURI) + target, err := authzErrorURL(redirectURI, state, errCode, errDesc, audience) if err != nil { writeOAuthError(w, http.StatusBadRequest, errCode, errDesc) return } + http.Redirect(w, r, target, http.StatusFound) +} + +// authzErrorURL builds the RFC 6749 §4.1.2.1 error envelope on the +// client's redirect_uri. Split out of redirectAuthzError so POST +// /consent responses can deliver the same envelope through the +// navigation interstitial instead of a 302 — see +// renderNavInterstitial for why a form POST must not answer with a +// cross-origin redirect. +func authzErrorURL(redirectURI, state, errCode, errDesc, audience string) (string, error) { + u, err := url.Parse(redirectURI) + if err != nil { + return "", err + } q := u.Query() q.Set("error", errCode) if errDesc != "" { @@ -516,5 +530,5 @@ func redirectAuthzError(w http.ResponseWriter, r *http.Request, redirectURI, sta u.RawQuery = q.Encode() u.Fragment = "" u.RawFragment = "" - http.Redirect(w, r, u.String(), http.StatusFound) + return u.String(), nil } diff --git a/handlers/consent.go b/handlers/consent.go index 765d0a4..287f1b6 100644 --- a/handlers/consent.go +++ b/handlers/consent.go @@ -7,11 +7,8 @@ import ( "html/template" "net/http" "net/url" - "slices" - "strings" "time" - "github.com/babs/mcp-auth-proxy/config" "github.com/babs/mcp-auth-proxy/metrics" "github.com/babs/mcp-auth-proxy/replay" "github.com/babs/mcp-auth-proxy/token" @@ -43,14 +40,18 @@ type consentPageData struct { ApproveURL string HasClientName bool HasResourceURI bool + // ReplayNotice is set when this render replaces a rejected + // replayed submit — the user sees why they are being asked again. + ReplayNotice bool } // 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(` @@ -80,11 +81,17 @@ var consentTmpl = template.Must(template.New("consent").Parse(` .approve { background: #18181b; color: #fafafa; border: 1px solid #18181b; } .deny { background: #fafafa; color: #18181b; border: 1px solid #d4d4d8; } .hint { font-size: 0.85rem; color: #52525b; margin-top: 1rem; } + .notice { background: #fef3c7; border: 1px solid #fcd34d; border-radius: 0.5rem; + padding: 0.75rem 1rem; font-size: 0.9rem; }

Authorize this MCP client?

+ {{if .ReplayNotice}} +

Your previous response was already processed. If you + want to authorize this client again, please confirm below.

+ {{end}} {{if .HasClientName}}

{{.ClientName}} is requesting access to {{if .ResourceName}}{{.ResourceName}}{{else}}this MCP service{{end}}.

{{else}} @@ -121,113 +128,105 @@ var consentTmpl = template.Must(template.New("consent").Parse(` `)) -// buildConsentCSPSources returns the static form-action source list -// for the consent page CSP — 'self', the upstream IdP origin (when -// derivable from authURL) and any operator-supplied extras. The -// client's redirect_uri origin is NOT included here; it is appended -// per-render in formatConsentCSP because each consent page is bound -// to one validated redirect_uri. +// 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. // -// form-action is enforced against every URL in the redirect chain -// initiated by a form submit on Blink-based browsers (Chrome, Edge, -// Brave, Opera) — CSP §6.5 "form submission" check, "navigate" -// algorithm. The consent POST 302s to oauth2Cfg.AuthCodeURL(...), -// so 'self' alone blocks the redirect client-side and /callback -// never fires. Firefox and Safari check only the immediate action= -// URL, which is why the bug reproduces only in Chromium. +// 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). // -// authURL is the upstream OIDC authorization endpoint (set from -// discovery at startup). On parse failure / empty value the -// returned source list falls back to 'self' only — the consent page -// will be Chrome-broken but the proxy still serves; a startup log -// emits the warning so deployments do not silently regress. +// 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 +// redirect_uri error envelope on deny / server error. // -// extraOrigins are additional scheme://host[:port] entries appended -// verbatim. Validated at config-load time (config.Load) so they are -// trusted shape-wise here. Needed for IdP topologies whose redirect -// chain crosses the authorize host — Entra B2C → *.b2clogin.com, -// personal MS accounts → login.live.com, federated AD FS → customer -// host, sovereign clouds → cloud-specific login domains. Each extra -// is a single fixed origin, not a wildcard. -func buildConsentCSPSources(authURL string, extraOrigins []string) (sources []string, idpOriginOK bool) { - sources = make([]string, 0, 3+len(extraOrigins)) - sources = append(sources, "'self'") - if authURL != "" { - if u, err := url.Parse(authURL); err == nil && u.Scheme != "" && u.Host != "" { - // Lower-case the canonical form per CSP3 §6.7.2.5 - // (host-source matching is case-insensitive). Keeps - // the emitted header readable when an operator greps - // it, and matches the canonicalisation applied to - // extraOrigins at config.Load time. - sources = append(sources, strings.ToLower(u.Scheme)+"://"+strings.ToLower(u.Host)) - idpOriginOK = true - } +// WHY a 200 page instead of a 302: Chromium enforces the consent +// page's form-action directive against EVERY hop of the redirect +// chain a form submit initiates (CSP §6.5 "form submission" check, +// "navigate" algorithm; Firefox and Safari check only the immediate +// action= URL). With a live IdP session the chain runs POST /consent +// → IdP authorize → /callback → client redirect_uri → any further +// redirects the CLIENT performs (e.g. Power Platform's +// global.consent.azure-apim.net hops onward to a regional UI +// origin). Those client-side hops are unknowable in advance, so no +// form-action source list can ever be complete — enumerating origins +// (#33 IdP extras, #35 redirect_uri) was whack-a-mole. Terminating +// the form navigation at this same-origin 200 ends form-action +// 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(` + + + + + + + Continuing… + + + +

Continuing… If you are not redirected automatically, +click here.

+ + +`)) + +// renderNavInterstitial answers a /consent form POST with the +// same-origin chain-breaking page described on navInterstitialTmpl. +func renderNavInterstitial(w http.ResponseWriter, logger *zap.Logger, targetURL string) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + // The target URL embeds a single-use sealed session (approve) or + // the client's error envelope — never serve it from a cache. + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Pragma", "no-cache") + 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. + logger.Warn("nav_interstitial_execute_failed", zap.Error(err)) } - sources = append(sources, extraOrigins...) - return sources, idpOriginOK } -// formatConsentCSP joins the precomputed source list with the -// per-render redirect_uri origin and returns the full -// Content-Security-Policy header value. -// -// The redirect_uri origin is added because Chromium's form-action -// check covers the *entire* redirect chain a form submit triggers. -// When the user already has a live session with the upstream IdP, -// the chain doesn't terminate at the IdP login page (which would -// end form-action enforcement at a 200 HTML response) — it stays in -// one navigation: POST /consent → IdP authorize 302 → proxy -// /callback 302 → client redirect_uri 302. The final hop crosses -// to the client's origin (e.g. https://claude.ai), and without it -// in form-action Chrome blocks the navigation at that step. The -// error surfaces in DevTools as "Sending form data to /consent -// violates form-action" — misleading; the form's action URL is -// just what Chrome names in the message, the actual block is the -// downstream hop. -// -// redirectURI has already been validated against the registered -// sealedClient.RedirectURIs at /authorize. Skipped when empty, -// when the parsed origin matches an entry already in baseSources -// (same-host loopback / proxy-hosted clients), or when the origin -// would not pass the CSP3 host-source ABNF check. -// -// The host-source check matters because RFC 3986 reg-name allows -// sub-delim characters (`;`, `,`, `&`, `=`) that, while accepted by -// Go's url.Parse, would terminate the form-action directive early -// when emitted into the header — same header-injection foot-gun the -// startup validator on CSP_FORM_ACTION_EXTRA closes for operator -// input. A malicious DCR client cannot weaken its own consent -// page's CSP by registering `https://evil.example;injected/cb`. -// When the redirect_uri host fails the check, the consent page -// renders with the unwidened CSP and a warn fires carrying the -// offending redirect_uri AND the client_id so operators can -// alert/group by client without a cross-grep against -// client_registered. In Chromium this surfaces as the original -// form-action block on the final redirect hop, but no header -// smuggling occurs. -func formatConsentCSP(logger *zap.Logger, baseSources []string, clientID, redirectURI string) string { - sources := baseSources - if redirectURI != "" { - if u, err := url.Parse(redirectURI); err == nil && u.Scheme != "" && u.Host != "" { - origin := strings.ToLower(u.Scheme) + "://" + strings.ToLower(u.Host) - // Skip when already covered by 'self' / IdP origin / - // operator extras (proxy-hosted clients, same-IdP-tenant - // redirects). The dedup keeps the emitted header tight - // and matches the case-folded canonical form above. - if !slices.Contains(baseSources, origin) { - if _, err := config.CanonicalCSPHostSource(origin); err != nil { - logger.Warn("consent_csp_redirect_uri_skipped", - zap.String("client_id", clientID), - zap.String("redirect_uri", redirectURI), - zap.Error(err), - ) - } else { - sources = slices.Concat(baseSources, []string{origin}) - } - } - } +// consentNavError delivers redirectAuthzError's RFC 6749 §4.1.2.1 +// error envelope through the interstitial. Responses to the consent +// form POST must not 302 cross-origin — Chromium would block the +// redirect against the consent page's form-action 'self'. Same +// parse-failure fallback as redirectAuthzError: proxy-hosted JSON. +func consentNavError(w http.ResponseWriter, logger *zap.Logger, redirectURI, state, errCode, errDesc, audience string) { + target, err := authzErrorURL(redirectURI, state, errCode, errDesc, audience) + if err != nil { + writeOAuthError(w, http.StatusBadRequest, errCode, errDesc) + return } - return "default-src 'none'; style-src 'unsafe-inline'; form-action " + strings.Join(sources, " ") + "; frame-ancestors 'none'; base-uri 'none'" + renderNavInterstitial(w, logger, target) } // renderConsent seals the validated /authorize parameters into a @@ -236,15 +235,21 @@ func formatConsentCSP(logger *zap.Logger, baseSources []string, clientID, redire // reopens it, runs the original Phase-3 logic (mint nonce + upstream // PKCE verifier + sealedSession), and redirects to the IdP. // -// On a seal failure we redirect the original error envelope back to -// the registered redirect_uri (server_error, RFC 6749 §4.1.2.1) -// rather than rendering a partial page — the client is already -// trusted at this point in the flow. -func renderConsent(w http.ResponseWriter, r *http.Request, tm *token.Manager, logger *zap.Logger, baseURL, resourceName string, cspSources []string, consent sealedConsent) { +// On a seal failure we deliver the error envelope to the registered +// redirect_uri (server_error, RFC 6749 §4.1.2.1) via the +// interstitial rather than rendering a partial page — the client is +// 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 { logger.Error("consent_seal_failed", zap.Error(err)) - redirectAuthzError(w, r, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) + consentNavError(w, logger, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) return } @@ -258,6 +263,7 @@ func renderConsent(w http.ResponseWriter, r *http.Request, tm *token.Manager, lo RedirectHost: host, ConsentToken: consentToken, ApproveURL: baseURL + "/consent", + ReplayNotice: replayNotice, } w.Header().Set("Content-Type", "text/html; charset=utf-8") @@ -272,13 +278,9 @@ func renderConsent(w http.ResponseWriter, r *http.Request, tm *token.Manager, lo // style-src for this response only; script-src stays default // (none) so the page remains JavaScript-free, and frame-ancestors // stays none so the consent UI cannot be framed by an attacker - // origin. form-action names the upstream IdP origin AND the - // client's redirect_uri origin so the approve POST's redirect - // chain (POST /consent → IdP authorize → /callback → client - // redirect_uri, when the upstream session is already live) is - // not blocked by Chromium's form-action enforcement at the final - // hop. See formatConsentCSP. - w.Header().Set("Content-Security-Policy", formatConsentCSP(logger, cspSources, consent.ClientID, consent.RedirectURI)) + // origin. form-action is 'self'-only — the POST is answered by + // the same-origin interstitial, see consentPageCSP. + w.Header().Set("Content-Security-Policy", consentPageCSP) w.WriteHeader(http.StatusOK) if err := consentTmpl.Execute(w, data); err != nil { // Body already started — log only. @@ -309,21 +311,27 @@ type ConsentConfig struct { // most once. nil = stateless fallback (configured opt-out — the // token is still audience- and TTL-bound). ReplayStore replay.Store + // ResourceName mirrors the AuthorizeConfig field of the same + // name. Needed because a detected replay re-renders the consent + // page (fresh JTI) instead of returning a dead-end 400. + ResourceName string } // Consent handles POST /consent (consent-page approval submit). // // 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 @@ -336,6 +344,15 @@ type ConsentConfig struct { // slot); a stolen consent_token can be POSTed at most once. Empty // JTI (token sealed by an older binary still in flight during // rollout) falls through to the prior stateless behavior. +// +// A detected replay re-renders the consent page with a fresh JTI +// instead of returning a dead-end 400. This loses nothing: the +// protected action is the approval *decision* (the replayed blob +// never auto-approves — a new explicit click is required), and +// /authorize is unauthenticated, so anyone holding the client's +// authorize URL can obtain a fresh consent page anyway. It fixes +// the back-button / double-submit UX where the user's second +// Approve used to land on a JSON error. func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *oauth2.Config, cfg ConsentConfig) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { r.Body = http.MaxBytesReader(w, r.Body, maxBodySize) @@ -417,7 +434,19 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o zap.String("jti", consent.JTI), zap.String("client_id", consent.ClientID), ) - writeOAuthError(w, http.StatusBadRequest, "invalid_request", "consent token already used", "consent_replay") + // Re-render with a fresh single-use slot rather + // than a dead-end 400 — the blob is authentic, + // unexpired and audience-bound (all checked + // 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() + renderConsent(w, tm, logger, baseURL, cfg.ResourceName, consent, true) return } // Reuse the same access_denied{replay_store_unavailable} @@ -442,7 +471,7 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o zap.String("client_id", consent.ClientID), zap.String("client_name", consent.ClientName), ) - redirectAuthzError(w, r, consent.RedirectURI, consent.OriginalState, "access_denied", "user declined to authorize this client", baseURL) + consentNavError(w, logger, consent.RedirectURI, consent.OriginalState, "access_denied", "user declined to authorize this client", baseURL) return } if action != "approve" { @@ -460,7 +489,7 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o // proxy used. nonceBytes := make([]byte, 16) if _, err := rand.Read(nonceBytes); err != nil { - redirectAuthzError(w, r, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) + consentNavError(w, logger, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) return } nonce := hex.EncodeToString(nonceBytes) @@ -498,7 +527,7 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o internalState, err := tm.SealJSON(session, token.PurposeSession) if err != nil { logger.Error("session_seal_failed", zap.Error(err)) - redirectAuthzError(w, r, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) + consentNavError(w, logger, consent.RedirectURI, consent.OriginalState, "server_error", "internal error", baseURL) return } @@ -508,18 +537,22 @@ func Consent(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *o oauth2.S256ChallengeOption(upstreamVerifier), ) - // Counter + log + redirect together — none of the three can - // fail (oauth2Cfg.AuthCodeURL is a string-builder, - // http.Redirect just writes a 302), so the order is - // observability-only. Keeping increment immediately before - // the redirect call keeps the funnel-counter semantics + // Counter + log + interstitial together — none of the three + // can fail (oauth2Cfg.AuthCodeURL is a string-builder, the + // interstitial render only logs on template error), so the + // order is observability-only. Keeping increment immediately + // before the render call keeps the funnel-counter semantics // unambiguous if a future change introduces failure between // these two lines. + // + // 200 interstitial, NOT a 302: see navInterstitialTmpl — a + // redirect here would re-enter Chromium's form-action chain + // enforcement that the interstitial exists to terminate. logger.Info("consent_approved", zap.String("client_id", consent.ClientID), zap.String("client_name", consent.ClientName), ) metrics.ConsentDecisions.WithLabelValues("approved").Inc() - http.Redirect(w, r, authURL, http.StatusFound) + renderNavInterstitial(w, logger, authURL) } } diff --git a/handlers/consent_test.go b/handlers/consent_test.go index 407e0d3..4384c64 100644 --- a/handlers/consent_test.go +++ b/handlers/consent_test.go @@ -3,6 +3,7 @@ package handlers import ( "context" "encoding/json" + "html" "net/http" "net/http/httptest" "net/url" @@ -15,10 +16,35 @@ import ( "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus/testutil" "go.uber.org/zap" - "go.uber.org/zap/zaptest/observer" - "golang.org/x/oauth2" ) +// extractNavTarget pulls the navigation target out of an +// interstitial response (the meta-refresh URL — see +// renderNavInterstitial). Fails the test when the body is not an +// interstitial. html/template escapes the URL into the attribute, so +// the & entities are decoded back before parsing. +func extractNavTarget(t *testing.T, rr *httptest.ResponseRecorder) *url.URL { + t.Helper() + if rr.Code != http.StatusOK { + t.Fatalf("interstitial: want 200, got %d: %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + const marker = `content="0;url=` + _, rest, found := strings.Cut(body, marker) + if !found { + t.Fatalf("meta refresh not found in body:\n%s", body) + } + raw, _, found := strings.Cut(rest, `"`) + if !found { + t.Fatalf("meta refresh close-quote not found") + } + u, err := url.Parse(html.UnescapeString(raw)) + if err != nil { + t.Fatalf("parse nav target %q: %v", raw, err) + } + return u +} + // authorizeConsentEnabled mirrors the production wiring of /authorize // with RenderConsentPage=true. Used to drive the GET /authorize → // HTML consent page path in handler-level tests. @@ -146,13 +172,9 @@ func TestConsent_DenyRedirectsAccessDenied(t *testing.T) { rr := httptest.NewRecorder() Consent(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), ConsentConfig{})(rr, req) - if rr.Code != http.StatusFound { - t.Fatalf("want 302, got %d: %s", rr.Code, rr.Body.String()) - } - loc, err := url.Parse(rr.Header().Get("Location")) - if err != nil { - t.Fatalf("parse Location: %v", err) - } + // Deny answers with the 200 interstitial carrying the error + // envelope on the client redirect_uri — not a 302 (form-action). + loc := extractNavTarget(t, rr) if loc.Query().Get("error") != "access_denied" { t.Errorf("error = %q, want access_denied", loc.Query().Get("error")) } @@ -172,8 +194,9 @@ func TestConsent_DenyRedirectsAccessDenied(t *testing.T) { // TestConsent_ApproveRedirectsToIdP pins the approve path: POST // /consent with action=approve and a valid token replays Phase-3 of -// /authorize and 302s to the upstream IdP authorize endpoint, NOT -// to the client's redirect_uri. Also asserts the sealed session +// /authorize and answers with the navigation interstitial targeting +// the upstream IdP authorize endpoint, NOT the client's +// redirect_uri. Also asserts the sealed session // inherits the consent blob's resource binding (RFC 8707) so a // regression that loses the binding through the consent fork is // caught here rather than far downstream at the bearer middleware. @@ -190,18 +213,17 @@ func TestConsent_ApproveRedirectsToIdP(t *testing.T) { rr := httptest.NewRecorder() Consent(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), ConsentConfig{})(rr, req) - if rr.Code != http.StatusFound { - t.Fatalf("want 302, got %d: %s", rr.Code, rr.Body.String()) - } - loc, err := url.Parse(rr.Header().Get("Location")) - if err != nil { - t.Fatalf("parse Location: %v", err) - } + loc := extractNavTarget(t, rr) // Upstream IdP host comes from testOAuth2Config().Endpoint.AuthURL. // Anything other than the IdP origin (or the client's redirect) // is a bug; check it's the IdP, not the client redirect. if loc.Host == "app.example.com" { - t.Errorf("approve redirected to client redirect_uri, want IdP host") + t.Errorf("approve targeted client redirect_uri, want IdP host") + } + // The interstitial must keep form-action useless to an injected + // form and stay JS-free — pin the locked-down CSP. + if csp := rr.Header().Get("Content-Security-Policy"); !strings.Contains(csp, "form-action 'none'") { + t.Errorf("interstitial CSP missing form-action 'none': %q", csp) } // state on the IdP redirect is the proxy's sealed session blob, // NOT the client's state. The client's state is preserved @@ -266,13 +288,7 @@ func TestConsent_ApproveSvrPKCE_H6(t *testing.T) { rr := httptest.NewRecorder() Consent(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), ConsentConfig{})(rr, req) - if rr.Code != http.StatusFound { - t.Fatalf("want 302, got %d: %s", rr.Code, rr.Body.String()) - } - loc, err := url.Parse(rr.Header().Get("Location")) - if err != nil { - t.Fatalf("parse Location: %v", err) - } + loc := extractNavTarget(t, rr) idpState := loc.Query().Get("state") var sess sealedSession @@ -331,17 +347,15 @@ func TestConsent_RelaxedCSP(t *testing.T) { t.Errorf("consent CSP must NOT relax script-src; the page is JS-free: got %q", csp) } // Other directives that lock down the page must still be present. - // form-action must include both 'self' (the proxy's /consent - // endpoint) AND the upstream IdP origin — Chromium enforces - // form-action against every URL in the redirect chain, so a - // 'self'-only list blocks the consent POST's 302 to the IdP - // client-side. testOAuth2Config()'s AuthURL is - // https://idp.example.com/authorize, so the expected origin is - // https://idp.example.com. + // form-action is 'self'-only: the consent POST is answered by the + // same-origin interstitial, so no IdP or client origin belongs in + // the source list anymore (see consentPageCSP). A regression that + // reintroduces origin enumeration would widen the attack surface + // an HTML injection could exfiltrate the form to. for _, want := range []string{ "default-src 'none'", "frame-ancestors 'none'", - "form-action 'self' https://idp.example.com", + "form-action 'self';", "base-uri 'none'", } { if !strings.Contains(csp, want) { @@ -350,270 +364,6 @@ func TestConsent_RelaxedCSP(t *testing.T) { } } -// TestBuildConsentCSPSources_IdPOrigin pins buildConsentCSPSources's two-mode -// contract: a valid AuthURL widens form-action to include its -// origin (scheme://host[:port]); a missing/invalid AuthURL falls -// back to 'self' only and signals !idpOriginOK so the caller can -// log a startup warning. Non-default ports must be preserved -// (operators self-host IdPs on arbitrary ports). -func TestBuildConsentCSPSources_IdPOrigin(t *testing.T) { - cases := []struct { - name string - authURL string - extra []string - wantFormAct string - wantOriginOK bool - }{ - { - name: "standard https IdP", - authURL: "https://login.microsoftonline.com/abc/oauth2/v2.0/authorize", - wantFormAct: "form-action 'self' https://login.microsoftonline.com", - wantOriginOK: true, - }, - { - name: "non-default port preserved", - authURL: "https://keycloak.example.com:8443/realms/x/protocol/openid-connect/auth", - wantFormAct: "form-action 'self' https://keycloak.example.com:8443", - wantOriginOK: true, - }, - { - name: "empty AuthURL falls back to self", - authURL: "", - wantFormAct: "form-action 'self';", - wantOriginOK: false, - }, - { - name: "malformed AuthURL falls back to self", - authURL: "::not-a-url::", - wantFormAct: "form-action 'self';", - wantOriginOK: false, - }, - { - name: "extra origins appended after IdP origin", - authURL: "https://login.microsoftonline.com/abc/oauth2/v2.0/authorize", - extra: []string{"https://tenant.b2clogin.com", "https://login.live.com"}, - wantFormAct: "form-action 'self' https://login.microsoftonline.com https://tenant.b2clogin.com https://login.live.com", - wantOriginOK: true, - }, - { - name: "extra origins still appended when IdP origin fell back", - authURL: "", - extra: []string{"https://adfs.customer.example"}, - wantFormAct: "form-action 'self' https://adfs.customer.example;", - wantOriginOK: false, - }, - { - // IPv6 hosts must round-trip with brackets intact — - // url.Parse returns Host="[::1]:8443" verbatim, and CSP - // source-list syntax accepts bracketed IPv6 per spec. - name: "IPv6 host preserved with brackets", - authURL: "https://[::1]:8443/authorize", - wantFormAct: "form-action 'self' https://[::1]:8443", - wantOriginOK: true, - }, - { - // http:// is accepted (OIDC dev / Docker-compose Keycloak - // demo) — the gating happens upstream at OIDC discovery - // (OIDC_ALLOW_INSECURE_HTTP), not here. - name: "http scheme accepted", - authURL: "http://keycloak.dev.local:8080/realms/x/protocol/openid-connect/auth", - wantFormAct: "form-action 'self' http://keycloak.dev.local:8080", - wantOriginOK: true, - }, - { - // CSP3 §6.7.2.5 makes host-source matching - // case-insensitive — we lower-case so the emitted header - // reads the way an operator would grep for it. Same - // canonicalisation as config.Load applies to extras. - name: "case canonicalised to lowercase", - authURL: "HTTPS://Foo.Example.COM/Auth", - wantFormAct: "form-action 'self' https://foo.example.com", - wantOriginOK: true, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - sources, ok := buildConsentCSPSources(tc.authURL, tc.extra) - csp := formatConsentCSP(zap.NewNop(), sources, "", "") - if ok != tc.wantOriginOK { - t.Errorf("idpOriginOK = %v, want %v (csp=%q)", ok, tc.wantOriginOK, csp) - } - if !strings.Contains(csp, tc.wantFormAct) { - t.Errorf("CSP missing %q: got %q", tc.wantFormAct, csp) - } - }) - } -} - -// TestFormatConsentCSP_RedirectURIOrigin pins the per-render -// redirect_uri origin append. When the upstream IdP session is -// already live, the consent POST's redirect chain stays in one -// navigation all the way to the client's redirect_uri — without -// that final origin in form-action, Chromium blocks the navigation -// at the last hop (the proxy's /callback 302 to the client) and -// surfaces a misleading "violates form-action 'self' " error -// naming /consent. The chain in this case is: -// -// POST /consent → IdP authorize → /callback → client redirect_uri -// -// when the user must log in, the IdP returns a 200 HTML login page, -// form-action enforcement ends there, and the post-login callback -// → client redirect_uri navigation is no longer governed by the -// consent page's CSP. -func TestFormatConsentCSP_RedirectURIOrigin(t *testing.T) { - base, _ := buildConsentCSPSources("https://login.microsoftonline.com/x/oauth2/v2.0/authorize", nil) - cases := []struct { - name string - redirectURI string - wantSubstr string - wantAbsent string - wantWarn bool - }{ - { - name: "cross-origin client redirect_uri appended", - redirectURI: "https://claude.ai/api/organizations/abc/mcp_callback", - wantSubstr: "form-action 'self' https://login.microsoftonline.com https://claude.ai;", - }, - { - name: "redirect_uri sharing self origin is deduped", - redirectURI: "https://login.microsoftonline.com/somewhere", - wantSubstr: "form-action 'self' https://login.microsoftonline.com;", - wantAbsent: "https://login.microsoftonline.com https://login.microsoftonline.com", - }, - { - name: "loopback redirect with port preserved", - redirectURI: "http://127.0.0.1:51789/oauth/callback", - wantSubstr: "http://127.0.0.1:51789", - }, - { - name: "IPv6 loopback redirect preserves brackets", - redirectURI: "http://[::1]:51789/oauth/callback", - wantSubstr: "http://[::1]:51789", - }, - { - name: "case canonicalised to lowercase", - redirectURI: "HTTPS://Claude.AI/Callback", - wantSubstr: "https://claude.ai", - }, - { - name: "empty redirect_uri leaves CSP unchanged", - redirectURI: "", - wantSubstr: "form-action 'self' https://login.microsoftonline.com;", - }, - { - name: "malformed redirect_uri silently dropped", - redirectURI: "::not-a-url::", - wantSubstr: "form-action 'self' https://login.microsoftonline.com;", - }, - { - // H1 / CSP header-injection defence: a DCR client whose - // host smuggles a sub-delim (legal per RFC 3986 reg-name, - // illegal per CSP3 §2.4 host-source) must NOT widen the - // directive — the offending origin would terminate - // form-action early and inject a bogus directive. - name: "sub-delim host rejected by CSP host-source check", - redirectURI: "https://evil.example;injected/cb", - wantSubstr: "form-action 'self' https://login.microsoftonline.com;", - wantAbsent: "evil.example", - wantWarn: true, - }, - { - // Underscore is valid in DNS resolution but not in CSP - // host-char (ALPHA/DIGIT/'-' only). Same defence as above. - name: "underscore host rejected by CSP host-source check", - redirectURI: "https://host_underscore.example/cb", - wantSubstr: "form-action 'self' https://login.microsoftonline.com;", - wantAbsent: "host_underscore", - wantWarn: true, - }, - } - const testClientID = "client-uuid-under-test" - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - core, logs := observer.New(zap.WarnLevel) - logger := zap.New(core) - csp := formatConsentCSP(logger, base, testClientID, tc.redirectURI) - if !strings.Contains(csp, tc.wantSubstr) { - t.Errorf("CSP missing %q: got %q", tc.wantSubstr, csp) - } - if tc.wantAbsent != "" && strings.Contains(csp, tc.wantAbsent) { - t.Errorf("CSP unexpectedly contained %q: got %q", tc.wantAbsent, csp) - } - warnEntries := logs.FilterMessage("consent_csp_redirect_uri_skipped").All() - if tc.wantWarn && len(warnEntries) == 0 { - t.Errorf("expected consent_csp_redirect_uri_skipped warn, got none (logs=%v)", logs.All()) - } - if !tc.wantWarn && len(warnEntries) != 0 { - t.Errorf("unexpected consent_csp_redirect_uri_skipped warn (logs=%v)", logs.All()) - } - // When the warn fires, both client_id and redirect_uri - // must be present so an operator can alert/group by - // client_id without joining against client_registered. - if tc.wantWarn && len(warnEntries) > 0 { - fields := warnEntries[0].ContextMap() - if got, _ := fields["client_id"].(string); got != testClientID { - t.Errorf("warn client_id = %q, want %q", got, testClientID) - } - if got, _ := fields["redirect_uri"].(string); got != tc.redirectURI { - t.Errorf("warn redirect_uri = %q, want %q", got, tc.redirectURI) - } - } - }) - } -} - -// TestAuthorize_ConsentCSPWarn pins the construction-time gate on -// the `consent_csp_idp_origin_missing` warn: it fires when -// RenderConsentPage=true AND OIDC discovery returned an unusable -// AuthURL (the only state where the consent page will render with a -// fallback CSP that breaks Chromium), and stays silent when the -// consent page is disabled (silent-redirect deployments would never -// hit the broken CSP anyway). Without this gate, every silent-mode -// deployment with an oddly-shaped AuthURL would emit misleading -// warns about a page it does not render. -func TestAuthorize_ConsentCSPWarn(t *testing.T) { - tm := newTestTokenManager(t) - // oauth2.Config with empty AuthURL is the only state buildConsentCSPSources - // flags as !idpOriginOK from real call sites — every other shape - // would have failed OIDC discovery before Authorize is constructed. - brokenOAuth2 := &oauth2.Config{ - ClientID: "test-oidc-client", - ClientSecret: "test-oidc-secret", - RedirectURL: "https://auth.example.com/callback", - Scopes: []string{"openid", "email", "profile"}, - } - cases := []struct { - name string - renderConsentPage bool - wantWarn bool - }{ - {"consent page enabled fires warn", true, true}, - {"consent page disabled stays silent", false, false}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - core, logs := observer.New(zap.WarnLevel) - logger := zap.New(core) - // Construct only — handler not invoked. The gate fires - // at construction so the assertion is purely on the - // captured log buffer. - Authorize(tm, logger, testBaseURL, brokenOAuth2, AuthorizeConfig{ - PKCERequired: true, - ResourceURIs: []string{testBaseURL + "/mcp"}, - CanonicalResource: testBaseURL + "/mcp", - RenderConsentPage: tc.renderConsentPage, - }) - got := logs.FilterMessage("consent_csp_idp_origin_missing").Len() - if tc.wantWarn && got != 1 { - t.Errorf("expected 1 consent_csp_idp_origin_missing warn, got %d entries (all logs: %v)", got, logs.All()) - } - if !tc.wantWarn && got != 0 { - t.Errorf("expected silent-mode to skip the warn, got %d entries (all logs: %v)", got, logs.All()) - } - }) - } -} - // TestConsent_DecisionCounters pins the funnel-counter contract: // every Approve increments mcp_auth_consent_decisions_total{decision= // "approved"}, every Deny increments {decision="denied"}, and Deny @@ -632,8 +382,8 @@ func TestConsent_DecisionCounters(t *testing.T) { 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.StatusFound { - t.Fatalf("%s: want 302, got %d: %s", action, rr.Code, rr.Body.String()) + if rr.Code != http.StatusOK { + t.Fatalf("%s: want 200 interstitial, got %d: %s", action, rr.Code, rr.Body.String()) } } @@ -761,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, `]*\saction=["']([^"']+)["']` // markup. var consentTokenInputRE = regexp.MustCompile(`(?is)]*name=["']consent_token["'][^>]*value=["']([^"']+)["']`) +// navRefreshRE extracts the meta-refresh target from the proxy's +// navigation interstitial (POST /consent answers 200 + meta refresh +// instead of 302 — see handlers.renderNavInterstitial). The captured +// URL is HTML-attribute-escaped; unescape before use. +var navRefreshRE = regexp.MustCompile(`(?is)]*content=["']0;url=([^"']+)["']`) + +// extractNavTarget pulls the navigation target out of an +// interstitial response body. +func extractNavTarget(t *testing.T, resp *http.Response) string { + t.Helper() + page, err := io.ReadAll(io.LimitReader(resp.Body, 256*1024)) + if err != nil { + t.Fatalf("read interstitial page: %v", err) + } + match := navRefreshRE.FindSubmatch(page) + if match == nil { + t.Fatalf("meta refresh not found in interstitial; first 500 bytes: %q", string(page[:min(len(page), 500)])) + } + return html.UnescapeString(string(match[1])) +} + func TestKeycloakE2EFullOAuthFlow(t *testing.T) { proxyBaseURL := envOrDefaultForTest("KEYCLOAK_E2E_PROXY_BASE_URL", "http://localhost:8080") keycloakBrowserBaseURL := envOrDefaultForTest("KEYCLOAK_BROWSER_BASE_URL", "http://localhost:8180") @@ -149,7 +170,8 @@ func authorizeViaKeycloak(t *testing.T, client *http.Client, proxyBaseURL, keycl // approveConsent walks the proxy-rendered consent page on RENDER_CONSENT_PAGE=true: // reads the sealed consent_token from the form, POSTs action=approve to /consent, -// and returns the Location header of the resulting 302 (which points at the IdP). +// and returns the meta-refresh target of the resulting interstitial +// (which points at the IdP). func approveConsent(t *testing.T, client *http.Client, resp *http.Response, proxyBaseURL string) string { t.Helper() consentToken := extractConsentToken(t, resp) @@ -162,10 +184,10 @@ func approveConsent(t *testing.T, client *http.Client, resp *http.Response, prox t.Fatalf("POST /consent: %v", err) } defer consentResp.Body.Close() - if consentResp.StatusCode != http.StatusFound { - t.Fatalf("POST /consent: got %d, want 302: %s", consentResp.StatusCode, readSnippet(consentResp.Body)) + if consentResp.StatusCode != http.StatusOK { + t.Fatalf("POST /consent: got %d, want 200 interstitial: %s", consentResp.StatusCode, readSnippet(consentResp.Body)) } - return consentResp.Header.Get("Location") + return extractNavTarget(t, consentResp) } // extractConsentToken pulls the sealed consent_token value out of @@ -392,9 +414,10 @@ func isRedirect(status int) bool { // helpers with the happy-path test above. // TestKeycloakE2E_ConsentDenied pins the denial branch of the -// proxy-rendered consent page: POST action=deny redirects 302 to -// the registered redirect_uri with error=access_denied per -// RFC 6749 §4.1.2.1, and the IdP login is never reached. +// proxy-rendered consent page: POST action=deny answers with the +// navigation interstitial targeting the registered redirect_uri +// with error=access_denied per RFC 6749 §4.1.2.1, and the IdP +// login is never reached. // // This test diverges before the Keycloak redirect, so it // exercises only the proxy's consent-page flow + chi/middleware @@ -443,16 +466,16 @@ func TestKeycloakE2E_ConsentDenied(t *testing.T) { t.Fatalf("POST /consent action=deny: %v", err) } defer denyResp.Body.Close() - if denyResp.StatusCode != http.StatusFound { - t.Fatalf("deny: want 302, got %d: %s", denyResp.StatusCode, readSnippet(denyResp.Body)) + if denyResp.StatusCode != http.StatusOK { + t.Fatalf("deny: want 200 interstitial, got %d: %s", denyResp.StatusCode, readSnippet(denyResp.Body)) } - loc := denyResp.Header.Get("Location") + loc := extractNavTarget(t, denyResp) if !strings.HasPrefix(loc, redirectURI) { - t.Fatalf("deny redirect Location = %q, want prefix %q", loc, redirectURI) + t.Fatalf("deny nav target = %q, want prefix %q", loc, redirectURI) } u, err := url.Parse(loc) if err != nil { - t.Fatalf("parse deny Location: %v", err) + t.Fatalf("parse deny nav target: %v", err) } if got := u.Query().Get("error"); got != "access_denied" { t.Errorf("error = %q, want access_denied", got) diff --git a/main.go b/main.go index 3c9dff3..e0473ff 100644 --- a/main.go +++ b/main.go @@ -66,6 +66,12 @@ func main() { zap.String("project_url", ProjectURL), ) + if len(cfg.CSPFormActionExtra) > 0 { + logger.Warn("csp_form_action_extra_deprecated", + zap.String("hint", "ignored since the consent interstitial: form-action is 'self'-only; remove CSP_FORM_ACTION_EXTRA from the deployment"), + ) + } + // Single structured line summarising the security-relevant runtime // posture. Lets oncall grep one log event per pod start to audit // which safety nets are active, without exposing secrets. Booleans @@ -349,7 +355,6 @@ func main() { CompatAllowStateless: cfg.CompatAllowStateless, RenderConsentPage: cfg.RenderConsentPage, ResourceName: cfg.ResourceName, - CSPFormActionExtra: cfg.CSPFormActionExtra, })) // /consent has its own bucket (see consentLimit construction // above): a single user-driven flow is /authorize GET + @@ -357,7 +362,8 @@ func main() { // so a human who clicks Approve quickly after Authorize doesn't // halve the per-IP budget for either path. r.With(consentLimit).Post("/consent", handlers.Consent(tm, logger, cfg.ProxyBaseURL, oauth2Cfg, handlers.ConsentConfig{ - ReplayStore: replayStore, + ReplayStore: replayStore, + ResourceName: cfg.ResourceName, })) var idpExchangeLimiter *rate.Limiter if cfg.IdPExchangeRatePerSec > 0 { @@ -876,12 +882,13 @@ func buildRPCMetrics(cfg *config.Config, logger *zap.Logger) *rpcMetrics { // Referer header to a downstream resource). // - Content-Security-Policy: default-src 'none'; frame-ancestors 'none' // — JSON / redirect responses do not need any subresource; the -// stricter CSP is honest about that. The consent page overrides -// this baseline in handlers/consent.go (it needs style-src -// 'unsafe-inline' for the inline