Skip to content

Commit db0d756

Browse files
committed
fix(mcp): prevent filesystem escapes through symbolic links
Resolve canonical paths for file operations and validate existing parents before creating files. Reject symbolic-link write targets and propagate containment failures before documentation generation. Add regression coverage for symlinks and junctions through FileService and MCP tools, and document the security boundary and concurrency limitations. Validation: typecheck, lint, smoke tests, build, 2126 passing tests and 130 browser tests. Changed TypeScript files have no SonarQube issues.
1 parent 50b4f97 commit db0d756

6 files changed

Lines changed: 348 additions & 16 deletions

File tree

‎mcp/README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ The generation step sends selected project content to the configured provider.
115115
- Search: up to 5,000 entries, 200 matches, 240 characters per matching line.
116116
- Documentation gathering: first 40 Markdown files among up to 2,000 entries; files over 40,000 bytes are skipped. Gathered file and Markdown context is limited to 24,000 characters. This is not a global prompt limit: tree/package context, an existing document and a caller-supplied question can add content according to the selected tool.
117117
- Listing skips build/dependency directories such as `node_modules`, `.git`, `.next`, `out`, `dist`, `prebuilt`, `.turbo` and `coverage`. It does not interpret `.gitignore`; files such as `.env` are not automatically excluded. Run only in a trusted project and select the content to expose.
118-
- Path checks use lexical containment; do not treat the process as an operating-system sandbox or assume symlink/junction isolation. Loading `project://config` can import JavaScript/TypeScript configuration.
118+
- Filesystem tools check both lexical paths and canonical filesystem paths. Reads and directory listings reject symlinks/junctions that resolve outside the project root. Writes also validate the nearest existing parent before creating directories and reject symbolic links at the destination, including links to internal files. Search skips linked files whose contents cannot be read safely.
119+
- These checks protect against links already present when a request is handled; they are not an operating-system sandbox against concurrent path replacement by another process. Run the server in a trusted project with appropriate filesystem permissions. Configuration loading and provider-selection persistence use separate filesystem paths; loading `project://config` can import JavaScript/TypeScript configuration.
119120
- Tool failures include `isError: true`. Missing resources return `Unavailable: ...`. Check the client's stderr log for startup errors; stdout is reserved for JSON-RPC.
120121
- If tools work but generation fails, verify provider selection, environment credentials, model availability and service connectivity.
121122

