diff --git a/playwright/global-utils.ts b/playwright/global-utils.ts index 32589696..185efa96 100644 --- a/playwright/global-utils.ts +++ b/playwright/global-utils.ts @@ -175,7 +175,20 @@ function dbConfig(testInfo: TestInfo){ } /** - * All parameters passed in `env` need to be added to the docker-compose.yml + * All parameters passed in `env` need to be added to the docker-compose.yml. + * + * `resetDB=false` skips the explicit DB wipe, but it does NOT guarantee the + * DB survives across calls: `docker compose up -d Vaultwarden` recreates + * the container whenever any env var listed in + * `playwright/docker-compose.yml`'s `environment:` block + * differs from the previous run, and recreation drops the tmpfs-backed + * sqlite DB along with the in-process RSA signing key. So + * `resetDB=false` reliably preserves state only when consecutive + * `startVault` calls pass the SAME env. If you need to preserve user + * state across an env change, toggle the relevant settings via + * `POST /admin/config` instead of restarting (see the + * `Passkey enrolment is rejected when SSO_ONLY is on` describe in + * `playwright/tests/passkey.spec.ts` for the pattern). **/ export async function startVault(browser: Browser, testInfo: TestInfo, env = {}, resetDB: Boolean = true) { if( resetDB ){ diff --git a/playwright/tests/account_lifecycle.spec.ts b/playwright/tests/account_lifecycle.spec.ts index 5d25fcd6..b26edc3f 100644 --- a/playwright/tests/account_lifecycle.spec.ts +++ b/playwright/tests/account_lifecycle.spec.ts @@ -3,7 +3,7 @@ import { test, expect, type Page, type TestInfo } from '@playwright/test'; import * as utils from '../global-utils'; import { createAccount, logUser as logUserMP } from './setups/user'; import { logNewUser as ssoLogNewUser, logUser as logUserSSO } from './setups/sso'; -import { activateTOTP, disableTOTP, type TwoFactor } from './setups/2fa'; +import { activateTOTP, disableTOTP, resetTotpTimeStep, type TwoFactor } from './setups/2fa'; import { addVirtualAuthenticator, changeKdfIterations, @@ -87,7 +87,14 @@ test.afterAll('Teardown', async () => { // CDP sessions are bound to a specific Page; Playwright recycles the page // between tests, so drop the cached session/authenticator IDs each time // (the next `addVirtualAuthenticator` lazily re-establishes them). -test.beforeEach(() => resetVirtualAuthenticators()); +test.beforeEach(() => { + resetVirtualAuthenticators(); + // `submitTwoFactor`'s TOTP `last_used` cache is module-scoped and would + // otherwise leak across tests (and across projects under `workers: 1`) + // — drop it so each test's first TOTP submission isn't paying a 30s + // boundary-wait based on a different test's history. + resetTotpTimeStep(); +}); const MP = 'Master Password'; diff --git a/playwright/tests/passkey.spec.ts b/playwright/tests/passkey.spec.ts index a4d61eff..6c0bc3ff 100644 --- a/playwright/tests/passkey.spec.ts +++ b/playwright/tests/passkey.spec.ts @@ -2,6 +2,7 @@ import { test, expect, type TestInfo } from '@playwright/test'; import * as utils from '../global-utils'; import { createAccount } from './setups/user'; +import { resetTotpTimeStep } from './setups/2fa'; import { addVirtualAuthenticator, clickLoginWithPasskey, @@ -32,7 +33,12 @@ test.afterAll('Teardown', async () => { // CDP sessions are bound to a specific Page; Playwright recycles the page // between tests, so drop the cached session/authenticator IDs each time // (no-op for the request-only suites below; only the UI flows touch CDP). -test.beforeEach(() => resetVirtualAuthenticators()); +// Also drop `submitTwoFactor`'s TOTP `last_used` cache to avoid cross-test +// boundary-wait penalties — see `resetTotpTimeStep` doc. +test.beforeEach(() => { + resetVirtualAuthenticators(); + resetTotpTimeStep(); +}); // --------------------------------------------------------------------------- // Unauthenticated API surface — `GET /identity/accounts/webauthn/assertion-options` @@ -410,6 +416,7 @@ test.describe('Passkey grant is rejected when SSO_ONLY is on', () => { // vault with SSO_ENABLED + SSO_ONLY for this describe's tests, then // restart with default config in afterAll. test.beforeAll('Start vault with SSO_ONLY', async ({ browser }, testInfo) => { + test.skip(useExternalVault, 'cannot reconfigure an externally-running vault from the test process'); utils.stopVault(true); await utils.startVault(browser, testInfo, { SSO_ENABLED: 'true', @@ -421,6 +428,7 @@ test.describe('Passkey grant is rejected when SSO_ONLY is on', () => { }); test.afterAll('Restore default vault', async ({ browser }, testInfo) => { + if (useExternalVault) return; utils.stopVault(true); await utils.startVault(browser, testInfo, {}, false); }); @@ -452,7 +460,7 @@ test.describe('Passkey grant is rejected when SSO_ONLY is on', () => { test.describe('Passkey enrolment is rejected when SSO_ONLY is on', () => { // Defends the deny-by-default gate on the management-side endpoints // (`src/api/core/mod.rs` lines 308, 390, 459, 516 — guarded by - // `sso_enabled() && sso_only() && !sso_only_allow_passkey_unlock()`). + // `sso_enabled() && sso_only()`). // // The enrol endpoints are authenticated, so we need a Bearer token // to reach the gate; under `SSO_ONLY=true` fresh logins must go @@ -474,10 +482,17 @@ test.describe('Passkey enrolment is rejected when SSO_ONLY is on', () => { }; test.beforeAll('Provision user, sniff bearer, flip SSO_ONLY via /admin/config', async ({ browser, request }) => { + test.skip(useExternalVault, 'toggling SSO config via /admin would mutate the externally-running vault'); const ctx = await browser.newContext({ ignoreHTTPSErrors: true }); const page = await ctx.newPage(); const tokens: string[] = []; page.on('request', req => { + // Filter on /api/ to avoid latching onto a non-vault token + // (e.g. analytics, manifest fetches that some Bitwarden builds + // attach a different bearer to). The /api/ endpoints are the + // ones we'll exercise post-toggle, so capturing their bearer + // is what we need. + if (!req.url().includes('/api/')) return; const auth = req.headers()['authorization']; if (auth?.startsWith('Bearer ')) tokens.push(auth.slice('Bearer '.length)); }); @@ -500,9 +515,14 @@ test.describe('Passkey enrolment is rejected when SSO_ONLY is on', () => { }); test.afterAll('Toggle SSO back off', async ({ request }) => { + if (useExternalVault) return; await adminLogin(request); + // Blank the SSO authority/client too — without this the throwaway + // http://127.0.0.1:65535/realms/test placeholder lingers in + // config.json and contaminates any later test that toggles + // `sso_enabled=true` without rewriting the URL. await request.post('/admin/config', { - data: { sso_enabled: false, sso_only: false }, + data: { sso_enabled: false, sso_only: false, sso_authority: '', sso_client_id: '', sso_client_secret: '' }, failOnStatusCode: false, }); }); @@ -526,6 +546,7 @@ test.describe('Passkey login rejects forged unverified-email handles with the ge // CONFIG.mail_enabled() returns true. Restart vault with that config; the // new signup lands in DB with verified_at = NULL. test.beforeAll('Start vault with SIGNUPS_VERIFY', async ({ browser }, testInfo) => { + test.skip(useExternalVault, 'cannot reconfigure an externally-running vault from the test process'); utils.stopVault(true); await utils.startVault(browser, testInfo, { SIGNUPS_VERIFY: 'true', @@ -538,6 +559,7 @@ test.describe('Passkey login rejects forged unverified-email handles with the ge }); test.afterAll('Restore default vault', async ({ browser }, testInfo) => { + if (useExternalVault) return; utils.stopVault(true); await utils.startVault(browser, testInfo, {}, true); }); @@ -735,13 +757,22 @@ test.describe('Passkey UI flows', () => { await createAccount(test, page, user); // `useForEncryption: false` enrols the credential without the - // PRF-wrapped user-key blobs; /api/sync's `webAuthnPrfOptions` stays - // empty (already pinned by `account_lifecycle.spec.ts`'s wire-shape - // probe), so the lock-screen "Unlock with passkey" button must stay - // hidden even though the credential is registered. + // PRF-wrapped user-key blobs. The credential can still authenticate + // the user via the discoverable-login grant (login affordance), but + // /api/sync's `webAuthnPrfOptions` stays empty so the lock-screen + // "Unlock with passkey" button must stay hidden (unlock affordance). await enrollLoginPasskey(page, user.password, 'noprf-key', { useForEncryption: false }); - await lockVault(page, user.name); + // Log out + log back in via passkey to exercise the "login + // affordance present" half of the test's title. Without PRF-wrapped + // key blobs, authentication succeeds but vault decryption cannot, so + // the expected destination is the MP lock screen. + await utils.logout(test, page, user); + await clickLoginWithPasskey(page); + await expect(page).toHaveURL(/\/lock/, { timeout: 30_000 }); + + // Non-PRF credential is in the DB, but webAuthnPrfOptions is empty, + // so the lock screen must not surface the passkey-unlock button. await expectLockScreenButtons(page, false); }); diff --git a/playwright/tests/setups/2fa.ts b/playwright/tests/setups/2fa.ts index b1b9681a..7f60b5e8 100644 --- a/playwright/tests/setups/2fa.ts +++ b/playwright/tests/setups/2fa.ts @@ -79,6 +79,18 @@ async function ensure2FAProvider(page: Page, kind: TwoFactor['kind']) { */ let lastSubmittedTotpTimeStep: number | null = null; +/** + * Drop the module-scoped `last_used` cache. With `workers: 1` the variable + * persists across every spec in a single Playwright invocation, so when + * project A's lifecycle test consumes a time-step and project B then runs + * its first TOTP (different DB, different secret, different time-step) the + * stale value would force a 30s sleep before the otherwise-fresh code. Call + * this from a `test.beforeEach` in any spec that submits TOTP. + */ +export function resetTotpTimeStep() { + lastSubmittedTotpTimeStep = null; +} + export async function submitTwoFactor(test: Test, page: Page, twoFactor: TwoFactor): Promise { await test.step(`Submit 2FA (${twoFactor.kind})`, async () => { await expect(page.getByRole('heading', { name: 'Verify your Identity' })).toBeVisible(); diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 6f9ea483..b494d07c 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -825,6 +825,13 @@ fn validate_passkey_rotation_data( .map(|c| &c.uuid) .collect::>(); let provided_passkey_ids = passkey_unlock_data.iter().map(|p| &p.id).collect::>(); + // Reject duplicate ids before they reach the update loop: the HashSet + // de-dups silently, so a payload like `[{id: A, key: K1}, {id: A, key: K2}]` + // would pass the superset check above and the loop would apply both + // writes in order, with the second silently overwriting the first. + if provided_passkey_ids.len() != passkey_unlock_data.len() { + err!("Duplicate passkey ids in rotation payload") + } if !provided_passkey_ids.is_superset(&existing_prf_credential_ids) { err!("All passkeys with encryption enabled must be included in the rotation") } @@ -980,6 +987,24 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: } } + // Re-check the PRF credential set immediately before committing the new + // account key. A concurrent enrol (another session/tab) can register a + // PRF passkey between the validation re-check above and now — that + // credential is missing from `passkey_unlock_data`, so its + // `encrypted_user_key` is still wrapped under the OLD account key. + // Erroring out here forces the caller to retry the rotation; without + // this guard, the new akey commits and the orphan credential can never + // unlock the vault again. + let post_loop_prf_ids = WebAuthnCredential::find_by_user(user_id, &conn) + .await + .into_iter() + .filter(WebAuthnCredential::has_prf_keyset) + .map(|c| c.uuid) + .collect::>(); + if post_loop_prf_ids != snapshot_prf_ids { + err!("Passkey credentials changed during key rotation; please retry") + } + // Update user data let mut user = headers.user; @@ -1853,4 +1878,17 @@ mod tests { assert!(validate_passkey_rotation_data(&data, &credentials).is_err()); } + + /// A duplicate id in `passkey_unlock_data` collapses into a single + /// `HashSet` entry for the superset check, but the rewrap loop in + /// `post_rotatekey` iterates the original `Vec` and would apply BOTH + /// updates, silently letting the second blob overwrite the first. + /// The validator must reject duplicates upfront. + #[test] + fn passkey_rotation_rejects_duplicate_ids() { + let credentials = vec![webauthn_credential("prf", true, true)]; + let data = vec![passkey_unlock_data("prf"), passkey_unlock_data("prf")]; + + assert!(validate_passkey_rotation_data(&data, &credentials).is_err()); + } } diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index bfe198a2..d41f4a96 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -257,22 +257,28 @@ struct WebAuthnPasskeyAssertionChallenge { } fn passkey_registration_challenge_state(data: &str, token: Option<&str>) -> ApiResult { - if let Ok(saved) = serde_json::from_str::(data) { - if token != Some(saved.token.as_str()) { - err!("Invalid registration challenge. Please try again.") - } - Ok(saved.state) - } else { - if token.is_some() { - err!("Invalid registration challenge. Please try again.") - } - Ok(serde_json::from_str::(data)?) + // Persisted challenge rows are always the `{token, state}` wrapper — + // nothing in the current code path writes the bare `PasskeyRegistration` + // shape. Reject a row that doesn't deserialise (corrupted, stale schema) + // with the same generic message we use for token mismatch, rather than + // falling through to an un-tokened legacy path. + let Ok(saved) = serde_json::from_str::(data) else { + err!("Invalid registration challenge. Please try again.") + }; + if !token.is_some_and(|t| crypto::ct_eq(t, &saved.token)) { + err!("Invalid registration challenge. Please try again.") } + Ok(saved.state) } fn passkey_assertion_challenge_state(data: &str, token: &str) -> ApiResult { - let saved: WebAuthnPasskeyAssertionChallenge = serde_json::from_str(data)?; - if token != saved.token.as_str() { + // Same shape contract as `passkey_registration_challenge_state` above — + // reject undecodable rows with the generic message rather than leaking + // the underlying serde error. + let Ok(saved) = serde_json::from_str::(data) else { + err!("Invalid assertion challenge. Please try again.") + }; + if !crypto::ct_eq(token, &saved.token) { err!("Invalid assertion challenge. Please try again.") } Ok(saved.state) @@ -321,19 +327,25 @@ async fn post_api_webauthn_attestation_options( let (mut challenge, state) = WEBAUTHN.start_passkey_registration(user_uuid, &user.email, user.display_name(), Some(existing_cred_ids))?; - // For passkey login, we need discoverable credentials (resident keys) - // and require user verification. - // start_passkey_registration() defaults to require_resident_key=false, but passkey login - // requires the credential to be discoverable (resident) so the authenticator can find it - // without the server providing allowCredentials. + // Tell the client we want a discoverable (resident) credential with UV. + // `start_passkey_registration` already pins UV=Required in the stored + // `state`; resident-key is NOT enforced server-side by webauthn-rs's + // `register_credential` (the corresponding field on the state is + // destructured as unused), so this mutation is a client-side hint only: + // it controls the challenge JSON shipped to the browser, not what the + // server validates against. A non-resident credential that completes + // the ceremony would still be accepted but later fail discoverable- + // login, so honest clients comply and tampering clients shoot themselves. if let Some(asc) = challenge.public_key.authenticator_selection.as_mut() { asc.user_verification = UserVerificationPolicy::Required; asc.require_resident_key = true; asc.resident_key = Some(webauthn_rs_proto::ResidentKeyRequirement::Required); } - // Drop any abandoned challenge from a previous, unfinished registration attempt - // so these rows cannot accumulate in the database. + // Drop any abandoned challenge from a previous, unfinished registration + // attempt so these rows cannot accumulate. `TwoFactor::save` below uses + // `replace_into` keyed on `uuid` (not on `(user_uuid, atype)`), so without + // this delete, sqlite/mysql would insert a sibling row each retry. if let Some(tf) = TwoFactor::find_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyRegisterChallenge as i32, &conn) .await @@ -572,6 +584,13 @@ async fn put_api_webauthn( Ok(Status::Ok) } +// NOTE: unlike the four enrol/update entry points above, this delete handler +// is intentionally NOT gated on `sso_enabled() && sso_only()`. SSO_ONLY restricts +// what a user can ADD to their own account — deletion only narrows capability +// (revoke a credential, never grants one), so leaving it accessible under +// SSO_ONLY lets an SSO-authenticated user clean up credentials that were +// enrolled when the policy was different. The session is still +// SSO-authenticated by the time this handler runs, so there's no auth bypass. #[post("/webauthn//delete", data = "")] async fn post_api_webauthn_delete( data: Json, @@ -659,12 +678,20 @@ mod tests { assert!(passkey_registration_challenge_state(&data, None).is_err()); } + /// `passkey_registration_challenge_state` has no legacy unwrapped fallback — + /// the only writer is the attestation-options endpoint, and it always + /// persists the `{token, state}` wrapper. A bare `PasskeyRegistration` + /// blob in `twofactor.data` (corrupted row, hand-crafted attack) must + /// be rejected regardless of whether a token is sent — accepting it + /// without a token would let an attacker bypass the token-binding + /// check by writing the wrong shape. #[test] - fn legacy_registration_challenge_rejects_finish_token() { + fn registration_challenge_rejects_unwrapped_legacy_state() { let data = serde_json::to_string(®istration_state()).unwrap(); - assert!(passkey_registration_challenge_state(&data, None).is_ok()); - assert!(passkey_registration_challenge_state(&data, Some("token")).is_err()); + assert!(passkey_registration_challenge_state(&data, None).is_err()); + assert!(passkey_registration_challenge_state(&data, Some("any-token")).is_err()); + assert!(passkey_registration_challenge_state(&data, Some("")).is_err()); } #[test] diff --git a/src/api/identity.rs b/src/api/identity.rs index a8332719..0cbb16b1 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -1166,14 +1166,22 @@ fn passkey_credential_id(passkey: &Passkey) -> ApiResult { } fn passkey_transports(passkey: &Passkey) -> Vec { - serde_json::to_value(passkey) - .ok() - .and_then(|value| { - value - .pointer("/cred/transports") - .and_then(Value::as_array) - .map(|transports| transports.iter().filter_map(Value::as_str).map(str::to_owned).collect::>()) - }) + // Serializing a `webauthn_rs::Passkey` should never fail in practice + // (it's a derive(Serialize) on a well-typed struct); if it does, log + // and fall through to an empty list rather than silently masking it + // — clients can't distinguish "authenticator reported no transports" + // from "we failed to encode the passkey" otherwise. + let value = match serde_json::to_value(passkey) { + Ok(v) => v, + Err(e) => { + error!("Failed to serialise passkey for transport extraction: {e:#?}"); + return Vec::new(); + } + }; + value + .pointer("/cred/transports") + .and_then(Value::as_array) + .map(|transports| transports.iter().filter_map(Value::as_str).map(str::to_owned).collect::>()) .unwrap_or_default() } diff --git a/src/db/models/two_factor.rs b/src/db/models/two_factor.rs index 82da4e48..1d8f311a 100644 --- a/src/db/models/two_factor.rs +++ b/src/db/models/two_factor.rs @@ -159,7 +159,7 @@ impl TwoFactor { pub async fn take_by_user_and_type(user_uuid: &UserId, atype: i32, conn: &DbConn) -> Option { let user_uuid = user_uuid.clone(); conn.run(move |conn| { - conn.transaction::, diesel::result::Error, _>(|conn| { + let result = conn.transaction::, diesel::result::Error, _>(|conn| { let tf = twofactor::table .filter(twofactor::user_uuid.eq(&user_uuid)) .filter(twofactor::atype.eq(atype)) @@ -171,8 +171,18 @@ impl TwoFactor { let deleted = diesel::delete(twofactor::table.filter(twofactor::uuid.eq(&existing.uuid))).execute(conn)?; Ok(tf.filter(|_| deleted == 1)) - }) - .unwrap_or(None) + }); + match result { + Ok(opt) => opt, + Err(e) => { + // Surface the underlying error so DB degradation + // (deadlock, conn drop, lock timeout) is operator- + // visible rather than indistinguishable from a normal + // "row consumed by a concurrent caller" result. + error!("TwoFactor::take_by_user_and_type failed for user {user_uuid} atype {atype}: {e:#?}"); + None + } + } }) .await } diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index 4e79b064..9f8302cb 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -235,7 +235,7 @@ impl WebAuthnLoginChallenge { // ensures the SELECT+DELETE pair rolls back atomically on a DB // error, leaving the challenge intact rather than silently // consuming it. - let taken = conn + let taken = match conn .transaction::, diesel::result::Error, _>(|conn| { let challenge = web_authn_login_challenges::table .filter(web_authn_login_challenges::id.eq(id)) @@ -246,8 +246,17 @@ impl WebAuthnLoginChallenge { ) .execute(conn)?; Ok(challenge.filter(|_| deleted == 1)) - }) - .unwrap_or(None); + }) { + Ok(opt) => opt, + Err(e) => { + // Surface the underlying error so a degrading DB + // (deadlock, conn drop, lock timeout) is operator- + // visible instead of masquerading as a stale-token + // rejection at the caller. + error!("WebAuthnLoginChallenge::take failed for id {id}: {e:#?}"); + None + } + }; let cutoff = Utc::now().naive_utc() - TimeDelta::seconds(WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS); taken.filter(|c| c.created_at >= cutoff) diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 8b35c549..81c7faa8 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -54,6 +54,17 @@ app-root ng-component > form > div:nth-child(1) > div > button[buttontype="secon } {{/if}} +/* Hide the `Log in with passkey` button on the login page under SSO_ONLY. + The server gate at `src/api/identity.rs` rejects the unauthenticated + `/accounts/webauthn/assertion-options` request the button triggers, + so clicking it would dead-end with "SSO sign-in is required". Keep the + button visible whenever passkey login is reachable (default + SSO modes). */ +{{#if (and sso_enabled sso_only)}} +.vw-passkey-login { + @extend %vw-hide; +} +{{/if}} + /* Hide the or text followed by the two buttons hidden above */ {{#if (webver ">=2025.5.1")}} {{#if (or (not sso_enabled) sso_only)}}