Cross-platform savePath + MCP write sandbox (markfetch 0.6.0)#4
Merged
Conversation
- 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.
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.
- 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).
- 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.
- 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.
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.
- 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().
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.
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 <tsx-loader-file-url> <entry>`, 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 ('<path>'); 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.
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.
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 `<nav`, `<script`, `<style`, no MediaWiki `mw-*` classes leaking through), substantial markdown length. No exact-string matching against upstream content; the pages will rewrite their copy over time, and brittle string assertions would generate maintenance churn. CI integration is deliberately deferred. These tests should not gate every PR — flakiness from network, rate limits, or upstream content drift would generate false negatives. A nightly schedule or a manual-dispatch live-check workflow would be a sensible follow-up if needed. Test count: 72 total — 68 active pass on all platforms, 2 win32-gated + 2 live-gated tests skip by default.
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.
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.
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.
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.
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.
- 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.
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.
There was a problem hiding this comment.
Pull request overview
This PR prepares markfetch 0.6.0 by making MCP savePath handling cross-platform and adding an MCP-only write sandbox, with docs and tests updated for the new behavior.
Changes:
- Adds
src/sandbox.tsand wires MCPsavePaththrough allowed-root validation withsave_forbidden. - Updates version strings, README/SPEC/CHANGELOG documentation, and CI to cover multiple OSes.
- Adds sandbox-focused tests plus Windows/path and live e2e coverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/sandbox.ts |
Adds allowed-root construction and save-path containment checks. |
src/mcp.ts |
Applies MCP sandbox validation and updates tool schema/version/docs. |
src/core.ts |
Adds save_forbidden to the shared error-code contract. |
src/cli.ts |
Bumps CLI version to 0.6.0. |
tests/server.test.ts |
Adds MCP sandbox integration coverage and updates version assertions. |
tests/sandbox.test.ts |
Adds unit tests for sandbox path edge cases. |
tests/e2e.test.ts |
Updates version assertions and adds live production URL tests. |
tests/cli.test.ts |
Updates CLI test spawning and version/timeout expectations. |
tests/_helpers.ts |
Adds shared tsx loader URL and updates error-prefix helpers. |
README.md |
Documents the write sandbox and new error code. |
docs/SPEC.md |
Updates design/spec notes for cross-platform save paths and sandboxing. |
CHANGELOG.md |
Adds the 0.6.0 release notes. |
package.json |
Bumps package version to 0.6.0. |
.github/workflows/ci.yml |
Expands CI to an OS matrix and adjusts test shell. |
.gitattributes |
Enforces LF line endings for text files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fetch_markdownaccepts absolutesavePathvalues on every host OS — POSIX/fooand WindowsC:\foo,C:/foo,\\server\share\foo,\foo. Validation runs throughpath.isAbsoluteinstead of a hardcoded/prefix.realpath(os.tmpdir()) ∪ realpath(process.cwd()). Symlinks inside the sandbox cannot be used to escape: the target is resolved throughfs.realpathbefore the containment check.MARKFETCH_ALLOWED_WRITE_ROOTS(path-delimiter-separated) replaces the defaults — it does not extend them. Bad input fails fast on stderr at startup, mirroring the existingMARKFETCH_TIMEOUT_MS/MARKFETCH_USER_AGENTconvention.savePathfalls outside the allowed roots, the MCP tool returns a newsave_forbiddenerror code (8th in the contract). The CLI is unaffected — its write surface is unchanged on every platform.testjob runs on aubuntu-latest/macos-latest/windows-latestmatrix. Only first-party actions (actions/checkout,actions/setup-node)..gitattributesenforces LF in the working tree so Windows checkouts don't introduce CRLF that would skew fixtures or shebangs.Breaking change
MCP callers that previously wrote outside
os.tmpdir()orprocess.cwd()will now receivesave_forbidden. Resolution: move the target inside the default roots, or setMARKFETCH_ALLOWED_WRITE_ROOTSto a path list that includes it. CLI callers are not affected.Why
Two problems surfaced together:
savePathwas Unix-only. Windows hosts could not use the MCP tool at all — every absolute Windows path shape was rejected at the schema layer.savePath; not acceptable now that MCP exposes the same write capability to a model.The threat-model asymmetry between the CLI (trusted human at a shell) and MCP (untrusted model, possibly prompt-injected) is the seam the sandbox lives along: enforcement is in the MCP adapter, not in
core.tsorcli.ts. This is documented as the "Asymmetric write sandbox" Core Decision indocs/SPEC.md.Follow-up candidates (deferred from review)
rel.startsWith("..")insrc/sandbox.tsrejects filenames literally starting with..(e.g.,/tmp/..notes.md). Uncommon but a real false-reject; one-line fix.tests/server.test.tsT11 overpromises "replace not merge" — the assertion only provesos.tmpdir()is not merged; theprocess.cwd()not-merged property is covered by a separate unit test.Full design rationale and acceptance criteria live in the internal plan file on the developer's machine.