Migrate from websocket to deprecated console domain #348
Conversation
WalkthroughThe changes represent a significant refactoring of the agent's communication and networking architecture. The primary focus is on removing WebSocket-based communication and replacing it with a new console-based communication mechanism. A new Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
soundofspace
force-pushed
the
migrate-from-websocket-to-deprecated-console-domain
branch
from
9ca1024 to
bb8ad29
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
agent/main/lib/Browser.ts (1)
371-371: Consider follow-up changes for console domain migrationWhile removing the websocket bypass is a good start, consider reviewing the following areas as part of the websocket to console domain migration:
- Event handling methods (
onAttachedToTarget,onTargetCreated, etc.)- Communication protocols in
DevtoolsSession- Target attachment and detachment logic
agent/main/lib/Console.ts (3)
23-23: Missing Semicolon in Variable DeclarationOn line 23, the variable
separatoris declared without a terminating semicolon. While TypeScript allows optional semicolons, it's a best practice to include them for consistency and to avoid potential issues.Apply this diff to add the missing semicolon:
- private separator = ' --- ' + private separator = ' --- ';
80-99: Error Handling inhandleConsoleMessageMethodIn the
handleConsoleMessagemethod, exceptions might be silently caught and ignored, which can make debugging difficult.Consider logging errors when they occur to aid in troubleshooting:
} catch (error) { - return; + this.logger.error('Error handling console message', { error }); + return; }
118-137: Inefficient Stringification ofinjectedScriptFunctionThe method of stringifying
injectedScriptand performing string replacements can be brittle and hard to maintain. This approach may lead to errors if the function body contains strings that match the placeholders.Consider refactoring to use template literals or embedding the variables directly without string replacements. For example:
function injectedScript(): void { const clientId = Math.random(); const secretKey = '${this.secretKey}'; const separator = '${this.separator}'; // ... rest of the code ... console.debug(`${secretKey}${separator}${clientId}${separator}${serializedData}`); }Then, in
injectCallbackIntoScript, use a template literal:const wsScript = `(${injectedScript.toString()})();`;This approach avoids the need for string replacements and reduces the risk of errors.
agent/main/lib/FramesManager.ts (3)
73-73: Initialization Order ofconsolePropertyThe
consoleproperty is initialized on line 73 after some event listeners are set up in the constructor. If any of these listeners rely onthis.console, it may lead to undefined behavior.Move the initialization of
this.consolebefore setting up event listeners that use it:this.page = page; //... this.devtoolsSession = devtoolsSession; bindFunctions(this); // Initialize console before using it this.console = new Console(devtoolsSession, this.page.browserContext.secretKey); // Event listeners that use this.console this.events.on(this.console, 'callback-received', this.onCallbackReceived); // Other event listeners...
Line range hint
699-717: Error Handling inonCallbackReceivedMethodIn the
onCallbackReceivedmethod, if the callback for a given event name is not found, the function proceeds without handling the case. This might lead to unintentional behavior or silent failures.Add error handling or logging when a callback is not registered:
if (callback) { await callback(event.payload, frame); + this.page.emit('page-callback-triggered', { + name: event.name, + frameId: frame.frameId, + payload: event.payload, + }); } else { + this.logger.warn('No callback registered for event', { eventName: event.name }); }
96-101: Possible Memory Leak with Event SubscribersIn the event subscription starting on line 96, an anonymous function is used without a reference, which may prevent proper unsubscription later.
Store the event handler in a variable to enable proper cleanup when needed.
this.internalRequestHandler = (event: IBrowserNetworkEvents['internal-request']) => { // ... }; this.events.on(this.networkManager, 'internal-request', this.internalRequestHandler);Ensure to clean up the event listener in the
closemethod:this.events.off(this.networkManager, 'internal-request', this.internalRequestHandler);specification/agent/browser/IConsole.ts (2)
2-2: Inconsistent Typing ofpayloadPropertyIn the
IConsoleEventsinterface,payloadis typed as astring. However, elsewhere in the code,payloadseems to beanytype (e.g., deserialized JSON object).Consider updating the type of
payloadto better reflect its usage:- 'callback-received': { id: string; name: string; payload: string }; + 'callback-received': { id: string; name: string; payload: any };
5-5: Typo in Type DefinitionThe
ConsoleCallbacktype is defined but not used in the code. Additionally, it acceptsnameandpayloadas parameters, whereas in theonCallbackReceivedmethod, onlypayloadandframeare used.Review the type definition and its usage to ensure consistency. If
ConsoleCallbackis intended to be used elsewhere, consider updating it accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
agent/main/lib/Agent.ts(3 hunks)agent/main/lib/Browser.ts(1 hunks)agent/main/lib/BrowserContext.ts(4 hunks)agent/main/lib/Console.ts(1 hunks)agent/main/lib/DevtoolsSessionLogger.ts(0 hunks)agent/main/lib/FrameOutOfProcess.ts(1 hunks)agent/main/lib/FramesManager.ts(6 hunks)agent/main/lib/NetworkManager.ts(3 hunks)agent/main/lib/Page.ts(1 hunks)agent/main/lib/WebsocketSession.ts(0 hunks)agent/main/lib/Worker.ts(1 hunks)agent/main/test/mitm.test.ts(1 hunks)agent/mitm/handlers/RequestSession.ts(2 hunks)specification/agent/browser/IBrowserNetworkEvents.ts(2 hunks)specification/agent/browser/IConsole.ts(1 hunks)specification/agent/browser/IWebsocketSession.ts(0 hunks)timetravel/lib/MirrorNetwork.ts(1 hunks)
💤 Files with no reviewable changes (3)
- agent/main/lib/DevtoolsSessionLogger.ts
- agent/main/lib/WebsocketSession.ts
- specification/agent/browser/IWebsocketSession.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test chrome-latest (node 22; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 22; windows-latest)
- GitHub Check: Test chrome-latest (node 20; windows-latest)
- GitHub Check: Test chrome-latest (node 18; windows-latest)
- GitHub Check: Test chrome-latest (node 22; windows-latest)
- GitHub Check: Test chrome-latest (node 20; windows-latest)
- GitHub Check: Test chrome-latest (node 18; windows-latest)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Test chrome-latest (node 22; macos-13)
- GitHub Check: Test chrome-latest (node 18; macos-13)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Test chrome-latest (node 18; macos-13)
🔇 Additional comments (18)
agent/main/lib/Browser.ts (1)
371-371: Verify localhost connectivity after removing websocket.localhost bypassThe removal of
websocket.localhostfrom the proxy bypass list aligns with the migration from websocket to console domain. However, we should verify that this change doesn't affect other localhost connections.Let's check for any other websocket-related configurations that might need updating:
✅ Verification successful
Proxy bypass configuration change is correct
The removal of
websocket.localhostfrom the proxy bypass list is consistent with the migration from websocket to console domain. The remaining<-loopback>configuration is sufficient for handling local connections.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining websocket configurations echo "Searching for websocket configurations..." rg -i "websocket" # Search for proxy bypass configurations echo "Searching for proxy bypass configurations..." rg "proxy-bypass-list"Length of output: 43080
agent/main/lib/Console.ts (4)
23-23: Hardcoded Separator Might Conflict with Payload ContentThe
separatorvalue' --- 'is hardcoded and used to split and join strings. If the payload or other data contains this separator sequence, it could lead to malformed data during processing.This issue was previously addressed, but please verify that the current implementation correctly handles cases where the separator might appear in the payload. If necessary, consider using a less likely sequence or escaping instances of the separator within the payload.
Also applies to: 71-74
129-134: 🛠️ Refactor suggestionImproper Handling of Global Variables in Injected Script
The injected script defines
callbackbut does not ensure it doesn't conflict with existing variables in the global scope.Wrap the injected script in an immediately invoked function expression (IIFE) to avoid polluting the global namespace:
function injectedScript(): void { + (function() { // existing code... + })(); }Alternatively, ensure variables are declared with
constorletand scoped appropriately.Likely invalid or redundant comment.
71-74: Potential Incompatibility with.replaceAllMethodThe use of
.replaceAllon lines 71-72 may cause compatibility issues in environments whereString.prototype.replaceAllis not supported (e.g., older versions of Node.js or some browsers). To ensure broader compatibility, consider using.replacewith a global regular expression.Replace the
.replaceAllcalls with.replaceand a global regex:- .replaceAll('${this.secretKey}', this.secretKey) - .replaceAll('${this.separator}', this.separator) + .replace(/\$\{this\.secretKey\}/g, this.secretKey) + .replace(/\$\{this\.separator\}/g, this.separator)Please verify if the deployment environments support
.replaceAll, and adjust accordingly.
121-122: Potential Issues with URL ParsingOn lines 121-122, constructing the URL using
new URL(document.URL).originmay not handle all cases correctly, especially if thedocument.URLis modified or in different browsing contexts.Ensure that
document.location.originis used instead for consistency:- const url = `${new URL(document.URL).origin}/heroInternalUrl?secretKey=${this.secretKey}&action=registerConsoleClientId&clientId=${clientId}`; + const url = `${document.location.origin}/heroInternalUrl?secretKey=${this.secretKey}&action=registerConsoleClientId&clientId=${clientId}`;Please verify if this change aligns with the intended functionality.
agent/main/lib/FramesManager.ts (1)
Line range hint
217-223: Potential Security Risk with Script InjectionIn the
addNewDocumentScriptmethod, user-provided scripts are being injected into pages. This can pose security risks if the scripts are not properly sanitized or trusted.[security_issue]
Ensure that only trusted scripts are injected, and consider sanitizing or validating the
scriptparameter. If scripts are sourced from user input, implement restrictions or escape mechanisms to prevent cross-site scripting (XSS) vulnerabilities.specification/agent/browser/IBrowserNetworkEvents.ts (1)
56-59: LGTM! Well-structured event interface for internal requests.The new event type is appropriately designed for handling internal communication, replacing the websocket-based approach.
agent/main/lib/Worker.ts (1)
64-64: LGTM! Consistent with the migration strategy.The change correctly integrates the new
secretKey-based approach, replacing the websocket session.timetravel/lib/MirrorNetwork.ts (1)
70-70: LGTM! CSP header correctly updated for console-based communication.The Content-Security-Policy has been appropriately modified to remove websocket-related directives while maintaining security with 'self' for connect-src.
agent/mitm/handlers/RequestSession.ts (1)
76-76: LGTM: New secretKey parameter added to support internal request handling.The addition of the optional
secretKeyparameter aligns with the PR's objective of migrating from websocket to console domain.agent/main/lib/FrameOutOfProcess.ts (1)
28-28: LGTM! Consistent implementation across componentsThe change maintains consistency with the Page class implementation, ensuring uniform network management across both in-process and out-of-process frames.
agent/main/test/mitm.test.ts (1)
40-42: LGTM! Filter added for internal traffic.The addition of the
heroInternalUrlfilter aligns well with the PR's objective of handling internal traffic differently after migrating from websocket to console domain.agent/main/lib/BrowserContext.ts (3)
43-43: LGTM! Interface updated with secretKey.The addition of the optional
secretKeytoIBrowserContextCreateOptionsis well-typed and maintains interface consistency.
79-79: LGTM! BrowserContext properly handles secretKey.The
secretKeyproperty is correctly declared and initialized from options, following the class's existing patterns.Also applies to: 99-99
117-117: LGTM! Simplified proxy bypass configuration.The proxy bypass list has been appropriately simplified after removing websocket functionality, now only including loopback addresses.
agent/main/lib/NetworkManager.ts (3)
65-65: LGTM! Constructor updated with secretKey parameter.The
secretKeyparameter is properly added to the constructor and declared as a public property.
630-630: LGTM! Simplified WebSocket handling.The WebSocket creation handler has been appropriately simplified as part of the migration away from WebSocket-based communication.
308-311: Verify the security implications of exposing secretKey in URLs.While the implementation is functional, including the
secretKeyin URLs might have security implications:
- The key could be logged in server logs
- It might be visible in browser history
- It could be exposed through referrer headers
Consider these alternatives:
- Use a custom header instead of URL parameters
- Use a more secure authentication mechanism
Run this script to check for potential security implications:
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
agent/main/lib/Console.ts (2)
125-125:⚠️ Potential issueDo not log
secretKeyto the console to prevent exposureIncluding
secretKeyin console output can lead to security vulnerabilities if logs are exposed. RemovesecretKeyfrom theconsole.debugstatement to protect sensitive information.Apply this diff:
- console.debug(`${this.secretKey}, ${clientId}, ${serializedData}`); + console.debug(`${clientId}, ${serializedData}`);Update the
handleConsoleMessagemethod to parse messages withoutsecretKey.
65-67:⚠️ Potential issueEnsure safe string interpolation to prevent injection vulnerabilities
Directly interpolating variables into stringified code can introduce security risks. Ensure
this.secretKeyandscriptare sanitized before embedding.Validate or sanitize
this.secretKeyandscriptto prevent injection attacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
agent/main/lib/Agent.ts(3 hunks)agent/main/lib/Console.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/main/lib/Agent.ts
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test chrome-115-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-120-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-130-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 20; ubuntu-latest)
- GitHub Check: Test chrome-115-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 18; ubuntu-latest)
- GitHub Check: Test chrome-120-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-130-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 22; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 20; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 18; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Test chrome-latest (node 22; windows-latest)
- GitHub Check: Test chrome-latest (node 18; macos-13)
- GitHub Check: Test chrome-latest (node 20; windows-latest)
- GitHub Check: Test chrome-latest (node 18; windows-latest)
- GitHub Check: Test chrome-latest (node 22; macos-13)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Build Javascript
- GitHub Check: Test chrome-latest (node 18; macos-13)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
agent/main/lib/Console.ts (1)
41-45:⚠️ Potential issueSecurity: Avoid exposing secretKey in URL parameters
The secretKey should not be included in URL parameters as it can be exposed in logs and referrer headers.
🧹 Nitpick comments (2)
agent/main/lib/Console.ts (2)
15-17: Update comment to reflect console-based communicationThe comment still references websocket messaging which is outdated since this implementation uses console-based communication.
- // We store resolvable when we received websocket message before, receiving - // targetId, this way we can await this, and still trigger to get proper ids. + // We store resolvable when we received console message before receiving + // targetId, allowing us to await and resolve with proper ids when available.
1-130: Consider alternative approaches to secure communicationWhile using the console for communication is creative, it introduces several security concerns and complexity. Consider these architectural alternatives:
- Use a dedicated internal protocol handler (e.g.,
hero://) for secure communication- Implement a proper message port or broadcast channel
- Use service workers for secure message passing
Would you like me to elaborate on any of these alternative approaches?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
agent/main/lib/Console.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test chrome-115-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-120-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-130-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 18; ubuntu-latest)
- GitHub Check: Test chrome-115-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-120-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-130-0 (node 20.x; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Test chrome-latest (node 22; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 20; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 18; macos-13)
- GitHub Check: Test chrome-latest (node 18; ubuntu-latest)
- GitHub Check: Test chrome-latest (node 22; windows-latest)
- GitHub Check: Test chrome-latest (node 20; windows-latest)
- GitHub Check: Test chrome-latest (node 18; windows-latest)
- GitHub Check: Test chrome-latest (node 22; macos-13)
- GitHub Check: Test chrome-latest (node 20; macos-13)
- GitHub Check: Build Javascript
- GitHub Check: Test chrome-latest (node 18; macos-13)
🔇 Additional comments (2)
agent/main/lib/Console.ts (2)
26-39: LGTM! Solid initialization implementationThe initialization logic properly handles the ready state and sets up console event subscription.
47-57: LGTM! Clean frame registration implementationThe frame registration logic properly handles both immediate and deferred resolution of frameIds.
There was a problem hiding this comment.
Looks great! A few changes/things I'd like to verify
-
Can we ensure the requests are not making it through in both mitm AND when mitm is disabled? Does request session apply to both, or should we add by default to the list of blocked urls we send to chrome or something?
-
Can you verify that these requests don't get stored in the session database? That caused a LOT of extra content that we didn't need last time.
By using fetch domain and custom url we can make sure it never leaves localhost
Nothing is stored in resources on network level but devtoolsMessages does contain |
soundofspace
force-pushed
the
migrate-from-websocket-to-deprecated-console-domain
branch
from
c77ed1e to
3a2ceca
Compare
Websockets were a nice workaround to solve the issues we had with runtime.enable, but it also introduced some new problems. Both fetch and websockets had problems with content security policies and some general websocket issues also.
This was fixed with:
Summary by CodeRabbit
Release Notes
New Features
Changes
Performance
Security