fix(passkey): mask email and user id in edge-function logs

[kshivang]

Consolidated dev handoff for #1436

Recreated from Raghav Bansal's two commits on the latest dev, preserving the original commit authorship. The only conflict was in auth.ts; the dev branch's enumeration-safe inert response is retained while the email masking is applied.

Scope: mask passkey edge-function email and user-id values in persistent logs, with auth/mobile regression coverage. Related host-side session/challenge sanitizer work remains in #1440/#567 and is intentionally not duplicated here.

Validation:

• deno test --allow-all tests/auth.test.ts tests/mobile.test.ts - 25 passed
• deno check --config deno.json index.ts - passed
• Original fix(passkey): mask email and user id in edge-function logs #1436 CI - 16 successful checks

Original PR: #1436

[github-actions]

Claude diff review of 168ed35 (no tests executed).

Review: PR #1490 @ 168ed35 → dev

Scope is a log-hygiene change: a new utils/logging.ts with three masking helpers, applied to ~25 console.log call sites, plus three tests. No behavioural code paths change, so regression risk to auth flows is low. My findings are therefore concentrated in the mask helpers themselves and in how strongly the new tests pin the behaviour.

---

Confirmed defects

1. maskEmail passes an unbounded, unsanitised attacker-controlled substring straight into the log line — supabase/functions/passkey/utils/logging.ts:11-16

export function maskEmail(email: string): string {
  ...
  return `${email.slice(0, 1)}***@${email.slice(at + 1)}`
}

The module docstring (logging.ts:3-7) states the exact threat model: "these call sites sit on unauthenticated request paths, so raw emails and user ids - attacker-controlled input - must not reach them." But the domain half is echoed verbatim with no length cap and no CR/LF/control-character stripping. The first call site is services/auth.ts:82, which logs before any lookup or validation of email:

console.log('🔑 Generating authentication challenge for email:', maskEmail(email))

An unauthenticated request with email = "a@x.com\n✅ Authentication successful for user: 1234…" produces a forged log line; a multi-kilobyte domain produces log flooding. Given the function's stated purpose is to neutralise attacker-controlled input on unauthenticated paths, this is a gap in the control itself, not merely a style nit. Suggested fix: cap length and strip [\r\n\u0000-\u001F] (or hash the domain) before interpolation.

Caveat: I cannot verify from the diff whether an upstream handler validates email shape before reaching generateAuthChallenge. If it does, severity drops to low; the helper is still the wrong place to rely on that.

2. Same passthrough, smaller blast radius, in maskUserId / maskSessionId — logging.ts:19-28. Both emit value.slice(0, 4). sessionId is explicitly client-supplied per the comment at services/auth.ts:472-474 ("a client-supplied sessionId can legitimately be reused"), so sessionId = "\n\n\n\nx" yields four raw newlines in the log. Bounded to 4 characters, so this is minor, but it is the same class of issue and the same one-line fix.

3. maskSessionId(undefined) and maskSessionId("abc") are indistinguishable from each other and from a real masked value — logging.ts:25-28, consumed at services/registration.ts:56:

console.log('… for user:', maskUserId(userId), 'sessionId:', maskSessionId(sessionId))

sessionId is optional here (sessionId?: string), and the same-device flow passes nothing. Previously the line read sessionId: undefined, which unambiguously identified the same-device path; it now reads sessionId: ***, which reads as "a session id existed and was masked." That is a real debuggability regression on the exact flow these logs exist to triage. utils/challenge.ts:30-31 got this right (options?.sessionId ? maskSessionId(...) : undefined); registration.ts:56 did not. Recommend making the helper return a distinct sentinel (e.g. 'none') for nullish input, or applying the ternary consistently.

---

Uncertain observations

4. Incomplete coverage of the stated policy. services/management.ts:48 masks the user id but leaves passkeyId raw:

console.log('🗑️ Deleting passkey:', passkeyId, 'for user:', maskUserId(userId))

If passkeyId is a DB row UUID this is probably fine; if it is (or is derived from) a credential id it is a per-user stable identifier and should be masked for consistency. I cannot determine which from the diff. Related: utils/challenge.ts:28 still logs challenge.substring(0, 20) — pre-existing and out of scope, but it means "logs carry no correlatable per-ceremony value" is not actually true.

5. maskEmail retains the full domain. For consumer domains this is fine; for single-tenant/corporate domains plus the first initial it is frequently re-identifying. Whether that clears your bar is a policy call, not a bug.

