Add browser use as MCP to providers#889
Merged
Merged
Conversation
…wser-use-independent
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full Browser Use feature: a Playwright-based server-side session manager ( ChangesBrowser Use Feature
Electron Desktop Packaging
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant BrowserUsePanel as BrowserUsePanel (React)
participant ServerAPI as /api/browser-use
participant BrowserService as browserUseService
participant Playwright as Playwright (Chromium)
rect rgba(100, 149, 237, 0.5)
note over User, Playwright: Session creation and first screenshot
User->>BrowserUsePanel: Click "New Session"
BrowserUsePanel->>ServerAPI: POST /sessions
ServerAPI->>BrowserService: createAgentSession(ownerId)
BrowserService->>Playwright: launch context + page
Playwright-->>BrowserService: page handle
BrowserService->>Playwright: captureScreenshot()
BrowserService-->>ServerAPI: session data + screenshot
ServerAPI-->>BrowserUsePanel: { success, data: session }
BrowserUsePanel->>BrowserUsePanel: render screenshot
end
rect rgba(144, 238, 144, 0.5)
note over User, Playwright: Click interaction
User->>BrowserUsePanel: Click on screenshot at (x, y)
BrowserUsePanel->>BrowserUsePanel: scale pixel coords to viewport percent
BrowserUsePanel->>ServerAPI: POST /sessions/:id/agentClick { x, y }
ServerAPI->>BrowserService: agentClick(sessionId, x, y)
BrowserService->>Playwright: page.mouse.click(x, y)
BrowserService->>Playwright: captureScreenshot()
BrowserService-->>ServerAPI: updated session + new screenshot
ServerAPI-->>BrowserUsePanel: { success, data: session }
BrowserUsePanel->>BrowserUsePanel: refresh screenshot + cursor overlay
end
rect rgba(255, 165, 0, 0.5)
note over User, Playwright: MCP agent access
User->>BrowserUsePanel: Enable agent access (MCP registration)
BrowserUsePanel->>ServerAPI: POST /registerAgentMcp
ServerAPI->>BrowserService: registerAgentMcp()
BrowserService-->>ServerAPI: { command, args }
note over ServerAPI, Playwright: Agent spawns browser-use-mcp CLI
ServerAPI->>ServerAPI: browser-use-mcp stdin: tools/call
ServerAPI->>BrowserService: agentSnapshot(sessionId)
BrowserService->>Playwright: page text + page metadata
BrowserService-->>ServerAPI: snapshot result
end
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 36: The desktop:dev script uses POSIX-only syntax for setting environment
variables inline, which fails on Windows. Replace the inline env assignment in
the desktop:dev script with cross-env, a cross-platform environment variable
setter. Specifically, prepend "cross-env " before the
ELECTRON_DEV_URL=http://127.0.0.1:5173 assignment so the command reads:
"cross-env ELECTRON_DEV_URL=http://127.0.0.1:5173 electron electron/main.js". If
cross-env is not already installed, add it to devDependencies.
In `@server/browser-use-mcp.ts`:
- Around line 39-57: The `callBrowserUseApi` function's fetch call lacks timeout
protection, which can cause indefinite hangs on network stalls. Add request
timeout handling by creating an AbortController before the fetch call, set up a
timeout (using setTimeout) that aborts the controller after a reasonable
duration (e.g., 30 seconds), pass the abort signal to the fetch options, and
handle the abort error appropriately by throwing a descriptive timeout error.
This ensures the API request will fail gracefully rather than hanging
indefinitely.
- Around line 380-387: The JSON.parse call that parses rawMessage is currently
outside the try block in the async IIFE, so parse errors for malformed JSON
bypass the catch block and cause unhandled rejections. Move the JSON.parse
statement inside the try block so that any JSON parsing errors are caught and
handled by the existing catch block that calls sendError, ensuring malformed
input is properly handled instead of destabilizing the process.
In `@server/cli.js`:
- Around line 11-12: The browser-use-mcp command is documented in the header
comments but is missing from the showHelp() function output. Add the
browser-use-mcp command to the help text displayed by showHelp() to keep the
help output in sync with the implemented commands, ensuring it appears alongside
other commands like status with appropriate formatting and description.
In `@server/index.js`:
- Around line 1716-1722: The shutdownRuntimeServices function lacks error
handling for async failures. If browserUseService.stopAllSessions() throws,
stopAllPlugins() and process.exit(0) will not execute, leaving the process
hanging. Wrap the shutdown logic in a try/finally block where the try block
contains browserUseService.stopAllSessions(), and the finally block contains
stopAllPlugins() and process.exit(0) to guarantee cleanup and exit occur
regardless of whether the first operation fails.
In `@server/modules/browser-use/browser-use.routes.ts`:
- Around line 122-127: Validate click coordinates and key input before passing
them to the service methods to prevent invalid values like NaN or empty strings
from causing runtime errors downstream. For the click coordinate validation in
the userClick method call (converting req.body?.x and req.body?.y to numbers),
check that the resulting values are valid numbers and not NaN before passing
them to browserUseService.userClick. Similarly, apply equivalent validation for
key input at the other affected location around line 137-140 to ensure empty or
invalid keys are rejected before calling the corresponding service method.
Reject requests with invalid parameters by returning an appropriate error
response instead of allowing them to propagate to the service layer.
In `@server/modules/browser-use/browser-use.service.ts`:
- Around line 628-646: The browser/context launch is not protected by error
handling, so if any step after launch fails (attachRequestGuard, captureSession,
publicSession, etc.), the browser and context remain orphaned and are never
cleaned up. Wrap the code from the launchPersistentContext and launch calls
through the final return statement in a try-catch block that closes the browser
and context resources if any error occurs, ensuring proper cleanup even when
bootstrap steps fail.
- Around line 389-397: The attachRequestGuard function is currently being
attached at the page level in multiple places, which means newly created pages
(popups, replacements) won't inherit the protection. Move the network guard
attachment from the per-page attachment approach to be applied once at the
browser context level immediately after context creation, ensuring it applies to
both persistent and non-persistent context creation paths. This way, every page
spawned from that context will automatically inherit the request guard without
needing individual page-level attachment.
- Around line 10-11: The imports in browser-use.service.ts are using deep module
paths (repositories/app-config.js and services/mcp.service.js) that violate
boundary rules. Replace these deep imports with module barrel imports by
importing appConfigDb from `@/modules/database` and providerMcpService from
`@/modules/providers` instead, assuming barrel index files exist at those module
levels to re-export these symbols.
- Around line 223-244: The runCommand function lacks timeout protection,
allowing spawned npm or playwright installation processes to hang indefinitely
if they get stuck. Add a timeout mechanism to the runCommand function that
rejects the promise if the spawned child process exceeds a reasonable time limit
(e.g., 5-10 minutes). Implement this by using setTimeout to set a deadline, and
if the child process has not completed within that timeframe, kill the child
process and reject with an appropriate timeout error. Ensure the timeout is
properly cleaned up when the child process closes normally to avoid resource
leaks or duplicate rejections.
- Around line 603-606: The session limit validation in the
browser-use.service.ts file only counts sessions with 'ready' status, allowing
users to create unlimited sessions in other states like 'unavailable' or
'stopped', which bypasses the cap and enables memory DoS. Remove the
`.filter((item) => item.status === 'ready')` clause when checking against
MAX_SESSIONS_PER_OWNER so that all sessions regardless of their status are
counted toward the per-user limit. This fix must be applied at all locations
where the session cap is checked using the ownerSessions function with status
filtering.
- Around line 409-411: The `canAccessSession` function has a security
vulnerability where the condition on the return statement grants access to any
authenticated user if `agentAccessEnabled === true`, bypassing per-owner
isolation. Fix this by modifying the logic so that `agentAccessEnabled` does not
independently grant access; instead, ensure that access is only allowed when the
user is the session owner or has the AGENT_OWNER_ID, and treat
`agentAccessEnabled` as an additional constraint that must be checked only after
ownership is verified, not as a standalone access permission.
In `@server/modules/browser-use/tests/browser-use.service.test.ts`:
- Around line 17-30: The test creates two sessions via
browserUseService.createSession() but does not clean them up, which can cause
resource leaks across test runs. Store the result of the second createSession
call for ownerB (currently the return value is discarded), then add a teardown
section after all assertions that calls the appropriate cleanup method (such as
stopSession or deleteSession) on both ownerASession and the ownerB session to
ensure proper resource cleanup and test isolation.
In `@src/components/browser-use/view/BrowserUsePanel.tsx`:
- Around line 71-85: The refresh callback is invoked without error handling,
which can leave unhandled promise rejections and fail to update error state for
the user. Add a try-catch block or .catch() handler around the refresh()
function call to capture any errors and set appropriate error state;
additionally, clear any prior error state (likely using setError or similar) at
the beginning or end of the successful refresh path to ensure stale errors don't
persist after a successful refresh.
- Around line 188-199: The pressViewerKey callback function prevents Tab key
events from propagating normally by calling event.preventDefault() on any key
not in the ignoredKeys set. This creates a keyboard trap that prevents users
from using Tab to traverse focus out of the browser viewer. Add 'Tab' to the
ignoredKeys set alongside the other modifier keys (Shift, Control, Alt, Meta,
CapsLock) so that Tab key presses are properly ignored and normal keyboard focus
traversal is preserved.
In `@src/components/main-content/view/subcomponents/MainContentTitle.tsx`:
- Around line 31-33: The string 'Browser Use' returned when activeTab equals
'browser' is hardcoded and bypasses the i18n system, preventing localization.
Replace the hardcoded 'Browser Use' string with an appropriate translation key
function call (using your project's i18n library) to enable proper localization
for non-English locales. Ensure the translation key consistently maps to the
browser tab label throughout the application.
In `@src/i18n/locales/ja/common.json`:
- Line 26: The "browser" key in the Japanese locale file
(src/i18n/locales/ja/common.json) currently has the English value "Browser"
instead of being translated to Japanese. Replace the English text with the
appropriate Japanese translation to ensure consistent UX across all localized
interfaces.
In `@src/i18n/locales/ko/common.json`:
- Around line 25-26: The "browser" key in the Korean, Russian, and Turkish
locale files (src/i18n/locales/ko/common.json, src/i18n/locales/ru/common.json,
and src/i18n/locales/tr/common.json) is using the English value "Browser"
instead of providing localized translations. Translate the "browser" value to
the appropriate language in each of these three locale files: Korean for
ko/common.json, Russian for ru/common.json, and Turkish for tr/common.json.
Ensure consistency across all non-English locales by either providing proper
translations or explicitly standardizing this as a product term that remains in
English across all locales if that is the intended behavior.
In `@src/i18n/locales/zh-CN/common.json`:
- Line 26: Replace the English string "Browser" with appropriate Chinese
localization in both locale files. In src/i18n/locales/zh-CN/common.json at line
26, change the value for the browser key from "Browser" to the Simplified
Chinese equivalent. In src/i18n/locales/zh-TW/common.json at line 26, change the
value for the browser key from "Browser" to the Traditional Chinese equivalent.
Ensure the Chinese terms used are consistent with your product's terminology and
localization standards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3856d51b-4947-4c0d-9c57-63197bf409c1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
package.jsonserver/browser-use-mcp.tsserver/cli.jsserver/index.jsserver/modules/browser-use/browser-use-mcp.routes.tsserver/modules/browser-use/browser-use.routes.tsserver/modules/browser-use/browser-use.service.tsserver/modules/browser-use/tests/browser-use.service.test.tssrc/components/browser-use/index.tssrc/components/browser-use/view/BrowserUsePanel.tsxsrc/components/main-content/types/types.tssrc/components/main-content/view/MainContent.tsxsrc/components/main-content/view/subcomponents/MainContentHeader.tsxsrc/components/main-content/view/subcomponents/MainContentTabSwitcher.tsxsrc/components/main-content/view/subcomponents/MainContentTitle.tsxsrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/Settings.tsxsrc/components/settings/view/SettingsSidebar.tsxsrc/components/settings/view/tabs/browser-use-settings/BrowserUseSettingsTab.tsxsrc/hooks/useProjectsState.tssrc/i18n/locales/de/common.jsonsrc/i18n/locales/en/common.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/it/common.jsonsrc/i18n/locales/ja/common.jsonsrc/i18n/locales/ko/common.jsonsrc/i18n/locales/ru/common.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-TW/common.jsonsrc/types/app.ts
Comment on lines
+31
to
+33
| if (activeTab === 'browser') { | ||
| return 'Browser Use'; | ||
| } |
Contributor
There was a problem hiding this comment.
Use translation key for browser tab title instead of a hardcoded string.
Line 31–33 bypasses i18n, so the tab title will stay English even in non-English locales.
Suggested fix
- if (activeTab === 'browser') {
- return 'Browser Use';
- }
+ if (activeTab === 'browser') {
+ return t('tabs.browser');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (activeTab === 'browser') { | |
| return 'Browser Use'; | |
| } | |
| if (activeTab === 'browser') { | |
| return t('tabs.browser'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/main-content/view/subcomponents/MainContentTitle.tsx` around
lines 31 - 33, The string 'Browser Use' returned when activeTab equals 'browser'
is hardcoded and bypasses the i18n system, preventing localization. Replace the
hardcoded 'Browser Use' string with an appropriate translation key function call
(using your project's i18n library) to enable proper localization for
non-English locales. Ensure the translation key consistently maps to the browser
tab label throughout the application.
| "git": "ソース管理", | ||
| "tasks": "タスク" | ||
| "tasks": "タスク", | ||
| "browser": "Browser" |
Contributor
There was a problem hiding this comment.
Localize the Japanese browser tab label.
Line 26 uses English in the JA locale, which creates inconsistent UX.
Suggested fix
- "browser": "Browser"
+ "browser": "ブラウザ"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "browser": "Browser" | |
| "browser": "ブラウザ" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/ja/common.json` at line 26, The "browser" key in the
Japanese locale file (src/i18n/locales/ja/common.json) currently has the English
value "Browser" instead of being translated to Japanese. Replace the English
text with the appropriate Japanese translation to ensure consistent UX across
all localized interfaces.
Comment on lines
+25
to
+26
| "tasks": "작업", | ||
| "browser": "Browser" |
Contributor
There was a problem hiding this comment.
Localize tabs.browser instead of leaving raw English in non-English locales.
On Line 26, Korean locale uses "Browser" (English). The same pattern is present in src/i18n/locales/ru/common.json Line 26 and src/i18n/locales/tr/common.json Line 26. Please translate these values (or explicitly standardize this as a product term across locales).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/ko/common.json` around lines 25 - 26, The "browser" key in
the Korean, Russian, and Turkish locale files (src/i18n/locales/ko/common.json,
src/i18n/locales/ru/common.json, and src/i18n/locales/tr/common.json) is using
the English value "Browser" instead of providing localized translations.
Translate the "browser" value to the appropriate language in each of these three
locale files: Korean for ko/common.json, Russian for ru/common.json, and Turkish
for tr/common.json. Ensure consistency across all non-English locales by either
providing proper translations or explicitly standardizing this as a product term
that remains in English across all locales if that is the intended behavior.
| "git": "源代码管理", | ||
| "tasks": "任务" | ||
| "tasks": "任务", | ||
| "browser": "Browser" |
Contributor
There was a problem hiding this comment.
Localize tabs.browser in Chinese locale files instead of leaving it in English.
src/i18n/locales/zh-CN/common.json#L26-L26: replace"Browser"with a Simplified Chinese term consistent with product terminology.src/i18n/locales/zh-TW/common.json#L26-L26: replace"Browser"with a Traditional Chinese term consistent with product terminology.
📍 Affects 2 files
src/i18n/locales/zh-CN/common.json#L26-L26(this comment)src/i18n/locales/zh-TW/common.json#L26-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/zh-CN/common.json` at line 26, Replace the English string
"Browser" with appropriate Chinese localization in both locale files. In
src/i18n/locales/zh-CN/common.json at line 26, change the value for the browser
key from "Browser" to the Simplified Chinese equivalent. In
src/i18n/locales/zh-TW/common.json at line 26, change the value for the browser
key from "Browser" to the Traditional Chinese equivalent. Ensure the Chinese
terms used are consistent with your product's terminology and localization
standards.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/browser-use/browser-use.routes.ts (1)
65-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse 5xx for session-list failures instead of 401.
Line 65 returns
401for any exception frombrowserUseService.listSessions(), but auth is already enforced upstream. This misclassifies server/runtime errors as authentication failures.Suggested fix
- res.status(401).json({ + res.status(500).json({ success: false, error: error instanceof Error ? error.message : 'Failed to list browser sessions.', });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/browser-use/browser-use.routes.ts` around lines 65 - 68, The response handler for the browserUseService.listSessions() call is returning a 401 status code for any exception, but since authentication is already enforced upstream, failures from listSessions() are server/runtime errors and should return a 5xx status code instead. Change the res.status(401) call to res.status(500) to properly classify these errors as internal server errors rather than authentication failures.
🧹 Nitpick comments (1)
src/components/settings/view/tabs/browser-use-settings/BrowserUseSettingsTab.tsx (1)
24-30: ⚡ Quick winExtract duplicated
readJsoninto a shared API helper.This logic is duplicated with
src/components/browser-use/view/BrowserUsePanel.tsx(Lines 45-51). Centralizing it avoids future drift in error parsing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/settings/view/tabs/browser-use-settings/BrowserUseSettingsTab.tsx` around lines 24 - 30, The readJson function is duplicated across BrowserUseSettingsTab.tsx and BrowserUsePanel.tsx, creating maintenance risk if error handling logic needs to change. Create a new shared API helper module (e.g., in a utils or api directory) and move the readJson generic function there, then update both BrowserUseSettingsTab.tsx and BrowserUsePanel.tsx to import and use this centralized version instead of maintaining their own copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/browser-use/browser-use.service.ts`:
- Around line 487-491: The current condition in the registerAgentMcp block only
checks if next.enabled is true, causing MCP to be re-registered on every
settings update even when the enabled state hasn't changed. Replace the
condition to only register MCP when there's an actual state transition from
disabled to enabled by checking that current.enabled is false and next.enabled
is true. Similarly, ensure the unregister block only triggers when transitioning
from enabled to disabled by checking that current.enabled is true and
next.enabled is false.
---
Outside diff comments:
In `@server/modules/browser-use/browser-use.routes.ts`:
- Around line 65-68: The response handler for the
browserUseService.listSessions() call is returning a 401 status code for any
exception, but since authentication is already enforced upstream, failures from
listSessions() are server/runtime errors and should return a 5xx status code
instead. Change the res.status(401) call to res.status(500) to properly classify
these errors as internal server errors rather than authentication failures.
---
Nitpick comments:
In
`@src/components/settings/view/tabs/browser-use-settings/BrowserUseSettingsTab.tsx`:
- Around line 24-30: The readJson function is duplicated across
BrowserUseSettingsTab.tsx and BrowserUsePanel.tsx, creating maintenance risk if
error handling logic needs to change. Create a new shared API helper module
(e.g., in a utils or api directory) and move the readJson generic function
there, then update both BrowserUseSettingsTab.tsx and BrowserUsePanel.tsx to
import and use this centralized version instead of maintaining their own copies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c1ba50e-e220-48ac-910b-d62afa4145ce
📒 Files selected for processing (7)
server/modules/browser-use/browser-use.routes.tsserver/modules/browser-use/browser-use.service.tsserver/modules/browser-use/tests/browser-use.service.test.tsserver/modules/providers/index.tssrc/components/browser-use/view/BrowserUsePanel.tsxsrc/components/main-content/view/MainContent.tsxsrc/components/settings/view/tabs/browser-use-settings/BrowserUseSettingsTab.tsx
✅ Files skipped from review due to trivial changes (1)
- server/modules/providers/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/modules/browser-use/tests/browser-use.service.test.ts
- src/components/main-content/view/MainContent.tsx
Comment on lines
+487
to
+491
| if (next.enabled) { | ||
| await this.registerAgentMcp(); | ||
| } else if (current.enabled) { | ||
| await this.unregisterAgentMcp(); | ||
| await this.stopAllSessions(); |
Contributor
There was a problem hiding this comment.
Guard MCP registration on state transition only.
Line 487 currently re-registers MCP on every PUT /settings where enabled remains true. That can repeatedly call provider registration and create duplicate MCP entries/config churn.
Suggested fix
- if (next.enabled) {
+ if (next.enabled && !current.enabled) {
await this.registerAgentMcp();
- } else if (current.enabled) {
+ } else if (!next.enabled && current.enabled) {
await this.unregisterAgentMcp();
await this.stopAllSessions();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/browser-use/browser-use.service.ts` around lines 487 - 491,
The current condition in the registerAgentMcp block only checks if next.enabled
is true, causing MCP to be re-registered on every settings update even when the
enabled state hasn't changed. Replace the condition to only register MCP when
there's an actual state transition from disabled to enabled by checking that
current.enabled is false and next.enabled is true. Similarly, ensure the
unregister block only triggers when transitioning from enabled to disabled by
checking that current.enabled is true and next.enabled is false.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/browser-use/browser-use.service.ts (1)
299-314:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftSSRF vulnerability: Private/internal network access is unrestricted.
The
normalizeUrlfunction only validates the protocol but does not block navigation to private, loopback, or link-local addresses. This allows the browser automation to access internal resources:
- Cloud metadata endpoints (e.g.,
169.254.169.254)- Internal services on
localhostor127.0.0.1- Private network hosts (
10.x.x.x,192.168.x.x,172.16-31.x.x)Since agents can invoke
agentNavigatevia the MCP bridge, a malicious or misled agent could exfiltrate data from internal endpoints or probe the internal network.The library context indicates this protection was previously present but removed. Consider restoring the private-IP blocking logic or implementing request-level guards at the Playwright context level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/browser-use/browser-use.service.ts` around lines 299 - 314, The normalizeUrl function validates the protocol but does not prevent access to private or internal network addresses, creating an SSRF vulnerability. After parsing the URL with new URL(withProtocol), add validation logic to extract the hostname and check if it is a loopback address (localhost, 127.0.0.1), a link-local address (169.254.x.x), or in a private IP range (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16). Throw an error with a clear message if the hostname matches any of these patterns to prevent navigation to internal resources, cloud metadata endpoints, and private network hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/modules/browser-use/browser-use.service.ts`:
- Around line 299-314: The normalizeUrl function validates the protocol but does
not prevent access to private or internal network addresses, creating an SSRF
vulnerability. After parsing the URL with new URL(withProtocol), add validation
logic to extract the hostname and check if it is a loopback address (localhost,
127.0.0.1), a link-local address (169.254.x.x), or in a private IP range
(10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16). Throw an error with a clear message
if the hostname matches any of these patterns to prevent navigation to internal
resources, cloud metadata endpoints, and private network hosts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 86720c1a-8e86-47b6-8118-cbf13ccc9a95
📒 Files selected for processing (3)
server/modules/browser-use/browser-use.service.tsserver/modules/browser-use/tests/browser-use.service.test.tssrc/components/browser-use/view/BrowserUsePanel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/browser-use/view/BrowserUsePanel.tsx
- Rename Browser Use surfaces to Browser - Register Browser MCP under the new server name - Mark CloudCLI-managed MCP servers read-only - Adjust MCP stdio framing and sidebar footer sizing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exposes the Browser Use feature to coding agents as an MCP server, so agents (Claude, Codex, Cursor, Gemini, OpenCode) can drive a real browser — navigate, click, type, fill forms, take screenshots/snapshots, and manage tabs/sessions — directly from a chat session.
Browser Use is now opt-in: it stays disabled until enabled in Settings, and its configuration is persisted in the database.
What's included
server/browser-use-mcp.ts): a stdio JSON-RPC MCP server exposing 13 tools —browser_create_session,browser_list_sessions,browser_close_session,browser_navigate,browser_click,browser_type,browser_fill_form,browser_select_option,browser_press_key,browser_snapshot,browser_take_screenshot,browser_tabs,browser_wait_for.browser-use-mcp.routes.ts): the MCP server calls back into the app over/api/browser-use-mcp/tools/:toolName, authenticated with a bearer token (CLOUDCLI_BROWSER_USE_MCP_TOKEN) so only the local MCP process can invoke browser actions.browser-use.service.ts): when enabled, registers thecloudcli-browser-useMCP server with all supported providers and unregisters it when disabled; the token is generated/stored automatically.How it works
cloudcli-browser-useMCP server with each provider.Notes
Summary by CodeRabbit
Release Notes
cloudcli://URL protocol and new desktop build commands.