fix(desktop): parse NO_PROXY ports strictly - #5498
Conversation
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: This fixes a real correctness bug — On approach: Scope is tight and minimal — one new Moving on to code review and testing. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:修复了一个真实的正确性 bug—— 方案:范围紧凑且最小化——新增一个 进入代码审查和测试。🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: add a strict The PR's approach matches this exactly. Formatting changes (arrow parens, line wrapping) are Prettier compliance per the root config ( Unit tests: 16/16 pass on the PR branch (3 new tests added covering malformed ports and IPv6 port-scoped rules). 13/13 pass on main (pre-fix baseline, without the new test cases). Before/After TestingBefore (main branch — bug present)After (PR #5498 — fix applied)Both bugs fixed, no regressions on valid entries. 中文说明代码审查独立方案:添加严格的 PR 的方案与此完全一致。 格式化改动(箭头函数括号、换行)是根配置 Prettier 合规性( 单元测试: PR 分支 16/16 通过(新增 3 个测试覆盖畸形端口和 IPv6 端口规则)。main 基线 13/13 通过(无新测试用例)。 前后对比测试两个 bug 均已修复,合法条目无回归。 — Qwen Code · qwen3.7-max |
|
This PR is a textbook minimal bugfix. The problem is real ( The Tests are thorough: 3 new cases covering malformed host:port, malformed IPv6 port, and valid IPv6 port-scoped rules. All 16 pass. No concerns. Approving. 中文说明这个 PR 是教科书级的最小 bugfix。问题真实存在(
测试充分:3 个新用例覆盖畸形 host:port、畸形 IPv6 端口、合法 IPv6 端口规则。全部 16 个测试通过。 没有顾虑。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local verification (real build + tests via tmux) — recommend mergeI verified this PR end-to-end on a real checkout (isolated
What the fix doesThe old parser used Why it's safe / inert: a malformed entry is kept as its full string (e.g. Results
Decisive evidence
Minor notes (non-blocking)
VerdictCorrect, minimal, and security-positive. Fixes #5497, closes a broad class of malformed- 🇨🇳 中文版(点击展开)✅ 本地真实构建 + 测试验证(tmux)— 建议合并我在真实检出环境中端到端验证了本 PR:在 PR head
修复内容: 旧逻辑用 为何安全 / 失效化(inert): 畸形条目被原样保留为完整字符串(如 结果:
关键证据:
补充(非阻塞):
结论: 改动正确、最小、且对安全有正向作用。修复 #5497,关闭了一整类畸形 |
| wildcard: boolean; | ||
| } | ||
|
|
||
| function parsePort(raw: string): number | undefined { |
There was a problem hiding this comment.
[Suggestion] After /^\d+$/ regex validation, Number(raw) always produces a non-negative integer — Number.isInteger() and port >= 0 are guaranteed true. Only port <= 65535 carries decision value.
| function parsePort(raw: string): number | undefined { | |
| return port <= 65535 ? port : undefined; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const port = parsed.port | ||
| ? parseInt(parsed.port, 10) | ||
| : DEFAULT_PORTS[parsed.protocol]; | ||
|
|
There was a problem hiding this comment.
[Suggestion] The PR converts both parseInt sites in parseNoProxyRules to the strict parsePort(), but shouldBypassProxy still uses bare parseInt(parsed.port, 10). While safe today (URL constructor validates ports at construction time), the inconsistency invites future copy-paste regressions. Consider replacing with parsePort() or adding a comment explaining why parseInt is safe here.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
What this PR does
Tightens desktop
NO_PROXYport parsing so only clean decimal port suffixes become port-scoped bypass rules. Malformed entries likeexample.com:443abcor[::1]:abcno longer create active proxy bypasses, while valid host:port and bracketed IPv6 port rules still work.Why it's needed
The previous parser used
parseInt, soexample.com:443abcbecame a validexample.com:443bypass. Bracketed IPv6 had a second failure mode:[::1]:abcfell through as a host-only rule and bypassed every::1port.Reviewer Test Plan
How to verify
Run the focused desktop proxy test and confirm malformed port suffixes do not bypass while valid IPv6 port rules still match.
Commands run locally:
bun test apps/electron/src/main/__tests__/network-proxy.test.tsfrompackages/desktopbunx prettier --check apps/electron/src/main/network-proxy-utils.ts apps/electron/src/main/__tests__/network-proxy.test.tsfrompackages/desktopbunx eslint --config eslint.config.mjs src/main/network-proxy-utils.ts src/main/__tests__/network-proxy.test.tsfrompackages/desktop/apps/electrongit diff --checkEvidence (Before & After)
Before:
example.com:443abcparsed as port443and bypassedhttps://example.com;[::1]:abcdegraded into an all-ports::1bypass.After: the same malformed rules stay non-matching, while
[::1]:3000still matches only port3000.Tested on
Environment (optional)
macOS local checkout, Bun desktop workspace tests.
Risk & Scope
NO_PROXYentries that previously matched accidentally will stop bypassing the proxy.Linked Issues
Fixes #5497
中文说明
这个 PR 做了什么
收紧 desktop
NO_PROXY端口解析,只有干净的十进制端口后缀才会变成带端口限制的绕过规则。example.com:443abc或[::1]:abc这类畸形条目不再创建有效 proxy bypass;合法的 host:port 和方括号 IPv6 端口规则仍然可用。为什么需要
之前的解析逻辑用了
parseInt,所以example.com:443abc会变成合法的example.com:443绕过规则。方括号 IPv6 还有第二个失败模式:[::1]:abc会退化成 host-only 规则,从而绕过所有::1端口。Reviewer Test Plan
如何验证
运行 focused desktop proxy test,确认畸形端口后缀不会 bypass,同时合法 IPv6 端口规则仍然匹配。
本地执行过的命令:
bun test apps/electron/src/main/__tests__/network-proxy.test.ts,目录为packages/desktopbunx prettier --check apps/electron/src/main/network-proxy-utils.ts apps/electron/src/main/__tests__/network-proxy.test.ts,目录为packages/desktopbunx eslint --config eslint.config.mjs src/main/network-proxy-utils.ts src/main/__tests__/network-proxy.test.ts,目录为packages/desktop/apps/electrongit diff --check前后证据
Before:
example.com:443abc被解析成端口443,会绕过https://example.com;[::1]:abc会退化成所有端口的::1绕过。After:同样的畸形规则不会匹配;
[::1]:3000仍然只匹配端口3000。测试平台
环境
macOS 本地 checkout,Bun desktop workspace tests。
风险和范围
NO_PROXY条目,现在不会再绕过代理。关联 Issue
Fixes #5497
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.