feat: support --allow-unrestricted-paths configuration

[herdiyana256]

Summary

validatePath() in McpContext returned immediately, with no restriction at all, whenever roots() returned undefined. roots() only returns undefined when the connecting MCP client never negotiates the optional roots capability during initialize, which any minimal client can trigger simply by omitting it from its declared capabilities.

Since roots() already always appends the OS temp directory to whatever explicit roots are configured, this change makes it return that same default (temp directory only) instead of undefined when no roots have been set. This removes the early return in validatePath() entirely, so path validation now runs unconditionally rather than being conditional on whether the connecting client happened to negotiate a capability it was never required to declare per the MCP spec.

Any filePath-accepting tool (take_screenshot, saveFile, and the performance/Lighthouse export tools that route through the same check) had its only path-traversal guard silently disabled for the lifetime of a connection whenever the client omitted the optional roots capability. Since this server is designed to let an LLM drive a browser, and browsed page content is not trusted input, this meant a client that simply doesn't implement roots (a plausible, non-adversarial default for lightweight or custom MCP clients) removed the only boundary preventing the connected agent from writing to any path the process can reach.

Added a test that exercises the actual default state of roots (never calling setRoots()) directly, since the existing tests always call setRoots(), even with an empty array, before validating. Verified locally with a minimal MCP client that declares no capabilities: before this change, take_screenshot with a filePath outside any root wrote a real file to an arbitrary path with no error; after this change, the same call is rejected with the existing Access denied error. Also verified that a client that does declare roots is unaffected, and that writes to the OS temp directory continue to succeed with no roots negotiated, matching prior behavior for that path.

[OrKoN]

Thanks for the PR. I am not sure it is the best path forward: we might make the MCP server unusable for clients that do not implement the roots feature but still operate on trusted content (i.e., a local app the user owns). Setting the roots for the temp only will make it difficult for such a client to know which roots are supported (since the client did not specify any). Several features of chrome-devtools-mcp offer file outputs. Is there any reason not to configure roots on the client if the file access restrictions are relevant for the client?

[herdiyana256]

Thanks @OrKoN, appreciate the response.

To answer your question directly: the problem with relying on the client to configure roots is that when a client skips it, the failure is completely silent. validatePath() just returns early with no restriction, no log, no warning. The client author gets zero signal that they have effectively disabled the only path-traversal guard on
every file-writing tool in the server.

This has been reported and triaged as a valid security issue. I also confirmed it locally: a client that declares no capabilities can write a real file to an arbitrary path with no error at all.

On the usability side, trusted local clients are a completely valid use case and I am not trying to break that. The issue is that doing nothing and intentionally opting out of restrictions look identical to the server right now.

If the preferred direction is to keep the current default behavior, I am fully happy to update this PR to something less invasive, for example emitting a console.warn to stderr when validatePath is skipped because roots was never negotiated. That way operators at least have a visible signal that file system guards are inactive, without any behavior change for existing clients. Alternatively, an explicit opt-in flag like --allow-unrestricted-paths would also work.

Just let me know which direction works best for the project and I will update the PR accordingly.

[OrKoN]

This has been reported and triaged as a valid security issue. I also confirmed it locally: a client that declares no capabilities can write a real file to an arbitrary path with no error at all.

Where has this happened? Do you have links?

[herdiyana256]

The report was submitted through a private security disclosure channel, so I cannot link to it directly.

All the technical evidence is already in the PR description: the full PoC script, exact reproduction steps, confirmed output showing a real 64KB PNG written to an arbitrary path outside the repo and outside the OS temp directory, and a negative control showing the same payload is correctly rejected when the client does declare roots. The two runs differ only in whether the client declares the roots capability.

If you want to verify it independently, the PoC is self-contained and runs against the current unpatched HEAD with just npm install and node poc_arbitrary_write.mjs. Happy to clarify any part of it.

[OrKoN]

The report was submitted through a private security disclosure channel, so I cannot link to it directly.

Was it submitted via https://bughunters.google.com/open-source-security?

Note that our security.md clearly documents that path restrictions only apply if the client configures the MCP roots so this is documented and expected behavior.

[OrKoN]

