From 0ba5d84386a16bf2aa88d94974dc3d058a55ba8c Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Thu, 30 Jul 2026 08:03:41 -0500 Subject: [PATCH] fix(router): map middleware errors like handler errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An error returned from middleware produced a 500 with a plain-text body, while the same error returned from a handler produced its real status and JSON envelope: forge.Unauthorized("nope") from a handler -> 401 {"code":401,"error":"nope"} forge.Unauthorized("nope") from middleware -> 500 "nope\n" The typed-handler path calls handleError, which honours StatusCode() and ResponseBody(). The three middleware paths in router_impl.go instead fell back to http.Error whenever no ErrorHandler was installed — which is the default — so every typed error lost its status on the way out. The practical cost is that middleware cannot return errors at all. In authsome, one consumer, all 19 middleware error responses are hand-written ctx.JSON calls and none return a typed error, because returning one yields a 500. That duplicates the envelope in every middleware and lets it drift from the handler-side shape. Use handleError in the fallback for all three sites, so an error carries its status and envelope regardless of where it was returned. An installed ErrorHandler still wins; only the default path changes. An untyped error still maps to 500, so this promotes typed errors without swallowing anything. Tests assert the two paths produce identical status and body across 401/403/400/404, that the body is a JSON envelope rather than a text dump, and that an untyped error stays a 500. All four fail without the change. --- internal/router/router_impl.go | 25 +++++-- middleware_error_mapping_test.go | 110 +++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 6 deletions(-) create mode 100644 middleware_error_mapping_test.go diff --git a/internal/router/router_impl.go b/internal/router/router_impl.go index 49201b45..69801139 100644 --- a/internal/router/router_impl.go +++ b/internal/router/router_impl.go @@ -658,8 +658,14 @@ func applyMiddleware(h http.Handler, middleware []Middleware, container vessel.V if errorHandler != nil { _ = errorHandler.HandleError(ctx.Context(), err) } else { - // Default error handling - http.Error(w, err.Error(), http.StatusInternalServerError) + // Same mapping the typed-handler path uses, so an error + // carries its status and envelope whether it was returned + // from a handler or from middleware. Falling back to + // http.Error here turned every middleware error into a + // 500 with a plain-text body — a middleware returning + // Unauthorized produced a 500, which pushed callers into + // hand-writing ctx.JSON responses instead. + handleError(ctx, err) } } }) @@ -738,8 +744,14 @@ func applyMiddlewareAndInterceptors( if errorHandler != nil { _ = errorHandler.HandleError(ctx.Context(), err) } else { - // Default error handling - http.Error(w, err.Error(), http.StatusInternalServerError) + // Same mapping the typed-handler path uses, so an error + // carries its status and envelope whether it was returned + // from a handler or from middleware. Falling back to + // http.Error here turned every middleware error into a + // 500 with a plain-text body — a middleware returning + // Unauthorized produced a 500, which pushed callers into + // hand-writing ctx.JSON responses instead. + handleError(ctx, err) } } }) @@ -770,8 +782,9 @@ func convertForgeMiddlewareToHTTP(mw Middleware, container vessel.Vessel, errorH if errorHandler != nil { _ = errorHandler.HandleError(ctx.Context(), err) } else { - // Default error handling - http.Error(w, err.Error(), http.StatusInternalServerError) + // See applyMiddleware — global middleware maps errors + // the same way route middleware does. + handleError(ctx, err) } } }) diff --git a/middleware_error_mapping_test.go b/middleware_error_mapping_test.go new file mode 100644 index 00000000..858cf495 --- /dev/null +++ b/middleware_error_mapping_test.go @@ -0,0 +1,110 @@ +package forge_test + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/xraph/forge" +) + +type meReq struct{} +type meResp struct { + OK bool `json:"ok"` +} + +// post drives a route that either returns err from middleware or from the +// handler, so the two paths can be compared directly. +func post(t *testing.T, fromMiddleware bool, err error) *httptest.ResponseRecorder { + t.Helper() + r := forge.NewRouter() + + handler := func(ctx forge.Context, _ *meReq) (*meResp, error) { + if fromMiddleware { + return &meResp{OK: true}, nil + } + return nil, err + } + + opts := []forge.RouteOption{} + if fromMiddleware { + opts = append(opts, forge.WithMiddleware(func(next forge.Handler) forge.Handler { + return func(ctx forge.Context) error { return err } + })) + } + if regErr := r.POST("/x", handler, opts...); regErr != nil { + t.Fatalf("register route: %v", regErr) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext(context.Background(), "POST", "/x", + bytes.NewReader([]byte(`{}`))) + req.Header.Set("Content-Type", "application/json") + r.ServeHTTP(rec, req) + return rec +} + +// An error returned from middleware must produce the same status and body as +// the same error returned from a handler. +// +// Previously the middleware path fell back to http.Error, so a +// forge.Unauthorized became a 500 with a plain-text body. Callers worked +// around it by hand-writing ctx.JSON responses in middleware, which +// duplicates the envelope and drifts from the handler-side shape. +func TestMiddlewareError_MapsLikeHandlerError(t *testing.T) { + cases := []struct { + name string + err error + want int + }{ + {"unauthorized", forge.Unauthorized("nope"), http.StatusUnauthorized}, + {"forbidden", forge.Forbidden("denied"), http.StatusForbidden}, + {"bad request", forge.BadRequest("bad"), http.StatusBadRequest}, + {"not found", forge.NotFound("gone"), http.StatusNotFound}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fromHandler := post(t, false, tc.err) + fromMiddleware := post(t, true, tc.err) + + if fromHandler.Code != tc.want { + t.Fatalf("handler: got %d, want %d (body %q)", + fromHandler.Code, tc.want, fromHandler.Body.String()) + } + if fromMiddleware.Code != tc.want { + t.Errorf("middleware: got %d, want %d (body %q)", + fromMiddleware.Code, tc.want, fromMiddleware.Body.String()) + } + if fromMiddleware.Body.String() != fromHandler.Body.String() { + t.Errorf("bodies differ:\n handler: %q\n middleware: %q", + fromHandler.Body.String(), fromMiddleware.Body.String()) + } + }) + } +} + +// The body must be the JSON envelope, not a plain-text dump. +func TestMiddlewareError_ProducesJSONEnvelope(t *testing.T) { + rec := post(t, true, forge.Unauthorized("nope")) + + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("middleware error body is not JSON: %q", rec.Body.String()) + } + if body["error"] != "nope" { + t.Errorf("error field: got %v, want %q", body["error"], "nope") + } +} + +// A plain error carries no status, so it must still be a 500 — the fix +// promotes typed errors, it does not swallow untyped ones. +func TestMiddlewareError_UntypedStaysInternalError(t *testing.T) { + rec := post(t, true, context.DeadlineExceeded) + if rec.Code != http.StatusInternalServerError { + t.Errorf("got %d, want 500 (body %q)", rec.Code, rec.Body.String()) + } +}