From 1886680e95a79f67d8fa82bb6f7482ba8e35781a Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Mon, 21 Sep 2026 13:02:29 -0400 Subject: [PATCH] fix(server): answer every process-generated error with the envelope Three responses the process generated were text/plain: an unmatched /api/ path (404), a method the route does not accept (405), and a parameter that fails to bind before the handler runs, where the generated router's default wrote the parser's own error text, rejected value included. Three more sat in the SSE handler before the stream starts. A client that parses every non-2xx as the envelope failed on all of them and could not tell an OpenWatch 404 from a proxy's. Contract first: system-http-server 1.5.0 adds C-15 (every 4xx or 5xx the process generates carries the envelope; a parameter message names the parameter and never echoes the value; intermediary bodies are out of scope and clients must tolerate them) and AC-20. api-events-stream 1.1.1 restates AC-02 for the envelope. The router is mounted with an ErrorHandlerFunc that maps the generated error types to request.missing_parameter or request.invalid_parameter; the SPA fallback and the 405 handler use writeError; the SSE handler's three early exits use it too. AC-20 exercises the three cases end to end (status, Content-Type, X-Correlation-Id, code, message naming the parameter and not the value) and walks internal/server for any bare http.Error call so a new plain-text error fails the criterion. The API guide's error section states the behavior and keeps the client guidance for non-JSON infrastructure errors. CP: bugs/doing/OW-063 --- docs/guides/API_GUIDE.md | 8 ++ internal/server/request_errors.go | 61 ++++++++++++ internal/server/request_errors_test.go | 129 +++++++++++++++++++++++++ internal/server/server.go | 9 +- internal/server/spa.go | 4 +- internal/server/sse_handler.go | 6 +- internal/server/sse_handler_test.go | 7 +- specs/api/events-stream.spec.yaml | 4 +- specs/system/http-server.spec.yaml | 35 ++++++- 9 files changed, 251 insertions(+), 12 deletions(-) create mode 100644 internal/server/request_errors.go create mode 100644 internal/server/request_errors_test.go diff --git a/docs/guides/API_GUIDE.md b/docs/guides/API_GUIDE.md index 51dacddf5..53641c949 100644 --- a/docs/guides/API_GUIDE.md +++ b/docs/guides/API_GUIDE.md @@ -373,6 +373,14 @@ rate-limited per client IP and return `429` with a `Retry-After` header over the limit. There is no `422` validation status: validation failures return `400` with the envelope above. +Every error the OpenWatch process generates carries the envelope, including +a `404` for an `/api/` path that does not exist, a `405`, and a `400` for a +query parameter that is missing or fails to parse; the message names the +parameter and never repeats the rejected value. A proxy or load balancer in +front of OpenWatch produces its own `502`, `503` or `504` bodies, so a client +should treat any non-2xx whose body is not JSON as an infrastructure error +rather than fail on the parse. + --- ## Operations: the CLI and systemd diff --git a/internal/server/request_errors.go b/internal/server/request_errors.go new file mode 100644 index 000000000..7eea2dbf1 --- /dev/null +++ b/internal/server/request_errors.go @@ -0,0 +1,61 @@ +package server + +import ( + "errors" + "net/http" + + "github.com/Hanalyx/openwatch/internal/server/api" +) + +// requestErrorHandler turns the generated router's parameter-binding +// failures into the canonical envelope. The generator's default answers +// text/plain with the parser's own error text, which repeats the rejected +// value back to the client. The envelope names the parameter and nothing +// else: the value is the client's own input and the parser's wording is +// internal detail. Spec: system-http-server C-15. +func requestErrorHandler(w http.ResponseWriter, _ *http.Request, err error) { + var ( + required *api.RequiredParamError + reqHeader *api.RequiredHeaderError + format *api.InvalidParamFormatError + tooMany *api.TooManyValuesForParamError + cookie *api.UnescapedCookieParamError + unmarshal *api.UnmarshalingParamError + ) + switch { + case errors.As(err, &required): + writeError(w, http.StatusBadRequest, "request.missing_parameter", "client", + "parameter "+required.ParamName+" is required", false) + case errors.As(err, &reqHeader): + writeError(w, http.StatusBadRequest, "request.missing_parameter", "client", + "header "+reqHeader.ParamName+" is required", false) + case errors.As(err, &format): + writeError(w, http.StatusBadRequest, "request.invalid_parameter", "client", + "parameter "+format.ParamName+" is not valid", false) + case errors.As(err, &tooMany): + writeError(w, http.StatusBadRequest, "request.invalid_parameter", "client", + "parameter "+tooMany.ParamName+" was given more than once", false) + case errors.As(err, &cookie): + writeError(w, http.StatusBadRequest, "request.invalid_parameter", "client", + "cookie "+cookie.ParamName+" is not valid", false) + case errors.As(err, &unmarshal): + writeError(w, http.StatusBadRequest, "request.invalid_parameter", "client", + "parameter "+unmarshal.ParamName+" is not valid", false) + default: + writeError(w, http.StatusBadRequest, "request.invalid", "client", + "the request could not be parsed", false) + } +} + +// writeNotFound is the envelope for an /api/ path no route matches. +func writeNotFound(w http.ResponseWriter) { + writeError(w, http.StatusNotFound, "request.not_found", "client", + "no such API route", false) +} + +// writeMethodNotAllowed is the envelope for a route that does not accept +// the request's method. +func writeMethodNotAllowed(w http.ResponseWriter) { + writeError(w, http.StatusMethodNotAllowed, "request.method_not_allowed", "client", + "the route does not accept this method", false) +} diff --git a/internal/server/request_errors_test.go b/internal/server/request_errors_test.go new file mode 100644 index 000000000..206d92664 --- /dev/null +++ b/internal/server/request_errors_test.go @@ -0,0 +1,129 @@ +// @spec system-http-server +package server + +import ( + "encoding/json" + "io" + "net/http" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/Hanalyx/openwatch/internal/auth" +) + +// Spec: specs/system/http-server.spec.yaml +// +// AC-20 TestRequestErrors_EnvelopeEverywhere + +type envelopeBody struct { + Error struct { + Code string `json:"code"` + Fault string `json:"fault"` + HumanMessage string `json:"human_message"` + CorrelationID string `json:"correlation_id"` + } `json:"error"` +} + +func readEnvelope(t *testing.T, resp *http.Response) (envelopeBody, string) { + t.Helper() + raw, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + var env envelopeBody + if err := json.Unmarshal(raw, &env); err != nil { + t.Fatalf("body is not the envelope: %v; body=%q", err, raw) + } + return env, string(raw) +} + +// @ac AC-20 +// AC-20: every 4xx the process itself generates is the envelope, including +// the three that no handler produces: an unmatched /api/ path, a method the +// route does not accept, and a parameter that fails to bind. The parameter +// message names the parameter and never repeats the rejected value. +func TestRequestErrors_EnvelopeEverywhere(t *testing.T) { + t.Run("system-http-server/AC-20", func(t *testing.T) { + srv, _ := freshAPIServer(t) + + cases := []struct { + name string + req *http.Request + status int + code string + mentions string + forbidden string + }{ + { + name: "unmatched /api/ path is a 404 envelope", + req: asRole(t, "GET", srv+"/api/v1/definitely-not-a-route", auth.RoleViewer, nil), + status: http.StatusNotFound, code: "request.not_found", + }, + { + name: "method the route does not accept is a 405 envelope", + req: asRole(t, "DELETE", srv+"/api/v1/health", auth.RoleViewer, nil), + status: http.StatusMethodNotAllowed, code: "request.method_not_allowed", + }, + { + name: "parameter that fails to bind is a 400 envelope naming the parameter only", + req: asRole(t, "GET", srv+"/api/v1/audit/events?limit=abc", auth.RoleViewer, nil), + status: http.StatusBadRequest, code: "request.invalid_parameter", + mentions: "limit", forbidden: "abc", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := doReq(t, tc.req) + defer resp.Body.Close() + if resp.StatusCode != tc.status { + t.Fatalf("status = %d, want %d", resp.StatusCode, tc.status) + } + if ct := resp.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if resp.Header.Get("X-Correlation-Id") == "" { + t.Errorf("no X-Correlation-Id header on the error response") + } + env, raw := readEnvelope(t, resp) + if env.Error.Code != tc.code { + t.Errorf("error.code = %q, want %q", env.Error.Code, tc.code) + } + if env.Error.Fault != "client" { + t.Errorf("error.fault = %q, want client", env.Error.Fault) + } + if tc.mentions != "" && !strings.Contains(env.Error.HumanMessage, tc.mentions) { + t.Errorf("human_message %q does not name %q", env.Error.HumanMessage, tc.mentions) + } + if tc.forbidden != "" && strings.Contains(raw, tc.forbidden) { + t.Errorf("body echoes the rejected value %q: %s", tc.forbidden, raw) + } + }) + } + + // Structural half: no bare http.Error in the package outside tests + // and generated code, so a new plain-text error cannot be added + // without this criterion noticing. + t.Run("no bare http.Error in internal/server", func(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + bare := regexp.MustCompile(`\bhttp\.Error\(`) + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + src, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + if bare.Match(src) { + t.Errorf("%s calls http.Error; use writeError so the response is the envelope (C-15)", f) + } + } + }) + }) +} diff --git a/internal/server/server.go b/internal/server/server.go index 8c1524f96..bba821a90 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -307,7 +307,7 @@ func New(cfg *config.Config, pool *pgxpool.Pool) *Server { // fallback) for everything else. r.NotFound(newSPAHandler().ServeHTTP) r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) { - http.Error(w, "405 method not allowed", http.StatusMethodNotAllowed) + writeMethodNotAllowed(w) // C-15: the envelope, not text/plain }) // Mount the Stage-0 API routes via oapi-codegen's HandlerFromMux. @@ -341,7 +341,12 @@ func New(cfg *config.Config, pool *pgxpool.Pool) *Server { } primeCancel() } - api.HandlerFromMux(apiHandlers, r) + // ErrorHandlerFunc replaces the generator's text/plain default for a + // parameter that is missing or fails to parse (C-15). + api.HandlerWithOptions(apiHandlers, api.ChiServerOptions{ + BaseRouter: r, + ErrorHandlerFunc: requestErrorHandler, + }) _ = license.PremiumDiagnostics // ensure import is exercised // OpenAPI spec + Swagger UI. The handlers do not call diff --git a/internal/server/spa.go b/internal/server/spa.go index 54de73926..6e0f9ae00 100644 --- a/internal/server/spa.go +++ b/internal/server/spa.go @@ -158,12 +158,12 @@ func contentTypeFor(p string) string { func (h *spaHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if strings.HasPrefix(r.URL.Path, "/api/") { - http.Error(w, "404 page not found", http.StatusNotFound) + writeNotFound(w) // C-15: the envelope, not text/plain return } if r.Method != http.MethodGet && r.Method != http.MethodHead { w.Header().Set("Allow", "GET, HEAD") - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + writeMethodNotAllowed(w) return } diff --git a/internal/server/sse_handler.go b/internal/server/sse_handler.go index e8e7defc0..3363521f8 100644 --- a/internal/server/sse_handler.go +++ b/internal/server/sse_handler.go @@ -50,7 +50,7 @@ func (h *handlers) GetEventsStream(w http.ResponseWriter, r *http.Request) { } if h.bus == nil { - http.Error(w, "events stream not wired", http.StatusServiceUnavailable) + writeError(w, http.StatusServiceUnavailable, "server.unavailable", "server", "events stream not wired", true) return } @@ -59,13 +59,13 @@ func (h *handlers) GetEventsStream(w http.ResponseWriter, r *http.Request) { // firehose by default). AC-02, AC-03. topics := parseTopics(r.URL.Query().Get("topics")) if len(topics) == 0 { - http.Error(w, "no topics requested", http.StatusBadRequest) + writeError(w, http.StatusBadRequest, "request.missing_parameter", "client", "parameter topics is required", false) return } flusher, ok := w.(http.Flusher) if !ok { - http.Error(w, "streaming unsupported", http.StatusInternalServerError) + writeError(w, http.StatusInternalServerError, "server.internal", "server", "streaming unsupported", false) return } diff --git a/internal/server/sse_handler_test.go b/internal/server/sse_handler_test.go index d5ad1d185..00d2e41a2 100644 --- a/internal/server/sse_handler_test.go +++ b/internal/server/sse_handler_test.go @@ -77,8 +77,11 @@ func TestEventsStream_NoTopicsReturns400(t *testing.T) { if w.Code != http.StatusBadRequest { t.Errorf("status = %d, want 400", w.Code) } - if !strings.Contains(w.Body.String(), "no topics requested") { - t.Errorf("body = %q, want 'no topics requested'", w.Body.String()) + // The body is the canonical envelope (system-http-server C-15), so + // the message lives under error.human_message. + if !strings.Contains(w.Body.String(), `"request.missing_parameter"`) || + !strings.Contains(w.Body.String(), "parameter topics is required") { + t.Errorf("body = %q, want the envelope naming the topics parameter", w.Body.String()) } }) } diff --git a/specs/api/events-stream.spec.yaml b/specs/api/events-stream.spec.yaml index 57c73c451..9d1b6e345 100644 --- a/specs/api/events-stream.spec.yaml +++ b/specs/api/events-stream.spec.yaml @@ -1,7 +1,7 @@ spec: id: api-events-stream title: SSE live-events stream for the operator UI - version: "1.1.0" + version: "1.1.1" status: approved tier: 2 @@ -94,7 +94,7 @@ spec: priority: critical references_constraints: [C-01] - id: AC-02 - description: GET /api/v1/events without ?topics (or with ?topics=) returns 400 with body containing "no topics requested". + description: GET /api/v1/events without ?topics (or with ?topics=) returns 400 carrying the canonical error envelope (system-http-server C-15) with code request.missing_parameter and a human_message naming the topics parameter. priority: critical references_constraints: [C-02] - id: AC-03 diff --git a/specs/system/http-server.spec.yaml b/specs/system/http-server.spec.yaml index ce1f5e3e3..9117c70da 100644 --- a/specs/system/http-server.spec.yaml +++ b/specs/system/http-server.spec.yaml @@ -1,7 +1,7 @@ spec: id: system-http-server title: HTTPS server with TLS hot-reload - version: "1.4.0" + version: "1.5.0" status: approved tier: 2 @@ -111,6 +111,26 @@ spec: type: security enforcement: error + - id: C-15 + description: >- + Every 4xx or 5xx response the OpenWatch process itself generates MUST + carry the canonical error envelope (error.code, error.fault, + error.retryable, error.human_message, error.correlation_id) with + Content-Type application/json, including the responses that no + handler produces: an unmatched /api/ path (404, request.not_found), + a method the route does not accept (405, + request.method_not_allowed), and a request whose path, query, header + or cookie parameter is missing or fails to parse before the handler + runs (400, request.missing_parameter or request.invalid_parameter). + A parameter-binding message names the parameter and never echoes the + rejected value or the parser's internal error text, so a client + cannot be reflected into a body and no internal detail leaks. + Out of scope by construction: bodies produced by a proxy, load + balancer or the kernel in front of the process; a client MUST treat + a non-2xx whose body is not JSON as an infrastructure error rather + than a parse failure, and the API guide says so. + type: technical + enforcement: error acceptance_criteria: - id: AC-01 description: Server.Run() binds to cfg.Server.Listen and accepts HTTPS connections using TLSCert and TLSKey via GetCertificate. @@ -182,3 +202,16 @@ spec: description: A cookie-authenticated unsafe request (session cookie present) with no X-CSRF-Token, or a header that does not match the XSRF-TOKEN cookie, returns 403 authz.csrf_invalid; the same request with a matching X-CSRF-Token proceeds. A request carrying no session cookie, or an Authorization (Bearer) header, or to an /api/v1/auth/* path, is exempt. POST /auth/login Set-Cookies a non-HttpOnly XSRF-TOKEN. priority: critical references_constraints: [C-14] + - id: AC-20 + description: >- + GET an unmatched /api/ path returns 404 with the envelope + (code request.not_found), DELETE /api/v1/health returns 405 with the + envelope (code request.method_not_allowed), and GET + /api/v1/audit/events?limit=abc as a viewer returns 400 with the + envelope (code request.invalid_parameter) whose human_message names + limit and does not contain abc; each carries Content-Type + application/json and an X-Correlation-Id header. A source walk of + internal/server finds no bare http.Error call outside tests, so a + new plain-text error cannot be added without failing this criterion. + priority: high + references_constraints: [C-15]