6. Cosmetic edge cases in maskEmail (logging.ts:13-15): input "@example.com" yields @***@example.com; input with no @ collapses to ***, silently erasing the fact that the input was malformed (which is itself a useful signal on an unauthenticated path).

7. Type-signature asymmetry. maskSessionId accepts string | null | undefined; maskEmail/maskUserId are typed string but carry runtime typeof guards (logging.ts:12, :20). Callers such as services/auth.ts:574 (maskEmail(userResult.user.email)) and utils/database.ts:553 (maskEmail(userData.email)) rely on those guards if the column is nullable. It works, but the types lie; widening the two signatures to match maskSessionId would make the contract honest.

---

Test gaps

8. The two service-level "no leak" tests can pass vacuously. tests/auth.test.ts:323-369 asserts only on captured log content and never asserts the call succeeded:

await generateAuthChallenge(mockClient as unknown as SupabaseClient, 'victim@example.com', 'session-xyz')
...
assertExists(logged.find(line => line.includes('v***@example.com')))
for (const line of logged) { assertEquals(line.includes('user-456'), false, …) }

The v***@example.com line is emitted unconditionally as the first statement of the function (auth.ts:82), so it is satisfied even if the RPC mock does not match and findUserByEmail returns User not found. In that early-return path the user-456 and session-xyz assertions are trivially true because those values were never reached — i.e. the test would still pass if auth.ts:93 (maskUserId(userId)) were reverted to raw. Adding assertEquals(result.success, true) (or asserting the specific masked user-id line is present) would close this. tests/mobile.test.ts:532-582 has the identical structure and the identical weakness, and additionally depends on mockChallenge.user_id === 'user-456', which is not visible in the diff — if it differs, the user-id assertion is inert.

9. No direct unit tests for utils/logging.ts. All three helpers are only exercised transitively. The interesting branches — empty string, missing @, length <= 4, null/undefined, non-string input (logging.ts:12, 20, 26) — are untested, and those are exactly the branches where the defensive guards live. A dozen table-driven assertions against the module directly would be cheaper and stricter than the service-level tests.

10. Assertion is coupled to an incidental prefix. tests/registration.test.ts:93 asserts logs.some(line => line.includes('sess…')), which only holds because the fixture happens to be 'session-secret-123'. Renaming the fixture silently guts the test. Asserting the exact expected masked string is more robust.

11. Console monkey-patching hygiene. All three tests use try/finally to restore, which is correct. Two residual risks: the capture closure calls JSON.stringify(arg) on non-strings (auth.test.ts:341, mobile.test.ts:555), which throws on circular structures — plausible for a Supabase error object — turning a masking regression into an opaque test crash; and global console replacement is not safe under intra-file parallelism should that ever be enabled.

---

Summary

The direction is right and the mechanical application across auth.ts, mobile.ts, registration.ts, management.ts, challenge.ts and database.ts looks consistent. Before merge I would fix (1) — sanitise and length-cap the domain passthrough in maskEmail, since it defeats the control's own stated threat model — and (3), the undefined → *** conflation at registration.ts:56. I would also strengthen the two service tests per (8) so they cannot pass on an early-return path, and add direct unit tests for logging.ts per (9).

Note: this is a diff-only review. I have not run the suite and have not inspected the surrounding modules, the mock Supabase client, or any call sites outside the hunks shown; claims about untouched code are explicitly flagged as uncertain above.

[github-actions]

Claude diff review of 56d05bc (no tests executed).

Review of PR #1490 @ 56d05bc — "mask identifiers in passkey function logs"

Scope of what I can assess: only the diff text. I did not run tests, and I cannot see unchanged lines outside the hunks (this matters for a couple of findings below, which I mark as unverified).

Overall: the change is mechanical and low-risk functionally — every masked value is consumed only by console.*, no control flow, return payloads, or DB predicates change. The concerns are about (a) the strength/semantics of the mask itself, (b) residual leak paths the PR leaves open, and (c) the shape of the new tests.

---

Confirmed defects / design problems (provable from the diff)

1. 4-character prefix masking collides at trivial scale and guts the security-alert logs

utils/logging.ts:25-28,36-40

return userId.length <= 4 ? '***' : `${safeFragment(userId, 4)}…`

For UUIDs that is 4 hex chars = 16 bits ⇒ ~50% chance of a collision once ~300 distinct ids appear in a log window, and near-certain ambiguity at thousands of users. The two call sites most damaged are the anti-cloning alerts:

