From cbfcce4e97c70affc1c37498212a4a9ce493a574 Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Fri, 18 Sep 2026 14:39:50 +0200 Subject: [PATCH 1/9] Lock down user roles, contact fields, product writes and docs editing - User roles and isTester are writable only by admin/superAdmin; any user could previously grant themselves superAdmin via PATCH /1/users/me. - User email, phone, roles and isTester are readable only by self and staff roles, so ?include=owner no longer leaks them. - Product create/update/delete require products.write. - /1/docs (unauthenticated writes to openapi.json) is mounted in development only. --- services/api/src/models/definitions/user.json | 69 +++++++++++++------ services/api/src/routes/index.js | 8 ++- services/api/src/routes/products.js | 7 +- services/api/src/routes/products.test.js | 38 ++++++++-- services/api/src/routes/uploads-local.test.js | 12 ++++ services/api/src/routes/users.test.js | 23 +++++++ 6 files changed, 128 insertions(+), 29 deletions(-) diff --git a/services/api/src/models/definitions/user.json b/services/api/src/models/definitions/user.json index 648a4369d..d2ae1b114 100644 --- a/services/api/src/models/definitions/user.json +++ b/services/api/src/models/definitions/user.json @@ -16,6 +16,12 @@ "writeAccess": [ "admin", "superAdmin" + ], + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" ] }, "emailVerified": { @@ -30,6 +36,12 @@ "writeAccess": [ "admin", "superAdmin" + ], + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" ] }, "phoneVerified": { @@ -71,29 +83,44 @@ } } ], - "roles": [ - { - "role": { - "type": "String", - "required": true - }, - "scope": { - "type": "String", - "required": true, - "enum": [ - "global", - "organization" - ] - }, - "scopeRef": { - "type": "ObjectId", - "ref": "Organization" + "$staff": { + "type": "Scope", + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" + ], + "writeAccess": [ + "admin", + "superAdmin" + ], + "attributes": { + "roles": [ + { + "role": { + "type": "String", + "required": true + }, + "scope": { + "type": "String", + "required": true, + "enum": [ + "global", + "organization" + ] + }, + "scopeRef": { + "type": "ObjectId", + "ref": "Organization" + } + } + ], + "isTester": { + "type": "Boolean", + "default": false } } - ], - "isTester": { - "type": "Boolean", - "default": false }, "$readOnly": { "type": "Scope", diff --git a/services/api/src/routes/index.js b/services/api/src/routes/index.js index af5b0a352..db1904ada 100644 --- a/services/api/src/routes/index.js +++ b/services/api/src/routes/index.js @@ -1,4 +1,5 @@ const Router = require('@koa/router'); +const config = require('@bedrockio/config'); const meta = require('./meta'); const docs = require('./docs'); @@ -22,7 +23,12 @@ const router = new Router({ }); router.use('/meta', meta.routes()); -router.use('/docs', docs.routes()); + +// Docs editing writes to openapi.json on disk and is unauthenticated. +if (config.get('ENV_NAME') === 'development') { + router.use('/docs', docs.routes()); +} + router.use('/auth', auth.routes()); router.use('/users', users.routes()); router.use('/products', products.routes()); diff --git a/services/api/src/routes/products.js b/services/api/src/routes/products.js index fb46ba131..c3db29587 100644 --- a/services/api/src/routes/products.js +++ b/services/api/src/routes/products.js @@ -2,6 +2,7 @@ const Router = require('@koa/router'); const { fetchByParam } = require('../utils/middleware/params'); const { validateBody } = require('../utils/middleware/validate'); const { authenticate } = require('../utils/middleware/authenticate'); +const { requirePermissions } = require('../utils/middleware/permissions'); const { csvExport } = require('../utils/csv'); const { Product } = require('../models'); @@ -10,7 +11,7 @@ const router = new Router(); router .use(authenticate()) .param('id', fetchByParam(Product)) - .post('/', validateBody(Product.getCreateValidation()), async (ctx) => { + .post('/', requirePermissions('products.write'), validateBody(Product.getCreateValidation()), async (ctx) => { const product = await Product.create(ctx.request.body); ctx.body = { @@ -44,7 +45,7 @@ router }; }, ) - .patch('/:id', validateBody(Product.getUpdateValidation()), async (ctx) => { + .patch('/:id', requirePermissions('products.write'), validateBody(Product.getUpdateValidation()), async (ctx) => { const { product } = ctx.state; product.assign(ctx.request.body); @@ -54,7 +55,7 @@ router data: product, }; }) - .delete('/:id', async (ctx) => { + .delete('/:id', requirePermissions('products.write'), async (ctx) => { const { product } = ctx.state; await product.delete(); ctx.status = 204; diff --git a/services/api/src/routes/products.test.js b/services/api/src/routes/products.test.js index 9b4ce4904..5df064e89 100644 --- a/services/api/src/routes/products.test.js +++ b/services/api/src/routes/products.test.js @@ -1,12 +1,12 @@ const mongoose = require('mongoose'); -const { request, createUser } = require('../utils/testing'); +const { request, createUser, createAdmin } = require('../utils/testing'); const { Product } = require('../models'); describe('/1/products', () => { describe('POST /', () => { it('should be able to create product', async () => { - const user = await createUser(); + const user = await createAdmin(); const response = await request( 'POST', '/1/products', @@ -20,6 +20,16 @@ describe('/1/products', () => { expect(response).toHaveStatus(200); expect(data.name).toBe('some other product'); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('POST', '/1/products', { name: 'x', shop: product.shop }, { user }); + expect(response).toHaveStatus(403); + }); }); describe('GET /:product', () => { @@ -74,7 +84,7 @@ describe('/1/products', () => { describe('PATCH /:product', () => { it('admins should be able to update product', async () => { - const user = await createUser(); + const user = await createAdmin(); const product = await Product.create({ name: 'test 1', description: 'Some description', @@ -86,11 +96,21 @@ describe('/1/products', () => { const dbProduct = await Product.findById(product.id); expect(dbProduct.name).toEqual('new name'); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('PATCH', `/1/products/${product.id}`, { name: 'new name' }, { user }); + expect(response).toHaveStatus(403); + }); }); describe('DELETE /:product', () => { it('should be able to delete product', async () => { - const user = await createUser(); + const user = await createAdmin(); const product = await Product.create({ name: 'test 1', description: 'Some description', @@ -101,5 +121,15 @@ describe('/1/products', () => { const dbProduct = await Product.findByIdDeleted(product.id); expect(dbProduct.deletedAt).toBeDefined(); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('DELETE', `/1/products/${product.id}`, {}, { user }); + expect(response).toHaveStatus(403); + }); }); }); diff --git a/services/api/src/routes/uploads-local.test.js b/services/api/src/routes/uploads-local.test.js index 51b67c22d..f13e18801 100644 --- a/services/api/src/routes/uploads-local.test.js +++ b/services/api/src/routes/uploads-local.test.js @@ -32,6 +32,18 @@ describe('/1/uploads', () => { expect(response.body.data.filename).toBe('test.png'); }); + it('should not expose owner contact details when included', async () => { + const owner = await createUser({ phone: '+12125551234' }); + const upload = await createUpload({ owner }); + const response = await request('GET', `/1/uploads/${upload.id}?include=owner`, {}, {}); + expect(response).toHaveStatus(200); + const { owner: data } = response.body.data; + expect(data.firstName).toBe(owner.firstName); + expect(data.email).toBeUndefined(); + expect(data.phone).toBeUndefined(); + expect(data.roles).toBeUndefined(); + }); + it('should be able to access private upload as admin', async () => { const admin = await createAdmin(); const upload = await createUpload({ diff --git a/services/api/src/routes/users.test.js b/services/api/src/routes/users.test.js index 3c53fd494..f4b439323 100644 --- a/services/api/src/routes/users.test.js +++ b/services/api/src/routes/users.test.js @@ -77,6 +77,29 @@ describe('/1/users', () => { user = await User.findById(user._id); expect(user.deviceToken).toBe('new-token'); }); + + it('should not allow changing own roles', async () => { + let user = await createUser(); + const response = await request( + 'PATCH', + '/1/users/me', + { + roles: [{ role: 'superAdmin', scope: 'global' }], + }, + { user }, + ); + expect(response).toHaveStatus(400); + user = await User.findById(user._id); + expect(user.roles).toHaveLength(0); + }); + + it('should not allow setting tester flag', async () => { + let user = await createUser(); + const response = await request('PATCH', '/1/users/me', { isTester: true }, { user }); + expect(response).toHaveStatus(400); + user = await User.findById(user._id); + expect(user.isTester).toBe(false); + }); }); describe('POST /', () => { From 2bbad12c9c903c92187ec3553b798ee7f9d32393 Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Fri, 18 Sep 2026 14:41:37 +0200 Subject: [PATCH 2/9] Harden password reset, TOTP login and OAuth account linking - /auth/password/update only accepts access tokens minted for reset-password; unsubscribe-link tokens could previously reset the password and return a full session. - TOTP login always requires a password verified in the last 5 minutes; enableTotp never set isMfa, so TOTP alone was a full login. - Google/Apple login into an account whose email was never verified clears its existing authenticators and sessions, so a password set by whoever signed up first with that email stops working. Google sign-ups are now marked emailVerified. --- services/api/src/routes/auth/apple.js | 3 +- services/api/src/routes/auth/google.js | 3 +- services/api/src/routes/auth/google.test.js | 21 ++++++++++++++ services/api/src/routes/auth/password.js | 8 ++++- services/api/src/routes/auth/password.test.js | 17 +++++++++-- services/api/src/routes/auth/totp.test.js | 29 ++++++++++++++----- services/api/src/utils/auth/google.js | 1 + services/api/src/utils/auth/login.js | 12 ++++++++ services/api/src/utils/auth/totp.js | 6 ++-- 9 files changed, 83 insertions(+), 17 deletions(-) diff --git a/services/api/src/routes/auth/apple.js b/services/api/src/routes/auth/apple.js index a129d4790..42fd704ad 100644 --- a/services/api/src/routes/auth/apple.js +++ b/services/api/src/routes/auth/apple.js @@ -3,7 +3,7 @@ const yd = require('@bedrockio/yada'); const { validateBody } = require('../../utils/middleware/validate'); const { authenticate } = require('../../utils/middleware/authenticate'); -const { login } = require('../../utils/auth'); +const { login, claimUnverifiedUser } = require('../../utils/auth'); const { createAuthToken } = require('../../utils/tokens'); const { verifyToken, upsertAppleAuthenticator, removeAppleAuthenticator } = require('../../utils/auth/apple'); const { User, AuditEntry } = require('../../models'); @@ -36,6 +36,7 @@ router let result; if (user) { + claimUnverifiedUser(user); token = await login(ctx, user, { message: 'Logged in with Apple', }); diff --git a/services/api/src/routes/auth/google.js b/services/api/src/routes/auth/google.js index 02fbe9777..d7d9cfbd6 100644 --- a/services/api/src/routes/auth/google.js +++ b/services/api/src/routes/auth/google.js @@ -3,7 +3,7 @@ const yd = require('@bedrockio/yada'); const { validateBody } = require('../../utils/middleware/validate'); const { authenticate } = require('../../utils/middleware/authenticate'); -const { login } = require('../../utils/auth'); +const { login, claimUnverifiedUser } = require('../../utils/auth'); const { createAuthToken } = require('../../utils/tokens'); const { verifyToken, upsertGoogleAuthenticator, removeGoogleAuthenticator } = require('../../utils/auth/google'); @@ -35,6 +35,7 @@ router let result; if (user) { + claimUnverifiedUser(user); token = await login(ctx, user, { message: 'Logged in with Google', }); diff --git a/services/api/src/routes/auth/google.test.js b/services/api/src/routes/auth/google.test.js index 919f032b5..1f596258d 100644 --- a/services/api/src/routes/auth/google.test.js +++ b/services/api/src/routes/auth/google.test.js @@ -63,6 +63,27 @@ describe('/1/auth/google', () => { expect(lastUsedAt).toEqual(new Date('2020-01-01T00:00:01.000Z')); }); + it('should remove credentials added before the email was verified', async () => { + let user = await createUser({ + email: 'foo@bar.com', + password: 'attacker password', + }); + const code = createCode({ + email: 'foo@bar.com', + }); + + const response = await request('POST', '/1/auth/google', { + code, + }); + expect(response).toHaveStatus(200); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(true); + expect(hasAuthenticator(user, 'password')).toBe(false); + expect(hasAuthenticator(user, 'google')).toBe(true); + expect(user.authTokens).toHaveLength(1); + }); + it('should not be able to register with an unverified email', async () => { const code = createCode({ givenName: 'Bob', diff --git a/services/api/src/routes/auth/password.js b/services/api/src/routes/auth/password.js index a7d627d92..6592d5133 100644 --- a/services/api/src/routes/auth/password.js +++ b/services/api/src/routes/auth/password.js @@ -125,8 +125,14 @@ router password: yd.string().password().required(), }), async (ctx) => { - const { authUser } = ctx.state; + const { authUser, jwt } = ctx.state; const { password } = ctx.request.body; + + // Other access tokens (e.g. unsubscribe links) share this token type. + if (jwt.action !== 'reset-password') { + ctx.throw(401, 'Token is not valid for password reset.'); + } + authUser.password = password; authUser.loginAttempts = 0; diff --git a/services/api/src/routes/auth/password.test.js b/services/api/src/routes/auth/password.test.js index 4a0d4356c..a10ffa77e 100644 --- a/services/api/src/routes/auth/password.test.js +++ b/services/api/src/routes/auth/password.test.js @@ -320,7 +320,7 @@ describe('/1/auth', () => { let user = await createUser(); const password = 'very new password'; const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); @@ -349,6 +349,17 @@ describe('/1/auth', () => { ]); }); + it('should reject access tokens issued for other actions', async () => { + const user = await createUser(); + const token = createAccessToken(user, { + action: 'unsubscribe', + channel: 'email', + }); + + const response = await request('POST', '/1/auth/password/update', { password: 'new password' }, { token }); + expect(response).toHaveStatus(401); + }); + it('should error without user', async () => { const response = await request('POST', '/1/auth/password/update', { password: 'new password', @@ -360,7 +371,7 @@ describe('/1/auth', () => { mockTime('2020-01-01T00:00:00.000Z'); const user = await createUser(); const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); @@ -419,7 +430,7 @@ describe('/1/auth', () => { const password = 'very new password'; const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); diff --git a/services/api/src/routes/auth/totp.test.js b/services/api/src/routes/auth/totp.test.js index 7e554172c..eda821965 100644 --- a/services/api/src/routes/auth/totp.test.js +++ b/services/api/src/routes/auth/totp.test.js @@ -10,7 +10,7 @@ describe('/1/auth/totp', () => { it('should verify a code', async () => { mockTime('2020-01-01T00:00:00.000Z'); - let user = await createUser(); + let user = await createUser({ password: 'password' }); const secret = createSecret(); enableTotp(user, secret); @@ -37,12 +37,26 @@ describe('/1/auth/totp', () => { assertAuthToken(user, response.body.data.token); user = await User.findById(user.id); - expect(user.authenticators.toObject()).toMatchObject([ - { - type: 'totp', - lastUsedAt: new Date('2020-01-01T00:00:01.000Z'), - }, - ]); + expect(user.authenticators.find((a) => a.type === 'totp').lastUsedAt).toEqual( + new Date('2020-01-01T00:00:01.000Z'), + ); + }); + + it('should require a recent password login', async () => { + mockTime('2020-01-01T00:00:00.000Z'); + + const user = await createUser({ password: 'password' }); + const secret = createSecret(); + enableTotp(user, secret); + await user.save(); + + advanceTime(10 * 60 * 1000); + + const response = await request('POST', '/1/auth/totp/login', { + email: user.email, + code: speakeasy.totp({ secret }), + }); + expect(response).toHaveStatus(401); }); it('should throttle logins', async () => { @@ -51,6 +65,7 @@ describe('/1/auth/totp', () => { let code; const user = await createUser({ + password: 'password', loginAttempts: 5, lastLoginAttemptAt: new Date(), }); diff --git a/services/api/src/utils/auth/google.js b/services/api/src/utils/auth/google.js index 3a0a36edb..791b56385 100644 --- a/services/api/src/utils/auth/google.js +++ b/services/api/src/utils/auth/google.js @@ -25,6 +25,7 @@ async function verifyToken(code) { } return { email: payload.email, + emailVerified: true, firstName: payload.given_name, lastName: payload.family_name, }; diff --git a/services/api/src/utils/auth/login.js b/services/api/src/utils/auth/login.js index b96981d1d..3afbb439a 100644 --- a/services/api/src/utils/auth/login.js +++ b/services/api/src/utils/auth/login.js @@ -33,6 +33,17 @@ async function login(ctx, user, options = {}) { return token; } +// Called when a provider has verified the email: credentials added before the +// address was verified may belong to whoever signed up with it first. +function claimUnverifiedUser(user) { + if (!user.emailVerified) { + user.authenticators = []; + user.authTokens = []; + user.mfaMethod = 'none'; + user.emailVerified = true; + } +} + async function verifyLoginAttempts(user, ctx) { let { loginAttempts = 0, lastLoginAttemptAt } = user; @@ -65,5 +76,6 @@ async function verifyLoginAttempts(user, ctx) { module.exports = { login, + claimUnverifiedUser, verifyLoginAttempts, }; diff --git a/services/api/src/utils/auth/totp.js b/services/api/src/utils/auth/totp.js index e84b28f6b..8f622df80 100644 --- a/services/api/src/utils/auth/totp.js +++ b/services/api/src/utils/auth/totp.js @@ -48,10 +48,8 @@ async function revokeTotp(user) { async function verifyTotp(user, code) { const authenticator = assertAuthenticator(user, 'totp'); verifyCode(authenticator.secret, code); - - if (authenticator.isMfa) { - await verifyRecentPassword(user); - } + // TOTP is only ever a second factor after password login. + verifyRecentPassword(user); authenticator.lastUsedAt = new Date(); } From 9e74e7dc580375e6b32838fcb5d73cab778f1795 Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Fri, 18 Sep 2026 14:48:48 +0200 Subject: [PATCH 3/9] Mark email verified on invite accept and password reset Both prove ownership of the mailbox, so Google/Apple login no longer treats these accounts as unverified and clears their credentials. --- services/api/src/routes/auth/password.js | 1 + services/api/src/routes/auth/password.test.js | 1 + services/api/src/routes/invites.js | 2 ++ 3 files changed, 4 insertions(+) diff --git a/services/api/src/routes/auth/password.js b/services/api/src/routes/auth/password.js index 6592d5133..30b9c0096 100644 --- a/services/api/src/routes/auth/password.js +++ b/services/api/src/routes/auth/password.js @@ -134,6 +134,7 @@ router } authUser.password = password; + authUser.emailVerified = true; authUser.loginAttempts = 0; const token = createAuthToken(ctx, authUser); diff --git a/services/api/src/routes/auth/password.test.js b/services/api/src/routes/auth/password.test.js index a10ffa77e..563f84966 100644 --- a/services/api/src/routes/auth/password.test.js +++ b/services/api/src/routes/auth/password.test.js @@ -341,6 +341,7 @@ describe('/1/auth', () => { user = await User.findById(user.id); await expect(verifyPassword(user, password)).resolves.not.toThrow(); + expect(user.emailVerified).toBe(true); expect(user.authTokens).toEqual([ expect.objectContaining({ diff --git a/services/api/src/routes/invites.js b/services/api/src/routes/invites.js index 9365a3afb..06e3feeeb 100644 --- a/services/api/src/routes/invites.js +++ b/services/api/src/routes/invites.js @@ -62,6 +62,7 @@ router }); if (user) { + user.emailVerified = true; const token = createAuthToken(ctx, user); await user.save(); ctx.body = { @@ -74,6 +75,7 @@ router const user = new User({ ...ctx.request.body, email, + emailVerified: true, ...(role && { roles: [ { From 3b68168a85e9f2bad5448b75b15f328f57a45f3c Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Fri, 18 Sep 2026 14:56:24 +0200 Subject: [PATCH 4/9] Clear pre-verification logins on password reset and invite accept Marking the email verified without clearing credentials let a squatter's passkey and sessions survive a reset and skip the later OAuth cleanup. --- services/api/src/routes/auth/password.js | 4 ++-- services/api/src/routes/auth/password.test.js | 21 +++++++++++++++++++ services/api/src/routes/invites.js | 3 ++- services/api/src/utils/auth/login.js | 5 +++-- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/services/api/src/routes/auth/password.js b/services/api/src/routes/auth/password.js index 30b9c0096..a725d0523 100644 --- a/services/api/src/routes/auth/password.js +++ b/services/api/src/routes/auth/password.js @@ -7,7 +7,7 @@ const { validateBody } = require('../../utils/middleware/validate'); const { authenticate } = require('../../utils/middleware/authenticate'); const { createAuthToken, createAccessToken } = require('../../utils/tokens'); -const { login, verifyLoginAttempts } = require('../../utils/auth'); +const { login, verifyLoginAttempts, claimUnverifiedUser } = require('../../utils/auth'); const { verifyPassword } = require('../../utils/auth/password'); const { sendOtp } = require('../../utils/auth/otp'); const { sendMail } = require('../../utils/messaging'); @@ -133,8 +133,8 @@ router ctx.throw(401, 'Token is not valid for password reset.'); } + claimUnverifiedUser(authUser); authUser.password = password; - authUser.emailVerified = true; authUser.loginAttempts = 0; const token = createAuthToken(ctx, authUser); diff --git a/services/api/src/routes/auth/password.test.js b/services/api/src/routes/auth/password.test.js index 563f84966..ac3e93fe2 100644 --- a/services/api/src/routes/auth/password.test.js +++ b/services/api/src/routes/auth/password.test.js @@ -350,6 +350,27 @@ describe('/1/auth', () => { ]); }); + it('should remove sessions created before the email was verified', async () => { + let user = await createUser(); + createAuthToken(context(), user); + const token = createAccessToken(user, { + action: 'reset-password', + duration: '30m', + }); + await user.save(); + + const response = await request('POST', '/1/auth/password/update', { password: 'new password' }, { token }); + expect(response).toHaveStatus(200); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(true); + expect(user.authTokens).toEqual([ + expect.objectContaining({ + jti: getJti(response.body.data.token), + }), + ]); + }); + it('should reject access tokens issued for other actions', async () => { const user = await createUser(); const token = createAccessToken(user, { diff --git a/services/api/src/routes/invites.js b/services/api/src/routes/invites.js index 06e3feeeb..4857c5c87 100644 --- a/services/api/src/routes/invites.js +++ b/services/api/src/routes/invites.js @@ -7,6 +7,7 @@ const { authenticate } = require('../utils/middleware/authenticate'); const { requirePermissions } = require('../utils/middleware/permissions'); const { createAuthToken } = require('../utils/tokens'); +const { claimUnverifiedUser } = require('../utils/auth'); const { Invite, User, AuditEntry } = require('../models'); const { sendMessage, sendMail } = require('../utils/messaging'); @@ -62,7 +63,7 @@ router }); if (user) { - user.emailVerified = true; + claimUnverifiedUser(user); const token = createAuthToken(ctx, user); await user.save(); ctx.body = { diff --git a/services/api/src/utils/auth/login.js b/services/api/src/utils/auth/login.js index 3afbb439a..e29a4a20d 100644 --- a/services/api/src/utils/auth/login.js +++ b/services/api/src/utils/auth/login.js @@ -33,8 +33,9 @@ async function login(ctx, user, options = {}) { return token; } -// Called when a provider has verified the email: credentials added before the -// address was verified may belong to whoever signed up with it first. +// Call once the user proves they own the email (OAuth, reset link, invite). +// Until then anyone could have signed up with this address, so its existing +// logins (password, passkeys, TOTP, sessions) are removed rather than trusted. function claimUnverifiedUser(user) { if (!user.emailVerified) { user.authenticators = []; From d6e7b0b128b0ba941673465dd4d08a1d2fdadd5e Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Fri, 18 Sep 2026 15:11:43 +0200 Subject: [PATCH 5/9] Verify email only when the OTP was delivered by email OTP login marked emailVerified based on the lookup field, so a code sent to an attacker's phone could verify a squatted email and exempt it from claimUnverifiedUser. Non-MFA email OTP login now also clears logins added before verification, like password reset. --- services/api/src/models/definitions/user.json | 4 ++++ services/api/src/routes/auth/otp.js | 19 ++++++++++++------ services/api/src/routes/auth/otp.test.js | 20 +++++++++++++++++-- services/api/src/utils/auth/otp.js | 4 +++- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/services/api/src/models/definitions/user.json b/services/api/src/models/definitions/user.json index d2ae1b114..15f9a4411 100644 --- a/services/api/src/models/definitions/user.json +++ b/services/api/src/models/definitions/user.json @@ -162,6 +162,10 @@ "type": "Boolean", "readAccess": "none" }, + "channel": { + "type": "String", + "readAccess": "none" + }, "info": { "type": "Object", "readAccess": "none" diff --git a/services/api/src/routes/auth/otp.js b/services/api/src/routes/auth/otp.js index 0741f4b0b..a86738327 100644 --- a/services/api/src/routes/auth/otp.js +++ b/services/api/src/routes/auth/otp.js @@ -4,7 +4,7 @@ const { validateBody } = require('../../utils/middleware/validate'); const { sendOtp } = require('../../utils/auth/otp'); const { verifyOtp } = require('../../utils/auth/otp'); -const { login, verifyLoginAttempts } = require('../../utils/auth'); +const { login, verifyLoginAttempts, claimUnverifiedUser } = require('../../utils/auth'); const { AuditEntry } = require('../../models'); const { findUser } = require('./utils'); @@ -44,7 +44,7 @@ router phone: yd.string().phone(), }), async (ctx) => { - const { code, email, phone } = ctx.request.body; + const { code } = ctx.request.body; const user = await findUser(ctx); if (!user) { @@ -58,8 +58,9 @@ router ctx.throw(401, error); } + let authenticator; try { - await verifyOtp(user, code); + authenticator = await verifyOtp(user, code); } catch (error) { await user.save(); await AuditEntry.append('OTP Verification Failure', { @@ -69,9 +70,15 @@ router ctx.throw(401, error); } - if (email) { - user.emailVerified = true; - } else if (phone) { + // Verify the channel the code was delivered to, not the one used to look up the user. + if (authenticator.channel === 'email') { + // An MFA code follows a password login, so this user already owns the account. + if (authenticator.isMfa) { + user.emailVerified = true; + } else { + claimUnverifiedUser(user); + } + } else if (authenticator.channel === 'sms') { user.phoneVerified = true; } diff --git a/services/api/src/routes/auth/otp.test.js b/services/api/src/routes/auth/otp.test.js index ed69e12f2..8250917b6 100644 --- a/services/api/src/routes/auth/otp.test.js +++ b/services/api/src/routes/auth/otp.test.js @@ -175,7 +175,7 @@ describe('/1/auth/otp', () => { const user = await createUser({ email: 'foo@bar.com', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'email' }); const response = await request('POST', '/1/auth/otp/login', { email: user.email, code, @@ -188,7 +188,7 @@ describe('/1/auth/otp', () => { const user = await createUser({ phone: '+12223456789', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'sms' }); const response = await request('POST', '/1/auth/otp/login', { phone: user.phone, code, @@ -282,6 +282,22 @@ describe('/1/auth/otp', () => { expect(user.phoneVerified).toBe(true); }); + it('should not verify email when the code was sent by sms', async () => { + let user = await createUser({ + email: 'foo@bar.com', + phone: '+12223456789', + }); + const code = await createOtp(user, { channel: 'sms' }); + await request('POST', '/1/auth/otp/login', { + email: user.email, + code, + }); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(false); + expect(user.phoneVerified).toBe(true); + }); + it('should throttle logins', async () => { mockTime('2020-01-01'); let response; diff --git a/services/api/src/utils/auth/otp.js b/services/api/src/utils/auth/otp.js index 7ed51640b..8737f0f3c 100644 --- a/services/api/src/utils/auth/otp.js +++ b/services/api/src/utils/auth/otp.js @@ -54,12 +54,13 @@ async function createOtp(user, options = {}) { clearAuthenticators(user, 'otp'); const code = user.isTester ? TESTER_CODE : generateCode(); - const { isMfa = false } = options; + const { isMfa = false, channel } = options; addAuthenticator(user, { type: 'otp', code, isMfa, + channel, expiresAt: new Date(Date.now() + EXPIRE), }); @@ -85,6 +86,7 @@ async function verifyOtp(user, code) { } clearAuthenticators(user, 'otp'); + return authenticator; } module.exports = { From 3c3d70f887a077c54fa73efa5a0c154eb46535d5 Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Tue, 22 Sep 2026 19:26:56 +0200 Subject: [PATCH 6/9] Require exactly one of email or phone on code login routes Both fields were optional and findUser silently preferred phone, so a request could name one account and be resolved against another. --- services/api/src/routes/auth/otp.js | 28 ++++++++++++---------- services/api/src/routes/auth/otp.test.js | 14 +++++++++++ services/api/src/routes/auth/totp.js | 14 ++++++----- services/api/src/routes/auth/utils.js | 30 ++++++++++-------------- 4 files changed, 51 insertions(+), 35 deletions(-) diff --git a/services/api/src/routes/auth/otp.js b/services/api/src/routes/auth/otp.js index a86738327..ec56424f0 100644 --- a/services/api/src/routes/auth/otp.js +++ b/services/api/src/routes/auth/otp.js @@ -7,19 +7,21 @@ const { verifyOtp } = require('../../utils/auth/otp'); const { login, verifyLoginAttempts, claimUnverifiedUser } = require('../../utils/auth'); const { AuditEntry } = require('../../models'); -const { findUser } = require('./utils'); +const { findUser, validateIdentity } = require('./utils'); const router = new Router(); router .post( '/send', - validateBody({ - type: yd.string().allow('link', 'code').default('code'), - channel: yd.string().allow('email', 'sms').default('email'), - email: yd.string().email(), - phone: yd.string().phone(), - }), + validateBody( + validateIdentity({ + type: yd.string().allow('link', 'code').default('code'), + channel: yd.string().allow('email', 'sms').default('email'), + email: yd.string().email(), + phone: yd.string().phone(), + }), + ), async (ctx) => { const { body } = ctx.request; const user = await findUser(ctx); @@ -38,11 +40,13 @@ router ) .post( '/login', - validateBody({ - code: yd.string().length(6).required(), - email: yd.string().email(), - phone: yd.string().phone(), - }), + validateBody( + validateIdentity({ + code: yd.string().length(6).required(), + email: yd.string().email(), + phone: yd.string().phone(), + }), + ), async (ctx) => { const { code } = ctx.request.body; const user = await findUser(ctx); diff --git a/services/api/src/routes/auth/otp.test.js b/services/api/src/routes/auth/otp.test.js index 8250917b6..3ce9ecb38 100644 --- a/services/api/src/routes/auth/otp.test.js +++ b/services/api/src/routes/auth/otp.test.js @@ -282,6 +282,20 @@ describe('/1/auth/otp', () => { expect(user.phoneVerified).toBe(true); }); + it('should not accept both an email and a phone', async () => { + const user = await createUser({ + email: 'foo@bar.com', + phone: '+12223456789', + }); + const code = await createOtp(user, { channel: 'email' }); + const response = await request('POST', '/1/auth/otp/login', { + email: user.email, + phone: user.phone, + code, + }); + expect(response).toHaveStatus(400); + }); + it('should not verify email when the code was sent by sms', async () => { let user = await createUser({ email: 'foo@bar.com', diff --git a/services/api/src/routes/auth/totp.js b/services/api/src/routes/auth/totp.js index 3b983c0c7..921802580 100644 --- a/services/api/src/routes/auth/totp.js +++ b/services/api/src/routes/auth/totp.js @@ -8,18 +8,20 @@ const { login, verifyLoginAttempts } = require('../../utils/auth'); const { verifyCode, verifyTotp, generateTotp, enableTotp, revokeTotp } = require('../../utils/auth/totp'); const { AuditEntry } = require('../../models'); -const { findUser } = require('./utils'); +const { findUser, validateIdentity } = require('./utils'); const router = new Router(); router .post( '/login', - validateBody({ - phone: yd.string().phone(), - email: yd.string().email(), - code: yd.string().length(6).required(), - }), + validateBody( + validateIdentity({ + phone: yd.string().phone(), + email: yd.string().email(), + code: yd.string().length(6).required(), + }), + ), async (ctx) => { const { code } = ctx.request.body; diff --git a/services/api/src/routes/auth/utils.js b/services/api/src/routes/auth/utils.js index 4edf6c4a1..0baaaec7d 100644 --- a/services/api/src/routes/auth/utils.js +++ b/services/api/src/routes/auth/utils.js @@ -1,26 +1,22 @@ +const yd = require('@bedrockio/yada'); const { User } = require('../../models'); -async function findUser(ctx) { - const { phone, email } = ctx.request.body; - - let query; - if (phone) { - query = { phone }; - } else if (email) { - query = { email }; - } else { - if (phone === '') { - ctx.throw(400, 'Phone is required.'); - } else if (email === '') { - ctx.throw(400, 'Email is required.'); - } else { - ctx.throw(400, 'Phone or email is required.'); +// Identify the account by exactly one of email or phone, so the channel a code +// is sent to is never ambiguous. +function validateIdentity(body) { + return yd.object(body).custom((val) => { + if (!val.email === !val.phone) { + throw new Error('Either email or phone is required.'); } - } + }); +} - return await User.findOne(query); +async function findUser(ctx) { + const { phone, email } = ctx.request.body; + return await User.findOne(phone ? { phone } : { email }); } module.exports = { findUser, + validateIdentity, }; From 459a754edd4b971a70ac3833bcf6a81c31caa1ee Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Tue, 22 Sep 2026 19:33:55 +0200 Subject: [PATCH 7/9] Send the login code to the identifier that named the account /otp/send defaulted to email even when the request named a phone, so an sms login emailed the code and never verified the phone. Also fixes test expectations: write-access violations return 401, and query params go in the request helper. --- services/api/src/routes/auth/otp.js | 4 ++- services/api/src/routes/auth/otp.test.js | 27 +++++++++++++++++-- services/api/src/routes/uploads-local.test.js | 2 +- services/api/src/routes/users.test.js | 4 +-- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/services/api/src/routes/auth/otp.js b/services/api/src/routes/auth/otp.js index ec56424f0..321aa3151 100644 --- a/services/api/src/routes/auth/otp.js +++ b/services/api/src/routes/auth/otp.js @@ -17,7 +17,7 @@ router validateBody( validateIdentity({ type: yd.string().allow('link', 'code').default('code'), - channel: yd.string().allow('email', 'sms').default('email'), + channel: yd.string().allow('email', 'sms'), email: yd.string().email(), phone: yd.string().phone(), }), @@ -28,6 +28,8 @@ router const challenge = await sendOtp(user, { ...body, + // Send to whichever identifier named the account unless told otherwise. + channel: body.channel || (body.phone ? 'sms' : 'email'), phase: 'login', }); diff --git a/services/api/src/routes/auth/otp.test.js b/services/api/src/routes/auth/otp.test.js index 3ce9ecb38..ca43b2195 100644 --- a/services/api/src/routes/auth/otp.test.js +++ b/services/api/src/routes/auth/otp.test.js @@ -258,7 +258,7 @@ describe('/1/auth/otp', () => { let user = await createUser({ email: 'foo@bar.com', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'email' }); await request('POST', '/1/auth/otp/login', { email: user.email, code, @@ -272,7 +272,30 @@ describe('/1/auth/otp', () => { let user = await createUser({ phone: '+12223456789', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'sms' }); + await request('POST', '/1/auth/otp/login', { + phone: user.phone, + code, + }); + + user = await User.findById(user.id); + expect(user.phoneVerified).toBe(true); + }); + + it('should text the code when identified by phone', async () => { + let user = await createUser({ + phone: '+12223456789', + }); + + await request('POST', '/1/auth/otp/send', { + phone: user.phone, + }); + assertSmsSent({ + phone: user.phone, + }); + + user = await User.findById(user.id); + const { code } = user.authenticators[0]; await request('POST', '/1/auth/otp/login', { phone: user.phone, code, diff --git a/services/api/src/routes/uploads-local.test.js b/services/api/src/routes/uploads-local.test.js index f13e18801..bcc033d19 100644 --- a/services/api/src/routes/uploads-local.test.js +++ b/services/api/src/routes/uploads-local.test.js @@ -35,7 +35,7 @@ describe('/1/uploads', () => { it('should not expose owner contact details when included', async () => { const owner = await createUser({ phone: '+12125551234' }); const upload = await createUpload({ owner }); - const response = await request('GET', `/1/uploads/${upload.id}?include=owner`, {}, {}); + const response = await request('GET', `/1/uploads/${upload.id}`, { include: 'owner' }, {}); expect(response).toHaveStatus(200); const { owner: data } = response.body.data; expect(data.firstName).toBe(owner.firstName); diff --git a/services/api/src/routes/users.test.js b/services/api/src/routes/users.test.js index f4b439323..b0cdae05f 100644 --- a/services/api/src/routes/users.test.js +++ b/services/api/src/routes/users.test.js @@ -88,7 +88,7 @@ describe('/1/users', () => { }, { user }, ); - expect(response).toHaveStatus(400); + expect(response).toHaveStatus(401); user = await User.findById(user._id); expect(user.roles).toHaveLength(0); }); @@ -96,7 +96,7 @@ describe('/1/users', () => { it('should not allow setting tester flag', async () => { let user = await createUser(); const response = await request('PATCH', '/1/users/me', { isTester: true }, { user }); - expect(response).toHaveStatus(400); + expect(response).toHaveStatus(401); user = await User.findById(user._id); expect(user.isTester).toBe(false); }); From 2c748167535a196b0475206421b71f0094942549 Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Tue, 22 Sep 2026 19:42:01 +0200 Subject: [PATCH 8/9] Regenerate OpenAPI definition Reflects the code login schemas and the user model scope change. --- services/api/openapi.json | 167 ++++++++++++++++++++------------------ 1 file changed, 89 insertions(+), 78 deletions(-) diff --git a/services/api/openapi.json b/services/api/openapi.json index cbd922ad8..5185517ef 100644 --- a/services/api/openapi.json +++ b/services/api/openapi.json @@ -63,7 +63,6 @@ }, "channel": { "type": "string", - "default": "email", "enum": [ "email", "sms" @@ -878,6 +877,40 @@ "required": [], "additionalProperties": false }, + "createdAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, + "updatedAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, "roles": { "type": "object", "properties": { @@ -963,40 +996,6 @@ "isTester": { "type": "boolean" }, - "createdAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, - "updatedAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, "include": { "$ref": "#/components/schemas/Includes" }, @@ -1463,7 +1462,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } } }, "/1/products/:id": { @@ -1562,7 +1565,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } }, "delete": { "summary": "Delete product", @@ -1581,7 +1588,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } } }, "/1/products/search": { @@ -3894,6 +3905,40 @@ "required": [], "additionalProperties": false }, + "createdAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, + "updatedAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, "roles": { "type": "object", "properties": { @@ -3979,40 +4024,6 @@ "isTester": { "type": "boolean" }, - "createdAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, - "updatedAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, "include": { "$ref": "#/components/schemas/Includes" }, @@ -4995,6 +5006,12 @@ "additionalProperties": false } }, + "createdAt": { + "$ref": "#/components/schemas/DateTime" + }, + "updatedAt": { + "$ref": "#/components/schemas/DateTime" + }, "roles": { "type": "array", "items": { @@ -5023,12 +5040,6 @@ }, "isTester": { "type": "boolean" - }, - "createdAt": { - "$ref": "#/components/schemas/DateTime" - }, - "updatedAt": { - "$ref": "#/components/schemas/DateTime" } }, "required": [ From 89ab5de3cfc834261a9ddb06f1c87e9addbdd9df Mon Sep 17 00:00:00 2001 From: Kaare Larsen Date: Tue, 22 Sep 2026 19:43:12 +0200 Subject: [PATCH 9/9] Distinguish the two identity validation failures --- services/api/src/routes/auth/utils.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/api/src/routes/auth/utils.js b/services/api/src/routes/auth/utils.js index 683dbd17d..f523eebd2 100644 --- a/services/api/src/routes/auth/utils.js +++ b/services/api/src/routes/auth/utils.js @@ -5,7 +5,9 @@ import { User } from '../../models/index.js'; // is sent to is never ambiguous. function validateIdentity(body) { return yd.object(body).custom((val) => { - if (!val.email === !val.phone) { + if (val.email && val.phone) { + throw new Error('Cannot provide both an email and a phone.'); + } else if (!val.email && !val.phone) { throw new Error('Either email or phone is required.'); } });