sec(handlers,middleware): escape renderAuthError HTML + drop secret-length leak in e2e bypass log#173
Merged
mastermanas805 merged 1 commit intoMay 29, 2026
Conversation
…ength leak in e2e bypass log
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 (<script> 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: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defense-in-depth security fixes uncovered during the 2026-05-29 api + common security audit. Full findings catalogue in
/tmp/qa-session/shared/SEC-INBOX.md(SEC-API FINDING-23..28).SEC-API FINDING-23 —
renderAuthErrorHTML interpolation (P2 hardening, CWE-79)renderAuthErroris the only HTML-emitting handler in api. It took(headline, detail string)andfmt.Sprintf'd both into a<!DOCTYPE html>template without escaping. Every existing caller (24 sites acrossauth.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 theapi.instanode.devorigin. Cookies for that host (oauth_state, future session cookies) would be stealable.Apply
html.EscapeStringto 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_HTMLEscapesPayloadthat asserts the literal<script>,</script>, and<img src=xpayloads do not survive into the response body, and that their escaped forms (<script>etc.) do.SEC-API FINDING-26 — e2e bypass mismatch logs expected secret length (P3, CWE-200)
middleware.e2eTokenAccepted's mismatch path loggedgot_len,expected_len, andgot_prefix. Two info-disclosure problems:expected_lenleaks the byte-length ofE2E_TEST_TOKENto anyone with log read access (NR Logs / log-aggregation breach). Narrows brute-force search space if the operator picked a short token.got_prefixechoes 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_lenandgot_prefix. Keepgot_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.Stats
TestAuth_RenderAuthError_HTMLEscapesPayload)go vet ./...cleanCoverage block (CLAUDE rule 17)
Out of scope (filed as findings, deferred):
🤖 Generated with Claude Code