From 09c0eb0085a7a02cca1b37230b849cf79b939a61 Mon Sep 17 00:00:00 2001 From: Martin Riedel <1713643+rado0x54@users.noreply.github.com> Date: Sun, 12 Jul 2026 11:43:40 +0200 Subject: [PATCH] =?UTF-8?q?refactor(go):=20internal/apierr=20owns=20every?= =?UTF-8?q?=20HTTP=20error=20envelope=20=E2=80=94=20collapses=20duplicate?= =?UTF-8?q?=20writeErrs=20(rest/webauthn),=20hydra=20error=20maps,=20DCR?= =?UTF-8?q?=20errors,=20and=20the=20gate's=20raw=20401=20into=20three=20re?= =?UTF-8?q?nderers;=20fixes=20/mcp=20allowlist=20403=20to=20application/js?= =?UTF-8?q?on=20(Node=20parity)=20and=20normalizes=20the=20unreachable=20d?= =?UTF-8?q?efensive=20401s=20in=20ws/agent-proxy/step-up=20to=20the=20{err?= =?UTF-8?q?or}=20shape?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/go-backend-architecture.md | 16 ++++++----- internal/agentproxy/route.go | 5 +++- internal/apierr/apierr.go | 43 ++++++++++++++++++++++++++++++ internal/apierr/apierr_test.go | 42 +++++++++++++++++++++++++++++ internal/auth/gate.go | 6 ++--- internal/auth/ipallowlist.go | 3 ++- internal/hydra/routes.go | 47 ++++++++++++++++----------------- internal/rest/rest.go | 3 ++- internal/webauthn/routes.go | 3 ++- internal/webauthn/stepup.go | 13 ++++----- internal/ws/handler.go | 5 +++- 11 files changed, 139 insertions(+), 47 deletions(-) create mode 100644 internal/apierr/apierr.go create mode 100644 internal/apierr/apierr_test.go diff --git a/docs/go-backend-architecture.md b/docs/go-backend-architecture.md index 4f467e9..e62b8f3 100644 --- a/docs/go-backend-architecture.md +++ b/docs/go-backend-architecture.md @@ -299,13 +299,15 @@ constraint is now one greppable package. scoped, `/mcp`) → bearer gate (§5.9) → handlers. `/ws`, `/mcp`, `/agent-proxy`, Hydra provider pages, and static files mount beside the REST routes on the same chi mux. -- **Error envelopes:** today's three wire shapes render via per-surface - helpers — `rest.writeErr` for the standard `{error}`, the step-up gate's - `{error, code}` (`internal/webauthn/stepup.go`), and mediated DCR's - `{error, error_description}` (`internal/hydra/routes.go`). (The planned - unified `apierr.E` type was never needed; flipping everything to - `{error, code}` later — contract item F — is still a change to three - helpers, not a hunt through handlers.) +- **Error envelopes:** `internal/apierr` owns all three wire shapes — the + standard `{error}` (`apierr.Write`), the step-up gate's `{error, code}` + (`apierr.WriteCode`, contract item F), and mediated DCR's + `{error, error_description}` (`apierr.WriteOAuth`). Every surface (rest, + webauthn, hydra, the bearer gate, the IP allowlist, the ws/agent-proxy + upgrade handlers) renders through it; per-surface `writeJSON` helpers keep + success payloads only. Flipping everything to `{error, code}` post-cutover + (item F) is a change to one package. (MCP tool errors are protocol-level + text blocks — `mcp.errResult` — and stay separate by design.) ### 5.6 Persistence (`internal/store`) (W7–W9, W13) diff --git a/internal/agentproxy/route.go b/internal/agentproxy/route.go index e9ef149..56e06f5 100644 --- a/internal/agentproxy/route.go +++ b/internal/agentproxy/route.go @@ -9,6 +9,7 @@ import ( "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/approval" "github.com/rado0x54/shellwatch/internal/auth" "github.com/rado0x54/shellwatch/internal/realip" @@ -44,7 +45,9 @@ func (d *Deps) Handler() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { principal, ok := auth.PrincipalFrom(r.Context()) if !ok { - http.Error(w, "unauthenticated", http.StatusUnauthorized) + // Defensive: the bearer gate 401s unauthenticated upgrades before + // this handler runs. + apierr.Write(w, http.StatusUnauthorized, "unauthenticated") return } wsc, err := websocket.Accept(w, r, nil) diff --git a/internal/apierr/apierr.go b/internal/apierr/apierr.go new file mode 100644 index 0000000..418bdca --- /dev/null +++ b/internal/apierr/apierr.go @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 +// Package apierr owns every HTTP error envelope ShellWatch emits. The wire +// contract (docs/api/, pinned by the err-* goldens) has three shapes: +// +// - {error} — the standard envelope (REST, WebAuthn, +// bearer gate, Hydra provider pages, IP allowlist) +// - {error, code} — the step-up gate's machine-readable 401 +// (contract item F, the only coded error today) +// - {error, error_description} — mediated DCR, RFC 7591 wording +// +// Handlers must not hand-roll error bodies: rendering through one package is +// what makes the planned post-cutover envelope convergence (item F — fold +// {error} into {error, code}) a change here instead of a hunt through +// handlers. Success payloads stay with the per-surface writeJSON helpers. +package apierr + +import ( + "encoding/json" + "net/http" +) + +func write(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +// Write renders the standard {error} envelope. Status is caller-chosen — the +// Hydra provider pages ship {error} bodies with 200 (pinned; the SPA +// branches on the field, not the status). +func Write(w http.ResponseWriter, status int, msg string) { + write(w, status, map[string]string{"error": msg}) +} + +// WriteCode renders the step-up {error, code} envelope. +func WriteCode(w http.ResponseWriter, status int, msg, code string) { + write(w, status, map[string]string{"error": msg, "code": code}) +} + +// WriteOAuth renders the {error, error_description} envelope (mediated DCR). +func WriteOAuth(w http.ResponseWriter, status int, code, desc string) { + write(w, status, map[string]string{"error": code, "error_description": desc}) +} diff --git a/internal/apierr/apierr_test.go b/internal/apierr/apierr_test.go new file mode 100644 index 0000000..a2bf78a --- /dev/null +++ b/internal/apierr/apierr_test.go @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 +package apierr + +import ( + "net/http/httptest" + "testing" +) + +// The exact bytes are the contract — handlers across five packages delegate +// here, so a change in encoding (key order, charset, trailing newline) would +// ripple across every error the server emits. +func TestEnvelopes(t *testing.T) { + for _, tc := range []struct { + name string + render func(w *httptest.ResponseRecorder) + status int + body string + }{ + {"standard", func(w *httptest.ResponseRecorder) { Write(w, 404, "Session not found") }, + 404, `{"error":"Session not found"}` + "\n"}, + {"status-200 body", func(w *httptest.ResponseRecorder) { Write(w, 200, "no_passkeys") }, + 200, `{"error":"no_passkeys"}` + "\n"}, + {"step-up code", func(w *httptest.ResponseRecorder) { + WriteCode(w, 401, "Step-up authentication required", "stepup_missing") + }, + 401, `{"code":"stepup_missing","error":"Step-up authentication required"}` + "\n"}, + {"oauth", func(w *httptest.ResponseRecorder) { WriteOAuth(w, 400, "invalid_scope", "scope must be a subset") }, + 400, `{"error":"invalid_scope","error_description":"scope must be a subset"}` + "\n"}, + } { + w := httptest.NewRecorder() + tc.render(w) + if w.Code != tc.status { + t.Errorf("%s: status %d want %d", tc.name, w.Code, tc.status) + } + if got := w.Header().Get("Content-Type"); got != "application/json; charset=utf-8" { + t.Errorf("%s: content-type %q", tc.name, got) + } + if w.Body.String() != tc.body { + t.Errorf("%s: body %q want %q", tc.name, w.Body.String(), tc.body) + } + } +} diff --git a/internal/auth/gate.go b/internal/auth/gate.go index 899b01b..c73869d 100644 --- a/internal/auth/gate.go +++ b/internal/auth/gate.go @@ -11,6 +11,8 @@ import ( "context" "net/http" "strings" + + "github.com/rado0x54/shellwatch/internal/apierr" ) const ( @@ -188,7 +190,5 @@ func send401(w http.ResponseWriter, p GateParams, scope, message, kind string) { parts = append(parts, `error="insufficient_scope"`) } w.Header().Set("WWW-Authenticate", strings.Join(parts, ", ")) - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(status) - _, _ = w.Write([]byte(`{"error":"` + message + `"}`)) + apierr.Write(w, status, message) } diff --git a/internal/auth/ipallowlist.go b/internal/auth/ipallowlist.go index 07494ee..f81aa68 100644 --- a/internal/auth/ipallowlist.go +++ b/internal/auth/ipallowlist.go @@ -9,6 +9,7 @@ import ( "net" "net/http" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/realip" ) @@ -48,7 +49,7 @@ func (c *IPChecker) Allowed(ip string) bool { func (c *IPChecker) Middleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !c.Allowed(realip.FromRequest(r)) { - http.Error(w, `{"error":"Forbidden"}`, http.StatusForbidden) + apierr.Write(w, http.StatusForbidden, "Forbidden") return } next.ServeHTTP(w, r) diff --git a/internal/hydra/routes.go b/internal/hydra/routes.go index f976bb1..353e4aa 100644 --- a/internal/hydra/routes.go +++ b/internal/hydra/routes.go @@ -15,6 +15,7 @@ import ( "github.com/go-chi/chi/v5" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/webauthn" ) @@ -199,11 +200,11 @@ func MountProviders(r chi.Router, p ProviderParams) { r.Post("/api/hydra/login/options", func(w http.ResponseWriter, r *http.Request) { opts, ok, err := p.WebAuthn.LoginOptions(r.Context()) if err != nil { - writeJSONStatus(w, 500, map[string]string{"error": "internal error"}) + apierr.Write(w, 500, "internal error") return } if !ok { - writeJSONStatus(w, 200, map[string]string{"error": "no_passkeys"}) + apierr.Write(w, 200, "no_passkeys") return } writeJSONStatus(w, 200, opts) @@ -217,7 +218,7 @@ func MountProviders(r chi.Router, p ProviderParams) { } _ = json.NewDecoder(r.Body).Decode(&body) if body.LoginChallenge == "" { - writeJSONStatus(w, 400, map[string]string{"error": "missing login_challenge"}) + apierr.Write(w, 400, "missing login_challenge") return } var assertion struct { @@ -227,7 +228,7 @@ func MountProviders(r chi.Router, p ProviderParams) { res := p.WebAuthn.VerifyLogin(r.Context(), body.ChallengeID, assertion.ID, body.Credential) if res.Error != "" { - writeJSONStatus(w, res.Status, map[string]string{"error": res.Error}) + apierr.Write(w, res.Status, res.Error) return } redirect, err := p.Admin.AcceptLoginRequest(r.Context(), body.LoginChallenge, AcceptLogin{ @@ -239,7 +240,7 @@ func MountProviders(r chi.Router, p ProviderParams) { if err != nil { // A stale challenge after a burned assertion is a clean restart, // not a 500 (guarded-admin behavior in routes.ts). - writeJSONStatus(w, 400, map[string]string{"error": "login_flow_expired"}) + apierr.Write(w, 400, "login_flow_expired") return } writeJSONStatus(w, 200, map[string]string{"redirectTo": redirect.RedirectTo}) @@ -266,21 +267,21 @@ func (p ProviderParams) consentOptions(w http.ResponseWriter, r *http.Request) { } _ = json.NewDecoder(r.Body).Decode(&body) if body.ConsentChallenge == "" { - writeJSONStatus(w, 400, map[string]string{"error": "missing consent_challenge"}) + apierr.Write(w, 400, "missing consent_challenge") return } cr, err := p.Admin.GetConsentRequest(r.Context(), body.ConsentChallenge) if err != nil { - writeJSONStatus(w, 400, map[string]string{"error": "invalid consent_challenge"}) + apierr.Write(w, 400, "invalid consent_challenge") return } opts, ok, err := p.WebAuthn.ConsentOptions(r.Context(), cr.Subject) if err != nil { - writeJSONStatus(w, 500, map[string]string{"error": "internal error"}) + apierr.Write(w, 500, "internal error") return } if !ok { - writeJSONStatus(w, 200, map[string]string{"error": "no_passkeys"}) + apierr.Write(w, 200, "no_passkeys") return } writeJSONStatus(w, 200, opts) @@ -294,12 +295,12 @@ func (p ProviderParams) consentVerify(w http.ResponseWriter, r *http.Request) { } _ = json.NewDecoder(r.Body).Decode(&body) if body.ConsentChallenge == "" { - writeJSONStatus(w, 400, map[string]string{"error": "missing consent_challenge"}) + apierr.Write(w, 400, "missing consent_challenge") return } cr, err := p.Admin.GetConsentRequest(r.Context(), body.ConsentChallenge) if err != nil { - writeJSONStatus(w, 400, map[string]string{"error": "invalid consent_challenge"}) + apierr.Write(w, 400, "invalid consent_challenge") return } var assertion struct { @@ -308,7 +309,7 @@ func (p ProviderParams) consentVerify(w http.ResponseWriter, r *http.Request) { _ = json.Unmarshal(body.Credential, &assertion) res := p.WebAuthn.VerifyConsent(r.Context(), body.ChallengeID, assertion.ID, body.Credential, cr.Subject) if res.Error != "" { - writeJSONStatus(w, res.Status, map[string]string{"error": res.Error}) + apierr.Write(w, res.Status, res.Error) return } p.acceptConsentAndRedirect(w, r, body.ConsentChallenge, cr) @@ -320,18 +321,18 @@ func (p ProviderParams) consentApprove(w http.ResponseWriter, r *http.Request) { } _ = json.NewDecoder(r.Body).Decode(&body) if body.ConsentChallenge == "" { - writeJSONStatus(w, 400, map[string]string{"error": "missing consent_challenge"}) + apierr.Write(w, 400, "missing consent_challenge") return } cr, err := p.Admin.GetConsentRequest(r.Context(), body.ConsentChallenge) if err != nil { - writeJSONStatus(w, 400, map[string]string{"error": "invalid consent_challenge"}) + apierr.Write(w, 400, "invalid consent_challenge") return } // The no-passkey approve shortcut is only valid when THIS flow's login was a // fresh passkey ceremony (stamped into the login context). if cr.Context["freshLogin"] != true { - writeJSONStatus(w, 400, map[string]string{"error": "passkey_required"}) + apierr.Write(w, 400, "passkey_required") return } p.acceptConsentAndRedirect(w, r, body.ConsentChallenge, cr) @@ -343,7 +344,7 @@ func (p ProviderParams) acceptConsentAndRedirect(w http.ResponseWriter, r *http. Remember: true, RememberFor: rememberFor, }) if err != nil { - writeJSONStatus(w, 400, map[string]string{"error": "consent_flow_expired"}) + apierr.Write(w, 400, "consent_flow_expired") return } writeJSONStatus(w, 200, map[string]string{"redirectTo": redirect.RedirectTo}) @@ -358,12 +359,12 @@ func (p ProviderParams) handleDCR(w http.ResponseWriter, r *http.Request, patter _ = json.NewDecoder(r.Body).Decode(&body) if len(body.RedirectURIs) == 0 { - writeDCRErr(w, 400, "invalid_redirect_uri", "redirect_uris is required") + apierr.WriteOAuth(w, 400, "invalid_redirect_uri", "redirect_uris is required") return } for _, uri := range body.RedirectURIs { if !matchAny(patterns, uri) { - writeDCRErr(w, 400, "invalid_redirect_uri", "redirect_uri not allowed by policy: "+uri) + apierr.WriteOAuth(w, 400, "invalid_redirect_uri", "redirect_uri not allowed by policy: "+uri) return } } @@ -379,7 +380,7 @@ func (p ProviderParams) handleDCR(w http.ResponseWriter, r *http.Request, patter } } if len(granted) == 0 { - writeDCRErr(w, 400, "invalid_scope", "scope must be a subset of: "+strings.Join(keys(allowed), " ")) + apierr.WriteOAuth(w, 400, "invalid_scope", "scope must be a subset of: "+strings.Join(keys(allowed), " ")) return } @@ -397,7 +398,7 @@ func (p ProviderParams) handleDCR(w http.ResponseWriter, r *http.Request, patter TokenEndpointAuthMethod: "none", }) if err != nil { - writeDCRErr(w, 502, "server_error", "client registration failed") + apierr.WriteOAuth(w, 502, "server_error", "client registration failed") return } redirectURIs := created.RedirectURIs @@ -437,12 +438,10 @@ func keys(m map[string]bool) []string { return out } +// writeJSONStatus renders success payloads; error envelopes go through +// internal/apierr. func writeJSONStatus(w http.ResponseWriter, status int, v any) { w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(status) _ = json.NewEncoder(w).Encode(v) } - -func writeDCRErr(w http.ResponseWriter, status int, code, desc string) { - writeJSONStatus(w, status, map[string]string{"error": code, "error_description": desc}) -} diff --git a/internal/rest/rest.go b/internal/rest/rest.go index a533429..39aada8 100644 --- a/internal/rest/rest.go +++ b/internal/rest/rest.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/auth" ) @@ -54,5 +55,5 @@ func writeJSON(w http.ResponseWriter, status int, v any) { } func writeErr(w http.ResponseWriter, status int, msg string) { - writeJSON(w, status, map[string]string{"error": msg}) + apierr.Write(w, status, msg) } diff --git a/internal/webauthn/routes.go b/internal/webauthn/routes.go index 25e0f1f..bcbf241 100644 --- a/internal/webauthn/routes.go +++ b/internal/webauthn/routes.go @@ -14,6 +14,7 @@ import ( "github.com/go-chi/chi/v5" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/auth" "github.com/rado0x54/shellwatch/internal/store" ) @@ -358,7 +359,7 @@ func writeJSON(w http.ResponseWriter, status int, v any) { } func writeErr(w http.ResponseWriter, status int, msg string) { - writeJSON(w, status, map[string]string{"error": msg}) + apierr.Write(w, status, msg) } var ( diff --git a/internal/webauthn/stepup.go b/internal/webauthn/stepup.go index c7f6348..5292cd4 100644 --- a/internal/webauthn/stepup.go +++ b/internal/webauthn/stepup.go @@ -9,10 +9,10 @@ package webauthn import ( "context" - "encoding/json" "net/http" "time" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/auth" "github.com/rado0x54/shellwatch/internal/clock" "github.com/rado0x54/shellwatch/internal/ephemeral" @@ -129,17 +129,14 @@ func (s *StepUpStore) RequireStepUp(action string) func(http.Handler) http.Handl return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { principal, ok := auth.PrincipalFrom(r.Context()) if !ok { - http.Error(w, `{"error":"unauthenticated"}`, http.StatusUnauthorized) + // Defensive: the bearer gate runs first on every gated route, + // so this branch is unreachable through the real mux. + apierr.Write(w, http.StatusUnauthorized, "unauthenticated") return } reason := s.Consume(r.Header.Get(stepUpHeader), principal.AccountID, action) if reason != ReasonOK { - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(http.StatusUnauthorized) - _ = json.NewEncoder(w).Encode(map[string]string{ - "error": stepUpErrorMessage[reason], - "code": stepUpErrorCode[reason], - }) + apierr.WriteCode(w, http.StatusUnauthorized, stepUpErrorMessage[reason], stepUpErrorCode[reason]) return } next.ServeHTTP(w, r) diff --git a/internal/ws/handler.go b/internal/ws/handler.go index dec71b0..a4a2a0d 100644 --- a/internal/ws/handler.go +++ b/internal/ws/handler.go @@ -6,6 +6,7 @@ import ( "github.com/coder/websocket" + "github.com/rado0x54/shellwatch/internal/apierr" "github.com/rado0x54/shellwatch/internal/auth" ) @@ -17,7 +18,9 @@ func (h *Hub) Handler() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { principal, ok := auth.PrincipalFrom(r.Context()) if !ok { - http.Error(w, "unauthenticated", http.StatusUnauthorized) + // Defensive: the bearer gate 401s unauthenticated /ws upgrades + // before this handler runs. + apierr.Write(w, http.StatusUnauthorized, "unauthenticated") return } c, err := websocket.Accept(w, r, &websocket.AcceptOptions{