diff --git a/docs/guides/API_GUIDE.md b/docs/guides/API_GUIDE.md index 51dacddf..53641c94 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 00000000..7eea2dbf --- /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 00000000..206d9266 --- /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 8c1524f9..bba821a9 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 54de7392..6e0f9ae0 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 e8e7defc..3363521f 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 d5ad1d18..00d2e41a 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 57c73c45..9d1b6e34 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 ce1f5e3e..9117c70d 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]