• services/auth.ts:298 — '❌ Signature counter regression for passkey:', maskPasskeyId(passkey.id)
• services/auth.ts:331 — '❌ Signature counter was claimed concurrently for passkey:', maskPasskeyId(passkey.id)
• utils/database.ts:414 — same for recordPasskeyUse

These are precisely the lines an operator needs to identify and revoke a specific credential. passkey.id and passkey.user_id are internal, server-generated UUIDs — not attacker-controlled, not user-identifying on their own. The PR's stated threat model in the header comment is "raw emails and user ids — attacker-controlled input — must not reach them" (utils/logging.ts:4-6), which does not apply to passkey.id. Suggest either (a) leave internal row UUIDs unmasked, or (b) if a pseudonym is required for compliance, use a truncated keyed HMAC (per-deployment secret) rather than a prefix — that gives you non-reversibility and uniqueness, which a prefix gives you neither of.

2. A prefix mask is partially reversible / low-entropy by construction

utils/logging.ts:11-13,21,27,39
A prefix leaks the highest-order bits of every identifier and is directly checkable against a candidate value (offline confirmation of "is user de30… you?"). For emails the mask retains the entire domain plus first initial (maskEmail returns v***@example.com), which for corporate or self-hosted domains is often re-identifying. This is pseudonymization, not masking; if the driver is a PII/GDPR requirement, reviewers should confirm that is acceptable, because the module comment claims the value is not "stored".

3. The most sensitive value in the same log record is still printed at 20 characters

utils/challenge.ts:27-32