‎mcp/src/tools/register-docs.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
3-
import { DocumentationService, patchManagedRegion, type DocKind, type AiProviderId } from "@gitpagedocs/tools";
3+
import { DocumentationService, RepositoryError, patchManagedRegion, type DocKind, type AiProviderId } from "@gitpagedocs/tools";
44
import type { ServerContext } from "../context";
55
import { text, safe } from "../result";
66
import { repoListing, fileContext, packageContext, gitLog, docsContext } from "../gather";
@@ -19,6 +19,11 @@ async function generate(
1919

2020
const providerOpt = z.string().optional().describe("Override the configured AI provider id.");
2121

22+
function isMissingFile(error: unknown): boolean {
23+
const cause = error instanceof RepositoryError ? error.cause : undefined;
24+
return typeof cause === "object" && cause !== null && "code" in cause && cause.code === "ENOENT";
25+
}
26+
2227
/** Documentation generators and analyzers (AI-backed, delegating to tools). */
2328
export function registerDocsTools(server: McpServer, ctx: ServerContext): void {
2429
const def = (
@@ -81,8 +86,8 @@ export function registerDocsTools(server: McpServer, ctx: ServerContext): void {
8186
let existing = "";
8287
try {
8388
existing = await ctx.files.read(path);
84-
} catch {
85-
existing = "";
89+
} catch (error) {
90+
if (!isMissingFile(error)) throw error;
8691
}
8792
const generated = await generate(
8893
ctx,
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2+
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
3+
import os from "node:os";
4+
import path from "node:path";
5+
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
6+
import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
7+
import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types.js";
8+
import { createServer } from "../src/server";
9+
10+
const FIXTURE_PREFIX = path.join(os.tmpdir(), "gpd-mcp-containment-");
11+
const EXTERNAL_CONTENT = "# External fixture\nOUTSIDE_MARKER\n";
12+
const providerRequest = vi.fn<typeof fetch>(async () => new Response(
13+
'data: {"choices":[{"delta":{"content":"Generated documentation"}}]}\n\ndata: [DONE]\n\n',
14+
{ status: 200 },
15+
));
16+
let fixture: string;
17+
let root: string;
18+
let outside: string;
19+
let client: Client;
20+
let server: ReturnType<typeof createServer>;
21+
22+
function toolText(result: unknown): string {
23+
return CallToolResultSchema.parse(result).content.flatMap((block) => block.type === "text" ? [block.text] : []).join("\n");
24+
}
25+
26+
beforeEach(async () => {
27+
fixture = mkdtempSync(FIXTURE_PREFIX);
28+
root = path.join(fixture, "root");
29+
outside = path.join(fixture, "outside");
30+
mkdirSync(path.join(root, "docs"), { recursive: true });
31+
mkdirSync(outside);
32+
writeFileSync(path.join(outside, "secret.md"), EXTERNAL_CONTENT, "utf8");
33+
symlinkSync(outside, path.join(root, "docs", "linked"), process.platform === "win32" ? "junction" : "dir");
34+
providerRequest.mockClear();
35+
vi.stubEnv("OPENAI_API_KEY", "fixture-key");
36+
vi.stubGlobal("fetch", providerRequest);
37+
server = createServer(root);
38+
client = new Client({ name: "filesystem-containment-test", version: "1.0.0" });
39+
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
40+
await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]);
41+
});
42+
43+
afterEach(async () => {
44+
await client?.close();
45+
await server?.close();
46+
vi.unstubAllGlobals();
47+
vi.unstubAllEnvs();
48+
if (!path.resolve(fixture).startsWith(FIXTURE_PREFIX)) throw new Error("Unexpected fixture path");
49+
rmSync(fixture, { recursive: true, force: true });
50+
});
51+
52+
describe("MCP filesystem containment", () => {
53+
it("rejects documentation updates through an external directory link before contacting the provider", async () => {
54+
const result = await client.callTool({ name: "update_documentation", arguments: { path: "docs/linked/secret.md" } });
55+
expect(result.isError).toBe(true);
56+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: Path escapes the project root/);
57+
expect(providerRequest).not.toHaveBeenCalled();
58+
expect(readFileSync(path.join(outside, "secret.md"), "utf8")).toBe(EXTERNAL_CONTENT);
59+
});
60+
61+
it("rejects writes through an external directory link without replacing the external file", async () => {
62+
const result = await client.callTool({
63+
name: "write_file",
64+
arguments: { path: "docs/linked/secret.md", content: "# Replacement\n" },
65+
});
66+
expect(result.isError).toBe(true);
67+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: /);
68+
expect(readFileSync(path.join(outside, "secret.md"), "utf8")).toBe(EXTERNAL_CONTENT);
69+
});
70+
71+
it("rejects new documentation beneath an external directory link before contacting the provider", async () => {
72+
const result = await client.callTool({
73+
name: "update_documentation",
74+
arguments: { path: "docs/linked/new/guide.md" },
75+
});
76+
expect(result.isError).toBe(true);
77+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: Path escapes the project root/);
78+
expect(providerRequest).not.toHaveBeenCalled();
79+
expect(existsSync(path.join(outside, "new"))).toBe(false);
80+
expect(readFileSync(path.join(outside, "secret.md"), "utf8")).toBe(EXTERNAL_CONTENT);
81+
});
82+
83+
it("rejects reading an external file through a symbolic link", async () => {
84+
symlinkSync(path.join(outside, "secret.md"), path.join(root, "linked.md"), "file");
85+
const result = await client.callTool({ name: "read_file", arguments: { path: "linked.md" } });
86+
expect(result.isError).toBe(true);
87+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: Path escapes the project root/);
88+
expect(toolText(result)).not.toContain(EXTERNAL_CONTENT);
89+
});
90+
91+
it("rejects listing an external directory through a link", async () => {
92+
const result = await client.callTool({
93+
name: "list_files",
94+
arguments: { path: "docs/linked", recursive: true },
95+
});
96+
expect(result.isError).toBe(true);
97+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: Path escapes the project root/);
98+
expect(toolText(result)).not.toContain("secret.md");
99+
});
100+
101+
it("searches internal files without returning content from external file links", async () => {
102+
symlinkSync(path.join(outside, "secret.md"), path.join(root, "linked.md"), "file");
103+
writeFileSync(path.join(root, "docs", "internal.md"), "# Internal\nOUTSIDE_MARKER inside root\n", "utf8");
104+
const result = await client.callTool({
105+
name: "search_project",
106+
arguments: { query: "OUTSIDE_MARKER", extension: ".md" },
107+
});
108+
expect(result.isError).not.toBe(true);
109+
expect(JSON.parse(toolText(result))).toEqual([
110+
{ file: "docs/internal.md", line: 2, text: "OUTSIDE_MARKER inside root" },
111+
]);
112+
});
113+
114+
it("rejects creating files beneath an external directory link without creating external directories", async () => {
115+
const result = await client.callTool({
116+
name: "write_file",
117+
arguments: { path: "docs/linked/new/guide.md", content: "# New document\n" },
118+
});
119+
expect(result.isError).toBe(true);
120+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: Path escapes the project root/);
121+
expect(existsSync(path.join(outside, "new"))).toBe(false);
122+
expect(readFileSync(path.join(outside, "secret.md"), "utf8")).toBe(EXTERNAL_CONTENT);
123+
});
124+
125+
it("rejects writing through a file link without replacing the external file", async () => {
126+
symlinkSync(path.join(outside, "secret.md"), path.join(root, "linked.md"), "file");
127+
const result = await client.callTool({
128+
name: "write_file",
129+
arguments: { path: "linked.md", content: "# Replacement\n" },
130+
});
131+
expect(result.isError).toBe(true);
132+
expect(toolText(result)).toMatch(/^VALIDATION_ERROR: /);
133+
expect(readFileSync(path.join(outside, "secret.md"), "utf8")).toBe(EXTERNAL_CONTENT);
134+
});
135+
136+
});

