-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix: apply URL filters to resource loading #2225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -66,6 +66,10 @@ interface McpContextOptions { | |||||||||||
| performanceCrux: boolean; | ||||||||||||
| // Whether allowlist/blocklist is configured. | ||||||||||||
| hasNetworkBlockOrAllowlist?: boolean; | ||||||||||||
| // URL patterns to block for direct resource loads. | ||||||||||||
| blocklist?: string[]; | ||||||||||||
| // URL patterns to allow for direct resource loads. | ||||||||||||
| allowlist?: string[]; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const DEFAULT_TIMEOUT = 5_000; | ||||||||||||
|
|
@@ -106,6 +110,8 @@ export class McpContext implements Context { | |||||||||||
| #options: McpContextOptions; | ||||||||||||
| #heapSnapshotManager = new HeapSnapshotManager(); | ||||||||||||
| #roots: Root[] | undefined = undefined; | ||||||||||||
| #blocklist: URLPattern[]; | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can have the same pattern of
Suggested change
And check for existence in |
||||||||||||
| #allowlist: URLPattern[] | undefined; | ||||||||||||
|
|
||||||||||||
| private constructor( | ||||||||||||
| browser: Browser, | ||||||||||||
|
|
@@ -123,6 +129,12 @@ export class McpContext implements Context { | |||||||||||
| this.logger = logger; | ||||||||||||
| this.#locatorClass = locatorClass; | ||||||||||||
| this.#options = options; | ||||||||||||
| this.#blocklist = | ||||||||||||
| options.blocklist?.map(pattern => new URLPattern(pattern)) ?? []; | ||||||||||||
| this.#allowlist = | ||||||||||||
| options.allowlist && options.allowlist.length > 0 | ||||||||||||
| ? options.allowlist.map(pattern => new URLPattern(pattern)) | ||||||||||||
| : undefined; | ||||||||||||
|
Comment on lines
+134
to
+137
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
|
|
||||||||||||
| this.#networkCollector = new NetworkCollector(this.browser); | ||||||||||||
|
|
||||||||||||
|
|
@@ -958,7 +970,7 @@ export class McpContext implements Context { | |||||||||||
| switch (url.protocol) { | ||||||||||||
| case 'https:': | ||||||||||||
| case 'http:': { | ||||||||||||
| // TODO: Verify allow/block list | ||||||||||||
| this.#validateNetworkResource(url); | ||||||||||||
| const response = await fetch(url); | ||||||||||||
| if (!response.ok) { | ||||||||||||
| throw new Error(`Failed to load resource: ${url}`); | ||||||||||||
|
|
@@ -976,6 +988,31 @@ export class McpContext implements Context { | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| #validateNetworkResource(url: URL): void { | ||||||||||||
| const urlString = url.href; | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed for this variable we can pass the |
||||||||||||
| for (const pattern of this.#blocklist) { | ||||||||||||
| if (pattern.test(urlString)) { | ||||||||||||
| throw new Error( | ||||||||||||
| `Access denied: URL ${urlString} is blocked by network blocklist.`, | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (!this.#allowlist) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| for (const pattern of this.#allowlist) { | ||||||||||||
| if (pattern.test(urlString)) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| throw new Error( | ||||||||||||
| `Access denied: URL ${urlString} is not allowed by network allowlist.`, | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| async getHeapSnapshotEdges( | ||||||||||||
| filePath: string, | ||||||||||||
| nodeId: number, | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,12 +216,30 @@ export const cliOptions = { | |
| 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; | ||
| } | ||
| for (const pattern of patterns) { | ||
| new URLPattern(String(pattern)); | ||
| } | ||
| return patterns.map(String); | ||
| }, | ||
| }, | ||
| allowedUrlPattern: { | ||
| type: 'array', | ||
| 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) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's extract this is a helper function - verifyUrlPatterns |
||
| if (!patterns) { | ||
| return; | ||
| } | ||
| for (const pattern of patterns) { | ||
| new URLPattern(String(pattern)); | ||
| } | ||
| return patterns.map(String); | ||
| }, | ||
| }, | ||
| ignoreDefaultChromeArg: { | ||
| type: 'array', | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,9 +18,12 @@ import {TextSnapshot} from '../src/TextSnapshot.js'; | |||||
| import type {HTTPResponse} from '../src/third_party/index.js'; | ||||||
| import type {TraceResult} from '../src/trace-processing/parse.js'; | ||||||
|
|
||||||
| import {serverHooks} from './server.js'; | ||||||
| import {getMockRequest, html, withMcpContext} from './utils.js'; | ||||||
|
|
||||||
| describe('McpContext', () => { | ||||||
| const server = serverHooks(); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this under |
||||||
|
|
||||||
| afterEach(() => { | ||||||
| sinon.restore(); | ||||||
| }); | ||||||
|
|
@@ -315,5 +318,73 @@ describe('McpContext', () => { | |||||
| } | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| it('http protocol respects the network blocklist', async () => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| server.addRoute('/blocked.map', (_req, res) => { | ||||||
| res.setHeader('Content-Type', 'application/json; charset=utf-8'); | ||||||
| res.end('{}'); | ||||||
| }); | ||||||
|
|
||||||
| const blockedUrl = server.getRoute('/blocked.map'); | ||||||
| await withMcpContext( | ||||||
| async (_response, context) => { | ||||||
| await assert.rejects( | ||||||
| context.loadResource(blockedUrl), | ||||||
| /blocked by network blocklist/, | ||||||
| ); | ||||||
| }, | ||||||
| { | ||||||
| blockedUrlPattern: [blockedUrl], | ||||||
| }, | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| it('http protocol respects the network allowlist', async () => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| server.addRoute('/allowed.map', (_req, res) => { | ||||||
| res.setHeader('Content-Type', 'application/json; charset=utf-8'); | ||||||
| res.end('{"version":3}'); | ||||||
| }); | ||||||
| server.addRoute('/blocked.map', (_req, res) => { | ||||||
| res.setHeader('Content-Type', 'application/json; charset=utf-8'); | ||||||
| res.end('{}'); | ||||||
| }); | ||||||
|
|
||||||
| const allowedUrl = server.getRoute('/allowed.map'); | ||||||
| const blockedUrl = server.getRoute('/blocked.map'); | ||||||
| await withMcpContext( | ||||||
| async (_response, context) => { | ||||||
| assert.strictEqual( | ||||||
| await context.loadResource(allowedUrl), | ||||||
| '{"version":3}', | ||||||
| ); | ||||||
| await assert.rejects( | ||||||
| context.loadResource(blockedUrl), | ||||||
| /not allowed by network allowlist/, | ||||||
| ); | ||||||
| }, | ||||||
| { | ||||||
| allowedUrlPattern: [allowedUrl], | ||||||
| }, | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| it('http protocol ignores an empty network allowlist', async () => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| server.addRoute('/source.map', (_req, res) => { | ||||||
| res.setHeader('Content-Type', 'application/json; charset=utf-8'); | ||||||
| res.end('{"version":3}'); | ||||||
| }); | ||||||
|
|
||||||
| await withMcpContext( | ||||||
| async (_response, context) => { | ||||||
| assert.strictEqual( | ||||||
| await context.loadResource(server.getRoute('/source.map')), | ||||||
| '{"version":3}', | ||||||
| ); | ||||||
| }, | ||||||
| { | ||||||
| allowedUrlPattern: [], | ||||||
| }, | ||||||
| ); | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,10 @@ | |
| import assert from 'node:assert'; | ||
| import {describe, it} from 'node:test'; | ||
|
|
||
| import {parseArguments} from '../src/bin/chrome-devtools-mcp-cli-options.js'; | ||
| import { | ||
| cliOptions, | ||
| parseArguments, | ||
| } from '../src/bin/chrome-devtools-mcp-cli-options.js'; | ||
|
|
||
| describe('cli args parsing', () => { | ||
| const defaultArgs = { | ||
|
|
@@ -391,6 +394,12 @@ describe('cli args parsing', () => { | |
| ]); | ||
| }); | ||
|
|
||
| it('rejects invalid blocked-url-pattern values', async () => { | ||
| assert.throws(() => | ||
| cliOptions.blockedUrlPattern.coerce(['https://[invalid']), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not test the function directly but use the |
||
| ); | ||
| }); | ||
|
|
||
| it('parses allowed-url-pattern flags as array', async () => { | ||
| const defaultArgs = parseArguments('1.0.0', ['node', 'main.js']); | ||
| assert.strictEqual(defaultArgs.allowedUrlPattern, undefined); | ||
|
|
@@ -435,4 +444,10 @@ describe('cli args parsing', () => { | |
| 'https://b.com/*', | ||
| ]); | ||
| }); | ||
|
|
||
| it('rejects invalid allowed-url-pattern values', async () => { | ||
| assert.throws(() => | ||
| cliOptions.allowedUrlPattern.coerce(['https://[invalid']), | ||
| ); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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