Add passkey (WebAuthn) authentication#487
Conversation
Add @simplewebauthn/server and @simplewebauthn/browser at ^13.3.0 as the foundation for upcoming WebAuthn/passkey authentication support. This commit only updates package.json and the pnpm lockfile; no source code changes yet. Assisted-by: Claude Code:claude-opus-4-7
Introduce a `passkeys` table to back the upcoming WebAuthn / passkey authentication flow. Each row stores one credential returned by the browser at registration: the base64url credential id, the COSE public key, the signature counter, transport hints, device-type / backup-state metadata reported by SimpleWebAuthn, a user-supplied nickname, and the timestamps Hollo needs to render the management UI. Rows cascade-delete with the parent `credentials.email` so the credential record stays authoritative — when a credential is replaced (re-setup), its passkeys go with it instead of becoming orphans bound to an obsolete WebAuthn user handle. Assisted-by: Claude Code:claude-opus-4-7
Introduce `src/passkey.ts` as the seam between Hollo and
@simplewebauthn/server. It exposes a small, intent-named API the
upcoming HTTP routes can consume:
- `getRpInfo(requestUrl)` derives the relying-party ID and origin
from the incoming request, which is what makes the split-domain
setup (HANDLE_HOST + WEB_ORIGIN) Just Work: the browser is always
on the web origin during a ceremony, so its hostname is the rpID.
- `buildRegistrationOptions` / `verifyRegistration` and the matching
authentication pair wrap the library, enforce `residentKey:
required` (so logins can be username-less) and
`userVerification: required` (so a biometric / PIN gesture is
always part of the ceremony), and swallow the library's "invalid
response" throws into a plain `null` return so route handlers can
respond with a 400 without a stack trace in the logs.
- `encodePublicKey` / `decodePublicKey` keep the COSE blob portable
through a `text` column without reaching for a custom Drizzle
type.
- `nicknameFromUserAgent` lights up sensible defaults
("macOS device", "iOS device", …) so the management UI doesn't
open with an empty label.
The companion test file was written first; it stubs the library's
verify-* functions with `vi.mock`, so the round-trip of a real
authenticator response isn't required to lock in the wrapper
contract.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
When a TOTP is enrolled, `loginRequired` previously required a matching `otp` cookie before letting any request through. This commit teaches it to also honour a matching `passkey` cookie written by the upcoming passkey login flow. A passkey is itself multi-factor — the user has the authenticator and confirms with a biometric or PIN gesture during the WebAuthn ceremony — so stacking TOTP on top would add no real security and just hurt the experience of signing in. Either cookie, bound to the current `login` value to prevent session-mixing, now satisfies the gate. The new `src/login.test.ts` mounts a throwaway Hono app behind the middleware and walks the four-way matrix: no TOTP, TOTP + otp, TOTP + passkey, TOTP + nothing, plus mismatched-cookie cases. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
Wire the admin Auth page into the WebAuthn helpers added earlier.
Three new routes sit alongside the existing TOTP forms:
- POST /auth/passkeys/registration/begin returns the WebAuthn
creation options and stashes the challenge in a signed cookie.
The cookie body is `${challenge}|${expiresAt}|${login}`, which
binds the ceremony to the current login session and lets the
server enforce a 5-minute TTL even though Max-Age is only a
browser hint.
- POST /auth/passkeys/registration/finish reads the cookie back,
verifies expiry / session binding / WebAuthn assertion, and
inserts a row. The cookie is deleted unconditionally so a
captured value can't be replayed within its TTL; duplicate
credential ids return 409 instead of a false-positive 204; and
the row's nickname falls back to a UA-derived label
("macOS device", etc.) when the user leaves the field blank.
- POST /auth/passkeys/:id/delete drops the row and redirects back
to /auth.
The rendered AuthPage gains a Passkeys section below the TOTP
section that lists enrolled passkeys with their nickname, the
date they were added, and the date they were last used, plus a
small form the upcoming client script will hook into for
enrollment.
CSRF is intentionally left as a project-wide concern: the
existing /auth/2fa/disable, /logout, and the new passkey delete
all share the same posture, so widening the scope here would
just be inconsistent. A follow-up can apply a single token
mechanism across all auth-management POSTs in one pass.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Add a public-side passkey login flow alongside the existing
email/password path:
- POST /login/passkey/begin returns the WebAuthn request options
(empty allowCredentials, so the browser surfaces any resident
credential it knows about) and stashes the challenge in a signed
cookie. The cookie body is `${challenge}|${expiresAt}` so the
server enforces the 5-minute TTL even when Max-Age is just a
browser hint.
- POST /login/passkey/finish consumes the cookie unconditionally
(so a captured value is good for at most one request), parses the
JSON body manually after the cookie is already burnt so schema
failures can't bypass the consumption, verifies the assertion
against the stored credential, compare-and-sets the counter to
defeat concurrent ceremonies racing over an old value, and on
success writes both the `login` cookie and the `passkey` cookie
that the middleware accepts as a TOTP stand-in. Response is
`{ redirect }` JSON for the client script to follow.
The redirect target is clamped to the request's origin via
`safeNext`, which parses the candidate as a URL against the
request URL and demands a matching origin. That neutralises
open-redirect tricks like `/\evil.example/path` that browsers
normalise into `//evil.example/path`.
- LoginPage is now passkey-first: when at least one passkey is
enrolled, a "Sign in with passkey" button is the primary action
with the email/password form tucked behind a <details> toggle.
The fallback is opened automatically when a previous password
attempt failed so the error stays visible.
A follow-up commit will replace the stateless challenge cookie
with a `passkey_login_challenges` table so a captured assertion
+ cookie pair can't be replayed within the TTL even if a sync
passkey reports `signCount == 0`. Tracked as the next task on
the branch.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Add the bit of client-side JavaScript the auth and login pages
need to actually drive `navigator.credentials.create/get`. Two
files land in src/public/:
- simplewebauthn-browser.umd.js is a verbatim copy of the
@simplewebauthn/browser v13.3.0 UMD bundle; it exposes
`window.SimpleWebAuthnBrowser`. A short note at the top of
passkey.js documents how to re-vendor it after a dep bump.
- passkey.js is an IIFE glue script that hooks the existing form
ids: `#passkey-enroll-form` on the admin Auth page (POSTs to
/auth/passkeys/registration/{begin,finish} and reloads on
success) and `#passkey-signin-button` on the login page (POSTs
to /login/passkey/{begin,finish} and follows the server-supplied
redirect). It surfaces a friendly status line via an
aria-live="polite" element, distinguishes a NotAllowedError
cancellation from a real failure, disables the trigger while a
ceremony is in flight, and tolerates the 204 No Content reply
from the registration-finish endpoint instead of trying to parse
an empty body as JSON.
Both files are loaded with `<script defer>` only on the pages
that need them: always on the dashboard Auth page, and on the
login page only when at least one passkey is enrolled, so the
rest of the dashboard stays zero-JS.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Move the WebAuthn login challenge off a stateless signed cookie and onto a new `passkey_login_challenges` table. Each /begin inserts a row keyed by a random UUID; the signed cookie carries only that id, so the challenge itself never leaves the server. /finish atomically `DELETE … WHERE id RETURNING challenge, expires_at` — if the row is gone the request is rejected, which makes a captured cookie + assertion pair good for at most one /finish call even within the TTL. Expired rows are GC'd opportunistically at the top of /begin so the table can't grow unbounded without a separate cleanup job. This closes the replay window the previous commit flagged as a defense-in-depth follow-up: a sync passkey that reports `signCount == 0` would otherwise allow a captured pair to be replayed up to the 5-minute TTL. Registration challenges stay stateless — they're already bound to the logged-in session, so session-binding plus the existing TTL suffice. A new test exercises the single-use semantics by sending two /finish requests with the same captured cookie and asserts the second one is rejected without even calling `verifyAuthentication`. The mock-clear in the test suite was switched from `mockClear()` to `mockReset()` to stop one-shot mocks leaking between tests. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
Add a CHANGES.md entry for the new WebAuthn / passkey support under the unreleased v0.9.0 section: enrollment + management on the admin Auth page, passkey-first login with a password fallback, both device-bound and synced passkeys accepted, the TOTP step skipped when the passkey ceremony succeeds. The security envelope (residentKey/userVerification both required, session-bound registration challenge, single-use login-challenge table) is called out so operators upgrading from 0.8 see the guarantees explicitly. Add a "Page-scoped client scripts" subsection to DESIGN.md that codifies the pattern this branch introduced: Layout and the dashboard chrome stay JavaScript-free, existing tiny inline scripts in pages are fine where they are, but anything that genuinely can't function without JS (currently just the WebAuthn ceremonies) should live as a small hand-written file in src/public/ linked via <script defer> from only the pages that need it, with a real fallback when the script fails to load. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
Three follow-ups from the branch-wide review: - /login/passkey/begin now refuses with 404 before any DB write when no passkeys are enrolled. Otherwise an unauthenticated caller could pump rows into passkey_login_challenges on an instance with nothing to actually authenticate against. - The endpoint also enforces a soft cap of 64 outstanding unexpired challenges and returns 429 + Retry-After: 300 when the cap is reached. Hollo is single-user, so realistic concurrent ceremonies are far below this number; the cap exists purely to keep the table bounded under abuse. GC + count + insert are wrapped in a single transaction guarded by pg_advisory_xact_lock so concurrent /begin calls serialise on the lock rather than racing between the count and the insert. - src/passkey.ts now exports `sanitizeTransports`, which keeps only the WebAuthn-defined values (ble, cable, hybrid, internal, nfc, smart-card, usb) and drops anything else. `verifyRegistration` runs the client-supplied `response.transports` through it before returning, so a bogus or hostile value from the browser never reaches the DB or a future excludeCredentials payload. Each behaviour has a focused test in the appropriate suite. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements passkey (WebAuthn) authentication, allowing users to enroll and manage passkeys on the admin Auth page and sign in using them on the public login page. The implementation introduces new database tables for passkeys and login challenges, updates the login middleware to accept passkeys as a second factor, and adds the necessary server-side and client-side logic for WebAuthn ceremonies. Feedback includes suggestions to use the central SECRET_KEY export in src/pages/login.tsx for consistency and to capture a single timestamp within the passkey login challenge transaction to ensure temporal consistency between data cleanup and counting.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e6a35f285
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/pages/login.tsx was reading SECRET_KEY straight off process.env with a local null check. src/login.ts and src/pages/auth.tsx both import it from src/env.ts, which is also where the 44-char minimum length is enforced; reading process.env directly bypassed that check. Switch to the same import so the validation runs and the pattern is consistent across the auth-adjacent modules. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
/login/passkey/begin called new Date() twice inside the transaction: once for the GC delete predicate and once for the outstanding-count predicate. If the wall clock ticked between the two calls, the two predicates could disagree about which rows were "expired", and a row sitting exactly on the boundary could be deleted and then re-counted (or vice versa) inside the same lock. Capture a single new Date() at the top of the transaction and reuse it for both predicates so they see the same instant. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
postJson() assumed every 2xx response had a JSON body. The /auth/passkeys/* endpoints sit behind auth middleware that redirects to /login when the session is gone (e.g., the user logged out in another tab and then clicked "Add passkey" here). fetch() follows that redirect transparently, so response.ok stays true and the final HTML page reaches response.json(), which throws. The enrollment then surfaced as a generic "could not enroll a passkey" message and the user had no signal to go re-authenticate. Check response.redirected before any JSON parsing. When it's true, navigate the browser to response.url so the user lands on the real /login page, and throw a short status message for the inline aria-live region. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62809887b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces passkey (WebAuthn) authentication, enabling users to register and manage device-bound or synced passkeys as a multi-factor authentication method. The implementation includes new database tables for passkeys and login challenges, session-bound challenges with a 5-minute TTL, and integration with the @simplewebauthn library suite. Review feedback highlighted a security vulnerability regarding open redirects in the URL sanitization logic and suggested an optimization to avoid unnecessary memory allocations when decoding public keys.
safeNext parses the incoming `next` value against the request URL and demands a matching origin. That catches `/\evil.example/x` style inputs (WHATWG URL parsing moves the origin away from the request origin in those cases). But `/.//evil.example/x` is normalised by the same parser into pathname `//evil.example/x` against the request origin, so the same-origin check passes and safeNext returned the protocol-relative-looking string. The client passes the result straight to window.location.assign(), which treats a leading `//` as a protocol-relative URL and navigates cross-origin. Reject any post-normalisation pathname that starts with `//` and fall back to `/`. Added a regression test asserting that `/.//evil.example/phish` collapses to `/` in the /login/passkey/finish response. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
The registration finish handler had zValidator("json", schema)
wired up as middleware in front of the handler. zValidator runs
before any handler code, so a request with a malformed body
returned 400 from the validator and the deleteCookie() call that
sits at the top of the handler never executed. A captured
passkey_reg signed cookie therefore stayed reusable for the rest
of its 5-minute TTL, contradicting the route's "consume on every
path" intent.
Drop the zValidator middleware on this route and instead read the
JSON body inside the handler with c.req.json(), then run
finishBodySchema.safeParse(). Both happen after the cookie has
already been deleted, so any path that leaves the handler burns
the cookie. This mirrors what login.tsx /passkey/finish already
does for the same reason.
A new test posts a body that fails the schema and asserts the
response still sets `passkey_reg=` to clear the browser-side
copy.
fedify-dev#487 (comment)
Assisted-by: Claude Code:claude-opus-4-7
Buffer is a subclass of Uint8Array, so Buffer.from(encoded, "base64url") already returns something that satisfies the Uint8Array<ArrayBuffer> return type and the only downstream caller (SimpleWebAuthn's verifyAuthenticationResponse) reads the bytes either way. The previous version allocated a fresh ArrayBuffer and copied the buffer's contents into a plain Uint8Array; nothing here benefits from that round trip. The two unit tests that round-trip a key now compare via Array.from() rather than toEqual() on the typed-array instance, since toEqual checks the prototype as well and a Buffer doesn't deep-equal a Uint8Array under that rule. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
/login/passkey/begin used to refuse with 429 and Retry-After: 300 once the table held 64 unexpired challenges. Because the endpoint is unauthenticated, any actor could send 64 POSTs in a burst and park the table at the cap for the full 5-minute TTL, which forced every legitimate passkey sign-in into 429 for that window and turned the anti-bloat safeguard into a trivial availability DoS. Switch the at-cap branch from "return 429" to "evict the oldest unexpired row, then insert." The cap still holds (size never exceeds 64) but no caller ever sees a 429, so an attacker can't hold the door shut. Eviction picks the row with the smallest expires_at, which is the FIFO head since expires_at is just insert time + TTL. There's a residual displacement risk: a sustained burst could race a legitimate user's row out of the table before they reach /finish. That's a much harder attack than the cap-park one this fixes, and it lives at the same threat tier (network-bandwidth abuse) where a reverse proxy is the right defense layer. The 429 test in src/pages/login.test.ts is replaced with one that asserts the eviction behaviour: pre-fill the table to 64 rows with ascending expires_at, hit /begin, and check the response is 200, the table size is still 64, and the oldest seeded row is gone. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces passkey (WebAuthn) authentication, allowing users to register, manage, and sign in with device-bound or synced passkeys. The implementation includes new database tables for passkeys and login challenges, server-side verification logic using the @simplewebauthn library, and a lightweight client-side script for handling WebAuthn ceremonies. Feedback from the reviewer suggests simplifying a complex type cast in the authentication page and adopting modern JavaScript conventions, such as using const/let and async callbacks, in the new client-side script.
The cast on `p.transports` was a multi-line conditional-types expression that fished the element type out of buildRegistrationOptions' parameter shape via `Parameters<...>` and `infer T`. The element type is just AuthenticatorTransportFuture from @simplewebauthn/server, which the same file already imports transitively, so import it directly and cast to AuthenticatorTransportFuture[]. Runtime is identical; the call site fits on one line again. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
Two cleanups in one pass, both inside src/public/passkey.js:
- Replace every `var` with `const`, except for the two locals in
postJson's !response.ok branch (`detail` and `error.message`
fallback path) which are written conditionally and stay `let`.
The rest of the script was already using async/await and arrow
functions, so `const` is in keeping with the style.
- Replace the `(async function () { ... })()` IIFEs that wrapped
the async work inside the submit and click handlers with
`async (event) => { ... }` / `async () => { ... }` passed
directly to addEventListener. The extra indentation level
goes away and the handler is a single function instead of a
sync function that immediately fires off an async one.
No runtime behaviour change.
fedify-dev#487 (comment)
fedify-dev#487 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements passkey (WebAuthn) authentication, allowing users to manage passkeys in the admin dashboard and use them as a second factor during login. The implementation includes new database tables for passkeys and login challenges, a passkey utility module, and client-side scripts for WebAuthn ceremonies. Feedback suggests that the safeNext origin check might be bypassed behind reverse proxies if proxy headers are not correctly trusted and recommends using local timezone formatting for dates in the UI instead of UTC.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The Passkeys section used `toISOString().slice(0, 10)` directly
in string concatenation, which renders a fixed YYYY-MM-DD in UTC
regardless of the operator's timezone. Hollo's other date
listings (e.g. src/components/AccountList.tsx) already wrap dates
in `<time dateTime={iso}>{date.toLocaleDateString()}</time>`, so
the visible text follows the server's locale and the dateTime
attribute carries the canonical ISO instant for any browser-side
enhancement to re-render in the visitor's locale.
Bring the auth page in line with that pattern: each "Added" and
"last used" date is wrapped in a `<time>` and formatDate now
returns `value.toLocaleDateString()`. The comment above
formatDate is updated to point at AccountList as the precedent.
fedify-dev#487 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces passkey (WebAuthn) authentication, enabling users to enroll, manage, and sign in with device-bound or synced passkeys. The changes include new database tables for passkeys and login challenges, middleware updates to support passkeys as a second factor, and a lightweight client-side script for WebAuthn ceremonies. The reviewer suggested enhancing the atomicity of challenge consumption by moving the expiration check directly into the database deletion query.
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
/login/passkey/finish used to DELETE any row matching the cookie id, return the row, and then check expires_at in JavaScript to decide whether to accept the challenge. That deleted expired rows on every poke, which is fine but redundant: the row is already going to be cleaned up on the next /begin's GC pass. Fold the expiry check into the WHERE clause as `id = ? AND expires_at >= now()`, drop the separate JS branch, and trim expires_at off the RETURNING list. Behaviour for a legitimate /finish is identical; behaviour for an expired one shifts from "delete the row, then return 400" to "leave the expired row alone, return 400 mentioning expired-or-used." Same security envelope; one SQL statement does the consume-only-if- still-valid step atomically. The DATE expression uses `new Date(Date.now())` rather than bare `new Date()` so the existing time-travel test, which stubs Date.now to fast-forward past the TTL, still trips the SQL expiry predicate. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements passkey (WebAuthn) authentication, enabling users to enroll and manage passkeys in the admin dashboard and use them for multi-factor authentication during login. The changes include new database migrations for passkeys and login challenges, server-side logic using the @simplewebauthn library, and a lightweight client-side script for browser-based ceremonies. Review feedback suggests enhancing Zod schema validation for credential IDs, removing a redundant string trimming operation, and adopting more idiomatic date instantiation.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The Zod schemas accepting passkey responses on both routes had `id: z.string()` (and matching `rawId`) without a length floor. A real authenticator never returns an empty credential id, but the validator would have let one through to the SimpleWebAuthn verifier, which would reject it later with a less specific error. Add `.min(1)` on `id` and `rawId` in both schemas (src/pages/auth.tsx finishBodySchema; src/pages/login.tsx passkeyFinishSchema where the same constraint was already on `id` but missing on `rawId`). fedify-dev#487 (comment) fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
finishBodySchema declares the nickname field as z.string().trim().max(80).optional(), so by the time body.nickname reaches the handler it has already been trimmed (or is undefined). The handler was calling .trim() on it again and stashing the result in a `trimmedNickname` local that did nothing useful. Use body.nickname directly and leave a short comment recording the schema-side guarantee so a future reader doesn't add the trim back out of habit. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
The /passkey/finish DELETE WHERE expiry predicate read `new Date(Date.now())` only because the existing time-travel test stubbed Date.now with vi.spyOn(Date, "now"), and V8's Date constructor doesn't go through that stub when called with no arguments. The rest of the file uses bare `new Date()` (e.g., the GC inside /begin), so this read like dead-weight defensive code. Migrate the test to timekeeper.travel(...), which mocks both Date.now() and `new Date()`. Other Hollo tests already use this pattern (src/federation/account.test.ts, src/oauth.test.ts). The finally block now calls timekeeper.reset() instead of mockRestore. With the test on timekeeper, the source-side workaround is no longer needed; switch the WHERE clause to bare `new Date()` to match the surrounding idiom. fedify-dev#487 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces passkey (WebAuthn) authentication, enabling users to register and manage passkeys through the admin interface and use them for multi-factor authentication during login. The changes encompass new database migrations, the integration of @simplewebauthn libraries, and updates to the authentication middleware. A review comment suggests explicitly importing webcrypto from node:crypto in src/passkey.ts to improve environment compatibility and maintain consistency with the project's existing import style.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Hollo's sign-in story so far has been email and password plus an optional single-instance TOTP. There was no way to enroll a phishing-resistant credential, and there was no way to skip the TOTP prompt for users who already authenticate the same gesture into their authenticator. This branch lands passkeys on both the admin Auth page and the public login page, using @simplewebauthn/server for verification and the matching browser helper as a vendored static asset.
Configuration
There is no new environment variable. The relying-party ID and origin are derived from the incoming request URL at runtime, so single-origin and split-domain (
HANDLE_HOST+WEB_ORIGIN) deployments both work without operator intervention: the browser is always on the web origin during a ceremony, and that hostname is therpID.Data model
Two new tables in src/schema.ts, both generated through
pnpm migrate:generate.passkeys (migration 0088_passkeys.sql) stores one row per enrolled credential: the base64url credential id as the primary key, the COSE public key, the signature counter, transport hints, the
credentialDeviceType/credentialBackedUpflags returned by SimpleWebAuthn, a user-supplied nickname, and the timestamps the management UI renders. Rows cascade-delete with the parentcredentials.emailso a credential reset clears its passkeys instead of leaving orphans bound to an obsolete WebAuthn user handle.passkey_login_challenges (migration 0089_passkey_login_challenges.sql) holds one row per outstanding login ceremony, keyed by a random UUID, with the challenge and an expiry. The signed cookie issued by
/login/passkey/begincarries only the row's id; the challenge itself never leaves the server./finishconsumes the row with an atomicDELETE … WHERE id RETURNING challenge, expires_at, so a captured cookie plus assertion pair can be redeemed at most once even if the underlying authenticator reportssignCount == 0.Model helpers
src/passkey.ts is the seam between Hollo and @simplewebauthn/server. It exposes
getRpInfo,buildRegistrationOptions/verifyRegistration,buildAuthenticationOptions/verifyAuthentication,encodePublicKey/decodePublicKey,sanitizeTransports, andnicknameFromUserAgent. The wrappers enforceresidentKey: requiredanduserVerification: requiredso every enrolled credential is discoverable and tied to a biometric or PIN gesture, and they swallow the library's “invalid response” throws into a plainnullreturn so route handlers can answer with a flat400 Bad Request.sanitizeTransportskeeps only the WebAuthn-defined values (ble,cable,hybrid,internal,nfc,smart-card,usb) and drops anything else, so a malicious browser cannot poison a futureexcludeCredentialspayload.The companion src/passkey.test.ts was written first. It stubs the library's verify functions with
vi.mock, which keeps the suite self-contained without a real authenticator response fixture.Login middleware
loginRequiredin src/login.ts previously demanded a matchingotpcookie when a TOTP was enrolled. It now accepts either anotpcookie or apasskeycookie. A passkey is itself multi-factor (the user has the authenticator and confirms with a biometric or PIN gesture), so stacking TOTP on top would not add real security and would hurt the experience. Both cookies are bound to the currentloginvalue so they cannot be reused across sessions. src/login.test.ts covers the four-way matrix plus mismatched-cookie cases.Admin Auth page
src/pages/auth.tsx gains a Passkeys section below the existing TOTP section. It lists enrolled passkeys with their nickname, creation date, and last-used date, with a per-row delete form. An enrollment panel takes an optional nickname (defaulting to a UA-derived label such as “macOS device”) and submits to three new routes:
POST /auth/passkeys/registration/beginreturns the WebAuthn creation options and stashes the challenge in a signed cookie. The cookie body is${challenge}|${expiresAt}|${login}, which binds the ceremony to the current login session and lets the server enforce a 5-minute TTL even thoughMax-Ageis only a browser hint.POST /auth/passkeys/registration/finishreads the cookie back, verifies expiry, session binding, and the WebAuthn assertion, then inserts a row. The cookie is deleted unconditionally so a captured value cannot be replayed within its TTL; duplicate credential ids return409 Conflictinstead of a false-positive204 No Content.POST /auth/passkeys/:id/deletedrops the row and redirects back to /auth.Login page
src/pages/login.tsx becomes passkey-first when at least one passkey is enrolled: a "Sign in with passkey" button is the primary action and the email/password form is tucked behind a
<details>toggle, which opens automatically when a previous password attempt failed so the error stays visible.POST /login/passkey/beginreturns the WebAuthn request options with an emptyallowCredentialsso the browser surfaces any resident credential it knows about, inserts a row into passkey_login_challenges, and returns the id in a signed cookie. The endpoint refuses with404 Not Foundbefore any database write when no passkeys are enrolled, so an unauthenticated caller cannot pump rows into the table on an instance with nothing to authenticate against. It also enforces a soft cap of 64 outstanding unexpired challenges and returns429 Too Many RequestswithRetry-After: 300when the cap is reached. GC plus count plus insert run inside a single transaction guarded bypg_advisory_xact_lock, so concurrent/begincalls serialize on the lock rather than racing between the count and the insert.POST /login/passkey/finishconsumes the challenge row atomically (so a captured cookie is good for at most one request), parses the JSON body manually after the row is already burnt so schema failures cannot bypass the consumption, verifies the assertion against the stored credential, and compare-and-sets the counter to defeat concurrent ceremonies racing over an old value. On success it writes thelogincookie and thepasskeycookie the middleware accepts, and returns{ redirect }for the client script to follow. The redirect target is clamped to the request's origin viasafeNext, which parses the candidate as a URL against the request URL and demands a matching origin, neutralizing open-redirect tricks like/\evil.example/paththat browsers normalize into//evil.example/path.Client script
The dashboard stays JavaScript-free outside the two pages that need the WebAuthn ceremonies. Two files land in src/public/: simplewebauthn-browser.umd.js is a verbatim copy of @simplewebauthn/browser 13.3.0 exposing
window.SimpleWebAuthnBrowser, and passkey.js is a small IIFE that hooks#passkey-enroll-formon the admin page and#passkey-signin-buttonon the login page. It surfaces status via anaria-live="polite"element, distinguishes aNotAllowedErrorcancellation from a real failure, disables the trigger while a ceremony is in flight, and tolerates the204 No Contentreply fromregistration/finish. Both files are loaded with<script defer>only on the pages that need them.DESIGN.md gains a short “Page-scoped client scripts” subsection that codifies this pattern: layout stays zero-JS, tiny inline scripts in pages are fine where they are, but anything that genuinely needs JS (currently just the WebAuthn ceremonies) should live in src/public/ and be linked only from the pages that need it.
Verification
pnpm checkis clean and the full Vitest suite passes (495 tests). The flow was also exercised end-to-end against a Cloudflare-tunnelled local dev server using Playwright plus a Chromium virtual WebAuthn authenticator: enrollment from /auth, passkey sign-in landing on the dashboard without a TOTP prompt,lastUsedupdating on every assertion, removal, and the password fallback toggle still working when no passkey is enrolled.