Skip to content

Commit fc2464d

Browse files
committed
fix(consent): break CSP form-action chain with navigation interstitial
Chromium enforces the consent page's form-action directive against every hop of the redirect chain a form submit initiates. Client-side hops after redirect_uri (e.g. Power Platform's global.consent.azure-apim.net -> regional UI origin) are unknowable in advance, so origin enumeration (#33 IdP extras, #35 redirect_uri) could never be complete. - answer POST /consent (approve/deny/server-error) with a 200 same-origin interstitial (meta refresh, no JS) instead of a 302; form navigation ends at the proxy, downstream hops are a regular navigation that form-action does not govern - tighten consent page CSP to form-action 'self'; drop buildConsentCSPSources/formatConsentCSP and per-render widening - deprecate CSP_FORM_ACTION_EXTRA (parsed, ignored, startup warn) - re-render consent page with fresh JTI on replayed submit instead of a dead-end 400 consent_replay; decision still requires a new explicit click, single-use guarantee unchanged
1 parent 3061697 commit fc2464d

9 files changed

Lines changed: 309 additions & 527 deletions

File tree

config/config.go

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -181,19 +181,14 @@ type Config struct {
181181
// env: UPSTREAM_AUTHORIZATION_HEADER. Treat as a secret in
182182
// deployment (mount from a Secret, not a ConfigMap).
183183
UpstreamAuthorization string
184-
// CSPFormActionExtra lists additional scheme://host[:port]
185-
// origins appended to the consent page's CSP form-action source
186-
// list, alongside 'self' and the discovered upstream authorize
187-
// endpoint origin. Needed for IdP redirect chains that cross the
188-
// authorize host: Entra B2C (*.b2clogin.com), personal Microsoft
189-
// accounts (login.live.com), federated AD FS (customer-owned
190-
// host), sovereign clouds (login.microsoftonline.us /
191-
// .partner.microsoftonline.cn). Each entry is a single fixed
192-
// origin — wildcards are intentionally not supported because the
193-
// directive's role is an explicit allowlist defense-in-depth
194-
// against HTML injection on the consent page. Empty for the
195-
// common one-host-IdP case. env: CSP_FORM_ACTION_EXTRA,
196-
// comma-separated.
184+
// CSPFormActionExtra is DEPRECATED and ignored. The consent POST
185+
// is now answered with a same-origin navigation interstitial
186+
// (handlers.renderNavInterstitial), which ends Chromium's
187+
// form-action redirect-chain enforcement at the proxy — no IdP or
188+
// client origin needs to be listed, so form-action is 'self'-only
189+
// again. The variable is still parsed and validated so existing
190+
// deployments keep starting; a startup warning flags it for
191+
// removal. env: CSP_FORM_ACTION_EXTRA, comma-separated.
197192
CSPFormActionExtra []string
198193
// secretWeakWarning is non-empty when TOKEN_SIGNING_SECRET matches
199194
// an obvious-weakness pattern (all-same byte, or short repeating

docs/configuration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ control.
9292
| `PKCE_REQUIRED` | `true` | Set `false` for legacy clients that omit PKCE (Cursor, MCP Inspector, ChatGPT). Rejected by `PROD_MODE`. |
9393
| `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`. |
9494
| `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. |
95-
| `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. |
95+
| `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. |
9696
| `OIDC_ALLOW_INSECURE_HTTP` | `false` | Dev-only escape hatch for cleartext `http://` OIDC issuers (Docker Compose Keycloak demo). Rejected when `PROD_MODE=true`. |
9797

9898
## Logging and observability

docs/threat-model.md

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,12 @@ rather than assuming they're already covered.
4646
- **Browser-side XSS in the consent page.** The page is JS-free and
4747
CSP-locked (`default-src 'none'`, `style-src 'unsafe-inline'`,
4848
`script-src` defaults to none, `frame-ancestors 'none'`,
49-
`base-uri 'none'`). `form-action` is widened only to the upstream
50-
IdP origin (from OIDC discovery), any `CSP_FORM_ACTION_EXTRA`
51-
entries, and the current request's validated `redirect_uri` origin
52-
— each of the three filtered through the CSP3 §2.4 host-source
53-
ABNF check so a DCR-registered redirect_uri whose host smuggles a
54-
sub-delim cannot break out of the directive. A browser-engine bug
55-
that escapes contextual HTML escaping is not separately mitigated.
49+
`base-uri 'none'`, `form-action 'self'`). The consent POST is
50+
answered with a same-origin navigation interstitial (200 + meta
51+
refresh), so `form-action` never needs to name the IdP or client
52+
origins — an injected form cannot point anywhere but the proxy
53+
itself. A browser-engine bug that escapes contextual HTML
54+
escaping is not separately mitigated.
5655
- **Network-level MITM between proxy and IdP.** TLS verification is
5756
on by default in the `oauth2` library; an operator who disables
5857
it (or a CA compromise) lets a MITM observe the upstream code

handlers/authorize.go

Lines changed: 2 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,6 @@ type AuthorizeConfig struct {
5555
// name via MCP_RESOURCE_NAME. Falls back to CanonicalResource
5656
// when empty.
5757
ResourceName string
58-
// CSPFormActionExtra is an operator-supplied list of additional
59-
// scheme://host[:port] origins appended to the consent page's
60-
// form-action source list (alongside 'self' and the discovered
61-
// upstream authorize endpoint origin). Needed for IdP redirect
62-
// chains that cross the authorize host (Entra B2C, federated
63-
// AD FS, personal MS accounts, sovereign clouds). Validated at
64-
// config.Load time. Empty for the common case.
65-
CSPFormActionExtra []string
6658
}
6759

6860
// Authorize handles GET /authorize (OAuth 2.1 PKCE authorization request).
@@ -78,31 +70,6 @@ type AuthorizeConfig struct {
7870
// front-loaded above the response_type / resource / PKCE / state
7971
// checks.
8072
func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg *oauth2.Config, authzCfg AuthorizeConfig) http.HandlerFunc {
81-
// Precompute the static form-action source list once at startup.
82-
// The IdP origin in form-action must match the upstream AuthURL
83-
// so the consent POST's 302 to the IdP is not blocked by
84-
// Chromium's form-action redirect-chain enforcement. Extra
85-
// operator-supplied origins (CSP_FORM_ACTION_EXTRA) cover IdP
86-
// topologies whose redirect chain leaves the authorize host
87-
// (Entra B2C, federated AD FS, personal MS accounts, sovereign
88-
// clouds). The client's redirect_uri origin is appended per
89-
// render — see formatConsentCSP.
90-
//
91-
// Skipped entirely when RenderConsentPage is false: the silent
92-
// fork bypasses renderConsent, the precomputed sources are
93-
// unused, and a "consent CSP misconfigured" warn on a deployment
94-
// that doesn't render the consent page would be misleading noise.
95-
var consentCSPSources []string
96-
if authzCfg.RenderConsentPage {
97-
var idpOriginOK bool
98-
consentCSPSources, idpOriginOK = buildConsentCSPSources(oauth2Cfg.Endpoint.AuthURL, authzCfg.CSPFormActionExtra)
99-
if !idpOriginOK {
100-
logger.Warn("consent_csp_idp_origin_missing",
101-
zap.String("auth_url", oauth2Cfg.Endpoint.AuthURL),
102-
zap.String("hint", "consent submit will be blocked in Chromium browsers; verify OIDC discovery"),
103-
)
104-
}
105-
}
10673
return func(w http.ResponseWriter, r *http.Request) {
10774
q := r.URL.Query()
10875
if rejectRepeatedParams(w, q,
@@ -266,7 +233,7 @@ func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg
266233
// redirect) replays from POST /consent on approval.
267234
if authzCfg.RenderConsentPage {
268235
metrics.AuthorizeInitiated.WithLabelValues("consent").Inc()
269-
renderConsent(w, r, tm, logger, baseURL, authzCfg.ResourceName, consentCSPSources, sealedConsent{
236+
renderConsent(w, tm, logger, baseURL, authzCfg.ResourceName, sealedConsent{
270237
// Per-render JTI: a fresh id every GET /authorize so
271238
// back-button = re-consent (each render gets its own
272239
// single-use claim slot) rather than dead-state errors.
@@ -281,7 +248,7 @@ func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg
281248
Typ: token.PurposeConsent,
282249
Audience: baseURL,
283250
ExpiresAt: time.Now().Add(consentTTL),
284-
})
251+
}, false)
285252
return
286253
}
287254

handlers/callback.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -499,11 +499,25 @@ func callbackHandler(tm *token.Manager, logger *zap.Logger, audience string, oau
499499
// upstream, so a parse error here is an invariant violation rather
500500
// than attacker-controlled input.
501501
func redirectAuthzError(w http.ResponseWriter, r *http.Request, redirectURI, state, errCode, errDesc, audience string) {
502-
u, err := url.Parse(redirectURI)
502+
target, err := authzErrorURL(redirectURI, state, errCode, errDesc, audience)
503503
if err != nil {
504504
writeOAuthError(w, http.StatusBadRequest, errCode, errDesc)
505505
return
506506
}
507+
http.Redirect(w, r, target, http.StatusFound)
508+
}
509+
510+
// authzErrorURL builds the RFC 6749 §4.1.2.1 error envelope on the
511+
// client's redirect_uri. Split out of redirectAuthzError so POST
512+
// /consent responses can deliver the same envelope through the
513+
// navigation interstitial instead of a 302 — see
514+
// renderNavInterstitial for why a form POST must not answer with a
515+
// cross-origin redirect.
516+
func authzErrorURL(redirectURI, state, errCode, errDesc, audience string) (string, error) {
517+
u, err := url.Parse(redirectURI)
518+
if err != nil {
519+
return "", err
520+
}
507521
q := u.Query()
508522
q.Set("error", errCode)
509523
if errDesc != "" {
@@ -516,5 +530,5 @@ func redirectAuthzError(w http.ResponseWriter, r *http.Request, redirectURI, sta
516530
u.RawQuery = q.Encode()
517531
u.Fragment = ""
518532
u.RawFragment = ""
519-
http.Redirect(w, r, u.String(), http.StatusFound)
533+
return u.String(), nil
520534
}

0 commit comments

Comments
 (0)