Skip to content

Commit 345e084

Browse files
authored
fix(security): co-pod-member rule + no ghost-users on POST /room; rate-limit POST /pods (#617)
* fix(security): enforce co-pod-member rule + stop ghost-user creation on POST /room; rate-limit POST /pods From the agent-runtime security audit. - **Co-pod-member bypass (CRITICAL)**: the agent path of POST /api/agents/runtime/room created an agent↔agent DM room with NO sharePod check — any runtime token could DM any agent, defeating the §3.7 isolation the /agent-dm route already enforces. Added the same DMService.sharePod gate (403 rule:'sharePod' when no shared pod). - **Ghost bot-user creation (HIGH)**: the target agent was resolved with getOrCreateAgentUser (upsert), so a made-up agentName from a leaked token materialized an orphan User row (unbounded table pollution). Now resolved with a non-upsert User.findOne — unknown agent → 404. - **POST /pods had no rate limit (HIGH)**: a runtime token could spray unlimited pod creation. Added phase4RateLimit (matches /room, /agent-dm, /messages). Regression lock: agentsRuntime.room.test.js gains a non-existent-agent 404 case and a no-shared-pod 403 case; existing same-pod cases still pass (alice+bob share a pod). * fix(#617): sanitize agent identity in /room to clear CodeQL js/sql-injection The new non-upsert User.findOne({username}) fed a req.body-derived username → CodeQL js/sql-injection. Applied the strip-then-use sanitizer CodeQL recognises (same pattern as routes/registry/install.ts): agentName/ instanceId/username are stripped to [a-z0-9-] before the query. No behavior change for valid inputs (usernames are already that charset). * fix(#617): put phase4RateLimit before auth on POST /pods for CodeQL recognition CodeQL's js/missing-rate-limiting only credits a limiter that PRECEDES the other route middleware (documented in install.ts/uploads.ts). The limiter was present but second (after agentRuntimeAuth) so the query still flagged the DB access. Reordered to first — the key generator reads the auth header directly, so it works pre-auth.
1 parent 7a42db7 commit 345e084

2 files changed

Lines changed: 71 additions & 13 deletions

File tree

backend/__tests__/service/agentsRuntime.room.test.js

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,5 +202,42 @@ describe('POST /api/agents/runtime/room — dual-auth (ADR-010 Phase 1)', () =>
202202
.send({});
203203
expect(res.status).toBe(400);
204204
});
205+
206+
// Security hardening (audit): the agent path must enforce the §3.7
207+
// co-pod-member rule (it previously did not) and must not materialise a
208+
// ghost bot User for an unknown agentName.
209+
it('returns 404 for a non-existent target agent (no ghost-user creation)', async () => {
210+
const res = await request(app)
211+
.post('/api/agents/runtime/room')
212+
.set('Authorization', `Bearer ${aliceToken}`)
213+
.send({ agentName: 'ghostface-nonexistent' });
214+
expect(res.status).toBe(404);
215+
// No User row was materialised for the made-up name.
216+
const ghost = await User.findOne({ username: 'ghostface-nonexistent', isBot: true });
217+
expect(ghost).toBeNull();
218+
});
219+
220+
it('returns 403 when the two agents share no pod (co-pod-member rule)', async () => {
221+
// charlie is registered + installed into a SEPARATE pod — alice and
222+
// charlie never share a pod, so alice must not be able to DM charlie.
223+
await registerAgent('charlie', 'Charlie');
224+
const otherPod = await Pod.create({
225+
name: 'Charlie-only pod',
226+
type: 'chat',
227+
createdBy: humanUser._id,
228+
members: [humanUser._id],
229+
});
230+
await request(app)
231+
.post('/api/registry/install')
232+
.set('Authorization', `Bearer ${humanToken}`)
233+
.send({ agentName: 'charlie', podId: otherPod._id.toString(), scopes: ['context:read'] });
234+
235+
const res = await request(app)
236+
.post('/api/agents/runtime/room')
237+
.set('Authorization', `Bearer ${aliceToken}`)
238+
.send({ agentName: 'charlie' });
239+
expect(res.status).toBe(403);
240+
expect(res.body.rule).toBe('sharePod');
241+
});
205242
});
206243
});

