feat(engine): passkey (WebAuthn) login — core auth provider (HT-75) - #94
Merged
Conversation
Implements specs/auth/passkeys.md end to end: migration 026 (webauthn_credentials, webauthn_challenges, webauthn_stepup_tokens), the WebAuthnStore, the htw./htsu. signed-token pair, the shared authentication-ceremony verifier (TOCTOU-safe FOR UPDATE counter policy, two-tier clone/regression detection routed to the HT-44 health check, userHandle cross-check), the WebAuthnAuthProvider (kind: 'webauthn' on the provider seam), and the full step-up + registration + credential-management API surface. root.ts wires the provider only when config.uiBaseUrl resolves to a domain-form hostname; an IP-literal or unset uiBaseUrl degrades to webauthn-absent rather than failing the whole engine boot. Engine + API only — no web/UI (HT-75 is design-blocked; a separate ticket). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fire (HT-75) Review findings (Codex SHIP, Fable lead-tier FIX-FIRST): MAJOR — the spec §8 "reject AND alert" signal was non-functional: - health.ts's runHealthCheck had no webauthn check at all; add webauthn-counter-regression, same 24h-growth idiom as forged-token-burst, trips the existing 200->503 pivot. - The real @simplewebauthn verifyAuthenticationResponse throws its own counter-regression error using the unlocked pre-read counter, before our locked Tier-1/Tier-2 logic ever runs — markCounterRegression was reachable only in a narrow concurrent-race window, never the common sequential-replay case. Fixed by always passing credential.counter: 0 to the library (structurally disables its internal throw) and relying entirely on our own FOR UPDATE-locked comparison, matching what spec §6.2 already describes as the intended split (signature verification vs. counter policy as two separate steps). - The regression test mocked a resolved regressed counter, which the real library never produces (it throws) — false-green over a dead path. Replaced with a mock that faithfully reproduces the library's own throw-on-regression guard against whatever counter we actually pass, plus an explicit assertion that we pass counter: 0. - Added the missing structured console.warn at the regression-detection point (spec §8: "the log line is what makes it investigable"). MINOR — src/store/webauthn.ts's module doc named a nonexistent src/auth/webauthn-service.ts; corrected to webauthn-ceremony.ts. Also corrects specs/auth/passkeys.md §1/§12: passkeys were already core (specs/modules/catalog.md §1/§2.2, accepted 2026-07-18; reconciled into agents-and-auth.md via HT-76/PR #85, merged same day as this spec's draft.1-3). This spec inherited the pre-reconciliation "licensed marketplace module, waiting on HT-5" framing rather than the fix — corrected to draft.4, documentation only, no design change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces core WebAuthn/passkey authentication, registration, step-up verification, credential management, persistence, routing, conditional application wiring, notification email construction, and counter-regression health monitoring. ChangesPasskey foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/db/migrate.ts (1)
1432-1446: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider indexing
agent_idonwebauthn_challenges/webauthn_stepup_tokens.Both tables have an
agent_idFK withON DELETE CASCADEbut no index on that column, unlikewebauthn_credentials(which getswebauthn_credentials_agent). Deleting an Agent will force a sequential scan of these tables to cascade. Impact is likely small since both tables self-purge expired rows opportunistically, but adding the indexes costs little and removes the asymmetry.♻️ Suggested addition
CREATE INDEX webauthn_challenges_expires ON webauthn_challenges (expires_at); +CREATE INDEX webauthn_challenges_agent ON webauthn_challenges (agent_id); CREATE TABLE webauthn_stepup_tokens ( ... CREATE INDEX webauthn_stepup_tokens_expires ON webauthn_stepup_tokens (expires_at); +CREATE INDEX webauthn_stepup_tokens_agent ON webauthn_stepup_tokens (agent_id);🤖 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 `@src/db/migrate.ts` around lines 1432 - 1446, Add indexes on the agent_id columns of webauthn_challenges and webauthn_stepup_tokens, matching the existing webauthn_credentials_agent indexing pattern while preserving the current table definitions and expiry indexes.src/composition/health.test.ts (1)
352-368: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winParameterize the interval instead of string-splicing it into the SQL.
Static analysis (OpenGrep, ast-grep) flags the template-literal-built
INSERTas a SQL-injection pattern.regressedAgoHoursis currently a typed, test-onlynumber | null, so there's no live exploit path, but building the query this way is still worth avoiding — a bound parameter closes the gap outright and keeps this fixture from becoming a copy-paste template for something less contained.🛡️ Suggested fix
- await database.query( - `INSERT INTO webauthn_credentials - (agent_id, credential_id, public_key, sign_count, backup_eligible, backup_state, name, sign_count_regression_at) - VALUES ($1, $2, $3, 10, false, false, 'Key', ${ - regressedAgoHours === null ? 'NULL' : `now() - interval '${regressedAgoHours} hours'` -})`, - [agent.id, `cred-${Math.random()}`, new Uint8Array([1])], - ) + await database.query( + `INSERT INTO webauthn_credentials + (agent_id, credential_id, public_key, sign_count, backup_eligible, backup_state, name, sign_count_regression_at) + VALUES ($1, $2, $3, 10, false, false, 'Key', + CASE WHEN $4::double precision IS NULL THEN NULL + ELSE now() - ($4::double precision * interval '1 hour') END)`, + [agent.id, `cred-${Math.random()}`, new Uint8Array([1]), regressedAgoHours], + )🤖 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 `@src/composition/health.test.ts` around lines 352 - 368, Update seedRegressedCredential to remove regressedAgoHours from the interpolated INSERT SQL and bind it as a query parameter, preserving NULL behavior when the value is null and calculating the requested interval through parameterized SQL for non-null values.Source: Linters/SAST tools
specs/auth/passkeys.md (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse "plugin exception," not "module exception," for the HT-5/AGPL §7 clause.
This is original (non-quoted) prose introduced in draft.4, so the repo's naming convention applies here: the specific legal clause should be referred to as the "plugin exception," not renamed to "module exception."
✏️ Suggested fix
-anywhere), so this is a documentation-only correction. HT-5 (the AGPL §7 -module exception) gates **in-process third-party modules and external +anywhere), so this is a documentation-only correction. HT-5 (the AGPL §7 +plugin exception) gates **in-process third-party modules and external contributions** — Google SSO, magic-link, and SAML/enterprise SSO remainAs per coding guidelines, "Call extension artifacts
Modules, neverplugins, except within the legal phraseplugin exceptionand quoted charter 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` around lines 45 - 46, Update the original prose in the HT-5/AGPL §7 passage to call the legal clause the “plugin exception” instead of the “module exception”; preserve “Modules” for extension artifacts elsewhere, except in this legal phrase and quoted charter language.Source: Coding guidelines
🤖 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 `@src/store/webauthn.ts`:
- Around line 325-353: Update deleteCredential to lock all webauthn_credentials
rows for the specified agentId before evaluating the last-credential guard,
rather than locking only the target row. Preserve the existing not_found
behavior and ensure the subsequent other_count check runs after the agent-wide
credential lock so concurrent deletes serialize.
---
Nitpick comments:
In `@specs/auth/passkeys.md`:
- Around line 45-46: Update the original prose in the HT-5/AGPL §7 passage to
call the legal clause the “plugin exception” instead of the “module exception”;
preserve “Modules” for extension artifacts elsewhere, except in this legal
phrase and quoted charter language.
In `@src/composition/health.test.ts`:
- Around line 352-368: Update seedRegressedCredential to remove
regressedAgoHours from the interpolated INSERT SQL and bind it as a query
parameter, preserving NULL behavior when the value is null and calculating the
requested interval through parameterized SQL for non-null values.
In `@src/db/migrate.ts`:
- Around line 1432-1446: Add indexes on the agent_id columns of
webauthn_challenges and webauthn_stepup_tokens, matching the existing
webauthn_credentials_agent indexing pattern while preserving the current table
definitions and expiry indexes.
🪄 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: 461e5b3b-6719-485a-a28e-d605cfca010b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
package.jsonspecs/auth/passkeys.mdsrc/api/agents.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/webauthn.test.tssrc/api/webauthn.tssrc/auth/provider.tssrc/auth/webauthn-ceremony.test.tssrc/auth/webauthn-ceremony.tssrc/auth/webauthn-notify-email.tssrc/auth/webauthn-provider.test.tssrc/auth/webauthn-provider.tssrc/auth/webauthn-rp.test.tssrc/auth/webauthn-rp.tssrc/auth/webauthn-token.test.tssrc/auth/webauthn-token.tssrc/composition/app.test.tssrc/composition/health.test.tssrc/composition/health.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/store/webauthn.test.tssrc/store/webauthn.ts
…(HT-75) CodeRabbit finding on PR #94: WebAuthnStore.deleteCredential took FOR UPDATE on the target credential row only; the "does this Agent have another credential" count was a separate, unlocked read. Two concurrent deletes of DIFFERENT credentials for the same passwordless Agent could each see the other's row as still present, both pass the guard, and both commit -- leaving the Agent with zero credentials and no password, locked out. Fix: SELECT ... WHERE agent_id = $1 FOR UPDATE now locks every credential row belonging to the Agent (not just the target) before the guard runs, so a real concurrent Postgres session deleting a different credential for the same Agent blocks on this same lock and re-reads current state after the first commits. Test note: a literal concurrent-call test does not reproduce this race against PGlite -- verified empirically that PGlite serializes whole transactions on its single connection (a second db.transaction() call does not even begin until the first fully commits), so such a test would pass identically against the old, buggy code and prove nothing. Matches the identical, already-documented limitation in src/store/agents.test.ts for the createFirstAdmin advisory-lock guard. Added instead: an instrumented-Db test proving the lock SQL targets the Agent's whole credential set (the structural fact real concurrent Postgres sessions serialize on), and a test proving the guard's arithmetic is correct against that locked set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zaridan
added a commit
that referenced
this pull request
Jul 19, 2026
…-75) (#96) * fix(auth): bind step-up challenge consumption to the acting Agent (HT-75) Found by an independent Codex review of PR #94, after that PR merged. `verifyAuthenticationCeremony` consumed the `webauthn_challenges` row immediately after the token's signature/ceremony check, and only then resolved the credential and applied `requireAgentId`. But `requireAgentId` was compared against the CREDENTIAL's owner — never against the Agent the challenge itself was minted for, even though both the signed token and the DB row already carry that `agentId`. Consequence: anyone holding a victim's step-up challenge token could spend it from their own session with any garbage `response`. The consume succeeded, verification then failed, and the victim's own legitimate verify came back `challenge_expired`. Not an authentication bypass — the assertion is still cryptographically verified and the credential-owner check still holds — but a griefing DoS against a real user's step-up. Fixed at both layers, matching spec §7's "two independent layers, not duplicated logic": - Application: reject when the token's `agentId` is not the acting Agent, BEFORE the consume runs. - Database: `consumeChallenge` takes an optional `expectedAgentId` that adds `AND agent_id = $3` to the UPDATE predicate. The login ceremony is deliberately unaffected: its challenges are minted with `agent_id IS NULL` (discoverable credential, spec §4.3), so the binding is conditional on `requireAgentId` rather than unconditional. Regression test proven to catch the original bug: with both layers reverted, it fails on the burn assertion (the victim's row is already consumed). One pre-existing step-up test was minting its challenge with `agentId: null`, which the new check would have short-circuited — it now mints for the impersonator so it still exercises the credential-owner check it is named for. Gates: typecheck, lint, test:coverage all exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(auth): cover the DB-layer Agent binding; bind registration consume too Codex review of the previous commit, findings 2 and 5. Finding 5 (the real one): the step-up ceremony test could not fail if the new `AND agent_id = $3` clause were deleted. The app-layer check returns first, so the store's clause was never reached — dead weight no test would notice losing. Two direct store-level tests added: a step-up row minted for one Agent is not consumable under another (proven to fail when the clause is removed), and a login row with `agent_id IS NULL` stays consumable when no Agent is expected, which guards the SQL branch itself. This also corrects an overclaim in the previous commit's PR description. Disabling the app layer and watching the test fail showed the DB clause firing in THAT configuration; it did not show the committed test covering the clause. Those are different properties and only the first was verified. Finding 2: registration's consume omitted the Agent argument, so its binding rested solely on the app-level check above it. No live bug — that check is correct — but the DB layer was not defense in depth the way step-up's now is. Passed through for consistency. Not addressed, deliberately: `verifyAuthenticationCeremony` still accepts `ceremony: 'step-up'` with no `requireAgentId`, which would silently disable the binding. No caller does this. Making it unrepresentable is a type-level signature change beyond this fix's scope — noted in the PR for TJ. Gates: typecheck, lint, test:coverage all exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
zaridan
added a commit
that referenced
this pull request
Aug 4, 2026
The previous pass keyed on tool names and left the framing they sat in. "HT-70 review fix:", "(review, 2026-07-31)", "(review fix)" narrate how a rule was found rather than stating it. 78 sites across specs and source, not the 49 the first scan reported. Ticket and PR references are kept as navigable provenance -- "HT-70:", "(PR #94)", "HT-101 (2026-07-31)". The "review fix" framing around them is gone. Also fixed: - Two lines the previous pass broke: a mangled JSDoc block in migrate.ts and a floating comma in conversations.ts. - Model names that the first sweep missed entirely -- "(Opus)" and "(Opus review fix)" in two files. - A test title used "assistant" for an AI actor; the fixed project term is "Assistant". - Two stale claims in agents-and-auth.md's consolidated changelog. It said `agent_mailbox_access` is "schema-only, no behaviour" while section 3.4 defines managed grants, auto-granting, and grant endpoints; and it said "no schema or behaviour in this spec changes" directly after describing `webauthn_credentials`. Both now match the sections they describe. Verified: typecheck, web typecheck, and lint clean. Every source edit is inside a comment except test titles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zaridan
added a commit
that referenced
this pull request
Aug 4, 2026
Source comments named the tooling that prompted a fix -- "CodeRabbit
(Major): claimThreadForDelivery's WHERE clause checked only the lease",
"HT-70 review fix (Codex)", "found by adversarial review" -- and several
recounted what an earlier revision got wrong instead of stating current
behaviour.
Each keeps its technical content and loses the narration. Where a comment
argued against an alternative by describing a past mistake, it now asks the
question directly:
store/mailboxes.ts "Why not tell the operator to disconnect and retry?
Because that instruction would be false:
markDisconnected only sets status ..."
providers/inbound-email.ts
"Why not return a NormalizedInboundEmail? That puts
the parse inside the provider ..."
mail/gmail-reconcile.ts
"Why not just ack? The tempting reasoning is 'the
holder will advance the cursor' ..."
Ticket and PR references are kept -- "HT-70:", "(PR #94)" -- as navigable
provenance. Vocabulary corrected in three places: "Agent" for a human
support role, "Assistant" for an AI actor, and "module loader" rather than
"plugin loader", since the substrate reserves "plugin" for the legal phrase
"plugin exception".
.coderabbit.yaml is untouched: a tool's own configuration has to name it.
No logic changed. Every edit is inside a comment except eight test titles,
which are descriptive strings.
Co-Authored-By: Claude Opus 5 <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.
Summary
Passkey (WebAuthn) login as a second core auth provider on the HT-54 seam (HT-75; spec
specs/auth/passkeys.md, merged in #88). Core-free — security hygiene is never paid (catalog §1). Engine + API only; the login/profile UI is design-blocked (separate ticket).webauthn_credentials,webauthn_challenges,webauthn_stepup_tokens(three additive tables, no backfill).ceremony-column layers: registration (gated on fresh ≤5min step-up re-auth — the planted-passkey ATO is impossible), authentication (conditional-UI with challenge re-mint), step-up. rpId/origin config-only (never request-derived;inbox.*UI origin; localhost-only HTTP).FOR UPDATE-locked comparison is the sole authority), sequential AND concurrent regressions rejected-recorded-logged, surfaced through a new/internal/healthwebauthn-counter-regressioncheck (200→503 pivot, HT-44 pattern).@simplewebauthn/server(MIT).Review trail — the full auth-critical gauntlet
Sonnet-authored → Codex independent pass: SHIP (verified the challenge-encoding round-trip against library source) → Fable lead-tier review: FIX-FIRST (1 MAJOR — the counter-regression observability shipped non-functional: no health check, the real library threw before our recording ran, tests mocked the throw away; all 8 security-critical lenses verified correct) → fix applied → targeted Codex re-check of the changed recording path: SHIP (confirmed
counter:0weakens no other library check). The two reviewers caught disjoint things — the diversity the tiered rule exists for.Gates
typecheck 0 · lint 0 · test 0 — 1507 passed. Migration 026 applied to prod ahead of merge (additive, no breaking window). No HT-5 dependency — passkeys are core, not a marketplace module (spec §1 corrected to match catalog §1 / #85 in this branch).
🤖 Generated with Claude Code
Summary by CodeRabbit
challenge_expiredunauthorized response.