From 3ec7846b0c6ebab4df6b80b931869086c3980588 Mon Sep 17 00:00:00 2001 From: Jilles van Gurp Date: Thu, 11 Jun 2026 09:31:07 +0200 Subject: [PATCH] Improve REST 4xx error responses --- cmd/hub/main.go | 7 +- docs/auth.md | 17 ++- internal/auth/errors.go | 13 ++- internal/auth/permissions.go | 66 ++++++++++-- internal/httpapi/gen/api.gen.go | 3 + internal/httpapi/handlers/handlers.go | 116 ++++++++++++++++----- internal/httpapi/handlers/handlers_test.go | 11 +- internal/hub/service.go | 1 + specifications/openapi/omlox-hub.v0.yaml | 5 + 9 files changed, 200 insertions(+), 39 deletions(-) diff --git a/cmd/hub/main.go b/cmd/hub/main.go index bc6ddde..57c1b8f 100644 --- a/cmd/hub/main.go +++ b/cmd/hub/main.go @@ -300,6 +300,8 @@ func runWithRuntime(ctx context.Context, rt runtimeDeps) error { } r := chi.NewRouter() + r.NotFound(handlers.WriteNotFound) + r.MethodNotAllowed(handlers.WriteMethodNotAllowed) r.Use(observability.RequestLogger(logger)) r.Use(auth.Middleware(authenticator, cfg.Auth, registry, r)) @@ -323,7 +325,10 @@ func runWithRuntime(ctx context.Context, rt runtimeDeps) error { RPC: rpcBridge, RequestBodyLimitBytes: cfg.HTTPRequestBodyLimitBytes, }) - gen.HandlerFromMux(h, r) + gen.HandlerWithOptions(h, gen.ChiServerOptions{ + BaseRouter: r, + ErrorHandlerFunc: handlers.WriteRequestError, + }) wsHub := ws.New( logger, service, diff --git a/docs/auth.md b/docs/auth.md index fef29cf..a9965db 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -153,9 +153,22 @@ For `*_OWN` permissions, the request path parameter must be present in the match ## Error Handling +- REST errors use the standard JSON envelope: + +```json +{ + "type": "bad_request", + "code": 400, + "message": "invalid request body", + "details": ["JSON syntax error at byte offset 18: invalid character '}' looking for beginning of object key string"] +} +``` + - `401 Unauthorized`: missing bearer header, malformed token, invalid signature, bad issuer, bad audience, expired token, or other authentication failure -- `403 Forbidden`: authenticated token lacks a matching permission or ownership claim -- `403 Forbidden` on RPC also covers missing method-level discovery or invocation permission +- `400 Bad Request`: invalid JSON, multiple JSON documents, oversized request bodies, invalid path parameters, or service validation failures; the `details` array identifies the field, parameter, byte offset, limit, or validation rule where possible +- `403 Forbidden`: authenticated token lacks a matching permission or ownership claim; REST responses include the required permission pair, the matched route rule when one exists, and ownership claim details when `_OWN` permissions are involved +- `403 Forbidden` on RPC and WebSocket policy checks also names the missing discovery, invocation, subscribe, or publish rule +- `404 Not Found`: unknown REST paths name the requested path; missing resources return the resource-specific not-found message - WebSocket auth failures are returned as OMLOX wrapper `error` events with code `10004` Authentication failures return a `WWW-Authenticate: Bearer` header and the API error body. diff --git a/internal/auth/errors.go b/internal/auth/errors.go index c8d79a7..cbabe79 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -11,6 +11,7 @@ type authError struct { status int typ string message string + details []string challenge string } @@ -20,6 +21,7 @@ func (e *authError) Type() string { return e.typ } func (e *authError) Message() string { return e.message } +func (e *authError) Details() []string { return e.details } func unauthorized(message string) error { return &authError{ @@ -30,11 +32,12 @@ func unauthorized(message string) error { } } -func forbidden(message string) error { +func forbidden(message string, details ...string) error { return &authError{ status: http.StatusForbidden, typ: "authorization_failed", message: message, + details: details, } } @@ -53,9 +56,13 @@ func writeAuthError(w http.ResponseWriter, err error) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(authErr.status) msg := authErr.message - _ = json.NewEncoder(w).Encode(gen.ErrorResponse{ + body := gen.ErrorResponse{ Type: authErr.typ, Code: authErr.status, Message: &msg, - }) + } + if len(authErr.details) > 0 { + body.Details = &authErr.details + } + _ = json.NewEncoder(w).Encode(body) } diff --git a/internal/auth/permissions.go b/internal/auth/permissions.go index 2c251fe..835b1cf 100644 --- a/internal/auth/permissions.go +++ b/internal/auth/permissions.go @@ -261,13 +261,14 @@ func validateRule(pattern string, perms map[Permission]struct{}) error { func (r *Registry) Authorize(req *http.Request, principal *Principal) error { requiredAny, requiredOwn, err := permissionsForMethod(req.Method) if err != nil { - return forbidden(err.Error()) + return forbidden("request method is not authorized", err.Error()) } if principal == nil { return unauthorized("missing authenticated principal") } matched := false + var matchedDetails []string for _, role := range principal.Roles { policy := r.roles[role] rule, params, ok := bestMatchingRule(policy.Routes, req.URL.Path) @@ -281,11 +282,15 @@ func (r *Registry) Authorize(req *http.Request, principal *Principal) error { if _, ok := rule.Permissions[requiredOwn]; ok && ownsAll(principal, params) { return nil } + matchedDetails = append(matchedDetails, authorizationFailureDetails(rule, params, principal, requiredAny, requiredOwn)...) } if !matched { - return forbidden("token does not grant access to this endpoint") + return forbidden("token does not grant access to this endpoint", + fmt.Sprintf("no authorization rule matched %s %s for the token roles", req.Method, req.URL.Path), + fmt.Sprintf("the request requires %s or %s on a matching route", requiredAny, requiredOwn), + ) } - return forbidden("token does not grant access to this endpoint") + return forbidden("token does not satisfy the authorization rule for this endpoint", matchedDetails...) } // AuthorizeRPCDiscover checks whether the principal may list reachable RPC methods. @@ -298,7 +303,7 @@ func (r *Registry) AuthorizeRPCDiscover(principal *Principal) error { return nil } } - return forbidden("token does not grant access to rpc method discovery") + return forbidden("token does not grant access to rpc method discovery", "the token roles must grant rpc.discover=true") } // AuthorizeRPCInvoke checks whether the principal may invoke the supplied RPC method. @@ -311,7 +316,7 @@ func (r *Registry) AuthorizeRPCInvoke(principal *Principal, method string) error return nil } } - return forbidden("token does not grant access to rpc method invocation") + return forbidden("token does not grant access to rpc method invocation", fmt.Sprintf("no rpc.invoke rule matched method %q", method)) } // AuthorizeWebSocketSubscribe checks whether the principal may subscribe to @@ -325,7 +330,7 @@ func (r *Registry) AuthorizeWebSocketSubscribe(principal *Principal, topic strin return nil } } - return forbidden("token does not grant access to websocket topic subscription") + return forbidden("token does not grant access to websocket topic subscription", fmt.Sprintf("no websocket.subscribe rule matched topic %q", topic)) } // AuthorizeWebSocketPublish checks whether the principal may publish to the @@ -339,7 +344,54 @@ func (r *Registry) AuthorizeWebSocketPublish(principal *Principal, topic string) return nil } } - return forbidden("token does not grant access to websocket topic publish") + return forbidden("token does not grant access to websocket topic publish", fmt.Sprintf("no websocket.publish rule matched topic %q", topic)) +} + +func authorizationFailureDetails(rule Rule, params map[string]string, principal *Principal, requiredAny, requiredOwn Permission) []string { + details := []string{ + fmt.Sprintf("matched route pattern %q", rule.Pattern), + fmt.Sprintf("request requires %s, or %s with matching ownership claims", requiredAny, requiredOwn), + fmt.Sprintf("matched rule grants: %s", formatPermissions(rule.Permissions)), + } + if _, ok := rule.Permissions[requiredOwn]; ok { + details = append(details, ownershipFailureDetails(params, principal)...) + } + return details +} + +func formatPermissions(perms map[Permission]struct{}) string { + if len(perms) == 0 { + return "" + } + items := make([]string, 0, len(perms)) + for perm := range perms { + items = append(items, string(perm)) + } + sort.Strings(items) + return strings.Join(items, ", ") +} + +func ownershipFailureDetails(params map[string]string, principal *Principal) []string { + if len(params) == 0 { + return []string{"ownership could not be evaluated because the route has no path parameters"} + } + details := make([]string, 0, len(params)) + for name, value := range params { + key := ownedResourceKey(name) + values, ok := principal.OwnedResources[key] + switch { + case !ok: + details = append(details, fmt.Sprintf("ownership claim %q is missing for path parameter %q", key, name)) + case values == nil: + details = append(details, fmt.Sprintf("ownership claim %q is empty for path parameter %q", key, name)) + default: + if _, ok := values[value]; !ok { + details = append(details, fmt.Sprintf("ownership claim %q does not include requested %s value %q", key, name, value)) + } + } + } + sort.Strings(details) + return details } func permissionsForMethod(method string) (Permission, Permission, error) { diff --git a/internal/httpapi/gen/api.gen.go b/internal/httpapi/gen/api.gen.go index c702420..09d727c 100644 --- a/internal/httpapi/gen/api.gen.go +++ b/internal/httpapi/gen/api.gen.go @@ -208,6 +208,9 @@ type ErrorResponse struct { // Code HTTP status code associated with the error. Code int `json:"code"` + // Details Actionable validation, routing, or authorization details that explain how to correct the request. + Details *[]string `json:"details,omitempty"` + // Message Human-readable error message. Message *string `json:"message,omitempty"` diff --git a/internal/httpapi/handlers/handlers.go b/internal/httpapi/handlers/handlers.go index 2a5f930..85ea9d1 100644 --- a/internal/httpapi/handlers/handlers.go +++ b/internal/httpapi/handlers/handlers.go @@ -4,12 +4,15 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" + "strings" "github.com/formation-res/open-location-hub/internal/httpapi/gen" "github.com/formation-res/open-location-hub/internal/hub" "github.com/formation-res/open-location-hub/internal/observability" + "github.com/go-chi/chi/v5" "go.uber.org/zap" ) @@ -618,7 +621,7 @@ func (h *Handler) PutRPC(w http.ResponseWriter, r *http.Request) { func decodeJSONBody(w http.ResponseWriter, r *http.Request, limit int64, dst any) error { if err := decodeSingleJSONDocument(w, r, limit, dst); err != nil { - return &hub.HTTPError{Status: 400, Type: "bad_request", Message: "invalid request body"} + return badRequestError("invalid request body", describeBodyDecodeError(err, limit)) } return nil } @@ -626,7 +629,7 @@ func decodeJSONBody(w http.ResponseWriter, r *http.Request, limit int64, dst any func readRawBody(w http.ResponseWriter, r *http.Request, limit int64) (json.RawMessage, error) { var raw json.RawMessage if err := decodeSingleJSONDocument(w, r, limit, &raw); err != nil { - return nil, &hub.HTTPError{Status: 400, Type: "bad_request", Message: "invalid request body"} + return nil, badRequestError("invalid request body", describeBodyDecodeError(err, limit)) } return raw, nil } @@ -666,13 +669,7 @@ func writeJSONOrError(w http.ResponseWriter, payload any, err error, successStat if err != nil { var httpErr *hub.HTTPError if errors.As(err, &httpErr) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(httpErr.Status) - _ = json.NewEncoder(w).Encode(gen.ErrorResponse{ - Type: httpErr.Type, - Code: httpErr.Status, - Message: &httpErr.Message, - }) + writeErrorResponse(w, httpErr.Status, httpErr.Type, httpErr.Message, httpErr.Details) return } var authErr interface { @@ -681,24 +678,18 @@ func writeJSONOrError(w http.ResponseWriter, payload any, err error, successStat Message() string } if errors.As(err, &authErr) { - message := authErr.Message() - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(authErr.Status()) - _ = json.NewEncoder(w).Encode(gen.ErrorResponse{ - Type: authErr.Type(), - Code: authErr.Status(), - Message: &message, - }) + var detailed interface { + Details() []string + } + var details []string + if errors.As(err, &detailed) { + details = detailed.Details() + } + writeErrorResponse(w, authErr.Status(), authErr.Type(), authErr.Message(), details) return } message := err.Error() - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusInternalServerError) - _ = json.NewEncoder(w).Encode(gen.ErrorResponse{ - Type: "internal_error", - Code: http.StatusInternalServerError, - Message: &message, - }) + writeErrorResponse(w, http.StatusInternalServerError, "internal_error", message, nil) return } w.Header().Set("Content-Type", "application/json") @@ -716,6 +707,83 @@ func writeStubNotImplemented(w http.ResponseWriter, message string) { }, 0) } +// WriteRequestError renders OpenAPI path and parameter validation failures as +// the standard JSON error envelope. +func WriteRequestError(w http.ResponseWriter, r *http.Request, err error) { + detail := err.Error() + var invalidParam *gen.InvalidParamFormatError + if errors.As(err, &invalidParam) { + detail = fmt.Sprintf("path parameter %q has invalid value %q: expected %s", invalidParam.ParamName, chiURLParam(r, invalidParam.ParamName), invalidParam.Err.Error()) + } + writeErrorResponse(w, http.StatusBadRequest, "bad_request", "invalid request parameters", []string{detail}) +} + +// WriteNotFound renders unmatched routes as a JSON 404 that identifies the +// requested method and path. +func WriteNotFound(w http.ResponseWriter, r *http.Request) { + message := fmt.Sprintf("path %s does not exist", r.URL.Path) + details := []string{fmt.Sprintf("%s %s did not match any Open Location Hub REST route", r.Method, r.URL.Path)} + writeErrorResponse(w, http.StatusNotFound, "not_found", message, details) +} + +// WriteMethodNotAllowed renders known paths with unsupported methods as JSON. +func WriteMethodNotAllowed(w http.ResponseWriter, r *http.Request) { + message := fmt.Sprintf("method %s is not allowed for path %s", r.Method, r.URL.Path) + details := []string{"use one of the HTTP methods documented for this Open Location Hub REST path"} + writeErrorResponse(w, http.StatusMethodNotAllowed, "method_not_allowed", message, details) +} + +func badRequestError(message string, details []string) error { + return &hub.HTTPError{Status: http.StatusBadRequest, Type: "bad_request", Message: message, Details: details} +} + +func describeBodyDecodeError(err error, limit int64) []string { + switch { + case errors.Is(err, io.EOF): + return []string{"request body is empty; send a single JSON document"} + case strings.Contains(err.Error(), "http: request body too large"): + return []string{fmt.Sprintf("request body exceeds the configured limit of %d bytes", limit)} + } + var syntaxErr *json.SyntaxError + if errors.As(err, &syntaxErr) { + return []string{fmt.Sprintf("JSON syntax error at byte offset %d: %s", syntaxErr.Offset, syntaxErr.Error())} + } + var typeErr *json.UnmarshalTypeError + if errors.As(err, &typeErr) { + field := typeErr.Field + if field == "" { + field = typeErr.Value + } + return []string{fmt.Sprintf("JSON field %q has value type %q but must be %s", field, typeErr.Value, typeErr.Type.String())} + } + if err.Error() == "unexpected trailing JSON content" { + return []string{"request body must contain exactly one JSON document; remove trailing content after the first document"} + } + return []string{err.Error()} +} + +func writeErrorResponse(w http.ResponseWriter, status int, typ, message string, details []string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + body := gen.ErrorResponse{ + Type: typ, + Code: status, + Message: &message, + } + if len(details) > 0 { + body.Details = &details + } + _ = json.NewEncoder(w).Encode(body) +} + +func chiURLParam(r *http.Request, name string) string { + value := chi.URLParam(r, name) + if value == "" { + return "" + } + return value +} + func (h *Handler) extendedService() (extendedService, bool) { svc, ok := h.deps.Service.(extendedService) return svc, ok diff --git a/internal/httpapi/handlers/handlers_test.go b/internal/httpapi/handlers/handlers_test.go index 7377793..ba32a96 100644 --- a/internal/httpapi/handlers/handlers_test.go +++ b/internal/httpapi/handlers/handlers_test.go @@ -160,6 +160,8 @@ func TestHandlerRoutesExerciseSuccessAndFailurePaths(t *testing.T) { {name: "get fence", method: http.MethodGet, path: "/v2/fences/" + fenceID.String(), wantStatus: http.StatusOK, wantContains: fenceID.String()}, {name: "delete fence", method: http.MethodDelete, path: "/v2/fences/" + fenceID.String(), wantStatus: http.StatusNoContent}, {name: "rpc available auth error", method: http.MethodGet, path: "/v2/rpc/available", wantStatus: http.StatusForbidden, wantContains: `"rpc discover denied"`}, + {name: "unknown path", method: http.MethodGet, path: "/v2/does-not-exist", wantStatus: http.StatusNotFound, wantContains: `"path /v2/does-not-exist does not exist"`}, + {name: "invalid path parameter", method: http.MethodGet, path: "/v2/zones/not-a-uuid", wantStatus: http.StatusBadRequest, wantContains: `"path parameter \"zoneId\" has invalid value \"not-a-uuid\"`}, } for _, tc := range tests { @@ -394,11 +396,16 @@ func TestReadRawBodyAcceptsArbitraryJSON(t *testing.T) { func newTestServer(svc Service, rpcBridge RPCBridge) http.Handler { router := chi.NewRouter() - return gen.HandlerFromMux(New(Dependencies{ + router.NotFound(WriteNotFound) + router.MethodNotAllowed(WriteMethodNotAllowed) + return gen.HandlerWithOptions(New(Dependencies{ Service: svc, RPC: rpcBridge, RequestBodyLimitBytes: testRequestBodyLimitBytes, - }), router) + }), gen.ChiServerOptions{ + BaseRouter: router, + ErrorHandlerFunc: WriteRequestError, + }) } func jsonEqual(a, b any) bool { diff --git a/internal/hub/service.go b/internal/hub/service.go index 85e2ffd..5f47b91 100644 --- a/internal/hub/service.go +++ b/internal/hub/service.go @@ -82,6 +82,7 @@ type HTTPError struct { Status int Type string Message string + Details []string } func (e *HTTPError) Error() string { diff --git a/specifications/openapi/omlox-hub.v0.yaml b/specifications/openapi/omlox-hub.v0.yaml index 2a822b8..cec6fc6 100644 --- a/specifications/openapi/omlox-hub.v0.yaml +++ b/specifications/openapi/omlox-hub.v0.yaml @@ -1240,6 +1240,11 @@ components: message: type: string description: Human-readable error message. + details: + type: array + description: Actionable validation, routing, or authorization details that explain how to correct the request. + items: + type: string ResourceSummary: type: object