‎tools/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ The **shared business-logic core** of the [Git Page Docs](../README.md) monorepo
1414
| `config-format/` | Browser-safe `compactSiteIcons` / `expandSiteIcons` and `applyRouteDefaults` / `factorRouteDefaults` for compact configuration and inherited route values |
1515
| `logger/` | Level-filtered logger with **secret redaction** |
1616
| `errors/` | `AppError` + typed subclasses |
17-
| `filesystem/` | `FileService` (list/read/write/search with lexical root containment; filesystem symlinks/junctions still require trusted deployment boundaries) |
17+
| `filesystem/` | `FileService` (list/read/write/search with lexical and canonical root containment; validates existing parents before creating files and rejects destination symlinks on writes; concurrent path replacement still requires trusted filesystem boundaries) |
1818
| `documentation/` | `DocumentationService`, `patchManagedRegion`, `DocUpdater` |
1919
| `i18n/` | Language bundles (`site.languages` toggles in `config.json` + `langs/<lang>.json`): `loadConfigLanguageBundles`, `applyLanguageBundles` (fold into `site.langmenu` / `translations`), `filterEnabledLanguages`, `splitLanguageBundles` (generator side); browser-safe |
2020
| `ports/` | Type-only contracts (logger, cache, crypto, security, config, ai) |

‎tools/src/filesystem/file-service.ts‎

Lines changed: 70 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import path from "node:path";
2-
import { readFile, writeFile, mkdir, readdir, stat } from "node:fs/promises";
2+
import type { Stats } from "node:fs";
3+
import { readFile, writeFile, mkdir, readdir, stat, realpath, lstat } from "node:fs/promises";
34
import { RepositoryError, ValidationError } from "../errors/app-error";
45

