fix(chrome,edge): enforce configured token on page CDP WebSocket route - #5512
Draft
artiom wants to merge 1 commit into
Draft
fix(chrome,edge): enforce configured token on page CDP WebSocket route#5512artiom wants to merge 1 commit into
artiom wants to merge 1 commit into
Conversation
The chrome and edge page WebSocket routes set `auth = false`, so a configured TOKEN was not enforced on `/devtools/page/*` connections. A token-less client could open a page target and issue arbitrary CDP commands, while chromium (and the base route) correctly returned 401. Remove the `auth = false` overrides so chrome and edge inherit the base route's `auth = true`, making page CDP auth consistent across chrome, edge, chromium, and multi. To keep DevTools inspector links usable once the route enforces auth, thread the requesting token into the generated `devtoolsFrontendUrl` for `/json/list`, `/json/new`, and `/sessions` via new `makeExternalWebSocketURL` / `makeDevtoolsFrontendURL` helpers. The `webSocketDebuggerUrl` continues to exclude the token by design, so authenticated clients add their own authorization when connecting. Redact top-level and nested `ws`/`wss` tokens from request-URL logs and document the `devtoolsFrontendUrl` field as credential-bearing. Add non-vacuous regression tests for chrome and edge: the previous "rejects unauthorized" assertions lived only inside `catch`, so they passed without executing whenever the connection was wrongly accepted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughToken-aware URL helpers now support external WebSocket and DevTools URL construction, JSON endpoints propagate request tokens, and server logs redact credential-bearing query parameters. Chrome and Edge page WebSocket tests explicitly verify unauthorized rejection and authorized CDP access. ChangesToken-aware DevTools URLs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JSONRoute
participant BrowserManager
participant URLHelpers
Client->>JSONRoute: request JSON endpoint with token
JSONRoute->>BrowserManager: pass extracted token
BrowserManager->>URLHelpers: build external WebSocket and DevTools URLs
URLHelpers-->>BrowserManager: return rewritten URLs
BrowserManager-->>JSONRoute: return JSON payload
JSONRoute-->>Client: send token-aware payload
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/browsers/index.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated DevTools inspector base-URL construction into a shared helper. The same 4-line pattern (
new URL(externalAddress)+path.posix.join(pathname, '/devtools/inspector.html')) is copy-pasted in three places; a shared helper alongsidemakeExternalWebSocketURL/makeDevtoolsFrontendURLinsrc/utils.ts(e.g.makeDevtoolsInspectorURL(externalAddress: string): URL) would remove the duplication and prevent the three sites from drifting if the inspector path ever changes.
src/browsers/index.ts#L331-335: replace the inlinedevtoolsFrontendURLconstruction ingetJSONListwith the shared helper.src/browsers/index.ts#L406-410: replace the inlinefrontendURLconstruction ingenerateSessionJsonwith the shared helper.src/shared/json-new.http.ts#L65-69: replace the inlinefrontendURLconstruction in the/json/newhandler with the shared helper.♻️ Proposed helper
+export const makeDevtoolsInspectorURL = (externalAddress: string): URL => { + const frontendURL = new URL(externalAddress); + frontendURL.pathname = path.posix.join( + frontendURL.pathname, + '/devtools/inspector.html', + ); + return frontendURL; +};🤖 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/browsers/index.ts` at line 1, Extract the repeated DevTools inspector URL construction into a shared makeDevtoolsInspectorURL helper alongside makeExternalWebSocketURL and makeDevtoolsFrontendURL in src/utils.ts. Update getJSONList, generateSessionJson, and the /json/new handler in json-new.http.ts to use this helper, preserving the existing URL and inspector path behavior.
🤖 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.
Nitpick comments:
In `@src/browsers/index.ts`:
- Line 1: Extract the repeated DevTools inspector URL construction into a shared
makeDevtoolsInspectorURL helper alongside makeExternalWebSocketURL and
makeDevtoolsFrontendURL in src/utils.ts. Update getJSONList,
generateSessionJson, and the /json/new handler in json-new.http.ts to use this
helper, preserving the existing URL and inspector path behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58665033-44e1-413a-82ba-5fbab270bd42
📒 Files selected for processing (14)
src/browsers/index.tssrc/routes/chrome/tests/page-websocket.spec.tssrc/routes/chrome/ws/page.tssrc/routes/chromium/tests/json.spec.tssrc/routes/chromium/tests/websocket.spec.tssrc/routes/edge/tests/page-websocket.spec.tssrc/routes/edge/ws/page.tssrc/routes/management/http/sessions.get.tssrc/server.tssrc/shared/json-list.http.tssrc/shared/json-new.http.tssrc/types.tssrc/utils.spec.tssrc/utils.ts
💤 Files with no reviewable changes (2)
- src/routes/edge/ws/page.ts
- src/routes/chrome/ws/page.ts
artiom
marked this pull request as draft
July 22, 2026 17:00
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
1 participant
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.
Problem
When the service is started with a
TOKEN, the chrome and edge page CDP WebSocket routes did not enforce it. Both routes overrode the base route withauth = false, so the server's token check was skipped for/devtools/page/*upgrades.Effect on a token-protected instance:
Runtime.evaluate,Page.navigate).chromiumand the base route were unaffected — they returned401for the same request.The
auth = falseoverride was originally added so the token-lessdevtoolsFrontendUrlinspector link kept working; the fix preserves that link while restoring auth.Changes
auth = falseoverrides on the chrome and edge page WebSocket routes so both inherit the base route'sauth = true. Page CDP auth is now consistent across chrome, edge, chromium, and multi.devtoolsFrontendUrlfor/json/list,/json/new, and/sessions, via newmakeExternalWebSocketURL/makeDevtoolsFrontendURLhelpers.webSocketDebuggerUrlcontinues to exclude the token by design (clients add their own authorization when connecting)./json/listbuilt its inspector link from Chrome's own (Google-hosted) frontend with the nested WebSocket pointing at the internal browser port — not reachable by an external client. It is now self-hosted and externally routable, with sub-path prefixes preserved.token, and tokens nested in DevToolsws/wssparams) without mutating the request used for auth/routing. DocumentdevtoolsFrontendUrlas credential-bearing.catchblock, so they passed without executing whenever the connection was wrongly accepted. New tests fail if a token-less page connection is accepted while a token is configured, and confirm the authorized connection still runs CDP. Added coverage for the token-carrying DevTools URLs (/json/list,/json/new,/sessions) and the URL helpers.Verification
Built and run locally in Docker (chromium; chrome and edge validated on
linux/amd64):401on chrome, edge, and chromium (previously accepted on chrome/edge).devtoolsFrontendUrlfrom/json/newand/json/listloads the inspector asset and connects with the embedded token; the same target with the token stripped is refused.TOKENconfigured, behavior is unchanged (page connections and/json/listwork without a token).Behavior change (worth a release note)
On token-protected
chrome/edge/multi, a client that connects to a raw page target (/devtools/page/*) without a token now receives401. This matcheschromium's existing behavior.webSocketDebuggerUrlhas always excluded the token, so authenticated clients must include their authorization when connecting to it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security
Documentation