fix(cli): reuse base URL sanitizer for provider warnings - #8524
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
CI note: the |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Thanks for the PR — and for the honest wrap-up on #8137 that led to this one.
Moving on to code review. 🔍 中文说明感谢这个 PR——也感谢你在 #8137 收尾时的坦诚，促成了这个版本。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal before reading the diff: the wrapper's whole-span I statically traced the shared sanitizer (unchanged on
No blockers. One observation, which is not a defect in this PR: when a span's URL has no Also checked: the deleted helpers ( Test evidence (PR's own CI at the reviewed commit, fetched once via API — no polling)At fetch time the two suites that matter are still running: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查读 diff 之前的独立方案：包装层对整个片段做 由于整个变更就是"现有片段循环 + 那一个共享函数"的组合，我对
无阻塞项。一个观察，不算本 PR 的缺陷：当片段中的 URL 在 authority 之后没有 另已核查：被删除的辅助函数（ 测试证据（受审提交上该 PR 自身的 CI，通过 API 一次性获取，不轮询）获取时两个关键套件仍在运行： （CI 表格见英文版，finalize 任务会在 CI 完成后原地更新。） — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix in the exact shape requested on the issue; the only open item is the unit suite still running, plus the known, explicitly deferred shared-sanitizer limitation (named in the Stage 2 comment). Stepping back: this is what good iteration looks like. The first attempt (#8137) tried to make the warning wrapper smarter and accumulated regressions across six review rounds; the author closed it themselves, verified every finding against the merge base first, and this PR goes the other way — it deletes the duplicate heuristics entirely and leans on the authority-aware sanitizer that already existed. The diff matches my independent proposal exactly, the problem was reproduced and maintainer-confirmed, the fix removes 31 production lines while adding pinned input→output tests for both reported shapes, and every edit is necessary for the stated goal. In six months this reads as obviously correct: there is now exactly one place where credentials get stripped from provider URLs. The Serve A/B daemon diff already came back clean; once the unit suite lands green, this is ready. Approval is deferred until CI lands green on 中文说明信心：4/5 —— 干净、最小的修复，形态与 issue 中要求的完全一致；唯一未决项是单元测试套件仍在运行，外加那个已知且已明确延后处理的共享清理函数局限（见 Stage 2 评论）。 退一步看：这是良性迭代的样板。第一次尝试（#8137）试图让 warning 包装层更聪明，在六轮 review 中不断累积回归；作者先对合并基点逐一验证了所有发现，然后自己关掉了它，而这个 PR 走了反方向——彻底删除重复的启发式，依赖已有的识别 authority 边界的清理函数。diff 与我的独立方案完全一致；问题有复现且经维护者确认；修复净删 31 行生产代码，同时为两个上报形态新增了钉住输入→输出的测试；每一处改动都为既定目标所必需。六个月后再看，这段代码显然是对的：现在只有一个地方负责从 provider URL 中剥离凭据。Serve A/B 的 daemon 差分已经返回干净结果；单元测试套件变绿后即可合入。 批准延迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
| const segment = warning.slice(next.index, segmentEnd); | ||
| result += sanitizeProviderWarningSegment(segment, next.marker.length); | ||
| result += sanitizeProviderBaseUrl(segment); |
There was a problem hiding this comment.
[Critical] The whole warning segment (URL + trailing prose to end-of-line) is now passed to sanitizeProviderBaseUrl, whose input contract everywhere else is a single standalone base URL. When the URL is pathless (no /, ?, or # after the host), findAuthorityEnd finds no authority terminator, so the "authority" spans the entire remainder of the line including the prose, and the credential strip lands on an unrelated later @ (e.g. a contact email). Independently, when the URL fails WHATWG parsing (even with a path), the catch-branch fallback findUnescapedUserInfoFallbackAt resolves @ via whole-string lastIndexOf — past the correctly-bounded authority and into the prose. — Failure scenario: a provider warning or ModelsConfig construction error of the shape Cannot reach https://api.example — contact admin@example.com (pathless base URL + a contact email on the same line) reaches sanitizeProviderWarning via the warnings map or the catch path; this PR then outputs Cannot reach https://example.com — the host is replaced by the email's domain and the contact text is deleted (probe-verified A/B: the merge base kept this input byte-identical). Credentialed variant Cannot reach https://user:pass@host.io please contact admin@corp.io → Cannot reach https://corp.io; unparseable variant Cannot reach https://host bad:x/v1 — contact admin@corp.io → Cannot reach https://corp.io. The three new tests pass only because their inputs have a correctly-bounded, parseable authority; the corrupting shapes are untested. The harm is message corruption (over-strip), not a credential leak.
| const segment = warning.slice(next.index, segmentEnd); | |
| result += sanitizeProviderWarningSegment(segment, next.marker.length); | |
| result += sanitizeProviderBaseUrl(segment); | |
| const segment = warning.slice(next.index, segmentEnd); | |
| const urlBreak = segment.search(/[\s'"`<>]/); | |
| const urlToken = urlBreak === -1 ? segment : segment.slice(0, urlBreak); | |
| const trailing = urlBreak === -1 ? '' : segment.slice(urlBreak); | |
| result += sanitizeProviderBaseUrl(urlToken) + trailing; |
中文说明
现在整个 warning 片段（URL + 一直到行尾的后续文字）被直接传入 sanitizeProviderBaseUrl，而该函数在其他调用点的输入契约是单个独立的 base URL。当 URL 没有路径（host 后没有 /、?、#）时，findAuthorityEnd 找不到 authority 终止符，因此 "authority" 会延伸到该行剩余的全部内容包括后续文字，凭据截取就会落在后面某个无关的 @（例如联系邮箱）上。此外，当 URL 无法被 WHATWG 解析时（即使有路径），catch 分支的回退逻辑 findUnescapedUserInfoFallbackAt 会通过整个字符串的 lastIndexOf 定位 @ —— 超出正确界定的 authority 而落到后续文字中。—— 失败场景：形如 Cannot reach https://api.example — contact admin@example.com（无路径 base URL + 同一行的联系邮箱）的 provider warning 或 ModelsConfig 构造错误，经由 warnings map 或 catch 路径进入 sanitizeProviderWarning；本 PR 会输出 Cannot reach https://example.com —— host 被替换为邮箱的域名，联系文字被删除（探针 A/B 验证：merge base 对该输入逐字节保持不变）。带凭据变体 Cannot reach https://user:pass@host.io please contact admin@corp.io → Cannot reach https://corp.io；不可解析变体 Cannot reach https://host bad:x/v1 — contact admin@corp.io → Cannot reach https://corp.io。三个新增测试之所以全部通过，仅因为其输入的 authority 界定正确且可解析；导致损坏的形态未被测试覆盖。危害是消息被破坏（过度截取），而非凭据泄露。
建议修复：在委托给 sanitizeProviderBaseUrl 之前，先把输入限定到 URL token（在第一个空白/引号/反引号/尖括号处截断），只对 URL token 做清理，并把后续文字原样拼接回去（等价于恢复被删除的 URL_LIKE_PATTERN 的应用方式）。也可考虑在 findAuthorityEnd 中把空白视为 authority 终止符（真实 base URL 不含空白），并补充"无路径 URL + 后续邮箱"的回归用例。
— qwen3.8-max via Qwen Code /review (v0.21.5)
github-actions
Bot
added
the
review/self-reported
|
Good catch — I hit the same thing when I ran sanitizeProviderBaseUrl against the pathless shapes locally. Went with the findAuthorityEnd change rather than trimming the segment at the first whitespace before calling it. Trimming first would leave Whitespace is now an authority terminator (raw whitespace isn't valid in a URL anyway). Node's URL parser still accepts a space in userinfo by percent-encoding it, so when |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
The review timeout looks like infra on the bot run, not a code failure — the test matrix on d8f6033 is green (ubuntu Node 22, Serve A/B, desktop shell, Java matrix). Happy to re-kick review on this head if maintainers want another bot pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // WHATWG accepts spaces in userinfo (encoded as %20), but our authority | ||
| // span stops at raw whitespace — so `@` can sit past authorityEnd for a | ||
| // password that contains a space. Fall through to the same recovery used | ||
| // when parsing fails. |
There was a problem hiding this comment.
[Critical] R2-1: The parse-success fall-through leaks credentials for whitespace-containing userinfo. When WHATWG parsing succeeds and reports username/password but the credential @ sits past the new whitespace-truncated authority span, control falls through to findUnescapedUserInfoFallbackAt, whose guards bail for these shapes — so the input is returned unchanged with credentials intact. A/B-probed against the merge base, which stripped them.
Failure scenario: sanitizeProviderBaseUrl('https://user:12 34@host.io') → returned byte-identical (password prefix 12 matches the /^\d+$/ port guard and is mistaken for a port); https://user name:pass@host.io (colon past authorityEnd) and https://user name@host.io / https://us er@host (no colon) leak the same way. Warning path: Cannot reach https://user:12 34@host.io leaks unchanged into serve status errors[].error. Reachable via configured baseUrl (settings schema constrains it to a plain string, so a WHATWG-valid URL with a space in userinfo can be configured) and via provider warning/error text routed through sanitizeProviderWarning.
Suggested fix: when parsed.username || parsed.password is set, WHATWG has already proven userinfo exists — strip at the userinfo-terminating @ directly instead of re-deriving it through the parse-failure heuristics (the colon/digit guards only disambiguate the genuinely ambiguous parse-failure case). Add regression rows for the leaking shapes.
中文说明
解析成功时的 fall-through 会泄露包含空白的 userinfo 凭据。当 WHATWG 解析成功并报告 username/password、但凭据中的 @ 位于新的"以空白截断 authority"范围之后时，控制流落入 findUnescapedUserInfoFallbackAt，其守卫对这些形态直接退出 —— 输入被原样返回，凭据完整保留。已对 merge base 做 A/B 探针验证：旧代码会剥掉这些凭据。
失败场景：sanitizeProviderBaseUrl('https://user:12 34@host.io') → 原样返回（密码前缀 12 命中 /^\d+$/ 端口守卫，被误判为端口）；https://user name:pass@host.io（冒号在 authorityEnd 之后）以及 https://user name@host.io / https://us er@host（没有冒号）以同样方式泄露。warning 路径：Cannot reach https://user:12 34@host.io 原样泄露进 serve status 的 errors[].error。可通过配置的 baseUrl（settings schema 仅约束为普通字符串，因此带空白的 WHATWG 合法 URL 可以被配置进来）以及经由 sanitizeProviderWarning 的 provider warning/错误文本到达。
建议修复：当 parsed.username || parsed.password 成立时，WHATWG 已证明 userinfo 存在 —— 直接在 userinfo 结尾的 @ 处剥离，而不是通过解析失败专用的启发式重新推导（冒号/数字守卫只应在真正有歧义的解析失败分支中使用）。并为这些泄露形态补充回归用例。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const fallbackAt = findUnescapedUserInfoFallbackAt( | ||
| baseUrl, | ||
| authorityStart, | ||
| authorityEnd, | ||
| ); |
There was a problem hiding this comment.
[Critical] R2-2: The promoted fallback's unbounded baseUrl.lastIndexOf('@') is now reachable from the parse-success path. A whitespace-in-password URL followed by any later @ after the authority (a contact email in trailing prose, or an @ in the path) is stripped at the LATER @ — destroying the real host, path, and prose. This is the same over-strip class this PR exists to fix, reintroduced for the space-in-password variant via the new routing.
Failure scenario (A/B-probed end-to-end through the status provider): warning Failed loading provider https://user:pa ss@host.io please contact admin@corp.io → merge base produced …https://host.io please contact admin@corp.io, this PR produces Failed loading provider https://corp.io — the host is replaced by the contact email's domain and the contact text is deleted; https://user:p w@host.io/a@b → https://b (regresses on both the direct and the warning paths). The surfaced error names the wrong domain as the unreachable provider — actively misleading diagnostics from the function whose purpose is safe display.
Suggested fix: bound the fallback's search — strip at the first @ at or after authorityEnd (baseUrl.indexOf('@', authorityEnd)) instead of the last @ in the whole string, and/or reserve the whole-string recovery for the parse-failure path only. Probes confirmed this preserves the port/email guard cases while restoring the host for these shapes.
中文说明
被提升为公共路径的 fallback 中无界的 baseUrl.lastIndexOf('@') 现在可以从解析成功路径到达。密码含空白的 URL 后面如果跟着 authority 之后的其他 @（后续文字中的联系邮箱，或路径中的 @），会在后面那个 @ 处剥离 —— 真实的 host、路径和说明文字全部被破坏。这正是本 PR 要修复的过度截取类别，经由新的路由在"密码含空白"变体上被重新引入。
失败场景（已通过 status provider 端到端 A/B 探针验证）：warning Failed loading provider https://user:pa ss@host.io please contact admin@corp.io → merge base 输出 …https://host.io please contact admin@corp.io，本 PR 输出 Failed loading provider https://corp.io —— host 被联系邮箱的域名替换，联系文字被删除；https://user:p w@host.io/a@b → https://b（直接调用路径和 warning 路径均回退）。对外展示的错误会把错误的域名说成不可达的 provider —— 在一个以安全展示为目的的函数里产生误导性诊断。
建议修复：限定 fallback 的搜索范围 —— 在 authorityEnd 之后的第一个 @（baseUrl.indexOf('@', authorityEnd)）处剥离，而不是整个字符串的最后一个 @；或者只把全串恢复逻辑保留在解析失败路径。探针确认这样既保留端口/邮箱守卫用例的行为，又能恢复这些形态下的 host。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| [ | ||
| 'https://user:pass@host.io please contact admin@corp.io', | ||
| 'https://host.io please contact admin@corp.io', | ||
| ], |
There was a problem hiding this comment.
[Suggestion] R2-3: The stripping side of the new fall-through is unpinned by tests. In the added row 'https://user:pass@host.io please contact admin@corp.io' the credential @ sits inside the whitespace-truncated span, so the strip happens in the ordinary try-branch without ever reaching the fall-through; 'https://api.example — contact admin@example.com' reaches the fall-through only to exercise its no-op bailout. (The pre-existing row 'https://user:p ass@api.example/v1' does pin the path-bearing space-password strip — the pathless variant is what is missing.)
Concrete cost: the documented recovery case in its pathless form ('https://user:pa ss@host.io' → 'https://host.io', confirmed working by probe) is pinned by nothing, and none of the R2-1/R2-2 broken shapes were ever put in front of the suite — the whole regression family ships green (58/58 pass while probes show wrong outputs for the unpinned shapes).
Suggested fix: add rows such as ['https://user:pa ss@host.io', 'https://host.io'] (pins the documented recovery) plus the R2-1/R2-2 shapes with their intended stripped/preserved expectations.
中文说明
新 fall-through 的"执行剥离"一侧没有测试固定。新增用例 'https://user:pass@host.io please contact admin@corp.io' 的凭据 @ 位于空白截断范围内，剥离发生在普通 try 分支，根本不会到达 fall-through；'https://api.example — contact admin@example.com' 到达 fall-through 只是触发其"原样返回"的退出分支。（已有用例 'https://user:p ass@api.example/v1' 确实固定了带路径的空白密码剥离 —— 缺的是无路径变体。）
具体代价：注释中记录的恢复用例的无路径形态（'https://user:pa ss@host.io' → 'https://host.io'，探针确认当前行为正确）没有任何测试固定；R2-1/R2-2 的损坏形态也从未进入测试套件 —— 整组回退会在测试全绿的情况下合入（58/58 通过，而探针显示这些未固定形态的输出是错误的）。
建议修复：补充用例行，例如 ['https://user:pa ss@host.io', 'https://host.io']（固定注释记录的恢复行为），并为 R2-1/R2-2 的形态补上期望的剥离/保留断言。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Truncate URL tokens in provider warnings before sanitizing, strip whitespace-containing userinfo on parse success without leaking or over-stripping trailing contact emails, and add regression tests.
|
Addressed the critical review items in 7cb5da1:
All 41 \�cpModelUtils\ sanitizer tests pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const urlBreak = segment.search(/[\s'"`<>]/); | ||
| const urlToken = urlBreak === -1 ? segment : segment.slice(0, urlBreak); | ||
| const trailing = urlBreak === -1 ? '' : segment.slice(urlBreak); | ||
| result += sanitizeProviderBaseUrl(urlToken) + trailing; |
There was a problem hiding this comment.
[Critical] R3-1: Truncating each URL token at the first whitespace/quote/backtick/angle bracket before delegating to sanitizeProviderBaseUrl severs credentials whose userinfo contains one of those characters — the token no longer contains the @, so the sanitizer returns it unchanged and the credential half is re-appended as "trailing prose". The result is a full credential leak into provider warnings/status, and the pre-existing pinned test sanitizes credentials from provider construction errors now FAILS at this head (it passes on the merge base; this file runs 1 failed | 23 passed, and CI's Test (ubuntu-latest, Node 22.x) is failing on it). — Failure scenario: warning Failed loading provider https://user:sec ret@broken.example/v1 → token cut to https://user:sec (no @, unparseable port, returned unchanged) + ret@broken.example/v1 appended verbatim → username user and password sec ret reach the serve /status errors[].error payload served to ACP clients. sanitizeProviderBaseUrl handles the FULL string correctly ('https://user:pa ss@host.io' → 'https://host.io', pinned by this PR's own new unit rows) — the wrapper's pre-truncation is what makes that logic unreachable from the warning path.
| const urlBreak = segment.search(/[\s'"`<>]/); | |
| const urlToken = urlBreak === -1 ? segment : segment.slice(0, urlBreak); | |
| const trailing = urlBreak === -1 ? '' : segment.slice(urlBreak); | |
| result += sanitizeProviderBaseUrl(urlToken) + trailing; | |
| result += sanitizeProviderBaseUrl(segment); |
findAuthorityEnd's whitespace terminator and the bounded fallback @ search added by this PR already protect trailing prose — probe-verified: all five new warning-level cases stay green under whole-segment delegation. If a split is kept for another reason it must not cut inside userinfo, and the failing pre-existing test must go green.
中文说明
在把 URL 片段交给 sanitizeProviderBaseUrl 之前先按第一个空白/引号/反引号/尖括号截断 token，会把 userinfo 中含这些字符的凭据从中切断 —— token 里不再包含 @，清理函数原样返回 token，凭据的后半段被当作"后续文字"原样拼回。结果是凭据完整泄露进 provider warning/status，且既有的固定测试 sanitizes credentials from provider construction errors 在本 head 上失败（merge base 上通过；本文件 1 failed | 23 passed，CI 的 Test (ubuntu-latest, Node 22.x) 也因此失败）。—— 失败场景：warning Failed loading provider https://user:sec ret@broken.example/v1 → token 被截成 https://user:sec（无 @、端口不可解析、原样返回）+ ret@broken.example/v1 原样拼接 → 用户名 user 和密码 sec ret 进入对外提供给 ACP 客户端的 serve /status errors[].error 载荷。sanitizeProviderBaseUrl 对完整字符串的处理是正确的（'https://user:pa ss@host.io' → 'https://host.io'，本 PR 新增的单测行已固定该行为）—— 正是包装层的预截断使 warning 路径永远到不了那段逻辑。建议修复：直接委托整个片段（见 suggestion 块）；本 PR 新增的 findAuthorityEnd 空白终止符和受限的 fallback @ 搜索已经能保护后续文字（探针验证：整段委托下五个新增 warning 用例仍然全绿）。若出于其他原因必须保留切分，也不能在 userinfo 中间切断，并且必须让既有的固定测试恢复通过。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // span stops at raw whitespace — so `@` can sit past authorityEnd for a | ||
| // password that contains a space. Strip at the first `@` after authorityEnd | ||
| // when the gap still looks like userinfo, not trailing prose. | ||
| const userinfoAt = baseUrl.indexOf('@', authorityEnd); |
There was a problem hiding this comment.
[Critical] R3-2: With findAuthorityEnd now terminating the authority span at whitespace, the @ searches land on the FIRST/inner @ when userinfo contains both whitespace and an additional @ — stripping only the prefix and leaving password material in the sanitized output. WHATWG's userinfo delimiter is the LAST @ before / ? #; this PR's own strips a password containing an at sign row asserts last-@ semantics for @-in-password inputs. This branch is one of three affected strip sites: the in-span lastIndexOf('@') (~line 176), this userinfoAt search, and the fallback's indexOf('@', authorityEnd) (~line 219). — Failure scenario (probe-verified A/B against merge base d91c661): sanitizeProviderBaseUrl('https://user:p ss@real@host.io') → https://real@host.io through this branch (base: https://host.io); https://user:p@ss w0rd@host.io → https://ss w0rd@host.io via the in-span strip (base: https://host.io). Reachable via user-configured baseUrl values displayed through acpAgent.ts, modelConfigUtils.ts, and the workspace provider status. Suggested fix: when credentials are confirmed, strip at the LAST @ before the first / ? # terminator after authorityStart (or end of string); keep the whitespace-bounded span only as the trailing-prose guard — and add regression rows for the whitespace + multi-@ shapes.
中文说明
由于 findAuthorityEnd 现在把 authority 范围终止于空白，当 userinfo 同时包含空白和额外的 @ 时，各处 @ 搜索会落在第一个/内部的 @ 上 —— 只剥掉前缀，密码的剩余部分留在"已清理"的输出里。WHATWG 的 userinfo 分隔符是 / ? # 之前的最后一个 @；本 PR 自己的 strips a password containing an at sign 用例也断言了对含 @ 密码采用最后一个 @ 的语义。本分支是三个受影响的剥离点之一：span 内的 lastIndexOf('@')（约第 176 行）、此处的 userinfoAt 搜索、以及 fallback 的 indexOf('@', authorityEnd)（约第 219 行）。—— 失败场景（已对 merge base d91c661 做 A/B 探针验证）：sanitizeProviderBaseUrl('https://user:p ss@real@host.io') 经本分支输出 https://real@host.io（base 为 https://host.io）；https://user:p@ss w0rd@host.io 经 span 内剥离输出 https://ss w0rd@host.io（base 为 https://host.io）。可通过用户配置的 baseUrl（经 acpAgent.ts、modelConfigUtils.ts 和 workspace provider status 展示）到达。建议修复：当确认存在凭据时，在 authorityStart 之后第一个 / ? # 终止符（或串尾）之前的最后一个 @ 处剥离；仅把空白界定的范围用作后续文字的防护 —— 并为"空白 + 多 @"形态补充回归用例。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| ): number { | ||
| const at = baseUrl.lastIndexOf('@'); | ||
| if (at < authorityStart || authorityEnd >= at) { | ||
| const at = baseUrl.indexOf('@', authorityEnd); |
There was a problem hiding this comment.
[Critical] R3-2: Same wrong-@ class as the comment at line 192, on the catch path: this fallback search strips at the FIRST @ at/after authorityEnd, which is the wrong delimiter when the password contains both a space and an @ and the URL fails WHATWG parsing. — Failure scenario (probe-verified A/B against merge base d91c661): sanitizeProviderBaseUrl('https://h:p x@y@[bad') → https://y@[bad (base: https://[bad) — the password fragment y survives in a URL that visibly still carries userinfo. Suggested fix: same as line 192 — strip at the LAST @ before the first / ? # terminator (or end of string), or reserve first-@ semantics for shapes that provably have no second @; add a catch-path regression row.
中文说明
与第 192 行评论相同的"错误 @"类别，发生在 catch 路径：该 fallback 搜索在 authorityEnd 之后的第一个 @ 处剥离；当密码同时包含空白和 @ 且 URL 无法通过 WHATWG 解析时，这是错误的分隔符。—— 失败场景（已对 merge base d91c661 做 A/B 探针验证）：sanitizeProviderBaseUrl('https://h:p x@y@[bad') → https://y@[bad（base 为 https://[bad）—— 密码片段 y 残留在一个明显仍带 userinfo 的 URL 中。建议修复：同第 192 行 —— 在第一个 / ? # 终止符（或串尾）之前的最后一个 @ 处剥离；并为 catch 路径补充回归用例。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const userinfoAt = baseUrl.indexOf('@', authorityEnd); | ||
| if (userinfoAt !== -1) { | ||
| const gap = baseUrl.slice(authorityEnd, userinfoAt); | ||
| if (parsed.password || /^\s*\S+$/.test(gap)) { |
There was a problem hiding this comment.
[Critical] R3-3: The gap gate requires exactly one non-whitespace run, so username-only userinfo whose gap between the whitespace-truncated authorityEnd and the @ is space-only or multi-word is rejected and returned unchanged with credentials intact — the merge base stripped them. — Failure scenario (probe-verified A/B against merge base d91c661): sanitizeProviderBaseUrl('https://user @host.io') returns the input unchanged (base: https://host.io); https://foo bar baz@corp.io unchanged (base: https://corp.io); https://user name @host.io unchanged (base: https://host.io). Reachable via every direct caller that displays a configured base URL (ACP model listings, workspace provider status). Suggested fix: when parsed.username || parsed.password holds, WHATWG has already proven userinfo exists — strip at the userinfo-terminating @ directly instead of re-deriving it through the gap heuristic (or widen the gate for username-only shapes), and add regression rows for these shapes.
中文说明
gap 守卫要求恰好一个非空白连续段，因此当 userinfo 只有用户名、且空白截断后的 authorityEnd 与 @ 之间的 gap 为纯空白或多个词时，会被拒绝剥离，输入带着凭据原样返回 —— merge base 会剥掉它们。—— 失败场景（已对 merge base d91c661 做 A/B 探针验证）：sanitizeProviderBaseUrl('https://user @host.io') 原样返回（base 为 https://host.io）；https://foo bar baz@corp.io 原样返回（base 为 https://corp.io）；https://user name @host.io 原样返回（base 为 https://host.io）。可通过所有展示已配置 base URL 的直接调用点到达（ACP 模型列表、workspace provider status）。建议修复：当 parsed.username || parsed.password 成立时，WHATWG 已证明 userinfo 存在 —— 直接在 userinfo 结尾的 @ 处剥离，而不是通过 gap 启发式重新推导（或为仅用户名的形态放宽守卫），并为这些形态补充回归用例。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| for (let i = authorityStart; i < end; i++) { | ||
| if (/\s/.test(baseUrl.charAt(i))) { | ||
| return i; |
There was a problem hiding this comment.
[Critical] R3-6: This new whitespace terminator truncates authorityEnd, and findUnescapedUserInfoFallbackAt's unchanged guards (~lines 225/229) evaluate against the truncated span — so on the catch path NO strip happens at all for WHATWG-unparseable URLs with whitespace-containing credentials, and they are returned fully intact. Regression: the merge base stripped them. — Failure scenario (probe-verified A/B against merge base d91c661): sanitizeProviderBaseUrl('https://user pa:ss@[bad') → unchanged, full user pa:ss leak (the password's colon sits past the truncated authorityEnd, so the colon > authorityEnd guard bails; base: https://[bad); https://user:12 34@[bad and https://user:12 34@[::1 → unchanged (portCandidate truncates to 12, mistaken for a port; base stripped both). Control: https://user:pa ss@[bad is stripped by both versions — the defect is the guard interaction with whitespace truncation, not the input class. No test covers the catch path with whitespace credentials, which is how this shipped green. Suggested fix: evaluate the fallback guards against the whitespace-agnostic authority extent (min of / ? # or end, computed from authorityStart); whitespace inside portCandidate disqualifies it as a port. Add catch-path regression tests.
中文说明
这个新增的空白终止符会截断 authorityEnd，而 findUnescapedUserInfoFallbackAt 中未改动的守卫（约第 225/229 行）仍按截断后的范围判断 —— 于是在 catch 路径上，WHATWG 无法解析且凭据含空白的 URL 完全不会被剥离，原样返回。这是回退：merge base 会剥掉它们。—— 失败场景（已对 merge base d91c661 做 A/B 探针验证）：sanitizeProviderBaseUrl('https://user pa:ss@[bad') → 原样返回，user pa:ss 完整泄露（密码中的冒号位于截断后的 authorityEnd 之后，colon > authorityEnd 守卫直接退出；base 为 https://[bad）；https://user:12 34@[bad 与 https://user:12 34@[::1 → 原样返回（portCandidate 被截成 12，误判为端口；base 两者均剥离）。对照：https://user:pa ss@[bad 两个版本都能剥离 —— 缺陷是守卫与空白截断的相互作用，而非输入类别。没有任何测试覆盖 catch 路径 + 含空白凭据，因此它在测试全绿的情况下合入。建议修复：让 fallback 守卫按"忽略空白的 authority 范围"（从 authorityStart 起取 / ? # 或串尾的最小值）判断；portCandidate 中含空白即不能视为端口。并补充 catch 路径的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.7)
Whitespace truncation in sanitizeProviderWarning was cutting passwords like "sec ret" before the authority @, so construction-error sanitizing no longer produced https://broken.example/v1. Extend single-token gaps before @ into the URL token; leave multi-word trailing prose alone.
|
CI failure on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #8524 reuses the authority-aware sanitizeProviderBase...: running the actual vitest suites ( packages/cli/src/utils/acpModelUtils.test.ts , packages/cli/src/serve/workspace-providers-status.test.ts ) — the review work…; PR #8524 reuses the authority-aware sanitizeProviderBase...: running the PR's own vitest files ( acpModelUtils.test.ts , workspace-providers-status.test.ts ) — skipped because dependencies are not installed in this workt….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8524 reuses the authority-aware sanitizeProviderBase...：running the actual vitest suites ( packages/cli/src/utils/acpModelUtils.test.ts , packages/cli/src/serve/workspace-providers-status.test.ts ) — the review work…；PR #8524 reuses the authority-aware sanitizeProviderBase...：running the PR's own vitest files ( acpModelUtils.test.ts , workspace-providers-status.test.ts ) — skipped because dependencies are not installed in this workt…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const gap = rest.slice(0, userinfoAt); | ||
| if (/^\s*\S+$/.test(gap)) { |
There was a problem hiding this comment.
[Critical] R4-1: Multi-word userinfo leaks through the warning path. The gap gate /^\s*\S+$/ accepts only a single non-whitespace run, so a URL whose userinfo contains two or more whitespace-separated words is truncated at the first space before sanitizeProviderBaseUrl sees the terminating @ — the token is returned unchanged and the credential half is re-appended as prose. This is the residual sibling of R3-1, whose own single-space input is fixed at this head. — Failure scenario: probe end-to-end through createWorkspaceProvidersStatusProvider (serve /status errors[].error): warning Failed loading provider https://user:pa ss word@host.io/v1 is emitted unchanged at this head — full user:pa ss word credential displayed; the merge base returned Failed loading provider https://host.io/v1. The direct-call path strips the same URL correctly at this head (https://host.io) — the leak exists only because the wrapper cuts the token before delegation. Note: the multi-word gap is genuinely ambiguous with trailing prose (https://api.example:8443 — contact admin@example.com has the same token shape, and folding on a pre-gap colon reproduces the port+prose corruption), so if stripping is not worth that risk, at minimum document that userinfo with 2+ raw spaces is not stripped from warnings.
中文说明
多词 userinfo 经由 warning 路径泄露。gap 守卫 /^\s*\S+$/ 只接受单个非空白连续段，因此 userinfo 含两个或以上空白分隔词的 URL 会在第一个空白处被截断，sanitizeProviderBaseUrl 永远看不到结尾的 @ —— token 原样返回，凭据后半段被当作“后续文字”原样拼回。这是 R3-1 的残留兄弟形态（R3-1 自身的单空白输入已在本 head 修复）。—— 失败场景：通过 createWorkspaceProvidersStatusProvider 端到端探针（serve /status 的 errors[].error）：warning Failed loading provider https://user:pa ss word@host.io/v1 在本 head 原样输出 —— 完整暴露 user:pa ss word 凭据；merge base 输出 Failed loading provider https://host.io/v1。直接调用路径在本 head 能正确剥离同一 URL（https://host.io）—— 泄露仅因包装层在委托前截断了 token。注意：多词 gap 与后续文字存在真正的歧义（https://api.example:8443 — contact admin@example.com 具有相同 token 形态，按 gap 前冒号折叠会重现“端口+散文”破坏），若不值得为此承担风险，至少应文档说明含两个及以上空白的 userinfo 不会从 warning 中剥离。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const at = baseUrl.indexOf('@', authorityEnd); | ||
| if (at === -1 || at < authorityStart) { |
There was a problem hiding this comment.
[Suggestion] R4-4: at < authorityStart is dead code left over from the lastIndexOf shape this line replaced. indexOf('@', authorityEnd) returns either -1 or an index ≥ authorityEnd, and findAuthorityEnd always returns a value ≥ authorityStart — so the comparison can never be true. In a credential-stripping function it misleads readers into thinking an @ before the authority can reach this branch.
| const at = baseUrl.indexOf('@', authorityEnd); | |
| if (at === -1 || at < authorityStart) { | |
| const at = baseUrl.indexOf('@', authorityEnd); | |
| if (at === -1) { |
中文说明
at < authorityStart 是本行所替换的 lastIndexOf 形态遗留的死代码。indexOf('@', authorityEnd) 只会返回 -1 或 ≥ authorityEnd 的下标，而 findAuthorityEnd 的返回值总是 ≥ authorityStart —— 因此该比较永远不可能成立。在一个凭据剥离函数中，它会误导读者以为 authority 之前的 @ 可能到达此分支。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ['https://user:pa ss@host.io', 'https://host.io'], | ||
| ['https://user:12 34@host.io', 'https://host.io'], |
There was a problem hiding this comment.
[Suggestion] R4-3: The parsed.password || disjunct of the new beyond-authority strip branch (acpModelUtils.ts:195) is unpinned — every password-bearing row in the added table has a single-word gap that /^\s*\S+$/ alone accepts. — Failure scenario: mutation probe — deleting parsed.password || leaves 67/67 tests green while sanitizeProviderBaseUrl('https://user:pa ss word@host.io') regresses from https://host.io to the unchanged credential-bearing input; the same mutant also silently removes the port+prose corruption noted in the R4-7 discussion. The untested disjunct is simultaneously the only handler of multi-word passwords and the cause of that corruption — neither direction is pinned.
| ['https://user:pa ss@host.io', 'https://host.io'], | |
| ['https://user:12 34@host.io', 'https://host.io'], | |
| ['https://user:pa ss@host.io', 'https://host.io'], | |
| ['https://user:12 34@host.io', 'https://host.io'], | |
| ['https://user:pa ss word@host.io', 'https://host.io'], |
中文说明
新的“authority 之外剥离”分支（acpModelUtils.ts:195）中的 parsed.password || 分支没有测试固定 —— 新增表格中所有含密码的用例都是单词 gap，仅靠 /^\s*\S+$/ 即可通过。—— 失败场景：变异探针 —— 删除 parsed.password || 后 67/67 测试仍全绿，而 sanitizeProviderBaseUrl('https://user:pa ss word@host.io') 从 https://host.io 回退为原样返回（凭据完整保留）；同一变异还会悄悄消除 R4-7 讨论中提到的“端口+散文”破坏。该未被测试固定的分支既是多词密码的唯一处理器，又是该破坏的成因 —— 两个方向都没有被测试固定。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const urlBreak = segment.search(/[\s'"`<>]/); | ||
| if (urlBreak === -1) { | ||
| return { urlToken: segment, trailing: '' }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-2: A bare <url> <one-word>@<host> prose pattern is folded into the URL token: the single-word gap extension treats admin@example.com as userinfo, WHATWG reads the real host as part of that userinfo, and the strip rewrites the displayed endpoint to the email's domain. — Failure scenario: probe through the real provider error path: Cannot reach https://api.example admin@example.com → Cannot reach https://example.com; colon variant Cannot reach https://host Contact:admin@corp.io → Cannot reach https://corp.io — the displayed error names the wrong endpoint and deletes the contact text (the merge base left both unchanged). Adjudicated Suggestion rather than Critical: the doc comment documents the boundary (multi-word gaps stay prose; single-word gaps read as userinfo), and the pinned rows (user name@host.io, us er@host) mandate stripping for structurally identical inputs — no lossless discriminator exists, and the harm is display-only. Consider stating the accepted limitation explicitly (a single-word gap before @ is always treated as userinfo, so <url> <email> loses the URL), or a lossy refinement such as not folding when the pre-gap token contains a dot and no colon — knowing it reopens a leak window for dotted usernames and does not fix the colon variant.
中文说明
单个词的散文邮箱会被折叠进 URL token：单词 gap 延伸把 admin@example.com 当作 userinfo，WHATWG 把真实 host 读成该 userinfo 的一部分，剥离后展示的 endpoint 被改写为邮箱域名。—— 失败场景：经真实 provider 错误路径探针：Cannot reach https://api.example admin@example.com → Cannot reach https://example.com；带冒号变体 Cannot reach https://host Contact:admin@corp.io → Cannot reach https://corp.io —— 展示的错误指向错误的 endpoint 且联系文字被删除（merge base 对两者均原样保留）。裁定为 Suggestion 而非 Critical：文档注释已声明该边界（多词 gap 保留为散文；单词 gap 视为 userinfo），且固定用例（user name@host.io、us er@host）要求对结构完全相同的输入执行剥离 —— 不存在无损的判别条件，危害仅限展示层。建议或明确声明该已接受的局限（@ 前的单词 gap 一律视为 userinfo，因此 <url> <email> 会丢失 URL），或采用有损改进，例如当 gap 前的 token 含点号且无冒号时不折叠 —— 但须知这会对带点号的用户名重新打开泄露窗口，且无法修复带冒号的变体。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const userinfoAt = rest.indexOf('@'); | ||
| if (userinfoAt !== -1) { |
There was a problem hiding this comment.
[Suggestion] R4-5: The single-word-gap predicate /^\s*\S+$/ now exists in two packages with divergent behaviour — the acpModelUtils.ts copy (line 195) is bypassed by parsed.password ||, this warning-path copy is not. That divergence is exactly how R4-1 leaks: the direct path strips https://user:pa ss word@host.io to https://host.io while the warning path emits the credential unchanged. Note (from verification): do not unify the two gates into one — the wrapper's stricter gate is forced by the R4-2 ambiguity (giving it a password disjunct would fold https://api.example:8443 — contact admin@example.com and reproduce the port+prose corruption inside the warning path). Cross-reference the two sites instead (a comment at each naming the other and why the gates must differ), or export a shared helper that makes the differing input contracts explicit.
中文说明
单词 gap 判定 /^\s*\S+$/ 现在存在于两个包中且行为不一致 —— acpModelUtils.ts（第 195 行）的那份会被 parsed.password || 短路，此 warning 路径的这份不会。这一差异正是 R4-1 泄露的成因：直接调用路径把 https://user:pa ss word@host.io 剥成 https://host.io，而 warning 路径原样输出凭据。注意（来自验证）：不要把两个守卫统一为一个 —— 包装层更严格的守卫是 R4-2 歧义所迫（给它加上 password 分支会折叠 https://api.example:8443 — contact admin@example.com，在 warning 路径内重现“端口+散文”破坏）。建议改为在两处互相引用（各自注释指明另一处及守卫必须不同的原因），或导出一个共享辅助函数使两种不同的输入契约显式化。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…ping Remove splitProviderWarningUrlToken pre-truncation so multi-word password gaps reach sanitizeProviderBaseUrl. Strip at the last host @ before path or prose when userinfo spans past whitespace, and add regression tests.
|
Addressed round 4 critical feedback in c821d81: R4-1 (Critical): Removed R3-2 / R3-3: When credentials are confirmed and the host R4-4: Fallback userinfo lookup now uses Added regression tests in |
|
CI |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
@qwen-code /review --timeout=360 Ubuntu Test is green on the current head (including the sanitizer suites). Previous review timed out after the 180-minute budget; re-kicking with a longer timeout so the round-4/5 findings can be re-evaluated against c821d81 + follow-ups. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Convergence: round 5 posted 7 inline comment(s), 6 of them reported for the first time. Findings keep coming back to the same files: packages/cli/src/utils/acpModelUtils.ts (findings in round 4; 6 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛情况：第 5 轮发布了 7 条行内评论，其中 6 条是首次提出。发现反复回到同一批文件：packages/cli/src/utils/acpModelUtils.ts（第 4 轮已出过发现，本轮又有 6 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| for (let i = authorityStart; i < end; i++) { | ||
| if (/\s/.test(baseUrl.charAt(i))) { | ||
| return i; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [regression] Credential leak: four probe-confirmed shapes share one root — the whitespace authority terminator truncates the authority span mid-userinfo, and every branch that re-derives the userinfo boundary afterwards fails on a different corner, so credentials survive sanitization.
The probe matrix at this head, all against the merge base: (a) the in-span lastIndexOf('@') strips at the inner @ when a password contains @ before whitespace — sanitizeProviderBaseUrl('https://user:pa@ss w0rd@host.io') returns https://ss w0rd@host.io where the base returned https://host.io; (b) the extended search's proseEnd bound stops at the whitespace after an inner @, so the strip again lands inside the userinfo — https://us a@b er@host.io returns https://b er@host.io, which re-parses with username b%20er (base: https://host.io); (c) the dotted-token gate suppresses the search entirely even though WHATWG parsing proved userinfo exists — https://john.doe @internal.corp.io is returned unchanged and the username reaches the served workspace status and ACP responses (base: https://internal.corp.io); (d) on the catch path the fallback's colon/port guards evaluate against the truncated span and bail — https://user pa:ss@[bad, https://user:12 34@[bad, https://user:12 34@[::1 and nine further WHATWG-unparseable shapes (e.g. https://user name:pass@broken host, https://user name:secret@host:99999999) are returned fully intact where the merge base stripped every one (deterministic split 19/19 base vs 15/19 PR on the probe file). Every shape reaches every display call site (acpAgent.ts, modelConfigUtils.ts, workspace-providers-status.ts). Shapes (a) and (d) are the carried blockers R3-2 and R3-6, still standing in these shapes; (b) and (c) are new this round. This family has produced a new leaking shape in every round since round 2, and per-entrance patching has not converged it — when parsed.username || parsed.password holds, WHATWG has already proven userinfo exists, so strip at the LAST @ before the first / ? # terminator after authorityStart (or end of string) instead of re-deriving the boundary per branch, and key the catch-path guards to the whitespace-agnostic extent (min of / ? # or end); a username-only unparseable shape is genuinely ambiguous with a prose email — decide it together with the over-strip direction in R5-2.
Witness:
BASE: 'https://user:pa@ss w0rd@host.io' -> 'https://host.io'
PR: 'https://user:pa@ss w0rd@host.io' -> 'https://ss w0rd@host.io'
BASE: 'https://us a@b er@host.io' -> 'https://host.io'
PR: 'https://us a@b er@host.io' -> 'https://b er@host.io' (re-parse: username 'b%20er')
BASE: 'https://john.doe @internal.corp.io' -> 'https://internal.corp.io'
PR: 'https://john.doe @internal.corp.io' -> unchanged (leak)
PR probe V-R36: Expected "https://[bad" Received "https://user pa:ss@[bad";
Expected "https://[bad" Received "https://user:12 34@[bad"
Nine further unparseable shapes: UNCHANGED on PR arm, stripped on BASE arm.
// Direction — in the parse-success arm, when credentials are proven, strip at the
// last '@' before the first '/ ? #' after authorityStart (WHATWG's own userinfo
// separator) instead of the truncated-span heuristics; on the catch arm, evaluate
// the guards against that same whitespace-agnostic extent.The fix must keep this diff's own prose-keep pins byte-identical — ['https://api.example — contact admin@example.com', …], ['https://user:pa ss@host.io please contact admin@corp.io', 'https://host.io please contact admin@corp.io'] (packages/cli/src/utils/acpModelUtils.test.ts:253-254, ~264) and ['https://host bad:x/v1 — contact admin@corp.io', …] (acpModelUtils.test.ts:261-262); trusting the parser's userinfo split unconditionally or extending the colon search past the first / ? # breaks them. Please add rows ['https://user:pa@ss w0rd@host.io', 'https://host.io'], ['https://us a@b er@host.io', 'https://host.io'], ['https://john.doe @internal.corp.io', 'https://internal.corp.io'], ['https://user pa:ss@[bad', 'https://[bad'] to packages/cli/src/utils/acpModelUtils.test.ts, then prove each branch by removing your fix for one shape at a time and confirming the matching row goes red.
中文说明
凭据泄露：四个经探针确认的形态同根 —— 空白 authority 终止符把 authority 范围从 userinfo 中间截断，之后每个重新推导 userinfo 边界的分支各漏一角，凭据残留在“已清理”的输出里。
探针矩阵（本 head 对 merge base）：(a) span 内 lastIndexOf('@') 落在内部 @ 上 —— 密码在空白之前含 @ 时，sanitizeProviderBaseUrl('https://user:pa@ss w0rd@host.io') 输出 https://ss w0rd@host.io（base 为 https://host.io）；(b) 扩展搜索的 proseEnd 边界停在内部 @ 之后的空白处，剥离再次落在 userinfo 内部 —— https://us a@b er@host.io 输出 https://b er@host.io，重新解析用户名为 b%20er（base 为 https://host.io）；(c) 点号 token 守卫在 WHATWG 已证明存在 userinfo 时仍压制搜索 —— https://john.doe @internal.corp.io 原样返回，用户名进入对外的 workspace status 与 ACP 响应（base 为 https://internal.corp.io）；(d) catch 路径上 fallback 的冒号/端口守卫按截断后的范围判断并直接退出 —— https://user pa:ss@[bad、https://user:12 34@[bad、https://user:12 34@[::1 以及另外九个 WHATWG 无法解析的形态（如 https://user name:pass@broken host、https://user name:secret@host:99999999）全部原样返回，而 merge base 全部剥离（探针文件确定性对比：base 19/19 对 PR 15/19）。所有形态都能到达全部展示调用点（acpAgent.ts、modelConfigUtils.ts、workspace-providers-status.ts）。形态 (a)、(d) 即原有阻塞项 R3-2、R3-6，在本 head 仍然成立；(b)、(c) 为本轮新发现。该家族自第 2 轮起每轮都出现新的泄露形态，逐入口修补并未收敛 —— 当 parsed.username || parsed.password 成立时，WHATWG 已证明 userinfo 存在：在 authorityStart 之后第一个 / ? # 终止符（或串尾）之前的最后一个 @ 处剥离，而不是逐分支重新推导边界；catch 路径的守卫也应按同一“忽略空白的范围”（/ ? # 或串尾的最小值）判断；仅有用户名且无法解析的形态与“散文邮箱”存在真正的歧义，请与 R5-2 的过度截取方向一并决策。
修复必须保持本 PR 自己固定的“保留散文”用例逐字节不变 —— ['https://api.example — contact admin@example.com', …]、['https://user:pa ss@host.io please contact admin@corp.io', 'https://host.io please contact admin@corp.io']（packages/cli/src/utils/acpModelUtils.test.ts:253-254、约 264）以及 ['https://host bad:x/v1 — contact admin@corp.io', …]（acpModelUtils.test.ts:261-262）；无条件信任 parser 的 userinfo 切分、或把冒号搜索延伸到第一个 / ? # 之后都会破坏这些用例。请补充用例行 ['https://user:pa@ss w0rd@host.io', 'https://host.io']、['https://us a@b er@host.io', 'https://host.io']、['https://john.doe @internal.corp.io', 'https://internal.corp.io']、['https://user pa:ss@[bad', 'https://[bad']，然后逐个移除你对各形态的修复并确认相应用例变红，以此证明每个分支。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| function shouldExtendUserInfoSearch( | ||
| authoritySlice: string, | ||
| parsed: URL, | ||
| ): boolean { |
There was a problem hiding this comment.
[Critical] R5-2: [certifies-falsely] [regression] Over-strip: prose after a URL keeps being read as userinfo, so warnings are rewritten to the contact email's domain — an unbounded surface this family has exposed in every round since round 1.
Warning-level A/B at the exact surface this diff changed: Cannot reach https://localhost — contact admin@example.com becomes Cannot reach https://example.com at this head while the merge base kept it byte-identical; Cannot reach https://api.example — error at 10:30, contact admin@example.com becomes Cannot reach https://example.com (base kept it) — a colon anywhere in the prose makes WHATWG report a password, which bypasses the dotted-host gate entirely; and R4-2's colon variant Cannot reach https://host Contact:admin@corp.io still becomes Cannot reach https://corp.io where the base kept it. The displayed error names a host that was never the provider and deletes the contact instruction — misleading diagnostics from the function whose purpose is safe display. The round-4 fix closed only dotted-host prose; the pre-existing siblings http://localhost:11434 — contact ops@corp.io and the https://api.example:v1[/x] — contact admin@example.com pair behave the same on both arms, and with the R5-1 candidate fix applied the localhost shape is still destroyed — per-entrance patching does not close this surface. Close it structurally: for spans new URL parses, reconstruct the output from the parser's own userinfo/host split only when the pre-@ text is provably userinfo (e.g. a whitespace-only gap), and for free-text segments that do not parse cleanly fail closed (redact the whole URL span) instead of performing indexOf-surgery on prose — the pattern getRouteEndpointIdentity in this same file already uses.
Witness:
W1 BASE: 'Cannot reach https://localhost — contact admin@example.com' -> unchanged
W1 PR: same -> 'Cannot reach https://example.com'
W2 BASE: 'Cannot reach https://api.example — error at 10:30, contact admin@example.com' -> unchanged
W2 PR: same -> 'Cannot reach https://example.com'
R4-2 colon variant PR: Expected 'Cannot reach https://host Contact:admin@corp.io'
Received 'Cannot reach https://corp.io' (BASE arm passes)
The fix must keep multi-word credentials stripping — ['https://user:pa ss word@host.io/v1', 'https://host.io/v1'] (packages/cli/src/utils/acpModelUtils.test.ts:277) — and the pinned keep Cannot reach https://api.example:8443/v1 — contact admin@example.com (packages/cli/src/serve/workspace-providers-status.test.ts, ~line 646); any port/digit heuristic must reconcile with the user:12 userinfo slice, which is also token:digits. Please add keep-rows ['https://localhost — contact admin@example.com', …] and ['https://api.example — error at 10:30, contact admin@example.com', …] (both RED at this head) to both test files, then remove your guard and confirm they go red again.
中文说明
过度截取：URL 之后的散文仍被当作用户信息，warning 被改写为联系邮箱的域名 —— 该家族自第 1 轮起每轮都暴露新的入口，是无界表面。
在本 diff 改动的表面上做 warning 级 A/B：Cannot reach https://localhost — contact admin@example.com 在本 head 变为 Cannot reach https://example.com，merge base 逐字节保留；Cannot reach https://api.example — error at 10:30, contact admin@example.com 变为 Cannot reach https://example.com（base 保留）—— 散文中任意位置的冒号都会让 WHATWG 报出 password，从而完全绕过点号 host 守卫；R4-2 的冒号变体 Cannot reach https://host Contact:admin@corp.io 仍变为 Cannot reach https://corp.io（base 保留）。对外展示的错误指向一个从未是 provider 的域名并删除联系方式 —— 在一个以安全展示为目的的函数里产生误导性诊断。第 4 轮的修复只封闭了“点号 host + 散文”一种形态；http://localhost:11434 — contact ops@corp.io 与 https://api.example:v1[/x] — contact admin@example.com 等既有兄弟形态在两侧表现相同，且在 R5-1 的候选修复之上 localhost 形态仍被破坏 —— 逐入口修补无法封闭该表面。请结构性关闭：对 new URL 可解析的片段，仅当 @ 之前的文本可证明是 userinfo（例如纯空白 gap）时按 parser 自身的 userinfo/host 切分重建输出；对无法干净解析的自由文本片段则失败关闭（整个 URL 片段打码），而不是对散文做 indexOf 手术 —— 同文件的 getRouteEndpointIdentity 已在用这一模式。
修复必须保持多词凭据继续被剥离 —— ['https://user:pa ss word@host.io/v1', 'https://host.io/v1']（packages/cli/src/utils/acpModelUtils.test.ts:277）—— 以及固定保留用例 Cannot reach https://api.example:8443/v1 — contact admin@example.com（packages/cli/src/serve/workspace-providers-status.test.ts 约 646 行）；任何端口/数字启发式都必须与 user:12 这种同为 token:数字 的 userinfo 片段兼容。请在两个测试文件中补充保留用例 ['https://localhost — contact admin@example.com', …] 与 ['https://api.example — error at 10:30, contact admin@example.com', …]（本 head 均为红），然后移除你的守卫并确认它们再次变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (parsed.password) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-3: The parsed.password disjunct is still unpinned at this head — every password-bearing row added since round 4 has a colon in its whitespace-truncated authority slice, so the colon branch alone carries them.
Still stands from round 4. Mutation probe at this head: deleting parsed.password || from the early-return guard (if (!(parsed.username || parsed.password)) → if (!parsed.username)) leaves all 73 tests in the two suites green, and the mutant is live — discriminator https://:pass@host.io returns unchanged under the mutant while intact code returns https://host.io. The same unpinned disjunct shape sits in shouldExtendUserInfoSearch's if (parsed.password) branch (this thread), whose inputs are exactly the prose-password shapes R5-2 names, so neither direction is pinned. Add rows that only the password disjunct can carry: ['https://:pass@host.io', 'https://host.io'] plus a prose-password shape whose truncated slice has no colon, with its intended stripped/preserved expectation pinned deliberately. The new rows must go red when the disjunct is deleted from either gate — the https://:pass@host.io discriminator is exactly that test.
Witness:
Mutant (parsed.password removed): 73/73 tests pass — none of the password rows fail.
Discriminator: 'https://:pass@host.io' -> unchanged under mutant;
intact HEAD -> 'https://host.io'.
中文说明
parsed.password 分支在本 head 仍然没有测试固定 —— 第 4 轮之后新增的所有含密码用例，其空白截断后的 authority 片段都含冒号，仅靠冒号分支即可通过。
第 4 轮的结论仍然成立。本 head 变异探针：从早退守卫中删除 parsed.password || （if (!(parsed.username || parsed.password)) → if (!parsed.username)）后，两个测试文件 73/73 全绿；变异体是活的 —— 判别输入 https://:pass@host.io 在变异体下原样返回，完整代码输出 https://host.io。同一未固定分支也存在于 shouldExtendUserInfoSearch 的 if (parsed.password)（本条所在线），其输入正是 R5-2 点名的“散文密码”形态，两个方向都没有被测试固定。请补充只有 password 分支能承载的用例：['https://:pass@host.io', 'https://host.io']，以及一个截断片段不含冒号的散文密码形态，并明确固定其剥离/保留期望。新用例必须在从任一处删除该分支时变红 —— https://:pass@host.io 判别输入正是该测试。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| function findPathDelimiterStart( | ||
| baseUrl: string, | ||
| authorityStart: number, | ||
| ): number { |
There was a problem hiding this comment.
[Suggestion] R5-3: findPathDelimiterStart re-implements, byte-identically, the first-of / ? # scan that findAuthorityEnd (~line 297) already contains in this same file — security-relevant boundary logic now lives in two copies, and this diff itself demonstrates the drift pressure: one copy gained whitespace termination, the other did not.
A future authority-terminator change applied to one copy only makes sanitizeProviderBaseUrl's authority span and findHostAtBeforePathOrProse's after-authority search disagree about where the path starts for the same input, so credential stripping silently diverges between a bare base URL and the same URL embedded in a provider warning — the exact drift this PR exists to eliminate. Compute the delimiter end once and keep the whitespace difference where it belongs:
function findAuthorityEnd(baseUrl: string, authorityStart: number): number {
const end = findPathDelimiterStart(baseUrl, authorityStart);
for (let i = authorityStart; i < end; i++) {
if (/\s/.test(baseUrl.charAt(i))) return i;
}
return end;
}Witness:
witness: not run — static duplication claim; both bodies quoted byte-identical at HEAD
(findPathDelimiterStart lines 249-261 are the first eight lines of findAuthorityEnd
lines 297-304); no behavior to execute without reimplementing both blocks.
The whitespace difference is deliberate and must survive the dedup: findHostAtBeforePathOrProse searches past intervening prose whitespace per const searchEnd = Math.min(pathStart, proseEnd); (packages/cli/src/utils/acpModelUtils.ts:245) — making pathStart stop at whitespace would cut the search before the @ in inputs like https://user:pa ss@host.io/v1 and leak the credential. The pinned rows ['https://user:pa ss word@host.io/v1', 'https://host.io/v1'] and ['https://user:p?x@api.example/v1', 'https://api.example/v1'] in packages/cli/src/utils/acpModelUtils.test.ts pin both sides of the difference — after the dedup, remove the whitespace scan (or make findPathDelimiterStart whitespace-aware) and confirm those rows go red.
中文说明
findPathDelimiterStart 与 findAuthorityEnd（约第 297 行）中已有的“取 / ? # 最小值”扫描逐字节重复 —— 安全相关的边界逻辑现在有两份，且本 diff 本身就展示了漂移压力：其中一份加了空白终止符，另一份没有。
将来若只修改其中一份的 authority 终止符，sanitizeProviderBaseUrl 的 authority 范围与 findHostAtBeforePathOrProse 的 authority 后搜索就会对同一路径起点判断不一致，凭据剥离会在“裸 base URL”与“嵌在 provider warning 中的同一 URL”之间悄悄分叉 —— 正是本 PR 要消除的那种漂移。建议把分隔符终点的计算合并为一处，并把空白差异放到它应在的位置（见上方代码）。
该空白差异是有意的，去重时必须保留：findHostAtBeforePathOrProse 依据 const searchEnd = Math.min(pathStart, proseEnd);（packages/cli/src/utils/acpModelUtils.ts:245）需要越过散文中的空白继续搜索 —— 若让 pathStart 停在空白处，https://user:pa ss@host.io/v1 这类输入的搜索会在 @ 之前被截断，导致凭据泄露。固定用例 ['https://user:pa ss word@host.io/v1', 'https://host.io/v1'] 与 ['https://user:p?x@api.example/v1', 'https://api.example/v1'] 分别固定了差异的两侧 —— 去重后，移除空白扫描（或让 findPathDelimiterStart 感知空白）并确认这些用例变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| return true; | ||
| } | ||
| // A dotted token without ':' is a complete host; trailing prose may follow. | ||
| return !(authoritySlice.includes('.') && !authoritySlice.includes(':')); |
There was a problem hiding this comment.
[Suggestion] R5-4: The !authoritySlice.includes(':') clause in this final return is always true — the colon case already early-returned two lines above, so the expression is exactly return !authoritySlice.includes('.');, and the doubled negation hides the rule the comment states.
A maintainer investigating colon-bearing authorities (a truncated user:pa… slice or host:port) reads this return and reasons about a colon case that is unreachable at that line, or edits the clause believing it is load-bearing — in a credential-stripping function. A routing probe over the 18 test-table inputs shows five reach this final return, every one with a colon-free slice, and the real function produced the expected outputs for all of them.
Witness:
Probe: 5/18 table inputs reach the final return — 'https://api.example — contact admin@example.com',
'https://user name@host.io', 'https://us er@host', 'https://user @host.io', 'https://foo bar baz@corp.io'
— every reaching slice colon-free; outputs all as pinned.
| return !(authoritySlice.includes('.') && !authoritySlice.includes(':')); | |
| return !authoritySlice.includes('.'); |
中文说明
最终返回里的 !authoritySlice.includes(':') 永远为真 —— 含冒号的情况已在上方两行提前返回，该表达式实际等价于 return !authoritySlice.includes('.');，双重否定掩盖了注释所描述的规则。维护者若调查含冒号的 authority（截断的 user:pa… 片段或 host:port），会在这行推理一个不可达的冒号分支，或误以为该子句起承重作用 —— 这发生在一个凭据剥离函数里。对 18 个测试表输入的路由探针显示 5 个到达该最终返回，其片段均不含冒号，且真实函数输出全部符合预期。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| baseUrl: string, | ||
| authorityEnd: number, | ||
| ): number { | ||
| const pathStart = findPathDelimiterStart(baseUrl, authorityEnd); |
There was a problem hiding this comment.
[Suggestion] R5-5: findPathDelimiterStart's parameter is named authorityStart, but this — its only call site — passes the authority's END, while the sibling findAuthorityEnd(baseUrl, authorityStart) takes a genuine start and contains a verbatim copy of the same delimiter block: the same parameter name means start in one helper and receives an end in the other.
A maintainer reusing the helper as its name and sibling suggest — from a real authority start, e.g. when de-duplicating the delimiter block per R5-3 — gets back the authority terminator itself instead of the path start, so any span computed from that "end" collapses and credential stripping silently misbehaves or no-ops. Rename the parameter to from (or searchStart); the signature at lines 249-252 and this sole call site are the only two touch points.
Witness:
witness: not run — static call-site fact; signature at lines 249-252 and this sole
call site quoted at HEAD; the misuse outcome follows from the quoted bodies.
中文说明
findPathDelimiterStart 的参数名为 authorityStart，但此处（唯一调用点）传入的是 authority 的终点；而同族的 findAuthorityEnd(baseUrl, authorityStart) 接收的是真正的起点并包含同一分隔符块的逐字节副本 —— 同一个参数名在一个辅助函数里表示起点，在另一个里接收的却是终点。维护者若按名称与同族函数的暗示复用该函数（例如按 R5-3 去重时从真正的 authority 起点调用），会得到 authority 终止符本身而非路径起点，基于该“终点”计算的范围会塌缩，凭据剥离会悄悄出错或空转。请将参数改名为 from（或 searchStart）；只需改 249-252 行的签名与本调用点两处。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| /\s/.test(baseUrl.charAt(i)) && | ||
| baseUrl.indexOf('@', authorityEnd) < i |
There was a problem hiding this comment.
[Suggestion] R5-6: baseUrl.indexOf('@', authorityEnd) in this loop is loop-invariant — neither baseUrl nor authorityEnd depends on i — yet it is recomputed for every whitespace character scanned, making the prose scan O(k·(n − authorityEnd)) instead of O(n).
A warning segment whose post-authority region holds k whitespace characters before the first @ re-runs an O(n − authorityEnd) scan k times. This is a cold path over short strings, so the practical magnitude is small; the fix is a one-line hoist:
const firstAtAfterAuthority = baseUrl.indexOf('@', authorityEnd);
for (let i = authorityEnd + 1; i < baseUrl.length; i++) {
if (/\s/.test(baseUrl.charAt(i)) && firstAtAfterAuthority < i) {
proseEnd = i;
break;
}
}Witness:
witness: not run — invariance is a static data-flow fact quoted from the loop body;
a timing measurement cannot discriminate hoisted vs unhoisted semantics.
中文说明
本循环中的 baseUrl.indexOf('@', authorityEnd) 是循环不变量 —— baseUrl 与 authorityEnd 都不依赖 i —— 却按扫描到的每个空白字符重复计算，使散文扫描从 O(n) 退化为 O(k·(n − authorityEnd))。authority 之后、第一个 @ 之前含 k 个空白的 warning 会把 O(n − authorityEnd) 的扫描重复 k 次。这是短字符串上的冷路径，实际影响很小；修复为一行外提（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Closing this after reproducing both round-5 Critical findings against the exact reviewed head (67f7899) and PR base (80b34ec). The current head is strictly worse than the base for every witness I checked: four credential-bearing inputs either leak credentials or strip at the wrong at-sign, and three ordinary warnings are rewritten to the contact email's domain. The common cause is structural: the warning path passes the remainder of a line (URL plus prose) into a sanitizer whose contract is a standalone base URL. Extending that sanitizer across whitespace makes userinfo and prose email ambiguous, and five review rounds have not converged on a safe rule. I do not think another per-shape heuristic is responsible here. The original narrow bugs remain valid, but this PR should not merge while it regresses credential handling. A future attempt should first separate warning tokenization/redaction from standalone base-URL sanitization and use the round-5 base/head witness matrix as acceptance tests. The remaining suggestions are moot for this implementation. Thanks for the detailed review and the concrete probes. |
3 participants
What this PR does
Provider warning URL spans now use the existing authority-aware base URL sanitizer directly. The warning-specific credential heuristics and duplicate URL matcher are removed; span splitting remains unchanged so warnings can still contain prose and multiple URLs.
Why it's needed
The duplicate warning sanitizer searched for the first colon and first at sign across the whole span. A port could therefore be mistaken for a password delimiter and a later email address for userinfo, truncating the warning. Conversely, an at sign inside a raw password could end the cut early and expose the rest of the password. Reusing the existing authority-scoped sanitizer fixes both defects without introducing another host grammar.
Reviewer Test Plan
How to verify
Confirm that a warning containing
https://api.example:8443/v1followed byadmin@example.comis returned byte-for-byte unchanged, thathttps://user:p@ssw0rd-tail@broken.example/v1becomeshttps://broken.example/v1, and that an uncredentialed URL with an explicit port remains byte-for-byte unchanged. The two existing warning and construction-error credential tests should continue to pass.Evidence (Before & After)
N/A (non-UI sanitization fix). The focused test file passes 22/22 tests.
Tested on
Environment (optional)
Node.js 24.13.0 on Windows. Verified with the focused Vitest file, ESLint on both changed files, package typecheck, and Prettier check.
Risk & Scope
Linked Issues
Fixes #8136
中文说明
本 PR 的改动
Provider warning 中切分出的 URL 片段现在直接复用现有的、能识别 authority 边界的 base URL 清理函数。删除 warning 专用的凭据启发式逻辑和重复 URL 匹配器；原有片段切分保持不变，因此 warning 仍可包含说明文字和多个 URL。
为什么需要
重复实现的 warning 清理逻辑会在整个片段中寻找第一个冒号和第一个
@。因此端口可能被误判为密码分隔符，后面的邮箱地址又被误判为 userinfo 终点，导致 warning 被截断。反过来，原始密码中的@也可能让截取过早结束并泄露剩余密码。复用现有按 authority 限定范围的清理逻辑，可以修复这两个问题，且无需再引入一套 host 语法。审查测试计划
如何验证
确认包含
https://api.example:8443/v1且后面跟有admin@example.com的 warning 原样返回；确认https://user:p@ssw0rd-tail@broken.example/v1变为https://broken.example/v1；确认带显式端口但无凭据的 URL 原样返回。现有两项 warning 和构造错误凭据测试应继续通过。证据（修改前后）
不适用（非 UI 清理修复）。聚焦测试文件 22/22 通过。
测试平台
环境（可选）
Windows 上的 Node.js 24.13.0。已验证聚焦 Vitest 文件、两个修改文件的 ESLint、包级 typecheck 和 Prettier 检查。
风险与范围
关联 Issue
修复 #8136