diff --git a/src/config/browser-options.ts b/src/config/browser-options.ts index 2b9af492d..d824301d3 100644 --- a/src/config/browser-options.ts +++ b/src/config/browser-options.ts @@ -91,7 +91,7 @@ export const browserOptions = { type: 'boolean', description: 'If specified, creates a temporary user-data-dir that is automatically cleaned up after the browser is closed. Defaults to false.', - defaultDescription: 'false', + default: false, }, userDataDir: { type: 'string', @@ -103,7 +103,7 @@ export const browserOptions = { description: 'Specify a different Chrome channel that should be used. The default is the stable channel version.', choices: ['canary', 'dev', 'beta', 'stable'] as const, - defaultDescription: 'stable', + default: 'stable' as const, }, proxyServer: { type: 'string', diff --git a/src/config/mcp-options.ts b/src/config/mcp-options.ts index 292bc7ad0..90be59606 100644 --- a/src/config/mcp-options.ts +++ b/src/config/mcp-options.ts @@ -5,7 +5,7 @@ */ import type {YargsOptions} from '../third_party/index.js'; -import {yargs, hideBin} from '../third_party/index.js'; +import {yargs, hideBin, zod as z} from '../third_party/index.js'; import os from 'node:os'; import {readFileSync} from 'node:fs'; import path from 'node:path'; @@ -144,7 +144,7 @@ export const mcpOptions = { describe: "Restricts browser's 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. A pattern that uses a regexp group in any component (for example `(127\\.\\d+\\.\\d+\\.\\d+)` in the hostname) is rejected, because it is not enforced on redirects or subresources; use an exact value or a `*`/`:name` wildcard instead.", coerce: (arg: string[] | undefined) => { - if (arg === undefined) { + if (arg === undefined || arg.length === 0) { return undefined; } const pattern = findUnenforceablePattern(arg); @@ -165,6 +165,11 @@ export const mcpOptions = { if (arg === undefined) { return undefined; } + if (arg.length === 0) { + throw new Error( + 'Invalid --allowedUrlPattern: at least one pattern is required.', + ); + } const pattern = findUnenforceablePattern(arg); if (pattern) { throw new Error( @@ -319,9 +324,14 @@ export const mcpOptions = { }, } satisfies Record; -export type ParsedArguments = ReturnType; +export type ParsedArguments = ReturnType< + ReturnType['parseSync'] +>; -export function getMcpOptionsForViaCli(): typeof mcpOptions { +export function getMcpOptionsForViaCli(): Record< + keyof typeof mcpOptions, + YargsOptions +> { if (!('default' in mcpOptions.headless)) { throw new Error('headless cli option unexpectedly does not have a default'); } @@ -473,18 +483,13 @@ function getErrorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } -/** - * Exported only for testing to not trigger process exit. - */ -export function parser( +function buildCliParser>( version: string, - argv = process.argv, - env = process.env, + argv: string[], + options: T, ) { - const isViaCli = argv.includes('--viaCli') || argv.includes('--via-cli'); - const options = isViaCli ? getMcpOptionsForViaCli() : mcpOptions; - - const yargsInstance = yargs(hideBin(argv)) + const yargsInstance = yargs(hideBin(argv)); + return yargsInstance .scriptName('npx chrome-devtools-mcp@latest') .parserConfiguration({ 'strip-aliased': true, @@ -492,126 +497,168 @@ export function parser( }) .options(options) .showHelpOnFail(false, 'Specify --help for available options') - .check(args => { - const activeArgs = new Set(); + .example(CLI_EXAMPLES) + .wrap(Math.min(120, yargsInstance.terminalWidth())) + .help() + .version(version); +} - for (const [key, val] of Object.entries(args)) { - if (val !== undefined && val !== false) { - activeArgs.add(key); - } - } +export function parser(version: string, argv = process.argv) { + // Used to derive the ParsedArguments type. + return buildCliParser(version, argv, mcpOptions); +} - for (const group of CONFLICTING_ARGS) { - // Find all active arguments within this conflict group - const activeInGroup = group.filter(arg => activeArgs.has(arg)); +export function parseArguments( + version: string, + argv = process.argv, + env = process.env, +) { + // Step 1 & 2: Independent Parsing (Bypassing yargs Defaults) + const optionsWithoutDefaults: Record = {}; + for (const [key, option] of Object.entries(mcpOptions)) { + const copy: YargsOptions = {...option}; + if (copy.default !== undefined) { + copy.defaultDescription ??= JSON.stringify(copy.default); + delete copy.default; + } + optionsWithoutDefaults[key] = copy; + } - if (activeInGroup.length > 1) { - const [arg1, arg2] = activeInGroup; - throw new Error( - `Arguments ${arg1} and ${arg2} are mutually exclusive`, - ); - } - } + // `--help` and `--version` print and exit the process; parse errors throw. + const yargsInstance = buildCliParser( + version, + argv, + optionsWithoutDefaults, + ).fail(false); - return true; - }) - .middleware(args => { - if (isViaCli) { - if (args.filesystemRoot === DEFAULT_FILESYSTEM_ROOT) { - const cliFilesystemArgs: { - allowUnrestrictedPaths?: boolean; - filesystemRoot?: unknown; - } = args; - cliFilesystemArgs.allowUnrestrictedPaths = true; - cliFilesystemArgs.filesystemRoot = undefined; - } - // Defaults that cannot be set in options without affecting yargs conflict resolution. - const connectsToExistingBrowser = - args.autoConnect || args.browserUrl || args.wsEndpoint; - if ( - args.isolated === undefined && - args.userDataDir === undefined && - !connectsToExistingBrowser - ) { - args.isolated = true; - } - if ( - args.categoryExtensions === undefined && - !connectsToExistingBrowser - ) { - args.categoryExtensions = true; - } - } - // Only fall back to stable when Chrome is launched by channel. Leaving it - // unset otherwise keeps it out of telemetry (computeFlagUsage) for - // browserUrl, wsEndpoint and executablePath. - if ( - !args.channel && - !args.browserUrl && - !args.wsEndpoint && - !args.executablePath - ) { - args.channel = 'stable'; - } - if (env['CI'] || env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']) { - console.error( - "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.", - ); - args.usageStatistics = false; + const rawCli: Record = yargsInstance.parseSync(); + + // Config file parsing + let parsedConfigFile: Record = {}; + if (typeof rawCli.config === 'string') { + try { + const fileContent: unknown = JSON.parse( + readFileSync(rawCli.config, 'utf-8'), + ); + if (!isPlainObject(fileContent)) { + throw new Error('Config must be a JSON object'); } - const cliOptionsAllowedArgs = [ - ...Object.keys(options), - // Yargs populated with positional args - '_', - '$0', - ]; + // Run the config through yargs to reject unknown keys and apply + // coercions, but without defaults. + parsedConfigFile = yargs([]) + .parserConfiguration({ + 'strip-aliased': true, + 'camel-case-expansion': false, + }) + .options(optionsWithoutDefaults) + .config(fileContent) + .strict() + .fail(false) + .exitProcess(false) + .parseSync([]); + } catch (err) { + throw new Error(`Invalid JSON config file: ${getErrorMessage(err)}`); + } + } - const unknownArgs = Object.keys(args).filter( - arg => !cliOptionsAllowedArgs.includes(arg), - ); + // Step 3: Cascade Merge of Explicit Inputs + const explicitConfig = {...parsedConfigFile, ...rawCli}; - if (unknownArgs.length > 0) { - console.error( - `Unknown arguments: ${unknownArgs.map(arg => `--${arg}`)}`, - ); - } - }) - .example(CLI_EXAMPLES); + // Step 4: Dynamic Default Resolution. `viaCli` is an explicit input like any + // other; it selects which defaults apply. + const isViaCli = explicitConfig.viaCli === true; + const baseOptions = isViaCli ? getMcpOptionsForViaCli() : mcpOptions; + const resolvedConfig = {...explicitConfig}; + // `channel` only applies when Chrome is launched by channel. Leaving it + // unset otherwise keeps it out of telemetry (computeFlagUsage). + const launchesByChannel = + !explicitConfig.browserUrl && + !explicitConfig.wsEndpoint && + !explicitConfig.executablePath; - return yargsInstance - .config('config', 'Path to JSON configuration file', configPath => { - try { - const parsed: unknown = JSON.parse(readFileSync(configPath, 'utf-8')); - if (!isPlainObject(parsed)) { - throw new Error('Config must be a JSON object'); + for (const [key, option] of Object.entries(baseOptions)) { + if (key === 'channel' && !launchesByChannel) { + continue; + } + if (resolvedConfig[key] === undefined && 'default' in option) { + resolvedConfig[key] = option.default; + } + } + + if (isViaCli) { + if (resolvedConfig.filesystemRoot === DEFAULT_FILESYSTEM_ROOT) { + resolvedConfig.allowUnrestrictedPaths = true; + resolvedConfig.filesystemRoot = undefined; + } + const connectsToExistingBrowser = + resolvedConfig.autoConnect || + resolvedConfig.browserUrl || + resolvedConfig.wsEndpoint; + if ( + explicitConfig.isolated === undefined && + resolvedConfig.userDataDir === undefined && + !connectsToExistingBrowser + ) { + resolvedConfig.isolated = true; + } + if ( + resolvedConfig.categoryExtensions === undefined && + !connectsToExistingBrowser + ) { + resolvedConfig.categoryExtensions = true; + } + } + + if (env['CI'] || env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']) { + console.error( + "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.", + ); + resolvedConfig.usageStatistics = false; + } + + const cliOptionsAllowedArgs = [...Object.keys(baseOptions), '_', '$0']; + + const unknownArgs = Object.keys(rawCli).filter( + arg => !cliOptionsAllowedArgs.includes(arg), + ); + + if (unknownArgs.length > 0) { + console.error(`Unknown arguments: ${unknownArgs.map(arg => `--${arg}`)}`); + } + + // Step 5: Final Schema Validation & Type Safety. Conflicts are only checked + // against explicit user input so that dynamic defaults never conflict. + const ConfigSchema = z + .object({}) + .passthrough() + .superRefine((_config, ctx) => { + const activeArgs = new Set(); + for (const [key, val] of Object.entries(explicitConfig)) { + if (val !== undefined && val !== false) { + activeArgs.add(key); } + } - yargs() - .parserConfiguration({ - 'strip-aliased': true, - 'camel-case-expansion': false, - }) - .options(options) - .config(parsed) - .strict() - .fail(false) - .exitProcess(false) - .parseSync([]); - return parsed; - } catch (err) { - throw new Error(`Invalid JSON config file: ${getErrorMessage(err)}`); + for (const group of CONFLICTING_ARGS) { + const activeInGroup = group.filter(arg => activeArgs.has(arg)); + if (activeInGroup.length > 1) { + const [arg1, arg2] = activeInGroup; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Arguments ${arg1} and ${arg2} are mutually exclusive`, + path: [arg1, arg2], + }); + } } - }) - .wrap(Math.min(120, yargsInstance.terminalWidth())) - .help() - .version(version); -} + }); -export function parseArguments( - version: string, - argv = process.argv, - env = process.env, -) { - return parser(version, argv, env).parseSync(); + const result = ConfigSchema.safeParse(resolvedConfig); + if (!result.success) { + throw new Error(result.error.issues[0].message); + } + // The merge and the default loop lose the static type that yargs infers from + // `mcpOptions`. Every value was produced by the same option definitions (CLI + // parser, strict config-file parser, option defaults), so the shape matches. + return result.data as ParsedArguments; } diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 7d63b7bf4..1be9fdaf2 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -14,6 +14,7 @@ import { DEFAULT_FILESYSTEM_ROOT, getCliOptions, mcpOptions, + parseArguments as parseArgumentsImpl, parser, } from '../src/config/mcp-options.js'; import {computeFlagUsage} from '../src/telemetry/flagUtils.js'; @@ -21,9 +22,7 @@ import {computeFlagUsage} from '../src/telemetry/flagUtils.js'; import {createTempFile} from './utils.js'; function parseArguments(argv: string[], env: NodeJS.ProcessEnv = {}) { - return parser('0.0.0', ['node', 'main.js', ...argv], env) - .exitProcess(false) - .parseSync(); + return parseArgumentsImpl('0.0.0', ['node', 'main.js', ...argv], env); } describe('cli args parsing', () => { @@ -37,6 +36,7 @@ describe('cli args parsing', () => { categoryMemory: true, autoConnect: false, headless: false, + isolated: false, acceptInsecureCerts: false, performanceCrux: true, usageStatistics: true, @@ -470,6 +470,44 @@ describe('cli args parsing', () => { ); }); + it('strips an empty blocked-url-pattern', async () => { + const args = parseArguments(['--blocked-url-pattern']); + assert.strictEqual(args.blockedUrlPattern, undefined); + }); + + it('allows an empty config blockedUrlPattern with allowed-url-pattern', async () => { + using testConfig = createTempFile( + JSON.stringify({blockedUrlPattern: []}), + 'cd4a.test.config.empty-blocked.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--allowed-url-pattern', + 'https://a.com/*', + ]); + assert.strictEqual(args.blockedUrlPattern, undefined); + assert.deepStrictEqual(args.allowedUrlPattern, ['https://a.com/*']); + }); + + it('rejects an empty allowed-url-pattern', async () => { + assert.throws( + () => parseArguments(['--allowed-url-pattern']), + /Invalid --allowedUrlPattern: at least one pattern is required/, + ); + }); + + it('rejects an empty config allowedUrlPattern', async () => { + using testConfig = createTempFile( + JSON.stringify({allowedUrlPattern: []}), + 'cd4a.test.config.empty-allowed.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file: Invalid --allowedUrlPattern: at least one pattern is required/, + ); + }); + it('parses source-maps flag', async () => { const defaultParsed = parseArguments(['main.js']); assert.strictEqual(defaultParsed.sourceMaps, true); @@ -561,7 +599,7 @@ describe('cli args parsing', () => { const args = parseArguments(['--viaCli', '--config', relativePath]); assert.strictEqual(args.config, testConfig.path); assert.strictEqual(args.userDataDir, '/tmp/custom-profile'); - assert.strictEqual(args.isolated, undefined); + assert.strictEqual(args.isolated, false); assert.strictEqual(args.headless, false); }); @@ -604,11 +642,141 @@ describe('cli args parsing', () => { ); }); + it('rejects a config file with malformed JSON', async () => { + using testConfig = createTempFile( + '{"headless": true,', + 'cd4a.test.config.malformed.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file:/, + ); + }); + + it('rejects a config file that is not a JSON object', async () => { + using testConfig = createTempFile( + JSON.stringify(['--headless']), + 'cd4a.test.config.array.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file: Config must be a JSON object/, + ); + }); + + it('rejects a missing config file', async () => { + assert.throws( + () => parseArguments(['--config', 'cd4a.test.config.missing.json']), + /Invalid JSON config file: ENOENT/, + ); + }); + + it('replaces config arrays with cli arrays instead of merging', async () => { + using testConfig = createTempFile( + JSON.stringify({chromeArg: ['--a']}), + 'cd4a.test.config.array-replace.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--chrome-arg=--b', + ]); + assert.deepStrictEqual(args.chromeArg, ['--b']); + }); + + it('lets the CI env disable usage statistics enabled in config', async () => { + using testConfig = createTempFile( + JSON.stringify({usageStatistics: true}), + 'cd4a.test.config.usage-statistics.json', + ); + const args = parseArguments(['--config', testConfig.path], {CI: 'true'}); + assert.strictEqual(args.usageStatistics, false); + }); + + it('lets explicit cli flags override viaCli dynamic defaults', async () => { + const args = parseArguments(['--viaCli', '--no-headless']); + assert.strictEqual(args.headless, false); + }); + + describe('viaCli defaults', () => { + for (const flag of ['--viaCli=true', '--via-cli=true']) { + it(`applies viaCli defaults for ${flag}`, async () => { + const args = parseArguments([flag]); + assert.strictEqual(args.viaCli, true); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, true); + assert.strictEqual(args.memoryDebugging, true); + }); + } + + it('does not apply viaCli defaults for --viaCli false', async () => { + const args = parseArguments(['--viaCli', 'false']); + assert.strictEqual(args.viaCli, false); + assert.strictEqual(args.headless, false); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.memoryDebugging, false); + }); + + it('applies viaCli defaults when viaCli is set in the config file', async () => { + using testConfig = createTempFile( + JSON.stringify({viaCli: true}), + 'cd4a.test.config.via-cli.json', + ); + const args = parseArguments(['--config', testConfig.path]); + assert.strictEqual(args.viaCli, true); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, true); + }); + + it('lets cli viaCli=false override config viaCli', async () => { + using testConfig = createTempFile( + JSON.stringify({viaCli: true}), + 'cd4a.test.config.via-cli-override.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--viaCli=false', + ]); + assert.strictEqual(args.viaCli, false); + assert.strictEqual(args.headless, false); + assert.strictEqual(args.isolated, false); + }); + + for (const connectArgs of [ + ['--auto-connect'], + ['--browserUrl', 'http://localhost:9222'], + ['--wsEndpoint', 'ws://localhost:9222'], + ['--user-data-dir', '/tmp/chrome-profile'], + ]) { + it(`does not default to isolated with ${connectArgs[0]}`, async () => { + const args = parseArguments(['--viaCli', ...connectArgs]); + assert.strictEqual(args.isolated, false); + }); + } + + it('does not default to isolated with autoConnect from the config file', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: true}), + 'cd4a.test.config.via-cli-auto-connect.json', + ); + const args = parseArguments(['--viaCli', '--config', testConfig.path]); + assert.strictEqual(args.autoConnect, true); + assert.strictEqual(args.isolated, false); + }); + }); + it('parses with devtoolsComments enabled', async () => { const args = parseArguments(['--devtoolsComments']); assert.strictEqual(args.devtoolsComments, true); }); + it('includes usage examples in help output', async () => { + const help = await parser('0.0.0', ['node', 'main.js']).getHelp(); + assert.match(help, /Examples:/); + assert.match(help, /--browserUrl http:\/\/127\.0\.0\.1:9222/); + }); + it('clears default values and populates defaultDescription in getCliOptions', () => { const cliOptions = getCliOptions(); @@ -832,6 +1000,73 @@ describe('cli args parsing', () => { ); }); + it('rejects cli channel with config-based browserUrl', async () => { + using testConfig = createTempFile( + JSON.stringify({browserUrl: 'http://localhost:9222'}), + 'cd4a.test.config.browser-url.json', + ); + assert.throws( + () => + parseArguments(['--config', testConfig.path, '--channel', 'canary']), + /Arguments channel and browserUrl are mutually exclusive/, + ); + }); + + it('rejects conflicting arguments within a config file', async () => { + using testConfig = createTempFile( + JSON.stringify({ + browserUrl: 'http://localhost:9222', + wsEndpoint: 'ws://localhost:9222', + }), + 'cd4a.test.config.conflict.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Arguments browserUrl and wsEndpoint are mutually exclusive/, + ); + }); + + it('allows explicitly disabled isolated with userDataDir', async () => { + const args = parseArguments([ + '--isolated=false', + '--user-data-dir', + '/tmp/chrome-profile', + ]); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.userDataDir, '/tmp/chrome-profile'); + }); + + it('allows a config value set to false alongside a conflicting arg', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: false}), + 'cd4a.test.config.false-no-conflict.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--executablePath', + '/bin/chrome', + ]); + assert.strictEqual(args.autoConnect, false); + assert.strictEqual(args.executablePath, '/bin/chrome'); + }); + + it('lets a cli false override a conflicting config value', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: true}), + 'cd4a.test.config.false-override.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--autoConnect=false', + '--executablePath', + '/bin/chrome', + ]); + assert.strictEqual(args.autoConnect, false); + assert.strictEqual(args.executablePath, '/bin/chrome'); + }); + it('rejects categoryExtensions with autoConnect', async () => { assert.throws( () => parseArguments(['--category-extensions', '--auto-connect']), @@ -882,7 +1117,7 @@ describe('cli args parsing', () => { it(`allows viaCli with ${connectArgs[0]} without enabling extensions`, async () => { const args = parseArguments(['--viaCli', ...connectArgs]); assert.strictEqual(args.categoryExtensions, undefined); - assert.strictEqual(args.isolated, undefined); + assert.strictEqual(args.isolated, false); }); } @@ -975,8 +1210,8 @@ describe('cli command strings', () => { describe('flag usage telemetry', () => { it('reports the stable channel for a default launch', async () => { const usage = computeFlagUsage(parseArguments([]), mcpOptions); - assert.strictEqual(usage.isolated_present, false); - assert.strictEqual(usage.channel_present, true); + assert.strictEqual(usage.isolated_present, undefined); + assert.strictEqual(usage.channel_present, undefined); assert.strictEqual(usage.channel, 'CHANNEL_STABLE'); }); @@ -987,7 +1222,7 @@ describe('flag usage telemetry', () => { ]) { it(`does not report channel with ${connectArgs[0]}`, async () => { const usage = computeFlagUsage(parseArguments(connectArgs), mcpOptions); - assert.strictEqual(usage.isolated_present, false); + assert.strictEqual(usage.isolated_present, undefined); assert.strictEqual(usage.channel_present, false); assert.strictEqual(usage.channel, undefined); }); diff --git a/tests/mocks.ts b/tests/mocks.ts index 08b97ac86..3a332c5ba 100644 --- a/tests/mocks.ts +++ b/tests/mocks.ts @@ -25,7 +25,10 @@ import type {Frame} from 'puppeteer-core'; import sinon from 'sinon'; -import {type ParsedArguments, parser} from '../src/config/mcp-options.js'; +import { + type ParsedArguments, + parseArguments, +} from '../src/config/mcp-options.js'; import {McpContext} from '../src/McpContext.js'; import {McpPage} from '../src/McpPage.js'; import {McpResponse} from '../src/McpResponse.js'; @@ -972,7 +975,7 @@ export function createMockContextAnalysisResult(): DevTools.HeapSnapshotModel.He export function createMockParsedArguments( options: Partial = {}, ): ParsedArguments { - const defaultArgs = parser('0.0.0', ['node', 'main.js']).parseSync(); + const defaultArgs = parseArguments('0.0.0', ['node', 'main.js']); return {...defaultArgs, ...options}; }