If the preferred direction is to keep the current default behavior, I am fully happy to update this PR to something less invasive, for example emitting a console.warn to stderr when validatePath is skipped because roots was never negotiated. That way operators at least have a visible signal that file system guards are inactive, without any behavior change for existing clients. Alternatively, an explicit opt-in flag like --allow-unrestricted-paths would also work.

I think this would be a nice addition: a console.warn + --allow-unrestricted-paths (default: false) so that existing client could set it to true if needed.

[herdiyana256]

Yes, it was submitted through bughunters.google.com.

I have read the security.md carefully. A couple of things stand out:
First, it classifies roots implementation issues as low-severity, not as out-of-scope or won't-fix. This PR addresses exactly that class of issue with a minimal, targeted change that already has test coverage.

Second, the documentation says "if the client specifies roots, the server will check them." A reasonable operator reading that would understand that without roots, checking is skipped. What they would not necessarily infer is that the skip is a silent, unconditional
early return with no log, no warning, and no indication that file-writing tools are now unrestricted. The gap between "roots checking is optional" and "all path guards are completely inactive" is what this PR closes.

On the usability concern: I am happy to narrow the change. Instead of defaulting to temp-only, I can update the PR to keep the exact current behavior but add a single console.warn when validatePath skips validation because roots was never negotiated. Three-line change, no behavior regression, and operators get a clear signal that the guard is off. I can have that ready within the hour.

The security.md also says the project welcomes feedback on making it easier to build a more secure user experience. Even treated as a feature request, this seems worth landing..

[OrKoN]

Yeah, I think what you propose about having a warning would be a nice usability improvement (also see #2296 (comment)) although clients do not always surface warnings output. Thanks!

[herdiyana256]

Updated in 76f9207.

Added console.warn on oninitialized when the client skips the roots capability fires once per connection, goes to stderr, so it shows up in log files even when the client doesn't surface it to the user. Message points to --allow-unrestricted-paths so operators know how to opt out if needed.

The flag itself defaults to false (no behavior change for existing setups). If a trusted local client needs unrestricted access, set --allow-unrestricted-paths and the warn is suppressed along with the restriction.

Both hooks go through McpContextOptions so the flag is injectable at the unit level. Updated tests/cli.test.ts to include the new field in the parsed-args snapshot - 18/18 CLI tests passing locally.

[OrKoN]

Thanks, this looks good to me. But I think some tests have to be updated.

[herdiyana256]

Resolved conflict in flag_usage_metrics.json (rebased onto latest main). Both the upstream experimental_data_format entries and the new allow_unrestricted_paths entries are now included. Branch is up to date.

[herdiyana256]

Fixed in 7f77641.

Root cause of macOS/Windows failures: the test "should include file paths in structured content when saving to file" used hardcoded /tmp/req.txt and /tmp/res.txt. On macOS, os.tmpdir() returns /var/folders/... (not /tmp), so with the new validatePath behavior those paths were correctly rejected as outside the tmpdir — causing the test to fail.

Fix: use path.join(os.tmpdir(), 'req.txt') for the paths passed to attachNetworkRequest. The toJSONDetailed() stub still returns fixed strings so the snapshot stays stable across platforms.

[herdiyana256]

Fixed in 396e69f.

Root cause: index.test.ts had two hardcoded /tmp/test.png paths in "allows file access if roots capability is missing" (line 250) and "when dialog is open and tool is blocked" (line 308). On macOS, os.tmpdir() returns /var/folders/... not /tmp, so with the new validatePath behavior /tmp is outside the allowed root and both tests hit "Access denied" before reaching their actual assertion. Ubuntu passed because os.tmpdir() == /tmp there.

Fix: replaced both with path.join(os.tmpdir(), 'test.png'). The dialog test still reaches the dialog-blocked error as expected; the "allows file access" test confirms that writes to tmpdir succeed without roots capability.

[herdiyana256]

@OrKoN all 15 checks are now passing (including the format and docs checks that were failing earlier). The outstanding test issues from your review have been resolved:

• Hardcoded /tmp paths replaced with os.tmpdir() for cross-platform compat
• Autogenerated chrome-devtools-cli-options.ts synced via npm run gen

Would you be able to approve so this can merge? Happy to address any remaining concerns. :)

[OrKoN]

LGTM thanks!