From ee2f80a0a5f3749bb0fbf9732a0856f5c2bd7a6a Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Fri, 2 Oct 2026 13:50:27 +0800 Subject: [PATCH] fix(server): answer an unreadable form body as invalid_request FormRequestFromHTTP, and the PushAuthorizationRequestFromHTTP, TokenEndpointRequestFromHTTP and BeginBackchannelAuthenticationRequestFromHTTP constructors built on it, returned plain errors for a body they couldn't take: not form-encoded, unreadable, over 1 MiB, or malformed. So server.WriteError, whose doc says every error this package returns is a *Error, answered the client's mistake with a 500 server_error. Each is now a 400 invalid_request *Error with a fixed public description; the detail stays in the cause, for logs. BeginAuthorizationRequestFromHTTP already did this. The demos' badRequest helpers and the conformance AS's err.Error() echo (which sent the internal message to the client) give way to server.WriteError. Co-Authored-By: Claude Opus 5.5 --- cmd/conformance-as/backchannel.go | 2 +- cmd/conformance-as/errors.go | 4 +- cmd/conformance-as/par.go | 2 +- cmd/conformance-as/token.go | 2 +- examples/decoupled-checkout/checkout/bank.go | 4 +- .../decoupled-checkout/checkout/errors.go | 7 --- examples/federated-union/union/errors.go | 7 --- examples/federated-union/union/idp.go | 4 +- examples/identity-check/identity/bank.go | 4 +- examples/identity-check/identity/errors.go | 7 --- examples/linked-accounts/linked/bank.go | 4 +- examples/linked-accounts/linked/errors.go | 7 --- examples/payment-consent/payment/bank.go | 4 +- examples/payment-consent/payment/errors.go | 7 --- examples/payroll-run/payroll/bank.go | 4 +- examples/payroll-run/payroll/errors.go | 7 --- server/formrequest.go | 16 +++-- server/formrequest_error_test.go | 62 +++++++++++++++++++ 18 files changed, 90 insertions(+), 64 deletions(-) create mode 100644 server/formrequest_error_test.go diff --git a/cmd/conformance-as/backchannel.go b/cmd/conformance-as/backchannel.go index aea0dff4..2619c95d 100644 --- a/cmd/conformance-as/backchannel.go +++ b/cmd/conformance-as/backchannel.go @@ -57,7 +57,7 @@ func newBackchannelHandler(srv *server.Server, clock server.Clock, defaultSubjec func (h *backchannelHandler) handleAuthenticate(w http.ResponseWriter, r *http.Request) { req, err := server.BeginBackchannelAuthenticationRequestFromHTTP(r) if err != nil { - writeRawOAuthError(w, http.StatusBadRequest, server.ErrorInvalidRequest, err.Error()) + writeOAuthJSONError(w, err) return } action, err := h.srv.BeginBackchannelAuthentication(r.Context(), req) diff --git a/cmd/conformance-as/errors.go b/cmd/conformance-as/errors.go index 9e3b3e43..07c06584 100644 --- a/cmd/conformance-as/errors.go +++ b/cmd/conformance-as/errors.go @@ -16,8 +16,8 @@ func writeOAuthJSONError(w http.ResponseWriter, err error) { } // writeRawOAuthError writes an OAuth JSON error response directly, for -// failures detected before a *server.Error exists at all (e.g. a -// malformed form body, or an unrecognized grant_type) — via +// failures detected before a *server.Error exists at all (e.g. an +// unrecognized grant_type) — via // server.NewError/*server.Error.WriteJSON, the same encoding every // *server.Error this binary ever receives already goes through. func writeRawOAuthError(w http.ResponseWriter, status int, code server.ErrorCode, description string) { diff --git a/cmd/conformance-as/par.go b/cmd/conformance-as/par.go index eb901201..d95fb0bc 100644 --- a/cmd/conformance-as/par.go +++ b/cmd/conformance-as/par.go @@ -10,7 +10,7 @@ func parHandler(srv *server.Server) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - writeRawOAuthError(w, http.StatusBadRequest, server.ErrorInvalidRequest, err.Error()) + writeOAuthJSONError(w, err) return } result, err := srv.PushAuthorizationRequest(r.Context(), req) diff --git a/cmd/conformance-as/token.go b/cmd/conformance-as/token.go index 81bce0ab..b8518fcf 100644 --- a/cmd/conformance-as/token.go +++ b/cmd/conformance-as/token.go @@ -10,7 +10,7 @@ func tokenHandler(srv *server.Server) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - writeRawOAuthError(w, http.StatusBadRequest, server.ErrorInvalidRequest, err.Error()) + writeOAuthJSONError(w, err) return } diff --git a/examples/decoupled-checkout/checkout/bank.go b/examples/decoupled-checkout/checkout/bank.go index 4e5649c8..7679aaa0 100644 --- a/examples/decoupled-checkout/checkout/bank.go +++ b/examples/decoupled-checkout/checkout/bank.go @@ -222,7 +222,7 @@ func (b *bank) jwks(w http.ResponseWriter, r *http.Request) { func (b *bank) backchannelAuthentication(w http.ResponseWriter, r *http.Request) { req, err := server.BeginBackchannelAuthenticationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } action, err := b.srv.BeginBackchannelAuthentication(r.Context(), req) @@ -256,7 +256,7 @@ func (b *bank) backchannelAuthentication(w http.ResponseWriter, r *http.Request) func (b *bank) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } if req.GrantType() != "urn:openid:params:grant-type:ciba" { diff --git a/examples/decoupled-checkout/checkout/errors.go b/examples/decoupled-checkout/checkout/errors.go index b5df63f2..95001b86 100644 --- a/examples/decoupled-checkout/checkout/errors.go +++ b/examples/decoupled-checkout/checkout/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/examples/federated-union/union/errors.go b/examples/federated-union/union/errors.go index 4a511ab9..c6578424 100644 --- a/examples/federated-union/union/errors.go +++ b/examples/federated-union/union/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/examples/federated-union/union/idp.go b/examples/federated-union/union/idp.go index 93894852..56014fed 100644 --- a/examples/federated-union/union/idp.go +++ b/examples/federated-union/union/idp.go @@ -277,7 +277,7 @@ func (p *identityProvider) jwks(w http.ResponseWriter, r *http.Request) { func (p *identityProvider) par(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } result, err := p.srv.PushAuthorizationRequest(r.Context(), req) @@ -291,7 +291,7 @@ func (p *identityProvider) par(w http.ResponseWriter, r *http.Request) { func (p *identityProvider) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } if req.GrantType() != "authorization_code" { diff --git a/examples/identity-check/identity/bank.go b/examples/identity-check/identity/bank.go index ced74391..9a37a32a 100644 --- a/examples/identity-check/identity/bank.go +++ b/examples/identity-check/identity/bank.go @@ -237,7 +237,7 @@ func (b *bank) jwks(w http.ResponseWriter, r *http.Request) { func (b *bank) par(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } result, err := b.srv.PushAuthorizationRequest(r.Context(), req) @@ -251,7 +251,7 @@ func (b *bank) par(w http.ResponseWriter, r *http.Request) { func (b *bank) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } if req.GrantType() != "authorization_code" { diff --git a/examples/identity-check/identity/errors.go b/examples/identity-check/identity/errors.go index 278b9b45..0b6fbaf6 100644 --- a/examples/identity-check/identity/errors.go +++ b/examples/identity-check/identity/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/examples/linked-accounts/linked/bank.go b/examples/linked-accounts/linked/bank.go index d0b4e37c..dae90327 100644 --- a/examples/linked-accounts/linked/bank.go +++ b/examples/linked-accounts/linked/bank.go @@ -202,7 +202,7 @@ func (b *bank) jwks(w http.ResponseWriter, r *http.Request) { func (b *bank) par(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } result, err := b.srv.PushAuthorizationRequest(r.Context(), req) @@ -217,7 +217,7 @@ func (b *bank) par(w http.ResponseWriter, r *http.Request) { func (b *bank) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } var result server.TokenResult diff --git a/examples/linked-accounts/linked/errors.go b/examples/linked-accounts/linked/errors.go index 0eccedb4..7c371506 100644 --- a/examples/linked-accounts/linked/errors.go +++ b/examples/linked-accounts/linked/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/examples/payment-consent/payment/bank.go b/examples/payment-consent/payment/bank.go index 36fb0b13..f503f627 100644 --- a/examples/payment-consent/payment/bank.go +++ b/examples/payment-consent/payment/bank.go @@ -168,7 +168,7 @@ func (b *bank) jwks(w http.ResponseWriter, r *http.Request) { func (b *bank) par(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } result, err := b.srv.PushAuthorizationRequest(r.Context(), req) @@ -182,7 +182,7 @@ func (b *bank) par(w http.ResponseWriter, r *http.Request) { func (b *bank) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } if req.GrantType() != "authorization_code" { diff --git a/examples/payment-consent/payment/errors.go b/examples/payment-consent/payment/errors.go index 96a646d3..0723f108 100644 --- a/examples/payment-consent/payment/errors.go +++ b/examples/payment-consent/payment/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/examples/payroll-run/payroll/bank.go b/examples/payroll-run/payroll/bank.go index 362cd841..cc2b591f 100644 --- a/examples/payroll-run/payroll/bank.go +++ b/examples/payroll-run/payroll/bank.go @@ -211,7 +211,7 @@ func (b *bank) jwks(w http.ResponseWriter, r *http.Request) { func (b *bank) par(w http.ResponseWriter, r *http.Request) { req, err := server.PushAuthorizationRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } result, err := b.srv.PushAuthorizationRequest(r.Context(), req) @@ -250,7 +250,7 @@ func (b *bank) authorize(w http.ResponseWriter, r *http.Request) { func (b *bank) token(w http.ResponseWriter, r *http.Request) { req, err := server.TokenEndpointRequestFromHTTP(r) if err != nil { - badRequest(w, err) + server.WriteError(w, err) return } if req.GrantType() != "client_credentials" { diff --git a/examples/payroll-run/payroll/errors.go b/examples/payroll-run/payroll/errors.go index 715da6a1..018fe9fc 100644 --- a/examples/payroll-run/payroll/errors.go +++ b/examples/payroll-run/payroll/errors.go @@ -15,13 +15,6 @@ import ( // logs show full errors on purpose: explaining a refusal is what they // are for.) -// badRequest answers a request this package couldn't read, such as a -// form a *FromHTTP constructor refused. -func badRequest(w http.ResponseWriter, err error) { - log.Printf("bad request: %v", err) - server.NewError(server.ErrorInvalidRequest, http.StatusBadRequest, "the request is malformed").WriteJSON(w) -} - // internalError answers a request that failed on this side. func internalError(w http.ResponseWriter, err error) { log.Printf("internal error: %v", err) diff --git a/server/formrequest.go b/server/formrequest.go index d3c3b3f3..6d0d82fa 100644 --- a/server/formrequest.go +++ b/server/formrequest.go @@ -23,16 +23,22 @@ const maxFormRequestBytes = 1 << 20 // there's no reason for every adapter to reimplement it. A caller // fronted by something other than net/http.Request is free to build a // FormRequest by hand instead. +// +// A body it can't take — not form-encoded, unreadable, over 1 MiB, or +// malformed — is an invalid_request *Error (400), so WriteError answers +// it as the client's mistake, never a 500. The same goes for the +// *FromHTTP constructors built on it (PushAuthorizationRequestFromHTTP, +// TokenEndpointRequestFromHTTP, BeginBackchannelAuthenticationRequestFromHTTP). func FormRequestFromHTTP(r *http.Request) (FormRequest, error) { if ct := r.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/x-www-form-urlencoded") { - return FormRequest{}, fmt.Errorf("server: unexpected content type %q", ct) + return FormRequest{}, newError(ErrorInvalidRequest, http.StatusBadRequest, "the request body must be application/x-www-form-urlencoded", fmt.Errorf("content type %q", ct)) } body, err := io.ReadAll(io.LimitReader(r.Body, maxFormRequestBytes+1)) if err != nil { - return FormRequest{}, fmt.Errorf("server: read body: %w", err) + return FormRequest{}, newError(ErrorInvalidRequest, http.StatusBadRequest, "the request body could not be read", err) } if len(body) > maxFormRequestBytes { - return FormRequest{}, fmt.Errorf("server: body exceeds %d bytes", maxFormRequestBytes) + return FormRequest{}, newError(ErrorInvalidRequest, http.StatusBadRequest, "the request body is too large", fmt.Errorf("body exceeds %d bytes", maxFormRequestBytes)) } var params []FormParameter @@ -43,11 +49,11 @@ func FormRequestFromHTTP(r *http.Request) (FormRequest, error) { name, value, _ := strings.Cut(pair, "=") decodedName, err := url.QueryUnescape(name) if err != nil { - return FormRequest{}, fmt.Errorf("server: malformed parameter name: %w", err) + return FormRequest{}, newError(ErrorInvalidRequest, http.StatusBadRequest, "the request body is malformed", fmt.Errorf("parameter name: %w", err)) } decodedValue, err := url.QueryUnescape(value) if err != nil { - return FormRequest{}, fmt.Errorf("server: malformed parameter value: %w", err) + return FormRequest{}, newError(ErrorInvalidRequest, http.StatusBadRequest, "the request body is malformed", fmt.Errorf("parameter value: %w", err)) } params = append(params, FormParameter{Name: decodedName, Value: decodedValue}) } diff --git a/server/formrequest_error_test.go b/server/formrequest_error_test.go new file mode 100644 index 00000000..34e344c1 --- /dev/null +++ b/server/formrequest_error_test.go @@ -0,0 +1,62 @@ +package server_test + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/idfoundry/fapigo/server" +) + +type failingBody struct{} + +func (failingBody) Read([]byte) (int, error) { return 0, errors.New("connection reset") } + +// TestFromHTTPBodyFailuresAreInvalidRequest covers every way a request +// body can't be taken, through each constructor built on +// FormRequestFromHTTP: each is a 400 invalid_request *Error, so +// WriteError answers the client's mistake as one, not as a 500. +func TestFromHTTPBodyFailuresAreInvalidRequest(t *testing.T) { + const form = "application/x-www-form-urlencoded" + bodies := map[string]func() *http.Request{ + "wrong content type": func() *http.Request { return newFormPOST(t, "application/json", `{}`) }, + "malformed name": func() *http.Request { return newFormPOST(t, form, "%zz=value") }, + "malformed value": func() *http.Request { return newFormPOST(t, form, "grant_type=%zz") }, + "oversized": func() *http.Request { return newFormPOST(t, form, "a="+strings.Repeat("a", 1<<20+1)) }, + "unreadable": func() *http.Request { + r := newFormPOST(t, form, "") + r.Body = io.NopCloser(failingBody{}) + return r + }, + } + constructors := map[string]func(*http.Request) error{ + "FormRequestFromHTTP": func(r *http.Request) error { _, err := server.FormRequestFromHTTP(r); return err }, + "PushAuthorizationRequestFromHTTP": func(r *http.Request) error { + _, err := server.PushAuthorizationRequestFromHTTP(r) + return err + }, + "TokenEndpointRequestFromHTTP": func(r *http.Request) error { _, err := server.TokenEndpointRequestFromHTTP(r); return err }, + "BeginBackchannelAuthenticationRequestFromHTTP": func(r *http.Request) error { + _, err := server.BeginBackchannelAuthenticationRequestFromHTTP(r) + return err + }, + } + for body, request := range bodies { + for name, construct := range constructors { + err := construct(request()) + var serr *server.Error + if !errors.As(err, &serr) || serr.Code() != server.ErrorInvalidRequest || serr.HTTPStatus() != http.StatusBadRequest { + t.Errorf("%s(%s) = %v, want a 400 invalid_request *server.Error", name, body, err) + continue + } + w := httptest.NewRecorder() + server.WriteError(w, err) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), `"invalid_request"`) { + t.Errorf("WriteError(%s(%s)) = %d %s, want 400 invalid_request", name, body, w.Code, w.Body) + } + } + } +}