From 30f2ca81f625d02912b5a2455e31e5b85a073617 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 29 Sep 2026 00:45:57 +0800 Subject: [PATCH] docs: require binding the interaction handle to the browser; fix refresh-token binding claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InteractionHandle's docs and GETTING_STARTED never said to bind the handle to the browser that started the interaction, and the reference login UI in cmd/conformance-as carries it in a hidden form field. CompleteAuthorization accepts a handle from whoever presents it, so a login UI built that way that signs users in from an existing session cookie is open to consent CSRF: a malicious client starts its own authorization, puts its handle in a self-submitting form, and a signed-in victim's browser approves it. The handle's doc comment and GETTING_STARTED now require keeping it in an HttpOnly, Secure, SameSite cookie and CSRF-protecting the form, and the reference code is marked as not a model for this. server/doc.go also said, in two places, that a refresh token is bound to the DPoP key it was issued under and that RefreshAccessToken rejects a proof from another key. It deliberately isn't: every client is confidential and RFC 9449 §5 binds its refresh token to client authentication instead. Co-Authored-By: Claude Opus 5.5 --- GETTING_STARTED.md | 20 +++++++++++++++++--- cmd/conformance-as/authorize.go | 7 +++++++ server/doc.go | 15 ++++++++++----- server/interaction.go | 16 ++++++++++++++++ 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 2ff37d98..5dc2bda8 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -234,10 +234,24 @@ case server.AuthorizationLocalError: } ``` +**Bind the interaction handle to the browser.** `CompleteAuthorization` +accepts a handle from whoever presents it, so a login UI that carries it +in a form field and signs users in from an existing session cookie is +open to consent CSRF: a malicious client starts its own authorization, +puts its handle in a self-submitting form on its own page, and a +signed-in victim's browser approves it, sending that client a code for +the victim's account. Set the handle (`a.Handle.String()`) in an +HttpOnly, Secure, SameSite cookie when the browser reaches `/authorize`, +read it back with `server.ParseInteractionHandle` when the form is +submitted, and protect that form with your usual CSRF defence. See +`server.InteractionHandle`'s doc comment. + `cmd/conformance-as/authorize.go` is a complete, working version of -exactly this — read it for the full picture, including how it bridges -an `InteractionHandle` across the GET (render the form) and POST -(handle the submission) halves of a real HTTP flow. Its own login form +exactly this — read it for the full picture of the GET (render the +form) and POST (handle the submission) halves of a real HTTP flow. Its +handle bridge is not one to copy: it carries the handle in a hidden form +field, which is only tolerable because it authenticates nobody — use +the cookie binding above instead. Its own login form (`consent_template.go`) is a deliberate non-example: a free-text "type any username" field with no real authentication behind it, because this binary exists to drive OIDF conformance testing, not to demonstrate diff --git a/cmd/conformance-as/authorize.go b/cmd/conformance-as/authorize.go index bf2332a6..851e79b0 100644 --- a/cmd/conformance-as/authorize.go +++ b/cmd/conformance-as/authorize.go @@ -22,6 +22,13 @@ const consentPendingTTL = 5 * time.Minute // genuine HTML consent form — not an auto-approve shortcut — so a real // browser (including an automated conformance-suite browser driver) has // something to load and submit. +// +// Not a model for a production login UI: it carries the interaction +// handle in a hidden form field, with no cookie binding it to the +// browser and no CSRF protection on the POST, which a real deployment +// must have — see server.InteractionHandle's doc comment for the +// consent CSRF this would otherwise allow. It gets away with it only +// because it authenticates nobody (the form's username is free text). type consentHandler struct { srv *server.Server clients storage.ClientRepository diff --git a/server/doc.go b/server/doc.go index 6ced1b1a..0c5441f2 100644 --- a/server/doc.go +++ b/server/doc.go @@ -46,8 +46,9 @@ // rotated — FAPI 2.0 Security Profile Final §5.3.2.1 says an // authorization server "shall not use refresh token rotation except // in extraordinary circumstances" — so it stays valid for repeated -// use until it expires or is revoked, and is bound to its client -// and (under DPoP) to the key it was issued under. +// use until it expires or is revoked, and is bound to the client it +// was issued to — only that client, authenticating itself, can +// redeem it. // - AuthorizationAction (from BeginAuthorization) and AuthorizationResult // (from CompleteAuthorization) are closed sum types, not structs with // optional fields, so a caller can never mistake a local error for a @@ -75,9 +76,13 @@ // ExchangeAuthorizationCode and RefreshAccessToken each require a // valid DPoP proof bound to the token endpoint and reject a replayed // proof jti the same way they reject a replayed client assertion or -// request object, and a refresh token is bound to the DPoP key it -// was issued under: RefreshAccessToken rejects a proof from any -// other key, even one belonging to the same client. Bearer +// request object. RefreshAccessToken does not require the proof to +// come from the key the refresh token was first issued under: every +// client here is confidential and authenticates at the token +// endpoint, and RFC 9449 §5 binds a confidential client's refresh +// token to that client authentication rather than to a DPoP key. +// Each new access token is bound to the key of the proof presented +// with it. Bearer // (non-sender-constrained) tokens are not supported. // - An ID token is issued alongside the access token exactly when the // (possibly refresh-narrowed) granted scope includes "openid"; diff --git a/server/interaction.go b/server/interaction.go index 291cfbef..62384064 100644 --- a/server/interaction.go +++ b/server/interaction.go @@ -25,6 +25,22 @@ const interactionHandleSize = 32 // confused with, or substituted for, a request_uri or an authorization // code: the login UI receives only this handle, never a storage // primary key or the underlying PAR reference. +// +// Bind it to the browser that started the interaction. The handle +// identifies a pending authorization, not who may complete it, and +// CompleteAuthorization accepts it from whoever presents it. If the +// login UI round-trips it through a form field and authenticates the +// user from an existing session cookie, a malicious client can start +// its own authorization, place its handle in a form on its own page +// that submits itself, and have a victim's already-signed-in browser +// approve that authorization, so the client receives a code for the +// victim's account (consent CSRF, the authorization-server side of RFC +// 9700 §4.7's login CSRF). Keep the handle in an HttpOnly, Secure, +// SameSite cookie set when the browser reaches the authorization +// endpoint, read it back from that cookie (ParseInteractionHandle) when +// the login or consent form is submitted, and protect that form with +// the application's usual CSRF defence. Never accept the handle from +// a form field or query parameter alone. type InteractionHandle struct { value string }