fix(passkey): escape mobile WebAuthn HTML templates against reflected XSS

[Antriksh1984]

Summary

getMobileRegistrationHTML/getMobileAuthenticationHTML/getMobileErrorHTML (supabase/functions/passkey/utils/html.ts) substituted every value into the returned HTML page with plain String.replace() and no escaping at all - into both HTML text and into single-quoted JavaScript string literals inside an inline <script> block.

challenge and email are constrained upstream (a base64url challenge matched against a stored row, .email() format validation), but sessionId (both /register/mobile and /auth/mobile) and rpName (/register/mobile) reach routes/mobile.ts as plain, unconstrained z.string() query parameters and are echoed straight through services/mobile.ts into the template with no character restriction anywhere on that path.

Impact: an attacker who knows a victim's email can call the public, unauthenticated challenge-issuing endpoint to obtain a real challenge and credentialId for that account, then send the victim a /auth/mobile (or /register/mobile) link with a crafted sessionId or rpName that breaks out of its JS string literal - closing the quote, running arbitrary script, and closing the surrounding </script> tag outright. The page is served from the real, trusted api.risaboss.com origin and also runs a genuine WebAuthn ceremony for the victim's own passkey, which makes this a convincing injection/phishing vector rather than a theoretical self-XSS.

Fix

