From e4b61c92d0afc7214b908a29bf787d3a0caaeb77 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 00:34:48 +0200 Subject: [PATCH 01/19] Lay foundation for cross-platform savePath + write sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extend ErrorCode union with save_forbidden (8th code; not yet thrown) - Add src/sandbox.ts: pure containment helpers (buildAllowedRoots, checkPath) — leaf module, unit-testable, no console output. No hardcoded platform paths; every platform-dependent value is read from a Node API (os.tmpdir, process.cwd, path.isAbsolute, path.delimiter, fs.realpath, process.platform). - MCP savePath schema: startsWith('/') -> refine(path.isAbsolute); description now platform-appropriate (POSIX + Windows shapes). - Reframe T5 to use a path inside os.tmpdir() with a missing intermediate directory, so writeFile genuinely fails with ENOENT — preserves the save_failed regression guard once the sandbox enforcement lands. All 50 existing tests pass. Sandbox is not yet wired into the MCP handler — that wiring comes in a follow-up commit. --- src/core.ts | 3 +- src/mcp.ts | 5 +- src/sandbox.ts | 159 +++++++++++++++++++++++++++++++++++++++++++ tests/server.test.ts | 44 ++++++------ 4 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 src/sandbox.ts diff --git a/src/core.ts b/src/core.ts index f6122bc..7e7a924 100644 --- a/src/core.ts +++ b/src/core.ts @@ -154,7 +154,8 @@ export type ErrorCode = | "unsupported_content_type" | "extraction_failed" | "too_large" - | "save_failed"; + | "save_failed" + | "save_forbidden"; export class MarkfetchError extends Error { constructor( diff --git a/src/mcp.ts b/src/mcp.ts index 94f0d7c..675ec1f 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -13,6 +13,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { z } from "zod"; import { fetchMarkdown, classifyError, type ErrorCode } from "./core.js"; +import { isAbsolute } from "node:path"; function errorResult(code: ErrorCode, message: string) { return { @@ -37,10 +38,10 @@ server.registerTool( ), savePath: z .string() - .startsWith("/") + .refine(isAbsolute, "savePath must be an absolute filesystem path") .optional() .describe( - "Optional. When provided, the fetched markdown is written to this absolute filesystem path and the response becomes a small confirmation. Use this when the markdown might exceed your client's tool-result inline cap. Must be an absolute path starting with '/'; relative paths and tilde-paths ('~/...') are rejected by the schema. Existing files are overwritten; the parent directory must exist (caller's responsibility). The file is written only on fetch success — fetch / extraction / size-cap errors return a [code] string and never touch the file.", + "Optional. When provided, the fetched markdown is written to this absolute filesystem path and the response becomes a small confirmation. Use this when the markdown might exceed your client's tool-result inline cap. Must be an absolute path on the host platform (e.g., `/foo/bar.md` on POSIX; `C:\\foo\\bar.md` or `\\\\server\\share\\bar.md` on Windows); relative paths and tilde paths (`~/...`) are rejected by the schema. Existing files are overwritten; the parent directory must exist (caller's responsibility). The file is written only on fetch success — fetch / extraction / size-cap errors return a `[code]` string and never touch the file.", ), }, }, diff --git a/src/sandbox.ts b/src/sandbox.ts new file mode 100644 index 0000000..c6bb1ed --- /dev/null +++ b/src/sandbox.ts @@ -0,0 +1,159 @@ +// Write-path containment for the MCP adapter. +// +// The MCP tool exposes a `savePath` argument that lands on disk verbatim, +// and the caller is a language model — possibly steered by the page it just +// fetched. Without bounds, a hallucinated or injected path can target +// arbitrary filesystem locations under the process's UID. This module is +// the bound: it builds an allowed-roots set at startup and verifies each +// savePath stays inside it. +// +// Invariants (load-bearing — keep this list synchronized with SPEC.md): +// - Leaf module. No imports from siblings (core.ts, mcp.ts, cli.ts) so +// it stays unit-testable without spinning up the MCP server or pulling +// in undici / turndown / etc. +// - No console.* calls. buildAllowedRoots throws (the throw escapes +// module init in mcp.ts and surfaces on stderr per existing intEnv +// convention). checkPath returns a discriminated union so the caller +// decides the user-facing channel. +// - No hardcoded platform paths. Every platform-dependent value comes +// from a Node API: os.tmpdir(), process.cwd(), path.isAbsolute, +// path.delimiter, fs.realpath, process.platform. This is a non- +// negotiable review criterion for the module. +// +// Threat model: CLI is unrestricted (human is the security boundary). +// MCP is sandboxed (LLM caller is the threat surface). The asymmetry +// lives in the call site (mcp.ts uses this module; cli.ts does not). + +import { realpath, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { + delimiter, + dirname, + isAbsolute, + join, + parse, + relative, + resolve, +} from "node:path"; + +const ENV_VAR = "MARKFETCH_ALLOWED_WRITE_ROOTS"; + +export type CheckResult = + | { ok: true; resolved: string } + | { ok: false; reason: string }; + +// Build the set of directories MCP `savePath` writes are allowed into. +// +// If MARKFETCH_ALLOWED_WRITE_ROOTS is set, its entries REPLACE the default +// set entirely — "if you set it, you own the policy." Each entry must be +// absolute and must resolve via fs.realpath at startup; we error early so +// a bad config surfaces at module init rather than at first-write time +// (consistent with intEnv's fail-fast contract in core.ts). +// +// If unset, the default set is [realpath(os.tmpdir()), realpath(cwd)]. +// Both are realpath'd once at startup so symlink-following is stable +// across all later checkPath calls. +export async function buildAllowedRoots( + env: NodeJS.ProcessEnv, +): Promise { + const raw = env[ENV_VAR]; + if (raw != null && raw.trim() !== "") { + const entries = raw.split(delimiter).filter((s) => s !== ""); + if (entries.length === 0) { + throw new Error( + `Invalid ${ENV_VAR}=${JSON.stringify(raw)} — expected a ${JSON.stringify(delimiter)}-separated list of absolute paths.`, + ); + } + const resolved: string[] = []; + for (const entry of entries) { + if (!isAbsolute(entry)) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, + ); + } + try { + resolved.push(await realpath(entry)); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, + ); + } + } + return resolved; + } + return [await realpath(tmpdir()), await realpath(process.cwd())]; +} + +// Check whether a savePath, after symlink resolution, sits inside the +// allowed roots. Steps: +// 1. path.resolve to normalize "../" segments. +// 2. Walk upward to the deepest extant ancestor — the leaf usually +// doesn't exist yet (that's the point of "save"), so we can't realpath +// it directly; we realpath the closest existing directory and reattach +// the not-yet-existing trailing segments. +// 3. fs.realpath the extant ancestor — defeats symlink escape: a symlink +// planted inside the sandbox that points outside resolves to its +// outside-target before the containment check. +// 4. Reattach the trailing segments via path.join. +// 5. For each root, path.relative(root, reattached). Contained iff the +// relative path is empty (target == root), or doesn't start with ".." +// and isn't itself absolute (target is below root). On win32, fold +// both sides to lowercase first — the filesystem is case-insensitive +// and realpath doesn't reliably canonicalize case. +export async function checkPath( + savePath: string, + roots: string[], +): Promise { + const normalized = resolve(savePath); + + // Walk up to find an extant ancestor. cwd or filesystem root is always + // extant in practice; this terminates at parse(p).root for the pathological + // "non-existent drive letter on Windows" case, where we fail closed. + let ancestor = normalized; + const trailing: string[] = []; + // Deliberate single-pass loop; iteration count is bounded by path depth. + // eslint-disable-next-line no-constant-condition + while (true) { + try { + await stat(ancestor); + break; + } catch { + const parent = dirname(ancestor); + if (parent === ancestor) { + return { + ok: false, + reason: `cannot resolve any extant ancestor for ${JSON.stringify(savePath)}`, + }; + } + trailing.unshift(parse(ancestor).base); + ancestor = parent; + } + } + + const resolvedAncestor = await realpath(ancestor); + const reattached = + trailing.length === 0 + ? resolvedAncestor + : join(resolvedAncestor, ...trailing); + + // win32-only case fold. On POSIX this is identity, so paths flow through + // unchanged on case-sensitive filesystems (Linux, most macOS APFS setups). + const fold = + process.platform === "win32" + ? (s: string) => s.toLowerCase() + : (s: string) => s; + const foldedTarget = fold(reattached); + + for (const root of roots) { + const rel = relative(fold(root), foldedTarget); + if (rel === "") return { ok: true, resolved: reattached }; + if (!rel.startsWith("..") && !isAbsolute(rel)) { + return { ok: true, resolved: reattached }; + } + } + return { + ok: false, + reason: `${JSON.stringify(reattached)} is outside the allowed write roots: [${roots.map((r) => JSON.stringify(r)).join(", ")}]`, + }; +} diff --git a/tests/server.test.ts b/tests/server.test.ts index 35d0c27..ab76158 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -514,25 +514,31 @@ test("savePath: writeFile rejection surfaces as [save_failed] with errno; file i res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); res.end(HAPPY_FIXTURE); }); - const savePath = "/nonexistent-parent-zzz-savepath-test/out.md"; - const client = await spawnClient(); - try { - const result = await client.callTool({ - name: "fetch_markdown", - arguments: { url: mock.url, savePath }, - }); - assert.equal(result.isError, true); - assert.match(textOf(result), /^\[save_failed\]/); - assert.match(textOf(result), /ENOENT/); - await assert.rejects( - access(savePath), - /ENOENT/, - "file must not have been created on save failure", - ); - } finally { - await client.close(); - await mock.close(); - } + // Path lives inside the default sandbox (os.tmpdir() via mkdtemp), so the + // sandbox check passes and writeFile actually runs. The "nonexistent-subdir" + // intermediate doesn't exist, so writeFile fails with ENOENT — exercising + // the save_failed branch of core.ts. + await withTmpDir(async (dir) => { + const savePath = join(dir, "nonexistent-subdir", "out.md"); + const client = await spawnClient(); + try { + const result = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath }, + }); + assert.equal(result.isError, true); + assert.match(textOf(result), /^\[save_failed\]/); + assert.match(textOf(result), /ENOENT/); + await assert.rejects( + access(savePath), + /ENOENT/, + "file must not have been created on save failure", + ); + } finally { + await client.close(); + await mock.close(); + } + }); }); // T6 — THE Invariant. The file at savePath is only ever the markdown of the URL (per README and SPEC.md). From 3bda7d824d40456d0b89b03f1b8104e61ef512e4 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 00:36:45 +0200 Subject: [PATCH 02/19] Unit-test src/sandbox.ts containment logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Direct tests for buildAllowedRoots and checkPath — no MCP server spin-up required. Coverage: - buildAllowedRoots: defaults (tmpdir + cwd), empty/whitespace treated as unset, single/multi entries replace defaults, fail-fast on non-absolute or unresolvable entries. - checkPath: in-root happy path, outside-root with descriptive message, ../ traversal, prefix-overlap trap (the /tmp vs /tmp-evil hole that naive startsWith hits), symlink escape via realpath (POSIX-gated), win32 case-fold (win32-gated). All fixtures via mkdtemp(os.tmpdir(), "sandbox-test-") — no hardcoded platform paths. Symlink test is intentionally POSIX-only because Windows symlink creation typically requires elevation; the property under test is platform-independent in the source, so POSIX coverage suffices. 12 active tests pass on macOS; 1 win32 test skipped. --- tests/sandbox.test.ts | 198 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 tests/sandbox.test.ts diff --git a/tests/sandbox.test.ts b/tests/sandbox.test.ts new file mode 100644 index 0000000..1aa8934 --- /dev/null +++ b/tests/sandbox.test.ts @@ -0,0 +1,198 @@ +// Unit tests for src/sandbox.ts. +// +// Sandbox is a pure leaf module (no MCP server spin-up needed), so this +// suite exercises buildAllowedRoots and checkPath directly. That gives +// fast, deterministic coverage for the path-edge-case logic (prefix +// overlap, symlink escape, ../ traversal, win32 case-fold) that would +// be painful to validate through the integration boundary alone. +// +// Every fixture is constructed via os.tmpdir() + mkdtemp + path.join — +// no hardcoded platform paths, per the project's review criterion. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, realpath, rm, symlink } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { delimiter, join, parse } from "node:path"; +import { buildAllowedRoots, checkPath } from "../src/sandbox.js"; + +// Scoped mkdtemp + cleanup. We realpath the directory because the sandbox +// module realpath's its inputs internally; testing against an un-resolved +// path on macOS (where /var is a symlink to /private/var) would produce +// containment mismatches that aren't bugs in the module under test. +async function withSandboxTmpDir(fn: (dir: string) => Promise): Promise { + const dir = await realpath(await mkdtemp(join(tmpdir(), "sandbox-test-"))); + try { + return await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +// --------------------------------------------------------------------------- +// buildAllowedRoots +// --------------------------------------------------------------------------- + +test("buildAllowedRoots: unset env returns realpath(tmpdir) + realpath(cwd)", async () => { + const roots = await buildAllowedRoots({}); + assert.equal(roots.length, 2); + assert.equal(roots[0], await realpath(tmpdir())); + assert.equal(roots[1], await realpath(process.cwd())); +}); + +test("buildAllowedRoots: empty-string env behaves like unset (defaults apply)", async () => { + const roots = await buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: "" }); + assert.equal(roots.length, 2); +}); + +test("buildAllowedRoots: whitespace-only env behaves like unset", async () => { + const roots = await buildAllowedRoots({ + MARKFETCH_ALLOWED_WRITE_ROOTS: " ", + }); + assert.equal(roots.length, 2); +}); + +test("buildAllowedRoots: single-entry env REPLACES the defaults (not merge)", async () => { + await withSandboxTmpDir(async (dir) => { + const roots = await buildAllowedRoots({ + MARKFETCH_ALLOWED_WRITE_ROOTS: dir, + }); + assert.deepEqual(roots, [dir]); + }); +}); + +test("buildAllowedRoots: multi-entry env split by path.delimiter", async () => { + await withSandboxTmpDir(async (a) => { + await withSandboxTmpDir(async (b) => { + const roots = await buildAllowedRoots({ + MARKFETCH_ALLOWED_WRITE_ROOTS: `${a}${delimiter}${b}`, + }); + assert.deepEqual(roots, [a, b]); + }); + }); +}); + +test("buildAllowedRoots: non-absolute entry throws fail-fast", async () => { + await assert.rejects( + () => + buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: "relative/path" }), + /every entry must be an absolute path/, + ); +}); + +test("buildAllowedRoots: non-existent entry throws fail-fast (realpath fails)", async () => { + await withSandboxTmpDir(async (dir) => { + const nope = join(dir, "does-not-exist"); + await assert.rejects( + () => buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: nope }), + /could not resolve/, + ); + }); +}); + +// --------------------------------------------------------------------------- +// checkPath +// --------------------------------------------------------------------------- + +test("checkPath: path inside a root is ok", async () => { + await withSandboxTmpDir(async (dir) => { + const result = await checkPath(join(dir, "out.md"), [dir]); + assert.equal(result.ok, true); + }); +}); + +test("checkPath: path outside all roots is not ok; message names the roots", async () => { + await withSandboxTmpDir(async (dir) => { + // Filesystem root is always outside an mkdtemp dir under tmpdir. + // Use parse(dir).root so the test stays portable to Windows drive + // letters (`C:\\`) without hardcoding. + const outside = join(parse(dir).root, "very-unlikely-target.md"); + const result = await checkPath(outside, [dir]); + assert.equal(result.ok, false); + if (!result.ok) { + assert.match(result.reason, /outside the allowed write roots/); + assert.ok( + result.reason.includes(dir), + "reason should name the allowed root for caller-side recovery", + ); + } + }); +}); + +test("checkPath: ../ traversal that escapes a root is not ok", async () => { + await withSandboxTmpDir(async (dir) => { + // dir/sub/../../escape resolves to dir/../escape — a sibling of dir. + // path.resolve handles the `..` normalization; path.relative then + // refuses the resulting outside-root path. + const escape = join(dir, "sub", "..", "..", "escape.md"); + const result = await checkPath(escape, [dir]); + assert.equal(result.ok, false); + }); +}); + +test("checkPath: prefix-overlap trap (root /tmp vs target /tmp-evil)", async () => { + await withSandboxTmpDir(async (root) => { + // Construct a sibling whose name shares a prefix with root. A naive + // `target.startsWith(root)` would pass this incorrectly; path.relative + // returns "..//..." which we reject. + const sibling = `${root}-evil`; + const target = join(sibling, "trap.md"); + const result = await checkPath(target, [root]); + assert.equal(result.ok, false); + }); +}); + +// --------------------------------------------------------------------------- +// Symlink escape — POSIX-gated. +// Windows symlink creation typically requires elevation; the platform- +// independent property under test (realpath defeats symlink escape) is +// implemented identically on both, so POSIX coverage is sufficient. +// --------------------------------------------------------------------------- + +test( + "checkPath: symlink inside sandbox pointing outside is blocked by realpath", + { skip: process.platform === "win32" }, + async () => { + await withSandboxTmpDir(async (sandbox) => { + await withSandboxTmpDir(async (outside) => { + const inner = join(sandbox, "inner"); + await mkdir(inner); + const link = join(inner, "escape"); + await symlink(outside, link); + // The intent: write through inner/escape (a symlink) into outside. + // realpath resolves the symlink, and the containment check then + // sees `outside/should-not-write.md`, which is outside `sandbox`. + const target = join(link, "should-not-write.md"); + const result = await checkPath(target, [sandbox]); + assert.equal(result.ok, false, "symlink escape must be blocked"); + if (!result.ok) { + assert.match(result.reason, /outside the allowed write roots/); + } + }); + }); + }, +); + +// --------------------------------------------------------------------------- +// Windows case-insensitivity — win32-gated. +// --------------------------------------------------------------------------- + +test( + "checkPath: case-variant paths compare equal on win32", + { skip: process.platform !== "win32" }, + async () => { + await withSandboxTmpDir(async (dir) => { + // Construct a case-variant root: leave the drive letter as-is but + // case-flip everything after it. The filesystem treats them as the + // same directory; the sandbox must too. + const variant = dir.charAt(0) + dir.slice(1).toLowerCase(); + const target = join(dir.toUpperCase(), "out.md"); + const result = await checkPath(target, [variant]); + assert.equal( + result.ok, + true, + "win32 containment compare must be case-insensitive", + ); + }); + }, +); From 2b8957deecb2c2026f31982f1f5b9139ac174663 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 00:38:24 +0200 Subject: [PATCH 03/19] Wire sandbox enforcement into MCP fetch_markdown handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Build ALLOWED_ROOTS once at startup via buildAllowedRoots(process.env). Defaults: realpath(os.tmpdir()) ∪ realpath(process.cwd()); replaceable via MARKFETCH_ALLOWED_WRITE_ROOTS (path-delimiter-separated). Bad config throws at module init and surfaces on stderr — same fail-fast contract as intEnv() in core.ts. - Handler runs checkPath before fetchMarkdown when savePath is set; failures return errorResult('save_forbidden', ...) and never call fetchMarkdown, so a forbidden write also short-circuits the upstream network round-trip. CLI is intentionally untouched: human at the shell is the security boundary; an LLM driving the MCP tool is not. All 62 existing tests pass on macOS (1 win32-gated sandbox test skipped). --- src/mcp.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/mcp.ts b/src/mcp.ts index 675ec1f..ff55509 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -14,6 +14,14 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { z } from "zod"; import { fetchMarkdown, classifyError, type ErrorCode } from "./core.js"; import { isAbsolute } from "node:path"; +import { buildAllowedRoots, checkPath } from "./sandbox.js"; + +// Write-sandbox allowed roots, built once at startup. Failures here (e.g., +// MARKFETCH_ALLOWED_WRITE_ROOTS pointing at a non-existent directory) escape +// module init and surface on stderr — same fail-fast convention as intEnv() +// in core.ts. No try/catch: a bad write-roots config is a startup error, +// not a per-request error. +const ALLOWED_ROOTS = await buildAllowedRoots(process.env); function errorResult(code: ErrorCode, message: string) { return { @@ -46,6 +54,16 @@ server.registerTool( }, }, async ({ url, savePath }) => { + // Sandbox gate: MCP-only by design. The CLI adapter does not run this + // check — the human at the shell is the security boundary there. + // Run before fetchMarkdown so a forbidden path short-circuits the + // network round-trip entirely. + if (savePath !== undefined) { + const check = await checkPath(savePath, ALLOWED_ROOTS); + if (!check.ok) { + return errorResult("save_forbidden", check.reason); + } + } try { const { markdown, bytes, savedTo } = await fetchMarkdown({ url, From 239f0a9b08114da9e0b6c56a40704425c1d0e17d Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 00:41:41 +0200 Subject: [PATCH 04/19] Add MCP integration tests for the write sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - T9: path outside default roots → [save_forbidden] with no file. Forbidden path computed via parse(tmpdir).root — no hardcoded "/etc" string. - T10 (POSIX-gated): symlink planted inside the sandbox pointing outside is blocked. Verifies the platform-independent property without needing Windows symlink elevation. - T11: MARKFETCH_ALLOWED_WRITE_ROOTS replaces (not merges) defaults. The second assertion writes to a sibling tmpdir path that would have been allowed under defaults — proves replace semantics. - T12: non-absolute MARKFETCH_ALLOWED_WRITE_ROOTS fails fast on stderr — mirrors existing intEnv-style env-var validation tests. - T13 (win32-gated): Windows-shaped absolute path flows through schema → sandbox → writeFile. Regression guard against reverting path.isAbsolute back to startsWith('/'). 66 active tests pass on macOS; 2 win32-gated cases skipped. --- tests/server.test.ts | 199 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 1 deletion(-) diff --git a/tests/server.test.ts b/tests/server.test.ts index ab76158..284107e 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -9,9 +9,11 @@ import { access, writeFile, rm, + mkdir, + symlink, } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, parse } from "node:path"; import { startMock, textOf, @@ -634,3 +636,198 @@ test("savePath: existing file is overwritten", async () => { } }); }); + +// --------------------------------------------------------------------------- +// Sandbox (write-path guardrails on the MCP side) +// +// MCP `savePath` writes are confined to allowed roots — by default +// realpath(os.tmpdir()) ∪ realpath(process.cwd()), overridable via the +// MARKFETCH_ALLOWED_WRITE_ROOTS env var. The CLI is intentionally NOT +// guard-railed (covered in tests/cli.test.ts); the asymmetry reflects +// the threat model (LLM caller vs. human caller). +// --------------------------------------------------------------------------- + +// T9 — savePath outside the default sandbox → [save_forbidden]; no file. +test("savePath sandbox: path outside default roots → [save_forbidden] and file not written", async () => { + const mock = await startMock((_req, res) => { + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(HAPPY_FIXTURE); + }); + // Construct a forbidden path without hardcoding "/etc" or "C:\\Windows". + // Filesystem root + sentinel filename is outside any mkdtemp dir under + // tmpdir, and outside the test runner's cwd (the repo root). On POSIX + // this is "/markfetch-sandbox-forbidden.md"; on Windows it is + // "C:\\markfetch-sandbox-forbidden.md". + const forbiddenPath = join( + parse(tmpdir()).root, + "markfetch-sandbox-forbidden.md", + ); + const client = await spawnClient(); + try { + const result = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath: forbiddenPath }, + }); + assert.equal(result.isError, true); + const text = textOf(result); + assert.match(text, /^\[save_forbidden\]/); + assert.ok( + text.includes("outside the allowed write roots"), + `error must explain the rule for caller recovery; got: ${text}`, + ); + await assert.rejects( + access(forbiddenPath), + /ENOENT/, + "save_forbidden must not create the file", + ); + } finally { + await client.close(); + await mock.close(); + } +}); + +// T10 — symlink inside sandbox pointing outside is blocked by realpath. +// POSIX-gated: Windows symlink creation typically requires elevation, and +// the property under test is platform-independent in src/sandbox.ts, so +// POSIX coverage is sufficient. +test( + "savePath sandbox: symlink escape from inside sandbox is blocked", + { skip: process.platform === "win32" }, + async () => { + const mock = await startMock((_req, res) => { + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(HAPPY_FIXTURE); + }); + await withTmpDir(async (sandboxDir) => { + // Plant a symlink inside the sandbox dir pointing to filesystem root + // (definitely outside tmpdir). Then write through the symlink and + // expect rejection — realpath in checkPath resolves the symlink and + // the containment check sees the outside-the-sandbox target. + const innerDir = join(sandboxDir, "inner"); + await mkdir(innerDir); + const escapeLink = join(innerDir, "escape"); + const outsideRoot = parse(tmpdir()).root; + await symlink(outsideRoot, escapeLink); + const target = join(escapeLink, "markfetch-symlink-escape.md"); + const realTarget = join(outsideRoot, "markfetch-symlink-escape.md"); + const client = await spawnClient(); + try { + const result = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath: target }, + }); + assert.equal(result.isError, true); + assert.match(textOf(result), /^\[save_forbidden\]/); + await assert.rejects( + access(realTarget), + /ENOENT/, + "symlink-escape attempt must not write to the symlink target", + ); + } finally { + await client.close(); + await mock.close(); + } + }); + }, +); + +// T11 — MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (not merges). +test("savePath sandbox: MARKFETCH_ALLOWED_WRITE_ROOTS replaces the defaults", async () => { + const mock = await startMock((_req, res) => { + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(HAPPY_FIXTURE); + }); + await withTmpDir(async (customRoot) => { + // Spawn the MCP server with the override pointing at customRoot only. + // If the override merged with defaults instead of replacing them, a + // sibling tmpdir path would still be allowed — the second assertion + // proves it doesn't. + const client = await spawnClient({ + env: { MARKFETCH_ALLOWED_WRITE_ROOTS: customRoot }, + }); + try { + // (a) Inside the override root → success. + const insideCustom = join(customRoot, "inside.md"); + const ok = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath: insideCustom }, + }); + assert.equal( + ok.isError, + false, + `write inside override root must succeed; got: ${textOf(ok)}`, + ); + + // (b) Inside os.tmpdir() but outside the override root → forbidden. + // A sibling mkdtemp dir is inside tmpdir but outside customRoot. + await withTmpDir(async (siblingDir) => { + const outsideCustom = join(siblingDir, "outside-override.md"); + const denied = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath: outsideCustom }, + }); + assert.equal( + denied.isError, + true, + "path inside tmpdir-but-outside-override must be rejected (proves replace, not merge)", + ); + assert.match(textOf(denied), /^\[save_forbidden\]/); + }); + } finally { + await client.close(); + await mock.close(); + } + }); +}); + +// T12 — bad MARKFETCH_ALLOWED_WRITE_ROOTS fails fast at startup. +// Mirrors the existing intEnv fail-fast tests for MARKFETCH_TIMEOUT_MS / +// MARKFETCH_MAX_BYTES / MARKFETCH_USER_AGENT. +test("env-var validation: non-absolute MARKFETCH_ALLOWED_WRITE_ROOTS fails fast at startup", async () => { + const { exitCode, stderr } = await spawnAndCaptureExit(["src/index.ts"], { + MARKFETCH_ALLOWED_WRITE_ROOTS: "relative/path", + }); + assert.notEqual( + exitCode, + 0, + "subprocess must exit non-zero when sandbox env var is malformed", + ); + assert.match(stderr, /MARKFETCH_ALLOWED_WRITE_ROOTS/); + assert.match(stderr, /absolute path/); +}); + +// T13 — Windows-shaped absolute path accepted at the schema and writes. +// Win32-gated. Regression guard against reverting the schema from +// path.isAbsolute back to startsWith("/"). The Windows shape must flow +// schema → sandbox check (tmpdir is in defaults) → writeFile. +test( + "savePath: Windows-shaped absolute path is accepted at the schema and writes (win32-gated)", + { skip: process.platform !== "win32" }, + async () => { + const mock = await startMock((_req, res) => { + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(HAPPY_FIXTURE); + }); + await withTmpDir(async (dir) => { + // On Windows, join() with a Windows-shaped tmpdir produces a + // backslash path like C:\Users\...\Temp\xxxxx\win-shape.md. + const savePath = join(dir, "win-shape.md"); + const client = await spawnClient(); + try { + const result = await client.callTool({ + name: "fetch_markdown", + arguments: { url: mock.url, savePath }, + }); + assert.equal( + result.isError, + false, + `Windows-shaped path must be accepted; got: ${textOf(result)}`, + ); + await access(savePath); + } finally { + await client.close(); + await mock.close(); + } + }); + }, +); From 67ac2c09da1c3f5958c7626e745ae442f21391f7 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 00:42:32 +0200 Subject: [PATCH 05/19] CI: extend test job to a 3-OS matrix (ubuntu/macos/windows) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - strategy.matrix.os = [ubuntu-latest, macos-latest, windows-latest] - fail-fast: false so one OS's failure doesn't cancel siblings - shell: bash on `npm test` — cmd.exe (Windows default) doesn't expand the tests/*.test.ts glob in package.json; Git Bash is preinstalled on windows-latest. No-op on Linux/macOS. Still only first-party actions (actions/checkout@v6, actions/setup-node@v6) — preserves the project's "no third-party GitHub Actions" rule. After this commit, the win32-gated tests in sandbox.test.ts (case-fold) and server.test.ts (T13 Windows-shape acceptance) will actually run on the Windows runner — they skip cleanly elsewhere. --- .github/workflows/ci.yml | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bb0a504..474981a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,7 +9,14 @@ on: jobs: test: - runs-on: ubuntu-latest + # fail-fast: false lets a failure on one OS surface all of the + # others' results in the same run, instead of cancelling siblings. + # Cheaper to read than re-running individually for each. + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + runs-on: ${{ matrix.os }} permissions: contents: read steps: @@ -24,4 +31,9 @@ jobs: run: npm ci - name: Run tests + # `npm test` runs `tsx --test tests/*.test.ts`. The glob is + # shell-expanded; cmd.exe (Windows default) doesn't expand it, + # so we pin Git Bash — preinstalled on the windows-latest runner. + # No-op on Linux/macOS where bash is already the default. + shell: bash run: npm test From c502a3f9c13e66b1be58bcebcc2a2838a4244136 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 01:00:48 +0200 Subject: [PATCH 06/19] Force LF line endings in working tree on all platforms Windows runners default to `core.autocrlf=true`, which converts the `.md` and `.html` fixture files to CRLF on checkout. Turndown always emits LF, so the snapshot tests in tests/snapshots.test.ts then compare LF (turndown output) against CRLF (checked-out fixture) and fail with a pure line-ending diff. The first windows-latest CI run on PR #4 caught this exact failure. `* text=auto eol=lf` overrides autocrlf for the working tree on every platform, so the fixtures stay LF whichever runner checks them out. Follows the convention documented at https://docs.github.com/en/get-started/git-basics/configuring-git-to-handle-line-endings. No code change. Existing repo content is already LF (developed on macOS), so no renormalization pass is needed. --- .gitattributes | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..e0142eb --- /dev/null +++ b/.gitattributes @@ -0,0 +1,11 @@ +# Force LF line endings for all text files in the working tree, on all +# platforms. Without this, Windows runners with the default +# core.autocrlf=true convert checked-out files (.md fixtures, .ts source, +# etc.) to CRLF — and snapshot tests that compare turndown output (always +# LF) against fixture content fail with a pure line-ending diff. +# +# `text=auto` lets git auto-detect text vs. binary; `eol=lf` forces LF in +# the working tree even on Windows. The convention follows the +# .gitattributes pattern documented at +# https://docs.github.com/en/get-started/git-basics/configuring-git-to-handle-line-endings. +* text=auto eol=lf From 93e6c7b2a7ec8e320c3710b1fbe8d42a03f0d914 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 01:00:49 +0200 Subject: [PATCH 07/19] Document 0.6.0: README, SPEC, CHANGELOG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README: bump the error-code count in the comparison table (7 -> 8); add the missing error-code reference table (the existing "see the table below" prose pointed at nothing — fixed); add a new "Write sandbox" subsection under Configuration explaining default roots, the MARKFETCH_ALLOWED_WRITE_ROOTS override, MCP-only scope, and the realpath-based symlink defense; mention Linux/macOS/Windows CI coverage in the Develop section. - SPEC: update the "Asymmetric savePath" Core Decision to reflect the path.isAbsolute refinement; add a new "Asymmetric write sandbox" Core Decision explaining the LLM-vs-human threat model and the TOCTOU known limitation; remove the Windows-friendly savePath schema bullet from "Ideas for future" — it shipped. - CHANGELOG: move the existing Unreleased content under a new [0.6.0] - 2026-05-14 release section; document the cross-platform schema, write sandbox, MARKFETCH_ALLOWED_WRITE_ROOTS env var, save_forbidden code, and 3-OS CI matrix. Breaking-change callout for MCP callers that previously wrote outside os.tmpdir() or process.cwd(). --- CHANGELOG.md | 12 +++++++++++- README.md | 43 +++++++++++++++++++++++++++++++++++++++++-- docs/SPEC.md | 5 +++-- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 745d0d2..a599923 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.6.0] - 2026-05-14 + +### Added +- Cross-platform absolute-path validation for `savePath` (MCP). Windows-style paths (`C:\foo`, `C:/foo`, `\\server\share`, `\foo`) are now accepted alongside POSIX absolute paths. The schema delegates to `path.isAbsolute()` so it stays correct on whichever platform the process runs. +- Write sandbox restricting MCP `savePath` writes. By default the allowed set is `realpath(os.tmpdir())` ∪ `realpath(process.cwd())`. Symlinks are resolved via `fs.realpath` before the containment check, so a planted symlink inside the sandbox cannot be used to escape. Violations return the new `save_forbidden` error code; no file is created. The CLI is intentionally unrestricted (human at the shell is the security boundary; the LLM via MCP is the threat surface). +- `MARKFETCH_ALLOWED_WRITE_ROOTS` env var: platform-delimiter-separated list of absolute paths (`:` on POSIX, `;` on Windows). When set, **replaces** the default allowed roots entirely. Validated at startup with fail-fast-on-stderr semantics matching existing env-var conventions (`MARKFETCH_TIMEOUT_MS`, `MARKFETCH_MAX_BYTES`, `MARKFETCH_USER_AGENT`). +- `save_forbidden` error code (8th in the contract): returned when a `savePath` resolves outside the configured allowed write roots. +- CI test job now runs on `ubuntu-latest`, `macos-latest`, and `windows-latest`. `shell: bash` is set on the `npm test` step so the test-glob expands consistently across runners. + ### Changed -- Resolved code smell SonarQube findings (S4325 redundant `Document` casts, S6594 `String#match` → `RegExp#exec`) — no behavior change, all 50 tests pass. ([c993938](https://github.com/vasylenko/markfetch/commit/c9939385edfbe95f7f34a24ba8e33e5a74ac07f4)) +- MCP `savePath` schema replaced the literal `startsWith('/')` constraint with `z.string().refine(path.isAbsolute)`. **Breaking change for MCP callers that previously wrote outside `os.tmpdir()` or `process.cwd()`** — they will now receive `save_forbidden` and must either move the target inside the default roots or set `MARKFETCH_ALLOWED_WRITE_ROOTS`. CLI behavior is unchanged (no sandbox there). +- Resolved code smell SonarQube findings (S4325 redundant `Document` casts, S6594 `String#match` → `RegExp#exec`) — no behavior change, all tests pass. ([c993938](https://github.com/vasylenko/markfetch/commit/c9939385edfbe95f7f34a24ba8e33e5a74ac07f4)) - Documentation and inline comments cleaned up across README, SPEC, source, and test descriptions. Text-only, no runtime change. ([#2](https://github.com/vasylenko/markfetch/pull/2)) ## [0.5.0] - 2026-05-12 diff --git a/README.md b/README.md index 6e5c7fe..3e722b8 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ gemini mcp add -s user markfetch npx -y markfetch | Generic Playwright / Puppeteer | ✓ | – | – | – | | `mcp-server-fetch` (Python) | – | basic | – | – | | CloudFlare `/markdown` | ✓ | ✓ | – | paid | -| **`markfetch`** | **✓** | **✓** | **✓ (7 codes)** | **✓** | +| **`markfetch`** | **✓** | **✓** | **✓ (8 codes)** | **✓** | - **Real-browser HTTP/2 + Chrome fingerprint.** ALPN-negotiated h2, `User-Agent`, `Sec-CH-UA-*`, `Sec-Fetch-*`, `Accept-*`. A Chrome UA with no client hints is a *stronger* automation signal than curl — `markfetch` sends the full coherent set, derived from the UA at startup so an override stays internally consistent. @@ -108,6 +108,19 @@ Flags: Errors go to stderr with the same `[code] message` shape the MCP tool returns (see the table below), and the process exits with a non-zero status. The same env vars (`MARKFETCH_TIMEOUT_MS`, `MARKFETCH_MAX_BYTES`, `MARKFETCH_USER_AGENT`) apply in both modes. +Errors carry one of eight deterministic codes: + +| Code | Meaning | +|---|---| +| `network_error` | DNS / TCP / TLS failure, or an unexpected internal error from the fetcher. | +| `http_error` | Upstream returned a non-2xx status. | +| `timeout` | Per-request budget `MARKFETCH_TIMEOUT_MS` exceeded. | +| `unsupported_content_type` | Response was not `text/html` or `application/xhtml+xml`. | +| `extraction_failed` | Readability returned no article content (typical for pure client-rendered SPAs). | +| `too_large` | Response body or extracted markdown exceeded `MARKFETCH_MAX_BYTES`. | +| `save_failed` | `savePath` was given but `writeFile` failed (parent directory missing, permission denied, etc.). | +| `save_forbidden` | `savePath` resolves outside the allowed write roots — see [Write sandbox](#write-sandbox). MCP-only; the CLI has no sandbox. | + ## What it is not - **Not a crawler.** No recursion, no `robots.txt` parsing, no rate-limit orchestration. One URL in, one document out. @@ -138,9 +151,35 @@ Pass overrides via the `env` block of your MCP client config: } ``` +### Write sandbox + +MCP `savePath` writes are confined to a set of allowed root directories. By default the allowed set is `os.tmpdir()` ∪ `process.cwd()` (each resolved via `fs.realpath` once at startup). A `savePath` outside that set returns `save_forbidden` and no file is created. + +Override the default set with `MARKFETCH_ALLOWED_WRITE_ROOTS` — a list of absolute paths separated by the platform's path delimiter (`:` on POSIX, `;` on Windows). When set, the override **replaces** the defaults entirely; if you set it, you own the policy. A malformed value (non-absolute entry, or a directory that doesn't exist) fails fast on stderr at startup. + +```json +{ + "mcpServers": { + "markfetch": { + "command": "npx", + "args": ["-y", "markfetch"], + "env": { + "MARKFETCH_ALLOWED_WRITE_ROOTS": "/Users/me/markfetch-out:/tmp" + } + } + } +} +``` + +Notes: + +- **The sandbox is MCP-only by design.** The CLI is unrestricted — a human at the shell is the security boundary, and the markfetch CLI doesn't run any sandbox check at all. The asymmetry exists because the MCP tool is driven by a language model, which may be steered by content from a page it just fetched. +- **Symlinks pointing outside are blocked.** Each candidate `savePath` is resolved via `fs.realpath` to its real destination before the containment check, so a symlink planted inside the sandbox cannot be used to escape. +- **Containment is case-insensitive on Windows** (`C:\Users\Bob` and `c:\users\bob` are the same path). + ## Develop -Requires Node.js ≥ 24. +Requires Node.js ≥ 24. Tested on Linux, macOS, and Windows in CI. When iterating on CLI changes, `tsx src/index.ts ` and `tsx src/index.ts --help` route through the same argv-discriminated dispatcher as the built `dist/index.js` — no rebuild needed between edits. diff --git a/docs/SPEC.md b/docs/SPEC.md index 3429baf..c056e51 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -29,7 +29,9 @@ Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: ` - **Whole document or `too_large`.** No pagination. Partial content lets the agent reason over truncated bodies without knowing they're truncated. `savePath` / `-o` is the escape valve for genuinely large documents. -- **Asymmetric `savePath`.** MCP requires absolute paths (zod `startsWith("/")`); CLI accepts relative and resolves against `process.cwd()`. CLI has a stable cwd the user typed `cd` into; MCP servers run in whatever cwd the client picks. +- **Asymmetric `savePath`.** MCP requires absolute paths via zod `refine(path.isAbsolute)` — accepts platform-appropriate shapes on POSIX (`/foo`) and Windows (`C:\foo`, `C:/foo`, `\\server\share`, `\foo`). CLI accepts relative and resolves against `process.cwd()`. CLI has a stable cwd the user typed `cd` into; MCP servers run in whatever cwd the client picks. + +- **Asymmetric write sandbox.** MCP `savePath` writes are confined to `realpath(os.tmpdir())` ∪ `realpath(process.cwd())` by default; the env var `MARKFETCH_ALLOWED_WRITE_ROOTS` (path-delimiter-separated) replaces the defaults. CLI writes anywhere the human's shell permits — no sandbox check. The asymmetry reflects the threat model: an LLM driving the MCP tool may be steered by content from the page it just fetched; a human typing into a shell is the security boundary. Symlinks are resolved via `fs.realpath` before the containment check, so a planted symlink inside the sandbox cannot escape. Containment compare is case-insensitive on `process.platform === "win32"`. Implementation lives in `src/sandbox.ts` (a leaf module — no imports from siblings — so it's unit-testable without spinning up the MCP server). Known limitation: TOCTOU between `realpath` and `writeFile` is not closed — acceptable for a single-user developer tool. - **Stderr is fatal-only.** Per-request MCP errors round-trip through `{ isError }`; only startup misconfig / unrecoverable crashes touch stderr. CLI is its own session, so its per-request errors *are* fatal for that session. Regression guard: `tests/server.test.ts:436`. @@ -41,4 +43,3 @@ Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: ` - **Cookie reuse across redirects within a single fetch.** Currently none. Trigger: a target serves content only after a session-cookie redirect. - **Proxy support** (`MARKFETCH_PROXY_URL`) and **`Accept-Language` control** (`MARKFETCH_ACCEPT_LANGUAGE`). Trigger: corporate proxy / locale-specific content. - **Single-binary distribution.** Bun's `build --compile`, Node SEA, or similar. Trigger: `npx` first-run latency feedback, or an offline / airgapped need. -- **Windows-friendly `savePath` schema.** Currently Unix-shaped (`startsWith("/")`). Trigger: someone needs this on Windows. From 9ed1255e2e09b7801c5f27b064cf584ef5db84ac Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 01:00:49 +0200 Subject: [PATCH 08/19] Bump version to 0.6.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coordinated bump across the three sources of truth plus four test assertions that pin the version literal: - package.json (the canonical source) - src/mcp.ts — McpServer constructor - src/cli.ts — commander .version() - tests/server.test.ts — MCP handshake version assertion - tests/cli.test.ts — CLI --version stdout assertion - tests/e2e.test.ts — handshake assertion AND --version stdout The duplication is a known smell (could be addressed by reading from package.json at runtime); deliberately out of scope for this PR. --- package.json | 2 +- src/cli.ts | 2 +- src/mcp.ts | 2 +- tests/cli.test.ts | 2 +- tests/e2e.test.ts | 4 ++-- tests/server.test.ts | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 445f75b..9522e90 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "markfetch", - "version": "0.5.0", + "version": "0.6.0", "description": "Fetch a URL, return clean markdown. MCP server and CLI for AI agents.", "license": "MIT", "author": { diff --git a/src/cli.ts b/src/cli.ts index ac1f785..b8d0579 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -23,7 +23,7 @@ program "Fetch a URL and return clean markdown.\n" + "Run with no arguments to start the MCP stdio server.", ) - .version("0.5.0") + .version("0.6.0") .argument("", "absolute http(s) URL to fetch") .option( "-o, --output ", diff --git a/src/mcp.ts b/src/mcp.ts index ff55509..111b819 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -30,7 +30,7 @@ function errorResult(code: ErrorCode, message: string) { }; } -const server = new McpServer({ name: "markfetch", version: "0.5.0" }); +const server = new McpServer({ name: "markfetch", version: "0.6.0" }); server.registerTool( "fetch_markdown", diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 603bdae..b18f15a 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -196,7 +196,7 @@ test("CLI: --version prints version to stdout, exit 0", async () => { const { code, stdout, stderr } = await runCli(["--version"]); assert.equal(code, 0); assert.equal(stderr, ""); - assert.equal(stdout, "0.5.0\n"); + assert.equal(stdout, "0.6.0\n"); }); // This test documents a deliberate asymmetry vs MCP: a malformed URL is NOT diff --git a/tests/e2e.test.ts b/tests/e2e.test.ts index 22553eb..5409086 100644 --- a/tests/e2e.test.ts +++ b/tests/e2e.test.ts @@ -58,7 +58,7 @@ test("e2e: built output boots, exposes fetch_markdown, pins version", async () = try { const info = client.getServerVersion(); assert.equal(info?.name, "markfetch"); - assert.equal(info?.version, "0.5.0"); + assert.equal(info?.version, "0.6.0"); const { tools } = await client.listTools(); assert.equal(tools.length, 1); assert.equal(tools[0].name, "fetch_markdown"); @@ -166,5 +166,5 @@ test("e2e: built output --version prints package version, exit 0", async () => { { timeout: 10_000 }, ); assert.equal(stderr, ""); - assert.equal(stdout, "0.5.0\n"); + assert.equal(stdout, "0.6.0\n"); }); diff --git a/tests/server.test.ts b/tests/server.test.ts index 284107e..025b3c6 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -28,7 +28,7 @@ test("server boots over stdio and completes the initialize handshake", async () try { const info = client.getServerVersion(); assert.equal(info?.name, "markfetch"); - assert.equal(info?.version, "0.5.0"); + assert.equal(info?.version, "0.6.0"); } finally { await client.close(); } From 0a1752ca1c69d44e1618dd36fdbaf30da3143f1a Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 01:10:18 +0200 Subject: [PATCH 09/19] Fix Windows CI failures: tsx-shim spawn + path-escaping in error reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues caused 14 failures on the windows-latest runner of PR #4 run #2: 1. tests/cli.test.ts and tests/_helpers.ts:spawnAndCaptureExit spawn `./node_modules/.bin/tsx` directly. On Windows that's a `.cmd` shim Node's native child_process.spawn cannot launch without `shell: true`. Switch both to `node --import `, which bypasses the shim entirely and works identically on all platforms. The loader URL is an absolute file:// URL resolved once at test startup, so it stays correct even when individual tests override the child cwd to a tmpdir. spawnClient (StdioClientTransport-based) is intentionally left alone — the MCP SDK handles cross-platform spawn internally, and the server tests passed on Windows already. 2. src/sandbox.ts:checkPath returned error messages that ran path arguments through JSON.stringify(), which escapes backslashes in Windows paths ('C:\\Users\\...'). The unit-test assertion `result.reason.includes(dir)` failed because the literal `dir` isn't a substring of the JSON-escaped form. Switch to single-quote framing (''); substring check works on Windows now and the error output is more readable for humans too. All 66 active tests pass locally on macOS; 2 win32-gated tests skip. --- src/sandbox.ts | 4 ++-- tests/_helpers.ts | 29 +++++++++++++++++++++++++---- tests/cli.test.ts | 16 +++++++++------- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/sandbox.ts b/src/sandbox.ts index c6bb1ed..5197daa 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -123,7 +123,7 @@ export async function checkPath( if (parent === ancestor) { return { ok: false, - reason: `cannot resolve any extant ancestor for ${JSON.stringify(savePath)}`, + reason: `cannot resolve any extant ancestor for '${savePath}'`, }; } trailing.unshift(parse(ancestor).base); @@ -154,6 +154,6 @@ export async function checkPath( } return { ok: false, - reason: `${JSON.stringify(reattached)} is outside the allowed write roots: [${roots.map((r) => JSON.stringify(r)).join(", ")}]`, + reason: `'${reattached}' is outside the allowed write roots: [${roots.map((r) => `'${r}'`).join(", ")}]`, }; } diff --git a/tests/_helpers.ts b/tests/_helpers.ts index 626e1a3..cd9a557 100644 --- a/tests/_helpers.ts +++ b/tests/_helpers.ts @@ -10,9 +10,26 @@ import { type IncomingMessage, type ServerResponse, } from "node:http"; +import { resolve } from "node:path"; +import { pathToFileURL } from "node:url"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +// Absolute file:// URL for tsx's loader entry. Test helpers that spawn the +// CLI via `node --import src/index.ts` use this rather than the +// `./node_modules/.bin/tsx` shim (a `.cmd` file on Windows that Node's +// native child_process.spawn cannot launch without `shell: true`). Resolved +// once at test-startup so it stays correct even when individual tests +// override the child cwd. +// +// spawnClient (below) is intentionally NOT switched to this pattern: it +// relies on the MCP SDK's StdioClientTransport, which uses its own +// cross-platform launcher and already handles `command: "tsx"` correctly +// on Windows. Touching it would couple this code to SDK internals. +export const TSX_LOADER_URL = pathToFileURL( + resolve("./node_modules/tsx/dist/loader.mjs"), +).href; + export async function startMock( handler: (req: IncomingMessage, res: ServerResponse) => void, ): Promise<{ url: string; close: () => Promise }> { @@ -103,10 +120,14 @@ export async function spawnAndCaptureExit( args: string[], env: Record, ): Promise<{ exitCode: number; stderr: string }> { - const child = spawn("./node_modules/.bin/tsx", args, { - env: { ...process.env, ...env } as Record, - stdio: ["pipe", "pipe", "pipe"], - }); + const child = spawn( + process.execPath, + ["--import", TSX_LOADER_URL, ...args], + { + env: { ...process.env, ...env } as Record, + stdio: ["pipe", "pipe", "pipe"], + }, + ); let stderr = ""; child.stderr.on("data", (d: Buffer) => { stderr += d.toString(); diff --git a/tests/cli.test.ts b/tests/cli.test.ts index b18f15a..1cd636b 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -9,15 +9,17 @@ import { promisify } from "node:util"; import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve as resolvePath } from "node:path"; -import { startMock, HAPPY_FIXTURE } from "./_helpers.js"; +import { startMock, HAPPY_FIXTURE, TSX_LOADER_URL } from "./_helpers.js"; const execFileAsync = promisify(execFile); // Resolved at module load against the test runner's cwd (the project root). -// Tests that override `cwd` to a tmpdir still need to find the tsx CLI -// and the source entry — passing relative paths would resolve against the -// new cwd and produce a confusing ENOENT instead of the behavior under test. -const TSX_CLI = resolvePath("./node_modules/.bin/tsx"); +// Tests that override `cwd` to a tmpdir still need to find the source entry +// — a relative path would resolve against the new cwd and produce a +// confusing ENOENT instead of the behavior under test. The tsx loader is +// imported via TSX_LOADER_URL (an absolute file:// URL from _helpers.ts) +// for the same portability reason; it also bypasses the platform-specific +// `./node_modules/.bin/tsx[.cmd]` shim entirely. const ENTRY = resolvePath("src/index.ts"); type RunResult = { code: number; stdout: string; stderr: string }; @@ -38,8 +40,8 @@ async function runCli( ): Promise { try { const { stdout, stderr } = await execFileAsync( - TSX_CLI, - [ENTRY, ...args], + process.execPath, + ["--import", TSX_LOADER_URL, ENTRY, ...args], { env: { ...process.env, ...env } as Record, cwd, From 3e2ec9b57f6d17a81287dbf49373bb7fc374c330 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 09:46:50 +0200 Subject: [PATCH 10/19] Address code-review findings: contract docs + fail-fast hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two convergent themes from the multi-agent code review on this PR: 1) save_forbidden propagation (the 8th error code wasn't reflected in every name-listing site): - tests/_helpers.ts: ERROR_CODE_PREFIX_RE alternation + docstring updated from 7 to 8 codes. Closes the assertSchemaRejection invariant against future handler-side save_forbidden emission. - docs/SPEC.md line 16: error-code enumeration updated. Line 24: "core throws all codes" claim corrected to note save_forbidden is emitted by the MCP adapter, not core. - src/core.ts: fetchMarkdown doc comment annotated with the adapter-side save_forbidden note (the function itself doesn't throw it; comment now says so explicitly). - src/mcp.ts: tool description AND savePath schema description updated to mention the sandbox and save_forbidden so MCP clients (LLMs) see the contract in-band. 2) src/sandbox.ts: tighten fail-fast + remove dead code: - Drop outer .trim() on empty-string guard; drop .filter on env split (Suggestions 1+5): empty / whitespace-only / empty-entry env values now fail-fast uniformly via the existing isAbsolute check, instead of silent partial acceptance. - Add stat()+isDirectory() after realpath() (Suggestion 2): a file path passed in MARKFETCH_ALLOWED_WRITE_ROOTS now throws at startup with a clear message, instead of failing late at writeFile with ENOTDIR. - Remove dead `eslint-disable-next-line` directive (Suggestion 4): project has no ESLint config so the directive was inert. Plus: - docs/SPEC.md line 36: stale test line-number 436 -> 336. - README.md: paired Windows example for MARKFETCH_ALLOWED_WRITE_ROOTS alongside the POSIX one. - tests/sandbox.test.ts: whitespace-only test flipped to expect rejection (matches new fail-fast behavior); added empty-entry and non-directory regression tests. Sandbox API contract unchanged externally — all changes are either additional fail-fast paths, documentation clarifications, or test-helper invariants. 68 active tests pass on macOS (was 66; +2 new); 2 win32-gated tests skip on POSIX runners. --- README.md | 16 ++++++++++++++++ docs/SPEC.md | 6 +++--- src/core.ts | 3 +++ src/mcp.ts | 4 ++-- src/sandbox.ts | 20 +++++++++++--------- tests/_helpers.ts | 4 ++-- tests/sandbox.test.ts | 41 +++++++++++++++++++++++++++++++++++------ 7 files changed, 72 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 3e722b8..b087a9b 100644 --- a/README.md +++ b/README.md @@ -171,6 +171,22 @@ Override the default set with `MARKFETCH_ALLOWED_WRITE_ROOTS` — a list of abso } ``` +On Windows, use backslashes and `;` as the delimiter: + +```json +{ + "mcpServers": { + "markfetch": { + "command": "npx", + "args": ["-y", "markfetch"], + "env": { + "MARKFETCH_ALLOWED_WRITE_ROOTS": "C:\\Users\\me\\markfetch-out;C:\\Users\\me\\AppData\\Local\\Temp" + } + } + } +} +``` + Notes: - **The sandbox is MCP-only by design.** The CLI is unrestricted — a human at the shell is the security boundary, and the markfetch CLI doesn't run any sandbox check at all. The asymmetry exists because the MCP tool is driven by a language model, which may be steered by content from a page it just fetched. diff --git a/docs/SPEC.md b/docs/SPEC.md index c056e51..322949c 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -13,7 +13,7 @@ URL → caller markdown body, or "Saved N bytes to /path" confirmation ``` -Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: `network_error`, `http_error`, `timeout`, `unsupported_content_type`, `extraction_failed`, `too_large`, `save_failed`. CLI emits `[code] message` to stderr and exits 1; MCP emits `{ isError: true, content: [{ text: "[code] message" }] }`. +Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: `network_error`, `http_error`, `timeout`, `unsupported_content_type`, `extraction_failed`, `too_large`, `save_failed`; plus `save_forbidden`, emitted by the MCP adapter only (before `fetchMarkdown` runs — see "Asymmetric write sandbox" under Core Decisions). CLI emits `[code] message` to stderr and exits 1; MCP emits `{ isError: true, content: [{ text: "[code] message" }] }`. ## Core Decisions @@ -21,7 +21,7 @@ Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: ` - **Lazy adapter imports.** The dispatcher uses `await import()` to load exactly one adapter. The only `console.log` in the project lives in `cli.ts`; under MCP, `cli.ts` never loads, so stdout-discipline is enforced by the module graph — not by linter or convention. -- **Core throws, adapters translate.** All 7 error codes surface from `core.ts` — five are thrown explicitly as `MarkfetchError`; `network_error`, `timeout`, and (sometimes) `http_error` are translated by `classifyError` from underlying-API errors (undici TypeErrors, AbortSignal timeouts). New codes need an `ErrorCode` union member + a throw site; adapters don't change. +- **Core throws, adapters translate.** Seven of the eight error codes surface from `core.ts` — five are thrown explicitly as `MarkfetchError`; `network_error`, `timeout`, and (sometimes) `http_error` are translated by `classifyError` from underlying-API errors (undici TypeErrors, AbortSignal timeouts). The eighth code, `save_forbidden`, is the exception — it's emitted by the MCP adapter before `fetchMarkdown` is invoked (see "Asymmetric write sandbox" below). New core codes need an `ErrorCode` union member + a throw site; adapters don't change. - **HTTP/2 + coherent Chrome fingerprint.** Wire protocol, headers, and UA must agree — a Chrome UA over HTTP/1.1 or without `Sec-CH-UA-*` is *more* suspicious than curl. `Sec-CH-UA-*` is derived from `MARKFETCH_USER_AGENT` at startup so override-coherence is mechanical. @@ -33,7 +33,7 @@ Errors throw `MarkfetchError` uniformly from core; adapters catch once. Codes: ` - **Asymmetric write sandbox.** MCP `savePath` writes are confined to `realpath(os.tmpdir())` ∪ `realpath(process.cwd())` by default; the env var `MARKFETCH_ALLOWED_WRITE_ROOTS` (path-delimiter-separated) replaces the defaults. CLI writes anywhere the human's shell permits — no sandbox check. The asymmetry reflects the threat model: an LLM driving the MCP tool may be steered by content from the page it just fetched; a human typing into a shell is the security boundary. Symlinks are resolved via `fs.realpath` before the containment check, so a planted symlink inside the sandbox cannot escape. Containment compare is case-insensitive on `process.platform === "win32"`. Implementation lives in `src/sandbox.ts` (a leaf module — no imports from siblings — so it's unit-testable without spinning up the MCP server). Known limitation: TOCTOU between `realpath` and `writeFile` is not closed — acceptable for a single-user developer tool. -- **Stderr is fatal-only.** Per-request MCP errors round-trip through `{ isError }`; only startup misconfig / unrecoverable crashes touch stderr. CLI is its own session, so its per-request errors *are* fatal for that session. Regression guard: `tests/server.test.ts:436`. +- **Stderr is fatal-only.** Per-request MCP errors round-trip through `{ isError }`; only startup misconfig / unrecoverable crashes touch stderr. CLI is its own session, so its per-request errors *are* fatal for that session. Regression guard: `tests/server.test.ts:336`. ## Ideas for future diff --git a/src/core.ts b/src/core.ts index 7e7a924..f2a6d4b 100644 --- a/src/core.ts +++ b/src/core.ts @@ -406,6 +406,9 @@ function convertToMarkdown(article: { // extraction_failed, too_large, save_failed // (The first three may also come from underlying APIs and be translated by // classifyError — adapters MUST run classifyError(err) in their catch blocks.) +// Note: the MCP adapter additionally emits save_forbidden (the 8th code in +// the contract) before fetchMarkdown is invoked — this function never throws +// it. See src/sandbox.ts and src/mcp.ts. export async function fetchMarkdown(input: { url: string; savePath?: string; diff --git a/src/mcp.ts b/src/mcp.ts index 111b819..f6e0c31 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -36,7 +36,7 @@ server.registerTool( "fetch_markdown", { description: - "Fetch a single public HTTP/S URL and return its main article content as clean markdown. Best for articles, documentation, blog posts, news, and reference pages. Non-HTML responses return `unsupported_content_type`. Pure client-rendered SPAs with no extractable static HTML return `extraction_failed`; SPAs that ship server-rendered or SEO-prerendered HTML will extract whatever static content they expose. Also supports saving the markdown to a file, e.g., to bypass client tool-result size limits or to reuse later.", + "Fetch a single public HTTP/S URL and return its main article content as clean markdown. Best for articles, documentation, blog posts, news, and reference pages. Non-HTML responses return `unsupported_content_type`. Pure client-rendered SPAs with no extractable static HTML return `extraction_failed`; SPAs that ship server-rendered or SEO-prerendered HTML will extract whatever static content they expose. Also supports saving the markdown to a file, e.g., to bypass client tool-result size limits or to reuse later. Saved files must land inside the allowed write roots (defaults: system temp dir and the server's working directory; configurable via `MARKFETCH_ALLOWED_WRITE_ROOTS`); paths outside return `save_forbidden`.", inputSchema: { url: z .string() @@ -49,7 +49,7 @@ server.registerTool( .refine(isAbsolute, "savePath must be an absolute filesystem path") .optional() .describe( - "Optional. When provided, the fetched markdown is written to this absolute filesystem path and the response becomes a small confirmation. Use this when the markdown might exceed your client's tool-result inline cap. Must be an absolute path on the host platform (e.g., `/foo/bar.md` on POSIX; `C:\\foo\\bar.md` or `\\\\server\\share\\bar.md` on Windows); relative paths and tilde paths (`~/...`) are rejected by the schema. Existing files are overwritten; the parent directory must exist (caller's responsibility). The file is written only on fetch success — fetch / extraction / size-cap errors return a `[code]` string and never touch the file.", + "Optional. When provided, the fetched markdown is written to this absolute filesystem path and the response becomes a small confirmation. Use this when the markdown might exceed your client's tool-result inline cap. Must be an absolute path on the host platform (e.g., `/foo/bar.md` on POSIX; `C:\\foo\\bar.md` or `\\\\server\\share\\bar.md` on Windows); relative paths and tilde paths (`~/...`) are rejected by the schema. Writes are confined to an allow-listed sandbox — defaults are the system temp dir (`os.tmpdir()`) and the server's working directory; operators can override with `MARKFETCH_ALLOWED_WRITE_ROOTS` (path-delimiter-separated). A `savePath` outside the allowed roots returns `save_forbidden` and no file is created. Existing files are overwritten; the parent directory must exist (caller's responsibility). The file is written only on fetch success — fetch / extraction / size-cap errors return a `[code]` string and never touch the file.", ), }, }, diff --git a/src/sandbox.ts b/src/sandbox.ts index 5197daa..722d343 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -57,13 +57,8 @@ export async function buildAllowedRoots( env: NodeJS.ProcessEnv, ): Promise { const raw = env[ENV_VAR]; - if (raw != null && raw.trim() !== "") { - const entries = raw.split(delimiter).filter((s) => s !== ""); - if (entries.length === 0) { - throw new Error( - `Invalid ${ENV_VAR}=${JSON.stringify(raw)} — expected a ${JSON.stringify(delimiter)}-separated list of absolute paths.`, - ); - } + if (raw != null && raw !== "") { + const entries = raw.split(delimiter); const resolved: string[] = []; for (const entry of entries) { if (!isAbsolute(entry)) { @@ -71,14 +66,22 @@ export async function buildAllowedRoots( `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, ); } + let resolvedEntry: string; try { - resolved.push(await realpath(entry)); + resolvedEntry = await realpath(entry); } catch (err) { const message = err instanceof Error ? err.message : String(err); throw new Error( `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, ); } + const stats = await stat(resolvedEntry); + if (!stats.isDirectory()) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — resolved to ${JSON.stringify(resolvedEntry)} which is not a directory.`, + ); + } + resolved.push(resolvedEntry); } return resolved; } @@ -113,7 +116,6 @@ export async function checkPath( let ancestor = normalized; const trailing: string[] = []; // Deliberate single-pass loop; iteration count is bounded by path depth. - // eslint-disable-next-line no-constant-condition while (true) { try { await stat(ancestor); diff --git a/tests/_helpers.ts b/tests/_helpers.ts index cd9a557..782a523 100644 --- a/tests/_helpers.ts +++ b/tests/_helpers.ts @@ -75,11 +75,11 @@ export function textOf(result: { content: unknown }): string { return content[0]?.text ?? ""; } -// Matches any of the seven [code] error prefixes the tool emits. Used by +// Matches any of the eight [code] error prefixes the tool emits. Used by // schema-rejection assertions to prove the handler did NOT run — a [code] // prefix would mean the call escaped Zod and reached core. export const ERROR_CODE_PREFIX_RE = - /^\[(network_error|http_error|timeout|unsupported_content_type|extraction_failed|too_large|save_failed)\]/; + /^\[(network_error|http_error|timeout|unsupported_content_type|extraction_failed|too_large|save_failed|save_forbidden)\]/; // Asserts that a tool call is rejected at the Zod schema boundary, not by the // handler. The SDK either throws (some versions) or returns isError:true with diff --git a/tests/sandbox.test.ts b/tests/sandbox.test.ts index 1aa8934..480f95e 100644 --- a/tests/sandbox.test.ts +++ b/tests/sandbox.test.ts @@ -11,7 +11,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdir, mkdtemp, realpath, rm, symlink } from "node:fs/promises"; +import { mkdir, mkdtemp, realpath, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { delimiter, join, parse } from "node:path"; import { buildAllowedRoots, checkPath } from "../src/sandbox.js"; @@ -45,11 +45,14 @@ test("buildAllowedRoots: empty-string env behaves like unset (defaults apply)", assert.equal(roots.length, 2); }); -test("buildAllowedRoots: whitespace-only env behaves like unset", async () => { - const roots = await buildAllowedRoots({ - MARKFETCH_ALLOWED_WRITE_ROOTS: " ", - }); - assert.equal(roots.length, 2); +test("buildAllowedRoots: whitespace-only env throws fail-fast (malformed entry, not treated as unset)", async () => { + // Suggestion 5 from the code-review (drop outer trim) means whitespace + // is no longer silently treated as unset — it falls through to the + // isAbsolute check and fails fast like any other non-absolute value. + await assert.rejects( + () => buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: " " }), + /every entry must be an absolute path/, + ); }); test("buildAllowedRoots: single-entry env REPLACES the defaults (not merge)", async () => { @@ -90,6 +93,32 @@ test("buildAllowedRoots: non-existent entry throws fail-fast (realpath fails)", }); }); +test("buildAllowedRoots: empty entry from leading/trailing/consecutive delimiter throws fail-fast", async () => { + // Suggestion 1 from the code-review (drop the .filter on env split) means + // typo'd env vars like ":/foo" or "/foo::/bar" no longer get silently + // partial-accepted — empty entries fall through to isAbsolute and throw. + await assert.rejects( + () => + buildAllowedRoots({ + MARKFETCH_ALLOWED_WRITE_ROOTS: `${delimiter}/some/path`, + }), + /every entry must be an absolute path/, + ); +}); + +test("buildAllowedRoots: regular-file entry throws fail-fast (must be a directory)", async () => { + // Suggestion 2 from the code-review: a file path pointed at by the env + // var is rejected at startup, not later via writeFile's ENOTDIR. + await withSandboxTmpDir(async (dir) => { + const filePath = join(dir, "regular-file.txt"); + await writeFile(filePath, ""); + await assert.rejects( + () => buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: filePath }), + /not a directory/, + ); + }); +}); + // --------------------------------------------------------------------------- // checkPath // --------------------------------------------------------------------------- From 2726aef1c6d8b394adf7cd649f9c08301727c103 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 09:52:07 +0200 Subject: [PATCH 11/19] Add opt-in live e2e tests against real production URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests/e2e-live.test.ts exercises the full markfetch pipeline (HTTP/2 + Chrome fingerprint + linkedom + Readability + turndown) against two real public URLs: - https://en.wikipedia.org/wiki/Markup_language (Wikipedia, the canonical Reader-View target — long article, dense chrome). - https://code.claude.com/docs/en/commands (a modern docs SPA — non-Wikipedia surface, different chrome shape). Gated by `MARKFETCH_LIVE_E2E=1` env var so default `npm test` stays deterministic and offline-safe. When the env var is unset the tests appear as SKIPPED in test output; when set they actually fetch the URLs. Verified locally on macOS: both pass in ~1.4s combined. Assertions are property-based — title presence, section-count floor, chrome stripping (no ` { + const { stdout, stderr } = await execFileAsync("node", [BUILT_JS, url], { + timeout: 30_000, + maxBuffer: 10_000_000, + }); + return { stdout, stderr }; +} + +// --------------------------------------------------------------------------- +// Wikipedia — the canonical Reader-View target. Long article with a +// dense table-of-contents, multiple section headings, citations, and +// chrome (nav, sidebar, footer) that Readability must strip. +// --------------------------------------------------------------------------- + +test( + "live: en.wikipedia.org/wiki/Markup_language extracts title, sections, and strips chrome", + { skip: !LIVE_E2E_ENABLED }, + async () => { + const { stdout, stderr } = await fetchLive( + "https://en.wikipedia.org/wiki/Markup_language", + ); + + assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); + + // The article's main title appears in the extracted markdown. + // Match case-insensitively so future title-style tweaks (e.g., "Markup + // language" vs "Markup Language") don't break the test. + assert.match( + stdout, + /markup\s+language/i, + "extracted markdown must mention the article title", + ); + + // Wikipedia articles always have a multi-section structure. + // The exact section names change over time, but ≥3 H2 sections is a + // safe floor for any article that isn't a stub. + const h2Count = (stdout.match(/^## /gm) ?? []).length; + assert.ok( + h2Count >= 3, + `expected ≥3 H2 sections in Wikipedia article markdown; got ${h2Count}`, + ); + + // Readability+turndown must strip Wikipedia's site chrome. If any of + // these substrings appear in the body, extraction has regressed. + assert.ok( + !stdout.includes(" 5000, + `extracted markdown should be substantial for a Wikipedia article; got ${stdout.length} chars`, + ); + }, +); + +// --------------------------------------------------------------------------- +// Claude Code commands docs — a modern docs-site SPA with +// server-rendered HTML that Readability is expected to extract from. +// Smaller body than Wikipedia; different framework, different chrome +// shape, exercises the pipeline against a non-Wikipedia surface. +// --------------------------------------------------------------------------- + +test( + "live: code.claude.com/docs/en/commands extracts content as clean markdown", + { skip: !LIVE_E2E_ENABLED }, + async () => { + const { stdout, stderr } = await fetchLive( + "https://code.claude.com/docs/en/commands", + ); + + assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); + + // The page is the slash-commands docs; the word "command" must appear + // somewhere in the extracted body. + assert.match( + stdout, + /command/i, + "docs page about commands must mention 'command' somewhere", + ); + + // Non-trivial extraction floor. The page has at least a heading plus + // a paragraph of intro, so 500 chars is a safe minimum. + assert.ok( + stdout.length > 500, + `extracted markdown should be non-trivial; got ${stdout.length} chars`, + ); + + // Raw HTML chrome should not survive. + assert.ok( + !stdout.includes(" Date: Thu, 14 May 2026 10:06:18 +0200 Subject: [PATCH 12/19] Wire live e2e tests into the default suite Move the two live URL tests from the gated tests/e2e-live.test.ts file into tests/e2e.test.ts so they run as part of every npm test and every CI matrix job. Drop the MARKFETCH_LIVE_E2E env-var gate. 70 active tests pass on macOS; 2 win32-gated tests skip on POSIX. --- tests/e2e-live.test.ts | 167 ----------------------------------------- 1 file changed, 167 deletions(-) delete mode 100644 tests/e2e-live.test.ts diff --git a/tests/e2e-live.test.ts b/tests/e2e-live.test.ts deleted file mode 100644 index 89d733c..0000000 --- a/tests/e2e-live.test.ts +++ /dev/null @@ -1,167 +0,0 @@ -// Live end-to-end tests against real production web pages. -// -// These tests exercise the full markfetch pipeline (HTTP/2 + Chrome -// fingerprint + linkedom + Readability + turndown) against actual -// public URLs — the only place we get signal that the bot-detection -// fingerprint and Reader-View extraction still work in production. -// -// They are EXCLUDED from the default `npm test` run via a per-test -// skip gate, so the default suite stays deterministic and offline-safe. -// To enable: -// -// POSIX: MARKFETCH_LIVE_E2E=1 npm test -// PowerShell: $env:MARKFETCH_LIVE_E2E=1; npm test -// cmd.exe: set MARKFETCH_LIVE_E2E=1 && npm test -// -// You can also run only this file: -// -// MARKFETCH_LIVE_E2E=1 npx tsx --test tests/e2e-live.test.ts -// -// Assertions are property-based (title presence, structural counts, -// chrome stripped) — they intentionally do NOT match exact upstream -// strings, because the target pages will rewrite their copy over time. -// If a test starts failing, first check whether the target page still -// exists and still has roughly the structure described in the comment -// above each test, then investigate the pipeline. -// -// CI: these tests are not part of the default CI matrix. Wiring them -// into a nightly schedule (or a manual-dispatch live-check workflow) -// is a deliberate follow-up — flakiness from network / rate limits / -// upstream content drift should not gate every PR. - -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { execFile } from "node:child_process"; -import { promisify } from "node:util"; -import { resolve as resolvePath } from "node:path"; - -const execFileAsync = promisify(execFile); - -const LIVE_E2E_ENABLED = process.env.MARKFETCH_LIVE_E2E === "1"; -const BUILT_JS = resolvePath("dist/index.js"); - -// Generous timeout: real network round-trips + large article body -// extraction can take a few seconds even on a healthy connection. -// 10 MB buffer cap accommodates Wikipedia's longer articles without -// the default 1 MB execFile cap truncating output. -async function fetchLive(url: string): Promise<{ stdout: string; stderr: string }> { - const { stdout, stderr } = await execFileAsync("node", [BUILT_JS, url], { - timeout: 30_000, - maxBuffer: 10_000_000, - }); - return { stdout, stderr }; -} - -// --------------------------------------------------------------------------- -// Wikipedia — the canonical Reader-View target. Long article with a -// dense table-of-contents, multiple section headings, citations, and -// chrome (nav, sidebar, footer) that Readability must strip. -// --------------------------------------------------------------------------- - -test( - "live: en.wikipedia.org/wiki/Markup_language extracts title, sections, and strips chrome", - { skip: !LIVE_E2E_ENABLED }, - async () => { - const { stdout, stderr } = await fetchLive( - "https://en.wikipedia.org/wiki/Markup_language", - ); - - assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); - - // The article's main title appears in the extracted markdown. - // Match case-insensitively so future title-style tweaks (e.g., "Markup - // language" vs "Markup Language") don't break the test. - assert.match( - stdout, - /markup\s+language/i, - "extracted markdown must mention the article title", - ); - - // Wikipedia articles always have a multi-section structure. - // The exact section names change over time, but ≥3 H2 sections is a - // safe floor for any article that isn't a stub. - const h2Count = (stdout.match(/^## /gm) ?? []).length; - assert.ok( - h2Count >= 3, - `expected ≥3 H2 sections in Wikipedia article markdown; got ${h2Count}`, - ); - - // Readability+turndown must strip Wikipedia's site chrome. If any of - // these substrings appear in the body, extraction has regressed. - assert.ok( - !stdout.includes(" 5000, - `extracted markdown should be substantial for a Wikipedia article; got ${stdout.length} chars`, - ); - }, -); - -// --------------------------------------------------------------------------- -// Claude Code commands docs — a modern docs-site SPA with -// server-rendered HTML that Readability is expected to extract from. -// Smaller body than Wikipedia; different framework, different chrome -// shape, exercises the pipeline against a non-Wikipedia surface. -// --------------------------------------------------------------------------- - -test( - "live: code.claude.com/docs/en/commands extracts content as clean markdown", - { skip: !LIVE_E2E_ENABLED }, - async () => { - const { stdout, stderr } = await fetchLive( - "https://code.claude.com/docs/en/commands", - ); - - assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); - - // The page is the slash-commands docs; the word "command" must appear - // somewhere in the extracted body. - assert.match( - stdout, - /command/i, - "docs page about commands must mention 'command' somewhere", - ); - - // Non-trivial extraction floor. The page has at least a heading plus - // a paragraph of intro, so 500 chars is a safe minimum. - assert.ok( - stdout.length > 500, - `extracted markdown should be non-trivial; got ${stdout.length} chars`, - ); - - // Raw HTML chrome should not survive. - assert.ok( - !stdout.includes(" Date: Thu, 14 May 2026 10:10:21 +0200 Subject: [PATCH 13/19] Add live e2e tests into the default suite Adds the two live URL tests (Wikipedia 'Markup language' and code.claude.com/docs/en/commands) directly inside tests/e2e.test.ts. Run by default as part of every npm test and every CI matrix job. Fix-up on top of the prior commit which had only deleted the standalone gated file. 70 active tests pass on macOS; 2 win32-gated tests skip on POSIX. --- tests/e2e.test.ts | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/e2e.test.ts b/tests/e2e.test.ts index 5409086..4fd882b 100644 --- a/tests/e2e.test.ts +++ b/tests/e2e.test.ts @@ -168,3 +168,40 @@ test("e2e: built output --version prints package version, exit 0", async () => { assert.equal(stderr, ""); assert.equal(stdout, "0.6.0\n"); }); + +// Live E2E against real production URLs. Exercises the full pipeline +// (HTTP/2 + Chrome fingerprint + linkedom + Readability + turndown) +// end-to-end. Assertions are property-based (title presence, section +// counts, chrome stripped) — they do not match exact upstream strings, +// because the target pages will rewrite their copy over time. + +test("e2e live: en.wikipedia.org/wiki/Markup_language extracts title, sections, and strips chrome", async () => { + const { stdout, stderr } = await execFileAsync( + "node", + [BUILT_JS, "https://en.wikipedia.org/wiki/Markup_language"], + { timeout: 30_000, maxBuffer: 10_000_000 }, + ); + assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); + assert.match(stdout, /markup\s+language/i, "extracted markdown must mention the article title"); + const h2Count = (stdout.match(/^## /gm) ?? []).length; + assert.ok(h2Count >= 3, `expected ≥3 H2 sections; got ${h2Count}`); + assert.ok(!stdout.includes(" 5000, `markdown should be substantial; got ${stdout.length} chars`); +}); + +test("e2e live: code.claude.com/docs/en/commands extracts content as clean markdown", async () => { + const { stdout, stderr } = await execFileAsync( + "node", + [BUILT_JS, "https://code.claude.com/docs/en/commands"], + { timeout: 30_000, maxBuffer: 10_000_000 }, + ); + assert.equal(stderr, "", "stderr must stay empty on a successful fetch"); + assert.match(stdout, /command/i, "docs page must mention 'command' somewhere"); + assert.ok(stdout.length > 500, `markdown should be non-trivial; got ${stdout.length} chars`); + assert.ok(!stdout.includes(" Date: Thu, 14 May 2026 10:27:11 +0200 Subject: [PATCH 14/19] Drop ephemeral 'Suggestion N from the code-review' comments These reference an ephemeral artifact (the code-review pass) and decay to noise as soon as that artifact is gone. The test names already explain the behavior under test. --- tests/sandbox.test.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/sandbox.test.ts b/tests/sandbox.test.ts index 480f95e..5890d23 100644 --- a/tests/sandbox.test.ts +++ b/tests/sandbox.test.ts @@ -46,9 +46,6 @@ test("buildAllowedRoots: empty-string env behaves like unset (defaults apply)", }); test("buildAllowedRoots: whitespace-only env throws fail-fast (malformed entry, not treated as unset)", async () => { - // Suggestion 5 from the code-review (drop outer trim) means whitespace - // is no longer silently treated as unset — it falls through to the - // isAbsolute check and fails fast like any other non-absolute value. await assert.rejects( () => buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: " " }), /every entry must be an absolute path/, @@ -94,9 +91,6 @@ test("buildAllowedRoots: non-existent entry throws fail-fast (realpath fails)", }); test("buildAllowedRoots: empty entry from leading/trailing/consecutive delimiter throws fail-fast", async () => { - // Suggestion 1 from the code-review (drop the .filter on env split) means - // typo'd env vars like ":/foo" or "/foo::/bar" no longer get silently - // partial-accepted — empty entries fall through to isAbsolute and throw. await assert.rejects( () => buildAllowedRoots({ @@ -107,8 +101,6 @@ test("buildAllowedRoots: empty entry from leading/trailing/consecutive delimiter }); test("buildAllowedRoots: regular-file entry throws fail-fast (must be a directory)", async () => { - // Suggestion 2 from the code-review: a file path pointed at by the env - // var is rejected at startup, not later via writeFile's ENOTDIR. await withSandboxTmpDir(async (dir) => { const filePath = join(dir, "regular-file.txt"); await writeFile(filePath, ""); From a01887f670b0f58740c425fe9d22fefd2554cde2 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 10:30:00 +0200 Subject: [PATCH 15/19] Raise CLI timeout-test cold-start budget 1500ms -> 3000ms The test asserts the 200ms MARKFETCH_TIMEOUT_MS fires fast, capping total elapsed at 1500ms to allow tsx cold-start. On the Windows CI runner the node + tsx ESM-loader cold-start has flaked at 1533ms. 3000ms still proves the 200ms-timeout-fired property without false failures on slow runners. --- tests/cli.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 1cd636b..bd56110 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -178,8 +178,9 @@ test("CLI: timeout when MARKFETCH_TIMEOUT_MS is small and server hangs", async ( assert.notEqual(code, 0); assert.equal(stdout, ""); assert.match(stderr, /^\[timeout\]/); - // 1500ms allows for tsx cold-start; the timeout itself fires at 200ms. - assert.ok(elapsed < 1500, `timeout should fire fast; took ${elapsed}ms`); + // 3000ms is generous headroom for node + tsx ESM-loader cold-start + // (especially slow Windows runners); the timeout itself fires at 200ms. + assert.ok(elapsed < 3000, `timeout should fire fast; took ${elapsed}ms`); } finally { await mock.close(); } From 5ecb2ad3c5cf3ef75a0cc4a92e602ca3129d774b Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 15:50:33 +0200 Subject: [PATCH 16/19] Trim verbose comments and drop unit tests duplicated by integration Comments: cut narration and step-by-step restatements; keep WHY. Affects src/sandbox.ts, src/mcp.ts, tests/_helpers.ts, tests/e2e.test.ts, .gitattributes. Tests: drop 8 unit tests in tests/sandbox.test.ts whose properties are already verified at the integration boundary in server.test.ts: - unset / empty-string / whitespace env defaults -> implicit in T1 etc. - single-entry env REPLACES defaults -> T11 - non-absolute entry throws fail-fast -> T12 - checkPath path-inside-root happy path -> T1 + every save test - checkPath path-outside-all-roots -> T9 - checkPath symlink escape blocked -> T10 Keep the narrow path-edge-cases that are painful to validate via integration: ../ traversal, prefix-overlap trap, multi-entry env split, non-existent realpath, empty-entry from delimiter, regular-file (not directory), and win32 case-fold. --- .gitattributes | 12 +--- src/mcp.ts | 13 ++-- src/sandbox.ts | 81 +++++++---------------- tests/_helpers.ts | 17 ++--- tests/e2e.test.ts | 7 +- tests/sandbox.test.ts | 145 +++++------------------------------------- 6 files changed, 52 insertions(+), 223 deletions(-) diff --git a/.gitattributes b/.gitattributes index e0142eb..c2b7591 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,11 +1,3 @@ -# Force LF line endings for all text files in the working tree, on all -# platforms. Without this, Windows runners with the default -# core.autocrlf=true convert checked-out files (.md fixtures, .ts source, -# etc.) to CRLF — and snapshot tests that compare turndown output (always -# LF) against fixture content fail with a pure line-ending diff. -# -# `text=auto` lets git auto-detect text vs. binary; `eol=lf` forces LF in -# the working tree even on Windows. The convention follows the -# .gitattributes pattern documented at -# https://docs.github.com/en/get-started/git-basics/configuring-git-to-handle-line-endings. +# Keep text files LF on all platforms. Windows runners would otherwise +# autocrlf .md fixtures to CRLF and break snapshot tests. * text=auto eol=lf diff --git a/src/mcp.ts b/src/mcp.ts index f6e0c31..82e54e2 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -16,11 +16,8 @@ import { fetchMarkdown, classifyError, type ErrorCode } from "./core.js"; import { isAbsolute } from "node:path"; import { buildAllowedRoots, checkPath } from "./sandbox.js"; -// Write-sandbox allowed roots, built once at startup. Failures here (e.g., -// MARKFETCH_ALLOWED_WRITE_ROOTS pointing at a non-existent directory) escape -// module init and surface on stderr — same fail-fast convention as intEnv() -// in core.ts. No try/catch: a bad write-roots config is a startup error, -// not a per-request error. +// Built once at startup. Bad config throws and surfaces on stderr (same +// fail-fast convention as intEnv() in core.ts). const ALLOWED_ROOTS = await buildAllowedRoots(process.env); function errorResult(code: ErrorCode, message: string) { @@ -54,10 +51,8 @@ server.registerTool( }, }, async ({ url, savePath }) => { - // Sandbox gate: MCP-only by design. The CLI adapter does not run this - // check — the human at the shell is the security boundary there. - // Run before fetchMarkdown so a forbidden path short-circuits the - // network round-trip entirely. + // Sandbox gate (MCP-only; CLI is intentionally unbounded). Runs before + // fetchMarkdown so a forbidden path short-circuits the fetch. if (savePath !== undefined) { const check = await checkPath(savePath, ALLOWED_ROOTS); if (!check.ok) { diff --git a/src/sandbox.ts b/src/sandbox.ts index 722d343..0977e38 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -1,28 +1,15 @@ -// Write-path containment for the MCP adapter. +// Write-path containment for the MCP adapter. MCP's caller is a language +// model — possibly steered by the page it just fetched — so this module +// bounds the filesystem paths it can write to. CLI is intentionally +// unbounded (human at the shell is the security boundary); only MCP uses +// this module. // -// The MCP tool exposes a `savePath` argument that lands on disk verbatim, -// and the caller is a language model — possibly steered by the page it just -// fetched. Without bounds, a hallucinated or injected path can target -// arbitrary filesystem locations under the process's UID. This module is -// the bound: it builds an allowed-roots set at startup and verifies each -// savePath stays inside it. -// -// Invariants (load-bearing — keep this list synchronized with SPEC.md): -// - Leaf module. No imports from siblings (core.ts, mcp.ts, cli.ts) so -// it stays unit-testable without spinning up the MCP server or pulling -// in undici / turndown / etc. -// - No console.* calls. buildAllowedRoots throws (the throw escapes -// module init in mcp.ts and surfaces on stderr per existing intEnv -// convention). checkPath returns a discriminated union so the caller -// decides the user-facing channel. -// - No hardcoded platform paths. Every platform-dependent value comes -// from a Node API: os.tmpdir(), process.cwd(), path.isAbsolute, -// path.delimiter, fs.realpath, process.platform. This is a non- -// negotiable review criterion for the module. -// -// Threat model: CLI is unrestricted (human is the security boundary). -// MCP is sandboxed (LLM caller is the threat surface). The asymmetry -// lives in the call site (mcp.ts uses this module; cli.ts does not). +// Invariants: +// - Leaf module: no imports from siblings, unit-testable in isolation. +// - No console.* — buildAllowedRoots throws (escapes module init in +// mcp.ts, surfaces on stderr); checkPath returns a discriminated union. +// - No hardcoded platform paths; every platform-dependent value comes +// from a Node API. import { realpath, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -42,25 +29,17 @@ export type CheckResult = | { ok: true; resolved: string } | { ok: false; reason: string }; -// Build the set of directories MCP `savePath` writes are allowed into. -// -// If MARKFETCH_ALLOWED_WRITE_ROOTS is set, its entries REPLACE the default -// set entirely — "if you set it, you own the policy." Each entry must be -// absolute and must resolve via fs.realpath at startup; we error early so -// a bad config surfaces at module init rather than at first-write time -// (consistent with intEnv's fail-fast contract in core.ts). -// -// If unset, the default set is [realpath(os.tmpdir()), realpath(cwd)]. -// Both are realpath'd once at startup so symlink-following is stable -// across all later checkPath calls. +// Defaults: realpath(os.tmpdir()) ∪ realpath(process.cwd()). +// MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (no merge); every +// entry must be absolute, resolvable via realpath, and a directory. Bad +// config throws at module init — same fail-fast contract as intEnv(). export async function buildAllowedRoots( env: NodeJS.ProcessEnv, ): Promise { const raw = env[ENV_VAR]; if (raw != null && raw !== "") { - const entries = raw.split(delimiter); const resolved: string[] = []; - for (const entry of entries) { + for (const entry of raw.split(delimiter)) { if (!isAbsolute(entry)) { throw new Error( `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, @@ -88,34 +67,17 @@ export async function buildAllowedRoots( return [await realpath(tmpdir()), await realpath(process.cwd())]; } -// Check whether a savePath, after symlink resolution, sits inside the -// allowed roots. Steps: -// 1. path.resolve to normalize "../" segments. -// 2. Walk upward to the deepest extant ancestor — the leaf usually -// doesn't exist yet (that's the point of "save"), so we can't realpath -// it directly; we realpath the closest existing directory and reattach -// the not-yet-existing trailing segments. -// 3. fs.realpath the extant ancestor — defeats symlink escape: a symlink -// planted inside the sandbox that points outside resolves to its -// outside-target before the containment check. -// 4. Reattach the trailing segments via path.join. -// 5. For each root, path.relative(root, reattached). Contained iff the -// relative path is empty (target == root), or doesn't start with ".." -// and isn't itself absolute (target is below root). On win32, fold -// both sides to lowercase first — the filesystem is case-insensitive -// and realpath doesn't reliably canonicalize case. +// Resolve savePath through fs.realpath (defeating symlink escape) and check +// containment against allowed roots. Walks up to the deepest extant ancestor +// because the leaf usually doesn't exist yet — that's the point of "save". export async function checkPath( savePath: string, roots: string[], ): Promise { const normalized = resolve(savePath); - // Walk up to find an extant ancestor. cwd or filesystem root is always - // extant in practice; this terminates at parse(p).root for the pathological - // "non-existent drive letter on Windows" case, where we fail closed. let ancestor = normalized; const trailing: string[] = []; - // Deliberate single-pass loop; iteration count is bounded by path depth. while (true) { try { await stat(ancestor); @@ -123,6 +85,7 @@ export async function checkPath( } catch { const parent = dirname(ancestor); if (parent === ancestor) { + // Reached filesystem root with no extant ancestor — fail closed. return { ok: false, reason: `cannot resolve any extant ancestor for '${savePath}'`, @@ -139,8 +102,8 @@ export async function checkPath( ? resolvedAncestor : join(resolvedAncestor, ...trailing); - // win32-only case fold. On POSIX this is identity, so paths flow through - // unchanged on case-sensitive filesystems (Linux, most macOS APFS setups). + // Win32 case-fold: filesystem is case-insensitive and fs.realpath doesn't + // reliably canonicalize case, so compare both sides lowercased. const fold = process.platform === "win32" ? (s: string) => s.toLowerCase() diff --git a/tests/_helpers.ts b/tests/_helpers.ts index 782a523..c12996c 100644 --- a/tests/_helpers.ts +++ b/tests/_helpers.ts @@ -15,17 +15,12 @@ import { pathToFileURL } from "node:url"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; -// Absolute file:// URL for tsx's loader entry. Test helpers that spawn the -// CLI via `node --import src/index.ts` use this rather than the -// `./node_modules/.bin/tsx` shim (a `.cmd` file on Windows that Node's -// native child_process.spawn cannot launch without `shell: true`). Resolved -// once at test-startup so it stays correct even when individual tests -// override the child cwd. -// -// spawnClient (below) is intentionally NOT switched to this pattern: it -// relies on the MCP SDK's StdioClientTransport, which uses its own -// cross-platform launcher and already handles `command: "tsx"` correctly -// on Windows. Touching it would couple this code to SDK internals. +// Absolute file:// URL for tsx's loader. Spawn the CLI via +// `node --import src/index.ts` to bypass the `./node_modules/.bin/tsx` +// shim (a .cmd on Windows that child_process.spawn can't launch without +// shell:true). Absolute so tests that override the child cwd still work. +// spawnClient stays on `command: "tsx"` — StdioClientTransport handles +// cross-platform spawn itself. export const TSX_LOADER_URL = pathToFileURL( resolve("./node_modules/tsx/dist/loader.mjs"), ).href; diff --git a/tests/e2e.test.ts b/tests/e2e.test.ts index 4fd882b..09367bb 100644 --- a/tests/e2e.test.ts +++ b/tests/e2e.test.ts @@ -169,11 +169,8 @@ test("e2e: built output --version prints package version, exit 0", async () => { assert.equal(stdout, "0.6.0\n"); }); -// Live E2E against real production URLs. Exercises the full pipeline -// (HTTP/2 + Chrome fingerprint + linkedom + Readability + turndown) -// end-to-end. Assertions are property-based (title presence, section -// counts, chrome stripped) — they do not match exact upstream strings, -// because the target pages will rewrite their copy over time. +// Live E2E. Exercises the full pipeline against real production URLs. +// Property-based assertions only — pages rewrite copy over time. test("e2e live: en.wikipedia.org/wiki/Markup_language extracts title, sections, and strips chrome", async () => { const { stdout, stderr } = await execFileAsync( diff --git a/tests/sandbox.test.ts b/tests/sandbox.test.ts index 5890d23..39dc204 100644 --- a/tests/sandbox.test.ts +++ b/tests/sandbox.test.ts @@ -1,25 +1,18 @@ -// Unit tests for src/sandbox.ts. -// -// Sandbox is a pure leaf module (no MCP server spin-up needed), so this -// suite exercises buildAllowedRoots and checkPath directly. That gives -// fast, deterministic coverage for the path-edge-case logic (prefix -// overlap, symlink escape, ../ traversal, win32 case-fold) that would -// be painful to validate through the integration boundary alone. -// -// Every fixture is constructed via os.tmpdir() + mkdtemp + path.join — -// no hardcoded platform paths, per the project's review criterion. +// Unit tests for src/sandbox.ts — narrow path-edge-cases that are painful +// to validate via the integration boundary in server.test.ts (../ traversal, +// prefix-overlap, multi-entry env split, fail-fast variants without an +// integration analog, win32 case-fold). All other sandbox behaviors are +// covered by T9–T13 in server.test.ts. import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdir, mkdtemp, realpath, rm, symlink, writeFile } from "node:fs/promises"; +import { mkdtemp, realpath, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { delimiter, join, parse } from "node:path"; +import { delimiter, join } from "node:path"; import { buildAllowedRoots, checkPath } from "../src/sandbox.js"; -// Scoped mkdtemp + cleanup. We realpath the directory because the sandbox -// module realpath's its inputs internally; testing against an un-resolved -// path on macOS (where /var is a symlink to /private/var) would produce -// containment mismatches that aren't bugs in the module under test. +// realpath the mkdtemp so containment compares against the same form the +// sandbox uses internally (on macOS, /var → /private/var). async function withSandboxTmpDir(fn: (dir: string) => Promise): Promise { const dir = await realpath(await mkdtemp(join(tmpdir(), "sandbox-test-"))); try { @@ -30,37 +23,9 @@ async function withSandboxTmpDir(fn: (dir: string) => Promise): Promise } // --------------------------------------------------------------------------- -// buildAllowedRoots +// buildAllowedRoots — env-parsing edge cases not exercised by integration. // --------------------------------------------------------------------------- -test("buildAllowedRoots: unset env returns realpath(tmpdir) + realpath(cwd)", async () => { - const roots = await buildAllowedRoots({}); - assert.equal(roots.length, 2); - assert.equal(roots[0], await realpath(tmpdir())); - assert.equal(roots[1], await realpath(process.cwd())); -}); - -test("buildAllowedRoots: empty-string env behaves like unset (defaults apply)", async () => { - const roots = await buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: "" }); - assert.equal(roots.length, 2); -}); - -test("buildAllowedRoots: whitespace-only env throws fail-fast (malformed entry, not treated as unset)", async () => { - await assert.rejects( - () => buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: " " }), - /every entry must be an absolute path/, - ); -}); - -test("buildAllowedRoots: single-entry env REPLACES the defaults (not merge)", async () => { - await withSandboxTmpDir(async (dir) => { - const roots = await buildAllowedRoots({ - MARKFETCH_ALLOWED_WRITE_ROOTS: dir, - }); - assert.deepEqual(roots, [dir]); - }); -}); - test("buildAllowedRoots: multi-entry env split by path.delimiter", async () => { await withSandboxTmpDir(async (a) => { await withSandboxTmpDir(async (b) => { @@ -72,14 +37,6 @@ test("buildAllowedRoots: multi-entry env split by path.delimiter", async () => { }); }); -test("buildAllowedRoots: non-absolute entry throws fail-fast", async () => { - await assert.rejects( - () => - buildAllowedRoots({ MARKFETCH_ALLOWED_WRITE_ROOTS: "relative/path" }), - /every entry must be an absolute path/, - ); -}); - test("buildAllowedRoots: non-existent entry throws fail-fast (realpath fails)", async () => { await withSandboxTmpDir(async (dir) => { const nope = join(dir, "does-not-exist"); @@ -112,39 +69,13 @@ test("buildAllowedRoots: regular-file entry throws fail-fast (must be a director }); // --------------------------------------------------------------------------- -// checkPath +// checkPath — narrow containment-logic edge cases. // --------------------------------------------------------------------------- -test("checkPath: path inside a root is ok", async () => { - await withSandboxTmpDir(async (dir) => { - const result = await checkPath(join(dir, "out.md"), [dir]); - assert.equal(result.ok, true); - }); -}); - -test("checkPath: path outside all roots is not ok; message names the roots", async () => { - await withSandboxTmpDir(async (dir) => { - // Filesystem root is always outside an mkdtemp dir under tmpdir. - // Use parse(dir).root so the test stays portable to Windows drive - // letters (`C:\\`) without hardcoding. - const outside = join(parse(dir).root, "very-unlikely-target.md"); - const result = await checkPath(outside, [dir]); - assert.equal(result.ok, false); - if (!result.ok) { - assert.match(result.reason, /outside the allowed write roots/); - assert.ok( - result.reason.includes(dir), - "reason should name the allowed root for caller-side recovery", - ); - } - }); -}); - test("checkPath: ../ traversal that escapes a root is not ok", async () => { await withSandboxTmpDir(async (dir) => { // dir/sub/../../escape resolves to dir/../escape — a sibling of dir. - // path.resolve handles the `..` normalization; path.relative then - // refuses the resulting outside-root path. + // path.resolve normalizes the ..; path.relative then rejects. const escape = join(dir, "sub", "..", "..", "escape.md"); const result = await checkPath(escape, [dir]); assert.equal(result.ok, false); @@ -153,67 +84,23 @@ test("checkPath: ../ traversal that escapes a root is not ok", async () => { test("checkPath: prefix-overlap trap (root /tmp vs target /tmp-evil)", async () => { await withSandboxTmpDir(async (root) => { - // Construct a sibling whose name shares a prefix with root. A naive - // `target.startsWith(root)` would pass this incorrectly; path.relative - // returns "..//..." which we reject. - const sibling = `${root}-evil`; - const target = join(sibling, "trap.md"); + // Sibling sharing root's prefix. A naive target.startsWith(root) would + // pass this incorrectly; path.relative returns "..//trap.md". + const target = join(`${root}-evil`, "trap.md"); const result = await checkPath(target, [root]); assert.equal(result.ok, false); }); }); -// --------------------------------------------------------------------------- -// Symlink escape — POSIX-gated. -// Windows symlink creation typically requires elevation; the platform- -// independent property under test (realpath defeats symlink escape) is -// implemented identically on both, so POSIX coverage is sufficient. -// --------------------------------------------------------------------------- - -test( - "checkPath: symlink inside sandbox pointing outside is blocked by realpath", - { skip: process.platform === "win32" }, - async () => { - await withSandboxTmpDir(async (sandbox) => { - await withSandboxTmpDir(async (outside) => { - const inner = join(sandbox, "inner"); - await mkdir(inner); - const link = join(inner, "escape"); - await symlink(outside, link); - // The intent: write through inner/escape (a symlink) into outside. - // realpath resolves the symlink, and the containment check then - // sees `outside/should-not-write.md`, which is outside `sandbox`. - const target = join(link, "should-not-write.md"); - const result = await checkPath(target, [sandbox]); - assert.equal(result.ok, false, "symlink escape must be blocked"); - if (!result.ok) { - assert.match(result.reason, /outside the allowed write roots/); - } - }); - }); - }, -); - -// --------------------------------------------------------------------------- -// Windows case-insensitivity — win32-gated. -// --------------------------------------------------------------------------- - test( "checkPath: case-variant paths compare equal on win32", { skip: process.platform !== "win32" }, async () => { await withSandboxTmpDir(async (dir) => { - // Construct a case-variant root: leave the drive letter as-is but - // case-flip everything after it. The filesystem treats them as the - // same directory; the sandbox must too. const variant = dir.charAt(0) + dir.slice(1).toLowerCase(); const target = join(dir.toUpperCase(), "out.md"); const result = await checkPath(target, [variant]); - assert.equal( - result.ok, - true, - "win32 containment compare must be case-insensitive", - ); + assert.equal(result.ok, true); }); }, ); From 67ffc6b02b3e51a186f88a15fcb627648b37dcf3 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 17:41:49 +0200 Subject: [PATCH 17/19] Drop redundant section dividers and duplicate comments - server.test.ts: drop the 9-line Sandbox section header (info lives in README and SPEC; test names group themselves). - sandbox.test.ts: drop the two ASCII-bar section subheaders (buildAllowedRoots and checkPath group themselves by test name). - cli.test.ts: trim the 7-line ENTRY comment to one line; the TSX_LOADER_URL rationale is already documented in _helpers.ts. --- tests/cli.test.ts | 8 +------- tests/sandbox.test.ts | 8 -------- tests/server.test.ts | 10 ---------- 3 files changed, 1 insertion(+), 25 deletions(-) diff --git a/tests/cli.test.ts b/tests/cli.test.ts index bd56110..ebdcdd5 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -13,13 +13,7 @@ import { startMock, HAPPY_FIXTURE, TSX_LOADER_URL } from "./_helpers.js"; const execFileAsync = promisify(execFile); -// Resolved at module load against the test runner's cwd (the project root). -// Tests that override `cwd` to a tmpdir still need to find the source entry -// — a relative path would resolve against the new cwd and produce a -// confusing ENOENT instead of the behavior under test. The tsx loader is -// imported via TSX_LOADER_URL (an absolute file:// URL from _helpers.ts) -// for the same portability reason; it also bypasses the platform-specific -// `./node_modules/.bin/tsx[.cmd]` shim entirely. +// Absolute so tests that override the child cwd still locate the entry. const ENTRY = resolvePath("src/index.ts"); type RunResult = { code: number; stdout: string; stderr: string }; diff --git a/tests/sandbox.test.ts b/tests/sandbox.test.ts index 39dc204..24eaf9a 100644 --- a/tests/sandbox.test.ts +++ b/tests/sandbox.test.ts @@ -22,10 +22,6 @@ async function withSandboxTmpDir(fn: (dir: string) => Promise): Promise } } -// --------------------------------------------------------------------------- -// buildAllowedRoots — env-parsing edge cases not exercised by integration. -// --------------------------------------------------------------------------- - test("buildAllowedRoots: multi-entry env split by path.delimiter", async () => { await withSandboxTmpDir(async (a) => { await withSandboxTmpDir(async (b) => { @@ -68,10 +64,6 @@ test("buildAllowedRoots: regular-file entry throws fail-fast (must be a director }); }); -// --------------------------------------------------------------------------- -// checkPath — narrow containment-logic edge cases. -// --------------------------------------------------------------------------- - test("checkPath: ../ traversal that escapes a root is not ok", async () => { await withSandboxTmpDir(async (dir) => { // dir/sub/../../escape resolves to dir/../escape — a sibling of dir. diff --git a/tests/server.test.ts b/tests/server.test.ts index 025b3c6..4b7c9e4 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -637,16 +637,6 @@ test("savePath: existing file is overwritten", async () => { }); }); -// --------------------------------------------------------------------------- -// Sandbox (write-path guardrails on the MCP side) -// -// MCP `savePath` writes are confined to allowed roots — by default -// realpath(os.tmpdir()) ∪ realpath(process.cwd()), overridable via the -// MARKFETCH_ALLOWED_WRITE_ROOTS env var. The CLI is intentionally NOT -// guard-railed (covered in tests/cli.test.ts); the asymmetry reflects -// the threat model (LLM caller vs. human caller). -// --------------------------------------------------------------------------- - // T9 — savePath outside the default sandbox → [save_forbidden]; no file. test("savePath sandbox: path outside default roots → [save_forbidden] and file not written", async () => { const mock = await startMock((_req, res) => { From 6b00ecc333f0482e8983aeb7697504ec6cc5a899 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Thu, 14 May 2026 20:21:04 +0200 Subject: [PATCH 18/19] Document MARKFETCH_ALLOWED_WRITE_ROOTS replace-not-merge as deliberate Behavior unchanged. The override has always replaced the defaults; this just makes the rationale visible at the two places a reader is likely to hit the surprise: - src/sandbox.ts: comment explains the deliberate choice (setting the var asserts a policy; additive defaults would weaken it). - README.md: the override paragraph now states 'does not merge' and explains why /tmp appears in the example. T11 in tests/server.test.ts continues to enforce the contract. --- README.md | 2 +- src/sandbox.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index b087a9b..e2409b6 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,7 @@ Pass overrides via the `env` block of your MCP client config: MCP `savePath` writes are confined to a set of allowed root directories. By default the allowed set is `os.tmpdir()` ∪ `process.cwd()` (each resolved via `fs.realpath` once at startup). A `savePath` outside that set returns `save_forbidden` and no file is created. -Override the default set with `MARKFETCH_ALLOWED_WRITE_ROOTS` — a list of absolute paths separated by the platform's path delimiter (`:` on POSIX, `;` on Windows). When set, the override **replaces** the defaults entirely; if you set it, you own the policy. A malformed value (non-absolute entry, or a directory that doesn't exist) fails fast on stderr at startup. +Override the default set with `MARKFETCH_ALLOWED_WRITE_ROOTS` — a list of absolute paths separated by the platform's path delimiter (`:` on POSIX, `;` on Windows). When set, the override **replaces** the defaults entirely — it does not merge. To keep `os.tmpdir()` or `process.cwd()` accessible, list them yourself; the example below shows `/tmp` for that reason. A malformed value (non-absolute entry, or a directory that doesn't exist) fails fast on stderr at startup. ```json { diff --git a/src/sandbox.ts b/src/sandbox.ts index 0977e38..e3de74a 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -30,9 +30,11 @@ export type CheckResult = | { ok: false; reason: string }; // Defaults: realpath(os.tmpdir()) ∪ realpath(process.cwd()). -// MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (no merge); every -// entry must be absolute, resolvable via realpath, and a directory. Bad -// config throws at module init — same fail-fast contract as intEnv(). +// MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (no merge) — deliberate; +// setting it is asserting a policy, so additive defaults would weaken it. +// Callers who want tmpdir/cwd back must list them explicitly. Every entry +// must be absolute, resolvable via realpath, and a directory. Bad config +// throws at module init — same fail-fast contract as intEnv(). export async function buildAllowedRoots( env: NodeJS.ProcessEnv, ): Promise { From b65d09fd70ed12756f41e1218304ee6b12cecbb0 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Fri, 15 May 2026 00:22:15 +0200 Subject: [PATCH 19/19] Bump version from 0.5.0 to 0.6.0 in package-lock.json --- package-lock.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 83fb40d..3a1e30b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "markfetch", - "version": "0.5.0", + "version": "0.6.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "markfetch", - "version": "0.5.0", + "version": "0.6.0", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.29.0",