From 283789be6e5ba1fc292857c809bd83ca557f5917 Mon Sep 17 00:00:00 2001 From: Damien Degois Date: Tue, 26 May 2026 03:56:29 +0200 Subject: [PATCH] fix(consent): allow IdP origin in CSP form-action for Chromium MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consent page CSP set `form-action 'self'`, which Chromium enforces against every URL in the redirect chain initiated by a form submit (CSP3 §6.5). The approve POST 302s to the upstream IdP, so Chrome / Edge / Brave / Opera blocked the navigation client-side and /callback never fired. Firefox and Safari work because they check only the immediate action= URL. Derive the upstream authorize endpoint's origin once at handler construction from oauth2Cfg.Endpoint.AuthURL (already populated by OIDC discovery — no new config required for the common case), lower- case it per CSP3 §6.7.2.5, and append to the form-action source list. Skip the precompute + warn when RenderConsentPage=false. Add CSP_FORM_ACTION_EXTRA for IdP topologies whose redirect chain crosses the authorize host (Entra B2C, federated AD FS, personal Microsoft accounts, sovereign clouds). Entries are validated against the CSP3 §2.4 host-source ABNF — stricter than RFC 3986 reg-name so sub-delims (`;`, `,`, `&`, `_`) that would break out of the directive when emitted are rejected at startup. --- config/config.go | 87 +++++++++++++++++++ config/config_test.go | 176 +++++++++++++++++++++++++++++++++++++++ docs/configuration.md | 1 + handlers/authorize.go | 33 +++++++- handlers/consent.go | 53 +++++++++++- handlers/consent_test.go | 157 +++++++++++++++++++++++++++++++++- main.go | 10 ++- 7 files changed, 509 insertions(+), 8 deletions(-) diff --git a/config/config.go b/config/config.go index 932fdfd..1b02fd3 100644 --- a/config/config.go +++ b/config/config.go @@ -181,6 +181,20 @@ 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 []string // secretWeakWarning is non-empty when TOKEN_SIGNING_SECRET matches // an obvious-weakness pattern (all-same byte, or short repeating // period). Exposed via @@ -493,6 +507,20 @@ func Load() (*Config, error) { c.UpstreamAuthorization = os.Getenv("UPSTREAM_AUTHORIZATION_HEADER") + if raw := os.Getenv("CSP_FORM_ACTION_EXTRA"); raw != "" { + for _, o := range strings.Split(raw, ",") { + o = strings.TrimSpace(o) + if o == "" { + continue + } + canon, err := canonicalCSPHostSource(o) + if err != nil { + return nil, fmt.Errorf("CSP_FORM_ACTION_EXTRA: %w", err) + } + c.CSPFormActionExtra = append(c.CSPFormActionExtra, canon) + } + } + c.PerSubjectConcurrency = 16 if v := os.Getenv("MCP_PER_SUBJECT_CONCURRENCY"); v != "" { n, err := strconv.ParseInt(v, 10, 64) @@ -666,6 +694,65 @@ func validateOIDCIssuerURL(raw string, allowInsecureHTTP bool) error { } } +// cspHostSourceHostPort matches the CSP3 host-source grammar's +// host-part + optional port-part, minus wildcards (we reject those +// upstream). See https://w3c.github.io/webappsec-csp/#grammardef-host-source +// +// host-part = 1*host-char *( "." 1*host-char ) / IP-literal +// host-char = ALPHA / DIGIT / "-" +// IP-literal = "[" ( IPv6 / IPvFuture ) "]" (from RFC 3986) +// port-part = ":" 1*DIGIT +// +// Intentionally stricter than RFC 3986 reg-name (which accepts +// sub-delims like ";", ",", "&"): an operator-supplied origin that +// round-trips through url.Parse may still contain CSP-meaningful +// characters that break out of the directive when emitted into the +// header ("https://host.com;foo" terminates form-action and starts +// a bogus directive). Mirroring the CSP grammar verbatim closes +// that header-injection foot-gun, which is operator-foot-gun rather +// than RCE but still warrants fail-loud over fail-silent. +var cspHostSourceHostPort = regexp.MustCompile( + `^(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)*)(?::[0-9]+)?$`, +) + +// 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. +// +// Canonicalisation: scheme and host are ASCII-lower-cased per CSP3 +// §6.7.2.5 (host-source matching is case-insensitive); operators +// who paste `HTTPS://Foo.example.com` get `https://foo.example.com` +// in the emitted header so a grep returns the form they expect. +// +// Rejection classes (paste-error shapes that should fail startup +// rather than silently weaken the deployment's CSP): +// - wildcards (`*` anywhere — the directive's role is an explicit +// allowlist; smuggling wildcards in defeats it under a +// misleading name) +// - missing scheme (`host.com` with a `/` in it but no `://`) +// - any path / query / fragment / userinfo component +// - host containing any character outside the CSP host-char set +// (catches "https://host.com;foo", "https://host_name.com", and +// similar header-injection-shaped paste errors) +func canonicalCSPHostSource(in string) (string, error) { + if in == "*" || strings.Contains(in, "*") { + return "", fmt.Errorf("%q wildcard sources are not supported; supply explicit origins", in) + } + if strings.Contains(in, "/") && !strings.Contains(in, "://") { + return "", fmt.Errorf("%q is not a valid origin (want scheme://host[:port])", in) + } + u, err := url.Parse(in) + if err != nil || u.Scheme == "" || u.Host == "" || u.Path != "" || u.RawQuery != "" || u.Fragment != "" || u.User != nil { + return "", fmt.Errorf("%q is not a valid origin (want scheme://host[:port], no path/query/fragment/userinfo)", in) + } + if !cspHostSourceHostPort.MatchString(u.Host) { + return "", fmt.Errorf("%q host component contains characters not allowed in a CSP host-source (must be ALPHA/DIGIT/'-'/'.' or [IPv6])", in) + } + return strings.ToLower(u.Scheme) + "://" + strings.ToLower(u.Host), nil +} + // validateUpstreamMCPURL enforces the shape the reverse-proxy relies // on: absolute URL with a real authority, http(s) scheme, no // userinfo/fragment/query, no opaque form, and a non-empty path diff --git a/config/config_test.go b/config/config_test.go index 85f2757..318d02b 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -206,6 +206,182 @@ func TestLoad_AllowedGroups_Empty(t *testing.T) { } } +// TestLoad_CSPFormActionExtra pins the validation contract for the +// operator-supplied form-action allowlist: accepts scheme://host[:port] +// entries (with whitespace tolerance and empty-entry skip), rejects +// every common paste-error shape (path/query/fragment, userinfo, +// missing scheme, bare wildcard, embedded wildcard). A misconfigured +// allowlist MUST fail startup rather than silently regress Chromium +// consent at first user click. +func TestLoad_CSPFormActionExtra(t *testing.T) { + t.Run("unset is nil", func(t *testing.T) { + setAllRequired(t) + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.CSPFormActionExtra != nil { + t.Errorf("CSPFormActionExtra = %v, want nil", cfg.CSPFormActionExtra) + } + }) + + t.Run("parses multiple with whitespace and empty skip", func(t *testing.T) { + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", " https://a.example , https://b.example:8443 , , https://c.example ") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"https://a.example", "https://b.example:8443", "https://c.example"} + if len(cfg.CSPFormActionExtra) != len(want) { + t.Fatalf("CSPFormActionExtra = %v, want %v", cfg.CSPFormActionExtra, want) + } + for i, w := range want { + if cfg.CSPFormActionExtra[i] != w { + t.Errorf("CSPFormActionExtra[%d] = %q, want %q", i, cfg.CSPFormActionExtra[i], w) + } + } + }) + + t.Run("commas-only collapses to nil", func(t *testing.T) { + // Operators paste-stripping the env value down to just + // commas (and incidentally whitespace) should land in the + // same place as unset — not a startup error, just no extras. + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", " , , , ") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.CSPFormActionExtra != nil { + t.Errorf("CSPFormActionExtra = %v, want nil", cfg.CSPFormActionExtra) + } + }) + + t.Run("accepts explicit default port", func(t *testing.T) { + // CSP source matching is default-port-aware per CSP3 §6.7.2, + // but operators sometimes paste the explicit port. The + // parser must accept it round-trip without trying to be + // clever (stripping :443 would be normalisation we don't owe). + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", "https://idp.example.com:443") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cfg.CSPFormActionExtra) != 1 || cfg.CSPFormActionExtra[0] != "https://idp.example.com:443" { + t.Errorf("CSPFormActionExtra = %v, want [https://idp.example.com:443]", cfg.CSPFormActionExtra) + } + }) + + t.Run("accepts IPv6 bracketed host", func(t *testing.T) { + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", "https://[2001:db8::1]:8443") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cfg.CSPFormActionExtra) != 1 || cfg.CSPFormActionExtra[0] != "https://[2001:db8::1]:8443" { + t.Errorf("CSPFormActionExtra = %v, want [https://[2001:db8::1]:8443]", cfg.CSPFormActionExtra) + } + }) + + t.Run("accepts http scheme", func(t *testing.T) { + // http:// is accepted at parse time — the gating for + // insecure schemes is OIDC_ALLOW_INSECURE_HTTP on the issuer + // URL, not a second gate here. An operator allowlisting an + // http origin in form-action is making an intentional dev + // choice. + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", "http://keycloak.dev.local:8080") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cfg.CSPFormActionExtra) != 1 || cfg.CSPFormActionExtra[0] != "http://keycloak.dev.local:8080" { + t.Errorf("CSPFormActionExtra = %v, want [http://keycloak.dev.local:8080]", cfg.CSPFormActionExtra) + } + }) + + t.Run("preserves order and accepts many entries", func(t *testing.T) { + // Order matters for the CSP header readability (and grep + // when debugging) — a future micro-optimisation that + // sorts or dedups entries would break the operator's + // expectation that "what I configured is what I see". + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", + "https://e.example,https://d.example,https://c.example,https://b.example,https://a.example") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{ + "https://e.example", + "https://d.example", + "https://c.example", + "https://b.example", + "https://a.example", + } + if len(cfg.CSPFormActionExtra) != len(want) { + t.Fatalf("CSPFormActionExtra len = %d, want %d (got %v)", len(cfg.CSPFormActionExtra), len(want), cfg.CSPFormActionExtra) + } + for i, w := range want { + if cfg.CSPFormActionExtra[i] != w { + t.Errorf("CSPFormActionExtra[%d] = %q, want %q", i, cfg.CSPFormActionExtra[i], w) + } + } + }) + + t.Run("canonicalises case to lowercase", func(t *testing.T) { + // CSP3 §6.7.2.5 makes host-source matching case-insensitive; + // we lower-case on the way in so the emitted header is the + // form an operator would grep for. + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", "HTTPS://Foo.Example.COM:8443") + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + const want = "https://foo.example.com:8443" + if len(cfg.CSPFormActionExtra) != 1 || cfg.CSPFormActionExtra[0] != want { + t.Errorf("CSPFormActionExtra = %v, want [%s]", cfg.CSPFormActionExtra, want) + } + }) + + for _, bad := range []struct { + name string + val string + }{ + {"path component", "https://idp.example.com/path"}, + {"trailing slash", "https://idp.example.com/"}, + {"query string", "https://idp.example.com?x=1"}, + {"fragment", "https://idp.example.com#frag"}, + {"userinfo", "https://user@idp.example.com"}, + {"missing scheme", "idp.example.com"}, + {"bare wildcard", "*"}, + {"embedded wildcard", "https://*.example.com"}, + {"empty scheme only", "https://"}, + // CSP3 §2.4 host-char is ALPHA/DIGIT/'-' only — stricter + // than RFC 3986 reg-name. The remaining sub-delims would + // terminate the form-action directive when emitted, so the + // parser rejects them rather than letting a misconfigured + // allowlist silently weaken the consent page's CSP. + {"semicolon in host", "https://idp.example.com;injected"}, + {"comma in host", "https://idp.example.com,injected"}, + {"ampersand in host", "https://idp.example.com&injected"}, + {"underscore in host", "https://host_name.example.com"}, + {"space in value", "https://idp.example.com /path"}, + } { + t.Run("rejects "+bad.name, func(t *testing.T) { + setAllRequired(t) + t.Setenv("CSP_FORM_ACTION_EXTRA", bad.val) + if _, err := Load(); err == nil { + t.Errorf("Load() unexpectedly succeeded for %q", bad.val) + } + }) + } +} + func TestLoad_RevokeBefore(t *testing.T) { setAllRequired(t) t.Setenv("REVOKE_BEFORE", "2026-03-28T12:00:00Z") diff --git a/docs/configuration.md b/docs/configuration.md index 07f7a16..fa5264f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -92,6 +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'` and the discovered OIDC authorize endpoint. 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. | | `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 diff --git a/handlers/authorize.go b/handlers/authorize.go index bd73dba..d095aa3 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -55,6 +55,14 @@ 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). @@ -70,6 +78,29 @@ 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 consent page's CSP 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). + // + // Skipped entirely when RenderConsentPage is false: the silent + // fork bypasses renderConsent, the precomputed CSP is unused, and + // a "consent CSP misconfigured" warn on a deployment that doesn't + // render the consent page would be misleading noise. + var consentCSP string + if authzCfg.RenderConsentPage { + var idpOriginOK bool + consentCSP, idpOriginOK = buildConsentCSP(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, @@ -233,7 +264,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, sealedConsent{ + renderConsent(w, r, tm, logger, baseURL, authzCfg.ResourceName, consentCSP, 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. diff --git a/handlers/consent.go b/handlers/consent.go index 14e6720..4bdcc89 100644 --- a/handlers/consent.go +++ b/handlers/consent.go @@ -7,6 +7,7 @@ import ( "html/template" "net/http" "net/url" + "strings" "time" "github.com/babs/mcp-auth-proxy/metrics" @@ -118,6 +119,50 @@ var consentTmpl = template.Must(template.New("consent").Parse(` `)) +// buildConsentCSP returns the Content-Security-Policy header value +// for the consent page, with form-action widened to include the +// upstream IdP origin derived from authURL and any operator-supplied +// extra origins. +// +// 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. +// +// authURL is the upstream OIDC authorization endpoint (set from +// discovery at startup). On parse failure / empty value the +// returned CSP 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. +// +// 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 buildConsentCSP(authURL string, extraOrigins []string) (csp string, idpOriginOK bool) { + sources := make([]string, 0, 2+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 + } + } + sources = append(sources, extraOrigins...) + return "default-src 'none'; style-src 'unsafe-inline'; form-action " + strings.Join(sources, " ") + "; frame-ancestors 'none'; base-uri 'none'", idpOriginOK +} + // renderConsent seals the validated /authorize parameters into a // sealedConsent token and writes the consent HTML page. The token // is the only thing carried across the user click — POST /consent @@ -128,7 +173,7 @@ var consentTmpl = template.Must(template.New("consent").Parse(` // 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, consent sealedConsent) { +func renderConsent(w http.ResponseWriter, r *http.Request, tm *token.Manager, logger *zap.Logger, baseURL, resourceName, csp string, consent sealedConsent) { consentToken, err := tm.SealJSON(consent, token.PurposeConsent) if err != nil { logger.Error("consent_seal_failed", zap.Error(err)) @@ -160,8 +205,10 @@ 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. - w.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'; form-action 'self'; frame-ancestors 'none'; base-uri 'none'") + // origin. form-action also names the upstream IdP origin so the + // approve POST's 302 to the IdP is not blocked by Chromium's + // redirect-chain enforcement (see buildConsentCSP). + w.Header().Set("Content-Security-Policy", csp) w.WriteHeader(http.StatusOK) if err := consentTmpl.Execute(w, data); err != nil { // Body already started — log only. diff --git a/handlers/consent_test.go b/handlers/consent_test.go index 8073489..d6217dd 100644 --- a/handlers/consent_test.go +++ b/handlers/consent_test.go @@ -15,6 +15,8 @@ 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" ) // authorizeConsentEnabled mirrors the production wiring of /authorize @@ -329,10 +331,17 @@ 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. for _, want := range []string{ "default-src 'none'", "frame-ancestors 'none'", - "form-action 'self'", + "form-action 'self' https://idp.example.com", "base-uri 'none'", } { if !strings.Contains(csp, want) { @@ -341,6 +350,152 @@ func TestConsent_RelaxedCSP(t *testing.T) { } } +// TestBuildConsentCSP_IdPOrigin pins buildConsentCSP'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 TestBuildConsentCSP_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) { + csp, ok := buildConsentCSP(tc.authURL, tc.extra) + 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) + } + }) + } +} + +// 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 buildConsentCSP + // 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 diff --git a/main.go b/main.go index a32ca76..3c9dff3 100644 --- a/main.go +++ b/main.go @@ -349,6 +349,7 @@ 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 + @@ -875,9 +876,12 @@ 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. /authorize ends in a 302 to -// the IdP whose own CSP applies on the IdP page; the redirect -// response itself has no body to which CSP applies. +// 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