pdf-server: robust path validation + folder/file root support (#497)
* pdf-server: support folders in cli + files in roots * fix(pdf-server): use path.relative for robust ancestor dir matching - Replace string prefix matching (startsWith) with path.relative-based isAncestorDir() which correctly handles trailing slashes, path normalization, and prevents both .. traversal and prefix attacks. - Use path.resolve consistently when adding paths to allowedLocalFiles and allowedLocalDirs (main.ts was missing this). - Use resolved path for exact file match check in validateUrl. - Improve error message formatting for easier debugging. - Add tests for trailing slashes, grandparent dirs, and isAncestorDir. * chore(pdf-server): remove JSONL interaction logging Remove the withLogging transport wrapper, logInteraction helper, InteractionLogger type, and all log parameter threading that were added for debugging. * fix(pdf-server): support computer:// URL scheme for local files Some clients (e.g. Claude Code) use computer:// instead of file:// for local file references. Handle both schemes in isFileUrl and fileUrlToPath. * fix(pdf-server): add debug logging for rejected files, ignore .jsonl logs * fix(pdf-server): accept bare file paths in addition to file:// URLs Clients like Claude Desktop may pass raw absolute paths (e.g. /Users/.../file.pdf) instead of file:// URLs. Handle these in validateUrl and readPdfRange. * debug(pdf-server): add hex char diagnostics for path mismatch * fix(pdf-server): resolve symlinks to handle sandbox path remapping Use fs.realpathSync to resolve symlinks/bind mounts when comparing file paths against allowed directories. This fixes the path namespace mismatch where Claude Desktop sends container paths (e.g. /sessions/...) while MCP roots use host paths (e.g. /Users/...). Both the file path and directory paths are resolved through symlinks before comparison, so either side can be a symlink. Also stores realpath of roots in allowedLocalDirs/Files so matching works in both directions. Removes verbose hex diagnostics (no longer needed). * fix(pdf-server): decode percent-encoded bare paths (e.g. %20 for spaces) Clients may send bare file paths with percent-encoded characters (e.g., Application%20Support). Apply decodeURIComponent to bare paths before resolving, matching the behavior already used for file:// URLs. * fix(pdf-server): add full diagnostics to client-facing error message Include url, resolved path, realpath, allowedFiles, and per-directory path.relative results in the error returned to the client, so we can diagnose why isAncestorDir fails when paths look identical. * debug(pdf-server): add hex dump to diagnose invisible char differences path.relative returns 10 levels of .. despite paths looking identical, meaning they differ at the byte level. Add hex dump of first 30 chars of both paths to catch invisible Unicode or encoding differences. * fix(pdf-server): use inode comparison for bind-mount path matching The cowork VM remaps paths via bind mounts: the tool input uses VM-internal paths (/sessions/...) while MCP roots use host paths (/Users/...). Since path.relative can't match across bind mounts, compare directory inodes instead (dev+ino from fs.statSync). Walks up the file's parent directories checking each against allowed dirs by inode, which works regardless of path namespace differences. * fix(pdf-server): resolve VM-internal paths via directory basename matching When the MCP server runs on the host but receives unrewritten VM paths (e.g., /sessions/name/mnt/uploads/file.pdf), the tool call url argument isn't rewritten by the cowork VM proxy because it doesn't know which arguments are file paths. Fix: when path validation fails, try to match the file by finding the directory basename (e.g., 'uploads') in the VM path and looking for the relative path suffix under each allowed directory on the host. Also returns the resolved host path so readPdfRange reads the correct file on the host filesystem. * revert(pdf-server): remove resolveVmPath hack The VM path mismatch (VM-internal paths not rewritten in tool call arguments) should be fixed in the cowork VM proxy layer, not worked around in the MCP server. Removes: - resolveVmPath() function - resolvedPath field from validateUrl return type - effectiveUrl remapping in tool handlers - VM path resolution test * nit
This commit is contained in:
Olivier Chafik
GitHub
parent
ec389f473b
commit
281a26b021
1 changed files with 264 additions and 30 deletions
|
|
@ -60,7 +60,7 @@ const DIST_DIR = import.meta.filename.endsWith(".ts")
|
|||
// =============================================================================
|
||||
|
||||
export function isFileUrl(url: string): boolean {
|
||||
return url.startsWith("file://");
|
||||
return url.startsWith("file://") || url.startsWith("computer://");
|
||||
}
|
||||
|
||||
export function isArxivUrl(url: string): boolean {
|
||||
|
|
@ -81,7 +81,8 @@ export function normalizeArxivUrl(url: string): string {
|
|||
}
|
||||
|
||||
export function fileUrlToPath(fileUrl: string): string {
|
||||
return decodeURIComponent(fileUrl.replace("file://", ""));
|
||||
// Support both file:// and computer:// (used by some clients for local files)
|
||||
return decodeURIComponent(fileUrl.replace(/^(?:file|computer):\/\//, ""));
|
||||
}
|
||||
|
||||
export function pathToFileUrl(filePath: string): string {
|
||||
|
|
@ -89,29 +90,86 @@ export function pathToFileUrl(filePath: string): string {
|
|||
return `file://${encodeURIComponent(absolutePath).replace(/%2F/g, "/")}`;
|
||||
}
|
||||
|
||||
export function validateUrl(url: string): { valid: boolean; error?: string } {
|
||||
if (isFileUrl(url)) {
|
||||
const filePath = fileUrlToPath(url);
|
||||
/**
|
||||
* Check if `dir` is an ancestor of `filePath` using path.relative,
|
||||
* which is more robust than string prefix matching (handles normalization).
|
||||
*/
|
||||
export function isAncestorDir(dir: string, filePath: string): boolean {
|
||||
const rel = path.relative(dir, filePath);
|
||||
// Must be non-empty (not the dir itself when checking files),
|
||||
// must not start with ".." (escaping), and must not be absolute (different root).
|
||||
return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if `url` looks like an absolute local file path (not a URL scheme).
|
||||
* Handles Unix paths (/...), home-relative (~), and Windows drive letters (C:\...).
|
||||
*/
|
||||
function isLocalPath(url: string): boolean {
|
||||
return (
|
||||
url.startsWith("/") || url.startsWith("~") || /^[A-Za-z]:[/\\]/.test(url)
|
||||
);
|
||||
}
|
||||
|
||||
export function validateUrl(url: string): {
|
||||
valid: boolean;
|
||||
error?: string;
|
||||
} {
|
||||
if (isFileUrl(url) || isLocalPath(url)) {
|
||||
// fileUrlToPath already decodes percent-encoding; for bare paths,
|
||||
// decode here in case the client sends %20 for spaces etc.
|
||||
const filePath = isFileUrl(url)
|
||||
? fileUrlToPath(url)
|
||||
: decodeURIComponent(url);
|
||||
const resolved = path.resolve(filePath);
|
||||
|
||||
// Check exact match (CLI args)
|
||||
const exactMatch = allowedLocalFiles.has(filePath);
|
||||
// Check exact match (CLI args / roots)
|
||||
if (allowedLocalFiles.has(resolved)) {
|
||||
if (!fs.existsSync(resolved)) {
|
||||
return { valid: false, error: `File not found: ${resolved}` };
|
||||
}
|
||||
return { valid: true };
|
||||
}
|
||||
|
||||
// Check directory match (MCP roots)
|
||||
const dirMatch = [...allowedLocalDirs].some(
|
||||
(dir) => resolved === dir || resolved.startsWith(dir + path.sep),
|
||||
// Check directory match (MCP roots / CLI dirs).
|
||||
// Try both the raw path and its realpath (resolves symlinks).
|
||||
let realResolved: string | undefined;
|
||||
try {
|
||||
realResolved = fs.realpathSync(resolved);
|
||||
} catch {
|
||||
// File may not exist yet at this path
|
||||
}
|
||||
if (
|
||||
[...allowedLocalDirs].some((dir) => {
|
||||
let realDir: string | undefined;
|
||||
try {
|
||||
realDir = fs.realpathSync(dir);
|
||||
} catch {
|
||||
// Dir may not exist
|
||||
}
|
||||
return (
|
||||
isAncestorDir(dir, resolved) ||
|
||||
(realResolved != null && isAncestorDir(dir, realResolved)) ||
|
||||
(realDir != null && isAncestorDir(realDir, resolved)) ||
|
||||
(realDir != null &&
|
||||
realResolved != null &&
|
||||
isAncestorDir(realDir, realResolved))
|
||||
);
|
||||
})
|
||||
) {
|
||||
if (!fs.existsSync(resolved)) {
|
||||
return { valid: false, error: `File not found: ${resolved}` };
|
||||
}
|
||||
return { valid: true };
|
||||
}
|
||||
|
||||
console.error(
|
||||
`[pdf-server] Local file not in allowed list: ${resolved}\n Allowed dirs: ${[...allowedLocalDirs].join(", ")}`,
|
||||
);
|
||||
|
||||
if (!exactMatch && !dirMatch) {
|
||||
return {
|
||||
valid: false,
|
||||
error: `Local file not in allowed list: ${filePath}`,
|
||||
};
|
||||
}
|
||||
if (!fs.existsSync(filePath)) {
|
||||
return { valid: false, error: `File not found: ${filePath}` };
|
||||
}
|
||||
return { valid: true };
|
||||
return {
|
||||
valid: false,
|
||||
error: `Local file not in allowed list: ${resolved}\nAllowed directories: ${[...allowedLocalDirs].join(", ")}`,
|
||||
};
|
||||
}
|
||||
|
||||
// Remote URL - require HTTPS
|
||||
|
|
@ -238,8 +296,10 @@ export function createPdfCache(): PdfCache {
|
|||
const normalized = isArxivUrl(url) ? normalizeArxivUrl(url) : url;
|
||||
const clampedByteCount = Math.min(byteCount, MAX_CHUNK_BYTES);
|
||||
|
||||
if (isFileUrl(normalized)) {
|
||||
const filePath = fileUrlToPath(normalized);
|
||||
if (isFileUrl(normalized) || isLocalPath(normalized)) {
|
||||
const filePath = isFileUrl(normalized)
|
||||
? fileUrlToPath(normalized)
|
||||
: decodeURIComponent(normalized);
|
||||
const stats = await fs.promises.stat(filePath);
|
||||
const totalBytes = stats.size;
|
||||
|
||||
|
|
@ -353,11 +413,17 @@ async function refreshRoots(server: Server): Promise<void> {
|
|||
const { roots } = await server.listRoots();
|
||||
allowedLocalDirs.clear();
|
||||
for (const root of roots) {
|
||||
if (root.uri.startsWith("file://")) {
|
||||
if (isFileUrl(root.uri)) {
|
||||
const dir = fileUrlToPath(root.uri);
|
||||
const resolved = path.resolve(dir);
|
||||
try {
|
||||
if (fs.statSync(resolved).isDirectory()) {
|
||||
const s = fs.statSync(resolved);
|
||||
if (s.isFile()) {
|
||||
console.error(
|
||||
`[pdf-server] Root is a file, not a directory (skipped): ${resolved}`,
|
||||
);
|
||||
allowedLocalFiles.add(resolved);
|
||||
} else if (s.isDirectory()) {
|
||||
allowedLocalDirs.add(resolved);
|
||||
console.error(`[pdf-server] Root directory allowed: ${resolved}`);
|
||||
}
|
||||
|
|
@ -441,7 +507,7 @@ export function createServer(): McpServer {
|
|||
title: "Read PDF Bytes",
|
||||
description: "Read a range of bytes from a PDF (max 512KB per request)",
|
||||
inputSchema: {
|
||||
url: z.string().describe("PDF URL"),
|
||||
url: z.string().describe("PDF URL or local file path"),
|
||||
offset: z.number().min(0).default(0).describe("Byte offset"),
|
||||
byteCount: z
|
||||
.number()
|
||||
|
|
@ -471,7 +537,11 @@ export function createServer(): McpServer {
|
|||
|
||||
try {
|
||||
const normalized = isArxivUrl(url) ? normalizeArxivUrl(url) : url;
|
||||
const { data, totalBytes } = await readPdfRange(url, offset, byteCount);
|
||||
const { data, totalBytes } = await readPdfRange(
|
||||
normalized,
|
||||
offset,
|
||||
byteCount,
|
||||
);
|
||||
|
||||
// Base64 encode for JSON transport
|
||||
const bytes = Buffer.from(data).toString("base64");
|
||||
|
|
@ -520,7 +590,10 @@ Accepts:
|
|||
- Local files under directories provided by the client as MCP roots
|
||||
- Any remote PDF accessible via HTTPS`,
|
||||
inputSchema: {
|
||||
url: z.string().default(DEFAULT_PDF).describe("PDF URL"),
|
||||
url: z
|
||||
.string()
|
||||
.default(DEFAULT_PDF)
|
||||
.describe("PDF URL or local file path"),
|
||||
page: z.number().min(1).default(1).describe("Initial page"),
|
||||
},
|
||||
outputSchema: z.object({
|
||||
|
|
|
|||
Loading…
Reference in a new issue