docs(auth): passkey (WebAuthn) login spec (HT-75) - #88
Conversation
📝 WalkthroughWalkthroughAdds a draft WebAuthn passkey specification covering storage, ceremonies, step-up verification, provider wiring, APIs, credential integrity, rollout constraints, and security policy. Corrects authentication documentation to state that passkeys use a separate credential table. ChangesPasskey authentication design
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant PasskeyUI
participant EngineAPI
participant Database
Agent->>PasskeyUI: Start passkey ceremony
PasskeyUI->>EngineAPI: Request options or submit assertion
EngineAPI->>Database: Consume challenge and validate credential
EngineAPI-->>PasskeyUI: Return verification result and session
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/auth/agents-and-auth.md`:
- Around line 116-121: Update the amended rationale near the
`agent_auth_identities` and `webauthn_credentials` references to remove the
claim that multiple credentials per Agent justify a separate table. Retain only
the valid reasons: per-credential mutable state and the incompatible
one-row/optional-secret-hash column shape, consistent with
`specs/auth/passkeys.md` §2.1.
In `@specs/auth/passkeys.md`:
- Around line 612-620: Update verifyAuthenticationResponse so counter validation
and persistence are atomic per credential: after cryptographic verification,
re-read the credential row using FOR UPDATE, compare the assertion counter with
the locked stored maximum according to §8, reject regressions, and update
sign_count to the maximum atomically before minting the session. Preserve the
existing backup_state and last_used_at updates and active-agent recheck.
- Around line 106-110: The passkey name requirement is inconsistent between the
database schema and API definition. Update the registration API field
declaration near the passkey request schema to make name required, or add a
validated server-side default such as “Passkey” before insertion, ensuring
omitted names cannot violate the NOT NULL database constraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d3ba3d7-5131-431e-8149-7b1929fbbaab
📒 Files selected for processing (2)
specs/auth/agents-and-auth.mdspecs/auth/passkeys.md
Three fixes from CodeRabbit's first pass on the HT-75 passkey spec:
- agents-and-auth.md §3.2's HT-75 amendment cited "an Agent holding many
credentials" as a reason passkeys need their own table — that's the
cardinality argument passkeys.md §2.1 explicitly rejects (nothing in
agent_auth_identities restricts provider='passkey' to one row). Fixed
to cite only the two reasons that hold: mutable per-use state and the
incompatible column shape.
- passkeys.md's registration/verify endpoint declared name? (optional)
while webauthn_credentials.name is NOT NULL, with nothing stating what
happens on omission. Reconciled: an omitted/blank name gets a
server-computed default ("Passkey — {date}") before insertion, stated
in both the endpoint table and §6.1's prose.
- §6.2's counter check and its persistence were described as sequential
steps against the same row read used for cryptographic verification —
a real race: two concurrent valid authentications could both pass
against a stale counter, and the later write (even with the lower
counter) could silently overwrite a higher one already stored,
undermining §8's Tier 2 policy. Fixed: the counter compare-and-set now
happens under SELECT ... FOR UPDATE inside one transaction, re-reading
the row fresh after cryptographic verification rather than reusing the
earlier read.
Still spec only — no code, no migrations.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extends the HT-54 auth-provider seam with a second provider (webauthn), the first to actually exercise the seam's marketplace-boundary claim. Pins the data model (a dedicated webauthn_credentials table, not rows in agent_auth_identities), RP ID/origin policy sourced from HELPTHREAD_UI_BASE_URL, registration/authentication ceremonies (UV required, attestation none, resident keys for conditional-UI login), a signed-token + DB-nonce challenge lifecycle, a log-only clone-detection policy tuned to real synced-passkey behavior, the endpoint surface, and the @simplewebauthn/server license verification. Spec only — no code, no migrations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes all findings from the first review round (5 major/6 minor lead-tier, 1 high/1 medium Codex): - Enrollment hardening: passkey registration (options + verify) now requires fresh step-up re-authentication (existing password or an existing passkey, bound to the session, 5-minute TTL) plus a best-effort "new passkey added" notification email — and §10 states plainly that sessions are stateless HMAC cookies with no revocation, which is why both mitigations exist. - Conditional-mediation staleness: proactive challenge re-mint below the TTL, plus a reactive challenge_expired retry-once fallback for backgrounded-tab timer throttling. - The challenge token's ceremony discriminator is now enforced at both the application and database layers, not just recorded. - Counter/clone-detection policy reversed from log-only to a two-tier exempt/reject-and-alert policy, routed to the existing HT-44 /internal/health alertable surface via a new sign_count_regression_at column — the zero-counter exemption already filters out the synced-passkey population the old justification worried about, so a regression on the remaining population is a high-quality signal. - The challenge-row volume claim was false (rows mint on every login-page mount, not just login attempts) — corrected, with an opportunistic purge-on-mint fix, no cron. - RP ID dev carve-out narrowed to localhost only, not loopback IP literals (WebAuthn requires a domain-form hostname). - Worked example fixed (inbox.resonantiq.app, the UI's own host, not desk.resonantiq.app, the engine's). - agents-and-auth.md §3.2 amended: it previously anticipated passkey identities living in agent_auth_identities; corrected to point at this spec's dedicated webauthn_credentials table instead, with the cardinality argument for that choice dropped (it was never valid) and the two arguments that actually hold kept. - Added: userHandle-vs-agent_id consistency check at authentication verify, a generic 409 for a credential_id already claimed by a different Agent, api-types.ts's descriptor-kind widening, and a rollout warning that changing HELPTHREAD_UI_BASE_URL's host silently invalidates every existing passkey. Still spec only — no code, no migrations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three fixes from CodeRabbit's first pass on the HT-75 passkey spec:
- agents-and-auth.md §3.2's HT-75 amendment cited "an Agent holding many
credentials" as a reason passkeys need their own table — that's the
cardinality argument passkeys.md §2.1 explicitly rejects (nothing in
agent_auth_identities restricts provider='passkey' to one row). Fixed
to cite only the two reasons that hold: mutable per-use state and the
incompatible column shape.
- passkeys.md's registration/verify endpoint declared name? (optional)
while webauthn_credentials.name is NOT NULL, with nothing stating what
happens on omission. Reconciled: an omitted/blank name gets a
server-computed default ("Passkey — {date}") before insertion, stated
in both the endpoint table and §6.1's prose.
- §6.2's counter check and its persistence were described as sequential
steps against the same row read used for cryptographic verification —
a real race: two concurrent valid authentications could both pass
against a stale counter, and the later write (even with the lower
counter) could silently overwrite a higher one already stored,
undermining §8's Tier 2 policy. Fixed: the counter compare-and-set now
happens under SELECT ... FOR UPDATE inside one transaction, re-reading
the row fresh after cryptographic verification rather than reusing the
earlier read.
Still spec only — no code, no migrations.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f620445 to
b76ef9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/auth/passkeys.md`:
- Around line 625-630: Update the successful authentication credential mutation
described in the counter-check transaction to set updated_at = now() alongside
sign_count, backup_state, and last_used_at. Ensure every credential mutation
path updates updated_at, or explicitly document the guaranteed external trigger
if one is relied upon.
- Around line 625-630: Define the Tier 2 counter-regression rejection path in
the passkey verification flow: ensure nonce consumption via consumed_at and the
sign_count_regression_at alert marker are committed or persisted separately even
when the counter update is rejected. Do not advance sign_count, backup_state, or
last_used_at on rejection, and apply the same behavior to the corresponding
later section.
- Line 698: Update the fenced token-format example in the passkeys documentation
to specify the text language tag, changing the untyped fence to a text fence
while preserving the example content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af0da10a-653c-409d-a45b-721180ac5b74
📒 Files selected for processing (2)
specs/auth/agents-and-auth.mdspecs/auth/passkeys.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/auth/agents-and-auth.md
| **The counter check and its persistence are one atomic unit, not two | ||
| sequential steps.** On a successful signature verification, the handler | ||
| re-reads the *same* row with `SELECT ... FOR UPDATE` inside a transaction, | ||
| applies §8's Tier 1/Tier 2 comparison against **that freshly locked read** | ||
| (not the earlier, pre-verification one), and — only if it passes — updates | ||
| `sign_count` to the new value, plus `backup_state` and `last_used_at`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep updated_at current on credential mutations.
The schema defines updated_at, but the authentication update list omits it. Unless an external trigger is guaranteed, it will remain at the creation timestamp. Add updated_at = now() to every mutation path or document the trigger explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/auth/passkeys.md` around lines 625 - 630, Update the successful
authentication credential mutation described in the counter-check transaction to
set updated_at = now() alongside sign_count, backup_state, and last_used_at.
Ensure every credential mutation path updates updated_at, or explicitly document
the guaranteed external trigger if one is relied upon.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the rejection transaction boundary.
If a Tier 2 counter regression rolls back the transaction, both consumed_at and sign_count_regression_at can be lost: the assertion remains replayable and HT-44 never receives the alert signal. Specify a rejection path that commits nonce consumption and the regression marker without advancing the counter or last-used fields, or persists them in a separate transaction.
Also applies to: 738-745
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/auth/passkeys.md` around lines 625 - 630, Define the Tier 2
counter-regression rejection path in the passkey verification flow: ensure nonce
consumption via consumed_at and the sign_count_regression_at alert marker are
committed or persisted separately even when the counter update is rejected. Do
not advance sign_count, backup_state, or last_used_at on rejection, and apply
the same behavior to the corresponding later section.
| key rotation, constant-time verification, a domain-separator prefix so a | ||
| signature minted for one purpose can never verify as another). | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced token example.
Use a text fence for the token-format example to resolve the MD040 warning.
Proposed fix
-```
+```text
htw.{keyId}.{payload-b64url}.{sig-b64url}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 698-698: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/auth/passkeys.md` at line 698, Update the fenced token-format example
in the passkeys documentation to specify the text language tag, changing the
untyped fence to a text fence while preserving the example content.
Source: Linters/SAST tools
Restores PR references the changelog consolidation dropped. The previous commit message claimed "ticket and PR references are kept as navigable provenance" while the diff deleted PR #82, #88, #69 and #85 from the consolidated entries. Claim and diff now agree. Rewrites 28 further sites the earlier passes missed. They used phrasings the patterns did not cover -- "an earlier revision", "the first draft wrongly hung", "retracted here rather than quietly patched", "corrected there in this same review round", "review round 2". Each is restated forward and keeps its technical lesson: store/mailboxes.ts "Why not tell the operator to disconnect and retry? Because that instruction would be false: ..." providers/inbound-email.ts "Why not return a NormalizedInboundEmail? That puts the parse inside the provider ..." gmail-reconcile.ts "Why not just ack? The tempting reasoning is ..." Also adds a supersession note to decision point 4 in agents-and-auth.md, which still recorded "no scoping behavior or UI" while section 3.4 manages real grants. The recorded decision is left intact; the note points at what superseded it. Two artifacts from the previous pass fixed: an orphaned closing paren in substrate-v1.md, and a substitution that made a date the grammatical subject in migrate.ts. Verified: typecheck, web typecheck, lint clean. Every edit is inside a comment or markdown prose; no source line outside a comment changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
specs/auth/passkeys.md(HT-75) — passkey login as a new provider on the HT-54 auth seam. Core-free per the catalog (security hygiene is never paid — the deliberate divergence from the reference ecosystem's paid-2FA posture).webauthn_credentialsstorage (COSE key, counter, transports, backup flags, per-credential naming) + stateless-plus-nonce challenge lifecycle (htw.tokens in the house signed-state tradition; DB nonce where replay mints sessions; opportunistic expiry purge, no cron).htsu.token/table), authentication incl. conditional-UI autofill with proactive challenge re-mint (conditional mediation ignores client timeouts — the headline flow survives a tab left open), UV required, attestation none, resident keys required.inbox.resonantiq.app— the UI origin where ceremonies actually run, never the engine host; never request-derived), ceremony discriminator enforced at verify AND in the consume SQL, "new passkey added" notification email on every enrollment, counter-regression policy two-tier (zero-history exempt; demonstrated-monotonic credentials reject-and-alert via the HT-44 health surface), no-enumeration posture stronger than the password path's, honest statements that stateless sessions survive password rotation (which is why step-up + notification exist) and that an rpId host change invalidates all passkeys.@simplewebauthn/server(MIT verified against the registry, transitive deps inventoried). Amendsagents-and-auth.md§3.2 in the same PR. UI surfaces are design-project-first (blocked on design; engine implementable now).Review trail — the full auth-critical gauntlet
Sonnet-authored → lead-tier (Fable) adversarial review: FIX-FIRST, 5 MAJOR / 6 MINOR → Codex independent pass: FIX-FIRST, 1 High (enrollment step-up — the planted-passkey ATO path) / 1 Medium (IP-loopback rpId) → all 13 combined findings applied (two were author-acknowledged reasoning errors) → targeted Codex re-check of the net-new step-up surface: SHIP, zero findings.
Decision points for the maintainer (spec §13)
Counter reject-on-regression as recommended default · step-up ceremony shape · revoke-last-credential policy · the §13 list in full.
Docs-only. Engine implementation is the follow-on ticket once this merges.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation