diff --git a/src/everything/resources/subscriptions.ts b/src/everything/resources/subscriptions.ts index 2a5e57460f..854a8633a2 100644 --- a/src/everything/resources/subscriptions.ts +++ b/src/everything/resources/subscriptions.ts @@ -99,10 +99,9 @@ export const setSubscriptionHandlers = (server: McpServer) => { /** * Sends simulated resource update notifications to the subscribed client. * - * This function iterates through all resource URIs stored in the subscriptions - * and checks if the specified session ID is subscribed to them. If so, it sends - * a notification through the provided server. If the session ID is no longer valid - * (disconnected), it removes the session ID from the list of subscribers. + * Iterates the URIs in `subscriptions` and emits a + * `notifications/resources/updated` notification for each URI the given + * sessionId is subscribed to. * * @param {McpServer} server - The server instance used to send notifications. * @param {string | undefined} sessionId - The session ID of the client to check for subscriptions. @@ -122,8 +121,6 @@ const sendSimulatedResourceUpdates = async ( method: "notifications/resources/updated", params: { uri }, }); - } else { - subscribers.delete(sessionId); // subscriber has disconnected } } }; diff --git a/src/everything/resources/templates.ts b/src/everything/resources/templates.ts index 6d4903f74c..4530741c54 100644 --- a/src/everything/resources/templates.ts +++ b/src/everything/resources/templates.ts @@ -127,31 +127,26 @@ export const blobResourceUri = (resourceId: number) => new URL(`${blobUriBase}/${resourceId}`); /** - * Parses the resource identifier from the provided URI and validates it - * against the given variables. Throws an error if the URI corresponds - * to an unknown resource or if the resource identifier is invalid. + * Parses the resource identifier from the provided variables and validates + * that it is a positive integer. * - * @param {URL} uri - The URI of the resource to be parsed. - * @param {Record} variables - A record containing context-specific variables that include the resourceId. - * @returns {number} The parsed and validated resource identifier as an integer. - * @throws {Error} Throws an error if the URI matches unsupported base URIs or if the resourceId is invalid. + * The SDK only routes URIs that match the registered template to the + * resource handler, so by the time we get here the URI prefix is already + * known to be one of `textUriBase` / `blobUriBase`. Only the resourceId + * variable still needs validating. + * + * @param {URL} uri - The URI of the resource (used in the error message). + * @param {Record} variables - Context variables including resourceId. + * @returns {number} The parsed and validated resource identifier as a positive integer. + * @throws {Error} If the resourceId is not a finite positive integer. */ const parseResourceId = (uri: URL, variables: Record) => { - const uriError = `Unknown resource: ${uri.toString()}`; - if ( - uri.toString().startsWith(textUriBase) && - uri.toString().startsWith(blobUriBase) - ) { - throw new Error(uriError); - } else { - const idxStr = String((variables as any).resourceId ?? ""); - const idx = Number(idxStr); - if (Number.isFinite(idx) && Number.isInteger(idx) && idx > 0) { - return idx; - } else { - throw new Error(uriError); - } + const idxStr = String((variables as any).resourceId ?? ""); + const idx = Number(idxStr); + if (Number.isFinite(idx) && Number.isInteger(idx) && idx > 0) { + return idx; } + throw new Error(`Unknown resource: ${uri.toString()}`); }; /** diff --git a/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts b/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts new file mode 100644 index 0000000000..0cbe48c65c --- /dev/null +++ b/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs/promises'; +import * as path from 'path'; +import * as os from 'os'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; + +describe('directory_tree MCP SDK regression', () => { + let client: Client; + let transport: StdioClientTransport; + let testDir: string; + + beforeEach(async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-tree-')); + testDir = await fs.realpath(tmp); + + await fs.writeFile(path.join(testDir, 'root.txt'), 'root'); + await fs.mkdir(path.join(testDir, 'nested')); + await fs.writeFile(path.join(testDir, 'nested', 'child.txt'), 'child'); + + const serverPath = path.resolve(__dirname, '../dist/index.js'); + transport = new StdioClientTransport({ + command: 'node', + args: [serverPath, testDir], + }); + + client = new Client( + { name: 'directory-tree-regression-test', version: '1.0.0' }, + { capabilities: {} }, + ); + + await client.connect(transport); + }); + + afterEach(async () => { + await client?.close(); + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it('returns structuredContent.content as a string (not an array) when called via MCP SDK', async () => { + const result = await client.callTool({ + name: 'directory_tree', + arguments: { path: testDir }, + }); + + // Regression test for issues where structuredContent was returned as an array, + // which causes MCP SDK validation to throw -32602 (invalid structured content). + const structured = result.structuredContent as { content: unknown }; + expect(structured).toBeDefined(); + expect(typeof structured.content).toBe('string'); + expect(Array.isArray(structured.content)).toBe(false); + + const parsed = JSON.parse(structured.content as string); + expect(Array.isArray(parsed)).toBe(true); + expect(parsed.length).toBeGreaterThan(0); + }); +}); diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index e0ae61224f..1da741f0b0 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -65,8 +65,10 @@ describe('Lib Functions', () => { }); it('handles negative numbers', () => { - // Negative numbers will result in NaN for the log calculation - expect(formatSize(-1024)).toContain('NaN'); + // Negative numbers should return '0 B' as file sizes cannot be negative + expect(formatSize(-1)).toBe('0 B'); + expect(formatSize(-1024)).toBe('0 B'); + expect(formatSize(-1000000)).toBe('0 B'); expect(formatSize(-0)).toBe('0 B'); }); @@ -308,6 +310,33 @@ describe('Lib Functions', () => { expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); + + it('preserves file permissions when overwriting existing file', async () => { + // First writeFile call with 'wx' flag fails because file exists + mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' })); + // stat returns executable permissions + mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 }); + // Second writeFile (to temp) succeeds + mockFs.writeFile.mockResolvedValueOnce(undefined); + mockFs.rename.mockResolvedValueOnce(undefined); + mockFs.chmod.mockResolvedValueOnce(undefined); + + await writeFileContent('/test/script.sh', 'new content'); + + expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh'); + expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755); + }); + + it('does not fail the write when chmod fails', async () => { + mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' })); + mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 }); + mockFs.writeFile.mockResolvedValueOnce(undefined); + mockFs.rename.mockResolvedValueOnce(undefined); + mockFs.chmod.mockRejectedValueOnce(Object.assign(new Error('EPERM'), { code: 'EPERM' })); + + await expect(writeFileContent('/test/script.sh', 'new content')).resolves.toBeUndefined(); + expect(mockFs.unlink).not.toHaveBeenCalled(); + }); }); }); @@ -416,6 +445,8 @@ describe('Lib Functions', () => { beforeEach(() => { mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); mockFs.writeFile.mockResolvedValue(undefined); + mockFs.stat.mockResolvedValue({ mode: 0o100644 }); + mockFs.chmod.mockResolvedValue(undefined); }); it('applies simple text replacement', async () => { @@ -510,6 +541,30 @@ describe('Lib Functions', () => { ); }); + it('preserves file permissions after applying edits', async () => { + mockFs.stat.mockResolvedValue({ mode: 0o100755 }); + const edits = [ + { oldText: 'line2', newText: 'modified line2' } + ]; + + mockFs.rename.mockResolvedValueOnce(undefined); + + await applyFileEdits('/test/script.sh', edits, false); + + expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh'); + expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755); + }); + + it('does not restore permissions in dry run mode', async () => { + const edits = [ + { oldText: 'line2', newText: 'modified line2' } + ]; + + await applyFileEdits('/test/file.txt', edits, true); + + expect(mockFs.chmod).not.toHaveBeenCalled(); + }); + it('throws error for non-matching edits', async () => { const edits = [ { oldText: 'nonexistent line', newText: 'replacement' } diff --git a/src/filesystem/__tests__/structured-content.test.ts b/src/filesystem/__tests__/structured-content.test.ts index 4605b72a8f..67da16f901 100644 --- a/src/filesystem/__tests__/structured-content.test.ts +++ b/src/filesystem/__tests__/structured-content.test.ts @@ -22,7 +22,9 @@ describe('structuredContent schema compliance', () => { beforeEach(async () => { // Create a temp directory for testing - testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-test-')); + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-test-')); + // macOS temp dirs may be symlinked (/var -> /private/var); resolve for server allowlist checks. + testDir = await fs.realpath(tmpDir); // Create test files await fs.writeFile(path.join(testDir, 'test.txt'), 'test content'); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index ce4af9f38a..a1c6f04b67 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -43,7 +43,7 @@ export interface SearchResult { // Pure Utility Functions export function formatSize(bytes: number): string { const units = ['B', 'KB', 'MB', 'GB', 'TB']; - if (bytes === 0) return '0 B'; + if (bytes <= 0) return '0 B'; const i = Math.floor(Math.log(bytes) / Math.log(1024)); @@ -168,6 +168,7 @@ export async function writeFileContent(filePath: string, content: string): Promi // Security: Use atomic rename to prevent race conditions where symlinks // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. + const origStats = await fs.stat(filePath); const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, content, 'utf-8'); @@ -178,6 +179,13 @@ export async function writeFileContent(filePath: string, content: string): Promi } catch {} throw renameError; } + // Restore original permission bits since the atomic rename replaces the + // inode and the temp file has default (0644) permissions. Mask off the + // file-type bits; POSIX leaves them unspecified for chmod. A chmod + // failure must not fail the write, which has already succeeded. + try { + await fs.chmod(filePath, origStats.mode & 0o777); + } catch {} } else { throw error; } @@ -266,6 +274,7 @@ export async function applyFileEdits( // Security: Use atomic rename to prevent race conditions where symlinks // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. + const origStats = await fs.stat(filePath); const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, modifiedContent, 'utf-8'); @@ -276,6 +285,13 @@ export async function applyFileEdits( } catch {} throw error; } + // Restore original permission bits since the atomic rename replaces the + // inode and the temp file has default (0644) permissions. Mask off the + // file-type bits; POSIX leaves them unspecified for chmod. A chmod + // failure must not fail the write, which has already succeeded. + try { + await fs.chmod(filePath, origStats.mode & 0o777); + } catch {} } return formattedDiff;