feat: port upstream PR#779 (debug host + stable Chrome shutdown)#8
feat: port upstream PR#779 (debug host + stable Chrome shutdown)#8
Conversation
…s group kill, improved CDP retries)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| this.chromeChild.kill('SIGINT'); | ||
| const signal = 'SIGINT'; | ||
| util.log(`Sending ${signal} to browser PID group ${this.chromeChild.pid}`); | ||
| try { |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
The core issue is that on non-macOS platforms, the code runs spawn(location, chromeFlags, { shell: true, detached: true }), where chromeFlags is user-influenced. With shell: true, the array is concatenated to form a command string passed to the system shell, exposing the process to command injection if any element in chromeFlags comes from untrusted input.
General Fix:
Do not use { shell: true } when not required, especially with argument arrays containing any untrusted input. Spawn the process with shell: false wherever possible, so arguments are passed as direct process arguments (not shell-parsed strings).
Specific Fix Here:
- On all platforms (not just macOS), use
shell: falsein the call tospawn. Since Chrome flags are simple option strings and do not require shell features (pipes, redirections, etc.), using the direct API is both safe and correct. - If, for Windows or particular older versions, some shell feature is strictly necessary, escaping and validating user-provided arguments would be required (but that's not the case here).
- To implement:
- On line 59, change
{ shell: true, ... }to{ shell: false, ... }(matching line 57). - No extra dependencies or changes to imports/exports are needed.
- On line 59, change
| @@ -56,7 +56,7 @@ | ||
| if (os.platform() === 'darwin') { | ||
| this.chromeChild = spawn(location, chromeFlags, { shell: false, detached: true }); | ||
| } else { | ||
| this.chromeChild = spawn(location, chromeFlags, { shell: true, detached: true }); | ||
| this.chromeChild = spawn(location, chromeFlags, { shell: false, detached: true }); | ||
| } | ||
| // detached will group all the processes so we can kill them easily later | ||
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR ports upstream changes to improve Chrome browser lifecycle management and debugging capabilities. The changes address persistent 504 errors likely caused by flaky Chrome connections and orphaned processes.
- Added configurable debug host support via BROWSER_DEBUGGING_HOST environment variable
- Improved Chrome process management with detached spawning and process group killing
- Enhanced logging for debugging and shutdown diagnostics
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/server.js | Added BROWSER_DEBUGGING_HOST configuration and shutdown logging |
| lib/browsers/chrome.js | Updated Chrome spawning, killing, and connection logic with better host handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| const CDP = require('chrome-remote-interface'); | |||
| const { spawn } = require('child_process'); | |||
| const { spawn } = require('node:child_process'); | |||
There was a problem hiding this comment.
The import uses the node: prefix which requires Node.js 14.18.0+. Consider checking if this is compatible with the project's minimum Node.js version requirements.
| const { spawn } = require('node:child_process'); | |
| const { spawn } = require('child_process'); |
| let browser = null; | ||
|
|
||
| const connectToBrowser = async (target, port) => { | ||
| const connectToBrowser = async (host, port, target) => { |
There was a problem hiding this comment.
The parameter order has been changed from (target, port) to (host, port, target). This breaks the existing API contract and could cause confusion since the CDP call uses a different parameter order.
| for (;;) { | ||
| try { | ||
| return await CDP({ host, target, port }); | ||
| return await CDP({ host, port, target }); |
There was a problem hiding this comment.
The parameter order has been changed from (target, port) to (host, port, target). This breaks the existing API contract and could cause confusion since the CDP call uses a different parameter order.
Port of upstream prerender#779
Key changes:
Motivation: We were experiencing persistent 504s likely caused by flaky Chrome connections and lingering orphaned Chrome processes. These changes improve observability and lifecycle management.
No behavior change expected for existing setups unless they rely on non-local debug hosts; set BROWSER_DEBUGGING_HOST explicitly if needed.