Skip to content

Commit d3a5f39

Browse files
mastermanas805instanode-sec-auditclaude
authored
sec(handlers,middleware): escape renderAuthError HTML + drop secret-length leak in e2e bypass log (#173)
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 <!DOCTYPE html> 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 <script>, </script>, and <img src=x payloads do not survive into the response body, and that their escaped forms (&lt;script&gt; etc.) do. SEC-API FINDING-26 — e2e bypass mismatch logs expected secret length (P3, CWE-200) middleware.e2eTokenAccepted's mismatch path logged got_len, expected_len, and got_prefix. Two info-disclosure problems: - expected_len leaks the byte-length of E2E_TEST_TOKEN to anyone with log read access (NR Logs / log-aggregation breach). Narrows brute- force search space if the operator picked a short token. - got_prefix echoes the attacker's own guess into long-term log storage, attesting the env var is configured in prod and enabling correlation-grep against future attacker payloads. Drop expected_len and got_prefix. Keep got_len — the attacker already knows the length of their own input, so this leaks nothing new while still letting an SRE distinguish "wrong-content" from "missing/malformed" failure modes. Production LOC delta: 24 lines (well under 50). No behavioural change on the happy path of either function. Existing tests pass; one new test added. Co-authored-by: instanode-sec-audit <security@instanode.dev> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f2fb140 commit d3a5f39

3 files changed

Lines changed: 61 additions & 3 deletions

File tree

‎internal/handlers/auth.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"encoding/json"
88
"errors"
99
"fmt"
10+
"html"
1011
"io"
1112
"log/slog"
1213
"net/http"
@@ -946,6 +947,15 @@ func (h *AuthHandler) consumeOAuthState(ctx context.Context, state string) bool
946947

947948
// renderAuthError sends a 400 with a small HTML page so a browser landing on
948949
// a broken callback URL gets a readable message instead of raw JSON.
950+
//
951+
// SEC-API FINDING-23 (2026-05-29): headline + detail are interpolated into HTML
952+
// via fmt.Sprintf. Every existing caller passes static literals, but the function
953+
// itself is the only HTML emitter in the api and was unsafe-by-default — any
954+
// future caller passing a user-influenced value (OAuth profile name, JWT claim,
955+
// upstream error string) would have introduced reflected XSS on api.instanode.dev
956+
// (cookies for that host stealable). Both arguments are now html.EscapeString'd
957+
// at the sink so the helper is safe for every caller without forcing them to
958+
// remember to escape — defense in depth.
949959
func renderAuthError(c *fiber.Ctx, status int, headline, detail string) error {
950960
c.Set("Content-Type", "text/html; charset=utf-8")
951961
body := fmt.Sprintf(`<!DOCTYPE html>
@@ -956,7 +966,7 @@ func renderAuthError(c *fiber.Ctx, status int, headline, detail string) error {
956966
<p style="color:#444;">%s</p>
957967
<p><a href="https://instanode.dev/login">Try signing in again &rarr;</a></p>
958968
</body>
959-
</html>`, headline, detail)
969+
</html>`, html.EscapeString(headline), html.EscapeString(detail))
960970
return c.Status(status).SendString(body)
961971
}
962972

‎internal/handlers/auth_helpers_coverage_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,46 @@ func TestAuth_RenderAuthError_StatusAndContentType(t *testing.T) {
271271
assert.Contains(t, body, "Detail")
272272
}
273273

274+
// SEC-API FINDING-23 regression: renderAuthError must HTML-escape both
275+
// the headline and detail args so a future caller passing user-influenced
276+
// input (OAuth profile name, JWT email claim, upstream error) cannot
277+
// inject script into the api.instanode.dev origin. Closed-form negative
278+
// — the literal `<script>` and `</script>` payloads MUST NOT appear in
279+
// the response body; their escaped forms MUST appear.
280+
func TestAuth_RenderAuthError_HTMLEscapesPayload(t *testing.T) {
281+
app := fiber.New()
282+
const xssHeadline = `<script>alert("xss-headline")</script>`
283+
const xssDetail = `</p><img src=x onerror="alert('xss-detail')">`
284+
app.Get("/e", func(c *fiber.Ctx) error {
285+
return renderAuthError(c, fiber.StatusBadRequest, xssHeadline, xssDetail)
286+
})
287+
288+
req := httptest.NewRequest(http.MethodGet, "/e", nil)
289+
resp, err := app.Test(req, 5000)
290+
require.NoError(t, err)
291+
defer resp.Body.Close()
292+
293+
buf := make([]byte, 4096)
294+
n, _ := resp.Body.Read(buf)
295+
body := string(buf[:n])
296+
297+
// Negative — raw opening tags must not survive (they would let the
298+
// browser parse the payload as live HTML). With `<` and `>` HTML-escaped,
299+
// the entire payload becomes inert text inside the surrounding <h2>/<p>
300+
// containers — attribute-like sequences (`onerror=...`) inside an inert
301+
// run never run.
302+
assert.NotContains(t, body, "<script>", "raw <script> must be escaped")
303+
assert.NotContains(t, body, "</script>", "raw </script> must be escaped")
304+
assert.NotContains(t, body, "<img src=x", "raw <img must be escaped")
305+
306+
// Positive — escaped forms must be present so the message still
307+
// renders as visible text in the browser.
308+
assert.Contains(t, body, "&lt;script&gt;", "headline must be HTML-escaped")
309+
assert.Contains(t, body, "&lt;/script&gt;", "headline closer must be HTML-escaped")
310+
assert.Contains(t, body, "&lt;/p&gt;", "detail prefix must be HTML-escaped")
311+
assert.Contains(t, body, "&lt;img src=x", "detail <img must be HTML-escaped")
312+
}
313+
274314
// TestSignSessionJWT_RoundTrip mints a JWT via signSessionJWT and
275315
// asserts the resulting token decodes with the expected uid/tid/email.
276316
func TestAuth_SignSessionJWT_RoundTrip(t *testing.T) {

‎internal/middleware/fingerprint.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,17 @@ func e2eTokenAccepted(c *fiber.Ctx) bool {
124124
if subtle.ConstantTimeCompare([]byte(got), []byte(expected)) == 1 {
125125
return true
126126
}
127+
// SEC-API FINDING-26 (2026-05-29): previously logged `expected_len` +
128+
// `got_prefix` on every mismatch. Both are info-disclosure: expected_len
129+
// hands an attacker who can read the api logs the byte-length of
130+
// E2E_TEST_TOKEN (narrows brute-force search), and got_prefix is the
131+
// attacker's own guess being echoed back into long-term log storage
132+
// (correlation-grep risk, plus attests the env var is configured in
133+
// prod). Keep only `got_len` — the attacker already knows the length of
134+
// their own input, so this leaks nothing new while still letting an SRE
135+
// distinguish "malformed/missing" from "wrong-content" failures.
127136
slog.Warn("e2e_bypass.token_mismatch",
128-
"got_len", len(got), "expected_len", len(expected),
129-
"got_prefix", got[:min(8, len(got))])
137+
"got_len", len(got))
130138
return false
131139
}
132140

0 commit comments

Comments
 (0)