fix(auth): support RS256 algorithm for passkeys#893
Conversation
🦋 Changeset detectedLatest commit: 15bfbd9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
/review |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for tackling this — Windows Hello / TPM-backed authenticators being locked out of setup is a real, painful bug, and the polymorphic verification design is the right shape. However, this PR has two blocking issues that need to be fixed before it can merge.
1. The migration is not forward-only (blocking)
008_auth.ts was released in v0.x and has already been applied on every existing install. Modifying that file in place adds the column for fresh databases only — Kysely sees 008_auth in _emdash_migrations and skips it on existing installs. After this PR, an existing install will fail to register any new passkey: the kysely adapter writes algorithm: <number> into a column that doesn't exist on the upgraded DB. That's exactly the Windows-Hello user this PR is meant to unblock.
Fix: revert packages/core/src/database/migrations/008_auth.ts to its pre-PR state, and add a brand-new migration packages/core/src/database/migrations/036_credential_algorithm.ts:
import { type Kysely } from "kysely";
export async function up(db: Kysely<unknown>): Promise<void> {
await db.schema
.alterTable("credentials")
.addColumn("algorithm", "integer", (col) => col.notNull().defaultTo(-7))
.execute();
}
export async function down(db: Kysely<unknown>): Promise<void> {
await db.schema.alterTable("credentials").dropColumn("algorithm").execute();
}Register it in packages/core/src/database/migrations/runner.ts (static import + entry in MIGRATIONS). The DEFAULT -7 makes existing rows valid (they're all ES256 today, matching the previous hardcoded behavior). See AGENTS.md → Rules: "Database migrations are forward-only — never write one that leaves existing content inaccessible." The inline CREATE TABLE in packages/auth/src/adapters/kysely.ts (used by standalone consumers of @emdash-cms/auth) is a separate path — that change is fine to keep.
2. Workspace typecheck almost certainly does not pass (blocking)
algorithm is now a required field on NewCredential and VerifiedRegistration, but three call sites outside packages/auth were not updated and will fail compilation:
packages/core/tests/unit/auth/passkey-management.test.ts — two adapter.createCredential({...}) literals omit algorithm:
- Lines 34–43 (in the
createTestCredentialhelper) - Lines 239–248 (in the
"should preserve all credential properties"test)
Fix: add algorithm: -7, to both.
packages/core/tests/integration/astro/setup-admin-nonce-success.test.ts — the verifyRegistrationResponse mock at lines 28–35 returns a VerifiedRegistration shape without algorithm. Add algorithm: -7, to the returned object.
The PR description says pnpm typecheck passes — please re-run it from the repo root (not just inside packages/auth). The Credential / NewCredential / VerifiedRegistration types are imported into those test files, so a workspace-level typecheck has to surface these.
3. Open question answered
Should we explicitly limit the supported RSA algorithms or is the current RSASSA-PKCS1-v1_5 implementation sufficient as a baseline?
Limiting to RSASSA-PKCS1-v1_5 + SHA-256 is exactly right — that's what COSE alg -257 (RS256) means by definition, and it's what Windows Hello / TPM authenticators emit. Don't expand scope here. If a future authenticator uses RS384/RS512/PS256 you'd add separate cases, but those are vanishingly rare in WebAuthn deployments today. The current pubKeyCredParams advertises only [ES256, RS256], which keeps the verifier and the announced algorithm list in sync — good.
Smaller notes
A few non-blocking comments inline.
| .addColumn("id", "text", (col) => col.primaryKey()) // Base64url credential ID | ||
| .addColumn("user_id", "text", (col) => col.notNull()) | ||
| .addColumn("public_key", binaryType(db), (col) => col.notNull()) // COSE public key | ||
| .addColumn("algorithm", "integer", (col) => col.notNull().defaultTo(-7)) // COSE algorithm (default ES256) |
There was a problem hiding this comment.
Blocking — forward-only migration violation. See review body for full explanation and the suggested replacement migration.
Short version: this file was released in v0.x. Existing installs already have an _emdash_migrations row for 008_auth, so this addColumn will never run on them — only on brand-new databases. Result: the very Windows Hello users this PR targets will hit a runtime SQL error when they try to register, because the kysely adapter writes algorithm into a column that doesn't exist on the upgraded DB.
Revert this file to its pre-PR state and add a new 036_credential_algorithm.ts migration with ALTER TABLE credentials ADD COLUMN algorithm INTEGER NOT NULL DEFAULT -7, registered in runner.ts.
|
|
||
| if (algorithm === coseAlgorithmES256) { | ||
| // Verify it's EC2 key type | ||
| // Verify EC2 key type for ES256 |
There was a problem hiding this comment.
Stale comment in this block at line 158 (just above the hunk): // Currently only supporting ES256 (ECDSA with P-256) is no longer true. Worth refreshing to something like // Supports ES256 (ECDSA P-256, stored as SEC1) and RS256 (RSA, stored as PKIX).
| id: row.id, | ||
| userId: row.user_id, | ||
| publicKey: row.public_key, | ||
| algorithm: row.algorithm ?? -7, // Default to ES256 (-7) for backward compatibility with legacy rows |
There was a problem hiding this comment.
The ?? -7 fallback is sensible defensive coding for the inline-schema path on line 676 (where the CREATE TABLE has DEFAULT -7). Once a proper ALTER TABLE migration is added on the EmDash core path with the same default, all rows will have a non-null algorithm and this fallback becomes dead code there — but it's still useful for standalone @emdash-cms/auth consumers who use the inline schema. Keep it; just consider broadening the comment so it doesn't suggest "legacy rows" is the only case.
No change required.
|
Review submitted successfully as CHANGES_REQUESTED with 4 inline comments. The two blocking issues are clearly called out:
Plus the open question is answered (RSASSA-PKCS1-v1_5 is the right scope) and two smaller nits (stale comment, error code naming). |
|
Thanks for the detailed review @ask-bonk! I've addressed both blocking issues and the smaller nits:
Ready for another look! |
|
@ascorbic Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
j-liszt
left a comment
j-liszt
left a comment
There was a problem hiding this comment.
All requested changes have been addressed:
- Created a new forward-only migration (036) for the algorithm column.
- Fixed global type safety issues in core tests.
- Made the migration idempotent to ensure it passes integration tests.
- Updated error naming and stale comments as requested.
The code now passes both typechecks and integration tests locally. Ready for another review!
|
/review |
There was a problem hiding this comment.
Thanks for iterating on the previous review — the migration revert + new 036_credential_algorithm.ts, the workspace-level type fixes, and the renamed error code all look right. The actual signature-verification logic is correctly wired (oslojs's verifyRSASSAPKCS1v15Signature with sha256ObjectIdentifier is exactly the right call for COSE alg -257/RS256, and PKIX is the right encoding for stored RSA public keys). Backwards compat for legacy ES256 rows via ?? -7 is reasonable.
A few things to address before merge — one is likely a real Postgres-build break, the rest are smaller:
Likely Postgres build break
packages/core/src/database/migrations/036_credential_algorithm.ts uses PRAGMA table_info(credentials) directly. That's SQLite-only. Core supports Postgres (see 008_auth.ts using binaryType(db)/currentTimestamp(db) from dialect-helpers.ts, and 019_i18n.ts/035_bounded_404_log.ts taking the same care). There's already a dialect-aware columnExists(db, table, column) helper in packages/core/src/database/dialect-helpers.ts that's used by 035_bounded_404_log.ts for exactly this pattern — please switch to it. setupTestPostgresDatabase() in the integration suite (when run with PG_CONNECTION_STRING) will fail otherwise.
Smaller things
- No automated test for the new RS256 signature path. The whole point of this PR is that ES256-only verification was silently locking out Windows Hello users; if the RSA branch ever regresses (e.g. someone refactors the encoding step) we'd hit the same bug class with no warning. A unit test in
authenticate.test.tsthat generates an RSA-2048 key withnode:crypto, encodes the public key as PKIX, signs an assertion, and asserts a successful verification would be straightforward and would mirror the existing ES256 helper. - The migrations test name and assertions in
packages/core/tests/integration/database/migrations.test.tsare now slightly out of sync with what the test actually does — see inline. - Throwing a
PasskeyAuthenticationErrorfrom the registration path is mildly off-genre — see inline.
The inline AUTH_TABLES_SQL path (CREATE TABLE IF NOT EXISTS credentials …) will not retroactively add the algorithm column for standalone @emdash-cms/auth consumers whose credentials table predates this change. That's a pre-existing limitation of the inline-schema pattern, not something this PR introduced — but it does mean any standalone consumer needs to apply ALTER TABLE credentials ADD COLUMN algorithm INTEGER NOT NULL DEFAULT -7 themselves. Worth a one-line note in the @emdash-cms/auth changeset entry so it's not a surprise.
|
|
||
| export async function up(db: Kysely<unknown>): Promise<void> { | ||
| const tableInfo = await sql<{ name: string }>`PRAGMA table_info(credentials)`.execute(db); | ||
| const columnExists = tableInfo.rows.some((col) => col.name === "algorithm"); |
There was a problem hiding this comment.
Postgres incompatibility. PRAGMA table_info is SQLite-only — this migration will fail under Postgres (setupTestPostgresDatabase() / PG_CONNECTION_STRING test runs, and any user running core on PG). The repo already has a dialect-aware helper for exactly this:
| const columnExists = tableInfo.rows.some((col) => col.name === "algorithm"); | |
| import { type Kysely } from "kysely"; | |
| import { columnExists } from "../dialect-helpers.js"; | |
| export async function up(db: Kysely<unknown>): Promise<void> { | |
| if (await columnExists(db, "credentials", "algorithm")) { | |
| return; | |
| } |
(Then drop the sql import and the tableInfo/columnExists block, and keep the existing addColumn call.) See packages/core/src/database/migrations/035_bounded_404_log.ts for the same pattern in use.
| } | ||
|
|
||
| export async function down(db: Kysely<unknown>): Promise<void> { | ||
| await db.schema.alterTable("credentials").dropColumn("algorithm").execute(); |
There was a problem hiding this comment.
Nit: the down won't be reachable on Postgres in its current form either (the up would have errored earlier), but if up is fixed via columnExists, this dropColumn is fine on both dialects (Kysely emits portable ALTER TABLE … DROP COLUMN). Consider gating with columnExists here too for symmetry — running down twice would otherwise throw.
Non-blocking; down is rarely run in practice.
| .where("name", "in", [ | ||
| "034_published_at_index", | ||
| "035_bounded_404_log", | ||
| "036_credential_algorithm", |
There was a problem hiding this comment.
Two things:
-
The test title (line 110) still says "034 and 035" but the body now also deletes
036_credential_algorithm. A future reader greping for "036" in tests won't find this regression coverage. Either rename the test (e.g."should re-run migrations 034, 035, and 036 when schema changes were partially applied") or split 036 into its own test. -
Since 036 is deleted from
_emdash_migrationshere, please also assert it was re-applied — otherwise this test wouldn't catch a regression where the new migration's idempotency check never re-runs:
| "036_credential_algorithm", | |
| "036_credential_algorithm", | |
| ]) | |
| .execute(); | |
| const { applied } = await runMigrations(db); | |
| expect(applied).toContain("034_published_at_index"); | |
| expect(applied).toContain("035_bounded_404_log"); | |
| expect(applied).toContain("036_credential_algorithm"); |
| throw new PasskeyAuthenticationError( | ||
| "unsupported_algorithm", | ||
| "Unsupported credential algorithm", | ||
| ); |
There was a problem hiding this comment.
Mildly off-genre: every other failure mode in verifyRegistrationResponse throws a plain Error (lines 105, 112, 119, 127, 143, 148, 153, 166, 170, 181), but this branch reaches across to ./authenticate.js to throw a PasskeyAuthenticationError. Pick one:
- Drop back to
throw new Error("Unsupported algorithm: " + algorithm);for consistency with the rest of this function, or - Introduce a
PasskeyRegistrationErrorclass with its own code union and migrate the other plainErrors to it (separate PR).
The current state is the worst of both worlds — typed for one branch, untyped for the others, with a class name (...AuthenticationError) that doesn't match the call site. Non-blocking, and the route handler at packages/core/src/astro/routes/api/auth/passkey/register/verify.ts doesn't instanceof it today, so nothing breaks — but it'll cost cleanup later.
| "unsupported_algorithm", | ||
| "Unsupported credential algorithm", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Non-blocking, but worth noting there's no automated test for either new branch of this if/else if/else. The whole point of this PR is that the missing RSA branch silently locked out Windows Hello users; a unit test that generates an RSA-2048 key with node:crypto, encodes via RSAPublicKey(...).encodePKIX(), signs an assertion, and asserts a successful verification would mirror the existing createValidAssertion() helper in authenticate.test.ts and prevent the same class of regression. The unsupported_algorithm branch is also worth a one-line test (set credential.algorithm = 0 and expect the typed error).
Manual verification on Windows is great but won't catch a future refactor.
|
Review posted successfully (id 4215121445). All five inline comments accepted, plus the summary body. Summary of the review:
Plus a non-blocking heads-up in the summary that the inline |
…n test repair, and type safety
…ssion tests for RS256, and polished error handling
j-liszt
left a comment
j-liszt
left a comment
There was a problem hiding this comment.
Summary of changes addressing the latest review:
- Database & Migrations: Switched to the dialect-aware
columnExistshelper in036_credential_algorithm.tsto ensure Postgres compatibility. Added idempotency gates to bothupanddownfunctions for symmetry. - Testing:
- Added automated regression tests for the RS256 path in
authenticate.test.tsusing real RSA-2048 key generation and signature verification. - Added a test case for the
unsupported_algorithmerror branch. - Renamed and updated the migration integration test to correctly assert the re-application of migration
036.
- Added automated regression tests for the RS256 path in
- Error Handling & Consistency:
- Replaced
PasskeyAuthenticationErrorwith a plainErrorin the registration path (register.ts) for consistency with the existing codebase. - Improved error messages in both registration and authentication to include the dynamic algorithm ID for better debugging.
- Replaced
- Documentation: Added a note to the
@emdash-cms/authchangeset regarding the mandatory manual schema update (ALTER TABLE) for standalone consumers.
Verified with pnpm test and typecheck. The PR is now ready for final review.
|
/review |
There was a problem hiding this comment.
Third pass: the dialect-aware migration + new tests + error-class consistency from the previous round all landed correctly. The motivating bug (ES256-only verification locking out Windows Hello / TPM-backed authenticators) is genuinely fixed, and the polymorphic verification path is wired up correctly end-to-end. decodePKIXRSAPublicKey + verifyRSASSAPKCS1v15Signature + sha256ObjectIdentifier is exactly the right call for COSE alg -257/RS256, and PKIX encoding from the COSE RSA key matches what the verifier decodes (the new authenticate-side test exercises this round-trip).
A few smaller things remain — none are blocking:
Stale type in packages/core/src/database/types.ts
CredentialTable (around line 72) still doesn't have an algorithm column. Today nothing in packages/core queries credentials directly through this typed Kysely schema — @emdash-cms/auth owns those reads/writes via its own CredentialTable interface inside adapters/kysely.ts — so this isn't a runtime bug. But the type now lies about the schema, and the // COSE public key comment is also misleading (the stored public key is no longer raw COSE; it's SEC1 for ECDSA or PKIX for RSA, per the encoding done in register.ts). Cheap to keep in sync — add algorithm: number; and update the comment on public_key.
No test coverage for the registration RSA branch
The new createValidRS256Assertion() in authenticate.test.ts hand-rolls the PKIX from node:crypto, which exercises the authenticate side but not verifyRegistrationResponse's new RSA branch — see inline.
Nit: changeset wording for standalone consumers
The note tells standalone @emdash-cms/auth consumers to run ALTER TABLE credentials ADD COLUMN algorithm INTEGER NOT NULL DEFAULT -7 manually. Worth mentioning that the DEFAULT -7 is what makes existing rows continue to work (they're all ES256 today by definition of the previous hardcoded behavior), so they don't need any data backfill — only the schema change.
No other findings.
| } | ||
| const cosePublicKey = credential.publicKey.rsa(); | ||
| // Encode as PKIX format for storage | ||
| encodedPublicKey = new RSAPublicKey(cosePublicKey.n, cosePublicKey.e).encodePKIX(); |
There was a problem hiding this comment.
No automated coverage for this branch. The new createValidRS256Assertion() in authenticate.test.ts hand-rolls the PKIX encoding via node:crypto.publicKey.export({ type: 'spki' }) — it doesn't run a real or fake COSE-RSA attestation through verifyRegistrationResponse, so the path that calls credential.publicKey.rsa() and new RSAPublicKey(n, e).encodePKIX() is untested. If cosePublicKey.rsa() ever returns a different shape, or encodePKIX() is replaced with encodePKCS1() in a refactor, the bug class this PR exists to fix re-emerges with no test failure.
A test that synthesizes a CBOR attestation object with an RSA COSE key (or mocks the parsed authenticatorData.credential.publicKey into verifyRegistrationResponse) and asserts the resulting verified.publicKey round-trips through decodePKIXRSAPublicKey would close the loop. Non-blocking but cheap insurance for the motivating bug.
| id: row.id, | ||
| userId: row.user_id, | ||
| publicKey: row.public_key, | ||
| algorithm: row.algorithm ?? -7, // Default to ES256 (-7) for compatibility with credentials created without an explicit algorithm |
There was a problem hiding this comment.
Nit: now that migration 036 adds the column with NOT NULL DEFAULT -7, the ?? -7 fallback is dead code on the EmDash core path — every row will have a non-null algorithm. It's still meaningful for standalone @emdash-cms/auth consumers who used the old inline CREATE TABLE (without the algorithm column) and haven't yet run the manual ALTER TABLE from the changeset note... but in that case Kysely would error on the SELECT before this fallback runs (the column doesn't exist in the DB), so it doesn't actually help. Consider either dropping the fallback or rewording the comment so it's clear it's purely defensive against rows written by an older codepath, not a recovery mechanism for unmigrated DBs.
| if (!(await columnExists(db, "credentials", "algorithm"))) { | ||
| await db.schema | ||
| .alterTable("credentials") | ||
| .addColumn("algorithm", "integer", (col) => col.notNull().defaultTo(-7)) |
There was a problem hiding this comment.
Heads-up — not a bug, just a note for the next person: defaultTo(-7) on a notNull integer column works on both SQLite and Postgres because Kysely emits the integer literal as-is. If a future migration ever needs a non-trivial default expression for an integer column, it'll need to go through sql\...`` to render correctly on both dialects. This one is fine.
|
Review submitted successfully (id 4216003420) with all three inline comments accepted. Summary of what I posted:
|
- Update CredentialTable type in core to include algorithm column - Add robust RSA-2048 automated test for registration flow using BigInt representations - Remove dead fallback code in Kysely adapter - Clarify schema backwards compatibility in changeset
…t/emdash into fix/rs256-passkey-support
j-liszt
left a comment
j-liszt
left a comment
There was a problem hiding this comment.
Thanks for the thorough review, I've addressed all the remaining non-blocking items from the third pass. Here is what was updated:
1. Test coverage for the registration RSA branch
Added a robust test in register.test.ts to ensure the verifyRegistrationResponse RSA branch is fully covered. We mocked the parsed authenticatorData.credential.publicKey to inject native BigInt representations of an RSA key and successfully asserted that the resulting verified.publicKey round-trips correctly through decodePKIXRSAPublicKey(). This locks down encodePKIX() and prevents any future regressions.
2. Stale type in packages/core/src/database/types.ts
Added the algorithm: number; property to the CredentialTable interface and updated the // COSE public key comment to accurately reflect that the stored value is now SEC1 for ECDSA or PKIX for RSA. Types and schemas are fully in sync again.
3. Dead fallback code in Kysely adapter
Dropped the ?? -7 fallback in packages/auth/src/adapters/kysely.ts entirely. Since the migration forces NOT NULL DEFAULT -7, and standalone consumers missing the column would crash on the SELECT phase anyway, the fallback was indeed dead code.
4. Changeset wording for standalone consumers
Updated the .changeset wording to explicitly mention that the DEFAULT -7 constraint automatically handles backwards compatibility for all existing rows (which are ES256 by definition), meaning standalone consumers only need the schema change and do not require any manual data backfilling.
Ready for the review! Let me know if you need anything else.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
Thanks for handling the detailed reviews! ANything security-related needs a very detailed review, so thanks for your patience. Right now the only thing remaining is to deal with the conflict in migration files – another PR has merged that used the same migraiton number, so it just needs some renaming and renumbering and then should be good to go. |
…x integration tests
…t/emdash into fix/rs256-passkey-support
2 participants
What does this PR do?
This PR implements full support for multiple cryptographic algorithms in Passkey authentication, moving away from the previous ES256-only implementation.
The Problem:
While many mobile devices (iOS/Android) default to ES256 (ECDSA), many desktop environments and hardware security keys (notably Windows Hello and certain TPM-backed authenticators) utilize RS256 (RSA).
Since EmDash was hardcoded to only support ECDSA, users on these platforms were completely blocked from completing the initial Setup Wizard. During the admin account creation step, the registration would fail with an "Unsupported algorithm" error, making it impossible for Windows users to initialize the CMS. Additionally, any user who managed to register with an RSA key (through a different flow) would find themselves unable to log in, receiving "Invalid Signature" (401) errors.
The Solution:
To align with FIDO2/WebAuthn best practices for Relying Parties (RP), this PR introduces a polymorphic verification layer:
algorithmcolumn to thecredentialstable to persist the COSE algorithm identifier.Closes #421
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runpnpm locale:extracthas been run (if applicable)AI-generated code disclosure
Test output
Updated
packages/auth/src/passkey/authenticate.test.tsto include thealgorithmproperty in credential mocks. Verified thatpnpm typecheckpasses globally across the workspace. Manual verification performed by initializing a fresh EmDash instance on a Windows environment using Windows Hello (RS256), which previously failed but now completes successfully.Open question for reviewers
Should we explicitly limit the supported RSA algorithms or is the current RSASSA-PKCS1-v1_5 implementation sufficient as a baseline for wider platform compatibility?