From 6ccec35288f710672196085f36fef403cf090bf7 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Fri, 2 Oct 2026 12:18:34 +0800 Subject: [PATCH 1/3] feat(server): read the authorization endpoint's request strictly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit server.BeginAuthorizationRequestFromHTTP(r) reads the client_id and request_uri a client sends the browser with after a pushed authorization request. Every embedder read them with r.URL.Query().Get, which silently takes the first of a repeated parameter, though RFC 6749 §3.1 says a parameter "MUST NOT be included more than once". This package's other endpoints already refuse duplicates through FormRequestFromHTTP; the authorization endpoint was the one place every integrator hand-rolled the parsing. A repeated client_id or request_uri, or a malformed query, is an invalid_request *Error, to render locally rather than redirect. A missing value is left for BeginAuthorization to answer, as it does today. The five demos with an authorization server, cmd/conformance-as and GETTING_STARTED now use it. Co-Authored-By: Claude Opus 5.5 --- GETTING_STARTED.md | 11 ++++--- cmd/conformance-as/authorize.go | 17 ++++++----- examples/federated-union/union/idp.go | 12 +++++--- examples/identity-check/identity/bank.go | 12 +++++--- examples/linked-accounts/linked/bank.go | 12 +++++--- examples/payment-consent/payment/bank.go | 12 +++++--- examples/payroll-run/payroll/bank.go | 12 +++++--- server/begin_authorization_http_test.go | 39 ++++++++++++++++++++++++ server/http_requests.go | 30 ++++++++++++++++++ 9 files changed, 126 insertions(+), 31 deletions(-) create mode 100644 server/begin_authorization_http_test.go diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 83d9de2e..4c5a3f52 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -187,10 +187,13 @@ has been redeemed) returns one of three outcomes; the one that matters here is `InteractionRequired`: ```go -action, err := srv.BeginAuthorization(ctx, server.BeginAuthorizationRequest{ - RequestURI: requestURI, // from the incoming request_uri query parameter - ClientID: clientID, -}) +// The request's client_id and request_uri, refusing a repeated one: +// render that error locally, never as a redirect. +req, err := server.BeginAuthorizationRequestFromHTTP(r) +if err != nil { + // render err locally +} +action, err := srv.BeginAuthorization(ctx, req) switch a := action.(type) { case server.InteractionRequired: diff --git a/cmd/conformance-as/authorize.go b/cmd/conformance-as/authorize.go index ed62ca75..c2b25b32 100644 --- a/cmd/conformance-as/authorize.go +++ b/cmd/conformance-as/authorize.go @@ -5,7 +5,6 @@ import ( "net/http" "time" - fapi "github.com/idfoundry/fapigo" "github.com/idfoundry/fapigo/server" "github.com/idfoundry/fapigo/storage" ) @@ -69,11 +68,14 @@ func newConsentHandler(srv *server.Server, clients storage.ClientRepository, clo // handleBegin serves GET /authorize. func (h *consentHandler) handleBegin(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := h.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), - ClientID: fapi.ClientID(q.Get("client_id")), - }) + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + writeLocalHTMLErrorRaw(w, http.StatusBadRequest, "invalid_request", "the authorization request is malformed") + return + } + action, err := h.srv.BeginAuthorization(r.Context(), req) if err != nil { writeLocalHTMLErrorRaw(w, http.StatusInternalServerError, "server_error", "failed to begin authorization") return @@ -131,7 +133,8 @@ func (h *consentHandler) handleBegin(w http.ResponseWriter, r *http.Request) { // gets a perfectly good, safe outcome either way — a rendered // local page is fine for a human in a browser — this exists // purely so the suite's own automated verification can run. - clientID := fapi.ClientID(q.Get("client_id")) + q := r.URL.Query() + clientID := req.ClientID redirectURI := q.Get("redirect_uri") if redirectURI != "" { if client, resolveErr := h.clients.ResolveClient(r.Context(), clientID); resolveErr == nil && client.HasRedirectURI(redirectURI) { diff --git a/examples/federated-union/union/idp.go b/examples/federated-union/union/idp.go index d541e73c..3b02ce72 100644 --- a/examples/federated-union/union/idp.go +++ b/examples/federated-union/union/idp.go @@ -306,10 +306,14 @@ func (p *identityProvider) token(w http.ResponseWriter, r *http.Request) { // cookie bound to this browser — never in the form — and renders the // sign-in and consent page. func (p *identityProvider) authorize(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := p.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), ClientID: fapi.ClientID(q.Get("client_id")), - }) + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + p.w.renderError(w, http.StatusBadRequest, "Sign-in could not start", publicMessage(err, "The sign-in request is malformed.")) + return + } + action, err := p.srv.BeginAuthorization(r.Context(), req) if err != nil { p.w.renderError(w, http.StatusInternalServerError, "Sign-in could not start", publicMessage(err, "Something went wrong. Please try again.")) return diff --git a/examples/identity-check/identity/bank.go b/examples/identity-check/identity/bank.go index e151e0fe..e6d8d418 100644 --- a/examples/identity-check/identity/bank.go +++ b/examples/identity-check/identity/bank.go @@ -322,10 +322,14 @@ func (b *bank) userinfo(w http.ResponseWriter, r *http.Request) { // page. The interaction goes with the browser in a cookie the bank // signs, so any instance of the bank can finish it. func (b *bank) authorize(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := b.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), ClientID: fapi.ClientID(q.Get("client_id")), - }) + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + b.w.renderError(w, bankHost, http.StatusBadRequest, "Sign-in could not start", publicMessage(err, "The sign-in request is malformed.")) + return + } + action, err := b.srv.BeginAuthorization(r.Context(), req) if err != nil { b.w.renderError(w, bankHost, http.StatusInternalServerError, "Sign-in could not start", publicMessage(err, "Something went wrong. Please try again.")) return diff --git a/examples/linked-accounts/linked/bank.go b/examples/linked-accounts/linked/bank.go index 8a2e9f25..f49ab5dc 100644 --- a/examples/linked-accounts/linked/bank.go +++ b/examples/linked-accounts/linked/bank.go @@ -239,10 +239,14 @@ func (b *bank) token(w http.ResponseWriter, r *http.Request) { // page. The interaction goes with the browser in a cookie the bank // signs, so any instance of the bank can finish it. func (b *bank) authorize(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := b.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), ClientID: fapi.ClientID(q.Get("client_id")), - }) + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + b.w.renderError(w, bankHost, http.StatusBadRequest, "Sign-in could not start", publicMessage(err, "The sign-in request is malformed.")) + return + } + action, err := b.srv.BeginAuthorization(r.Context(), req) if err != nil { b.w.renderError(w, bankHost, http.StatusInternalServerError, "Sign-in could not start", publicMessage(err, "Something went wrong. Please try again.")) return diff --git a/examples/payment-consent/payment/bank.go b/examples/payment-consent/payment/bank.go index 6d3dfb21..ab7b1d2c 100644 --- a/examples/payment-consent/payment/bank.go +++ b/examples/payment-consent/payment/bank.go @@ -199,10 +199,14 @@ func (b *bank) token(w http.ResponseWriter, r *http.Request) { // page. The interaction goes with the browser in a cookie the bank // signs, so any instance of the bank can finish it. func (b *bank) authorize(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := b.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), ClientID: fapi.ClientID(q.Get("client_id")), - }) + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + b.w.renderError(w, bankHost, http.StatusBadRequest, "Sign-in could not start", publicMessage(err, "The sign-in request is malformed.")) + return + } + action, err := b.srv.BeginAuthorization(r.Context(), req) if err != nil { b.w.renderError(w, bankHost, http.StatusInternalServerError, "Sign-in could not start", publicMessage(err, "Something went wrong. Please try again.")) return diff --git a/examples/payroll-run/payroll/bank.go b/examples/payroll-run/payroll/bank.go index 91252360..0dca666c 100644 --- a/examples/payroll-run/payroll/bank.go +++ b/examples/payroll-run/payroll/bank.go @@ -220,10 +220,14 @@ func (b *bank) par(w http.ResponseWriter, r *http.Request) { } func (b *bank) authorize(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - action, err := b.srv.BeginAuthorization(r.Context(), server.BeginAuthorizationRequest{ - RequestURI: q.Get("request_uri"), ClientID: fapi.ClientID(q.Get("client_id")), - }) + // Local, never a redirect: an unreadable request names no + // redirect URI it can be trusted with. + req, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + http.Error(w, publicMessage(err, "The authorization request is malformed."), http.StatusBadRequest) + return + } + action, err := b.srv.BeginAuthorization(r.Context(), req) if err != nil { http.Error(w, publicMessage(err, "The authorization request is invalid."), http.StatusBadRequest) return diff --git a/server/begin_authorization_http_test.go b/server/begin_authorization_http_test.go new file mode 100644 index 00000000..e806b633 --- /dev/null +++ b/server/begin_authorization_http_test.go @@ -0,0 +1,39 @@ +package server_test + +import ( + "net/http/httptest" + "testing" + + "github.com/idfoundry/fapigo/server" +) + +func TestBeginAuthorizationRequestFromHTTP(t *testing.T) { + r := httptest.NewRequest("GET", "/authorize?client_id=client-1&request_uri=urn%3Aietf%3Aparams%3Aoauth%3Arequest_uri%3Aabc&ui_locales=en", nil) + got, err := server.BeginAuthorizationRequestFromHTTP(r) + if err != nil { + t.Fatalf("BeginAuthorizationRequestFromHTTP: %v", err) + } + if got.ClientID != "client-1" || got.RequestURI != "urn:ietf:params:oauth:request_uri:abc" { + t.Errorf("got %+v", got) + } + + // Missing values are BeginAuthorization's to answer. + if got, err := server.BeginAuthorizationRequestFromHTTP(httptest.NewRequest("GET", "/authorize", nil)); err != nil || got != (server.BeginAuthorizationRequest{}) { + t.Errorf("no parameters = %+v, %v; want empty values and no error", got, err) + } +} + +func TestBeginAuthorizationRequestFromHTTPRefuses(t *testing.T) { + for name, query := range map[string]string{ + "repeated request_uri": "client_id=c&request_uri=urn%3Aa&request_uri=urn%3Ab", + "repeated client_id": "client_id=c&client_id=d&request_uri=urn%3Aa", + "malformed query": "client_id=%zz", + } { + r := httptest.NewRequest("GET", "/authorize", nil) + r.URL.RawQuery = query + _, err := server.BeginAuthorizationRequestFromHTTP(r) + if code := serverErrorCode(t, err); code != server.ErrorInvalidRequest { + t.Errorf("%s: error code %q, want %q", name, code, server.ErrorInvalidRequest) + } + } +} diff --git a/server/http_requests.go b/server/http_requests.go index f7eeed18..fda85992 100644 --- a/server/http_requests.go +++ b/server/http_requests.go @@ -3,6 +3,9 @@ package server import ( "crypto/x509" "net/http" + "net/url" + + fapi "github.com/idfoundry/fapigo" ) // httpRequestParts is what every client-authenticated endpoint request @@ -111,3 +114,30 @@ func (t TokenEndpointRequest) ClientCredentials() ClientCredentialsTokenRequest func (t TokenEndpointRequest) BackchannelTokenExchange() BackchannelTokenExchangeRequest { return BackchannelTokenExchangeRequest(t.parts) } + +// BeginAuthorizationRequestFromHTTP reads the authorization endpoint's +// request from r's query string: the client_id and request_uri a client +// sends the browser with after a pushed authorization request (RFC 9126 +// §4). r.URL.Query().Get would silently take the first of a repeated +// parameter; RFC 6749 §3.1 says one "MUST NOT be included more than +// once", so a repeated client_id or request_uri, or a malformed query, +// is an invalid_request *Error. Render it locally: an authorization +// endpoint never redirects for a request it can't attribute to a +// client's registered redirect URI. A missing value is left empty for +// BeginAuthorization to answer, as it does any request_uri or client_id +// it doesn't accept. +func BeginAuthorizationRequestFromHTTP(r *http.Request) (BeginAuthorizationRequest, error) { + query, err := url.ParseQuery(r.URL.RawQuery) + if err != nil { + return BeginAuthorizationRequest{}, newError(ErrorInvalidRequest, 400, "the authorization request's query is malformed", err) + } + for _, name := range []string{"client_id", "request_uri"} { + if len(query[name]) > 1 { + return BeginAuthorizationRequest{}, newError(ErrorInvalidRequest, 400, name+" is included more than once", nil) + } + } + return BeginAuthorizationRequest{ + RequestURI: query.Get("request_uri"), + ClientID: fapi.ClientID(query.Get("client_id")), + }, nil +} From 4c0ef7a60b3bac8405b75cacb0b6e60adf1c6116 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Fri, 2 Oct 2026 12:23:13 +0800 Subject: [PATCH 2/3] test(conformance-as): cover the authorize endpoint's refusal and error redirect Co-Authored-By: Claude Opus 5.5 --- cmd/conformance-as/authorize_smoke_test.go | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 cmd/conformance-as/authorize_smoke_test.go diff --git a/cmd/conformance-as/authorize_smoke_test.go b/cmd/conformance-as/authorize_smoke_test.go new file mode 100644 index 00000000..5845d93a --- /dev/null +++ b/cmd/conformance-as/authorize_smoke_test.go @@ -0,0 +1,43 @@ +package main + +import ( + "io" + "net/http" + "net/url" + "strings" + "testing" +) + +// TestSmokeAuthorizeRefusesARepeatedParameter covers the authorization +// endpoint reading its request strictly: a repeated request_uri is +// refused locally as invalid_request, never redirected. +func TestSmokeAuthorizeRefusesARepeatedParameter(t *testing.T) { + h := newSmokeHarness(t, AccessTokenFormatJWT) + res, err := h.httpClient.Get(h.authorize + "?client_id=smoke-test-client&request_uri=urn%3Aa&request_uri=urn%3Ab") + if err != nil { + t.Fatalf("GET /authorize: %v", err) + } + defer func() { _ = res.Body.Close() }() + body, _ := io.ReadAll(res.Body) + if res.StatusCode != http.StatusBadRequest || !strings.Contains(string(body), "invalid_request") { + t.Fatalf("repeated request_uri = %d:\n%s\nwant a local 400 invalid_request", res.StatusCode, body) + } +} + +// TestSmokeAuthorizeRedirectsAnErrorToARegisteredRedirectURI covers the +// suite-only path: an unrecognised request_uri, with a redirect_uri the +// client registered, is answered by redirecting the error there. +func TestSmokeAuthorizeRedirectsAnErrorToARegisteredRedirectURI(t *testing.T) { + h := newSmokeHarness(t, AccessTokenFormatJWT) + q := url.Values{"client_id": {"smoke-test-client"}, "request_uri": {"urn:ietf:params:oauth:request_uri:unknown"}, + "redirect_uri": {"https://rp.smoketest.internal/callback"}, "state": {"s-1"}} + res, err := h.httpClient.Get(h.authorize + "?" + q.Encode()) + if err != nil { + t.Fatalf("GET /authorize: %v", err) + } + defer func() { _ = res.Body.Close() }() + loc, _ := url.Parse(res.Header.Get("Location")) + if res.StatusCode != http.StatusFound || loc == nil || loc.Host != "rp.smoketest.internal" || loc.Query().Get("state") != "s-1" || loc.Query().Get("error") == "" { + t.Fatalf("unknown request_uri = %d, Location %q; want a redirect of the error, with state, to the registered URI", res.StatusCode, res.Header.Get("Location")) + } +} From 15b461010619235c2aabe60e32e48c0f3b7e31c7 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Fri, 2 Oct 2026 12:23:38 +0800 Subject: [PATCH 3/3] test(conformance-as): assert the repeat itself is what's refused Co-Authored-By: Claude Opus 5.5 --- cmd/conformance-as/authorize_smoke_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/conformance-as/authorize_smoke_test.go b/cmd/conformance-as/authorize_smoke_test.go index 5845d93a..a823ca32 100644 --- a/cmd/conformance-as/authorize_smoke_test.go +++ b/cmd/conformance-as/authorize_smoke_test.go @@ -19,8 +19,9 @@ func TestSmokeAuthorizeRefusesARepeatedParameter(t *testing.T) { } defer func() { _ = res.Body.Close() }() body, _ := io.ReadAll(res.Body) - if res.StatusCode != http.StatusBadRequest || !strings.Contains(string(body), "invalid_request") { - t.Fatalf("repeated request_uri = %d:\n%s\nwant a local 400 invalid_request", res.StatusCode, body) + // The repeat itself is refused, not the request_uri it names. + if res.StatusCode != http.StatusBadRequest || !strings.Contains(string(body), "invalid_request") || !strings.Contains(string(body), "the authorization request is malformed") { + t.Fatalf("repeated request_uri = %d:\n%s\nwant a local 400 invalid_request for the malformed request", res.StatusCode, body) } }