@@ -3,7 +3,8 @@ import * as path from "path"
33
44import type { Mock } from "vitest"
55import type { ExtensionContext , Uri } from "vscode"
6- import type { Client } from "@modelcontextprotocol/sdk/client/index.js"
6+ import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"
7+ import { Client } from "@modelcontextprotocol/sdk/client/index.js"
78import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
89
910import type { ClineProvider } from "../../../core/webview/ClineProvider"
@@ -35,6 +36,7 @@ vi.mock("fs/promises", () => {
3536} )
3637
3738// Import safeWriteJson to use in mocks
39+ import { safeWriteJson } from "../../../utils/safeWriteJson"
3840
3941// Mock safeWriteJson
4042vi . mock ( "../../../utils/safeWriteJson" , ( ) => ( {
@@ -154,10 +156,35 @@ vi.mock("chokidar", () => ({
154156 } ,
155157} ) )
156158
159+ // Only getWorkspacePath is replaced; the original module is loaded and spread so its side effect
160+ // (installing String.prototype.toPosix) and its other exports keep working for this file. The fallback
161+ // is what the fallback test inside the confinement describe below exercises, so it must be distinguishable
162+ // from any workspace path a test could otherwise compute.
163+ vi . mock ( "../../../utils/path" , async ( importOriginal ) => {
164+ const actual = await importOriginal < typeof import ( "../../../utils/path" ) > ( )
165+ return { ...actual , getWorkspacePath : vi . fn ( ( ) => "/fallback/workspace" ) }
166+ } )
167+
157168describe ( "McpHub" , ( ) => {
158169 let mcpHub : McpHubType
159170 let mockProvider : Partial < ClineProvider >
160171
172+ // A fully typed connection double: the SDK constructors are mocked at the top of the
173+ // file, so no cast is needed to build a connected connection here. Shared by the
174+ // write-confinement tests for every project-capable entry point.
175+ const connectionFor = ( source : "global" | "project" ) : ConnectedMcpConnection => ( {
176+ type : "connected" ,
177+ server : {
178+ name : "test-server" ,
179+ config : JSON . stringify ( { type : "stdio" , command : "node" , args : [ "test.js" ] , alwaysAllow : [ ] } ) ,
180+ status : "connected" ,
181+ source,
182+ errorHistory : [ ] ,
183+ } ,
184+ client : new Client ( { name : "test-client" , version : "1.0.0" } ) ,
185+ transport : new StdioClientTransport ( { command : "node" , args : [ "test.js" ] } ) ,
186+ } )
187+
161188 // Store original console methods
162189 const originalConsoleError = console . error
163190 const originalPlatform = Object . getOwnPropertyDescriptor ( process , "platform" )
@@ -1205,7 +1232,148 @@ describe("McpHub", () => {
12051232 } )
12061233 } )
12071234
1235+ describe ( "project-scoped write confinement at every call site" , ( ) => {
1236+ // safeWriteJson resolves the publish target with realpath before staging beside it, so
1237+ // a project .roo/mcp.json that links outside the workspace would be written THROUGH the
1238+ // link unless the caller declares the workspace as the confinement root. Every
1239+ // project-capable entry point has to pass it. A lower-level safeWriteJson test cannot
1240+ // see a missing MCP argument (the writer is mocked here), so each call site is pinned.
1241+ const projectConfigJson = JSON . stringify ( {
1242+ mcpServers : { "test-server" : { type : "stdio" , command : "node" , args : [ "test.js" ] , timeout : 60 } } ,
1243+ } )
1244+
1245+ it ( "confines a project-scoped timeout write to the workspace root" , async ( ) => {
1246+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1247+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1248+ mcpHub . connections = [ connectionFor ( "project" ) ]
1249+
1250+ await mcpHub . updateServerTimeout ( "test-server" , 120 )
1251+
1252+ const write = vi . mocked ( safeWriteJson ) . mock . calls . find ( ( call ) => String ( call [ 0 ] ) . includes ( ".roo" ) )
1253+ expect ( write ) . toBeDefined ( )
1254+ expect ( write ! [ 2 ] ) . toEqual ( expect . objectContaining ( { confineTo : "/mock/workspace" } ) )
1255+ } )
1256+
1257+ it ( "leaves a global-scoped timeout write unconstrained" , async ( ) => {
1258+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1259+ // The global path creates the default settings file when it is missing, and that
1260+ // write's merge callback reads the file; updateServerConfig then reads it too.
1261+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1262+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1263+ mcpHub . connections = [ connectionFor ( "global" ) ]
1264+
1265+ await mcpHub . updateServerTimeout ( "test-server" , 120 )
1266+
1267+ const writes = vi
1268+ . mocked ( safeWriteJson )
1269+ . mock . calls . filter ( ( call ) => String ( call [ 0 ] ) . includes ( "mcp_settings" ) )
1270+ expect ( writes . length ) . toBeGreaterThan ( 0 )
1271+ for ( const write of writes ) {
1272+ expect ( write [ 2 ] ?. confineTo ) . toBeUndefined ( )
1273+ }
1274+ } )
1275+
1276+ it ( "confines a project-scoped server deletion to the workspace root" , async ( ) => {
1277+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1278+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1279+ mcpHub . connections = [ connectionFor ( "project" ) ]
1280+
1281+ await mcpHub . deleteServer ( "test-server" , "project" )
1282+
1283+ const write = vi . mocked ( safeWriteJson ) . mock . calls . find ( ( call ) => String ( call [ 0 ] ) . includes ( ".roo" ) )
1284+ expect ( write ) . toBeDefined ( )
1285+ expect ( write ! [ 2 ] ) . toEqual ( expect . objectContaining ( { confineTo : "/mock/workspace" } ) )
1286+ } )
1287+
1288+ it ( "leaves a global-scoped server deletion unconstrained" , async ( ) => {
1289+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1290+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1291+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1292+ mcpHub . connections = [ connectionFor ( "global" ) ]
1293+
1294+ await mcpHub . deleteServer ( "test-server" , "global" )
1295+
1296+ const writes = vi
1297+ . mocked ( safeWriteJson )
1298+ . mock . calls . filter ( ( call ) => String ( call [ 0 ] ) . includes ( "mcp_settings" ) )
1299+ expect ( writes . length ) . toBeGreaterThan ( 0 )
1300+ for ( const write of writes ) {
1301+ expect ( write [ 2 ] ?. confineTo ) . toBeUndefined ( )
1302+ }
1303+ } )
1304+ it ( "uses the fallback workspace path for both the settings path and the confinement root when the provider carries no cwd" , async ( ) => {
1305+ // The provider is reachable but has no cwd, which is the state the `?? getWorkspacePath()` fallback at
1306+ // src/services/mcp/McpHub.ts:2080 exists for. The row pins that ONE root serves both purposes: the
1307+ // project settings path is built from the fallback and the confinement root handed to safeWriteJson is
1308+ // that same fallback, so a project write taken without a provider cwd is confined to the very directory
1309+ // it writes into rather than going unconstrained or being confined to a different directory. cwd is a
1310+ // getter on ClineProvider, so the test installs the absent value.
1311+ Object . defineProperty ( mockProvider , "cwd" , { value : undefined , configurable : true } )
1312+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce ( projectConfigJson )
1313+ mcpHub . connections = [ connectionFor ( "project" ) ]
1314+
1315+ await mcpHub . updateServerTimeout ( "test-server" , 120 )
1316+
1317+ const write = vi . mocked ( safeWriteJson ) . mock . calls . find ( ( call ) => String ( call [ 0 ] ) . includes ( ".roo" ) )
1318+ expect ( write ) . toBeDefined ( )
1319+ expect ( String ( write ! [ 0 ] ) . split ( "\\" ) . join ( "/" ) ) . toContain ( "/fallback/workspace/.roo/mcp.json" )
1320+ expect ( write ! [ 2 ] ) . toEqual ( expect . objectContaining ( { confineTo : "/fallback/workspace" } ) )
1321+ } )
1322+ } )
1323+
12081324 describe ( "toggleToolAlwaysAllow" , ( ) => {
1325+ const projectConnection = ( source : "global" | "project" = "project" ) : ConnectedMcpConnection =>
1326+ connectionFor ( source )
1327+
1328+ it ( "confines a project-scoped allowlist write to the workspace root" , async ( ) => {
1329+ // A repository can ship .roo/mcp.json as a symlink to a file outside the workspace.
1330+ // safeWriteJson resolves the publish target before staging, so without confinement an
1331+ // allowlist edit would replace that outside file. The workspace root has to be handed
1332+ // over as confineTo so the write fails closed instead.
1333+ // cwd is a read-only getter on ClineProvider, so the test installs the value.
1334+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1335+ vi . mocked ( fs . readFile ) . mockResolvedValueOnce (
1336+ JSON . stringify ( {
1337+ mcpServers : {
1338+ "test-server" : { type : "stdio" , command : "node" , args : [ "test.js" ] , alwaysAllow : [ ] } ,
1339+ } ,
1340+ } ) ,
1341+ )
1342+ mcpHub . connections = [ projectConnection ( ) ]
1343+
1344+ await mcpHub . toggleToolAlwaysAllow ( "test-server" , "project" , "new-tool" , true )
1345+
1346+ const write = vi . mocked ( safeWriteJson ) . mock . calls . find ( ( call ) => String ( call [ 0 ] ) . includes ( "mcp.json" ) )
1347+ expect ( write ) . toBeDefined ( )
1348+ expect ( write ! [ 2 ] ) . toEqual ( expect . objectContaining ( { confineTo : "/mock/workspace" } ) )
1349+ } )
1350+
1351+ it ( "leaves a global-scoped allowlist write unconstrained" , async ( ) => {
1352+ // Global settings live in the user's own settings directory, which is not under the
1353+ // workspace; confining them would break every global edit.
1354+ Object . defineProperty ( mockProvider , "cwd" , { value : "/mock/workspace" , configurable : true } )
1355+ mcpHub . connections = [ projectConnection ( "global" ) ]
1356+
1357+ await mcpHub . toggleToolAlwaysAllow ( "test-server" , "global" , "another-tool" , true )
1358+
1359+ // Name the exact call instead of picking any match: a find on the substring
1360+ // "mcp" returns the FIRST mcp-named write, which need not be the allowlist
1361+ // write - the default-creation write getMcpSettingsFilePath makes when the
1362+ // settings file is missing (McpHub.ts:513) also matches, and it never passes
1363+ // confineTo, so the assertion could judge that write and stay green while a
1364+ // confinement root on the real allowlist write went unnoticed. Check every
1365+ // write to the settings file, as the timeout and delete tests above do.
1366+ const writes = vi
1367+ . mocked ( safeWriteJson )
1368+ . mock . calls . filter ( ( call ) => String ( call [ 0 ] ) . endsWith ( "mcp_settings.json" ) )
1369+ expect ( writes . length ) . toBeGreaterThan ( 0 )
1370+ // undefined confineTo is the unconstrained case: safeWriteJson only checks the path
1371+ // when a confinement root is supplied.
1372+ for ( const write of writes ) {
1373+ expect ( write [ 2 ] ?. confineTo ) . toBeUndefined ( )
1374+ }
1375+ } )
1376+
12091377 it ( "should add tool to always allow list when enabling" , async ( ) => {
12101378 const mockConfig = {
12111379 mcpServers : {
0 commit comments