test(docx-mcp): harden vitest path allowlist against /tmp symlink topology#105
Merged
stevenobiajulu merged 2 commits intomainfrom Apr 27, 2026
Merged
Conversation
…ology The docx-mcp suite uses openDocument, which gates reads through enforceReadPathPolicy against SAFE_DOCX_ALLOWED_ROOTS. A vitest setup file is responsible for adding the workspace root and process.cwd() so tests work regardless of where the worktree lives, but two papercuts made env-only PATH_NOT_ALLOWED failures recur: 1. setup-path-roots.ts resolved "repo root" three segments up from src/testing/, which lands on packages/ rather than the actual workspace root. As a side effect, path.join(repoRoot, 'packages', 'docx-core') resolved to the bogus packages/packages/docx-core and nothing under the workspace root proper was on the allowlist. 2. The setup added raw paths like /tmp/worktree-foo, never realpathed. On macOS /tmp is a symlink to /private/tmp; enforceReadPathPolicy realpaths the requested file (giving /private/tmp/...) and rejected it because the env entry was the un-realpathed form. Fix both: walk four segments up to the workspace root, and run every candidate through fs.realpathSync before joining. The lazy canonicalization inside resolveAllowedRoots() still works as a backstop but the env var is now correct on its face, which is easier to reason about. Also improve assertSuccess() in session-test-utils.ts so a failing result surfaces error.code and error.message in the assertion failure instead of "expected true received false". The previous form wasted ~10 minutes per occurrence whenever PATH_NOT_ALLOWED came back from openDocument. Add a path_policy regression test that builds a directory + sibling symlink and verifies access is allowed in both directions, pinning the /tmp ↔ /private/tmp behavior on Linux too. Fixes: #104
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Peer review flagged that the comment overclaimed: runtime path policy already canonicalizes every SAFE_DOCX_ALLOWED_ROOTS entry inside resolveAllowedRoots, so setup-side realpath is hardening (and a log-readability nicety), not the primary fix. The primary fix is the workspace-root segment count correction one block above. Ref: #104
3 tasks
stevenobiajulu
added a commit
that referenced
this pull request
Apr 28, 2026
* chore(release): bump workspace versions to 0.9.1 Patch release covering bug fixes accumulated since v0.9.0: - fix(docx-core): merge auxiliary parts in rebuild mode (#94, #101) - fix(docx-core): emit paragraph-level markers outside <w:r> on rebuild (#109) - fix(docx-core): preserve reply comments in rebuild mode (#108, #112) - test(docx-mcp): harden vitest path allowlist against /tmp symlink topology (#105) - ci: SHA-pin all GitHub Actions, add dependency review and Dependabot - ci: fix PR title check for acronyms and fork PRs (#88) - chore(skills): sync docx-editing to 0.3.0 (#95) Generated via scripts/bump_version.mjs. * fix(docx-mcp): declare @xmldom/xmldom dependency explicitly The bumped package-lock.json regenerated cleanly and exposed a latent bug: docx-mcp directly imports @xmldom/xmldom in tag_parser.ts but did not declare it as a dependency. It was previously masked by npm hoisting from docx-core's transitive dep — the workspace happened to work because the lockfile carried a stale hoisted xmldom@0.8.11. Pinning to ^0.8.11 (matching what was already running in production via the silent hoist) preserves observable behavior. tag_parser depends on xmldom 0.8's lenient parsing of cross-nested tags (<b><i>text</b></i>) — xmldom 0.9 throws on such input. Migrating docx-mcp's tag_parser to xmldom 0.9 is its own concern; this commit keeps the 0.9.1 release a pure version bump. docx-core continues to use xmldom ^0.9.9 (PR #75); the two versions coexist under their respective package's node_modules. No DOM nodes cross the docx-core/docx-mcp boundary, so the version split is safe.
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
1 participant
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
setup-path-roots.tsto compute the workspace root with the right segment count (was landing onpackages/, broke thepackages/docx-corejoin), and realpath every candidate before writingSAFE_DOCX_ALLOWED_ROOTSso/tmp↔/private/tmptopology no longer drops fixtures off the allowlist.assertSuccessinsession-test-utils.tsto surfaceerror.code+error.messageon failure instead of the bareexpected true received false.path_policyregression test that pins symlinked-root equivalence using a self-built/tmp/realdir+ symlink pair (works on Linux too).Fixes: #104
Why
A worktree under
/tmp(which is/private/tmpon macOS) was producingPATH_NOT_ALLOWEDfromopenDocumenteven though setup-path-roots already addedcwdand the repo path. Two papercuts: the "repo root" walk was off by one segment, and the env entries weren't realpathed before being compared against the realpathed file path. TheassertSuccesschange is the smaller half of the same problem — when the gate did fire, the test failure didn't show the error code, so root-causing burned ~10 minutes per occurrence.Test plan
npm run test:run --workspace=packages/docx-mcp— all 638 tests green.npm run lint:workspaces— no errors (one preexisting warning unrelated to this change).npm run check:spec-coverage— pass.env -u SAFE_DOCX_ALLOWED_ROOTS npx vitest --root packages/docx-mcp run src/tools/path_policy.test.ts src/tools/path_policy_symlink_bounds.test.ts src/tools/open_document.test.ts— passes from a worktree under/tmp//private/tmpwithout any env var override.enforceReadPathPolicy > allows access when allowed root and file path differ only by symlinkexercises both directions of the symlink-root equivalence and was added rather than just relying on existing coverage.