From c9cce6010027dac2268ad8c482539231db949170 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:59:13 -0700 Subject: [PATCH] fix(auth): the browser-JWT branch now carries username, email and role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `middleware/auth.ts` dispatched on the token prefix and left two different shapes on `req.user`: the `cm_` API-token branch assigned `{ id, username, email, role }`, the browser-JWT branch assigned `{ id }`. Nothing errored — consumers simply read `undefined` for every browser session. Two live consequences, both confirmed at 799e0d7d: - `github.ts:146` refuses genuine admins with `403 Admin only`; the same admin holding an API token gets through. - registry publish/install persist `publisher.name: undefined` (#1211). Three call sites had already grown a private DB re-read to work around it — `podController.isGlobalAdminRequest`, `agentProfile.canEditAgentAvatar` and `marketplace-api.resolveUsername`, the last of which #1211 duplicates rather than moves. `github.ts` was the site that never got its copy. The JWT branch already runs one indexed `User.findById` per request for ban enforcement, so widening that projection makes the two branches shape-identical at zero extra round-trips. The existing local re-reads stay correct and become redundant fast-paths; removing them is separate work. The test drives the real middleware over a real JWT, because the suites covering the affected routes inject `{ _id, role }` from a fake auth middleware — i.e. the API-token shape — and so cannot see this defect. Its `User` mock honours the projection string: with a `select()` that ignores its argument, narrowing the middleware back to `.select('banned')` leaves all seven tests green. Both mutations (narrowed projection, restored `{ id }`) redden four of seven. Co-Authored-By: Claude Opus 5 --- .../middleware/authUserShapeParity.test.js | 132 ++++++++++++++++++ backend/middleware/auth.ts | 24 +++- 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/unit/middleware/authUserShapeParity.test.js diff --git a/backend/__tests__/unit/middleware/authUserShapeParity.test.js b/backend/__tests__/unit/middleware/authUserShapeParity.test.js new file mode 100644 index 000000000..58d17f3c9 --- /dev/null +++ b/backend/__tests__/unit/middleware/authUserShapeParity.test.js @@ -0,0 +1,132 @@ +const jwt = require('jsonwebtoken'); +const mongoose = require('mongoose'); + +// `middleware/auth.ts` dispatches on the token prefix and, until this test, +// the two branches left DIFFERENT shapes on `req.user`: the `cm_` API-token +// path assigned `{ id, username, email, role }`, the browser-JWT path assigned +// `{ id }`. Nothing errored — consumers just read `undefined`: +// +// - `github.ts:146` refused genuine admins with `403 Admin only` +// - registry publish/install persisted `publisher.name: undefined` (#1211) +// +// The bug is invisible to the suites that cover those routes, because their +// fake auth middleware injects `{ _id, role }` directly — i.e. the API-token +// shape. Only a test that drives the real middleware over a real browser JWT +// can see it, which is why it lives here rather than beside either consumer. + +let mockLiveUser = null; +let mockApiTokenUser = null; + +// The mock HONOURS the projection string. A `select()` that ignores its +// argument and hands back the whole row makes this suite fail open: narrowing +// the middleware's projection back to `.select('banned')` then leaves all +// seven tests green, because the fields arrive from the fixture rather than +// from the query. Verified by mutation — with the naive mock, that narrowing +// is invisible; with this one it reddens. +const project = (row, fields) => { + if (!row) return row; + const keys = String(fields).split(/\s+/).filter(Boolean); + return Object.fromEntries(keys.filter((k) => k in row).map((k) => [k, row[k]])); +}; + +jest.mock('../../../models/User', () => ({ + findOne: jest.fn(() => ({ + select: async (fields) => ( + mockApiTokenUser && { ...project(mockApiTokenUser, fields), _id: mockApiTokenUser._id } + ), + })), + findById: jest.fn(() => ({ + select: (fields) => ({ lean: async () => project(mockLiveUser, fields) }), + })), + updateOne: jest.fn(() => Promise.resolve()), +})); + +const authMiddleware = require('../../../middleware/auth'); + +const ID = new mongoose.Types.ObjectId(); + +const runWithJwt = async () => { + process.env.JWT_SECRET = 'test-jwt-secret'; + const token = jwt.sign({ id: ID.toString() }, process.env.JWT_SECRET); + const req = { header: (h) => (h === 'Authorization' ? `Bearer ${token}` : null) }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const next = jest.fn(); + await authMiddleware(req, res, next); + return { req, res, next }; +}; + +const runWithApiToken = async () => { + const req = { header: (h) => (h === 'Authorization' ? 'Bearer cm_testtoken' : null) }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const next = jest.fn(); + await authMiddleware(req, res, next); + return { req, res, next }; +}; + +describe('auth middleware: both token branches leave the same req.user shape', () => { + beforeEach(() => { + mockLiveUser = { + banned: false, username: 'lily', email: 'lily@example.com', role: 'admin', + }; + mockApiTokenUser = { + _id: ID, username: 'lily', email: 'lily@example.com', role: 'admin', banned: false, + }; + }); + + it('the browser-JWT branch carries username, email and role', async () => { + const { req, next } = await runWithJwt(); + expect(next).toHaveBeenCalled(); + expect(req.user).toEqual({ + id: ID.toString(), username: 'lily', email: 'lily@example.com', role: 'admin', + }); + }); + + it('the two branches agree on the key set', async () => { + const jwtReq = (await runWithJwt()).req; + const apiReq = (await runWithApiToken()).req; + // Fails closed in BOTH directions: narrowing the JWT branch reintroduces + // the original defect, and widening only the API-token branch reintroduces + // it for whatever field gets added next. + expect(Object.keys(jwtReq.user).sort()).toEqual(Object.keys(apiReq.user).sort()); + expect(jwtReq.user).toEqual(apiReq.user); + }); + + it('the github.ts:146 predicate now admits a browser-session admin', async () => { + const { req } = await runWithJwt(); + // Verbatim the consumer's own test, not a paraphrase of it. + expect(req.user?.role !== 'admin').toBe(false); + }); + + it('CONTROL: the predicate still refuses a browser-session non-admin', async () => { + // Without this, the test above would pass just as well against a + // middleware that hardcoded `role: 'admin'` for everyone. + mockLiveUser = { + banned: false, username: 'mallory', email: 'm@example.com', role: 'member', + }; + const { req } = await runWithJwt(); + expect(req.user?.role !== 'admin').toBe(true); + }); + + it('a user row without a role leaves role undefined, not defaulted', async () => { + // Absence must stay absent: a default here would grant or deny on data the + // row does not contain. + mockLiveUser = { banned: false, username: 'nobody' }; + const { req } = await runWithJwt(); + expect(req.user.role).toBeUndefined(); + expect(req.user.username).toBe('nobody'); + }); + + it('the ban check still fires on the same widened read', async () => { + mockLiveUser = { banned: true, username: 'lily', role: 'admin' }; + const { res, next } = await runWithJwt(); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('a deleted account is still refused', async () => { + mockLiveUser = null; + const { res, next } = await runWithJwt(); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + }); +}); diff --git a/backend/middleware/auth.ts b/backend/middleware/auth.ts index d021fb628..90625286a 100644 --- a/backend/middleware/auth.ts +++ b/backend/middleware/auth.ts @@ -73,12 +73,32 @@ export default async function auth(req: Request, res: Response, next: NextFuncti // Admin moderation: one indexed read so a ban (or account deletion) takes // effect on the NEXT request, not at JWT expiry days later. - const live = await User.findById(id).select('banned').lean() as { banned?: boolean } | null; + // + // The projection is wider than the ban check needs, deliberately. This + // branch used to assign `req.user = { id }` while the `cm_` branch above + // assigned `{ id, username, email, role }`, so every consumer of + // `req.user.username` or `req.user.role` silently got `undefined` for + // browser sessions — `github.ts:146` refused genuine admins with + // `403 Admin only`, and the registry persisted `publisher.name: undefined` + // (#1211). Three call sites had already grown a private DB re-read to work + // around it (`podController.isGlobalAdminRequest`, + // `agentProfile.canEditAgentAvatar`, `marketplace-api.resolveUsername`); + // a fourth was one copy short. Widening a query this branch already runs + // makes both branches shape-identical at zero extra round-trips, so no + // consumer has to know which token type it was called with. + const live = await User.findById(id) + .select('banned username email role') + .lean() as { banned?: boolean; username?: string; email?: string; role?: string } | null; if (!live) return res.status(401).json({ msg: 'Account no longer exists' }); if (live.banned) return res.status(403).json({ msg: 'This account has been suspended.' }); req.userId = id; - req.user = { id }; + req.user = { + id, + username: live.username, + email: live.email, + role: live.role, + }; touchLastActive(id); next(); } catch (err: unknown) {