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