12968 sso pairing - #113
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
There are confirmed runtime-breaking bugs (SQL placeholder mismatch; incorrect user lookup condition) and a high-risk authentication bypass block that should be removed or properly gated before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR introduces support for passwordless/SSO pairing flows by adding new API2 endpoints and database fields to track passwordless unlock eligibility and to manage SSO-enrolled devices pending password-backup approval.
Changes:
- Add passwordless-unlock detection (bank/user/pattern-based) and expose it via a new API endpoint.
- Add SSO pairing endpoints to authorize devices via OpenID, reject pending devices, and (re)send password-backup public keys.
- Extend vault and password-backup flows to surface pending SSO devices and handle concurrent approval/rejection outcomes; add DB migrations for new columns.
File summaries
| File | Description |
|---|---|
| src/server.ts | Registers new API2 routes for SSO pairing and passwordless unlock detection. |
| src/api2/routes/passwordReset/getPasswordBackup.ts | Adjusts password-reset bookkeeping for passwordless SSO unlock cases. |
| src/api2/routes/passwordReset/backupPassword.ts | Clears pending public key on successful backup and returns unapplied device IDs. |
| src/api2/routes/deviceAuthorization/sendPasswordBackupPublicKey.ts | New route to store a device’s password-backup public key post-authorization. |
| src/api2/routes/deviceAuthorization/requestDeviceAccess.ts | Removes OpenID enrollment path; keeps this route email-enrollment only. |
| src/api2/routes/deviceAuthorization/rejectSsoDevice.ts | New route to reject/revoke an authorized-but-unpaired device. |
| src/api2/routes/deviceAuthorization/authorizeDeviceWithOpenId.ts | New route to authorize devices using OpenID session (SSO), including admin-check gating. |
| src/api2/routes/data/getVaultData.ts | Adds pending SSO devices list to vault-data response. |
| src/api2/routes/bank/getBankConfig.ts | Removes is_sso_v2 from SSO config payload and tightens typing/returns. |
| src/api2/routes/authentication/usesPasswordlessUnlock.ts | New endpoint + helper to determine passwordless unlock eligibility. |
| src/api2/routes/authentication/authenticateWithOpenidAuthCode.ts | Adds a debug bypass branch for OpenID auth-code exchange. |
| migrations/2026-07-28_14-11-47_sso_passwordless.js | Adds passwordless-auth columns; drops is_sso_v2 from bank_sso_config. |
| migrations/2026-07-27_10-00-00_add_password_backup_public_key.js | Adds password_backup_public_key to user_devices. |
Review details
Suppressed comments (1)
src/api2/routes/deviceAuthorization/authorizeDeviceWithOpenId.ts:76
hasVaultDatais computed but never used. Either remove it or use it to drive the authorization/pairing logic (e.g., only track pairing state when a vault already exists), otherwise it becomes misleading dead code.
// whether this user's vault already has data (ie. this is not the first, vault-creation
// device) — only an additional device on an already-created passwordless vault needs its
// backup public key tracked, so a peer can discover and approve it.
const hasVaultData = userRes.rowCount !== 0 && !!userRes.rows[0].encrypted_data_2;
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces security/behavioral risks (debug bypass snippet, account-enumeration response behavior, and unused locals that may break builds) that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (6)
src/api2/routes/deviceAuthorization/requestDeviceAccess.ts:63
encrypted_data_2is being selected andhasVaultDatais computed but never used, which will triggernoUnusedLocals/lint noise and adds an unnecessary column fetch.
let userRes = await db.query(
`SELECT
users.id AS id,
users.deactivated AS deactivated,
users.settings_override AS settings_override,
src/api2/routes/deviceAuthorization/authorizeDeviceWithOpenId.ts:75
hasVaultDatais computed fromencrypted_data_2but never used; this will fail builds ifnoUnusedLocalsis enabled and also adds an unnecessary column to the query.
let userRes = await db.query(
`SELECT
users.id AS id,
users.deactivated AS deactivated,
users.settings_override AS settings_override,
src/api2/routes/authentication/authenticateWithOpenidAuthCode.ts:69
- The commented-out "bypass code" debug block should be removed before merging; leaving authentication bypass snippets in the codebase is risky and may be accidentally re-enabled later.
// // BYPASS CODE FOR DEBUGGING PURPOSES
// if (safeBody.authCode === 'bypass_code') {
// // bypass code for debugging purposes
// const userEmail = youremail@domain.com;
// const openidSession = await SessionStore.createOpenIdSession(
src/api2/routes/passwordReset/backupPassword.ts:27
- This comment references
authorizeSsoDevice, which doesn't exist in this codebase; it makes the flow harder to understand/maintain.
// Also clears the target device's pending public key: receiving a backup here is the
// proof that another authorized device just approved it (see getPendingSsoDevices /
// authorizeSsoDevice). The device is already AUTHORIZED by this point (either via SSO's
// dedicated authorizeDeviceWithOpenId route, or via the classic email validation code) —
// this route never authorizes a device by itself.
src/api2/routes/authentication/usesPasswordlessUnlock.ts:22
- This endpoint currently returns 404 when the user/bank isn't found or the user is deactivated, which makes it usable for account enumeration; consider returning a normal 200 response with
usesPasswordlessUnlock: falseinstead.
const usesPasswordless = await usesPasswordlessUnlockForEmail(email, bankIds.internalId);
if (usesPasswordless === null) {
res.status(404).end();
return;
}
src/api2/routes/passwordReset/backupPassword.ts:47
backupPassword2used to return 204 with no body; switching to a 200 JSON response is a behavioral/API contract change that may break older clients that expect 204. A simple compatibility approach is to keep returning 204 when all backups applied, and only return 200 with a body when there are unapplied deviceIds.
const unappliedDeviceIds = results.filter((r) => !r.applied).map((r) => r.deviceId);
logInfo(req.body?.userEmail, 'backupPassword2 OK');
// Return res
return res.status(200).json({ unappliedDeviceIds });
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The device-access email flow appears broken for new/expired requests (missing email send) and a risky auth-bypass debug block is left in the OpenID auth handler.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/api2/routes/authentication/authenticateWithOpenidAuthCode.ts:54
- The commented-out "bypass code" block is a production authentication bypass (even if currently commented). Keeping this in the codebase is risky because it can be accidentally re-enabled and it advertises a bypass mechanism. Please remove it entirely before merging.
// // BYPASS CODE FOR DEBUGGING PURPOSES
// if (safeBody.authCode === 'bypass_code') {
// // bypass code for debugging purposes
// const userEmail = youremail@domain.com;
// const openidSession = await SessionStore.createOpenIdSession(
src/api2/routes/deviceAuthorization/requestDeviceAccess.ts:52
openidSessionis now required by Joi, but the rest of the handler still treats it as optional (email-based flow). As written, clients that don’t have an OpenID session must send an empty string to pass validation, and any non-empty value will force OpenID validation and skip the email path. MakeopenidSessionoptional (and allow empty) so the email authorization flow remains backward-compatible and unambiguous.
osNameAndVersion: Joi.string().required(),
installType: Joi.string().required(),
appVersion: Joi.string().required(),
openidSession: Joi.string().required(),
}).validate(req.body);
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
c6621d0 to
fdc90b8
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
There are security-impacting issues (reset tokens not being consumed in a passwordless path and a committed auth-bypass debug block) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/api2/routes/authentication/authenticateWithOpenidAuthCode.ts:68
- A commented-out auth-code bypass block is committed in the OpenID auth handler. Even commented, this is easy to accidentally re-enable and undermines the security posture of the endpoint.
// // BYPASS CODE FOR DEBUGGING PURPOSES
// if (safeBody.authCode === 'bypass_code') {
// // bypass code for debugging purposes
// const userEmail = youremail@domain.com;
// const openidSession = await SessionStore.createOpenIdSession(
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
https://github.com/rgsystemes/kb/issues/12526
https://github.com/rgsystemes/kb/issues/12968