56
const DEFAULT_IGNORE = new Set([
@@ -60,36 +61,93 @@ function compareCodeUnits(a: string, b: string): number {
6061
return 0;
6162
}
6263

64+
async function inspectExistingPath(absolute: string): Promise<Stats | undefined> {
65+
try {
66+
return await lstat(absolute);
67+
} catch (cause) {
68+
if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") return undefined;
69+
throw cause;
70+
}
71+
}
72+
73+
function assertContained(root: string, absolute: string, requestedPath: string): void {
74+
const relative = path.relative(root, absolute);
75+
if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) {
76+
throw new ValidationError(`Path escapes the project root: ${requestedPath}`);
77+
}
78+
}
79+
80+
interface ResolvedPath {
81+
absolute: string;
82+
symbolicLink: boolean;
83+
}
84+
6385
/**
6486
* Root-bounded filesystem service. All paths are resolved relative to `root`
6587
* and may not escape it — the single safe FS surface for the CLI and MCP.
6688
*/
6789
export class FileService {
68-
constructor(private readonly root: string) {}
90+
private readonly root: string;
91+
92+
constructor(root: string) {
93+
this.root = path.resolve(root);
94+
}
6995

7096
private resolveInside(relativePath: string): string {
7197
const abs = path.resolve(this.root, relativePath);
72-
const rel = path.relative(this.root, abs);
73-
if (rel.startsWith("..") || path.isAbsolute(rel)) {
74-
throw new ValidationError(`Path escapes the project root: ${relativePath}`);
75-
}
98+
assertContained(this.root, abs, relativePath);
7699
return abs;
77100
}
78101

79102
private toRel(abs: string): string {
80103
return path.relative(this.root, abs).split(path.sep).join("/");
81104
}
82105

106+
private async resolvePhysicalPath(relativePath: string): Promise<ResolvedPath> {
107+
const requested = this.resolveInside(relativePath);
108+
try {
109+
const root = await realpath(this.root);
110+
let ancestor = requested;
111+
const missing: string[] = [];
112+
let existing = await inspectExistingPath(ancestor);
113+
while (!existing) {
114+
const parent = path.dirname(ancestor);
115+
if (parent === ancestor) throw new RepositoryError(`Cannot resolve path: ${relativePath}`);
116+
missing.unshift(path.basename(ancestor));
117+
ancestor = parent;
118+
existing = await inspectExistingPath(ancestor);
119+
}
120+
const resolved = await realpath(ancestor).catch((cause) => {
121+
if (existing.isSymbolicLink()) {
122+
throw new ValidationError(`Cannot resolve symbolic link: ${relativePath}`, { cause });
123+
}
124+
throw cause;
125+
});
126+
assertContained(root, resolved, relativePath);
127+
return { absolute: path.join(resolved, ...missing), symbolicLink: missing.length === 0 && existing.isSymbolicLink() };
128+
} catch (cause) {
129+
if (cause instanceof ValidationError || cause instanceof RepositoryError) throw cause;
130+
throw new RepositoryError(`Cannot resolve path: ${relativePath}`, { cause });
131+
}
132+
}
133+
134+
private async resolveWritablePath(relativePath: string): Promise<string> {
135+
const resolved = await this.resolvePhysicalPath(relativePath);
136+
if (resolved.symbolicLink) throw new ValidationError(`Cannot write through a symbolic link: ${relativePath}`);
137+
return resolved.absolute;
138+
}
139+
83140
async list(relativeDir = ".", options: ListOptions = {}): Promise<string[]> {
84141
const { recursive = false, maxEntries = 2000 } = options;
85142
const start = this.resolveInside(relativeDir);
86143
const out: string[] = [];
87144

88145
const walk = async (dir: string): Promise<void> => {
89146
if (out.length >= maxEntries) return;
147+
const resolved = await this.resolvePhysicalPath(this.toRel(dir));
90148
let entries: import("node:fs").Dirent[];
91149
try {
92-
entries = await readdir(dir, { withFileTypes: true });
150+
entries = await readdir(resolved.absolute, { withFileTypes: true });
93151
} catch (cause) {
94152
throw new RepositoryError(`Cannot read directory: ${this.toRel(dir)}`, { cause });
95153
}
@@ -107,7 +165,7 @@ export class FileService {
107165
}
108166

109167
async read(relativePath: string, maxBytes = 512_000): Promise<string> {
110-
const abs = this.resolveInside(relativePath);
168+
const { absolute: abs } = await this.resolvePhysicalPath(relativePath);
111169
const info = await stat(abs).catch((cause) => {
112170
throw new RepositoryError(`File not found: ${relativePath}`, { cause });
113171
});
@@ -118,10 +176,11 @@ export class FileService {
118176
}
119177

120178
async write(relativePath: string, content: string): Promise<string> {
121-
const abs = this.resolveInside(relativePath);
179+
const abs = await this.resolveWritablePath(relativePath);
122180
await mkdir(path.dirname(abs), { recursive: true });
123-
await writeFile(abs, content, "utf8");
124-
return this.toRel(abs);
181+
const target = await this.resolveWritablePath(relativePath);
182+
await writeFile(target, content, "utf8");
183+
return this.toRel(this.resolveInside(relativePath));
125184
}
126185

127186
/** Directories are skipped; an explicit extension filter replaces the text-extension default. */

0 commit comments

Comments
 (0)