diff --git a/backend/__tests__/services/dmService.test.ts b/backend/__tests__/services/dmService.test.ts index a8248267ed..b04a059cb0 100644 --- a/backend/__tests__/services/dmService.test.ts +++ b/backend/__tests__/services/dmService.test.ts @@ -150,4 +150,61 @@ describe('DMService — agent rooms', () => { expect(room.name).toBe('tarik'); }); + it.each(['nova', 'Nova [reviewer]'])('reuses the agent-admin pod created with resolved label %s', async (label) => { + agentUser.botMetadata = { agentName: 'openclaw', instanceId: 'nova', displayName: label === 'nova' ? 'openclaw (nova)' : label }; + await agentUser.save(); + const options = { agentName: 'openclaw', instanceId: 'nova' }; + const first = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, options); + const second = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, options); + expect(first.name).toBe(`DM: ${label}`); + expect(second._id.toString()).toBe(first._id.toString()); + expect(await Pod.countDocuments({ type: 'agent-admin' })).toBe(1); + expect(second.members.map(String).sort()).toEqual([agentUser._id, humanUser._id].map(String).sort()); + }); + + it.each(['Supernova', 'nova notes'])('never adds an agent to the unrelated owner-only DM %s', async (otherLabel) => { + agentUser.botMetadata = { agentName: 'openclaw', instanceId: 'nova', displayName: 'openclaw (nova)' }; + await agentUser.save(); + const unrelated = await Pod.create({ name: `DM: ${otherLabel}`, description: `Debug channel for ${otherLabel}`, + type: 'agent-admin', createdBy: humanUser._id, members: [humanUser._id], joinPolicy: 'invite-only' }); + const room = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, { agentName: 'openclaw', instanceId: 'nova' }); + expect(room._id.toString()).not.toBe(unrelated._id.toString()); + const stored = await Pod.findById(unrelated._id); + expect(stored.name).toBe(`DM: ${otherLabel}`); + expect(stored.members.map(String)).toEqual([humanUser._id.toString()]); + }); + + it('reuses a punctuation-ending description after the pod name is edited', async () => { + agentUser.botMetadata = { agentName: 'openclaw', instanceId: 'nova', displayName: 'Nova [reviewer]' }; + await agentUser.save(); + const first = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, { agentName: 'openclaw', instanceId: 'nova' }); + first.name = 'Private review notes'; + await first.save(); + const second = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, { agentName: 'openclaw', instanceId: 'nova' }); + expect(second._id.toString()).toBe(first._id.toString()); + expect(await Pod.countDocuments({ type: 'agent-admin' })).toBe(1); + }); + + it.each(['nova', 'openclaw (nova)'])('never repairs an owner-only pod from an ambiguous exact label %s', async (label) => { + agentUser.botMetadata = { agentName: 'openclaw', instanceId: 'nova', displayName: 'openclaw (nova)' }; + await agentUser.save(); + const unrelated = await Pod.create({ name: `DM: ${label}`, description: `Debug channel for ${label}`, + type: 'agent-admin', createdBy: humanUser._id, members: [humanUser._id], joinPolicy: 'invite-only' }); + const room = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, { agentName: 'openclaw', instanceId: 'nova' }); + expect(room._id.toString()).not.toBe(unrelated._id.toString()); + expect((await Pod.findById(unrelated._id)).members.map(String)).toEqual([humanUser._id.toString()]); + expect(room.members.map(String).sort()).toEqual([agentUser._id, humanUser._id].map(String).sort()); + }); + + it('reuses a legacy composite label only when the requested agent is already a member', async () => { + agentUser.botMetadata = { agentName: 'legacy-runtime', instanceId: 'nova', displayName: 'legacy-runtime (nova)' }; + await agentUser.save(); + const legacy = await Pod.create({ name: 'Private notes', description: 'Debug channel for legacy-runtime (nova)', + type: 'agent-admin', createdBy: humanUser._id, members: [humanUser._id, agentUser._id], joinPolicy: 'invite-only' }); + const room = await DMService.getOrCreateAgentDM(agentUser._id, humanUser._id, { agentName: 'openclaw', instanceId: 'nova' }); + expect(room._id.toString()).toBe(legacy._id.toString()); + expect(room.members.map(String).sort()).toEqual([agentUser._id, humanUser._id].map(String).sort()); + expect(await Pod.countDocuments({ type: 'agent-admin' })).toBe(1); + }); + }); diff --git a/backend/__tests__/unit/middleware/dualAuth.test.js b/backend/__tests__/unit/middleware/dualAuth.test.js new file mode 100644 index 0000000000..73f6f76432 --- /dev/null +++ b/backend/__tests__/unit/middleware/dualAuth.test.js @@ -0,0 +1,60 @@ +const rateLimit = require('express-rate-limit'); +const express = require('express'); +const request = require('supertest'); + +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { + const token = req.header('Authorization')?.replace('Bearer ', '') || req.header('x-auth-token'); + if (token !== 'valid-user') { + return res.status(401).json({ verifier: 'human' }); + } + req.userId = 'person-1'; + return next(); +})); +jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => { + const token = req.header('Authorization')?.replace('Bearer ', '').trim() || req.header('x-commonly-agent-token'); + if (token !== 'cm_agent_valid') return res.status(401).json({ verifier: 'agent' }); + req.agentUser = { _id: 'agent-1' }; + return next(); +})); +const dualAuth = require('../../../middleware/dualAuth'); +const humanAuth = require('../../../middleware/auth'); +const agentAuth = require('../../../middleware/agentRuntimeAuth'); +const app = express(); +// Keep even this unserved HTTP fixture behind the normal limiter-before-auth order. +const limiter = rateLimit({ windowMs: 60000, max: 100 }); +app.get('/', limiter, dualAuth, (req, res) => res.json({ human: req.userId || null, agent: req.agentUser?._id || null })); +beforeEach(() => jest.clearAllMocks()); + +test.each([ + [{ Authorization: 'Bearer cm_agent_valid' }, 200, 'agent'], + [{ 'x-commonly-agent-token': 'cm_agent_valid' }, 200, 'agent'], + [{ Authorization: 'Bearer valid-user' }, 200, 'human'], + [{ 'x-auth-token': 'valid-user' }, 200, 'human'], + [{ 'x-auth-token': 'cm_agent_valid' }, 401, 'human'], + [{ 'x-commonly-agent-token': 'invalid', 'x-auth-token': 'valid-user' }, 200, 'human'], + [{ Authorization: 'Bearer invalid', 'x-auth-token': 'valid-user' }, 401, 'human'], + [{ Authorization: 'Bearer valid-user', 'x-commonly-agent-token': 'cm_agent_valid' }, 200, 'human'], + [{ Authorization: 'Bearer cm_agent_revoked', 'x-auth-token': 'valid-user' }, 401, 'agent'], + [{ Authorization: 'Bearer invalid', 'x-commonly-agent-token': 'cm_agent_valid' }, 401, 'human'], + [{ 'x-commonly-agent-token': 'cm_daemon_invalid' }, 401, 'human'], + [{ 'x-commonly-agent-token': 'cm_agent_revoked' }, 401, 'agent'], + [{}, 401, 'human'], +])('selects one verifier and preserves refusal for %j', async (headers, status, verifier) => { + const res = await request(app).get('/').set(headers); + expect(res.status).toBe(status); + expect(humanAuth).toHaveBeenCalledTimes(verifier === 'human' ? 1 : 0); + expect(agentAuth).toHaveBeenCalledTimes(verifier === 'agent' ? 1 : 0); + if (status === 200) expect(res.body).toEqual(verifier === 'agent' + ? { human: null, agent: 'agent-1' } : { human: 'person-1', agent: null }); +}); + +test.each([ + [{ Authorization: 'Bearer valid-user', 'x-commonly-agent-token': 'cm_agent_valid' }, 'human', 'valid-user'], + [{ 'x-commonly-agent-token': 'cm_agent_valid', 'x-auth-token': 'spoof-a' }, 'agent', 'cm_agent_valid'], + [{ 'x-commonly-agent-token': 'cm_agent_valid', 'x-auth-token': 'spoof-b' }, 'agent', 'cm_agent_valid'], + [{ 'x-commonly-agent-token': 'not-a-runtime-token', 'x-auth-token': 'valid-user' }, 'human', 'valid-user'], + [{ 'x-auth-token': 'cm_agent_valid' }, 'human', 'cm_agent_valid'], +])('the limiter keys the selected verifier credential for %j', (headers, verifier, token) => { + const { resolveDualAuthCredential } = require('../../../middleware/dualAuth'); + expect(resolveDualAuthCredential({ header: (name) => headers[name] })).toEqual({ verifier, token }); +}); diff --git a/backend/__tests__/unit/routes/agentsRuntime.podCreateDedup.test.js b/backend/__tests__/unit/routes/agentsRuntime.podCreateDedup.test.js index c4b93b5205..4d00e15a1b 100644 --- a/backend/__tests__/unit/routes/agentsRuntime.podCreateDedup.test.js +++ b/backend/__tests__/unit/routes/agentsRuntime.podCreateDedup.test.js @@ -242,6 +242,8 @@ describe('POST /pods dedup refuses to widen a 1:1 DM (ADR-001 §3.10)', () => { // FOUND — its type or its id — since that is what the caller is fishing // for and did not already know. const body = JSON.stringify(dm.body); + expect(body).toContain('commonly_open_dm'); + expect(body).toContain('commonly_dm_agent'); expect(body).not.toMatch(/agent-dm|agent-room|pod-agent-dm/); }); diff --git a/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js index b72ed70d23..55e7034e9a 100644 --- a/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js +++ b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js @@ -13,16 +13,16 @@ * configuration and signs locally; it must keep its honest local 500. */ -jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { +jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => { req.agentUser = { _id: 'bot-1' }; next(); -}); +})); -jest.mock('../../../middleware/auth', () => (req, res, next) => { +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { req.user = { _id: 'user-1', role: 'member' }; req.userId = 'user-1'; next(); -}); +})); jest.mock('../../../services/githubAppService', () => ({ isPatConfigured: jest.fn(), @@ -188,3 +188,15 @@ describe('routes that lent out the server GitHub credential stay removed', () => ); }); }); + +test('GitHub issue reads accept the alternate runtime header without human auth', async () => { + const agentAuth = require('../../../middleware/agentRuntimeAuth'); + const humanAuth = require('../../../middleware/auth'); + jest.clearAllMocks(); + GitHubAppService.isConfigured.mockReturnValue(true); + GitHubAppService.listOpenIssues.mockResolvedValue([]); + const res = await request(app).get('/api/github/issues').set('x-commonly-agent-token', 'cm_agent_valid'); + expect(res.status).toBe(200); + expect(agentAuth).toHaveBeenCalledTimes(1); + expect(humanAuth).not.toHaveBeenCalled(); +}); diff --git a/backend/__tests__/unit/routes/grants.read.test.js b/backend/__tests__/unit/routes/grants.read.test.js index 61887cbd46..f0b7ebcb27 100644 --- a/backend/__tests__/unit/routes/grants.read.test.js +++ b/backend/__tests__/unit/routes/grants.read.test.js @@ -540,3 +540,23 @@ describe('GET /api/grants/:grantId — the seat confinement refusal (TASK-063)', expect((await read(row.grantId)).body.grantBrokerRefusalScope).toBe('seat'); }); }); + +test.each(['human', 'dual', 'agent'])('grant %s routes budget the actual credential despite unrelated header rotation', async (mode) => { + const credential = `${mode === 'human' ? 'user' : 'cm_agent'}_budget_${mode}`; + const send = (index, token = credential) => { + if (mode === 'human') return request(app).post('/api/grants').set('x-test-user', OWNER) + .set('x-auth-token', token).set('x-commonly-agent-token', `cm_agent_unrelated_${index}`).send({}); + const path = mode === 'agent' ? '/api/grants/missing/attenuate' : '/api/grants/missing'; + const req = mode === 'agent' ? request(app).post(path) : request(app).get(path); + return req.set('x-test-agent', SEAT).set('x-commonly-agent-token', token).set('x-auth-token', `unrelated_${index}`); + }; + const expected = 404; // Authenticated handler refuses the deliberately missing connection/grant. + for (let index = 0; index < 60; index += 1) expect((await send(index)).status).toBe(expected); + expect((await send(60)).status).toBe(429); + expect((await send(61, `${credential}_other`)).status).toBe(expected); + // A read through the dual dispatcher spends the same budget as the write path. + const read = request(app).get('/api/grants/missing'); + if (mode === 'human') read.set('x-test-user', OWNER).set('x-auth-token', credential); + else read.set('x-test-agent', SEAT).set('x-commonly-agent-token', credential); + expect((await read).status).toBe(429); +}); diff --git a/backend/__tests__/unit/routes/tasksApi.queryCoercion.test.js b/backend/__tests__/unit/routes/tasksApi.queryCoercion.test.js index 2865c52834..cfa500f745 100644 --- a/backend/__tests__/unit/routes/tasksApi.queryCoercion.test.js +++ b/backend/__tests__/unit/routes/tasksApi.queryCoercion.test.js @@ -15,12 +15,12 @@ const request = require('supertest'); const express = require('express'); -jest.mock('../../../middleware/auth', () => (req, res, next) => { +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { req.userId = 'u1'; req.user = { id: 'u1', _id: 'u1' }; next(); -}); -jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => next()); +})); +jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => { req.agentUser = { _id: 'u1' }; next(); })); jest.mock('../../../models/Pod', () => ({ findById: jest.fn(() => ({ @@ -56,6 +56,29 @@ describe('GET /api/v1/tasks/:podId — query coercion at the boundary', () => { app.use('/api/v1/tasks', tasksApiRoutes); }); + test('task reads accept the alternate runtime header without human auth', async () => { + const agentAuth = require('../../../middleware/agentRuntimeAuth'); + const humanAuth = require('../../../middleware/auth'); + const res = await request(app).get(`/api/v1/tasks/${POD}`).set('x-commonly-agent-token', 'cm_agent_valid'); + expect(res.status).toBe(200); + expect(agentAuth).toHaveBeenCalledTimes(1); + expect(humanAuth).not.toHaveBeenCalled(); + }); + + test('an alternate runtime token keeps one write budget while x-auth-token rotates', async () => { + for (let i = 0; i < 20; i += 1) { + const response = await request(app).post(`/api/v1/tasks/${POD}`) + .set('x-commonly-agent-token', 'cm_agent_budget').set('x-auth-token', `unrelated-${i}`).send({}); + expect(response.status).toBe(400); // Missing title, after the real limiter/auth chain. + } + const refused = await request(app).post(`/api/v1/tasks/${POD}`) + .set('x-commonly-agent-token', 'cm_agent_budget').set('x-auth-token', 'another-unused-header').send({}); + expect(refused.status).toBe(429); + const control = await request(app).post(`/api/v1/tasks/${POD}`) + .set('x-commonly-agent-token', 'cm_agent_other_budget').set('x-auth-token', 'unrelated-0').send({}); + expect(control.status).toBe(400); + }); + // Domain guard, by the rule these tests are an instance of: every assertion // below is negative-form (`not.toHaveProperty`), and a negative passes on a // subject that does not exist. Asserting the query is well-formed FIRST means diff --git a/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js b/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js index 8b25bf4b1c..b4876a77b8 100644 --- a/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js @@ -108,3 +108,9 @@ describe('resolveAgentDisplayLabel', () => { }); }); }); + +it('preserves a curated default-instance name equal to the agent name', () => { + expect(resolveAgentDisplayLabel({ username: 'pixel-default', botMetadata: { + agentName: 'pixel', instanceId: 'default', displayName: 'Pixel', + } })).toBe('Pixel'); +}); diff --git a/backend/__tests__/unit/services/agentMessageService.threadWrites.test.js b/backend/__tests__/unit/services/agentMessageService.threadWrites.test.js index d03790d564..d4da832587 100644 --- a/backend/__tests__/unit/services/agentMessageService.threadWrites.test.js +++ b/backend/__tests__/unit/services/agentMessageService.threadWrites.test.js @@ -431,6 +431,17 @@ describe('agent in-thread posts (threadRootId, no reply edge)', () => { }); }); +test('a DM membership refusal names both runtime and MCP tools without posting', async () => { + AgentIdentityService.ensureAgentInPod.mockResolvedValueOnce(null); + const Pod = require('../../../models/Pod'); + Pod.findById.mockReturnValueOnce({ select: () => ({ lean: async () => ({ type: 'agent-dm' }) }) }); + await expect(AgentMessageService.postMessage({ agentName: 'lark', podId: POD, content: 'hello' })).rejects.toMatchObject({ + code: 'dm_membership_refused', statusCode: 403, + message: expect.stringContaining('commonly_open_dm or commonly_dm_agent'), + }); + expect(PGMessage.create).not.toHaveBeenCalled(); + expect(emitted).toEqual([]); +}); test('server run identity survives PG storage and live message broadcast with existing card fields', async () => { const runId = 'aaaaaaaaaaaaaaaaaaaaa111'; diff --git a/backend/__tests__/unit/services/dmService.displayLabel.test.js b/backend/__tests__/unit/services/dmService.displayLabel.test.js new file mode 100644 index 0000000000..f66ad2d90f --- /dev/null +++ b/backend/__tests__/unit/services/dmService.displayLabel.test.js @@ -0,0 +1,40 @@ +const mockUser = { value: null }; +jest.mock('../../../models/User', () => ({ + findById: () => ({ select: () => ({ lean: async () => mockUser.value }) }), +})); +jest.mock('../../../models/Pod', () => { + const Pod = jest.fn(function Pod(data) { + Object.assign(this, data, { _id: 'room-1', save: async () => {} }); + }); + Pod.findOne = jest.fn().mockResolvedValue(null); + Pod.find = () => ({ sort: () => ({ limit: async () => [] }) }); + return Pod; +}); +jest.mock('../../../models/pg/Pod', () => null); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { findOne: () => ({ sort: () => ({ lean: async () => null }) }), install: jest.fn() }, +})); +const DMService = require('../../../services/dmService'); +const { AgentInstallation } = require('../../../models/AgentRegistry'); + +test.each([ + ['openclaw (nova)', 'nova'], ['OpenClaw (NOVA)', 'nova'], ['openclaw', 'nova'], + ['Nova, the reviewer', 'Nova, the reviewer'], +])('room and installation names resolve %s as %s', async (displayName, expected) => { + mockUser.value = { username: 'openclaw-nova', botMetadata: { agentName: 'openclaw', instanceId: 'nova', displayName } }; + const room = await DMService.getOrCreateAgentRoom('agent-1', 'person-1', { agentName: 'openclaw', instanceId: 'nova' }); + expect(room.name).toBe(expected); + expect(AgentInstallation.install).toHaveBeenLastCalledWith('openclaw', 'room-1', expect.objectContaining({ displayName: expected })); +}); + +test('the agent-admin DM creation path uses the same runtime-leak guard', async () => { + mockUser.value = { username: 'openclaw-nova', botMetadata: { agentName: 'openclaw', instanceId: 'nova', displayName: 'openclaw (nova)' } }; + const room = await DMService.getOrCreateAgentDM('agent-1', 'person-1', { agentName: 'openclaw', instanceId: 'nova' }); + expect(room.name).toBe('DM: nova'); + expect(room.description).toBe('Debug channel for nova'); +}); +test('a curated default-instance label is preserved in a new room', async () => { + mockUser.value = { username: 'pixel-default', botMetadata: { agentName: 'pixel', instanceId: 'default', displayName: 'Pixel' } }; + const room = await DMService.getOrCreateAgentRoom('agent-1', 'person-1', { agentName: 'pixel', instanceId: 'default' }); + expect(room.name).toBe('Pixel'); +}); diff --git a/backend/middleware/dualAuth.ts b/backend/middleware/dualAuth.ts new file mode 100644 index 0000000000..ff8445e75f --- /dev/null +++ b/backend/middleware/dualAuth.ts @@ -0,0 +1,25 @@ +import type { Request, Response, NextFunction } from 'express'; +import auth from './auth'; +import agentRuntimeAuth from './agentRuntimeAuth'; + +/** The chosen credential also keys rate limits. An unrelated header cannot + * rotate the bucket while a runtime token continues to authenticate. */ +export function resolveDualAuthCredential( + req: Pick, +): { verifier: 'agent' | 'human'; token: string } { + const bearer = ((req.header?.('Authorization') || '').replace('Bearer ', '')).trim(); + const altHeader = (req.header?.('x-commonly-agent-token') || '').trim(); + const token = bearer || altHeader; + if (token.startsWith('cm_agent_')) return { verifier: 'agent', token }; + // x-auth-token remains a human-verifier input, even if its text starts cm_agent_. + return { verifier: 'human', token: bearer || req.header?.('x-auth-token') || '' }; +} + +/** Select once; refusal never falls through to another identity's verifier. */ +export default function dualAuth(req: Request, res: Response, next: NextFunction) { + if (resolveDualAuthCredential(req).verifier === 'agent') return agentRuntimeAuth(req, res, next); + return auth(req, res, next); +} +// CJS compatibility for the existing route modules. +// eslint-disable-next-line @typescript-eslint/no-require-imports +module.exports = exports.default; Object.assign(module.exports, exports); diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 129a7c8151..13f6a56948 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1,3 +1,4 @@ +import dualAuth from '../middleware/dualAuth'; import { validateAgentCardPayload } from '../services/uiCardValidation'; export {}; @@ -148,25 +149,8 @@ const longPollIpLimit = platformIpRateLimit({ label: '3000 long-poll reads per 60s per IP', }); -// Dual-auth dispatcher (mirrors `backend/routes/tasksApi.ts:34-36`). Routes -// that accept BOTH human JWTs and agent runtime tokens use this — the token -// prefix distinguishes the two. `cm_agent_*` → `agentRuntimeAuth` (stamps -// `req.agentUser`), anything else → `auth` (stamps `req.userId`/`req.user`). -// -// Both `Authorization: Bearer ` and `x-commonly-agent-token: ` -// are checked because `agentRuntimeAuth` accepts either; missing one would -// silently route the alternate-header caller to the human path and 401. -// -// Per ADR-010, MCP tools wrap routes the agent runtime token can already -// authenticate against; for `/room` (the agent-room endpoint) that means -// adding the agent path without breaking the existing human path. -const dualAuth = (req: any, res: any, next: any) => { - const bearer = ((req.header?.('Authorization') || '').replace('Bearer ', '')).trim(); - const altHeader = (req.header?.('x-commonly-agent-token') || '').trim(); - const token = bearer || altHeader; - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return auth(req, res, next); -}; +// /room accepts human sessions and runtime credentials through shared dualAuth. +// Keep the IP limiter before it: both verification paths read the database. const Integration = require('../models/Integration'); const AgentMemory = require('../models/AgentMemory'); @@ -3344,7 +3328,7 @@ router.post('/pods', phase4RateLimit, agentRuntimeAuth, async (req: any, res: an code: 'pod_name_unavailable', message: 'That pod name is already taken by a pod you cannot join. Pick a ' + 'different name. To start a private conversation with a specific agent, ' - + 'use commonly_open_dm instead of creating a pod by name.', + + 'use commonly_open_dm or commonly_dm_agent instead of creating a pod by name.', }); }; diff --git a/backend/routes/artifacts.ts b/backend/routes/artifacts.ts index b6943b6235..56109ccba5 100644 --- a/backend/routes/artifacts.ts +++ b/backend/routes/artifacts.ts @@ -20,8 +20,7 @@ import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { ARTIFACT_KINDS, listArtifacts } from '../services/artifactService'; const express = require('express'); -const auth = require('../middleware/auth'); -const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); +const dualAuth = require('../middleware/dualAuth'); const Pod = require('../models/Pod'); const DMService = require('../services/dmService'); @@ -43,14 +42,6 @@ const artifactsRateLimit = rateLimit({ handler: (_req: unknown, res: Res) => res.status(429).json({ code: 'rate_limited' }), }); -const dualAuth = (req: any, res: any, next: any) => { - const bearer = ((req.header?.('Authorization') || '').replace('Bearer ', '')).trim(); - const altHeader = (req.header?.('x-commonly-agent-token') || '').trim(); - const token = bearer || altHeader; - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return auth(req, res, next); -}; - // Two scope resolvers, one read. async function resolveScope(req: AuthReq): Promise { if (req.agentUser) { diff --git a/backend/routes/github.ts b/backend/routes/github.ts index e20aef7d5a..2332841bb5 100644 --- a/backend/routes/github.ts +++ b/backend/routes/github.ts @@ -3,7 +3,7 @@ const express = require('express'); import rateLimit from 'express-rate-limit'; import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require -const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); +const dualAuth = require('../middleware/dualAuth'); // eslint-disable-next-line global-require const auth = require('../middleware/auth'); // eslint-disable-next-line global-require @@ -124,12 +124,6 @@ function mapGitHubUpstreamError( return { status: 500, body: { error: labels.fallback, code: 'github_proxy_error', retryable: false, detail } }; } -function anyAuth(req: AuthReq, res: Res, next: () => void) { - const token = ((req.header?.('Authorization') || '').replace('Bearer ', '')); - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return auth(req, res, next); -} - const router: ReturnType = express.Router(); // REMOVED — `POST /token`, which handed our GitHub credential to callers. @@ -165,7 +159,7 @@ router.get('/status', auth, async (req: AuthReq, res: Res) => { } }); -router.get('/issues', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res) => { +router.get('/issues', githubIpRateLimit, dualAuth, async (req: AuthReq, res: Res) => { try { if (!GitHubAppService.isPatConfigured() && !GitHubAppService.isConfigured()) { return res.status(503).json({ error: 'No GitHub credentials configured' }); @@ -180,7 +174,7 @@ router.get('/issues', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res) } }); -router.post('/issues', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res) => { +router.post('/issues', githubIpRateLimit, dualAuth, async (req: AuthReq, res: Res) => { try { if (!GitHubAppService.isPatConfigured() && !GitHubAppService.isConfigured()) { return res.status(503).json({ error: 'No GitHub credentials configured' }); @@ -196,7 +190,7 @@ router.post('/issues', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res } }); -router.post('/issues/:number/comment', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res) => { +router.post('/issues/:number/comment', githubIpRateLimit, dualAuth, async (req: AuthReq, res: Res) => { try { const issueNumber = Number(req.params?.number); const { body } = (req.body || {}) as { body?: string }; @@ -210,7 +204,7 @@ router.post('/issues/:number/comment', githubIpRateLimit, anyAuth, async (req: A } }); -router.post('/issues/:number/close', githubIpRateLimit, anyAuth, async (req: AuthReq, res: Res) => { +router.post('/issues/:number/close', githubIpRateLimit, dualAuth, async (req: AuthReq, res: Res) => { try { const issueNumber = Number(req.params?.number); const { comment } = (req.body || {}) as { comment?: string }; diff --git a/backend/routes/grants.ts b/backend/routes/grants.ts index b3c36750cc..3bce56061e 100644 --- a/backend/routes/grants.ts +++ b/backend/routes/grants.ts @@ -1,3 +1,4 @@ +import dualAuth, { resolveDualAuthCredential } from '../middleware/dualAuth'; import express from 'express'; import rateLimit from 'express-rate-limit'; import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; @@ -39,13 +40,23 @@ const router = express.Router(); // The trail is read by the people in the room and by the seat it was granted // to, so the calls route takes either identity — the shape reactions and // artifacts use. -const dualAuth = (req: any, res: any, next: any) => { - const bearer = ((req.header?.('Authorization') || '').replace('Bearer ', '')).trim(); - const altHeader = (req.header?.('x-commonly-agent-token') || '').trim(); - const token = bearer || altHeader; - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return auth(req, res, next); + +// The same budget spans human-only, agent-only and dual-auth operations. +// Bind the route's actual credential before the limiter; no verifier/DB work runs here. +const grantLimiterCredential = Symbol('grantLimiterCredential'); +type GrantLimitRequest = express.Request & { [grantLimiterCredential]?: string }; +const bindGrantLimiterCredential = ( + select: (req: express.Request) => string | undefined, +): express.RequestHandler => (req, _res, next) => { + (req as GrantLimitRequest)[grantLimiterCredential] = select(req) || ''; + next(); }; +const dualGrantCredential = bindGrantLimiterCredential((req) => resolveDualAuthCredential(req).token); +const agentGrantCredential = bindGrantLimiterCredential((req) => { + const authorization = req.get('Authorization'); + return authorization?.startsWith('Bearer ') + ? authorization.replace('Bearer ', '').trim() : req.get('x-commonly-agent-token'); +}); const grantRateLimit = rateLimit({ windowMs: 60_000, @@ -53,7 +64,8 @@ const grantRateLimit = rateLimit({ standardHeaders: true, legacyHeaders: false, keyGenerator: (req: express.Request): string => { - const authHeader = req.get('Authorization') || req.get('x-auth-token'); + const authHeader = (req as GrantLimitRequest)[grantLimiterCredential] + ?? ((req.get('Authorization') || '').replace('Bearer ', '').trim() || req.get('x-auth-token')); if (authHeader) return `grant:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; return cloudflareIpRateLimitKeyGenerator(req as never); }, @@ -435,6 +447,7 @@ router.post('/', grantRateLimit, auth, async (req: AuthenticatedRequest, res: ex /** Agent-only delegation; all parent capabilities are loaded server-side. */ router.post( '/:grantId/attenuate', + agentGrantCredential, grantRateLimit, agentRuntimeAuth, async (req: AuthenticatedRequest, res: express.Response) => { @@ -534,7 +547,7 @@ const gateGrantRead = async ( * outcome. A line carries `argsDigest`, never the arguments — the broker never * stored them (ToolCall.digestArgs). */ -router.get('/:grantId/calls', grantRateLimit, dualAuth, async (req: AuthenticatedRequest, res: express.Response) => { +router.get('/:grantId/calls', dualGrantCredential, grantRateLimit, dualAuth, async (req: AuthenticatedRequest, res: express.Response) => { try { const grant = await RoomGrant.findOne({ grantId: req.params.grantId }).lean(); if (!grant) return res.status(404).json({ error: 'grant_not_found' }); @@ -574,7 +587,7 @@ router.get('/:grantId/calls', grantRateLimit, dualAuth, async (req: Authenticate // Read is gated exactly as the trail is, and projected: the explicit field // list and nothing else — never connectionId, brokerId or the raw audience. -router.get('/:grantId', grantRateLimit, dualAuth, async (req: AuthenticatedRequest, res: express.Response) => { +router.get('/:grantId', dualGrantCredential, grantRateLimit, dualAuth, async (req: AuthenticatedRequest, res: express.Response) => { try { const grant = await RoomGrant.findOne({ grantId: req.params.grantId }).lean(); if (!grant) return res.status(404).json({ error: 'grant_not_found' }); diff --git a/backend/routes/messages.ts b/backend/routes/messages.ts index f70b2d682e..c354fe9abc 100644 --- a/backend/routes/messages.ts +++ b/backend/routes/messages.ts @@ -8,7 +8,7 @@ const express = require('express'); // eslint-disable-next-line global-require const auth = require('../middleware/auth'); // eslint-disable-next-line global-require -const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); +const dualAuth = require('../middleware/dualAuth'); // eslint-disable-next-line global-require const { getMessages, @@ -126,13 +126,7 @@ const reactionRateLimit = rateLimit({ // Agents reacting is first-class — see ADR-006 + reactionController's // agentUser fallback. Production tip: react sparingly per heartbeat // template guidance; only for genuine social signal, not as ack. -const dualAuth = (req: any, res: any, next: any) => { - const bearer = ((req.header?.('Authorization') || '').replace('Bearer ', '')).trim(); - const altHeader = (req.header?.('x-commonly-agent-token') || '').trim(); - const token = bearer || altHeader; - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return auth(req, res, next); -}; + router.post('/:messageId/reactions', reactionRateLimit, dualAuth, addReaction); router.delete('/:messageId/reactions/:emoji', reactionRateLimit, dualAuth, removeReaction); diff --git a/backend/routes/tasksApi.ts b/backend/routes/tasksApi.ts index c4749addab..671ac72b97 100644 --- a/backend/routes/tasksApi.ts +++ b/backend/routes/tasksApi.ts @@ -8,9 +8,8 @@ const express = require('express'); // eslint-disable-next-line global-require const mongoose = require('mongoose'); // eslint-disable-next-line global-require -const regularAuth = require('../middleware/auth'); -// eslint-disable-next-line global-require -const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); +const auth = require('../middleware/dualAuth'); +const { resolveDualAuthCredential } = auth; // eslint-disable-next-line global-require const Pod = require('../models/Pod'); // eslint-disable-next-line global-require @@ -97,12 +96,6 @@ interface Res { json: (d: unknown) => void; } -function auth(req: AuthReq, res: Res, next: () => void) { - const token = ((req.header?.('Authorization') || '').replace('Bearer ', '')); - if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); - return regularAuth(req, res, next); -} - const router: ReturnType = express.Router(); // A pending unassigned row is offered again only when updatedAt advances past @@ -152,7 +145,7 @@ const updateTaskWithRevision = async ( ) => (await updateTaskWithRevisionSnapshot(filter, update))?.task || null; const taskWriteRateLimitKey = (req: Request): string => { - const authHeader = req.get('Authorization') || req.get('x-auth-token'); + const authHeader = resolveDualAuthCredential(req).token; if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 0d3d947d79..39c0fffb77 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -43,64 +43,8 @@ try { // invariant keeps the rule visible to anyone reading that function. export const DM_POD_TYPES_GUARD = new Set(['agent-room', 'agent-dm']); -// Resolve the human-readable label for an agent User row. The fallback -// chain is intentional and load-bearing for any UI / pod-naming surface -// that shows agent identity (agent-dm pod names, sidebar member rows): -// -// 1. `botMetadata.displayName` — the curated label the registry/preset -// sets ("Pixel", "Strategist (Aria)"). This is what humans actually -// recognize. -// 2. `botMetadata.instanceId` — when displayName is missing AND the -// instanceId carries identity (i.e. != 'default'). For OpenClaw-driven -// agents the User row stores `agentName: 'openclaw'` (the RUNTIME) + -// `instanceId: 'aria' | 'pixel' | ...` (the identity), so falling back -// to instanceId here beats falling back to the runtime label. -// 3. `username` — last-resort identifier; always set. -// 4. fallback string — for the non-User case (e.g. resolved- -// by-alias before the User row is loaded). -// -// Why NOT fall back to `botMetadata.agentName`: that field can be the -// runtime name ('openclaw') rather than the agent's identity. Falling -// back to it produces "openclaw ↔ openclaw" pod names where what we want -// is "Pixel ↔ Strategist (Aria)". -export function resolveAgentDisplayLabel( - user: { - username?: string; - botMetadata?: { displayName?: string; instanceId?: string; agentName?: string }; - } | null | undefined, - fallback?: string, -): string { - const safeFallback = fallback || 'agent'; - if (!user) return safeFallback; - const meta = user.botMetadata; - const display = meta?.displayName?.trim(); - const agentName = meta?.agentName?.trim() || ''; - const instanceId = meta?.instanceId?.trim() || ''; - // Leak-pattern detection. If displayName is literally ` ()` - // (e.g. "openclaw (nova)"), it's the runtime label leaking through — some - // historical writer formatted displayName as `${agentName} (${instanceId})` - // instead of using the curated label. Treat these as if displayName were - // missing and fall through to instanceId. Matches both the - // ` ()` form and the bare `` form - // (e.g. displayName === 'openclaw'). Also catches the case where - // displayName equals just instanceId — the chain below would render the - // same thing, but we drop the redundant pass here. - const leakedPattern = ( - !!display - && !!agentName - && ( - display.toLowerCase() === agentName.toLowerCase() - || ( - !!instanceId - && display.toLowerCase() === `${agentName.toLowerCase()} (${instanceId.toLowerCase()})` - ) - ) - ); - if (display && !leakedPattern) return display; - if (instanceId && instanceId !== 'default') return instanceId; - if (user.username) return user.username; - return safeFallback; -} +// Re-export for existing callers; the pure helper is safe for DM naming too. +export { resolveAgentDisplayLabel } from '../utils/agentDisplayLabel'; const normalizeSegment = (value: unknown): string => ( (String(value || '')).toLowerCase().replace(/[^a-z0-9-]/g, '').slice(0, 40) diff --git a/backend/services/agentMessageService.ts b/backend/services/agentMessageService.ts index 7fcd6759d8..1fa51684d1 100644 --- a/backend/services/agentMessageService.ts +++ b/backend/services/agentMessageService.ts @@ -1298,7 +1298,7 @@ class AgentMessageService { } // It's a 1:1 DM and this agent isn't a member. const err: Error & { code?: string; statusCode?: number } = new Error( - `Agent is not a member of ${podDoc.type} pod ${podId} — DM pods are 1:1 (ADR-001 §3.10). Open a new DM via commonly_open_dm if a private channel with this peer is needed.`, + `Agent is not a member of ${podDoc.type} pod ${podId} — DM pods are 1:1 (ADR-001 §3.10). Open a new DM via commonly_open_dm or commonly_dm_agent if a private channel with this peer is needed.`, ); err.code = 'dm_membership_refused'; err.statusCode = 403; diff --git a/backend/services/dmService.ts b/backend/services/dmService.ts index 4f3114ab9c..2590163a5f 100644 --- a/backend/services/dmService.ts +++ b/backend/services/dmService.ts @@ -1,3 +1,4 @@ +import { resolveAgentDisplayLabel } from '../utils/agentDisplayLabel'; import Pod from '../models/Pod'; import User from '../models/User'; import { AgentInstallation } from '../models/AgentRegistry'; @@ -60,26 +61,6 @@ interface AgentDmUpsertResult { }; } -// Mirror of agentIdentityService.resolveAgentDisplayLabel — duplicated here -// to avoid a service-to-service import cycle (dmService is required by -// agentMessageService, which is required by agentIdentityService callers). -// Stays in sync with the same fallback chain: botMetadata.displayName → -// instanceId (when not 'default') → username → fallback. Never falls back -// to botMetadata.agentName (runtime-leaning, "openclaw" for OpenClaw-driven -// agents). Producing pod names like "openclaw (aria)" is the bug this -// avoids — the agent's heartbeat surfaces those names back into chat. -const resolveAgentDisplayLabel = ( - user: { username?: string; botMetadata?: { displayName?: string; instanceId?: string } } | null | undefined, - fallback: string, -): string => { - if (!user) return fallback; - const meta = user.botMetadata || {}; - const display = meta.displayName?.trim(); - if (display) return display; - const instanceId = meta.instanceId?.trim(); - if (instanceId && instanceId !== 'default') return instanceId; - return user.username || fallback; -}; class DMService { static agentDmPairKey(a: unknown, b: unknown): string { @@ -121,25 +102,6 @@ class DMService { return dmPod; } - static async isCompatibleStalePod(dmPod: InstanceType, ownerId: unknown, agentId: unknown): Promise { - const ownerIdStr = String(ownerId); - const agentIdStr = String(agentId); - const otherMemberIds = (dmPod.members || []) - .map((member: unknown) => String((member as unknown as Record)?._id || member)) - .filter((id: string) => id !== ownerIdStr); - - if (!otherMemberIds.length) return true; - - const botMembers = await User.find({ - _id: { $in: otherMemberIds }, - isBot: true, - }).select('_id').lean(); - - if (!botMembers.length) return true; - - return botMembers.some((member) => String((member as unknown as Record)._id) === agentIdStr); - } - /** * Find or create a private agent-admin pod between an agent user and its * installer (the owner who installed the agent into a pod). Agent-admin is @@ -149,57 +111,46 @@ class DMService { const agentId = String(agentUserId); const ownerId = String(ownerUserId); + const agentUser = await User.findById(agentId) + .select('username botMetadata') + .lean<{ username?: string; botMetadata?: { displayName?: string; instanceId?: string } } | null>(); + const labelFallback = (agentName && agentName !== 'agent') ? agentName : 'agent'; + const label = resolveAgentDisplayLabel(agentUser, labelFallback); + // Include the stored label to find rooms created before runtime-label cleanup. + const searchLabels = [...new Set( + [agentName, label, agentUser?.botMetadata?.displayName?.trim()] + .filter((value): value is string => Boolean(value)), + )]; + const searchPattern = searchLabels.map((value) => DMService.escapeRegex(value)).join('|'); + const descriptionPattern = `^Debug channel for (?:${searchPattern})$`; + const namePattern = `^DM:\\s*(?:${searchPattern})$`; + // Look for an existing agent-admin pod where both users are members and - // the agent name matches (prevents cross-agent DM collisions). + // the complete stored identity matches (prevents cross-agent DM collisions). const existing = await Pod.findOne({ type: 'agent-admin', members: { $all: [agentId, ownerId] }, - description: { $regex: `\\b${agentName}\\b`, $options: 'i' }, + description: { $regex: descriptionPattern, $options: 'i' }, }); if (existing) return DMService.ensureDmMembers(existing, ownerId, agentId); - // Fallback: broader search without description regex in case the + // Fallback: exact DM name match without description in case the // description was edited or stored differently. const fallback = await Pod.findOne({ type: 'agent-admin', members: { $all: [agentId, ownerId] }, - name: { $regex: agentName, $options: 'i' }, + name: { $regex: namePattern, $options: 'i' }, }); if (fallback) return DMService.ensureDmMembers(fallback, ownerId, agentId); - // Repair stale pods that match this agent hint but are missing one of the - // expected members (for example older records with owner-only membership). - const safeAgentName = DMService.escapeRegex(agentName || 'agent'); - const staleCandidates = await Pod.find({ - type: 'agent-admin', - members: ownerId, - $or: [ - { description: { $regex: `\\b${safeAgentName}\\b`, $options: 'i' } }, - { name: { $regex: safeAgentName, $options: 'i' } }, - ], - }) - .sort({ updatedAt: -1 }) - .limit(5); - const staleChecks = await Promise.all( - staleCandidates.map(async (candidate) => ({ - candidate, - compatible: await DMService.isCompatibleStalePod(candidate, ownerId, agentId), - })), - ); - const stale = staleChecks.find((row) => row.compatible)?.candidate || null; - if (stale) { - return DMService.ensureDmMembers(stale, ownerId, agentId); - } + // Display names, including an exact legacy label, are not stable identity. + // An owner-only pod may belong to another agent with the same name. Leave + // ambiguous private pods untouched and create a new room for this pair. // Create a new DM pod. Use the agent's curated displayName (from // botMetadata) instead of the runtime-leaning agentName, so the pod // name reads "DM: Aria" not "DM: openclaw (aria)" — the latter // surfaces back into agent chat via heartbeat pod-list rendering. - const agentUser = await User.findById(agentId) - .select('username botMetadata') - .lean<{ username?: string; botMetadata?: { displayName?: string; instanceId?: string } } | null>(); - const labelFallback = (agentName && agentName !== 'agent') ? agentName : 'agent'; - const label = resolveAgentDisplayLabel(agentUser, labelFallback); const dmPod = new Pod({ name: `DM: ${label}`, description: `Debug channel for ${label}`, diff --git a/backend/utils/agentDisplayLabel.ts b/backend/utils/agentDisplayLabel.ts new file mode 100644 index 0000000000..d52928a27f --- /dev/null +++ b/backend/utils/agentDisplayLabel.ts @@ -0,0 +1,59 @@ +// Resolve the human-readable label for an agent User row. The fallback +// chain is intentional and load-bearing for any UI / pod-naming surface +// that shows agent identity (agent-dm pod names, sidebar member rows): +// +// 1. `botMetadata.displayName` — the curated label the registry/preset +// sets ("Pixel", "Strategist (Aria)"). This is what humans actually +// recognize. +// 2. `botMetadata.instanceId` — when displayName is missing AND the +// instanceId carries identity (i.e. != 'default'). For OpenClaw-driven +// agents the User row stores `agentName: 'openclaw'` (the RUNTIME) + +// `instanceId: 'aria' | 'pixel' | ...` (the identity), so falling back +// to instanceId here beats falling back to the runtime label. +// 3. `username` — last-resort identifier; always set. +// 4. fallback string — for the non-User case (e.g. resolved- +// by-alias before the User row is loaded). +// +// Why NOT fall back to `botMetadata.agentName`: that field can be the +// runtime name ('openclaw') rather than the agent's identity. Falling +// back to it produces "openclaw ↔ openclaw" pod names where what we want +// is "Pixel ↔ Strategist (Aria)". +export function resolveAgentDisplayLabel( + user: { + username?: string; + botMetadata?: { displayName?: string; instanceId?: string; agentName?: string }; + } | null | undefined, + fallback?: string, +): string { + const safeFallback = fallback || 'agent'; + if (!user) return safeFallback; + const meta = user.botMetadata; + const display = meta?.displayName?.trim(); + const agentName = meta?.agentName?.trim() || ''; + const instanceId = meta?.instanceId?.trim() || ''; + // Leak-pattern detection. If displayName is literally ` ()` + // (e.g. "openclaw (nova)"), it's the runtime label leaking through — some + // historical writer formatted displayName as `${agentName} (${instanceId})` + // instead of using the curated label. Treat these as if displayName were + // missing and fall through to instanceId. Matches both the + // ` ()` form and the bare `` form + // (e.g. displayName === 'openclaw') for a non-default instance. A default + // agent can have a curated label equal to its name ('Pixel'), so bare + // equality alone does not prove a runtime leak there. + const leakedPattern = ( + !!display + && !!agentName + && ( + (!!instanceId && instanceId !== 'default' && display.toLowerCase() === agentName.toLowerCase()) + || ( + !!instanceId + && display.toLowerCase() === `${agentName.toLowerCase()} (${instanceId.toLowerCase()})` + ) + ) + ); + if (display && !leakedPattern) return display; + if (instanceId && instanceId !== 'default') return instanceId; + if (user.username) return user.username; + return safeFallback; +} +