From ff3af97f979f33ee7e7b2a5bda70fcde5e16dead Mon Sep 17 00:00:00 2001 From: instanode-sec-audit Date: Fri, 29 May 2026 15:53:55 +0530 Subject: [PATCH] sec(handlers,middleware): escape renderAuthError HTML + drop secret-length leak in e2e bypass log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defense-in-depth security fixes uncovered during the 2026-05-29 api + common security audit (see /tmp/qa-session/shared/SEC-INBOX.md). SEC-API FINDING-23 — renderAuthError HTML interpolation (P2 hardening, CWE-79) renderAuthError is the only HTML-emitting handler in api. It took (headline, detail string) and fmt.Sprintf'd both into a template without escaping. Every existing caller (24 sites across auth.go + magic_link.go) passes static literals so there is no live exploit today, but the function is unsafe-by-default — any future caller that passes a user-influenced value (OAuth profile name, JWT email claim, raw upstream error message) silently introduces reflected XSS on the api.instanode.dev origin. Cookies for that host (oauth_state, future session cookies, etc.) would be stealable. Apply html.EscapeString to both args at the sink. The function is now safe for every caller regardless of input provenance — they don't have to remember to escape. Adds one new regression test TestAuth_RenderAuthError_HTMLEscapesPayload that asserts the literal , and --- internal/handlers/auth.go | 12 +++++- .../handlers/auth_helpers_coverage_test.go | 40 +++++++++++++++++++ internal/middleware/fingerprint.go | 12 +++++- 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 01513052..a22675cd 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "html" "io" "log/slog" "net/http" @@ -946,6 +947,15 @@ func (h *AuthHandler) consumeOAuthState(ctx context.Context, state string) bool // renderAuthError sends a 400 with a small HTML page so a browser landing on // a broken callback URL gets a readable message instead of raw JSON. +// +// SEC-API FINDING-23 (2026-05-29): headline + detail are interpolated into HTML +// via fmt.Sprintf. Every existing caller passes static literals, but the function +// itself is the only HTML emitter in the api and was unsafe-by-default — any +// future caller passing a user-influenced value (OAuth profile name, JWT claim, +// upstream error string) would have introduced reflected XSS on api.instanode.dev +// (cookies for that host stealable). Both arguments are now html.EscapeString'd +// at the sink so the helper is safe for every caller without forcing them to +// remember to escape — defense in depth. func renderAuthError(c *fiber.Ctx, status int, headline, detail string) error { c.Set("Content-Type", "text/html; charset=utf-8") body := fmt.Sprintf(` @@ -956,7 +966,7 @@ func renderAuthError(c *fiber.Ctx, status int, headline, detail string) error {

%s

Try signing in again →

-`, headline, detail) +`, html.EscapeString(headline), html.EscapeString(detail)) return c.Status(status).SendString(body) } diff --git a/internal/handlers/auth_helpers_coverage_test.go b/internal/handlers/auth_helpers_coverage_test.go index e9eb66d2..b901495a 100644 --- a/internal/handlers/auth_helpers_coverage_test.go +++ b/internal/handlers/auth_helpers_coverage_test.go @@ -271,6 +271,46 @@ func TestAuth_RenderAuthError_StatusAndContentType(t *testing.T) { assert.Contains(t, body, "Detail") } +// SEC-API FINDING-23 regression: renderAuthError must HTML-escape both +// the headline and detail args so a future caller passing user-influenced +// input (OAuth profile name, JWT email claim, upstream error) cannot +// inject script into the api.instanode.dev origin. Closed-form negative +// — the literal `` payloads MUST NOT appear in +// the response body; their escaped forms MUST appear. +func TestAuth_RenderAuthError_HTMLEscapesPayload(t *testing.T) { + app := fiber.New() + const xssHeadline = `` + const xssDetail = `

` + app.Get("/e", func(c *fiber.Ctx) error { + return renderAuthError(c, fiber.StatusBadRequest, xssHeadline, xssDetail) + }) + + req := httptest.NewRequest(http.MethodGet, "/e", nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + buf := make([]byte, 4096) + n, _ := resp.Body.Read(buf) + body := string(buf[:n]) + + // Negative — raw opening tags must not survive (they would let the + // browser parse the payload as live HTML). With `<` and `>` HTML-escaped, + // the entire payload becomes inert text inside the surrounding

/

+ // containers — attribute-like sequences (`onerror=...`) inside an inert + // run never run. + assert.NotContains(t, body, "", "raw must be escaped") + assert.NotContains(t, body, "