Skip to content

Commit 29db105

Browse files
cliffhallcliffhall
authored
Merge pull request #5099 from modelcontextprotocol/v2/fix/5097-release-1.0.0-cherry-pick
v1.0.0: cherry-pick the #5097 roots refresh ordering fix onto the release branch
2 parents cebc963 + ee1ad25 commit 29db105

3 files changed

Lines changed: 213 additions & 16 deletions

File tree

‎src/filesystem/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,5 @@ The first semantic version, resuming semver after the date-stamped releases (las
2323
- [#5009](https://github.com/modelcontextprotocol/servers/pull/5009) [`f621c75`](https://github.com/modelcontextprotocol/servers/commit/f621c75a92347738f28f26c9461286764beba3c8) [@cliffhall](https://github.com/cliffhall) - Paths that differ from the names on disk only in Unicode form now resolve ([#1970](https://github.com/modelcontextprotocol/servers/issues/1970)). An NFC spelling of an allowed directory whose name is stored NFD (macOS "Capture d’écran", Japanese dakuten) is no longer refused as outside the allowed directories, and a request that spells a U+202F or U+00A0 in a name as a plain space (the macOS screenshot "Screenshot … at 2.40.40 PM.png") finds the file instead of failing with ENOENT. The exact spelling still wins when it exists, and a name matching two entries is refused as ambiguous.
2424

2525
- [#5094](https://github.com/modelcontextprotocol/servers/issues/5094) [`bea1c9a`](https://github.com/modelcontextprotocol/servers/commit/bea1c9a1bd623aea093a4408b0f71e95cd0e1d03) [@cliffhall](https://github.com/cliffhall) - A `roots/list_changed` update that leaves no valid root now revokes access once the client's roots are in force, instead of keeping the previous allowed directories, so a client that withdraws its roots withdraws the server's access too (#5094). Every path is then refused with "Access denied - no allowed directories" until the client exposes a root again. Before any root has been in force, such an update still keeps the command-line directories.
26+
27+
- [#5097](https://github.com/modelcontextprotocol/servers/issues/5097) [`a13e809`](https://github.com/modelcontextprotocol/servers/commit/a13e809d95d6771159483a69e707d0466a9fe1b1) [@cliffhall](https://github.com/cliffhall) - Overlapping roots refreshes now take effect in the order they started: when a client sends `roots/list_changed` again before the previous `roots/list` answer arrived (or while the initial roots are still loading), an answer that arrives after a newer refresh has started is discarded instead of overwriting it, so a stale answer can no longer undo a newer update such as a revocation (#5097).

‎src/filesystem/__tests__/server-roots.test.ts‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,163 @@ describe("roots/list_changed", () => {
345345
expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
346346
});
347347

348+
// #5097: the answers to overlapping refreshes can arrive out of order; only
349+
// the newest refresh's answer is applied, so a stale one cannot undo it.
350+
it("applies the newest of two overlapping updates when their answers arrive out of order (#5097)", async () => {
351+
let calls = 0;
352+
let releaseStale!: () => void;
353+
const staleHeld = new Promise<void>((resolve) => (releaseStale = resolve));
354+
const { client } = await connectTracked([], {
355+
capabilities: ROOTS,
356+
listRoots: async () => {
357+
calls += 1;
358+
if (calls === 1) return [rootOf(rootDir)]; // the initial load
359+
if (calls === 2) {
360+
await staleHeld; // the older update, answered last
361+
return [rootOf(rootDir)];
362+
}
363+
return []; // the newer update withdraws the root
364+
},
365+
});
366+
await vi.waitFor(async () =>
367+
expect(await allowedDirectoriesOf(client)).toEqual([rootDir]),
368+
);
369+
370+
await client.sendRootsListChanged();
371+
await vi.waitFor(() => expect(calls).toBe(2));
372+
await client.sendRootsListChanged();
373+
await vi.waitFor(async () =>
374+
expect(await allowedDirectoriesOf(client)).toEqual([]),
375+
);
376+
377+
releaseStale();
378+
await vi.waitFor(() =>
379+
expect(stderr).toHaveBeenCalledWith(
380+
"Discarded a stale roots/list answer: a newer roots update has started",
381+
),
382+
);
383+
expect(await allowedDirectoriesOf(client)).toEqual([]);
384+
});
385+
386+
it("discards the initial answer when an update overtakes it (#5097)", async () => {
387+
let calls = 0;
388+
let releaseInitial!: () => void;
389+
const initialHeld = new Promise<void>(
390+
(resolve) => (releaseInitial = resolve),
391+
);
392+
const { client } = await connectTracked([], {
393+
capabilities: ROOTS,
394+
listRoots: async () => {
395+
calls += 1;
396+
if (calls === 1) {
397+
await initialHeld;
398+
return [rootOf(rootDir)];
399+
}
400+
return [rootOf(cliDir)];
401+
},
402+
});
403+
await vi.waitFor(() => expect(calls).toBe(1));
404+
await client.sendRootsListChanged();
405+
await vi.waitFor(() =>
406+
expect(stderr).toHaveBeenCalledWith(
407+
"Updated allowed directories from MCP roots: 1 valid directories",
408+
),
409+
);
410+
411+
releaseInitial();
412+
await vi.waitFor(() =>
413+
expect(stderr).toHaveBeenCalledWith(
414+
"Discarded a stale roots/list answer: a newer roots update has started",
415+
),
416+
);
417+
expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
418+
});
419+
420+
// #5097: a call waiting for the initial roots must not be released by the
421+
// stale initial answer; it waits for the update that overtook it.
422+
it("holds waiting tool calls until the update that overtook the initial load applies (#5097)", async () => {
423+
let calls = 0;
424+
let releaseInitial!: () => void;
425+
let releaseUpdate!: () => void;
426+
const initialHeld = new Promise<void>(
427+
(resolve) => (releaseInitial = resolve),
428+
);
429+
const updateHeld = new Promise<void>(
430+
(resolve) => (releaseUpdate = resolve),
431+
);
432+
const { client } = await connectTracked([cliDir], {
433+
capabilities: ROOTS,
434+
listRoots: async () => {
435+
calls += 1;
436+
await (calls === 1 ? initialHeld : updateHeld);
437+
return [rootOf(rootDir)];
438+
},
439+
});
440+
await vi.waitFor(() => expect(calls).toBe(1));
441+
await client.sendRootsListChanged();
442+
await vi.waitFor(() => expect(calls).toBe(2));
443+
444+
releaseInitial();
445+
await vi.waitFor(() =>
446+
expect(stderr).toHaveBeenCalledWith(
447+
"Discarded a stale roots/list answer: a newer roots update has started",
448+
),
449+
);
450+
// Asked before the update has answered: it must not run against the
451+
// command-line directory in the meantime.
452+
let settled = false;
453+
const pending = call(client, "list_directory", { path: cliDir }).then(
454+
(result) => {
455+
settled = true;
456+
return result;
457+
},
458+
);
459+
await new Promise((resolve) => setTimeout(resolve, 50));
460+
expect(settled).toBe(false);
461+
462+
releaseUpdate();
463+
expect(textOf(await pending)).toMatch(/^Access denied/);
464+
expect(await allowedDirectoriesOf(client)).toEqual([rootDir]);
465+
});
466+
467+
it("releases waiting tool calls when the update that overtook the initial load fails (#5097)", async () => {
468+
let calls = 0;
469+
let releaseInitial!: () => void;
470+
const initialHeld = new Promise<void>(
471+
(resolve) => (releaseInitial = resolve),
472+
);
473+
const { client } = await connectTracked([cliDir], {
474+
capabilities: ROOTS,
475+
listRoots: async () => {
476+
calls += 1;
477+
if (calls === 1) {
478+
await initialHeld;
479+
return [rootOf(rootDir)];
480+
}
481+
throw new Error("gone");
482+
},
483+
});
484+
await vi.waitFor(() => expect(calls).toBe(1));
485+
await client.sendRootsListChanged();
486+
await vi.waitFor(() =>
487+
expect(stderr).toHaveBeenCalledWith(
488+
"Failed to request roots from client:",
489+
"MCP error -32603: gone",
490+
),
491+
);
492+
493+
// Asked while the initial roots/list is still unanswered: the failed update
494+
// releases waiting calls instead of leaving them blocked on it.
495+
expect(await allowedDirectoriesOf(client)).toEqual([cliDir]);
496+
// Let the stale initial answer finish before teardown removes its roots.
497+
releaseInitial();
498+
await vi.waitFor(() =>
499+
expect(stderr).toHaveBeenCalledWith(
500+
"Discarded a stale roots/list answer: a newer roots update has started",
501+
),
502+
);
503+
});
504+
348505
it("logs and keeps the current directories when the re-fetch fails", async () => {
349506
let fail = false;
350507
const { client } = await connectTracked([], {

‎src/filesystem/server.ts‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,12 @@ export function createServer(
177177
// withdraws its roots withdraws the server's access too (#5094). Until then an
178178
// update with no valid roots keeps the command-line directories.
179179
let rootsInForce = false;
180+
// Each roots refresh (the initial load and every roots/list_changed) takes
181+
// the next generation before it asks for roots. The SDK runs notification
182+
// handlers without waiting for earlier ones, so answers can come back out of
183+
// order; one is applied only if no newer refresh has started since, so a
184+
// stale answer cannot undo a newer update, such as a revocation (#5097).
185+
let rootsGeneration = 0;
180186

181187
const server = new McpServer({
182188
name: "secure-filesystem-server",
@@ -190,14 +196,24 @@ export function createServer(
190196
// command-line directories (#3204). It is replaced in oninitialized, which
191197
// the SDK runs before any request sent after notifications/initialized.
192198
let initialization: Promise<void> = Promise.resolve();
199+
// True from the initial roots/list until a refresh's answer has been
200+
// applied or the newest refresh has failed. A roots/list_changed that
201+
// overtakes the initial load while this holds replaces `initialization`, so
202+
// waiting tool calls keep waiting for the refresh that will actually apply
203+
// rather than running against the command-line directories (#5097).
204+
let initialRootsPending = false;
193205
const registerTool = server.registerTool.bind(server);
194206
server.registerTool = ((
195207
name: string,
196208
config: Parameters<typeof registerTool>[1],
197209
handler: (...args: unknown[]) => unknown,
198210
) =>
199211
registerTool(name, config, (async (...args: unknown[]) => {
200-
await initialization;
212+
let awaited: Promise<void>;
213+
do {
214+
awaited = initialization;
215+
await awaited;
216+
} while (awaited !== initialization);
201217
return handler(...args);
202218
}) as never)) as typeof server.registerTool;
203219

@@ -763,9 +779,19 @@ export function createServer(
763779
},
764780
);
765781

766-
// Updates allowed directories based on MCP client roots
767-
async function updateAllowedDirectoriesFromRoots(requestedRoots: Root[]) {
782+
// Updates allowed directories based on MCP client roots, unless a newer
783+
// refresh started after this one's `generation` was taken (#5097).
784+
async function updateAllowedDirectoriesFromRoots(
785+
requestedRoots: Root[],
786+
generation: number,
787+
) {
768788
const validatedRootDirs = await getValidRootDirectories(requestedRoots);
789+
if (generation !== rootsGeneration) {
790+
console.error(
791+
"Discarded a stale roots/list answer: a newer roots update has started",
792+
);
793+
return;
794+
}
769795
if (validatedRootDirs.length > 0) {
770796
allowedDirectories = [...validatedRootDirs];
771797
rootsInForce = true;
@@ -786,19 +812,27 @@ export function createServer(
786812
server.server.setNotificationHandler(
787813
RootsListChangedNotificationSchema,
788814
async () => {
789-
try {
790-
// Request the updated roots list from the client
791-
const response = await server.server.listRoots();
792-
/* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the implicit else cannot run */
793-
if (response && "roots" in response) {
794-
await updateAllowedDirectoriesFromRoots(response.roots);
815+
const generation = ++rootsGeneration;
816+
const refresh = (async () => {
817+
try {
818+
// Request the updated roots list from the client
819+
const response = await server.server.listRoots();
820+
/* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the implicit else cannot run */
821+
if (response && "roots" in response) {
822+
await updateAllowedDirectoriesFromRoots(response.roots, generation);
823+
}
824+
} catch (error) {
825+
console.error(
826+
"Failed to request roots from client:",
827+
error instanceof Error ? error.message : String(error),
828+
);
829+
} finally {
830+
if (generation === rootsGeneration) initialRootsPending = false;
795831
}
796-
} catch (error) {
797-
console.error(
798-
"Failed to request roots from client:",
799-
error instanceof Error ? error.message : String(error),
800-
);
801-
}
832+
})();
833+
// Overtaking the initial load: tool calls wait for this refresh instead.
834+
if (initialRootsPending) initialization = refresh;
835+
await refresh;
802836
},
803837
);
804838

@@ -812,11 +846,13 @@ export function createServer(
812846
const clientCapabilities = server.server.getClientCapabilities();
813847

814848
if (clientCapabilities?.roots) {
849+
const generation = ++rootsGeneration;
850+
initialRootsPending = true;
815851
try {
816852
const response = await server.server.listRoots();
817853
/* v8 ignore else -- the SDK validates the roots/list result against ListRootsResultSchema, which requires roots, so the else cannot run */
818854
if (response && "roots" in response) {
819-
await updateAllowedDirectoriesFromRoots(response.roots);
855+
await updateAllowedDirectoriesFromRoots(response.roots, generation);
820856
} else {
821857
console.error(
822858
"Client returned no roots set, keeping current settings",
@@ -827,6 +863,8 @@ export function createServer(
827863
"Failed to request initial roots from client:",
828864
error instanceof Error ? error.message : String(error),
829865
);
866+
} finally {
867+
if (generation === rootsGeneration) initialRootsPending = false;
830868
}
831869
} else {
832870
if (allowedDirectories.length > 0) {

0 commit comments

Comments
 (0)