backend/routes/agentsRuntime.ts

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -591,27 +591,44 @@ router.post('/room', dualAuth, phase4RateLimit, async (req: any, res: any) => {
591591
agentName: rawAgentName,
592592
instanceId: rawInstanceId,
593593
} = req.body || {};
594-
const agentName = String(rawAgentName || '').trim().toLowerCase();
595-
const instanceId = String(rawInstanceId || '').trim() || 'default';
594+
// Sanitize agent identity from the request body via the strip-then-
595+
// compare pattern CodeQL recognises as a SqlSanitizer for js/sql-injection
596+
// (same shape as routes/registry/install.ts). Usernames are [a-z0-9-],
597+
// instanceIds [a-z0-9-]; anything else is invalid input, not one of our
598+
// agents.
599+
const agentName = String(rawAgentName || '').trim().toLowerCase().replace(/[^a-z0-9-]/g, '');
600+
const instanceId = (String(rawInstanceId || '').trim().toLowerCase().replace(/[^a-z0-9-]/g, '')) || 'default';
596601
if (!agentName) {
597602
return res.status(400).json({ message: 'agentName is required' });
598603
}
599604

600-
// Resolve target agent's User row. `getOrCreateAgentUser` is upsert,
601-
// which means a misspelled `agentName` materialises a ghost bot User
602-
// row. ADR-010 Phase 1 accepts this side effect (it mirrors the
603-
// existing human-path semantics on the same route, and the blast
604-
// radius is bounded — the ghost is just an unattached User). A name-
605-
// existence check or rate limit is filed for v1.x if abuse surfaces.
606-
const targetAgentUser = await AgentIdentityService.getOrCreateAgentUser(
607-
agentName,
608-
{ instanceId },
609-
);
605+
// Resolve the target agent WITHOUT upserting — a misspelled or made-up
606+
// agentName must 404, not materialise a ghost bot User row (unbounded
607+
// User-table pollution from a leaked token). Only real, already-existing
608+
// agents are DM-able.
609+
const targetUsername = String(AgentIdentityService.buildAgentUsername(agentName, instanceId))
610+
.replace(/[^a-z0-9-]/g, '');
611+
const targetAgentUser = await User.findOne({ username: targetUsername, isBot: true }).select('_id');
612+
if (!targetAgentUser) {
613+
return res.status(404).json({ message: 'target agent not found' });
614+
}
610615

611616
if (String(targetAgentUser._id) === String(callerAgentUserId)) {
612617
return res.status(400).json({ message: 'Cannot DM yourself' });
613618
}
614619

620+
// §3.7 co-pod-member rule — an agent may only open a DM with another
621+
// agent it already shares a pod with. The /agent-dm route enforces this;
622+
// the /room agent path previously did not, letting any agent token DM
623+
// any agent (cross-tenant collaboration bypass).
624+
const shared = await DMService.sharePod(callerAgentUserId, targetAgentUser._id);
625+
if (!shared) {
626+
return res.status(403).json({
627+
message: 'No shared pod with target — refused per co-pod-member rule',
628+
rule: 'sharePod',
629+
});
630+
}
631+
615632
const room = await DMService.getOrCreateAgentRoom(
616633
targetAgentUser._id,
617634
callerAgentUserId,
@@ -2287,7 +2304,11 @@ router.get('/pods', agentRuntimeAuth, async (req: any, res: any) => {
22872304
* POST /pods (agent runtime token auth)
22882305
* Create a new pod as the agent's bot user
22892306
*/
2290-
router.post('/pods', agentRuntimeAuth, async (req: any, res: any) => {
2307+
// phase4RateLimit FIRST (before auth) so CodeQL's js/missing-rate-limiting
2308+
// query recognises the guard — it only credits a limiter that precedes the
2309+
// other middleware. The key generator reads the auth header directly, so it
2310+
// works pre-auth.
2311+
router.post('/pods', phase4RateLimit, agentRuntimeAuth, async (req: any, res: any) => {
22912312
try {
22922313
const agentUser = req.agentUser;
22932314
if (!agentUser) {

0 commit comments

Comments
 (0)