fix(server): answer an unreadable form body as invalid_request

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)

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

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)

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
 		}
 

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

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)

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)

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

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

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)

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

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)

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

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)

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

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)

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

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