From 5e3cdd0b2c7f10d02065512fce04eeb33754a297 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 15:38:32 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=AE=20fix:=20Consume=20an=20Invite=20O?= =?UTF-8?q?nly=20Once=20the=20Account=20Exists=20(#15611)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 📮 fix: Consume an Invite Only Once the Account Exists `checkInviteUser` deleted the invite token and then called `next()`, but everything that can still reject a registration runs after it: the schema, the allowed-domain check, and the email-already-in-use check. A mistyped password confirmation therefore destroyed the invite — the invitee corrected it, resubmitted, and got "Invalid invite token" with no way back short of an admin re-inviting them. The deletion moves to `registrationController`, after `registerUser` reports success. That report needed a new signal. `registerUser` returns the same 200 and the same generic message whether it created an account or found the email already in use — deliberately, so the response cannot be used to enumerate accounts — so the status alone cannot say whether an account exists. It now also returns `userCreated` on the creation path, which the controller reads and never forwards; the response body is unchanged. A failed deletion is logged rather than surfaced. By that point the account exists, and leaving a usable invite behind is recoverable in a way that telling the user their registration failed is not. Fixes #15541 * ♻️ test: Fold the Invite Tests Into the Existing AuthController Spec From Copilot's review. The new spec reached for `jest.requireActual` on `@librechat/data-schemas`, `~/server/services/AuthService` and `~/models`, which pulls real implementations into a unit test — `~/models` builds the data-schemas methods against mongoose — and diverges from `AuthController.spec.js`, which stubs each module outright. Rather than restate that harness with narrower stubs, the tests move into the spec that already has it. `deleteTokens` joins its `~/models` mock, which also keeps that shared mock in step with the controller's imports: a stale mock there hands the controller an undefined function, and the omission only surfaces when some later test happens to exercise the path. --- api/server/controllers/AuthController.js | 20 ++++- api/server/controllers/AuthController.spec.js | 84 ++++++++++++++++++- api/server/middleware/checkInviteUser.js | 8 +- api/server/middleware/checkInviteUser.spec.js | 70 ++++++++++++++++ api/server/services/AuthService.js | 5 +- 5 files changed, 181 insertions(+), 6 deletions(-) create mode 100644 api/server/middleware/checkInviteUser.spec.js diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index 47f5faf5b7..320f72f860 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -19,7 +19,13 @@ const { setAuthTokens, registerUser, } = require('~/server/services/AuthService'); -const { deleteAllUserSessions, getUserById, findSession, updateUser } = require('~/models'); +const { + deleteAllUserSessions, + getUserById, + findSession, + updateUser, + deleteTokens, +} = require('~/models'); const { getGraphApiToken } = require('~/server/services/GraphTokenService'); const { getRefreshTokenBridge } = require('~/server/services/RefreshTokenBridge'); const { @@ -52,6 +58,18 @@ const registrationController = async (req, res) => { try { const response = await registerUser(req.body); const { status, message } = response; + /** Consume the invite only once the account exists. `registerUser` returns the same + * 200 whether it created a user or found the email already in use, so the decision + * rests on `userCreated` rather than the status. A failure to delete leaves a + * usable invite, which is recoverable; failing the response here would tell a user + * whose account was just created that registration failed, which is not. */ + if (response.userCreated === true && req.invite?.token != null) { + try { + await deleteTokens({ token: req.invite.token }); + } catch (error) { + logger.error('[registrationController] Failed to consume invite after registration', error); + } + } res.status(status).send({ message }); } catch (err) { logger.error('[registrationController]', err); diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index d597e82edc..08d86646b1 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -45,6 +45,7 @@ jest.mock('~/models', () => ({ findSession: jest.fn(), updateUser: jest.fn(), findUser: jest.fn(), + deleteTokens: jest.fn(), })); jest.mock('~/server/services/RefreshTokenBridge', () => ({ OPENID_REFRESH_BRIDGE_GRACE_MS: 60 * 1000, @@ -119,7 +120,11 @@ const openIdClient = require('openid-client'); const jwt = require('jsonwebtoken'); const { logger } = require('@librechat/data-schemas'); const { isEnabled, findOpenIDUser, buildOpenIDRefreshParams } = require('@librechat/api'); -const { graphTokenController, refreshController } = require('./AuthController'); +const { + graphTokenController, + refreshController, + registrationController, +} = require('./AuthController'); const { getGraphApiToken } = require('~/server/services/GraphTokenService'); const { clearOpenIDAuthTokens, @@ -128,9 +133,10 @@ const { storeOpenIDSession, setCloudFrontAuthCookies, setAuthTokens, + registerUser, } = require('~/server/services/AuthService'); const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies'); -const { deleteSession, getUserById, findSession, updateUser } = require('~/models'); +const { deleteSession, getUserById, findSession, updateUser, deleteTokens } = require('~/models'); const { createRefreshTokenBridgeFlightKey, deleteRefreshTokenBridges, @@ -2361,3 +2367,77 @@ describe('refreshController – LibreChat path', () => { }); }); }); + +describe('registrationController - invite consumption', () => { + const invite = { token: 'hashed-invite', email: 'invitee@example.com' }; + + const buildRes = () => { + const res = {}; + res.status = jest.fn(() => res); + res.send = jest.fn(() => res); + res.json = jest.fn(() => res); + return res; + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('consumes the invite once the account exists', async () => { + registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true }); + + await registrationController({ body: {}, invite }, buildRes()); + + expect(deleteTokens).toHaveBeenCalledWith({ token: 'hashed-invite' }); + }); + + it('leaves the invite when registration is rejected', () => { + /** A mistyped password confirmation is the common case; it has to stay retryable. */ + registerUser.mockResolvedValue({ status: 404, message: 'The passwords did not match' }); + + return registrationController({ body: {}, invite }, buildRes()).then(() => { + expect(deleteTokens).not.toHaveBeenCalled(); + }); + }); + + it('leaves the invite when the email is already in use, despite the 200', async () => { + /** `registerUser` returns the same status and message whether it created an account + * or found the email taken, so the status alone cannot drive this decision. */ + registerUser.mockResolvedValue({ status: 200, message: 'ok' }); + + await registrationController({ body: {}, invite }, buildRes()); + + expect(deleteTokens).not.toHaveBeenCalled(); + }); + + it('does not attempt a deletion for an uninvited registration', async () => { + registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true }); + + await registrationController({ body: {} }, buildRes()); + + expect(deleteTokens).not.toHaveBeenCalled(); + }); + + it('still reports success when consuming the invite fails', async () => { + /** The account exists by this point; reporting failure would be worse than + * leaving a usable invite behind. */ + registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true }); + deleteTokens.mockRejectedValue(new Error('mongo unavailable')); + const res = buildRes(); + + await registrationController({ body: {}, invite }, res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(res.send).toHaveBeenCalledWith({ message: 'ok' }); + expect(logger.error).toHaveBeenCalled(); + }); + + it('never forwards the creation signal to the client', async () => { + registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true }); + const res = buildRes(); + + await registrationController({ body: {}, invite }, res); + + expect(res.send).toHaveBeenCalledWith({ message: 'ok' }); + }); +}); diff --git a/api/server/middleware/checkInviteUser.js b/api/server/middleware/checkInviteUser.js index 22f2824ffc..bb3fd778a8 100644 --- a/api/server/middleware/checkInviteUser.js +++ b/api/server/middleware/checkInviteUser.js @@ -1,5 +1,5 @@ const { getInvite: getInviteFn } = require('@librechat/api'); -const { createToken, findToken, deleteTokens } = require('~/models'); +const { createToken, findToken } = require('~/models'); const getInvite = (encodedToken, email) => getInviteFn(encodedToken, email, { createToken, findToken }); @@ -19,7 +19,11 @@ async function checkInviteUser(req, res, next) { return res.status(400).json({ message: 'Invalid invite token' }); } - await deleteTokens({ token: invite.token }); + /** The invite is deliberately left in place here. Everything that can still + * reject this registration — the schema, the allowed-domain check, an email + * already in use — runs after this middleware, and consuming the invite first + * meant a mistyped password confirmation destroyed it. `registrationController` + * deletes it once an account actually exists. */ req.invite = invite; next(); } catch (error) { diff --git a/api/server/middleware/checkInviteUser.spec.js b/api/server/middleware/checkInviteUser.spec.js new file mode 100644 index 0000000000..cc8b31f41b --- /dev/null +++ b/api/server/middleware/checkInviteUser.spec.js @@ -0,0 +1,70 @@ +const mockGetInvite = jest.fn(); +const mockDeleteTokens = jest.fn(); + +jest.mock('@librechat/api', () => ({ + getInvite: (...args) => mockGetInvite(...args), +})); + +jest.mock('~/models', () => ({ + createToken: jest.fn(), + findToken: jest.fn(), + deleteTokens: (...args) => mockDeleteTokens(...args), +})); + +const checkInviteUser = require('./checkInviteUser'); + +const buildRes = () => { + const res = {}; + res.status = jest.fn(() => res); + res.json = jest.fn(() => res); + return res; +}; + +describe('checkInviteUser', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('does not consume the invite, so a later failure cannot destroy it', async () => { + /** The reported bug: the invite was deleted here, before the schema, the + * allowed-domain check or the email-in-use check had run. A mistyped password + * confirmation left the invitee with no invite and no account. */ + mockGetInvite.mockResolvedValue({ token: 'hashed-invite', email: 'invitee@example.com' }); + const req = { body: { token: 'raw-token', email: 'invitee@example.com' } }; + const next = jest.fn(); + + await checkInviteUser(req, buildRes(), next); + + expect(mockDeleteTokens).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalledTimes(1); + }); + + it('hands the validated invite to the rest of the chain', async () => { + const invite = { token: 'hashed-invite', email: 'invitee@example.com' }; + mockGetInvite.mockResolvedValue(invite); + const req = { body: { token: 'raw-token', email: 'invitee@example.com' } }; + + await checkInviteUser(req, buildRes(), jest.fn()); + + expect(req.invite).toBe(invite); + }); + + it('rejects an invalid invite without touching the token store', async () => { + mockGetInvite.mockResolvedValue(null); + const res = buildRes(); + + await checkInviteUser({ body: { token: 'raw-token', email: 'a@b.c' } }, res, jest.fn()); + + expect(res.status).toHaveBeenCalledWith(400); + expect(mockDeleteTokens).not.toHaveBeenCalled(); + }); + + it('passes straight through when no invite token is supplied', async () => { + const next = jest.fn(); + + await checkInviteUser({ body: {} }, buildRes(), next); + + expect(next).toHaveBeenCalledTimes(1); + expect(mockGetInvite).not.toHaveBeenCalled(); + }); +}); diff --git a/api/server/services/AuthService.js b/api/server/services/AuthService.js index fb736ded81..911b7ebdc4 100644 --- a/api/server/services/AuthService.js +++ b/api/server/services/AuthService.js @@ -443,7 +443,10 @@ const registerUser = async (user, additionalData = {}) => { await updateUser(newUserId, { emailVerified: true }); } - return { status: 200, message: genericVerificationMessage }; + /** `userCreated` separates this from the identical 200 returned when the email is + * already in use, so a caller can act on an account having actually been created + * without the response body revealing which of the two happened. */ + return { status: 200, message: genericVerificationMessage, userCreated: true }; } catch (err) { logger.error('[registerUser] Error in registering user:', err); if (newUserId) {