From 5986f46f958b17cff93a596de7ad46421288e96b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:07:18 -0700 Subject: [PATCH 1/2] fix(security): stop GET /api/users/:id leaking any user's apiToken and email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL. Confirmed by live execution against production: a plain, non-admin, verified account fetched another user's profile and received the full Mongo document — including `apiToken`, a working `cm_` bearer that authenticates AS that user via middleware/auth.ts, plus their email address. Any account that could sign up could take over any other account, including admins. 68 verified humans were exposed. Cause: `toSocialProfile` spread `userDoc.toObject()` wholesale, and `.select('-password')` was the only projection standing between the schema and the response body. Two layers, because either alone is incomplete: 1. The serializer now drops credentials unconditionally (apiToken, agentRuntimeTokens, digestUnsubscribeToken, password) and drops account-private fields (email, entitlements, token scopes, ban state, preferences, contacts) whenever the viewer is not the profile's owner. Public social fields are untouched, so profiles still render. 2. `apiToken` becomes `select: false` on the schema, so a live credential can no longer ride along on any incidental query anywhere else in the codebase. Filtering by the field is unaffected (projection and filter are separate), so token authentication keeps working. Layer 2 has one dangerous failure mode: code that READS the value back now gets `undefined` unless it asks for `+apiToken`, and `issueUserTokenForInstallation` treats an absent token as "none exists" and rotates — which would have silently invalidated the credentials of all 42 agent accounts that currently hold one. Every reader is therefore updated explicitly: `agentIdentityService.getOrCreateAgentUser` (which backs the token-issuance and bootstrap paths), the agent-user lookup at agentIdentityService:526, and the `agent-tokens` hasToken probe. Two existing test mocks returned bare objects where production now chains `.select()`; both are updated to thenable query builders. Their assertions are unchanged. Not yet fixed, filed separately: the /api/pg/* shadow router (unfiltered instance-wide pod listing and a join with no policy check, both confirmed live) and instance-wide pod enumeration via getPodsByType/getPodById. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DMeWzgFxsfBcjoLVLewEES --- .../userController.secretLeak.test.js | 128 ++++++++++++++++++ .../routes/registry.runtime-tokens.test.js | 4 + ...tIdentityService.collisionResolver.test.js | 11 +- backend/controllers/userController.ts | 49 ++++++- backend/models/User.ts | 12 +- backend/routes/registry/agent-tokens.ts | 3 +- backend/services/agentIdentityService.ts | 6 +- 7 files changed, 207 insertions(+), 6 deletions(-) create mode 100644 backend/__tests__/unit/controllers/userController.secretLeak.test.js diff --git a/backend/__tests__/unit/controllers/userController.secretLeak.test.js b/backend/__tests__/unit/controllers/userController.secretLeak.test.js new file mode 100644 index 000000000..4696f8407 --- /dev/null +++ b/backend/__tests__/unit/controllers/userController.secretLeak.test.js @@ -0,0 +1,128 @@ +/** + * Regression: GET /api/users/:id leaked another user's credentials. + * + * Confirmed live against production before the fix — a plain, non-admin, + * verified account fetched an admin's profile and received the full Mongo + * document, including `apiToken` (a working `cm_` bearer that authenticates AS + * that user via middleware/auth.ts) and their email address. That is + * account-takeover of any user, including admins, by anyone who can sign up. + * + * Cause: `toSocialProfile` spread `userDoc.toObject()` wholesale, and + * `.select('-password')` was the only projection applied. + * + * Two layers are tested here: + * 1. the serializer drops secrets unconditionally and account-private fields + * when the viewer is not the owner + * 2. `apiToken` is `select: false` on the schema, so it cannot ride along on + * an incidental query anywhere else + */ + +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); +jest.mock('../../../models/User'); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/Post', () => ({})); +jest.mock('../../../models/Pod', () => ({})); +jest.mock('../../../services/agentIdentityService', () => ({})); +jest.mock('../../../services/avatarService', () => ({ normalizeAvatarUrl: (v) => v })); + +const User = require('../../../models/User'); +const userController = require('../../../controllers/userController'); + +const OWNER = 'user-owner'; +const VIEWER = 'user-viewer'; + +const makeUserDoc = (id) => ({ + _id: id, + username: 'sam', + followers: [], + following: [], + followedThreads: [], + toObject: () => ({ + _id: id, + username: 'sam', + email: 'sam@example.com', + profilePicture: 'default', + role: 'admin', + apiToken: 'cm_3767c5f74deadbeef', + apiTokenScopes: ['agent:events:read'], + agentRuntimeTokens: [{ tokenHash: 'abc' }], + digestUnsubscribeToken: 'unsub-123', + entitlements: { cloudAgents: true }, + banned: false, + followers: [], + following: [], + }), +}); + +const runGetUserById = async (targetId, viewerId) => { + User.findById.mockReturnValue({ + select: jest.fn().mockResolvedValue(makeUserDoc(targetId)), + }); + const req = { params: { id: targetId }, user: { id: viewerId } }; + let body; + const res = { + json: (b) => { body = b; }, + status: () => ({ json: (b) => { body = b; }, send: (b) => { body = b; } }), + send: (b) => { body = b; }, + }; + await userController.getUserById(req, res); + return body; +}; + +describe('GET /api/users/:id does not leak credentials (confirmed live exploit)', () => { + beforeEach(() => jest.clearAllMocks()); + + test('another user cannot obtain the target apiToken', async () => { + const body = await runGetUserById(OWNER, VIEWER); + + // The exact exploit: a working cm_ bearer for someone else. + expect(body).not.toHaveProperty('apiToken'); + expect(JSON.stringify(body)).not.toContain('cm_3767c5f74deadbeef'); + }); + + test('another user cannot obtain the target email or other private fields', async () => { + const body = await runGetUserById(OWNER, VIEWER); + + expect(body).not.toHaveProperty('email'); + expect(body).not.toHaveProperty('entitlements'); + expect(body).not.toHaveProperty('apiTokenScopes'); + expect(body).not.toHaveProperty('banned'); + }); + + test('agent runtime tokens and the unsubscribe token never leak either', async () => { + const body = await runGetUserById(OWNER, VIEWER); + + expect(body).not.toHaveProperty('agentRuntimeTokens'); + expect(body).not.toHaveProperty('digestUnsubscribeToken'); + }); + + test('public social fields still render, so profiles are not broken', async () => { + const body = await runGetUserById(OWNER, VIEWER); + + expect(body.username).toBe('sam'); + expect(body.profilePicture).toBe('default'); + expect(body).toHaveProperty('followersCount', 0); + expect(body).toHaveProperty('isFollowing', false); + }); + + test('viewing your OWN profile keeps private fields but STILL never the token', async () => { + const body = await runGetUserById(OWNER, OWNER); + + expect(body.email).toBe('sam@example.com'); + expect(body.entitlements).toEqual({ cloudAgents: true }); + // The UI gets a token from POST /api/auth/api-token/generate, never here. + expect(body).not.toHaveProperty('apiToken'); + expect(body).not.toHaveProperty('agentRuntimeTokens'); + }); +}); + +describe('apiToken schema projection', () => { + test('the field is select:false so it cannot ride along on an incidental query', () => { + // Layer 2. Asserted against the real schema rather than the mocked model. + const actual = jest.requireActual('../../../models/User'); + const schema = (actual.schema || actual.default?.schema); + const path = schema.path('apiToken'); + expect(path).toBeDefined(); + expect(path.options.select).toBe(false); + }); +}); diff --git a/backend/__tests__/unit/routes/registry.runtime-tokens.test.js b/backend/__tests__/unit/routes/registry.runtime-tokens.test.js index c9471420e..295896012 100644 --- a/backend/__tests__/unit/routes/registry.runtime-tokens.test.js +++ b/backend/__tests__/unit/routes/registry.runtime-tokens.test.js @@ -252,6 +252,8 @@ describe('agent runtime tokens', () => { }); User.findOne.mockReturnValue({ + // apiToken is select:false, so the route asks for it explicitly. + select: jest.fn().mockReturnThis(), lean: jest.fn().mockResolvedValue({ username: 'clawd-bot', apiToken: 'cm_token_123', @@ -285,6 +287,8 @@ describe('agent runtime tokens', () => { }); User.findOne.mockReturnValue({ + // apiToken is select:false, so the route asks for it explicitly. + select: jest.fn().mockReturnThis(), lean: jest.fn().mockResolvedValue({ username: 'clawd-bot', apiToken: 'cm_token_123', diff --git a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js index a896e61da..75b35387b 100644 --- a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js @@ -30,7 +30,10 @@ jest.mock('../../../models/User', () => { mockSaved.push(JSON.parse(JSON.stringify(this))); return this; }; - User.findOne = jest.fn(async (query) => { + // `apiToken` is select:false on the schema, so production code loads the + // agent user with `.select('+apiToken')`. The mock therefore has to be a + // thenable query builder, not a bare promise. + const findOneResult = (query) => { if (mockExisting && query.username === mockExisting.username) { const doc = JSON.parse(JSON.stringify(mockExisting)); doc.save = async function save() { @@ -40,6 +43,12 @@ jest.mock('../../../models/User', () => { return doc; } return null; + }; + User.findOne = jest.fn((query) => { + const promise = Promise.resolve(findOneResult(query)); + promise.select = () => promise; + promise.lean = () => promise; + return promise; }); User.find = (query) => ({ select: () => ({ diff --git a/backend/controllers/userController.ts b/backend/controllers/userController.ts index af9e5ec38..8305e8f68 100644 --- a/backend/controllers/userController.ts +++ b/backend/controllers/userController.ts @@ -5,6 +5,45 @@ const Pod = require('../models/Pod'); const AgentIdentityService = require('../services/agentIdentityService'); const { normalizeAvatarUrl } = require('../services/avatarService'); +// Credentials. These must never reach a response body under any circumstance, +// not even the account's own profile: `apiToken` is a live `cm_` bearer that +// authenticates as this user (middleware/auth.ts), and `agentRuntimeTokens` +// carries agent runtime credentials. The UI receives a freshly minted token +// from POST /api/auth/api-token/generate, which is the only place it belongs. +const SECRET_USER_FIELDS = [ + 'password', + 'apiToken', + 'agentRuntimeTokens', + 'digestUnsubscribeToken', +]; + +// Account-private, but legitimately visible to the account holder (and to an +// admin). Leaking these to any logged-in stranger is what turned a profile +// lookup into an enumeration tool: `GET /api/users/:id` used to return another +// user's email and their plaintext apiToken to any authenticated caller. +const PRIVATE_USER_FIELDS = [ + 'email', + 'emailPreferences', + 'digestPreferences', + 'entitlements', + 'apiTokenScopes', + 'apiTokenCreatedAt', + 'authProviders', + 'contacts', + 'activityFeed', + 'banned', + 'banReason', +]; + +/** + * Serialize a user for a response. + * + * `viewerId` is the caller. When it does not match the profile's owner, the + * account-private block is dropped as well as the secrets. Secrets are dropped + * unconditionally — a spread of `toObject()` is how they escaped before, so + * the deletion happens here rather than relying on every call site to project + * correctly. + */ const toSocialProfile = (userDoc: any, viewerId: any = null) => { const followers = Array.isArray(userDoc.followers) ? userDoc.followers : []; const following = Array.isArray(userDoc.following) ? userDoc.following : []; @@ -13,8 +52,16 @@ const toSocialProfile = (userDoc: any, viewerId: any = null) => { viewerIdStr && followers.some((id: any) => String(id) === viewerIdStr), ); + const plain = typeof userDoc.toObject === 'function' ? userDoc.toObject() : { ...userDoc }; + for (const field of SECRET_USER_FIELDS) delete plain[field]; + + const isSelf = Boolean(viewerIdStr && String(userDoc._id) === viewerIdStr); + if (!isSelf) { + for (const field of PRIVATE_USER_FIELDS) delete plain[field]; + } + return { - ...userDoc.toObject(), + ...plain, followersCount: followers.length, followingCount: following.length, followedThreadsCount: Array.isArray(userDoc.followedThreads) ? userDoc.followedThreads.length : 0, diff --git a/backend/models/User.ts b/backend/models/User.ts index 02785b540..4c2481acb 100644 --- a/backend/models/User.ts +++ b/backend/models/User.ts @@ -181,7 +181,17 @@ const userSchema = new Schema({ entitlements: { cloudAgents: { type: Boolean, default: false }, }, - apiToken: { type: String, unique: true, sparse: true }, + // `select: false` so a live bearer credential can never ride along on an + // incidental `findById().select('-password')` or a populate(). Queries that + // FILTER on this field still work (projection is separate from the filter), + // so token authentication in middleware/auth.ts is unaffected. Any code that + // needs to READ the value back must ask for it explicitly with + // `.select('+apiToken')` — see agentIdentityService.getOrCreateAgentUser, + // where failing to do so would make an existing agent token look absent and + // silently rotate it out from under every running wrapper. + apiToken: { + type: String, unique: true, sparse: true, select: false, + }, apiTokenCreatedAt: { type: Date }, apiTokenScopes: [{ type: String }], isBot: { type: Boolean, default: false }, diff --git a/backend/routes/registry/agent-tokens.ts b/backend/routes/registry/agent-tokens.ts index 773dac01e..42e9fad8b 100644 --- a/backend/routes/registry/agent-tokens.ts +++ b/backend/routes/registry/agent-tokens.ts @@ -261,7 +261,8 @@ agentTokensRouter.get('/pods/:podId/agents/:name/user-token', auth, async (req: const resolvedType = AgentIdentityService.resolveAgentType(name); const agentUsername = AgentIdentityService.buildAgentUsername(resolvedType, instanceId); - const agentUser = await User.findOne({ username: agentUsername }).lean(); + // Needs +apiToken (select:false on the field) to report hasToken correctly. + const agentUser = await User.findOne({ username: agentUsername }).select('+apiToken').lean(); if (!agentUser || !agentUser.apiToken) { return res.json({ hasToken: false, scopes: [], scopeMode: 'none' }); } diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 30a9d8eec..9ec081e87 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -371,7 +371,9 @@ class AgentIdentityService { const instanceId = options.instanceId || 'default'; const username = buildAgentUsername(resolvedType, instanceId); - let agentUser = await User.findOne({ username }); + // `apiToken` is `select: false`; ask for it explicitly or an existing + // agent credential looks absent and gets rotated out from under the fleet. + let agentUser = await User.findOne({ username }).select('+apiToken'); // Determine if this is an official (default instance) agent const isOfficial = instanceId === 'default' && !!typeConfig; @@ -523,7 +525,7 @@ class AgentIdentityService { static async removeAgentFromPod(agentType: string, podId: unknown, instanceId = 'default'): Promise | null> { if (!agentType || !podId) return null; const username = buildAgentUsername(agentType, instanceId); - const agentUser = await User.findOne({ username }); + const agentUser = await User.findOne({ username }).select('+apiToken'); if (!agentUser) return null; const pod = await Pod.findById(podId); From 46930801502bc366bf7ad8334db7377a72d962d4 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:34:12 -0700 Subject: [PATCH 2/2] fix(security): rate-limit the agent token routes CodeQL flagged js/missing-rate-limiting on routes/registry/agent-tokens.ts once this PR touched the file. The gap was pre-existing and these are exactly the routes that read and mint agent credentials, so an unbounded loop against them is worth blunting: 60 requests/min, keyed on the hashed bearer token. Imported as ESM and placed first on each route because the query cannot trace a limiter through a require() return or a router.use wrapper. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DMeWzgFxsfBcjoLVLewEES --- backend/routes/registry/agent-tokens.ts | 41 +++++++++++++++++++++---- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/backend/routes/registry/agent-tokens.ts b/backend/routes/registry/agent-tokens.ts index 42e9fad8b..bf392629b 100644 --- a/backend/routes/registry/agent-tokens.ts +++ b/backend/routes/registry/agent-tokens.ts @@ -1,5 +1,34 @@ // Agent token management routes — extracted from registry.js (GH#112) // Handles: runtime-tokens (R/W/D) and user-token (R/W/D) + +// ESM import (not require) so CodeQL's js/missing-rate-limiting query can +// trace the middleware; it cannot follow a limiter through a require() return +// or a router.use wrapper. Same shape as routes/messages.ts. +import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import { createHash } from 'crypto'; + +interface TokenRateReq { get?: (name: string) => string | undefined; ip?: string } +interface TokenRateRes { status: (code: number) => { json: (body: unknown) => void } } + +// These routes read and mint agent credentials, so they are deliberately +// tighter than ordinary reads. Keyed on the hashed bearer token so one NAT'd +// office doesn't share a bucket. +const tokenRouteLimit = rateLimit({ + windowMs: 60_000, + max: 60, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: (req: TokenRateReq) => { + const authHeader = req.get?.('authorization'); + if (authHeader) { + return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; + } + return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + }, + handler: (_req: unknown, res: TokenRateRes) => { + res.status(429).json({ error: 'rate limit exceeded: 60 token requests per 60s' }); + }, +}); const express = require('express'); const auth = require('../../middleware/auth'); const { AgentInstallation } = require('../../models/AgentRegistry'); @@ -24,7 +53,7 @@ const agentTokensRouter = express.Router(); * GET /api/registry/pods/:podId/agents/:name/runtime-tokens * List runtime tokens for an installed agent */ -agentTokensRouter.get('/pods/:podId/agents/:name/runtime-tokens', auth, async (req: any, res: any) => { +agentTokensRouter.get('/pods/:podId/agents/:name/runtime-tokens', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name } = req.params; const { installation, instanceId } = await resolveInstallation({ @@ -75,7 +104,7 @@ agentTokensRouter.get('/pods/:podId/agents/:name/runtime-tokens', auth, async (r * POST /api/registry/pods/:podId/agents/:name/runtime-tokens * Issue a runtime token for an installed agent */ -agentTokensRouter.post('/pods/:podId/agents/:name/runtime-tokens', auth, async (req: any, res: any) => { +agentTokensRouter.post('/pods/:podId/agents/:name/runtime-tokens', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name } = req.params; const { label, instanceId, force } = req.body || {}; @@ -155,7 +184,7 @@ agentTokensRouter.post('/pods/:podId/agents/:name/runtime-tokens', auth, async ( * DELETE /api/registry/pods/:podId/agents/:name/runtime-tokens/:tokenId * Revoke a runtime token for an installed agent */ -agentTokensRouter.delete('/pods/:podId/agents/:name/runtime-tokens/:tokenId', auth, async (req: any, res: any) => { +agentTokensRouter.delete('/pods/:podId/agents/:name/runtime-tokens/:tokenId', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name, tokenId } = req.params; const { installation, instanceId } = await resolveInstallation({ @@ -226,7 +255,7 @@ agentTokensRouter.delete('/pods/:podId/agents/:name/runtime-tokens/:tokenId', au * GET /api/registry/pods/:podId/agents/:name/user-token * Get metadata for the agent's designated user token (no raw token returned) */ -agentTokensRouter.get('/pods/:podId/agents/:name/user-token', auth, async (req: any, res: any) => { +agentTokensRouter.get('/pods/:podId/agents/:name/user-token', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name } = req.params; const { installation, instanceId } = await resolveInstallation({ @@ -285,7 +314,7 @@ agentTokensRouter.get('/pods/:podId/agents/:name/user-token', auth, async (req: * POST /api/registry/pods/:podId/agents/:name/user-token * Issue a designated user API token for the agent user */ -agentTokensRouter.post('/pods/:podId/agents/:name/user-token', auth, async (req: any, res: any) => { +agentTokensRouter.post('/pods/:podId/agents/:name/user-token', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name } = req.params; const { scopes, instanceId, displayName } = req.body || {}; @@ -347,7 +376,7 @@ agentTokensRouter.post('/pods/:podId/agents/:name/user-token', auth, async (req: * DELETE /api/registry/pods/:podId/agents/:name/user-token * Revoke designated user token for the agent user */ -agentTokensRouter.delete('/pods/:podId/agents/:name/user-token', auth, async (req: any, res: any) => { +agentTokensRouter.delete('/pods/:podId/agents/:name/user-token', tokenRouteLimit, auth, async (req: any, res: any) => { try { const { podId, name } = req.params; const { installation, instanceId } = await resolveInstallation({