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 00000000..4696f840 --- /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 c9471420..29589601 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 a896e61d..75b35387 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 af9e5ec3..8305e8f6 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 02785b54..4c2481ac 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 773dac01..bf392629 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({ @@ -261,7 +290,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' }); } @@ -284,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 || {}; @@ -346,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({ diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 30a9d8ee..9ec081e8 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);