diff --git a/backend/__tests__/service/install.podid-operator-injection.test.js b/backend/__tests__/service/install.podid-operator-injection.test.js new file mode 100644 index 000000000..bf4cefb6c --- /dev/null +++ b/backend/__tests__/service/install.podid-operator-injection.test.js @@ -0,0 +1,137 @@ +/** + * Regression test for /api/registry/install accepting a query operator as + * `podId`. + * + * CodeQL flagged the `AgentInstallation.findOne` at install.ts and the alert + * was first read as a false positive, on the premise that `Pod.findById(podId)` + * + 404 above it already rejects anything that is not a real id. It does not. + * Mongoose casts an operator object in a filter position rather than throwing, + * so `Pod.findById({ $ne: null })` MATCHES the first pod in the collection and + * the 404 never fires โ€” the raw object then reaches every Mongoose filter + * below, one of which (`AgentProfile.findOneAndUpdate`) is an upsert key. + * The route's own comment says why that set is stated as a position rather + * than a count: podId also flows through helpers which query on it again. + * + * The first test is the positive control for that premise: it asserts the + * upstream guard really does match, so a later refactor cannot quietly turn + * this suite green for the wrong reason. + */ + +const express = require('express'); +const request = require('supertest'); +const jwt = require('jsonwebtoken'); + +const { setupMongoDb, closeMongoDb } = require('../utils/testUtils'); + +const User = require('../../models/User'); +const Pod = require('../../models/Pod'); +const { AgentRegistry, AgentInstallation } = require('../../models/AgentRegistry'); +const AgentProfile = require('../../models/AgentProfile').default || require('../../models/AgentProfile'); + +const registryRoutes = require('../../routes/registry'); + +const JWT_SECRET = 'test-jwt-secret-install-podid-operator'; + +jest.setTimeout(60000); + +describe('Install endpoint rejects a query operator as podId', () => { + let app; + let installer; + let installerToken; + let pod; + + beforeAll(async () => { + process.env.JWT_SECRET = JWT_SECRET; + await setupMongoDb(); + + app = express(); + app.use(express.json()); + app.use('/api/registry', registryRoutes); + + installer = await User.create({ + username: 'operator-installer', + email: 'operator-installer@test.com', + password: 'password123', + entitlements: { cloudAgents: true }, + }); + installerToken = jwt.sign({ id: installer._id.toString() }, JWT_SECRET); + + pod = await Pod.create({ + name: 'Operator-Injection Test Pod', + type: 'chat', + createdBy: installer._id, + members: [installer._id], + }); + + await AgentRegistry.create({ + agentName: 'openclaw', + displayName: 'Cuz ๐Ÿฆž', + description: 'OpenClaw test', + manifest: { + name: 'openclaw', + version: '1.0.0', + capabilities: [], + context: { required: [], optional: [] }, + }, + latestVersion: '1.0.0', + versions: [{ version: '1.0.0', manifest: { name: 'openclaw', version: '1.0.0', capabilities: [], context: { required: [], optional: [] } }, publishedAt: new Date() }], + registry: 'private', + }); + }); + + afterAll(async () => { + await closeMongoDb(); + }); + + beforeEach(async () => { + await AgentInstallation.deleteMany({}); + await AgentProfile.deleteMany({}); + }); + + // Positive control for the premise the fix rests on. If this ever starts + // throwing or returning null, the entry-point guard below is still correct + // but this suite is no longer testing the thing it claims to. + it('control: Pod.findById with an operator object matches rather than 404ing', async () => { + const matched = await Pod.findById({ $ne: null }).lean(); + expect(matched).toBeTruthy(); + expect(String(matched._id)).toBe(String(pod._id)); + }); + + it('400s on an operator object instead of letting it reach the Mongoose filters', async () => { + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: { $ne: null }, + version: '1.0.0', + scopes: ['context:read'], + }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/podId must be a string/); + + // Nothing may have been written on the way to the rejection โ€” the + // AgentProfile write is an upsert keyed on podId, so a leak here would + // create a row rather than merely read one. + expect(await AgentInstallation.countDocuments({})).toBe(0); + expect(await AgentProfile.countDocuments({})).toBe(0); + }); + + it('still installs normally when podId is the ordinary id string', async () => { + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: pod._id.toString(), + version: '1.0.0', + scopes: ['context:read'], + }); + + expect(res.status).toBe(200); + expect(await AgentInstallation.countDocuments({ podId: pod._id })).toBe(1); + }); +}); diff --git a/backend/__tests__/service/install.preserves-displayname.test.js b/backend/__tests__/service/install.preserves-displayname.test.js index cc1d1380f..504c9fdc3 100644 --- a/backend/__tests__/service/install.preserves-displayname.test.js +++ b/backend/__tests__/service/install.preserves-displayname.test.js @@ -238,4 +238,66 @@ describe('Install endpoint preserves curated displayName (cycle-of-Aria regressi // Registry default is acceptable when there's no curated identity to preserve. expect(installation.displayName).toBe('Cuz ๐Ÿฆž'); }); + + // TASK-032, ruled 2026-09-02: AgentInstallation is canonical; the User row + // is a seed for an installation that has no name, never a preference over + // one that does. Before the ruling this case reverted a per-pod label to + // the shared identity's label on every re-install, while the read paths + // (agentMessageService, dmService) already preferred the installation. + // + // This is the discriminating case for that precedence and nothing else in + // the file reaches it: every other test starts from an empty + // AgentInstallation collection, where seed and preference are the same + // value and the two orderings agree. + // + // The reachable shape is uninstall-then-reinstall, not install-twice: an + // ACTIVE row makes the route 400 ('Agent already installed in this pod'), + // so the only way a second install meets an existing row is the + // reactivation path โ€” where ADR-001 ยง3 identity continuity keeps the row + // (and its curated name) across the uninstall. + it('keeps an existing installation displayName over the curated User row on re-install', async () => { + await AgentInstallation.create({ + agentName: 'openclaw', + podId: pod._id, + instanceId: 'aria', + installedBy: installer._id, + version: '1.0.0', + status: 'uninstalled', + scopes: ['context:read'], + // Curated for THIS pod, and deliberately different from the User row's + // 'Aria' so the assertion cannot pass by coincidence. + displayName: 'Aria (Sprint Desk)', + }); + + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: pod._id.toString(), + version: '1.0.0', + scopes: ['context:read', 'summaries:read', 'messages:write'], + // No explicit displayName โ€” the caller is re-installing, not renaming. + }); + + expect(res.status).toBe(200); + + const installation = await AgentInstallation.findOne({ + podId: pod._id, + agentName: 'openclaw', + instanceId: 'aria', + }).lean(); + expect(installation.displayName).toBe('Aria (Sprint Desk)'); + // The two values the old precedence would have written instead. + expect(installation.displayName).not.toBe('Aria'); + expect(installation.displayName).not.toBe('Cuz ๐Ÿฆž'); + + const profile = await AgentProfile.findOne({ + podId: pod._id, + agentName: 'openclaw', + instanceId: 'aria', + }).lean(); + expect(profile.name).toBe('Aria (Sprint Desk)'); + }); }); diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index b3576d9f3..d718132f7 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -133,6 +133,26 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any return res.status(400).json({ error: 'Invalid agentName: must match /^(@[a-z0-9-]+\\/)?[a-z0-9-]+$/' }); } + // Mongoose does not reject an operator object in a filter position โ€” a + // `{$ne: null}` here casts cleanly and MATCHES, so the `Pod.findById` + // 404 below is not a guard against one. Reject a non-string at the entry + // point, the same shape agentName already uses above, so no query below + // can be handed a query operator. `undefined` is left to fall through to + // the existing 404 / self-serve 400 so no legitimate path changes status + // code. + // + // Deliberately no count of the filters downstream. podId reaches them + // both directly and through helpers that fan it out again โ€” + // `AgentInstallation.install` (a `findOne` AND a `create`), + // `AgentIdentityService.ensureAgentInPod` (a third `Pod.findById`), + // `AgentMessageService.postMessage` โ€” so any number is a claim about how + // far the reader followed the value, and it decays on the next refactor. + // The property that actually holds is positional: this returns before + // every one of them. + if (podId !== undefined && podId !== null && typeof podId !== 'string') { + return res.status(400).json({ error: 'podId must be a string' }); + } + // Fetched once and reused for the #609 owner-scoping decision below and // the cloud-entitlement gate further down. const installerUser = await User.findById(userId).select('role entitlements isBot').lean(); @@ -418,20 +438,54 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any ...AUTO_GRANTED_INTEGRATION_SCOPES, ])); - // Task #62 (round 2): prefer the curated User.botMetadata.displayName - // for the SAME agentName + instanceId over the registry-default - // (`agent.displayName` e.g. "Cuz ๐Ÿฆž" / "Codex"). PR #408 fixed this - // seam on the intro-post path; this is the install-path equivalent. - // Without this, installing an existing agent identity (e.g. openclaw:nova) - // into a NEW pod writes "Cuz ๐Ÿฆž" to both AgentInstallation.displayName - // AND AgentProfile.name โ€” and the V2 member list reads AgentProfile FIRST, - // so users see "Cuz" on every member row even though the underlying - // identity has the right name. Order: explicit caller intent > existing - // identity > registry default. Resolve the existing identity through the - // same leak guard every display surface uses: historical rows may contain - // a runtime-shaped displayName such as "openclaw (nova)", which must not - // become a higher-precedence installation or profile label. + // TASK-032, ruled 2026-09-02: AgentInstallation is canonical for a + // display name. `User.botMetadata.displayName` is a SEED for an + // installation that has none โ€” never a preference over one that does. + // + // Precedence: explicit caller intent > this pod's existing installation + // > the curated identity on the User row > registry default + // (`agent.displayName` e.g. "Cuz ๐Ÿฆž" / "Codex"). + // + // The installation term is the new one and it is what the ruling adds. + // Without it a re-install re-seeded the per-pod label from the shared + // User row, so a name curated for THIS pod silently reverted โ€” while + // the read paths (agentMessageService, dmService) already preferred the + // installation precisely to stop a sibling pod's name leaking through. + // + // Task #62 / PR #408 is NOT reinstated by this reordering: the User-row + // seed still beats the registry default, so installing an existing + // identity (e.g. openclaw:nova) into a NEW pod โ€” where there is no + // installation yet โ€” still writes "Aria" rather than "Cuz ๐Ÿฆž" to both + // AgentInstallation.displayName and AgentProfile.name. What changes is + // only the case where BOTH are curated, and there the per-pod value wins. + // The seed is still read through the same leak guard every display + // surface uses: historical rows may contain a runtime-shaped displayName + // such as "openclaw (nova)", which must not become a higher-precedence + // installation or profile label. let effectiveDisplayName: string = displayName || ''; + if (!effectiveDisplayName) { + try { + const existingInstallation = await AgentInstallation.findOne({ + agentName: agent.agentName, + podId: String(podId), + instanceId: normalizedInstanceId, + }).select('displayName').lean() as { displayName?: string } | null; + if (existingInstallation?.displayName) { + effectiveDisplayName = existingInstallation.displayName; + } + } catch (lookupErr) { + // Non-fatal โ€” fall through to the User-row seed below. Logged with + // the agent identity so an operator chasing "the name reverted on + // re-install" can correlate the attempt, in the CodeQL-safe shape + // used by the sibling handlers (identifiers as arguments, not in + // the format-string slot). + console.warn('[install] installation displayName lookup failed', { + agent: agent.agentName, + instance: normalizedInstanceId, + error: (lookupErr as Error).message, + }); + } + } if (!effectiveDisplayName) { try { const existingAgentUser = await User.findOne({ @@ -609,31 +663,17 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any // getOrCreateAgentUser. If we pass the AgentRegistry default // ("Cuz ๐Ÿฆž" / "Codex"), the User row's curated per-instance // displayName ("Aria") gets clobbered or sticky-dedup-suffixed - // to "Cuz ๐Ÿฆž (Aria)". Resolve the intro display label by - // preferring the live agent identity (User.botMetadata.displayName) - // first, so we never overwrite a curated label with a registry default. - let displayName: string; - try { - const existingBot = await User.findOne({ - isBot: true, - 'botMetadata.agentName': agent.agentName, - 'botMetadata.instanceId': normalizedInstanceId, - }).select('botMetadata').lean() as { botMetadata?: { displayName?: string } } | null; - displayName = existingBot?.botMetadata?.displayName - || installation.displayName - || agent.displayName; - } catch (lookupErr: unknown) { - // Fall back to the legacy chain if the identity lookup blew up โ€” - // don't take down the intro flow on a transient mongo hiccup. - // Log with agent identity for operator correlation. - // Same CodeQL-safe shape as the install-path log above. - console.warn('[install] intro displayName lookup failed', { - agent: agent.agentName, - instance: normalizedInstanceId, - error: (lookupErr as Error).message, - }); - displayName = installation.displayName || agent.displayName; - } + // to "Cuz ๐Ÿฆž (Aria)". + // + // This used to re-query User.botMetadata and prefer it. Under + // TASK-032's ruling the installation is canonical, and + // `installation.displayName` was resolved a few lines above through + // the full precedence chain (explicit > installation > User-row seed + // > registry default) โ€” so it already carries the curated label and + // the second lookup could only disagree with the row we just wrote. + // Reading it here keeps the intro post and the member list on one + // source of truth, and drops a per-install User query. + const displayName: string = installation.displayName || agent.displayName; const blurb = (agent.description || '').trim().replace(/\s+/g, ' '); // Skip the blurb when it just repeats the name (the publish step in // older CLI versions seeded description from displayName, producing