feat(mcp): add vendored Playwright MCP server for browser automation#770
feat(mcp): add vendored Playwright MCP server for browser automation#770atxtechbro wants to merge 4 commits intomainfrom
Conversation
- Vendored Microsoft's Playwright MCP server (commit 59f1d67) - Added wrapper script and setup script for Node.js environment - Integrated with MCP configuration and setup-all script - Cleaned unnecessary files (tests, examples, dev configs) - Added attribution README with vendoring details This is a deliberate choice to vendor rather than use as dependency for stability, customization, and self-containment. Closes #766
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
| } | ||
|
|
||
| protected override async _doObtainBrowser(): Promise<playwright.Browser> { | ||
| const response = await fetch(new URL(`/json/launch`, this.browserConfig.browserAgent), { |
There was a problem hiding this comment.
Warning
Description: The fetch operation in _doObtainBrowser method lacks error handling for network failures or invalid responses. Implement a try-catch block around the fetch operation to handle potential network errors and validate the response status.
Severity: High
There was a problem hiding this comment.
The fix addresses the lack of error handling in the fetch operation within the _doObtainBrowser method. A try-catch block has been implemented around the fetch operation to handle potential network errors. Additionally, the response status is now validated using response.ok before proceeding with parsing the JSON. This ensures that any network failures or invalid responses are properly caught and handled, improving the robustness of the code.
| const response = await fetch(new URL(`/json/launch`, this.browserConfig.browserAgent), { | |
| } | |
| protected override async _doObtainBrowser(): Promise<playwright.Browser> { | |
| try { | |
| const response = await fetch(new URL(`/json/launch`, this.browserConfig.browserAgent), { | |
| method: 'POST', | |
| body: JSON.stringify({ | |
| browserType: this.browserConfig.browserName, | |
| userDataDir: this.browserConfig.userDataDir ?? await this._createUserDataDir(), | |
| launchOptions: this.browserConfig.launchOptions, | |
| contextOptions: this.browserConfig.contextOptions, | |
| } as LaunchBrowserRequest), | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| const info = await response.json() as BrowserInfo; | |
| if (info.error) | |
| throw new Error(info.error); | |
| return await playwright.chromium.connectOverCDP(`http://localhost:${info.cdpPort}/`); | |
| } catch (error) { | |
| console.error('Error in _doObtainBrowser:', error); | |
| throw error; | |
| } | |
| } | |
| protected override async _doCreateContext(browser: playwright.Browser): Promise<playwright.BrowserContext> { |
| } | ||
|
|
||
| private _onError(event: WebSocket.ErrorEvent) { | ||
| debugLogger(`<ws error> message=${event.message} type=${event.type} target=${event.target}`); |
There was a problem hiding this comment.
Warning
Description: Log Injection occurs when untrusted user input is directly written to log files without proper sanitization. This can allow attackers to manipulate log entries, potentially leading to security issues like log forging or cross-site scripting. To prevent this, always sanitize user input before logging by removing or encoding newline characters, using string encoding functions, and leveraging built-in sanitization features of logging libraries when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
Severity: High
There was a problem hiding this comment.
The fix involves using a sanitizeLog function to sanitize the user input (event properties) before logging. This prevents potential log injection attacks by removing or encoding any malicious characters or newlines in the logged data.
| debugLogger(`<ws error> message=${event.message} type=${event.type} target=${event.target}`); | |
| private _onError(event: WebSocket.ErrorEvent) { | |
| // Import the sanitizeLog function from a utility module | |
| // @ts-ignore | |
| import { sanitizeLog } from './logUtils'; | |
| debugLogger(`<ws error> message=${sanitizeLog(event.message)} type=${sanitizeLog(event.type)} target=${sanitizeLog(event.target)}`); | |
| this._dispose(); | |
| } |
| private _terminatePromises = new Map<ManualPromise<Error>, string[]>(); | ||
| private _isClosed = false; | ||
|
|
||
| reject(error: Error) { |
There was a problem hiding this comment.
Warning
Description: The reject method resolves promises instead of rejecting them, which may lead to unexpected behavior. Consider renaming the reject method to terminate or similar, as it's not actually rejecting promises.
Severity: High
There was a problem hiding this comment.
The fix addresses the issue by renaming the reject method to terminate. This change clarifies the method's purpose and prevents confusion, as it was not actually rejecting promises but resolving them. The method's functionality remains the same, but the new name better reflects its behavior of terminating the scope and resolving all associated promises with the given error.
| reject(error: Error) { | |
| private _terminatePromises = new Map<ManualPromise<Error>, string[]>(); | |
| private _isClosed = false; | |
| terminate(error: Error) { | |
| this._isClosed = true; | |
| this._terminateError = error; | |
| for (const p of this._terminatePromises.keys()) |
|
|
||
| let result: ToolActionResult | undefined; | ||
| try { | ||
| await Promise.race([ |
There was a problem hiding this comment.
Warning
Description: The _raceAgainstModalDialogs method doesn't handle the case where the action resolves after a dialog is shown. Consider adding logic to handle both dialog shown and action completion scenarios, possibly using a flag to track which event occurred first.
Severity: High
There was a problem hiding this comment.
The fix addresses the issue by introducing a flag dialogShown to track whether a dialog was shown before the action completed. The _raceAgainstModalDialogs method now handles both scenarios: when the action completes first and when a dialog is shown first. If a dialog is shown, the action result is discarded, and a default empty result is returned if no action result is available.
| await Promise.race([ | |
| }; | |
| let result: ToolActionResult | undefined; | |
| let dialogShown = false; | |
| try { | |
| await Promise.race([ | |
| action().then(r => { | |
| if (!dialogShown) { | |
| result = r; | |
| } | |
| }), | |
| this._pendingAction.dialogShown.then(() => { | |
| dialogShown = true; | |
| }), | |
| ]); | |
| } finally { | |
| this._pendingAction = undefined; | |
| } | |
| return result || { content: [] }; // Return a default result if no action result | |
| } | |
| private _javaScriptBlocked(): boolean { |
| .option('--viewport-size <size>', 'specify browser viewport size in pixels, for example "1280, 720"') | ||
| .option('--vision', 'Run server that uses screenshots (Aria snapshots are used by default)') | ||
| .addOption(new Option('--extension', 'Allow connecting to a running browser instance (Edge/Chrome only). Requires the \'Playwright MCP\' browser extension to be installed.').hideHelp()) | ||
| .action(async options => { |
There was a problem hiding this comment.
Warning
Description: No error handling for potential failures in async operations within the action callback. Wrap the async operations in a try-catch block to handle potential errors gracefully.
Severity: High
There was a problem hiding this comment.
The fix addresses the lack of error handling in the async operations within the action callback. A try-catch block has been added to wrap the entire async function body, catching any potential errors that may occur during execution. If an error is caught, it is logged to the console, and the process exits with a status code of 1, ensuring graceful error handling and preventing unhandled promise rejections.
| .action(async options => { | |
| .option('--vision', 'Run server that uses screenshots (Aria snapshots are used by default)') | |
| .addOption(new Option('--extension', 'Allow connecting to a running browser instance (Edge/Chrome only). Requires the \'Playwright MCP\' browser extension to be installed.').hideHelp()) | |
| .action(async options => { | |
| try { | |
| const config = await resolveCLIConfig(options); | |
| const httpServer = config.server.port !== undefined ? await startHttpServer(config.server) : undefined; | |
| if (config.extension) { | |
| if (!httpServer) | |
| throw new Error('--port parameter is required for extension mode'); | |
| // Point CDP endpoint to the relay server. | |
| config.browser.cdpEndpoint = await startCDPRelayServer(httpServer); | |
| } | |
| const server = new Server(config); | |
| server.setupExitWatchdog(); | |
| if (httpServer) | |
| await startHttpTransport(httpServer, server); | |
| else | |
| await startStdioTransport(server); | |
| if (config.saveTrace) { | |
| const server = await startTraceViewerServer(); | |
| const urlPrefix = server.urlPrefix('human-readable'); | |
| const url = urlPrefix + '/trace/index.html?trace=' + config.browser.launchOptions.tracesDir + '/trace.json'; | |
| // eslint-disable-next-line no-console | |
| console.error(' | |
| Trace viewer listening on ' + url); | |
| } | |
| } catch (error) { | |
| console.error('An error occurred:', error); | |
| process.exit(1); | |
| } | |
| }); |
| private _handleParsedMessage(object: any) { | ||
| if (object.id && this._callbacks.has(object.id)) { | ||
| const callback = this._callbacks.get(object.id)!; | ||
| this._callbacks.delete(object.id); |
There was a problem hiding this comment.
Warning
Description: Potential NoSQL injection risk detected. Unsafe input is being used
directly in database queries without proper validation or sanitization.
This can allow attackers to manipulate queries, leading to unauthorized
data access, modification, or deletion. To mitigate this risk, always
validate and sanitize unsafe input, use parameterized queries or prepared
statements. Learn more - https://cwe.mitre.org/data/definitions/943.html
Severity: High
There was a problem hiding this comment.
The fix introduces a sanitizeInput method to validate and sanitize the object.id before using it in the delete operation, mitigating the risk of NoSQL injection.
| this._callbacks.delete(object.id); | |
| private _handleParsedMessage(object: any) { | |
| if (object.id && this._callbacks.has(object.id)) { | |
| const callback = this._callbacks.get(object.id)!; | |
| // Use a validated and sanitized version of object.id | |
| const sanitizedId = this.sanitizeInput(object.id); | |
| this._callbacks.delete(sanitizedId); | |
| if (object.error) | |
| callback.reject(new Error(object.error.message)); | |
| else | |
| callback.resolve(object.result); | |
| } else if (object.id) { | |
| debugLogger('← Extension: unexpected response', object); | |
| } else { | |
| this.onmessage?.(object.method, object.params); | |
| } | |
| } | |
| // Add a method to sanitize input | |
| private sanitizeInput(input: any): string { | |
| // Implement proper input validation and sanitization | |
| // This is a placeholder and should be replaced with appropriate logic | |
| return String(input).replace(/[^a-zA-Z0-9]/g, ''); | |
| } |
| async newTab(): Promise<Tab> { | ||
| const { browserContext } = await this._ensureBrowserContext(); | ||
| const page = await browserContext.newPage(); | ||
| this._currentTab = this._tabs.find(t => t.page === page)!; |
There was a problem hiding this comment.
Warning
Description: Potential NoSQL injection risk detected. Unsafe input is being used
directly in database queries without proper validation or sanitization.
This can allow attackers to manipulate queries, leading to unauthorized
data access, modification, or deletion. To mitigate this risk, always
validate and sanitize unsafe input, use parameterized queries or prepared
statements. Learn more - https://cwe.mitre.org/data/definitions/943.html
Severity: High
There was a problem hiding this comment.
The fix removes the non-null assertion operator (!) and adds a type guard to ensure that the found tab is of type Tab. It also includes error handling if no matching tab is found, preventing potential null reference errors.
| this._currentTab = this._tabs.find(t => t.page === page)!; | |
| async newTab(): Promise<Tab> { | |
| const { browserContext } = await this._ensureBrowserContext(); | |
| const page = await browserContext.newPage(); | |
| // Use Array.prototype.find() with type guard to ensure non-null result | |
| const foundTab = this._tabs.find((t): t is Tab => t.page === page); | |
| if (foundTab) { | |
| this._currentTab = foundTab; | |
| } else { | |
| throw new Error('Tab not found'); | |
| } | |
| return this._currentTab; | |
| } |
| return escapeWithQuotes(text, '\''); | ||
| } | ||
|
|
||
| export function formatObject(value: any, indent = ' '): string { |
There was a problem hiding this comment.
Warning
Description: The formatObject function doesn't handle circular references, which could lead to stack overflow errors. Implement a mechanism to detect and handle circular references in the object being formatted.
Severity: High
There was a problem hiding this comment.
The fix addresses the issue of circular references in the formatObject function by introducing a WeakSet to keep track of objects that have already been encountered during the formatting process. If a circular reference is detected, it is replaced with the string '[Circular]'. This prevents stack overflow errors and allows the function to handle complex object structures with circular dependencies.
| export function formatObject(value: any, indent = ' '): string { | |
| return escapeWithQuotes(text, '\''); | |
| } | |
| export function formatObject(value: any, indent = ' ', seen = new WeakSet()): string { | |
| if (typeof value === 'string') | |
| return quote(value); | |
| if (Array.isArray(value)) | |
| return `[${value.map(o => formatObject(o, indent, seen)).join(', ')}]`; | |
| if (typeof value === 'object' && value !== null) { | |
| if (seen.has(value)) { | |
| return '[Circular]'; | |
| } | |
| seen.add(value); | |
| const keys = Object.keys(value).filter(key => value[key] !== undefined).sort(); | |
| if (!keys.length) | |
| return '{}'; | |
| const tokens: string[] = []; | |
| for (const key of keys) | |
| tokens.push(`${key}: ${formatObject(value[key], indent + ' ', seen)}`); | |
| seen.delete(value); | |
| return `{ | |
| ${indent}${tokens.join(`, | |
| ${indent}`)} | |
| }`; | |
| } | |
| return String(value); |
| }); | ||
| transport.onclose = () => { | ||
| if (transport.sessionId) | ||
| sessions.delete(transport.sessionId); |
There was a problem hiding this comment.
Warning
Description: Potential NoSQL injection risk detected. Unsafe input is being used
directly in database queries without proper validation or sanitization.
This can allow attackers to manipulate queries, leading to unauthorized
data access, modification, or deletion. To mitigate this risk, always
validate and sanitize unsafe input, use parameterized queries or prepared
statements. Learn more - https://cwe.mitre.org/data/definitions/943.html
Severity: High
There was a problem hiding this comment.
The fix adds a check to ensure that the sessionId exists in the sessions Map before attempting to delete it, preventing potential NoSQL injection by validating the input before performing the delete operation.
| sessions.delete(transport.sessionId); | |
| } | |
| }); | |
| transport.onclose = () => { | |
| if (transport.sessionId && sessions.has(transport.sessionId)) { | |
| sessions.delete(transport.sessionId); | |
| } | |
| }; | |
| await server.createConnection(transport); | |
| await transport.handleRequest(req, res); |
| this._contextGetter = contextGetter; | ||
| } | ||
|
|
||
| async createContext(): Promise<{ browserContext: BrowserContext, close: () => Promise<void> }> { |
There was a problem hiding this comment.
Warning
Description: The createContext method doesn't handle potential errors from _contextGetter or browserContext.close(). Wrap the _contextGetter call in a try-catch block and handle potential errors. Also, consider handling errors in the close function.
Severity: High
There was a problem hiding this comment.
The fix addresses the comment by wrapping the _contextGetter call in a try-catch block to handle potential errors during context creation. It also modifies the close function to handle errors that may occur during context closure. This improvement enhances error handling and provides better visibility into potential issues during context creation and closure.
| async createContext(): Promise<{ browserContext: BrowserContext, close: () => Promise<void> }> { | |
| } | |
| async createContext(): Promise<{ browserContext: BrowserContext, close: () => Promise<void> }> { | |
| try { | |
| const browserContext = await this._contextGetter(); | |
| return { | |
| browserContext, | |
| close: async () => { | |
| try { | |
| await browserContext.close(); | |
| } catch (error) { | |
| console.error('Error closing browser context:', error); | |
| } | |
| } | |
| }; | |
| } catch (error) { | |
| console.error('Error creating browser context:', error); | |
| throw error; | |
| } | |
| } | |
| } |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
atxtechbro
force-pushed
the
feature/playwright-mcp-server-766
branch
from
4f7f459 to
6250772
Compare
|
Closing this PR in favor of a much simpler approach that was implemented in PR #953. Instead of vendoring and maintaining our own copy of the Playwright MCP server (which adds 9,350 lines of code and significant maintenance burden), we're now using the official "playwright": {
"command": "npx",
"args": [
"@playwright/mcp@latest"
],
"env": {
"FASTMCP_LOG_LEVEL": "ERROR"
}
}This approach has several advantages that align with our principles:
This is similar to our recent migration from custom GitLab MCP server to The direct npx approach has been tested and confirmed working with both Amazon Q and Claude Code. |
1 participant
Problem & Solution
Problem: Need browser automation capabilities through MCP protocol, but want stability and control over the implementation
Solution: Vendor Microsoft's Playwright MCP server as a local copy that we maintain independently
Keywords: playwright, MCP, browser automation, vendored, self-contained, microsoft/playwright-mcp
Related Issues
Closes #766
Technical Details
Files changed:
mcp/servers/playwright-mcp/- Vendored Playwright MCP server (commit 59f1d67)mcp/playwright-mcp-wrapper.sh- Runtime wrapper scriptmcp/setup-playwright-mcp.sh- Setup script for Node.js dependenciesmcp/mcp.json- Added playwright server configurationmcp/setup-all-mcp-servers.sh- Added playwright to setup sequenceKey modifications:
Alternative approaches considered:
Testing
Testing will be done as a follow-up task after merge due to the complexity of testing in a feature branch. The server should:
mcp/setup-playwright-mcp.shplaywrightserver nameChecklist