• Added escapeHtml (HTML-text context: email, credentialDisplayName, the error message) and escapeJsString (inline <script> string-literal context: challenge, userId, email, sessionId, rpId, rpName, credentialId) in utils/html.ts.
• escapeJsString escapes backslash/quote (so a value can't terminate its own literal early) and also neutralizes </> so a value can't spell out </script> and close the surrounding script element - quote-escaping alone does not stop a <script>-context breakout.
• Template placeholders are renamed with _HTML/_JS suffixes per context, since email in particular is substituted into both an HTML-text spot and a script spot in the same template and needs different escaping at each - a single shared placeholder can't apply two different escaping functions.

Tests

tests/html.test.ts (new): a same-payload regression proving a script-breakout string in sessionId/rpName cannot terminate its JS string literal or the surrounding <script> tag, an HTML-breakout payload in email/credentialDisplayName/the error message is entity-escaped, and benign values still round-trip readably in the final page (no over-escaping).

Validation: deno check across the whole function (all files pass), deno test --allow-all - 153 passed, 0 failed (includes the 7 new tests plus the full existing suite, unaffected).

Test plan

• deno check on every .ts file in supabase/functions/passkey
• deno test --allow-all - 153/153 passing
• Deploy to a preview/staging project and manually confirm /auth/mobile?...&sessionId=x'%3B alert(1)%3B// renders the literal escaped text rather than executing

🤖 Generated with Claude Code

Review follow-up (2026-09-11, maintainer review)

Review found one remaining gap in the escaping mechanism itself: String.prototype.replace applies $-pattern expansion ($$, $&, $ + backtick, $') to string replacement values, and that expansion runs after the escaping. Against the original head this meant:

• a sessionId of $& rendered the raw {{SESSION_ID_JS}} placeholder back into the page (and used it as the ceremony's sessionId, so the flow failed);
• $$ collapsed to $, silently corrupting the value;
• a $ + backtick sessionId spliced the entire pre-match document into the string literal, which kills the inline <script> (page JS SyntaxError, ceremony dead) for that crafted link.

None of these reach arbitrary code execution - the spliced fragments are template text with no attacker-controlled quotes - but the fix's own guarantee ("every value lands escaped, intact") did not hold. The fix: all 18 placeholder substitutions now use function-form replacements (() => escapedValue), which inserts the escaped value verbatim, with a comment pinning the reason. tests/html.test.ts gains a regression test covering every template.

Original contribution: the vulnerability analysis, both escapers, the per-context placeholder split, and the breakout regression tests are @Antriksh1984's. Validation at the new head: deno check clean, deno test --allow-all 154/154, deno lint unchanged from the PR head (the function's 21 pre-existing findings are untouched; the edge-functions workflow does not gate dev-targeted PRs).

Claude review follow-up (2026-09-11, commit c434df6)

Claude Code Review (requested via @claude, run 34584248893) verdict at 5f889c2: "the escaping is correct and complete for the stated threat model. No security defect found at this head." Its findings, applied in c434df6:

• Single-pass render (renderTemplate): a value that literally contains a later placeholder name (e.g. a sessionId of {{ANON_KEY_JS}}) is no longer expanded by a subsequent substitution.
• Pinned the untested backslash rule: a x\'; alert(1); // payload test that fails if the backslash rule is removed or reordered after the quote rule (mutation-verified both ways).
• Pinned the untested CR/LF rule (two-character escapes, not raw line breaks).
• Closed the test asymmetry: $' dollar pattern, exact rpName literal assertion, &-first ordering in escapeHtml, placeholder-in-value regression test.
• Test env-var convention: SUPABASE_ANON_KEY now set per test with try/finally (the routes.test.ts pattern) instead of a leaking module-top-level set.

Not applied: the semicolon point (function declarations take none - deno lint clean) and the optional U+2028/2029 note (legal in JS string literals in the target environments; review marked it non-blocking). Validation at c434df6: deno check clean, deno test --allow-all 157/157, deno lint unchanged (23 pre-existing/convention findings).

[kshivang]

@Antriksh1984 Review follow-up - one gap found and fixed in the escaping mechanism itself.

What was already right (yours, unchanged): the vulnerability analysis, escapeHtml / escapeJsString, the per-context _HTML / _JS placeholder split, and the breakout regression tests. The original head's CI was fully green.

The gap: String.prototype.replace applies $-pattern expansion ($$, $&, $ + backtick, $') to string replacement values, and that expansion runs after the escaping. I reproduced it against your head with deno:

• sessionId=$& rendered the raw {{SESSION_ID_JS}} placeholder back into the page, and the page then used that placeholder text as its ceremony sessionId (the server had stored the raw $& in passkey_challenges.session_id), so the flow failed;
• sessionId=$$ silently collapsed to '$';
• a sessionId of $ + backtick spliced the entire pre-match document into the string literal, which makes the inline <script> a SyntaxError - the page dies and the WebAuthn ceremony never runs, on a link an attacker crafted for the victim.

None of these reach arbitrary code execution (the spliced fragments are template text with no attacker-controlled quotes) - the impact is corrupted values plus a targeted page/ceremony DoS - but the fix's own guarantee that every value lands escaped and intact did not hold.

The fix (commit 5f889c21f): all 18 placeholder substitutions now use function-form replacements (() => escapedValue), which inserts the escaped value verbatim, with a comment pinning the reason so it is not "simplified" back. tests/html.test.ts gains a regression test covering $&, $$ and $ + backtick across the registration, authentication and error templates.

Validation at the new head: deno check clean, deno test --allow-all 154/154, deno lint unchanged from your head (the function's 21 pre-existing findings are untouched; the 2 new ones at your head are the test file's jsr:@std/assert import, identical to every other test file's convention, and the edge-functions workflow does not gate dev-targeted PRs). Hosted CI for the new head is running.

One related hardening I deliberately left out of this PR's scope: the in-page deep link 'boss://passkey/registered?sessionId=' + sessionId concatenates the attacker-influenceable value without encodeURIComponent (the host regex sessionId=([^&]+) truncates at &, so the practical impact is a failed lookup, not injection). That plus the in-page error.message innerHTML sink are covered by the broader passkey-hardening draft #545; keeping this PR focused on the template escaping.

[kshivang]

@claude Please review PR #546 at head 5f889c2 (diff against dev). Focus on the two changed files, supabase/functions/passkey/utils/html.ts and supabase/functions/passkey/tests/html.test.ts: verify the HTML/JS-string escaping is complete for the stated threat model (unconstrained sessionId/rpName query params echoed into the mobile WebAuthn pages), that the function-form String.replace usage is correct, and whether the regression tests actually pin the behaviour. Distinguish confirmed defects from style observations.

[claude]

Claude finished @kshivang's task in 5m 10s —— View job

---

Review: PR #546 at 5f889c2 (vs origin/dev)

• Read utils/html.ts at head and the diff against dev
• Verify escaping completeness per context (HTML text vs JS string literal)
• Verify function-form String.replace usage
• Trace the threat model: sessionId / rpName → routes/mobile.ts → services/mobile.ts → template
• Assess whether tests/html.test.ts pins the behaviour

Verdict: the escaping is correct and complete for the stated threat model. No security defect found at this head. Everything below is one low-severity robustness issue, three test-coverage gaps (one of which guards a security-critical line), and two style points.

I could not execute deno/node in this environment (command approval denied), so this is from code reading; the author's deno check / 154-test run stands unchallenged.

---

What I verified as correct

Context coverage is complete. All 12 distinct placeholders across the three templates have a matching .replace (counts line up: 2 template sites + 2 call sites each, or 1+1 for the single-template ones), so no {{...}} can leak into a served page. Every placeholder sits in either an element text node (EMAIL_HTML, CREDENTIAL_DISPLAY_NAME_HTML, CREDENTIAL_CREATED_AT_HTML, MESSAGE_HTML) or a single-quoted JS string literal inside one inline <script> (the _JS set). There are no attribute, URL, or CSS contexts, which is what makes two escapers sufficient rather than five.

escapeJsString (html.ts:33-41) is complete for its context, and the ordering is load-bearing. \ is escaped before ', which is the only correct order: with the backslash rule removed or moved after the quote rule, sessionId=x\'; alert(1); // escapes to x\\'; alert(1); //, whose first two chars close the literal and hand the rest to the parser - arbitrary execution. The </> → </> step is what actually closes the </script> breakout (quote escaping alone cannot, since script content is raw text), and it also covers the <!-- / nested-<script HTML-parser oddities in the same context.

escapeHtml (html.ts:13-20) escapes & first, so no double-escaping inversion; five-char coverage is more than a text node needs.

The function-form replacement is correct and is the right fix. () => escaped is inserted verbatim - $-pattern expansion applies only to string replacements - so the whole $$ / $& / $` / $' class is gone rather than patched. The comment at html.ts:43-51 pinning the reason is exactly right for a line someone would otherwise "simplify" back.

Threat model traced and confirmed. routes/mobile.ts:31 (sessionId: z.string()), :34 (rpName: z.string().optional()) and :130 are unconstrained; services/mobile.ts:65-67, 153-155 echoes them straight back. rpId is separately allow-listed (isAllowedRpId), and the error template's message is only ever one of six static strings from services/mobile.ts today - escaping it is correct defense-in-depth, not a live fix.

---

1. Sequential replace chain expands placeholders that appear inside values (low, robustness - not exploitable)

utils/html.ts:1150-1158. The eight .replace calls run in sequence, so a value substituted early that literally contains a later placeholder name has that placeholder expanded. escapeJsString and escapeHtml both leave { and } untouched, so:

• ?sessionId=%7B%7BANON_KEY_JS%7D%7D renders const sessionId = '<the anon key>'; (position 5 of 8, ANON_KEY_JS is position 8);
• a value in an HTML slot containing {{SESSION_ID_JS}} puts the JS-escaped sessionId into an HTML text node.

Not a vulnerability, and I want to be precise about why: the anon key is already published in the same page (const anonKey = ...), so there is no disclosure; and the cross-context landing is inert only because escapeJsString also strips </>, so a JS-escaped value is harmless in a text node. That is a real invariant - "each escaper must be safe in the other's context" - and nothing in the file states it or tests it. It would break the day someone drops the </> rule on the grounds that quote-escaping is enough for a JS literal.

A single pass removes the class outright, and is shorter than eight chained calls:

const render = (template: string, values: Record<string, string>) =>
  template.replace(/{{(\w+)}}/g, (match, key) => key in values ? values[key] : match)

This also sidesteps a latent footgun in the current regexes: /{{EMAIL_HTML}}/ only works because a { that cannot start a quantifier is a literal under Annex B web-compat; adding a u flag would make every one of them a SyntaxError.

Fix this →

2. The backslash rule - the security-critical line - is untested (test coverage)

tests/html.test.ts contains no payload with a \ anywhere. Delete .replace(/\\/g, "\\\\") from html.ts:35, or move it below the quote rule, and all 8 tests still pass while ?sessionId=x\'; alert(1); // becomes full script execution on api.risaboss.com. This is the highest-value missing assertion in the file - it guards the one line whose removal reopens the exact vulnerability the PR closes.

const BACKSLASH_PAYLOAD = String.raw`x\'; alert(1); //`
// escaped: backslash doubled first, THEN the quote - so the literal cannot close early
assertStringIncludes(html, String.raw`const sessionId = 'x\\\'; alert(1); //';`)

Fix this →

3. CR/LF escaping is untested (test coverage)

html.ts:39-40 escapes \r/\n and the KDoc calls it out as load-bearing ("a raw newline would otherwise be a syntax error, not just odd formatting") - correctly, since a raw newline in a single-quoted literal kills the inline script and therefore the ceremony. That is the same severity class as the $` splice this second commit exists to fix, and it has no test. sessionId = "a\nb" asserting 'a\\nb' covers it.

4. Smaller asymmetries in the pinning (test coverage, minor)

• $' (post-match splice) is named in both the source comment and the test comment but is not exercised; $&, $$ and $` are.
• The rpName breakout test (:65-80) only asserts "payload not verbatim" + one </script>, where the sessionId test (:58) pins the exact resulting literal. The weaker of the two would pass under escaping that mangles the value instead of escaping it.
• escapeHtml's &-first ordering has nothing pinning it: moving that line last still passes everything (<img> would render as <img> rather than <img>). Cosmetic rather than security, but it is one assertStringIncludes.

5. Env handling differs from the file-local convention (style)

tests/html.test.ts:24-30 + :203-207 sets SUPABASE_ANON_KEY at module top level and restores it in a trailing Deno.test({ name: "cleanup" }). tests/routes.test.ts:259-281 - the only other file that touches this same variable - does set-then-try/finally inside the test. Two consequences of the top-level form, neither load-bearing today (the suite is green): Deno imports all test modules before running any test, so the value leaks into every other file for the whole run and the restore lands mid-run; and under --filter the cleanup test never executes. try/finally is shorter and matches the neighbour.

Also minor: the two new escape functions omit trailing semicolons while the rest of html.ts uses them.

6. U+2028 / U+2029 (optional hardening, very low)

Not escaped. Legal inside string literals since ES2019 (Chrome 66 / Safari 12 / Firefox 62), so no practical impact for a mobile WebAuthn page, and I would not block on it - but the escaper already handles CR/LF for precisely this reason, so it is the same argument one codepoint further. Two more .replace calls, or JSON.stringify(value).slice(1, -1) followed by the '/</> rules.

---

On the two acknowledged out-of-scope items

Agreed with the scoping to #545, and worth stating why they are genuinely not covered by this change rather than merely deferred: the escaping is source-level only, so at runtime the sessionId / rpName variables hold the original raw attacker string (< decodes back to < on parse). So 'boss://passkey/registered?sessionId=' + sessionId (html.ts:419, :857) and status.innerHTML = ... + error.message (html.ts:428, :866) are untouched by this PR by construction, not by accident. Nobody reading "the mobile templates are escaped" should conclude those sinks are closed.

Recommendation: approve on the security question. Finding 2 is the one I would ask for before merge - it is a five-line test guarding the line whose removal reopens arbitrary script execution, and the suite currently cannot tell. Findings 1, 3 and 4 are good follow-ups; 5 and 6 are optional.
• branch fix/passkey-mobile-html-xss

[kshivang]

@Antriksh1984 Claude review disposition (review comment 5632397837, head 5f889c2) - applied in c434df6d4, pushed to fix/passkey-mobile-html-xss.

Applied:

• Finding 1 (single-pass render). renderTemplate() now substitutes every {{PLACEHOLDER}} in one pass over the original template, so a value that literally contains a later placeholder name (e.g. a sessionId of {{ANON_KEY_JS}}) is never expanded. New regression test pins it.
• Finding 2 (backslash rule untested). New test: a sessionId of x\'; alert(1); // must render with the backslash doubled before the quote. Mutation-verified both ways - the test fails if the backslash rule is removed or moved after the quote rule (in either mutation the literal closes early and the rest of the line is handed to the JS parser).
• Finding 3 (CR/LF). New test: "a\nb\r" renders as 'a\nb\r' (two-character escapes), not raw line breaks.
• Finding 4 (test asymmetry). All three: the $' case added to the dollar-pattern test, the rpName breakout test now pins the exact escaped literal (same strength as the sessionId test), and a new test pins &-first ordering in escapeHtml (&<x must render as &<x, not double-escaped).
• Finding 5 (env-var convention). tests/html.test.ts now sets SUPABASE_ANON_KEY per test with try/finally (the routes.test.ts pattern) instead of a module-top-level set; the "cleanup" test is gone. The top-level set leaked its value into every other test file in the same process and could not restore under --filter.

Deliberately not applied:

• Finding 5 (semicolons). escapeHtml/escapeJsString are function declarations - the } of a function declaration is not a statement and takes no semicolon; deno lint is clean on both. No change.
• Finding 6 (U+2028/2029). Agreed it is legal in JS string literals in the target environments (Deno 2.x edge runtime, modern mobile browsers for the WebAuthn page); the review itself marks it non-blocking. Left out to keep the diff scoped; happy to add \u2028/\u2029 escaping if maintainers want the extra belt.

Local at c434df6d4: deno check clean, deno test --allow-all 157/157, deno lint unchanged (23 findings, all pre-existing/convention). Hosted CI for the new head is running.