feat(browser): add per-start (unauthenticated) proxy server support - #3420
feat(browser): add per-start (unauthenticated) proxy server support#3420cofyc wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/mcp-integrations/browser-backend/proxy-auth.ts | 新增 CDP 代理认证与失败关闭逻辑，但自动附加事件未纳入启动等待链。 |
| apps/desktop/src/main/mcp-integrations/browser-backend/external-chrome-backend.ts | 新增按路由管理的 Chrome 生命周期和阻断状态；启动提交仍依赖一次可能过早的阻断检查。 |
| packages/browser-control-runtime/src/proxy.ts | 集中实现代理 URL 解析、规范化、路由键和脱敏支持。 |
| apps/desktop/src/main/messagePersistBroadcaster.ts | 在持久化及非可信边界前对 browser 代理凭据进行脱敏。 |
Sequence Diagram
sequenceDiagram
participant B as ExternalChromeBackend
participant C as ProxyAuthCoordinator
participant P as Chrome 页面
B->>C: start()
C->>C: Target.setAutoAttach
P-->>C: Target.attachedToTarget
C-->>C: void handleEvent()
C-->>B: start() 返回
B->>B: "检查 routeBlocked=false"
B-->>B: 提交代理路由并返回成功
C->>P: Fetch.enable
P-->>C: 失败
C->>B: onFailure()，阻断路由
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/main/mcp-integrations/browser-backend/proxy-auth.ts:336
**启动认证失败晚于路由提交**
当带凭据的代理启动期间自动附加新页面且该页面的 `Fetch.enable` 失败时，这里的 `handleEvent` 未被等待，`coordinator.start()` 因此可以先返回，后端随即提交代理路由并让 `start` 返回成功。异步失败随后才阻断路由，导致普通浏览器调用返回 `BROWSER_RUNTIME_UNAVAILABLE`，而 `status` 将刚报告可用的代理改为未知状态。
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (4): Last reviewed commit: "[browser] Add per-start proxy server sup..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb41e6022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1bb41e6 to
849754c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 849754c994
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MagicLizi
added
touches:core
849754c to
4102dd2
Compare
|
这条 PR 给 Browser start 加了按次 proxyServer，属于产品能力与核心路径上的大改动，先走维护者确认，暂不合并。 请维护者在本 PR 上 Approve；如果要改，请 Request Changes，作者改完后再 Approve。讨论 issue：#3425 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4102dd2463
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4102dd2 to
b9940ab
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9940ab1b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b9940ab to
55f7e62
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55f7e62667
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
55f7e62 to
fbb5b9c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb5b9c2e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@cofyc 👋 这个 PR 还有 1 条 review conversation 没 resolve(apps/desktop/src/main/messagePersistBroadcaster.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
fbb5b9c to
5e6d0e1
Compare
|
@cofyc 👋 这个 PR 还有 5 条 review conversation 没 resolve(apps/desktop/src/main/mcp-integrations/browser-managed-config.ts / apps/desktop/src/main/mcp-integrations/browser-backend/external-chrome-backend.ts / packages/browser-control-runtime/src/_generated/extension/src/browser/pw-tools-core.interactions.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76fe6fe850
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@cofyc 👋 这个 PR 还有 5 条 review conversation 没 resolve(apps/desktop/src/main/mcp-integrations/browser-managed-config.ts / apps/desktop/src/main/mcp-integrations/browser-backend/external-chrome-backend.ts / apps/desktop/src/main/mcp-integrations/browser-backend/proxy-auth.ts / packages/browser-control-runtime/src/_generated/extension/src/browser/chrome.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a743fa8bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0520943c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@cofyc 👋 这个 PR 还有 3 条 review conversation 没 resolve(apps/desktop/src/main/mcp-integrations/browser-managed-config.ts / apps/desktop/src/main/mcp-integrations/browser-backend/external-chrome-backend.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bcffcf58c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `--proxy-server=${proxyServer}`, | ||
| `--proxy-pac-url=data:application/x-ns-proxy-autoconfig;base64,${Buffer.from(pac, 'utf8').toString('base64')}`, |
There was a problem hiding this comment.
在 Chromium 将命令行代理配置解析为单一模式的环境中，--proxy-server 的固定代理配置会优先于同时提供的 --proxy-pac-url，因此这里生成的 allowlist PAC 根本不会执行（参见 Chromium 的命令行代理配置文档）。这样在 CDP coordinator attach 之前，持久 profile 恢复的标签页和 service worker 可以访问任意主机；attach 之后，未被 Fetch 拦截的 WebSocket 也会绕过 allowlist。只传 PAC 已足以让 browser-proxy-mode.ts:13-17 判定为显式代理，应避免同时下发互斥的固定代理配置。
Useful? React with 👍 / 👎.
|
@cofyc 👋 这个 PR 目前与 请在本地 merge 最新的 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98a41406f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cdpHttpUrl: 'http://127.0.0.1:18800', | ||
| managedUserDataDir: process.env.XDT_BROWSER_RUNTIME_DIR | ||
| ? nodePath.join(process.env.XDT_BROWSER_RUNTIME_DIR, 'browser', MANAGED_PROFILE, 'user-data') |
There was a problem hiding this comment.
当用户启用“使用我的浏览器登录态”时，运行时实际启动的是 Cindy-real，而且 pickManagedCdpPort() 会在 18800 被占用时选择 18801 等端口；这里却始终让协调器连接 18800 并按 Cindy/user-data 验证进程。因此端口发生迁移时每次代理启动都会在 Chrome 已启动后因无法连接守卫而失败；即使仍使用 18800，协调器断线后的 fresh-CDP／OS fail-closed 清理也会因 profile 身份不匹配而拒绝终止 Cindy-real，使现有页面在 Fetch 守卫消失后继续运行。应让这些值随本次 launch 的实际 profile 和 cdpPort 一起更新，而不是固定为隔离 profile 的身份。
Useful? React with 👍 / 👎.
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
独立审查（heavy，HEAD 98a41406）结论：代理路径上的 lifetime Fetch 门会被 Playwright page.route 挤掉，请修后再审。
packages/browser-control-runtime/src/_generated/extension/src/browser/pw-session.ts:1410/apps/desktop/src/main/mcp-integrations/browser-backend/proxy-auth.ts:600：P1 Chromium 只把 Fetch 交给一个 client。本 PR 已在MAINTAINING.md§4.1 写明 vendoredconnectOverCDP+page.route("**")会抢走该 client。gotoPageWithNavigationGuard仍在整段goto上装page.route("**")再unroute，等于在 Playwright session 上Fetch.enable/Fetch.disable。coordinator 的 lifetimeFetch.enable（handleAuthRequests: this.hasCredentials）因此在第一次工具导航时被挤掉且不会再武装。之后该页不再跑请求级 HTTPS + allowlist + DNS（isBrowserProxyRequestUrlAllowedAsync的 rebinding 半边），只剩 PAC 启动底线。PACdnsResolve只有 IPv4，allowlist 名字若有公网 A + 私网 AAAA、或之后 IPv6 rebind，不会按proxy.ts/ 注释声称的那样检查。请在explicit-browser-proxy模式不要用 Playwrightpage.route（只让 coordinator 当 Fetch client），或在unroute之后重新Fetch.enable；并加一条组合测试：同一 Chrome 上同时跑 coordinator +gotoPageWithNavigationGuard，断言goto之后requestPaused仍会触发。
相关单测与 desktop typecheck 通过，security-scan 无新增依赖；不能抵消这条默认 navigate 路径上的策略空洞。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
审查未通过（P1×1）
P1 apps/desktop/src/main/mcp-integrations/browser.ts:181
Proxied start 的 CDP 身份写死为 isolated Cindy / 端口 18800，没有跟随 Cindy-real 或搬迁后的 CDP 端口。ExternalChromeBackend 以 cdpHttpUrl: 'http://127.0.0.1:18800' 和 browser/Cindy/user-data 构造；PAC/extraArgs 会经 applyManagedConfig 落到真实进程，但 Fetch 门、liveness、adopted-close 与 fail-closed 杀进程仍对着 18800 + isolated 目录。
两条产品已支持路径会错位：
- 「使用我的浏览器登录态」启动
Cindy-real（18800 占用时还会换端口），lifetime 闸门挂在错误 listener 上。 - 18800 已被占用时
coordinator.start('http://127.0.0.1:18800')可能贴到残留 Chrome 并报告 proxied start 成功，真实窗口只剩 PAC；fail-closed kill 会因 user-data-dir 不匹配拒杀真进程。
请把当前活动 profile 的 CDP URL 和 browser/<activeProfile>/user-data 传入 coordinator；身份无法证明时 fail closed。补测：useRealProfile / pickManagedCdpPort 与 ExternalChromeBackend 共用同一 CDP URL 和 user-data-dir。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
P1×1（独立重审，HEAD 98a4140）
- [P1]
apps/desktop/src/main/mcp-integrations/browser.ts:181—ExternalChromeBackend的 CDP 守卫身份写死http://127.0.0.1:18800和browser/Cindy/user-data。开启「使用我的浏览器登录态」时，runtime 实际启动的是Cindy-real，且readCopiedLoginsCdpPort会把端口迁到 18801+。createManagedRuntime虽把真实cdpPort写进 config，coordinator 的start/ 领养关闭 / fail-closed 杀进程仍走构造时那份固定身份（external-chrome-backend.ts约 405、500、601 行；proxy-auth.ts的isManagedChromeProcess要求 user-data-dir 与 debug port 同时匹配）。结果：端口迁移后代理启动会在 Chrome 起来后因连不上守卫而失败；即便仍在 18800，断线清理也会因 profile 对不上而拒绝终止Cindy-real，Fetch 守卫消失后页面继续跑。请让这些值随本次 launch 的实际 profile 与cdpPort更新。
未把 PAC 与 --proxy-server 并存写成 P1：当前 Chromium ApplyProxyMode 先匹配 --proxy-pac-url。请在修身份时顺手确认受管 Chrome 实机确实在执行 PAC allowlist。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d53e4a6eac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (err) { | ||
| if (popupError === undefined) popupError = err; |
There was a problem hiding this comment.
When an interaction-created popup navigates to a URL rejected by the proxy/SSRF policy, assertObservedDelayedNavigations reaches assertPageNavigationCompletedSafely, which only quarantines the target and explicitly does not close it, while this catch merely records the error. Because the popup is still within the normal validation budget, none of the overflow close/context/browser teardown runs; the listener then detaches and the denied popup remains alive and executing, especially dangerously in direct mode where there is no lifetime Fetch gate. Close each denied popup with bounded fail-closed teardown before surfacing its error.
Useful? React with 👍 / 👎.
| let parsed: URL; | ||
| try { | ||
| parsed = new URL(`https://${hostname}`); | ||
| } catch { | ||
| throw invalidProxy('allowed hostnames must be valid DNS names'); |
There was a problem hiding this comment.
When proxyAllowedHostnames contains a value such as example..com, _foo.example.com, or -foo.example.com, WHATWG URL preserves it and these checks accept it even though it is not a valid public host name. If an existing browser is running, the input therefore passes preflight and triggers a destructive route transition, after which Chrome starts successfully but PAC/DNS resolution for the malformed allowlist entry fails, leaving the reported-success proxy route unusable and the previous tabs lost. Fresh evidence beyond the earlier length-limit issue is that syntactically invalid labels still pass; validate every label's nonempty LDH form before route switching.
Useful? React with 👍 / 👎.
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
P1×2，当前 head 按免密代理实现，但描述和安全门证据还停在认证代理那一轮。
-
[P1] packages/browser-control-runtime/src/types.ts:101 — 摘要已写「仅免密、带 userinfo 会在解析边界拒绝」，
parseBrowserProxyServer也会对 username/password 抛authenticated proxies are not supported，hasCredentials因此恒为 false。但「范围」仍写「HTTP 代理认证的 CDP Fetch challenge 处理」；「真实端到端验证」仍把 Chrome 151 + Basic-auth CONNECT、context.setHTTPCredentials、allowlist 内 HTTPS（认证代理）写成当前已执行且仍成立；风险段仍写「代理凭据仅保存在浏览器生命周期状态」。types.ts:101注释仍是Credentials are accepted only in-memory by the host.，与实现相反。请把范围/e2e 表/风险改成「仅免密；认证 URL 被拒绝；未再跑认证出口」，并改掉过期注释。验证：对照 PR 正文与parseBrowserProxyServer/hasCredentials；http://user:pass@host:port的start应得到解析错误。 -
[P1] packages/browser-control-runtime/src/_generated/extension/src/browser/pw-session.ts:1410 — 文档和 host 把 CDP Fetch 写成生命周期请求层终局门，但同一 target 上 vendored 导航仍
page.route("**")，非顶层/子 frame 文档请求走continueRouteSafely，不做 allowlist/DNS。作者自己用 Chrome 151 证明过同一 target 的 Fetch 事件只投给一个 CDP client；host 协调器单测是直接投喂伪造事件，绕过双 client 竞争。请在 proxied 模式下不要再page.route("**")（或 route handler 走同一套isBrowserProxyRequestUrlAllowedAsync），并补真实双 CDP client 回归：先 coordinator.start，再 Playwright route/goto，断言 host 仍收到 requestPaused，或明确改文档为「PAC + 文档导航守卫，Fetch 非终局」。不要只用向协调器 inject 的单测当通过证据。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
P1 × 1（不改不能合）
packages/browser-control-runtime/src/_generated/extension/src/browser/pw-session.ts:1410
本 PR 把 host 侧 CDP Fetch（proxy-auth.ts 的 Fetch.requestPaused → isBrowserProxyRequestUrlAllowedAsync）写成请求级终局门：HTTPS + allowlist + DNS 答案校验。同一 target 上 gotoPageWithNavigationGuard 仍无条件 page.route("**")。
Chrome 对同一 target 只把 Fetch 事件投给一个 CDP client（upstream/MAINTAINING.md §4.1 已用 Chrome 151 写明：Playwright page.route 会抢走该 client）。结果：
- 第一次工具导航即可挤掉 coordinator 的 Fetch；
unroute后也没有再Fetch.enable。 - route handler 对非顶层、非 subframe 文档请求直接
continueRouteSafely，不做 allowlist / DNS。 - 之后该页只剩 PAC 启动底线。PAC
dnsResolve是 IPv4-only；字面 IPv6（非::1/fe80:/ v4-mapped）在browser-managed-config.ts的 PACblocked()里return false不拦。allowlist 名若有公网 A + 私网 AAAA 或随后 IPv6 rebind，不会按proxy.ts/ 注释声称的那样检查。
现有 coordinator 单测是直接投喂伪造 requestPaused，本 diff 没有双 CDP client 回归。
请任选其一落地，并补真实双 client回归（先 coordinator.start，再 Playwright route/goto，断言 goto 之后 host 仍收到 requestPaused）：
- proxied 模式不要再用 Playwright
page.route，只让 coordinator 当 Fetch client；或 - route handler 走同一套
isBrowserProxyRequestUrlAllowedAsync，并在unroute后重新Fetch.enable。
不要只用向协调器 inject 的单测当通过证据。
|
@cofyc 👋 这个 PR 还有 4 条 review conversation 没 resolve（browser-managed-config.ts 的 如果你已经按评论改完或回应了，请到对应 thread 上点 Resolve conversation；全部 resolve 后，下一轮 auto-review 会自动重新审查这个 PR。 |
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
审查结论：Request Changes（P1×1）
- [P1] packages/browser-control-runtime/src/proxy.ts:192 —
parseBrowserProxyServer在解析边界拒绝带 userinfo 的代理（authenticated proxies are not supported），摘要也已收窄为「仅免密代理」。但 PR 描述仍与 HEAD 相反：- 「范围」仍写「HTTP 代理认证的 CDP Fetch challenge 处理」；
- 「真实端到端验证」仍写已用 Basic-auth CONNECT 代理，并给出「allowlist 内 HTTPS（认证代理）✅ 通过代理成功，代理日志出现带凭据的 CONNECT」；
- 「风险 / 影响范围」仍按「代理凭据仅保存在浏览器生命周期状态」叙述。
这违反 development-workflow.md：怎么验证的必须如实，description 必须与 diff 一致。审阅者会以为认证代理已可用，掩盖「认证通道会把凭据交给任意 WWW-Authenticate 站点」这一已回滚结论。
请按当前实现改正文：验证表改成免密 CONNECT 结果、删掉认证成功行，「范围」里的认证协调改成「请求层 CDP Fetch 门（无凭据）」，风险段不要再写会持有代理凭据。代码侧免密 fail-closed 本身没有 P0/P1。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
P1×1
- [P1] packages/browser-control-runtime/src/proxy.ts:192 — HEAD 在解析边界拒绝带 userinfo 的代理（
authenticated proxies are not supported），MCP start 同样走该解析。摘要已写明仅支持免密，但「范围」仍列出「HTTP 代理认证的 CDP Fetch challenge 处理」，「真实端到端验证」表仍写 allowlist 内 HTTPS（认证代理）成功且代理日志出现带凭据 CONNECT，风险段仍写代理凭据保存在浏览器生命周期 / CDP Fetch。按 Description 会以为认证代理已交付。请把范围、E2E 表、风险、关联需求全部改成与免密-only HEAD 一致，删除setHTTPCredentials/ 认证 CONNECT 成功表述。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
独立审查（head d53e4a6）发现 P1×2，请修完后再请维护者确认。
P1 packages/browser-control-runtime/src/types.ts:101 — PR 描述与 HEAD 不一致。代码已在 parse 边界拒绝带 userinfo 的代理（proxy.ts:192，authenticated proxies are not supported），摘要也写了仅免密代理；但「范围」仍列「HTTP 代理认证的 CDP Fetch challenge 处理」，「真实端到端验证」仍声称 Chrome 151 + Basic-auth CONNECT 成功、context.setHTTPCredentials、认证代理下 allowlist HTTPS 通过，「风险」仍写凭据保存在浏览器生命周期。types.ts:101 仍写 Credentials are accepted only in-memory by the host。审查者会以为认证代理已随本 PR 交付。请把范围 / 验证表 / 风险 / 类型注释改成与免密-only HEAD 一致；免密 Chrome e2e 若已跑请改表记录，未跑请写明未跑。
P1 packages/browser-control-runtime/src/_generated/extension/src/browser/pw-session.ts:1394 — gotoPageWithNavigationGuard 仍 page.route("**") 再 unroute。Chrome 每个 target 只有一个 Fetch client；host proxy-auth.ts:600 对网络 target Fetch.enable 后靠 Fetch.requestPaused + isBrowserProxyRequestUrlAllowedAsync 做生命周期 HTTPS/allowlist/DNS 门。第一次工具 goto 会抢走该 client，unroute 不会让协调器重新 enable（enableFetch 见 session 已登记就 return）。之后只剩 PAC + 导航守卫，paused 请求上的 DNS/rebinding 检查停止。单测直接给协调器喂假 requestPaused，没有和 Playwright 竞争。请在代理启动路径不要 page.route，或 unroute 后重新 Fetch.enable，并补两条 client 竞态测试。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
审查结论：Request Changes（P1×2）
独立重审 HEAD d53e4a6eac0f7cf88f6a99d0fa4037b602bba23e。免密 fail-closed 本身没有 P0；下面两条挡住合并。
-
[P1]
packages/browser-control-runtime/src/proxy.ts:192— 代码在解析边界拒绝带 userinfo 的代理（authenticated proxies are not supported），摘要也已收窄为仅免密。但 PR 描述仍与 HEAD 相反：「范围」仍写「HTTP 代理认证的 CDP Fetch challenge 处理」；「真实端到端验证」仍写 Chrome 151 + Basic-auth CONNECT、context.setHTTPCredentials、allowlist 内 HTTPS（认证代理）成功且代理日志出现带凭据 CONNECT；「风险」仍写代理凭据保存在浏览器生命周期。packages/browser-control-runtime/src/types.ts:101仍写Credentials are accepted only in-memory by the host.。这违反 development-workflow.md：怎么验证的必须如实，Description 必须与 diff 一致。请把范围 / E2E 表 / 风险 / 类型注释改成免密-only；认证 E2E 若已作废请删或标明历史。验证：对照正文与parseBrowserProxyServer；http://user:pass@host:port的start应得到解析错误。 -
[P1]
packages/browser-control-runtime/src/_generated/extension/src/browser/pw-session.ts:1410—gotoPageWithNavigationGuard仍page.route("**")再 unroute。Chrome 每个 target 只有一个 Fetch client；hostproxy-auth.ts:600对网络 targetFetch.enable后靠requestPaused+isBrowserProxyRequestUrlAllowedAsync做生命周期 HTTPS/allowlist/DNS 门。第一次工具 goto 会抢走该 client，unroute 不会让协调器重新 enable。之后只剩 PAC + 导航守卫。单测直接给协调器喂假 requestPaused，没有和 Playwright 竞争。请在代理启动路径不要page.route，或 unroute 后重新Fetch.enable，并补双 client 竞态测试。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
独立审查（heavy）有 1 条 P1，请先改 Description 再提审。
P1 packages/browser-control-runtime/src/proxy.ts:192
当前 head 已在解析边界拒绝带 userinfo 的代理（authenticated proxies are not supported），摘要也写明「仅免密」。但 Description 仍把已回滚的认证代理设计写成当前事实：
- 「本 PR 包含」仍列「HTTP 代理认证的 CDP Fetch challenge 处理」
- 「真实端到端验证」仍把 Playwright
context.setHTTPCredentials、认证 CONNECT 成功、凭据只在进程内存写成现网结论
作者评论已说明该方案会把代理凭据交给 allowlist 内任意 WWW-Authenticate 站点并已撤回。请按当前 head 重写范围与验证（免密 E2E、认证拒绝）；未再跑的项放到「未执行的验证」。代码本身这次不要求改。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
P1
PR body（范围 / 怎么验证的 / 风险）
HEAD d53e4a6e 在 packages/browser-control-runtime/src/proxy.ts:184-194 于解析边界拒绝一切 userinfo（authenticated proxies are not supported），生产路径不会再带凭据启动。摘要也写了「仅支持免密代理」。但同一份 Description 仍把「HTTP 代理认证的 CDP Fetch challenge 处理」列进范围；E2E 表仍写「allowlist 内 HTTPS（认证代理）✅ 通过代理成功，代理日志出现带凭据的 CONNECT」，并声称已修「认证代理此前完全不工作」；风险节仍写「代理凭据仅保存在浏览器生命周期状态」「原生 Agent transcript 可能记录原始输入；调用方应使用可轮换凭据」。这与 .github/PULL_REQUEST_TEMPLATE.md 和 docs/dev-rules/development-workflow.md「怎么验证的必须如实、没跑不许写已跑」以及「PR 描述与 diff 不符」不一致。影响：维护者会按仍支持认证代理来评估安全面，E2E 表也无法在当前 HEAD 复现。请把范围改成明确不包含认证代理；删掉或改写带凭据 CONNECT 的 E2E 与风险文案；契约注释 packages/browser-control-runtime/src/types.ts:101（「Credentials are accepted only in-memory」）一并改掉。验证：对照 body 与 parseBrowserProxyServer / MCP schema，确认不再出现「凭据进入生命周期 / 认证 CONNECT」表述。
P1
packages/browser-control-runtime/src/__tests__/interaction-proxy-policy.test.ts:533
同文件 :868。本 PR 自己的测试让该包 build（tsc --noEmit）失败：TS2345，mockImplementation 回调签名对不上 NormalizedProcedure<() => Promise<undefined>>。PR 写「pnpm --filter @cindy/browser-control-runtime build 结果：通过」，本 worktree 复跑 EXIT=2。Vitest 仍能跑过，所以这是类型门而非运行时红线，但广告出来的验证命令是假的，且失败文件在本 diff 内。请改 mock 签名（无参或与 NormalizedProcedure 一致）后再跑同一条 build。验证：pnpm --filter @cindy/browser-control-runtime build EXIT=0。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
审查未通过（P1×1）。描述与当前 diff 不一致，请改描述后再请审。
- [P1] PR body（「本 PR 包含」/「真实端到端验证」/「风险」）— 代码已在
parseBrowserProxyServer对 userinfo 抛authenticated proxies are not supported（headd53e4a6eac0f7cf88f6a99d0fa4037b602bba23e），但描述仍写：包含「HTTP 代理认证的 CDP Fetch challenge 处理」；Chrome 151 上认证代理已通过 Playwrightcontext.setHTTPCredentials；风险节仍写凭据保存在浏览器生命周期、userinfo 跨边界脱敏。违反docs/dev-rules/development-workflow.md：验证与范围必须如实。请按现行免密-only 范围改描述——删掉已回滚的认证 e2e 表与凭据生命周期表述；「本 PR 包含」改为 CDP Fetch 仅作请求层 allowlist/DNS 门（handleAuthRequests: false）。验证：对照packages/browser-control-runtime/src/proxy.ts的 userinfo 拒绝与apps/desktop/src/main/mcp-integrations/browser-backend/proxy-auth.ts的hasCredentials。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
Findings
-
[P1] packages/browser-control-runtime/src/proxy.ts:54 —
parseAllowedProxyHostname/assertDnsNameWithinLimits只拦长度，不拦空 label 与非法 LDH。example..com、_foo.example.com、-foo.example.com都能过预检。已有浏览器在跑时，这会先停掉旧进程再按「成功」切 route，随后 PAC/DNS 匹配失败，旧标签页被丢掉。请在切 route 之前按非空 LDH label 拒绝，并补测试。 -
[P1] packages/browser-control-runtime/src/_generated/extension/src/browser/pw-tools-core.interactions.ts:537 — 交互创建的 popup 命中 proxy/SSRF 拒绝时，
assertObservedDelayedNavigations→assertPageNavigationCompletedSafely只 quarantine、明确不 close；这里的 catch 只记下popupError。在预算内的拒绝弹窗不会走进 overflow close/teardown，listener detach 后页面仍在执行（direct 模式没有持续 Fetch 闸门时更危险）。请在上抛错误前对每个被拒绝的 popup 做有界 fail-closed close。
Codex 仍开着的 --proxy-server 与 --proxy-pac-url 互斥问题本轮未独立核实（文档写了双 flag 是有意设计），先保持 thread，不作为本条打回依据。
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
独立审查重核（head d53e4a6eac0f7cf88f6a99d0fa4037b602bba23e）仍有 2 条 P1，本轮不能合。Codex 未 resolve 的 PAC/--proxy-server thread 先保持，不作为本条打回依据。
-
[P1]
packages/browser-control-runtime/src/proxy.ts:54：parseAllowedProxyHostname/assertDnsNameWithinLimits只拦长度，不拦空 label 与非法 LDH。new URL('https://example..com')、_foo.example.com、-foo.example.com的hostname原样保留且能过预检。已有浏览器在跑时，这会先停掉旧进程再按「成功」切 route，随后 PAC/DNS 匹配失败，旧标签页被丢掉。请在切 route 之前按非空 LDH label 拒绝，并补测试。 -
[P1]
packages/browser-control-runtime/src/_generated/extension/src/browser/pw-tools-core.interactions.ts:544：交互创建的 popup 命中 proxy/SSRF 拒绝时，assertObservedDelayedNavigations→assertPageNavigationCompletedSafely只 quarantine、注释写明 never closes（pw-session.ts:1318）；这里的 catch 只记下popupError。预算内被拒绝的弹窗不会走进 overflow close/teardown，listener detach 后页面仍在执行（direct 模式没有持续 Fetch 闸门时更危险）。请在上抛错误前对每个被拒绝的 popup 调用closeBlockedNavigationTarget（或等价的有界 fail-closed close）。
请修完后在最新 head 上再请求审查。
Launch Chrome with a caller-selected proxy, a PAC allowlist floor (including PAC dnsResolve for WSS/pre-attach), and a lifetime CDP request gate. Authenticated proxies are rejected at parse time. The CDP guard identity follows the live managed profile: Cindy-real and a relocated CDP port are used for Fetch, liveness, and adopted-close instead of the isolated 18800/Cindy defaults. Signed-off-by: Yecheng Fu <yechengfu1987@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b54498c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await Promise.race([ | ||
| closeBlockedNavigationTarget({ | ||
| cdpUrl: opts.cdpUrl, | ||
| page: popupPage as Page, | ||
| }).catch(() => undefined), |
There was a problem hiding this comment.
当策略拒绝的弹窗其 closeBlockedNavigationTarget() 拒绝或超过 2 秒未完成时，这个 Promise.race 会把失败／超时都当作成功继续；随后 popupIndex 已等于队列长度，因而不会进入 context/browser teardown，监听器却仍被移除，direct 模式下该弹窗可继续执行与联网。相对此前评论的新证据是新增的有界关闭只验证了调用发生，测试也仅断言调用与 listener 移除，并未验证关闭完成；应把未确认关闭的情况升级到整 context/browser 及宿主可验证的 teardown。
AGENTS.md reference: AGENTS.md:L30-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b54498c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new Promise<void>((resolve) => { | ||
| const timer = setTimeout(resolve, POPUP_CHAIN_CLOSE_BUDGET_MS); | ||
| (timer as unknown as { unref?: () => void }).unref?.(); | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
当一个未触发 overflow 的弹窗因策略校验失败、且 closeBlockedNavigationTarget() 超过两秒仍未完成时，这个 timer 会让 Promise.race 按成功完成继续执行；随后 popupIndex 已经前进，后面的 overflow teardown 分支不会运行，最终抛出的仍是普通导航拒绝，也不会触发 Desktop 对 could not be torn down 的进程级清理。相对此前“关闭被拒弹窗”的评论，新证据是新增的有界关闭仍未区分“确已关闭”和“计时器先完成”，因此 direct 模式下被拒弹窗仍可在监听器移除后继续执行和联网。
Useful? React with 👍 / 👎.
| }), | ||
| ]); | ||
| } | ||
| if (!browserClosed) { |
There was a problem hiding this comment.
当 overflow 清理走到 browser.close() 且该调用返回、但 Chrome 进程尚未真正退出或仍有页面存活时，browserClosed 会被置为 true，这里便跳过唯一会生成 could not be torn down 信号的分支；Desktop 因而不会执行其已验证的进程 teardown。相对此前浏览器关闭验证线程，新证据是当前相邻注释已经明确承认 resolved close() 只证明调用返回，却仍把该结果当作完成隔离并立即移除监听器。
Useful? React with 👍 / 👎.
|
@cofyc 👋 这个 PR 目前与 请在本地 merge 最新的 |
4 participants
这次改了什么
摘要
为 Cindy Browser 的
start操作增加可选的proxyServer，让调用方可以按次选择直连或免密代理出口，同时保留现有安全自动化接口。代理配置由 Desktop 的外置浏览器适配层持有并串行切换：相同路由重复启动保持幂等，路由变化会先停止再重启；代理解析、连接或启动失败时不会回退直连。由于不接受凭据，代理凭据不可能进入 Chrome 参数、工具结果、状态、日志或错误文本。
代理启动额外带
--webrtc-ip-handling-policy=disable_non_proxied_udp，避免页面用 WebRTC 绕过代理暴露本机真实 IP。CDP 守卫身份跟随本次 launch 的真实 profile / 端口（含 Cindy-real 与迁出 18800 的情况）。变更类型
feat新功能fix缺陷修复refactor/perf重构或性能优化docs/test/chore文档、测试或工程维护范围
browserMCP schema 与中立请求契约支持start.proxyServer（免密 URL）dnsResolve私网闸门，覆盖 WSS / pre-attach）Fetch.authRequired、PlaywrightsetHTTPCredentials）{ "action": "start", "proxyServer": "http://host:port" }启动外置 Cindy Browser；带凭据的 URL 会被拒绝。start与status返回脱敏后的有效代理模式。UI 变化
不涉及。
怎么验证的
自动验证
GitHub CI（HEAD
d53e4a6ea）：Linux / Windows unit、DCO、verify 全绿。手工验证
已验证构建后的 MCP schema 能暴露
proxyServer字段。未使用生产代理或凭据。真实端到端验证（已执行，免密代理）
已用真实 Chrome 151 + 本地免密 CONNECT 代理执行端到端验证（macOS，
--headless=new，独立 CDP 端口，未触碰本机既有受管浏览器）。requires an explicit public-hostname allowlistproxied browser navigation requires HTTPSproxyServerauthenticated proxies are not supportedstopstopped: true，路由回到 unknown缺陷（已修，与认证无关）： per-start allowlist 曾把浏览器自己的 loopback CDP 端点拦掉，每次导航都以
browser endpoint blocked by policy失败。已修（一次性豁免同时满足两个字段），并补回归测试。验证边界：仅 macOS + Chrome 151 +
--headless=new。未执行的验证
context.setHTTPCredentials能应答 407，但会把代理凭据交给 allowlist 内任意WWW-Authenticate站点，该方案已撤回，不得当作现网结论。风险
风险分类
影响与回滚
proxyServer时明确拒绝。本 PR 不接受、不存储、不转发代理凭据——带 userinfo 的 URL 在解析边界失败，因此也不会进入 Chrome 参数、Cindy 持久配置、会话数据库或 renderer/device-link。proxyServer时仍显式使用--no-proxy-server。提交前检查
git commit -s，见 DCO）远程与手机版适配结论
cindy_browser的外置受管浏览器仍由本机 Desktop host 启动，不读取 workdir 文件；现有远程 Codex 不提供本地 lizi MCP 工具，因此无需 remote-file-service 或 cc-manager 适配。