From 142b56861a6efd4b66b3a110762a6e748696e196 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 30 Apr 2025 12:23:29 +0200 Subject: [PATCH 1/5] fix: instruct models not to generate passwords for createDBUser --- src/common/atlas/generatePassword.ts | 10 +++++ src/tools/atlas/create/createDBUser.ts | 24 +++++++++++- src/tools/atlas/metadata/connectCluster.ts | 11 +----- tests/integration/helpers.ts | 2 +- tests/integration/tools/atlas/dbUsers.test.ts | 37 +++++++++++++++---- 5 files changed, 64 insertions(+), 20 deletions(-) create mode 100644 src/common/atlas/generatePassword.ts diff --git a/src/common/atlas/generatePassword.ts b/src/common/atlas/generatePassword.ts new file mode 100644 index 00000000..9e07267c --- /dev/null +++ b/src/common/atlas/generatePassword.ts @@ -0,0 +1,10 @@ +import { randomBytes } from "crypto"; +import { promisify } from "util"; + +const randomBytesAsync = promisify(randomBytes); + +export async function generateSecurePassword(): Promise { + const buf = await randomBytesAsync(16); + const pass = buf.toString("base64url"); + return pass; +} diff --git a/src/tools/atlas/create/createDBUser.ts b/src/tools/atlas/create/createDBUser.ts index a477862b..b8ed2ac1 100644 --- a/src/tools/atlas/create/createDBUser.ts +++ b/src/tools/atlas/create/createDBUser.ts @@ -3,6 +3,7 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { AtlasToolBase } from "../atlasTool.js"; import { ToolArgs, OperationType } from "../../tool.js"; import { CloudDatabaseUser, DatabaseUserRole } from "../../../common/atlas/openapi.js"; +import { generateSecurePassword } from "../../../common/atlas/generatePassword.js"; export class CreateDBUserTool extends AtlasToolBase { protected name = "atlas-create-db-user"; @@ -11,7 +12,16 @@ export class CreateDBUserTool extends AtlasToolBase { protected argsShape = { projectId: z.string().describe("Atlas project ID"), username: z.string().describe("Username for the new user"), - password: z.string().describe("Password for the new user"), + // Models will generate overly simplistic passwords like SecurePassword123 or + // AtlasPassword123, which are easily guessable and exploitable. We're instructing + // the model not to try and generate anything and instead leave the field unset. + password: z + .string() + .optional() + .nullable() + .describe( + "Password for the new user. If the user hasn't supplied an explicit password, leave it unset and under no circumstances try to generate a random one. A secure password will be generated by the MCP server if necessary." + ), roles: z .array( z.object({ @@ -34,6 +44,11 @@ export class CreateDBUserTool extends AtlasToolBase { roles, clusters, }: ToolArgs): Promise { + const shouldGeneratePassword = !password; + if (shouldGeneratePassword) { + password = await generateSecurePassword(); + } + const input = { groupId: projectId, awsIAMType: "NONE", @@ -62,7 +77,12 @@ export class CreateDBUserTool extends AtlasToolBase { }); return { - content: [{ type: "text", text: `User "${username}" created sucessfully.` }], + content: [ + { + type: "text", + text: `User "${username}" created sucessfully${shouldGeneratePassword ? ` with password: \`${password}\`` : ""}.`, + }, + ], }; } } diff --git a/src/tools/atlas/metadata/connectCluster.ts b/src/tools/atlas/metadata/connectCluster.ts index 523226ba..17fe9ddb 100644 --- a/src/tools/atlas/metadata/connectCluster.ts +++ b/src/tools/atlas/metadata/connectCluster.ts @@ -2,19 +2,10 @@ import { z } from "zod"; import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { AtlasToolBase } from "../atlasTool.js"; import { ToolArgs, OperationType } from "../../tool.js"; -import { randomBytes } from "crypto"; -import { promisify } from "util"; +import { generateSecurePassword } from "../../../common/atlas/generatePassword.js"; const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours -const randomBytesAsync = promisify(randomBytes); - -async function generateSecurePassword(): Promise { - const buf = await randomBytesAsync(16); - const pass = buf.toString("base64url"); - return pass; -} - export class ConnectClusterTool extends AtlasToolBase { protected name = "atlas-connect-cluster"; protected description = "Connect to MongoDB Atlas cluster"; diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index bacc89b9..c4187318 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -117,7 +117,7 @@ export function getResponseElements(content: unknown | { content: unknown }): { content = (content as { content: unknown }).content; } - expect(Array.isArray(content)).toBe(true); + expect(content).toBeArray(); const response = content as { type: string; text: string }[]; for (const item of response) { diff --git a/tests/integration/tools/atlas/dbUsers.test.ts b/tests/integration/tools/atlas/dbUsers.test.ts index 892bb89e..3b5b8b38 100644 --- a/tests/integration/tools/atlas/dbUsers.test.ts +++ b/tests/integration/tools/atlas/dbUsers.test.ts @@ -1,7 +1,7 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { Session } from "../../../../src/session.js"; import { describeWithAtlas, withProject, randomId } from "./atlasHelpers.js"; -import { expectDefined } from "../../helpers.js"; +import { expectDefined, getResponseElements } from "../../helpers.js"; describeWithAtlas("db users", (integration) => { const userName = "testuser-" + randomId; @@ -34,10 +34,11 @@ describeWithAtlas("db users", (integration) => { expect(createDbUser.inputSchema.properties).toHaveProperty("roles"); expect(createDbUser.inputSchema.properties).toHaveProperty("clusters"); }); - it("should create a database user", async () => { + + it("should create a database user with supplied password", async () => { const projectId = getProjectId(); - const response = (await integration.mcpClient().callTool({ + const response = await integration.mcpClient().callTool({ name: "atlas-create-db-user", arguments: { projectId, @@ -50,10 +51,32 @@ describeWithAtlas("db users", (integration) => { }, ], }, - })) as CallToolResult; - expect(response.content).toBeArray(); - expect(response.content).toHaveLength(1); - expect(response.content[0].text).toContain("created sucessfully"); + }); + const elements = getResponseElements(response); + expect(elements).toHaveLength(1); + expect(elements[0].text).toContain("created sucessfully"); + }); + + it("should create a database user with generated password", async () => { + const projectId = getProjectId(); + + const response = await integration.mcpClient().callTool({ + name: "atlas-create-db-user", + arguments: { + projectId, + username: userName, + roles: [ + { + roleName: "readWrite", + databaseName: "admin", + }, + ], + }, + }); + const elements = getResponseElements(response); + expect(elements).toHaveLength(1); + expect(elements[0].text).toContain("created sucessfully"); + expect(elements[0].text).toContain("with password: `"); }); }); describe("atlas-list-db-users", () => { From 5cc18fc132515f7386ac6452db6f882efb0f6026 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 30 Apr 2025 12:49:53 +0200 Subject: [PATCH 2/5] fix typo --- src/tools/atlas/create/createDBUser.ts | 2 +- tests/integration/tools/atlas/dbUsers.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tools/atlas/create/createDBUser.ts b/src/tools/atlas/create/createDBUser.ts index b8ed2ac1..a8266a0a 100644 --- a/src/tools/atlas/create/createDBUser.ts +++ b/src/tools/atlas/create/createDBUser.ts @@ -80,7 +80,7 @@ export class CreateDBUserTool extends AtlasToolBase { content: [ { type: "text", - text: `User "${username}" created sucessfully${shouldGeneratePassword ? ` with password: \`${password}\`` : ""}.`, + text: `User "${username}" created successfully${shouldGeneratePassword ? ` with password: \`${password}\`` : ""}.`, }, ], }; diff --git a/tests/integration/tools/atlas/dbUsers.test.ts b/tests/integration/tools/atlas/dbUsers.test.ts index 3b5b8b38..f8919a17 100644 --- a/tests/integration/tools/atlas/dbUsers.test.ts +++ b/tests/integration/tools/atlas/dbUsers.test.ts @@ -54,7 +54,7 @@ describeWithAtlas("db users", (integration) => { }); const elements = getResponseElements(response); expect(elements).toHaveLength(1); - expect(elements[0].text).toContain("created sucessfully"); + expect(elements[0].text).toContain("created successfully"); }); it("should create a database user with generated password", async () => { @@ -75,7 +75,7 @@ describeWithAtlas("db users", (integration) => { }); const elements = getResponseElements(response); expect(elements).toHaveLength(1); - expect(elements[0].text).toContain("created sucessfully"); + expect(elements[0].text).toContain("created successfully"); expect(elements[0].text).toContain("with password: `"); }); }); From 5d14a16effab50b550031d8bf597df1791daabb2 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 30 Apr 2025 13:44:40 +0200 Subject: [PATCH 3/5] Delete users after each test; simplify testing a little --- tests/integration/tools/atlas/dbUsers.test.ts | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/tests/integration/tools/atlas/dbUsers.test.ts b/tests/integration/tools/atlas/dbUsers.test.ts index f8919a17..63147024 100644 --- a/tests/integration/tools/atlas/dbUsers.test.ts +++ b/tests/integration/tools/atlas/dbUsers.test.ts @@ -4,9 +4,30 @@ import { describeWithAtlas, withProject, randomId } from "./atlasHelpers.js"; import { expectDefined, getResponseElements } from "../../helpers.js"; describeWithAtlas("db users", (integration) => { - const userName = "testuser-" + randomId; withProject(integration, ({ getProjectId }) => { - afterAll(async () => { + let userName: string; + beforeEach(() => { + userName = "testuser-" + randomId; + }); + + const createUserWithMCP = async (password?: string): Promise => { + return await integration.mcpClient().callTool({ + name: "atlas-create-db-user", + arguments: { + projectId: getProjectId(), + username: userName, + password, + roles: [ + { + roleName: "readWrite", + databaseName: "admin", + }, + ], + }, + }); + }; + + afterEach(async () => { const projectId = getProjectId(); const session: Session = integration.mcpServer().session; @@ -36,46 +57,21 @@ describeWithAtlas("db users", (integration) => { }); it("should create a database user with supplied password", async () => { - const projectId = getProjectId(); + const response = await createUserWithMCP("testpassword"); - const response = await integration.mcpClient().callTool({ - name: "atlas-create-db-user", - arguments: { - projectId, - username: userName, - password: "testpassword", - roles: [ - { - roleName: "readWrite", - databaseName: "admin", - }, - ], - }, - }); const elements = getResponseElements(response); expect(elements).toHaveLength(1); expect(elements[0].text).toContain("created successfully"); + expect(elements[0].text).toContain(userName); + expect(elements[0].text).not.toContain("testpassword"); }); it("should create a database user with generated password", async () => { - const projectId = getProjectId(); - - const response = await integration.mcpClient().callTool({ - name: "atlas-create-db-user", - arguments: { - projectId, - username: userName, - roles: [ - { - roleName: "readWrite", - databaseName: "admin", - }, - ], - }, - }); + const response = await createUserWithMCP(); const elements = getResponseElements(response); expect(elements).toHaveLength(1); expect(elements[0].text).toContain("created successfully"); + expect(elements[0].text).toContain(userName); expect(elements[0].text).toContain("with password: `"); }); }); @@ -91,6 +87,8 @@ describeWithAtlas("db users", (integration) => { it("returns database users by project", async () => { const projectId = getProjectId(); + await createUserWithMCP(); + const response = (await integration .mcpClient() .callTool({ name: "atlas-list-db-users", arguments: { projectId } })) as CallToolResult; From a9edff35df606e494f63d2d7a670c7fbb2a6027a Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 30 Apr 2025 14:43:35 +0200 Subject: [PATCH 4/5] Ignore 404 errors --- tests/integration/tools/atlas/dbUsers.test.ts | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/integration/tools/atlas/dbUsers.test.ts b/tests/integration/tools/atlas/dbUsers.test.ts index 63147024..0fa2dba2 100644 --- a/tests/integration/tools/atlas/dbUsers.test.ts +++ b/tests/integration/tools/atlas/dbUsers.test.ts @@ -2,6 +2,7 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { Session } from "../../../../src/session.js"; import { describeWithAtlas, withProject, randomId } from "./atlasHelpers.js"; import { expectDefined, getResponseElements } from "../../helpers.js"; +import { ApiClientError } from "../../../../src/common/atlas/apiClientError.js"; describeWithAtlas("db users", (integration) => { withProject(integration, ({ getProjectId }) => { @@ -28,18 +29,22 @@ describeWithAtlas("db users", (integration) => { }; afterEach(async () => { - const projectId = getProjectId(); - - const session: Session = integration.mcpServer().session; - await session.apiClient.deleteDatabaseUser({ - params: { - path: { - groupId: projectId, - username: userName, - databaseName: "admin", + try { + await integration.mcpServer().session.apiClient.deleteDatabaseUser({ + params: { + path: { + groupId: getProjectId(), + username: userName, + databaseName: "admin", + }, }, - }, - }); + }); + } catch (error) { + // Ignore 404 errors when deleting the user + if (!(error instanceof ApiClientError) || error.response?.status !== 404) { + throw error; + } + } }); describe("atlas-create-db-user", () => { From 003179be05689a99b66cc27f5a157bff1b51a345 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 30 Apr 2025 15:01:01 +0200 Subject: [PATCH 5/5] fix lint --- tests/integration/tools/atlas/dbUsers.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/tools/atlas/dbUsers.test.ts b/tests/integration/tools/atlas/dbUsers.test.ts index 0fa2dba2..2bcb95fa 100644 --- a/tests/integration/tools/atlas/dbUsers.test.ts +++ b/tests/integration/tools/atlas/dbUsers.test.ts @@ -1,5 +1,4 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; -import { Session } from "../../../../src/session.js"; import { describeWithAtlas, withProject, randomId } from "./atlasHelpers.js"; import { expectDefined, getResponseElements } from "../../helpers.js"; import { ApiClientError } from "../../../../src/common/atlas/apiClientError.js";