console.log('Storing challenge:', {
  challenge: challenge.substring(0, 20) + '...',
  ...
  sessionId: options?.sessionId ? maskSessionId(options.sessionId) : undefined,

The session id (a client-supplied polling handle) is now cut to 4 chars, while the WebAuthn challenge — the ceremony's actual bearer value, and the lookup key used by .eq('challenge', challenge) in services/mobile.ts:63,152 — is still emitted as a 20-char prefix (~120 bits of a ~43-char base64url string) on the line right above. Pre-existing, but the PR touches this exact object and leaves the inconsistency in place; it undercuts the stated goal.

4. Empty-string session id silently logs undefined instead of a mask

utils/challenge.ts:30 uses a truthiness guard (options?.sessionId ? mask… : undefined) while maskSessionId already handles null/undefined → 'none' and '' → '***' (utils/logging.ts:37-38). The guard is redundant and produces a third, inconsistent rendering. Call maskSessionId(options?.sessionId) unconditionally.

5. Doc/implementation mismatch: masks are not ASCII

utils/logging.ts:10 states "Only bounded, single-line ASCII fragments may survive", but maskUserId/maskSessionId append U+2026 (…) at lines 27 and 39. Harmless in a UTF-8 sink, but the invariant as written is false and downstream log parsers/encoders sometimes aren't UTF-8 clean. Use ... or fix the comment.

6. No case normalization ⇒ same user yields different masks

utils/logging.ts:20-21. Victim@Example.com → V***@Example.com vs v***@example.com. Since the whole point is cross-line correlation, lowercase the domain (and initial) before masking.

---

Uncertain observations (need verification against unchanged code)

7. Raw error objects are still logged next to the masked values — likely residual leak

services/auth.ts:597

console.error('⚠️ Failed to persist session for session id:', maskSessionId(sessionId), tokenStoreError)

PostgREST/postgrest-js errors carry message/details/hint that routinely embed the offending row values (e.g. Key (session_id)=(…) already exists). If any of the masked identifiers can appear in a constraint violation, the raw value is re-emitted on the same line. Same pattern exists in utils/database.ts (console.error('Exception finding user:', error) at the context line after :516). I could not verify what those errors actually contain; worth a sanitizing wrapper around error logging rather than masking only the explicit arguments.

8. Coverage of the codebase is partial and unenforced

The diff masks services/{auth,management,mobile,registration}.ts and utils/{challenge,database}.ts, but does not touch utils/jwt.ts (which receives userResult.user.email at services/auth.ts:575), utils/error-handler.ts (withErrorHandler/withStatusErrorHandler wrap every service and presumably log the thrown error), routes/*, or the function entrypoint. I can't see those files, so I can't claim they leak — but there is no lint rule, no wrapper logger, and no type-level barrier added here, so nothing prevents the next console.log('…', email) from regressing this. A no-restricted-syntax/custom Deno lint rule, or routing all logging through a log() helper that masks by field name, would make this durable.

9. assertExists may not be imported in mobile.test.ts

tests/mobile.test.ts:584,585 use assertExists(...), but the hunk begins at line 7 and the diff never adds to the jsr:@std/assert import (unlike tests/auth.test.ts, whose import of assertExists is visible in its hunk). If line ~5 only imports assertEquals/assertStringIncludes, this file fails to type-check. Please confirm the existing import list.

---

Test gaps

10. The leak detectors can produce false negatives (Deno.inspect defaults)

tests/auth.test.ts:349-351 and tests/mobile.test.ts:558-560:

logged.push(args.map(arg => … : Deno.inspect(arg)).join(' '))

Deno.inspect defaults truncate: nested objects beyond depth render as [Object], long strings are abbreviated, and iterables are capped. A raw email nested inside a logged Supabase row/error (exactly the case in finding #7) would be elided, and the assertEquals(line.includes('victim@example.com'), false) loop would pass while the real console.log had leaked it. Use Deno.inspect(arg, { depth: Infinity, strAbbreviateSize: Infinity, iterableLimit: Infinity }) — or serialize with a full walker.

11. The strongest assertions are only applied to the two least sensitive paths

The all-console capture + raw-leak loop is applied to generateAuthChallenge and generateMobileRegistrationPage only. There is no test for:

• checkAuthStatus (services/auth.ts:471,512,519,574,597) — the session-replay path, and the one that logs completedAuth.user_id and the persistence error.
• completeAuthentication (services/auth.ts:298,331,338,374-380) — the cloned-authenticator alerts and the '🔍 Challenge data:' object.
• services/mobile.ts:92,121,152 (authentication page).
• services/management.ts:12 (listUserPasskeys) and :99 (updatePasskeyDisplayName).
• utils/database.ts:258,414,422,448,470,494,509,513,536,553 directly.

12. Asymmetric console stubbing in the two smaller tests

tests/management.test.ts:135 and tests/registration.test.ts:89 stub only console.log; anything written to console.error/warn in those paths (e.g. deleteUserPasskey's fetch-error branch) escapes the assertion. Use the same all-channel capture helper as auth.test.ts — and factor that helper into tests/helpers/ rather than duplicating the 15-line stub/restore block four times.

13. Missing unit cases in tests/logging.test.ts

• maskEmail('@') (at===0 and at===len-1 simultaneously).
• maskEmail('Victim@Example.com') — would pin down finding security: stop DigiCert signing secrets leaking into public CI logs #6 either way.
• maskSessionId('') → '***', and the challenge.ts call-site behavior (undefined) from finding security: gate release workflows behind owner-approved environment #4.
• maskEmail with an IDN/non-ASCII domain (a@exämple.com → ex_mple.com) — currently unspecified behavior.
• No test that the mask output is injection-safe for the local part beyond the first character (it is, by construction, but there's no guard against someone later widening the local-part disclosure).

14. No regression guard against new unmasked call sites

Covered in #8; worth stating as a test gap too — the suite asserts today's call sites are masked but nothing fails when a new one is added.

---

Nits

• maskPasskeyId (utils/logging.ts:31-33) is applied both to passkey row ids (services/auth.ts:298) and to credential ids (utils/database.ts:258,448). Two different namespaces rendered identically means pass… in one log line and pass… in another are not comparable — consider distinct prefixes (pk:/cred:) if you keep prefix masking.
• maskEmail retains up to 100 attacker-controlled characters per line (utils/logging.ts:20) on unauthenticated endpoints; combined with findUserByEmail logging twice per miss (utils/database.ts:494,509), that's a small log-volume amplification vector. Probably acceptable; flagging for awareness.
• Test assertions like logs.some(line => line.includes(maskPasskeyId('passkey-123'))) (tests/management.test.ts:147) compare the implementation against itself; they're only meaningful because the paired raw-value assertion exists. Prefer the literal 'pass…' as logging.test.ts correctly does.

---

Verdict

No functional regression found in the production code paths — masking is confined to log arguments. The blocking-ish items in my view are #1 (masking internal passkey/user UUIDs in the cloned-authenticator alerts makes those alerts unactionable, and the 4-char prefix collides at small scale) and #10 (the leak-detection tests can silently pass). #3, #7, and #8 determine whether this PR actually achieves its stated goal or just moves the leak one argument to the right.