feat(passkeys): WebAuthn coordinate storage API (WA-2113..WA-2119)#3078
feat(passkeys): WebAuthn coordinate storage API (WA-2113..WA-2119)#3078compojoom wants to merge 14 commits into
Conversation
Creates the passkey_coordinates table with strict CHECK constraints on every fixed-width bytea column, FILLFACTOR=100 for the insert-only/immutable workload, and a destructive-rollback warning on the down migration.
…A-2114) Adds the empty PasskeysModule (controller / service / repository skeletons, TypeORM entity for passkey_coordinates), conditional registration in AppModule behind FF_PASSKEYS, and configuration keys for RP-ID / origin / verifier allowlists, registration & lookup rate limits, and the attestation verification timeout. The module's onModuleInit fails closed if any of the three allowlists is empty when the flag is on, preventing a silent fail-open regression. Repository carries the idempotent first-write-wins create() with ON CONFLICT DO NOTHING + post-conflict re-select to distinguish identical / conflict / cross-RP-conflict outcomes; controller/service remain empty until Phase 3/4 attach endpoints.
Adds a NestJS route that verifies a WebAuthn attestation via
@simplewebauthn/server, derives the (x, y) P-256 coordinates from the
COSE_Key in the attestation, and persists them idempotently against the
unforgeable credentialId.
Key invariants:
- DTO has no x/y field — coordinates are NEVER trusted from the client.
- expectedChallenge is a deterministic stateless function bound to
(rpId, origin, verifiers); compared in constant time.
- 24 KiB per-route body cap (route-scoped express.json) plus per-field
DTO maxLengths defend against CBOR-bomb DoS.
- 500ms timeout around verifyRegistrationResponse protects the worker
thread from cert-chain stalls (tpm / android-safetynet).
- ON CONFLICT DO NOTHING + post-conflict re-select distinguishes
201 / 200 / 409 (PASSKEY_CONFLICT vs PASSKEY_CROSS_RP_CONFLICT).
- Error responses are an opaque { code: errorId } envelope; library
internals never leak.
- verifiers (uint176, 22 bytes) is allowlist-checked at 403 before
attestation verification runs.
…A-2116) Public, no-auth lookup of stored coordinates by credentialId. Validates the path parameter is well-formed base64url that decodes to 1..1023 bytes (WebAuthn L3 credentialId limits) before hitting the database. Cache strategy via a route-scoped interceptor: - 200 → public, max-age=86400, s-maxage=2592000, immutable + ETag (rows are immutable, key is unforgeable; CDN absorbs ~90% of read load) - 4xx → no-store (avoids stale-negative cache that would lock first-launch flows immediately after a brand-new POST lands at the origin) The interceptor replaces the global no-cache CacheControlInterceptor on this route only; ETag is the first 16 bytes of the credentialId, which the client already supplied on the URL.
Two guards subclass the existing RateLimitGuard: - PasskeysRegistrationRateLimitGuard (default 20 / 600s) - PasskeysLookupRateLimitGuard (default 120 / 600s; reads >> writes) Both are wired via @UseGuards on the relevant controller methods. A small canActivateWithRateLimitHeaders helper sets Retry-After and Cache-Control: no-store on the response before the 429 propagates — the guard layer runs before interceptors, so the lookup interceptor's catchError cannot reach those exceptions. Asymmetric budgets are deliberate per RFC §4.6: reads are first-launch on a new device, writes are already gated by attestation verification.
Adds a PasskeyErrorResponse DTO that exports the full errorId enum from the Phase 3 status-mapping table, and wires it into every error response on both endpoints so generated clients receive a typed union for the 'code' field. Also documents the GET 200 response headers (Cache-Control, ETag) so edge clients can plan around the long s-maxage / immutable contract.
…WA-2119) Spins up a NestJS test app with mocked PasskeyAttestationService and IPasskeysRepository, then exercises the full Express pipeline: - 201 / 200 / 403 / 422 / 503 / 409 / 400 / oversize-body status paths - ETag and Cache-Control: public, max-age=86400, s-maxage=2592000, immutable on GET hits - Cache-Control: no-store on GET 404 (prevents stale-negative cache) Real attestation-fixture e2e (recorded once from iOS / Android / hardware key, replayed in CI) is a follow-up — committing fixtures requires a live registration ceremony outside this PR. Two non-test changes were needed to make this work end-to-end: 1. The global CacheControlInterceptor now respects an existing Cache-Control set by an inner interceptor or @Header decorator. It used to unconditionally overwrite to 'no-cache' because its tap runs last in the chain — that silently downgraded the immutable lookup policy. 2. The route-scoped 24 KiB body cap is now bound via forRoutes( PasskeysController) instead of a path string, so it survives any future URI-versioning change.
![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: 3ad0053eba
ℹ️ 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".
Five fixes from the multi-agent review of the WebAuthn coordinate
storage API:
1. **mapLibraryError** now logs unexpected errors before remapping. It
only catches SyntaxError/RangeError (legitimate CBOR/base64 decode
failures) → PASSKEY_MALFORMED_ATTESTATION. Anything else is logged
via LoggingService.error and re-thrown so a real bug in
@simplewebauthn/server isn't silently reported as "user sent bad
input".
2. **runWithTimeout** no longer leaks the underlying verification
promise on the timeout path. The work promise gets a .catch() that
logs late rejections so they aren't unhandled. A note documents that
setTimeout-based timeout cannot interrupt synchronous CPU work —
cancellation is best-effort.
3. **mapAttestationError** 500 fallthrough now logs the original error
(type, message, stack) and attaches it as `cause` on the
HttpException so post-deploy debugging has something to grep for.
4. **Error envelope** is now a superset of CGW's standard shape:
`{ statusCode, code, message }`. Stable machine-readable codes are
preserved while message+statusCode keep generated typed clients
compatible with the rest of the API. Every throw site updated;
PasskeyErrorResponse DTO documents the superset relationship.
5. **e2e body-cap test** is no longer relaxed to "any 4xx". It now
asserts [413, 400] AND that attestation.verify was never called, so
it fails if some unrelated handler-side check (verifiers allowlist,
Zod) accidentally satisfies the test. Added a complementary
boundary test that sends a payload just under the cap and asserts
the handler did run — proves the cap isn't accidentally tighter
than 24 KiB.
Tests: 39/39 passing.
Local-only planning docs shouldn't ship in PRs. Untrack the existing passkey plan and add /docs/plans/ to .gitignore so future plans stay out of git by default.
- Drop redundant `async` on rate-limit guard canActivate (returns the helper's promise directly). - Type `verified` as VerifiedPasskey instead of implicit any. - Convert fakeCache.increment from async to Promise.resolve to satisfy useAwait. - Auto-formatter passes (line breaks, simplified logical expressions in cache-control interceptor and attestation service).
|
@claude review |
|
Claude finished @compojoom's task —— View job Review of feat(passkeys): WebAuthn coordinate storage APIOverall this is well-structured with good security posture (fail-closed bootstrap, timing-safe challenge comparison, deterministic stateless challenges, per-field byte caps). Here are the items to address: Bug —
|
Codex P1 on PR #3078: `INSERT ... RETURNING *` exposes driver-level rows (snake_case columns), not entity-mapped fields. The success path was reading `raw[0].credentialId` / `raw[0].createdAt` (camelCase) and getting `undefined`, which would have crashed `serialize` on the very first successful POST. Fix: re-select by credentialId via the entity repository on the insert path. The extra ~1 ms round-trip is negligible — the cold path is already gated behind 500 ms attestation verification. Resolves: #3078 (comment)
Five follow-ups from claude.ai's review of PR #3078: - DTO validates `origin` as a URL at the framework boundary as defense-in-depth (the allowlist is still the load-bearing check). - Conflict detection now reports cross_rp_conflict whenever the RP differs, even if coords also differ — gives clients a clearer signal ("different domain" vs "different keys on this domain"). - Repository now has a dedicated unit spec covering the snake_case bug (would have caught it pre-merge), the new rpId-first conflict semantics, and the row-vanished defensive branch. - Removed dead `createdAt: new Date()` in the service — DB defaults to `now()` and we re-select after insert, so the placeholder was never read. Introduced PasskeyRecordInput (Omit<...,'createdAt'>) for the write path so the type system enforces the distinction. - Documented the hardcoded windowSeconds in configuration.ts (rate limit `max` is the only env-tunable knob; per-window seconds is fixed by the use case). Tests: 45/45 passing (up from 39 with the new repo spec).
|
@claude review |
|
Claude finished @compojoom's task —— View job Review of feat(passkeys): WebAuthn coordinate storage API
Well-structured PR with strong security posture. The previous review's blocking bug ( 1. Stateless challenge is replayable across time
2.
|
The allowlist was meant to prevent unbounded write-spam with attacker-chosen verifiers, but the four other defenses already cover it: - WebAuthn attestation requires the credentialId's private key. - The stateless challenge binds (rpId, origin, verifiers); the server recomputes and constant-time-compares — verifiers cannot be swapped on someone else's attestation. - Per-IP registration rate limit caps write volume. - Rows are immutable; first-write-wins. What the allowlist actually added was operational cost (must update env vars before each contract deployment, fail-closed bootstrap on empty config) for marginal data-quality gain. Drop it. `verifiers` is still validated as 22-byte well-formed hex by the DTO regex; clients are still trusted to provide a Safe-deployment-aligned value, and any garbage they submit is bounded by their rate budget.
PooyaRaki
left a comment
PooyaRaki
left a comment
There was a problem hiding this comment.
I reviewed the PR except for the tests, since I expect there will be quite a few changes based on the remarks. I’ll review the tests together with the next iteration.
| `DO $$ | ||
| DECLARE row_count bigint; | ||
| BEGIN | ||
| SELECT count(*) INTO row_count FROM passkey_coordinates; |
There was a problem hiding this comment.
We should remove this query or at least change it to count(credential_id)
There was a problem hiding this comment.
Resolved in a926edf — removed the diagnostic RAISE NOTICE count query entirely. It was purely informational ahead of the DROP TABLE, and the destructive nature is already documented in the file header.
| verifiers bytea NOT NULL, | ||
| rp_id text NOT NULL, | ||
| created_at timestamptz NOT NULL DEFAULT now(), | ||
| CONSTRAINT PK_passkey_coordinates PRIMARY KEY (credential_id), |
There was a problem hiding this comment.
Should we create an integer id as the PK and then make this one unique?
There was a problem hiding this comment.
Done in a926edf — added a SERIAL surrogate id as the PK and a UNIQUE constraint on credential_id, matching the repo convention (create_users, create_wallets, create-counterfactual-safes). The entity was updated to @PrimaryGeneratedColumn + @Unique, and lookups still go through credential_id.
| rateLimit: { | ||
| registration: { | ||
| max: faker.number.int({ min: 10, max: 100 }), | ||
| windowSeconds: 600, |
There was a problem hiding this comment.
windowSeconds s should also use faker
There was a problem hiding this comment.
Done in a926edf — both windowSeconds entries now use faker.number.int({ min: 60, max: 3600 }), consistent with the surrounding faker-generated fields.
| .map((s) => s.trim()) | ||
| .filter(Boolean), | ||
| originAllowlist: ( | ||
| process.env.PASSKEYS_ORIGIN_ALLOWLIST ?? 'https://app.safe.global' |
There was a problem hiding this comment.
All these env vars should be validated:
src/config/entities/schemas/configuration.schema.ts
There was a problem hiding this comment.
Done in a926edf — all PASSKEYS_* env vars are now validated in configuration.schema.ts (allowlists as CSV piped to arrays, rate-limit maxima and timeout as coerced positive ints). The lookup-cache TTL vars added in this round are validated too.
| export type PasskeyRecordInput = Omit<PasskeyRecord, 'createdAt'>; | ||
|
|
||
| export type WriteOutcome = | ||
| | { status: 'created'; record: PasskeyRecord } |
There was a problem hiding this comment.
We should read status from a const e.g.:
https://github.com/safe-global/safe-client-gateway/blob/022284aebe99fd896690709f0615a4fb542fbde9/src/modules/users/domain/entities/member.entity.ts
There was a problem hiding this comment.
Done in a926edf — extracted a WriteOutcomeStatus enum mirroring the MemberStatus pattern, and migrated all call sites (repository, service, specs).
| // The origin allowlist in the service is the load-bearing check, but URL | ||
| // parsing here strips control characters / whitespace / non-URL strings at | ||
| // the framework boundary as defense-in-depth. | ||
| origin: z.string().min(1).max(512).url(), |
There was a problem hiding this comment.
This is deprecated we should use z.url() instead.
There was a problem hiding this comment.
Done in a926edf — origin now uses z.url().min(1).max(512) instead of the deprecated z.string().url().
| return next.handle().pipe( | ||
| tap(() => { | ||
| if (!response.headersSent) { | ||
| response.setHeader('Cache-Control', HIT_CACHE_CONTROL); |
There was a problem hiding this comment.
QQ: Why do we need to override the global values here? I’m concerned this could create friction in the future if we want to change the cache duration.
There was a problem hiding this comment.
The override is intentional and not avoidable: the global CacheControlInterceptor sets no-cache for every endpoint (sensible default), but passkey rows are immutable (credentialId is the PK and the row is never mutated), so the lookup can be cached aggressively. The global interceptor explicitly defers to inner interceptors via a getHeader(Cache-Control) guard, so this is the route-specific policy the framework expects rather than a fight against a configurable global. On the friction concern (fair point): in a926edf the TTLs are moved out of constants into passkeys.lookupCache.hitMaxAgeSeconds / hitSharedMaxAgeSeconds, tunable via PASSKEYS_LOOKUP_CACHE_HIT_MAX_AGE_SECONDS and PASSKEYS_LOOKUP_CACHE_HIT_S_MAX_AGE_SECONDS. Defaults preserve current behaviour, so future tuning is a config change. The no-store miss policy stays hardcoded since it is a correctness property (prevents stale-negative caching).
| @HttpCode(HttpStatus.CREATED) | ||
| public async register( | ||
| @Body(new ValidationPipe(RegisterPasskeySchema, HttpStatus.BAD_REQUEST)) | ||
| dto: import('@/modules/passkeys/routes/entities/register-passkey.dto.entity').RegisterPasskeyDto, |
There was a problem hiding this comment.
Why do we need to import this inline? We could import the type at the top and have:
dto: RegisterPasskeyDto
There was a problem hiding this comment.
Done in a926edf — added a top-level import type { RegisterPasskeyDto } and the parameter now reads dto: RegisterPasskeyDto. It is a pure type alias so there is no runtime import or circular-dependency risk.
| return createHash('sha256').update(material).digest('base64url'); | ||
| } | ||
|
|
||
| function serialize(record: PasskeyRecord): PasskeyRecordResponse { |
There was a problem hiding this comment.
If we used TypeOrm transformers we could remove this function from here.
There was a problem hiding this comment.
Partially — the databaseBufferTransformer (a926edf) removed the manual Buffer conversions, but a serialization step is still needed to shape the PasskeyRecordResponse returned to clients (hex/base64url string encoding for the API contract), which is separate from DB storage. So rather than delete it, it is now a private serialize() method on the class (see below).
| * them without regenerating the attestation, and | ||
| * - the comparison is constant-time (in PasskeyAttestationService). | ||
| */ | ||
| function deriveStatelessChallenge(dto: RegisterPasskeyDto): string { |
There was a problem hiding this comment.
Why is this a function rather than a class method?
Ideally, each file should contain a single type of object (e.g. class, interface, etc.).
Since this method is only used here, it could be implemented as a class method. If we intended to reuse it elsewhere, we should move it into a shared helper within the passkey module.
There was a problem hiding this comment.
Done in a926edf — the standalone helpers in this file (decodeCredentialId, deriveStatelessChallenge, serialize, stripHex) are now private methods on PasskeysService, so the file holds a single class per the convention. Call sites updated to this.….
- migration: drop diagnostic notice query; introduce SERIAL id PK with UNIQUE constraint on credential_id (follows repo convention) - entity: add @PrimaryGeneratedColumn id, switch bytea columns to use a new shared databaseBufferTransformer so reads land as Buffer - repository: replace post-INSERT SELECT with insertResult.generatedMaps, split create() into tryInsert/resolveConflict, fold Buffer helpers into the class via the transformer - attestation service: extract PasskeyAttestationError, error-id union, VerifyAttestationInput, VerifiedPasskey, and parse/compare helpers to sibling files; split the 79-line verify() into four sub-methods; replace the parsed-JSON cast with an isClientDataJSON type guard; TsDoc all public methods - passkey-record: extract WriteOutcomeStatus enum (mirrors MemberStatus) - routes: turn standalone helpers in passkeys.service.ts into private class methods; remove inline import in controller in favour of a top-level `import type` - DTOs: switch to zod v4 z.base64url() / z.url() built-ins - config: validate all PASSKEYS_* env vars in configuration.schema.ts; use faker for windowSeconds in test config; move lookup cache TTLs into config (PASSKEYS_LOOKUP_CACHE_HIT_*_MAX_AGE_SECONDS) - cache interceptor: read TTLs from configuration service; keep override (global interceptor sets no-cache and explicitly defers to inner ones)
2 participants
Summary
Implements the Passkey Coordinate Storage API — a CGW-hosted endpoint that maps a WebAuthn
credentialIdto its P-256 public-key coordinates(x, y)plus theP256.Verifierspackeduint176config, so a synced passkey on a second device can be used as a Safe signer.Two endpoints, one Postgres table, no auth coupling. Off by default behind
FF_PASSKEYS.Resolves: WA-2113, WA-2114, WA-2115, WA-2116, WA-2117, WA-2118, WA-2119
Changes
One commit per Linear ticket:
feat(passkeys): add passkey_coordinates migrationfeat(passkeys): scaffold PasskeysModule + FF_PASSKEYSfeat(passkeys): POST /v1/passkeys + attestation verification@simplewebauthn/server@13.3.0attestation verify, deterministic stateless challenge, 24 KiB route body cap, 500 ms verification timeout, idempotent first-write-wins viaON CONFLICT DO NOTHINGfeat(passkeys): GET /v1/passkeys/{credentialId}Cache-Control: public, max-age=86400, s-maxage=2592000, immutable+ETag;no-storeon missfeat(passkeys): per-IP rate limitsRateLimitGuardsubclasses; 429 carriesRetry-AfterandCache-Control: no-storefeat(passkeys): OpenAPI/Swagger error envelopePasskeyErrorResponseDTO with full errorId enum; documented cache headerstest(passkeys): HTTP-layer integration specNotable cross-cutting changes
CacheControlInterceptornow respects an existingCache-Controlheader set by inner interceptors /@Headerdecorators, instead of unconditionally overwriting tono-cache. Without this, the immutable lookup policy was silently downgraded.@simplewebauthn/server@13.3.0. Pinned at v13 (current latest); v11 was the API-stability boundary, v12/v13 are functionally compatible.Key invariants
x/yfield — coordinates are NEVER trusted from the client; they come from the verified attestation.expectedChallengeis a deterministic constant-time-comparing function, never() => true.verifiersisuint176(22 bytes) — packedP256.Verifiers, NOT an Ethereum address. No EIP-55 checksum.PASSKEY_CONFLICT/PASSKEY_CROSS_RP_CONFLICTcodes.rpId/origin/verifiersallowlists is empty whenFF_PASSKEYS=true.How to test it
Out of scope (per RFC author)
signer_address/ CREATE2 derivation — dropped from v1; an additive migration path is documented in the plan.Checklist