From 219b32be14b91171067e334ea7fabbe78f93ebfd Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Sun, 7 Jul 2024 21:12:55 -0400 Subject: [PATCH 01/13] feat: Allow port option when launching Chrome --- src/cmd/run.js | 2 ++ src/extension-runners/chromium.js | 7 +++++++ tests/unit/test-extension-runners/test.chromium.js | 14 ++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/src/cmd/run.js b/src/cmd/run.js index be640c8407..516073ccf9 100644 --- a/src/cmd/run.js +++ b/src/cmd/run.js @@ -48,6 +48,7 @@ export default async function run( firefoxApkComponent, // Chromium CLI options. chromiumBinary, + chromiumPort, chromiumProfile, }, { @@ -186,6 +187,7 @@ export default async function run( const chromiumRunnerParams = { ...commonRunnerParams, chromiumBinary, + chromiumPort, chromiumProfile, }; diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index f6337fc181..a49acf9b6a 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -202,6 +202,12 @@ export class ChromiumExtensionRunner { chromeFlags.push(...startingUrls); } + let port; + if(this.params.chromiumPort && !isNaN(this.params.chromiumPort)) { + port = this.params.chromiumPort; + log.debug(`(port: ${port})`); + } + this.chromiumInstance = await this.chromiumLaunch({ enableExtensions: true, chromePath: chromiumBinary, @@ -210,6 +216,7 @@ export class ChromiumExtensionRunner { userDataDir, // Ignore default flags to keep the extension enabled. ignoreDefaultFlags: true, + port, }); this.chromiumInstance.process.once('close', () => { diff --git a/tests/unit/test-extension-runners/test.chromium.js b/tests/unit/test-extension-runners/test.chromium.js index 488db24490..45add2563d 100644 --- a/tests/unit/test-extension-runners/test.chromium.js +++ b/tests/unit/test-extension-runners/test.chromium.js @@ -615,6 +615,20 @@ describe('util/extension-runners/chromium', async () => { }), ); + it('does pass port to chrome', async () => { + const port = 9222; + const { params } = prepareExtensionRunnerParams({ + chromiumPort: port, + }); + + sinon.assert(calledOnce(params.chromiumLaunch)); + sinon.assert(calledWithMatch(params.chromiumLaunch, { + port, + })); + + await runnerInstance.exit(); + }); + describe('reloadAllExtensions', () => { let runnerInstance; let wsClient; From 01d737c67d2ce045dc8b8e420945b91ba83024ed Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Sun, 7 Jul 2024 21:30:35 -0400 Subject: [PATCH 02/13] Fixed code styling --- src/extension-runners/chromium.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index a49acf9b6a..b7a47fe749 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -203,7 +203,7 @@ export class ChromiumExtensionRunner { } let port; - if(this.params.chromiumPort && !isNaN(this.params.chromiumPort)) { + if (this.params.chromiumPort && !isNaN(this.params.chromiumPort)) { port = this.params.chromiumPort; log.debug(`(port: ${port})`); } From da9dd76ddaa0e3b1f89db8e8b3b9dc31a04815a4 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Sun, 7 Jul 2024 21:30:45 -0400 Subject: [PATCH 03/13] Fixed tester --- tests/unit/test-extension-runners/test.chromium.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/unit/test-extension-runners/test.chromium.js b/tests/unit/test-extension-runners/test.chromium.js index 45add2563d..63fdb5bed2 100644 --- a/tests/unit/test-extension-runners/test.chromium.js +++ b/tests/unit/test-extension-runners/test.chromium.js @@ -618,13 +618,16 @@ describe('util/extension-runners/chromium', async () => { it('does pass port to chrome', async () => { const port = 9222; const { params } = prepareExtensionRunnerParams({ - chromiumPort: port, + params: { chromiumPort: port }, }); - sinon.assert(calledOnce(params.chromiumLaunch)); - sinon.assert(calledWithMatch(params.chromiumLaunch, { + const runnerInstance = new ChromiumExtensionRunner(params); + await runnerInstance.run(); + + sinon.assert.calledOnce(params.chromiumLaunch); + sinon.assert.calledWithMatch(params.chromiumLaunch, { port, - })); + }); await runnerInstance.exit(); }); From 8b9a5e49f6d396ef8ba12c1b42dbd61531e89038 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Wed, 10 Jul 2024 15:23:32 -0400 Subject: [PATCH 04/13] feat: add new Errors --- src/errors.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/errors.js b/src/errors.js index 0a25d05fbe..c92ca053d1 100644 --- a/src/errors.js +++ b/src/errors.js @@ -54,6 +54,22 @@ export class MultiExtensionsReloadError extends WebExtError { } } +export class UnusablePortError extends WebExtError { + constructor(message) { + super(message); + } +} +export class PortInUseError extends UnusablePortError { + constructor(message = 'The requested port is in use by another process') { + super(message); + } +} +export class PortVerificationFailedError extends UnusablePortError { + constructor(message = 'The requested port cannot be used.') { + super(message); + } +} + /* * Sugar-y way to catch only instances of a certain error. * From 9eed3b691c0d8eaa09b858a960979e828042400e Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Wed, 10 Jul 2024 15:24:46 -0400 Subject: [PATCH 05/13] feat: add port validation --- src/extension-runners/chromium.js | 31 ++++- src/util/verify-chromium-port.js | 217 ++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 7 deletions(-) create mode 100644 src/util/verify-chromium-port.js diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index b7a47fe749..92a81da860 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -17,6 +17,8 @@ import { createLogger } from '../util/logger.js'; import { TempDir } from '../util/temp-dir.js'; import isDirectory from '../util/is-directory.js'; import fileExists from '../util/file-exists.js'; +import { validatePort } from '../util/verify-chromium-port.js'; +import { PortInUseError, PortVerificationFailedError } from '../errors.js'; const log = createLogger(import.meta.url); @@ -202,13 +204,25 @@ export class ChromiumExtensionRunner { chromeFlags.push(...startingUrls); } - let port; - if (this.params.chromiumPort && !isNaN(this.params.chromiumPort)) { - port = this.params.chromiumPort; - log.debug(`(port: ${port})`); + const { chromiumPort } = this.params; + if (chromiumPort) { + log.debug('(port: %d)', chromiumPort); + + const isPortUsable = await validatePort( + this.params.chromiumPort, + chromiumBinary, + chromeFlags, + ).catch((error) => { + log.error('Port verification failed: %s', error.message); + throw new PortVerificationFailedError(); + }); + + if (!isPortUsable) { + throw new PortInUseError(); + } } - this.chromiumInstance = await this.chromiumLaunch({ + const chromiumConfig = { enableExtensions: true, chromePath: chromiumBinary, chromeFlags, @@ -216,8 +230,11 @@ export class ChromiumExtensionRunner { userDataDir, // Ignore default flags to keep the extension enabled. ignoreDefaultFlags: true, - port, - }); + port: chromiumPort, + }; + + log.debug('(config: %O)', chromiumConfig); + this.chromiumInstance = await this.chromiumLaunch(chromiumConfig); this.chromiumInstance.process.once('close', () => { this.chromiumInstance = null; diff --git a/src/util/verify-chromium-port.js b/src/util/verify-chromium-port.js new file mode 100644 index 0000000000..9e16279856 --- /dev/null +++ b/src/util/verify-chromium-port.js @@ -0,0 +1,217 @@ +import { createServer } from 'node:http'; +import { exec } from 'node:child_process'; + +import { createLogger } from './logger.js'; +import { UnusablePortError, UsageError } from '../errors.js'; + +const log = createLogger(import.meta.url); + +/** + * Returns a random, available port + * @returns {number} Available port number to use; + */ +export async function getRandomPort() { + return new Promise((resolve, reject) => { + const server = createServer(); + server.once('listening', () => { + const { port } = server.address(); + server.close(() => resolve(port)); + }); + server.once('error', () => reject); + server.listen(0); + }); +} + +/** + * Search for existing process + * @param {string} chromeBinary + * @returns {Promise} Process list output + */ +async function findProcess(chromeBinary) { + const platform = process.platform; + let cmd = ''; + + switch (platform) { + case 'freebsd': + // no dash + cmd = 'ps axo cmd'; + break; + case 'darwin': + cmd = 'ps -axo cmd'; + break; + case 'linux': + // dash + cmd = 'ps -Ao cmd'; + cmd = `${cmd} | grep ${chromeBinary}`; + break; + case 'win32': + cmd = `powershell "Get-CimInstance Win32_Process -Filter \\"Name -Like '${chromeBinary}'\\" | Select CommandLine"`; + break; + default: + throw new UsageError('Unsupported platform'); + } + + // log.info('Searching for process %s', chromeBinary); + // log.debug('(cmd: %s)', cmd); + + const promise = new Promise((resolve, reject) => { + exec(cmd, (error, stdout, stderr) => { + // log.debug('Output: %s', stdout); + if (stderr) { + log.debug('Error: %s', stderr); + } + + // const isMatch = + // stdout.toLowerCase().indexOf(chromeBinary.toLowerCase()) > -1; + + if (error) { + reject(error); + } else { + // log.debug('Found at least one instance of %s', chromeBinary); + // log.debug('(result: %o)', isMatch); + resolve(stdout); + } + }); + }); + + return promise; +} + +/** + * Inspect process list output and match against specified port and extension + * @param {number} port User-requested port + * @param {string} extension Web-ext temporary extension + * @param {string} output Output from OS-specific process list containing process command line + * @returns {Promise} Whether an eligible instance was found + */ +async function inspectProcessList(port, extension, output) { + if (!output) { + log.info('Browser instance not found'); + return false; + } + const lines = output.split('\n'); + let foundEligibleInstance = false; + + // log.debug('Found %d browser instance(s). Inspecting...', lines.length); + + lines.forEach((line) => { + const extensionMatch = `--load-extension=${extension}`; + const portMatch = `--remote-debugging-port=${port}`; + + const isPortMatch = line.toLowerCase().indexOf(portMatch) > -1; + const isExtension = line.indexOf(extensionMatch) > -1; + + if (!isPortMatch) { + // log.debug('[%d/%d] Not using target port', i, lines.length, port); + return; + } + + // log.debug('[%d/%d] Found chromium instance', i, lines.length); + + if (!isExtension) { + // throw new UnusablePortError( + // 'Port is in use by another browser instance that does not have this extension loaded', + // ); + // log.debug('[%d/%d] Extension not loaded', i, lines.length); + return; + } + + // log.debug( + // '[%d/%d] Found extension loaded in this instance', + // i, + // lines.length, + // ); + foundEligibleInstance = true; + }); + + // log.debug('Inspection complete. Result: %o', foundEligibleInstance); + return foundEligibleInstance; +} + +/** + * Determine if a port is available + * @param {number} port Port to test + * */ +export async function portAvailable(port) { + return new Promise((resolve) => { + const server = createServer(); + server.once('listening', () => { + server.close(() => resolve(true)); + }); + server.once('error', () => resolve(false)); + server.listen(port); + }); +} + +/** + * Validate user-supplied port to ensure it is suitable for use + * @param {number} port User-supplied port request + * @param {string} chromeBinary Chromium binary being requested for launch + * @param {string[]} chromeFlags Array of flags requested for launch + * @returns {boolean} Whether requested port is usable + */ +export async function validatePort(port, chromeBinary, chromeFlags) { + if (!port) { + return; + } + if (isNaN(port)) { + throw new UnusablePortError(`Non-numeric port provided (${port})`); + } + if (port < 0 || port > 65535) { + throw new UnusablePortError(`Invalid port number: ${port}`); + } + + const isAvailable = await portAvailable(port); + // const flags = new Map(); + const extensions = chromeFlags.find( + (flag) => flag.toLowerCase().indexOf('--load-extension') > -1, + ); + if (!extensions.length || !extensions.length > 1) { + // This shouldn't happen... + throw new UnusablePortError( + 'Port is in use and verification of whether the extension is loaded failed', + ); + } + + const extension = extensions[0].substring(extensions[0].indexOf('=') + 1); + + if (!extension) { + // This also shouldn't happen... + throw new UnusablePortError( + 'Port is in use and verification of whether the extension is loaded failed', + ); + } + // chromeFlags.forEach((flag) => { + // const valueAt = flag.indexOf('='); + // const [key, value] = + // valueAt === -1 + // ? [flag, undefined] + // : [flag.substring(0, valueAt), flag.substring(valueAt + 1)]; + + // // let key; + // // let value; + // // if (valueAt === -1) { + // // key = flag; + // // log.debug('flag: [%s]', key); + // // } else { + // // key = flag.substring(0, valueAt); + // // value = flag.substring(valueAt + 1); + // // log.debug('flag: [%s] => %s', key, value); + // // } + // flags.set(key, value); + // }); + + // const extension = flags.get('--load-extension'); + + if (isAvailable) { + // log.debug('Using debugging port: %d', port); + return true; + } + + return findProcess(chromeBinary, port) + .then((ps) => inspectProcessList(port, extension, ps)) + .catch((error) => { + log.error(error); + throw new UnusablePortError(`Unable to validate port: ${error}`); + }); +} From 53554357ec559e93f395068aa43eb492f432da8f Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 19:20:01 -0400 Subject: [PATCH 06/13] cleanup --- src/util/verify-chromium-port.js | 49 +------------------------------- 1 file changed, 1 insertion(+), 48 deletions(-) diff --git a/src/util/verify-chromium-port.js b/src/util/verify-chromium-port.js index 9e16279856..4128a56a4a 100644 --- a/src/util/verify-chromium-port.js +++ b/src/util/verify-chromium-port.js @@ -51,24 +51,15 @@ async function findProcess(chromeBinary) { throw new UsageError('Unsupported platform'); } - // log.info('Searching for process %s', chromeBinary); - // log.debug('(cmd: %s)', cmd); - const promise = new Promise((resolve, reject) => { exec(cmd, (error, stdout, stderr) => { - // log.debug('Output: %s', stdout); if (stderr) { log.debug('Error: %s', stderr); } - // const isMatch = - // stdout.toLowerCase().indexOf(chromeBinary.toLowerCase()) > -1; - if (error) { reject(error); } else { - // log.debug('Found at least one instance of %s', chromeBinary); - // log.debug('(result: %o)', isMatch); resolve(stdout); } }); @@ -92,8 +83,6 @@ async function inspectProcessList(port, extension, output) { const lines = output.split('\n'); let foundEligibleInstance = false; - // log.debug('Found %d browser instance(s). Inspecting...', lines.length); - lines.forEach((line) => { const extensionMatch = `--load-extension=${extension}`; const portMatch = `--remote-debugging-port=${port}`; @@ -102,29 +91,16 @@ async function inspectProcessList(port, extension, output) { const isExtension = line.indexOf(extensionMatch) > -1; if (!isPortMatch) { - // log.debug('[%d/%d] Not using target port', i, lines.length, port); return; } - // log.debug('[%d/%d] Found chromium instance', i, lines.length); - if (!isExtension) { - // throw new UnusablePortError( - // 'Port is in use by another browser instance that does not have this extension loaded', - // ); - // log.debug('[%d/%d] Extension not loaded', i, lines.length); return; } - // log.debug( - // '[%d/%d] Found extension loaded in this instance', - // i, - // lines.length, - // ); foundEligibleInstance = true; }); - // log.debug('Inspection complete. Result: %o', foundEligibleInstance); return foundEligibleInstance; } @@ -152,7 +128,7 @@ export async function portAvailable(port) { */ export async function validatePort(port, chromeBinary, chromeFlags) { if (!port) { - return; + return false; } if (isNaN(port)) { throw new UnusablePortError(`Non-numeric port provided (${port})`); @@ -162,7 +138,6 @@ export async function validatePort(port, chromeBinary, chromeFlags) { } const isAvailable = await portAvailable(port); - // const flags = new Map(); const extensions = chromeFlags.find( (flag) => flag.toLowerCase().indexOf('--load-extension') > -1, ); @@ -181,30 +156,8 @@ export async function validatePort(port, chromeBinary, chromeFlags) { 'Port is in use and verification of whether the extension is loaded failed', ); } - // chromeFlags.forEach((flag) => { - // const valueAt = flag.indexOf('='); - // const [key, value] = - // valueAt === -1 - // ? [flag, undefined] - // : [flag.substring(0, valueAt), flag.substring(valueAt + 1)]; - - // // let key; - // // let value; - // // if (valueAt === -1) { - // // key = flag; - // // log.debug('flag: [%s]', key); - // // } else { - // // key = flag.substring(0, valueAt); - // // value = flag.substring(valueAt + 1); - // // log.debug('flag: [%s] => %s', key, value); - // // } - // flags.set(key, value); - // }); - - // const extension = flags.get('--load-extension'); if (isAvailable) { - // log.debug('Using debugging port: %d', port); return true; } From 62628b4bf3483ecfd31440e0a795cbb4c8931a05 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 19:32:00 -0400 Subject: [PATCH 07/13] fix: move port validation higher in the flow to prevent unnecessary work on failure --- src/extension-runners/chromium.js | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index 92a81da860..e04f6d4ac7 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -153,6 +153,24 @@ export class ChromiumExtensionRunner { chromeFlags.push(...this.params.args); } + const { chromiumPort } = this.params; + if (chromiumPort) { + log.debug('(port: %d)', chromiumPort); + const isPortUsable = await validatePort( + chromiumPort, + chromiumBinary, + chromeFlags, + ).catch(async (error) => { + // Validation wasn't able to complete successfully + throw new PortVerificationFailedError(error.message); + }); + + if (!isPortUsable) { + // Validation completed, and the port definitely isn't usable + throw new PortInUseError(); + } + } + // eslint-disable-next-line prefer-const let { userDataDir, profileDirName } = await ChromiumExtensionRunner.getProfilePaths( @@ -204,24 +222,6 @@ export class ChromiumExtensionRunner { chromeFlags.push(...startingUrls); } - const { chromiumPort } = this.params; - if (chromiumPort) { - log.debug('(port: %d)', chromiumPort); - - const isPortUsable = await validatePort( - this.params.chromiumPort, - chromiumBinary, - chromeFlags, - ).catch((error) => { - log.error('Port verification failed: %s', error.message); - throw new PortVerificationFailedError(); - }); - - if (!isPortUsable) { - throw new PortInUseError(); - } - } - const chromiumConfig = { enableExtensions: true, chromePath: chromiumBinary, From c8cae1e74b2c93fe15632b0e997bcac0a553e4dc Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 21:39:36 -0400 Subject: [PATCH 08/13] cleanup --- src/extension-runners/chromium.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index e04f6d4ac7..c62fa9aa3f 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -235,7 +235,7 @@ export class ChromiumExtensionRunner { log.debug('(config: %O)', chromiumConfig); this.chromiumInstance = await this.chromiumLaunch(chromiumConfig); - + log.debug('(process: %O)', this.chromiumInstance); this.chromiumInstance.process.once('close', () => { this.chromiumInstance = null; From 7c1eeda183a6013176b4065091d4efff2e01c713 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 21:40:25 -0400 Subject: [PATCH 09/13] feat: allow test fake process to inherit port --- tests/unit/helpers.js | 13 +++++++++++++ tests/unit/test-extension-runners/test.chromium.js | 5 ++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index 708cb9507d..e92960fcdf 100644 --- a/tests/unit/helpers.js +++ b/tests/unit/helpers.js @@ -193,6 +193,13 @@ export class StubChildProcess extends EventEmitter { stderr = new EventEmitter(); stdout = new EventEmitter(); kill = sinon.spy(() => {}); + + constructor(params = {}) { + super(); + + // mimic chrome-launch and set a port property, defaulting to a random port + this.port = params.chromiumPort ? params.chromiumPort : getRandomInt(1024, 65536); + } } export function createFakeProcess() { @@ -336,3 +343,9 @@ export function mockModule({ export function resetMockModules() { td.reset(); } + +export function getRandomInt(min, max) { + const _min = Math.ceil(min); + const _max = Math.floor(max); + return Math.floor(Math.random() * (_max - _min) + _min); +} \ No newline at end of file diff --git a/tests/unit/test-extension-runners/test.chromium.js b/tests/unit/test-extension-runners/test.chromium.js index 63fdb5bed2..552e37f1fe 100644 --- a/tests/unit/test-extension-runners/test.chromium.js +++ b/tests/unit/test-extension-runners/test.chromium.js @@ -23,7 +23,7 @@ import isDirectory from '../../../src/util/is-directory.js'; function prepareExtensionRunnerParams({ params } = {}) { const fakeChromeInstance = { - process: new StubChildProcess(), + process: new StubChildProcess(params), kill: sinon.spy(async () => {}), }; const runnerParams = { @@ -628,6 +628,9 @@ describe('util/extension-runners/chromium', async () => { sinon.assert.calledWithMatch(params.chromiumLaunch, { port, }); + assert.isDefined(runnerInstance.chromiumInstance, 'returned process instance'); + assert.isDefined(runnerInstance.chromiumInstance.process.port, 'process instance has port value'); + assert.equal(runnerInstance.chromiumInstance.process.port, port, 'process instance configured with correct port'); await runnerInstance.exit(); }); From d0cdd3eaeaf1bb6cb1e09f0acfa3616a84ac8b06 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 22:28:34 -0400 Subject: [PATCH 10/13] style: code formatting --- tests/unit/helpers.js | 6 ++++-- .../unit/test-extension-runners/test.chromium.js | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index e92960fcdf..d495d919b1 100644 --- a/tests/unit/helpers.js +++ b/tests/unit/helpers.js @@ -198,7 +198,9 @@ export class StubChildProcess extends EventEmitter { super(); // mimic chrome-launch and set a port property, defaulting to a random port - this.port = params.chromiumPort ? params.chromiumPort : getRandomInt(1024, 65536); + this.port = params.chromiumPort + ? params.chromiumPort + : getRandomInt(1024, 65536); } } @@ -348,4 +350,4 @@ export function getRandomInt(min, max) { const _min = Math.ceil(min); const _max = Math.floor(max); return Math.floor(Math.random() * (_max - _min) + _min); -} \ No newline at end of file +} diff --git a/tests/unit/test-extension-runners/test.chromium.js b/tests/unit/test-extension-runners/test.chromium.js index 552e37f1fe..624558241c 100644 --- a/tests/unit/test-extension-runners/test.chromium.js +++ b/tests/unit/test-extension-runners/test.chromium.js @@ -628,9 +628,19 @@ describe('util/extension-runners/chromium', async () => { sinon.assert.calledWithMatch(params.chromiumLaunch, { port, }); - assert.isDefined(runnerInstance.chromiumInstance, 'returned process instance'); - assert.isDefined(runnerInstance.chromiumInstance.process.port, 'process instance has port value'); - assert.equal(runnerInstance.chromiumInstance.process.port, port, 'process instance configured with correct port'); + assert.isDefined( + runnerInstance.chromiumInstance, + 'returned process instance', + ); + assert.isDefined( + runnerInstance.chromiumInstance.process.port, + 'process instance has port value', + ); + assert.equal( + runnerInstance.chromiumInstance.process.port, + port, + 'process instance configured with correct port', + ); await runnerInstance.exit(); }); From bd410f4aed272b46b440f9ac95c4175bbc643fce Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 22:29:18 -0400 Subject: [PATCH 11/13] style: renamed error for simplicity --- src/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/errors.js b/src/errors.js index c92ca053d1..c467ff2f80 100644 --- a/src/errors.js +++ b/src/errors.js @@ -64,7 +64,7 @@ export class PortInUseError extends UnusablePortError { super(message); } } -export class PortVerificationFailedError extends UnusablePortError { +export class PortInvalidError extends UnusablePortError { constructor(message = 'The requested port cannot be used.') { super(message); } From 41ad45ad211f20ef7afc4985d757085be904d087 Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 22:32:31 -0400 Subject: [PATCH 12/13] fix: remove extraneous logging, use renamed Error --- src/extension-runners/chromium.js | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/extension-runners/chromium.js b/src/extension-runners/chromium.js index c62fa9aa3f..c683afd9c0 100644 --- a/src/extension-runners/chromium.js +++ b/src/extension-runners/chromium.js @@ -18,7 +18,7 @@ import { TempDir } from '../util/temp-dir.js'; import isDirectory from '../util/is-directory.js'; import fileExists from '../util/file-exists.js'; import { validatePort } from '../util/verify-chromium-port.js'; -import { PortInUseError, PortVerificationFailedError } from '../errors.js'; +import { PortInUseError, PortInvalidError } from '../errors.js'; const log = createLogger(import.meta.url); @@ -156,17 +156,11 @@ export class ChromiumExtensionRunner { const { chromiumPort } = this.params; if (chromiumPort) { log.debug('(port: %d)', chromiumPort); - const isPortUsable = await validatePort( - chromiumPort, - chromiumBinary, - chromeFlags, - ).catch(async (error) => { - // Validation wasn't able to complete successfully - throw new PortVerificationFailedError(error.message); + const isPortUsable = await validatePort(chromiumPort).catch((error) => { + throw new PortInvalidError(error.message); }); if (!isPortUsable) { - // Validation completed, and the port definitely isn't usable throw new PortInUseError(); } } @@ -222,7 +216,7 @@ export class ChromiumExtensionRunner { chromeFlags.push(...startingUrls); } - const chromiumConfig = { + this.chromiumInstance = await this.chromiumLaunch({ enableExtensions: true, chromePath: chromiumBinary, chromeFlags, @@ -231,11 +225,7 @@ export class ChromiumExtensionRunner { // Ignore default flags to keep the extension enabled. ignoreDefaultFlags: true, port: chromiumPort, - }; - - log.debug('(config: %O)', chromiumConfig); - this.chromiumInstance = await this.chromiumLaunch(chromiumConfig); - log.debug('(process: %O)', this.chromiumInstance); + }); this.chromiumInstance.process.once('close', () => { this.chromiumInstance = null; From 1f9754f9bceaebb11b5e59150831fa8d89cef72d Mon Sep 17 00:00:00 2001 From: Tyler Devereaux Date: Thu, 11 Jul 2024 22:32:46 -0400 Subject: [PATCH 13/13] fix: remove extended process validation --- src/util/verify-chromium-port.js | 160 ++++--------------------------- 1 file changed, 21 insertions(+), 139 deletions(-) diff --git a/src/util/verify-chromium-port.js b/src/util/verify-chromium-port.js index 4128a56a4a..319b2b5782 100644 --- a/src/util/verify-chromium-port.js +++ b/src/util/verify-chromium-port.js @@ -1,114 +1,12 @@ import { createServer } from 'node:http'; -import { exec } from 'node:child_process'; -import { createLogger } from './logger.js'; -import { UnusablePortError, UsageError } from '../errors.js'; - -const log = createLogger(import.meta.url); - -/** - * Returns a random, available port - * @returns {number} Available port number to use; - */ -export async function getRandomPort() { - return new Promise((resolve, reject) => { - const server = createServer(); - server.once('listening', () => { - const { port } = server.address(); - server.close(() => resolve(port)); - }); - server.once('error', () => reject); - server.listen(0); - }); -} - -/** - * Search for existing process - * @param {string} chromeBinary - * @returns {Promise} Process list output - */ -async function findProcess(chromeBinary) { - const platform = process.platform; - let cmd = ''; - - switch (platform) { - case 'freebsd': - // no dash - cmd = 'ps axo cmd'; - break; - case 'darwin': - cmd = 'ps -axo cmd'; - break; - case 'linux': - // dash - cmd = 'ps -Ao cmd'; - cmd = `${cmd} | grep ${chromeBinary}`; - break; - case 'win32': - cmd = `powershell "Get-CimInstance Win32_Process -Filter \\"Name -Like '${chromeBinary}'\\" | Select CommandLine"`; - break; - default: - throw new UsageError('Unsupported platform'); - } - - const promise = new Promise((resolve, reject) => { - exec(cmd, (error, stdout, stderr) => { - if (stderr) { - log.debug('Error: %s', stderr); - } - - if (error) { - reject(error); - } else { - resolve(stdout); - } - }); - }); - - return promise; -} - -/** - * Inspect process list output and match against specified port and extension - * @param {number} port User-requested port - * @param {string} extension Web-ext temporary extension - * @param {string} output Output from OS-specific process list containing process command line - * @returns {Promise} Whether an eligible instance was found - */ -async function inspectProcessList(port, extension, output) { - if (!output) { - log.info('Browser instance not found'); - return false; - } - const lines = output.split('\n'); - let foundEligibleInstance = false; - - lines.forEach((line) => { - const extensionMatch = `--load-extension=${extension}`; - const portMatch = `--remote-debugging-port=${port}`; - - const isPortMatch = line.toLowerCase().indexOf(portMatch) > -1; - const isExtension = line.indexOf(extensionMatch) > -1; - - if (!isPortMatch) { - return; - } - - if (!isExtension) { - return; - } - - foundEligibleInstance = true; - }); - - return foundEligibleInstance; -} +import { UnusablePortError } from '../errors.js'; /** * Determine if a port is available * @param {number} port Port to test * */ -export async function portAvailable(port) { +async function isPortAvailable(port) { return new Promise((resolve) => { const server = createServer(); server.once('listening', () => { @@ -120,51 +18,35 @@ export async function portAvailable(port) { } /** - * Validate user-supplied port to ensure it is suitable for use - * @param {number} port User-supplied port request - * @param {string} chromeBinary Chromium binary being requested for launch - * @param {string[]} chromeFlags Array of flags requested for launch - * @returns {boolean} Whether requested port is usable + * Validate that requested port is a valid port + * @param {number} port Debugging port + * @returns {boolean} */ -export async function validatePort(port, chromeBinary, chromeFlags) { +function isPortValid(port) { if (!port) { return false; } - if (isNaN(port)) { - throw new UnusablePortError(`Non-numeric port provided (${port})`); + if (Number.isNaN(port) || !Number.isInteger(port)) { + throw new UnusablePortError(`Port provided is not an integer (${port})`); } if (port < 0 || port > 65535) { throw new UnusablePortError(`Invalid port number: ${port}`); } - const isAvailable = await portAvailable(port); - const extensions = chromeFlags.find( - (flag) => flag.toLowerCase().indexOf('--load-extension') > -1, - ); - if (!extensions.length || !extensions.length > 1) { - // This shouldn't happen... - throw new UnusablePortError( - 'Port is in use and verification of whether the extension is loaded failed', - ); - } - - const extension = extensions[0].substring(extensions[0].indexOf('=') + 1); - - if (!extension) { - // This also shouldn't happen... - throw new UnusablePortError( - 'Port is in use and verification of whether the extension is loaded failed', - ); - } + return true; +} - if (isAvailable) { - return true; +/** + * Validate user-supplied port to ensure it is suitable for use + * @param {number} port User-supplied port request + * @param {string} chromiumBinary Chromium binary being requested for launch + * @param {string[]} chromeFlags Array of flags requested for launch + * @returns {boolean} Whether requested port is usable + */ +export async function validatePort(port) { + if (!isPortValid(port)) { + return false; } - return findProcess(chromeBinary, port) - .then((ps) => inspectProcessList(port, extension, ps)) - .catch((error) => { - log.error(error); - throw new UnusablePortError(`Unable to validate port: ${error}`); - }); + return isPortAvailable(port); }