fix: apply URL filters to resource loading#2225
Conversation
fac5343 to
a611c2c
Compare
nattallius
left a comment
There was a problem hiding this comment.
Thanks you for contribution! Looks good for me mostly.
Сan you also add validation of both patterns in src/bin/chrome-devtools-mcp-cli-options.ts
blockedUrlPattern: {
type: 'array',
describe:
'Restricts network access by blocking specified URL patterns (uses https://urlpattern.spec.whatwg.org/). Silently detaches from targets with blocked URLs upon connection, and blocks runtime requests (including navigations and subresources). Accepts an array of patterns.',
conflicts: ['allowedUrlPattern'],
coerce: (patterns: unknown[] | undefined) => {
if (!patterns) {
return undefined;
}
for (const pattern of patterns) {
new URLPattern(String(pattern));
}
return patterns.map(String);
},
}
| performanceCrux: boolean; | ||
| // Whether allowlist/blocklist is configured. | ||
| hasNetworkBlockOrAllowlist?: boolean; | ||
| // URL patterns to block for direct resource loads. |
There was a problem hiding this comment.
Now as we pass options into context, we don't need to pass hasNetworkBlockOrAllowlist and we can calculate and assign it in the context constructor ourself. Can you please remove it from McpContextOptions
| }); | ||
| }); | ||
|
|
||
| it('http protocol respects the network blocklist', async () => { |
There was a problem hiding this comment.
| it('http protocol respects the network blocklist', async () => { | |
| it('should block remote resources matching the blocklist', async () => { |
| ); | ||
| }); | ||
|
|
||
| it('http protocol respects the network allowlist', async () => { |
There was a problem hiding this comment.
| it('http protocol respects the network allowlist', async () => { | |
| it('should only load remote resources matching the allowlist', async () => { |
| ); | ||
| }); | ||
|
|
||
| it('http protocol ignores an empty network allowlist', async () => { |
There was a problem hiding this comment.
| it('http protocol ignores an empty network allowlist', async () => { | |
| it('should allow all remote resources if the allowlist is empty', async () => { |
| describe: | ||
| 'Restricts network access by allowing only specified URL patterns (uses https://urlpattern.spec.whatwg.org/). Requires Chrome 149+. Silently detaches from targets with unallowed URLs upon connection, and blocks runtime requests (including navigations and subresources). Accepts an array of patterns.', | ||
| conflicts: ['blockedUrlPattern'], | ||
| coerce: (patterns: unknown[] | undefined) => { |
There was a problem hiding this comment.
Let's extract this is a helper function - verifyUrlPatterns
| this.#allowlist = | ||
| options.allowlist && options.allowlist.length > 0 | ||
| ? options.allowlist.map(pattern => new URLPattern(pattern)) | ||
| : undefined; |
There was a problem hiding this comment.
| this.#allowlist = | |
| options.allowlist && options.allowlist.length > 0 | |
| ? options.allowlist.map(pattern => new URLPattern(pattern)) | |
| : undefined; | |
| this.#allowlist = options.allowlist?.map(pattern => new URLPattern(pattern)); |
| #options: McpContextOptions; | ||
| #heapSnapshotManager = new HeapSnapshotManager(); | ||
| #roots: Root[] | undefined = undefined; | ||
| #blocklist: URLPattern[]; |
There was a problem hiding this comment.
We can have the same pattern of
| #blocklist: URLPattern[]; | |
| #blocklist: URLPattern[] | undefined; |
And check for existence in #validateNetworkResource
| } | ||
|
|
||
| #validateNetworkResource(url: URL): void { | ||
| const urlString = url.href; |
There was a problem hiding this comment.
Not needed for this variable we can pass the url directly to the pattern.test
| import {getMockRequest, html, withMcpContext} from './utils.js'; | ||
|
|
||
| describe('McpContext', () => { | ||
| const server = serverHooks(); |
There was a problem hiding this comment.
Let's move this under describe('loadResource', () => {
|
|
||
| it('rejects invalid blocked-url-pattern values', async () => { | ||
| assert.throws(() => | ||
| cliOptions.blockedUrlPattern.coerce(['https://[invalid']), |
There was a problem hiding this comment.
We should not test the function directly but use the parseArguments to validate the parsing. Similar to how it's done for other tests.
ace7cb8 to
574cd29
Compare
Fixes #2218.
McpContext.loadResource()already validatedfile:resources through workspace roots, but directhttp:/https:resource fetches did not apply the configured URL filters. This threads the existing blocked/allowed URL patterns into the context and checks them before fetching remote resources.Regression coverage uses the local test server to verify blocked URLs are rejected, allowed URLs still load, and an empty allowlist does not accidentally deny all resource loads.
How I tested:
npm run test tests/McpContext.test.tsnpm run test tests/network_blocking.test.tsnpm run check-formatI also tried
npm run test; the build completed, but the full suite failed locally in timing-sensitive unrelated tests (telemetry.test.tswaiting forserver_start, andshutdown.test.tsstdin-EOF shutdown budget).