feat(server): read the authorization endpoint's request strictly

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:

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) {

cmd/conformance-as/authorize_smoke_test.go

@@ -0,0 +1,44 @@
+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)
+	// 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)
+	}
+}
+
+// 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"))
+	}
+}

examples/federated-union/union/idp.go

@@ -306,10 +306,14 @@
 // 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

examples/identity-check/identity/bank.go

@@ -322,10 +322,14 @@
 // 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

examples/linked-accounts/linked/bank.go

@@ -239,10 +239,14 @@
 // 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

examples/payment-consent/payment/bank.go

@@ -199,10 +199,14 @@
 // 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

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

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)
+		}
+	}
+}

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
+}