fix(serve): use authority-scoped credential stripping in provider warning sanitizer - #8408
fix(serve): use authority-scoped credential stripping in provider warning sanitizer#8408C0d3N1nja97342 wants to merge 15 commits into
Conversation
…ning sanitizer
sanitizeProviderWarningSegment used naive indexOf(':') and indexOf('@')
over the full segment, causing two bugs:
1. A URL port (:8443) was read as a password delimiter, and a later @
(e.g. in an email) was taken as the userinfo end - truncating the
message after the URL.
2. An @ inside a password was matched as the userinfo end, leaking the
remainder of the password.
Delete sanitizeProviderWarningSegment and hasCredentialPrefix, and
delegate to the existing sanitizeProviderBaseUrl which uses
authority-scoped lastIndexOf('@') and port detection. Preserves the
space-in-userinfo handling by first trying the full segment through
sanitizeProviderBaseUrl, then falling back to per-URL-fragment
sanitization.
Two regression tests: port+email (no truncation), @ in password (no
leak).
Fixes QwenLM#8136
|
Thanks for the PR — this one picks up #8136 after the first fix attempt (#8137) was closed. Template looks good ✓ — all sections present. Problem: observed bug, strong evidence. #8136 ships before/after reproductions for both failure modes (port + later Direction: aligned. The issue itself proposed delegating to the authority-scoped Size: not a core-infrastructure path ( Approach: minimal and focused — deletes two bespoke helpers, adds one wrapper that delegates, plus two regression tests. No unrelated changes. One thing code review will need to look at closely: the wrapper tries the whole segment through Risk: no elevated-risk paths matched the revert-history scan. Moving on to code review. 🔍 中文说明感谢贡献——这个 PR 是在第一次修复尝试（#8137）被关闭之后接手 #8136 的。 模板完整 ✓——所有必需章节齐全。 问题：已观测到的 bug，证据充分。#8136 提供了两种失败模式（端口 + 后续 方向：对齐。issue 本身就建议委托给 authority 作用域的 规模：不属于核心基础设施路径（ 方案：最小且聚焦——删除两个自定义辅助函数，新增一个委托包装函数，外加两条回归测试。无夹带改动。代码审查需要重点看一处：包装函数先把整个 segment 交给 风险：回滚历史扫描未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewBefore reading the diff, my independent proposal for this problem was essentially what this PR does: stop re-implementing userinfo detection in But the whole-segment pass is also where it breaks. 1. The headline bug survives without a path. 2. A regression vs 3. A regression vs To be clear about severity: none of these leak credentials — the stripping is over-eager, not too shallow (and the ambiguous Suggested direction, not a prescription: the strip point needs to be bounded by spaces as well as Test evidence (this PR's own CI, read via API — PR code is never executed in triage)The unit suite has not finished on the reviewed commit yet; the table below is a snapshot and the finalize job will update it once CI settles. No failures so far. Not verified by me directly: static traces only, since this run cannot execute PR code. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle what the suite cannot pin here: 中文说明代码审查：在读 diff 之前，我的独立方案与这个 PR 基本一致——不要在 但"整段优先"正是问题所在。
严重度说明：这些都不泄露凭据——裁剪过度而非不足（ 建议方向（非强制）：当输入不是独立 URL 时，裁剪点需要同时以空格为界——可以在包装函数里做，也可以（在与 测试证据：以上 CI 表格来自 API 对当前 commit 的一次性快照（triage 环境不执行 PR 代码）；单元套件尚未跑完，finalize 任务会在 CI 结束后更新表格。目前无失败。沙箱验证行（ — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 2/5 — the approach is the right one and the two reported repro shapes are genuinely fixed, but the delegation as written re-corrupts messages for pathless URLs and regresses portless ones that Stepping back: my independent proposal before reading the diff was exactly this PR's shape — delete the bespoke This is the same wall #8137 died at: the hard part of this sanitizer was never the credentials case in isolation, it's that a warning line mixes a URL with prose and the boundary between them is ambiguous. That PR answered with host-recognition rules and regressed five ways; this one answers with delegation and inherits the delegate's blind spot. Neither is hopeless — a space bound on the authority (in the wrapper or in CI note: the ubuntu unit suite and the Serve A/B run were still in flight on the reviewed commit when this pass ran; nothing had failed. That doesn't change this verdict — the gaps above are traceable from the code and are not covered by any test, green or otherwise. @C0d3N1nja97342 requesting changes on the three traces in the review comment — the fix direction is sound, please don't take this as a rejection of the approach. 中文说明置信度：2/5——方向是对的，两个上报的复现形态确实被修复了，但当前的委托写法仍会破坏无路径 URL 的消息，并让 退一步看：我在读 diff 之前的独立方案与这个 PR 完全同形——删掉自定义的 这正是 #8137 倒下的同一堵墙：这个 sanitizer 的难点从来不是孤立的凭据场景，而是警告行里 URL 与正文混在一起、二者边界模糊。那个 PR 用主机识别规则来回答，结果五处回归；这个 PR 用委托来回答，继承了被委托方的盲区。两者都不是死局——给 authority 加空格边界（在包装函数里，或在 CI 说明：本 pass 运行时，ubuntu 单元套件与 Serve A/B 仍在进行中，暂无失败。这不改变结论——上述缺口可以从代码追踪得出，且没有任何测试覆盖，无论绿与否。 @C0d3N1nja97342 基于审查评论中的三条追踪请求修改——修复方向是合理的，请不要把这理解为对方案的否定。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The direction is right and both reported repros are fixed, but the whole-segment pass through sanitizeProviderBaseUrl corrupts messages whenever the URL has no path: the authority then runs to end of segment, and a later @ (an email, a mention) becomes the strip point. Three traces in my review comment — including the issue's own example sentence minus the port, and two regressions versus main where the old code preserved the message. Please bound the strip point by spaces as well (the space-in-password case still has a home via the existing colon/port fallback) and add tests for the pathless/portless shapes. Details above. 🙏
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
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: |
sanitizeProviderBaseUrl computed the URL authority only up to '/', '?', '#' — not whitespace. For a pathless URL followed by prose (an email, a mention), the authority swallowed the rest of the message and a later '@' became the strip point, rewriting the host and dropping prose. - findAuthorityEnd now also stops at whitespace, so a pathless URL's authority ends at the first space. - new URL() percent-encodes spaces inside userinfo rather than throwing, so a space-in-password case (user:sec ret@host) sees the '@' past the whitespace-bounded authority and authorityAtIndex is -1. Route that from the try branch through the same colon/non-numeric-port heuristic used in the catch branch. Regression tests for all three QwenLM#8136 shapes: pathless port + email, portless URL + email, credentials + pathless + email.
|
@qwen-code /triage |
|
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=240 |
| const fallbackAt = findUnescapedUserInfoFallbackAt( | ||
| baseUrl, | ||
| authorityStart, | ||
| authorityEnd, | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-1: The strip point of this new fallback is baseUrl.lastIndexOf('@') over the ENTIRE input string (findUnescapedUserInfoFallbackAt). When userinfo contains whitespace — the exact case this fallback exists for — the whitespace-bounded authority excludes the real @, so any LATER @ on the line becomes the strip point and stripAt deletes everything from the scheme through it: host, path and prose. This is the same corruption class #8136 was filed for, reintroduced for space-containing credentials; the deleted sanitizeProviderWarningSegment anchored on the FIRST @ and handled these shapes correctly. — Failure scenario: A/B-verified against the merge base: sanitizeProviderBaseUrl('https://user:sec ret@host.example/v1 — contact admin@example.com') → https://example.com (host rewritten to the email's domain, host.example/v1 — contact admin deleted); the merge base returned https://host.example/v1 — contact admin@example.com. Same through the warning pipeline. Path variant: 'https://user:sec ret@host/p@th' → 'https://th' (base: 'https://host/p@th'). No test covers space-in-password combined with a later @.
Suggested direction (the fix belongs in findUnescapedUserInfoFallbackAt, whose @ search must be bounded by the URL rather than the whole string): a userinfo terminator can never legitimately appear at/after the first /, ? or #, so bound the search there and prefer the FIRST @ at/after authorityEnd whose following run up to the next delimiter contains no further @:
const wideEnd = firstOf(baseUrl, ['/', '?', '#'], authorityStart) ?? baseUrl.length;
const at = baseUrl.indexOf('@', authorityEnd); // then validate host-shaped region up to wideEndVerified this keeps the PR's user:p@ssw0rd-tail@… fix intact (first candidate still contains @, so it is skipped) while preserving host + prose. Please add regression tests for 'https://user:sec ret@host contact admin@example.com' and 'https://user:sec ret@host/p@th'.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const whitespace = baseUrl.slice(authorityStart).search(/\s/); | ||
| if (whitespace !== -1) end = Math.min(end, authorityStart + whitespace); |
There was a problem hiding this comment.
[Critical] R1-2: With the authority now whitespace-bounded, any parseable URL whose password contains a space is routed into findUnescapedUserInfoFallbackAt. Its heuristic requires a colon and vetoes all-digit portCandidates (/^\d+$/), so userinfo whose password STARTS WITH DIGITS followed by a space is returned unchanged — a credential leak this diff introduces (pre-PR the unbounded authority stripped it). — Failure scenario: A/B-verified: sanitizeProviderBaseUrl('https://user:1234 secret@host') returns the input unchanged at HEAD (leaking user:1234 secret@ into the serve /status payload via errors[].error/models[].baseUrl/current.baseUrl, into ACP agent responses, and into model-config warnings); the merge base returned https://host. Same for 'https://user:12 34@host', 'https://user:80 secret@host' and the colon-less 'https://user name@host'. Note the veto is load-bearing: WHATWG parses the PR's required case 'https://api.example:8443 — contact admin@example.com' as spurious userinfo (username: 'api.example', password '8443…'), so the two input classes are locally indistinguishable at the veto — this needs a redesigned terminator heuristic, not a one-line tweak (e.g. when new URL() has proven userinfo exists, derive the strip point from the parsed userinfo boundary instead of the digit/colon test). Whichever tradeoff is chosen, please pin it with a test for 'https://user:1234 secret@host'.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // "authority" swallow the rest of the message and a later '@' becomes the | ||
| // strip point. #8136. | ||
| const whitespace = baseUrl.slice(authorityStart).search(/\s/); | ||
| if (whitespace !== -1) end = Math.min(end, authorityStart + whitespace); |
There was a problem hiding this comment.
[Critical] R1-3: For a password containing BOTH @ and a space, this whitespace bound truncates the authority mid-userinfo, so the authority-scoped lastIndexOf('@') above lands on the @ INSIDE the password and stripAt leaks the tail — violating #8136's Expected bullet "A password containing @ is removed in full" on the direct configured-baseUrl path, where main stripped the credential in full. — Failure scenario: A/B-verified: sanitizeProviderBaseUrl('https://user:p@ss word@host.example/v1') → 'https://ss word@host.example/v1' at HEAD (password fragment ss word ships into the /status payload via buildCurrent/model baseUrl); the merge base returned 'https://host.example/v1'. Trigger: a configured provider baseUrl whose password contains both characters. (On the warning path the old segment heuristic leaked this shape too — the regression is the direct baseUrl path.) Suggested fix: when the parsed userinfo extends past this bound (e.g. parsed.password contains %20), do not trust the truncated window for the @ search — strip at the last @ before the parsed host instead. Coordinate with the R1-1/R1-2 fix so all paths agree, and add a test for this input.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| return fallbackAt === -1 ? baseUrl : stripAt(fallbackAt); | ||
| } | ||
| return baseUrl; | ||
| } catch { |
There was a problem hiding this comment.
[Suggestion] R1-4: The strip-decision ladder (authority-scoped @ → findUnescapedUserInfoFallbackAt → return unchanged) now exists TWICE in this function — this added try-branch copy duplicates the pre-existing catch-branch ladder below, with only the parsed.username || parsed.password gate differing. — Concrete cost: the next correction to the strip-point heuristic (which R1-1/R1-2/R1-3 show is coming) must be applied in two places; missing one reintroduces a per-input-class credential leak that tests cannot catch, since each branch covers a different input class. This PR itself is evidence of the drift mode — #8136 existed because two sanitizer implementations diverged.
// one shared ladder, e.g.:
const hasUserInfo = parseSucceeded ? Boolean(parsed.username || parsed.password) : true;
if (!hasUserInfo) return baseUrl;
const stripPoint = authorityAtIndex >= authorityStart
? authorityAtIndex
: findUnescapedUserInfoFallbackAt(baseUrl, authorityStart, authorityEnd);
return stripPoint === -1 ? baseUrl : stripAt(stripPoint);— qwen3.8-max via Qwen Code /review (v0.21.5)
| // If the segment contains a space in the userinfo (e.g. "user:sec ret@host"), | ||
| // new URL() will throw and the fallback uses lastIndexOf('@') within the | ||
| // authority scope. |
There was a problem hiding this comment.
[Suggestion] R1-5: This comment is factually wrong on both counts and contradicts the sibling comment the same PR adds in acpModelUtils.ts: new URL() does NOT throw on space-in-userinfo — WHATWG percent-encodes it and parsing succeeds (verified: new URL('https://user:sec ret@host') → username 'user', password 'sec%20ret'), so the TRY-branch fallback runs, not the catch path; and findUnescapedUserInfoFallbackAt does not search "within the authority scope" — it uses whole-string lastIndexOf('@') and requires the @ to be strictly PAST authorityEnd (the JSDoc above sanitizeSegmentCredentials repeats the same error). — Concrete cost: a maintainer debugging the R1-1 corruption looks in the catch branch for an authority-scoped search, finds neither, and misdiagnoses the most security-sensitive code in this diff.
| // If the segment contains a space in the userinfo (e.g. "user:sec ret@host"), | |
| // new URL() will throw and the fallback uses lastIndexOf('@') within the | |
| // authority scope. | |
| // If the segment contains a space in the userinfo (e.g. "user:sec ret@host"), | |
| // new URL() percent-encodes it, so the whitespace-bounded authority misses | |
| // the '@' and the fallback searches the full string for an '@' past it. |
Please fix the sanitizeSegmentCredentials JSDoc in the same pass.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // this, a pathless URL followed by prose (emails, mentions) lets the | ||
| // "authority" swallow the rest of the message and a later '@' becomes the | ||
| // strip point. #8136. | ||
| const whitespace = baseUrl.slice(authorityStart).search(/\s/); |
There was a problem hiding this comment.
[Suggestion] R1-6: Colon-less (username-only) userinfo containing whitespace is no longer stripped once this bound is in place: sanitizeProviderBaseUrl('https://user @host') returns the input unchanged at HEAD, while the merge base stripped it to https://host (A/B-verified) — so the username renders into the provider status/ACP model list. Low impact (username only, unusual configured-baseUrl shape), and structurally locked in: dropping the fallback's colon requirement to fix this would corrupt the PR's required case 'https://ollama.local — contact admin@example.com'. Please either document at findUnescapedUserInfoFallbackAt that colon-less userinfo past whitespace is an accepted residual, or fold it into the R1-2 heuristic redesign — and pin the chosen behavior with a test.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // to find the real userinfo terminator. #8136. | ||
| const fallbackAt = findUnescapedUserInfoFallbackAt( |
There was a problem hiding this comment.
[Suggestion] R1-7: The fallback's colon/digit-port veto is defeated by IPv6 bracket colons and by trailing punctuation after a port, so the #8136 corruption class still occurs at HEAD for those families: indexOf(':', authorityStart) lands inside the IPv6 literal (portCandidate ':1]:8443', not all digits), the veto never fires, and the whole-string lastIndexOf('@') strips at the prose email. Not a regression — the merge base corrupts these identically and HEAD is strictly better — but the fix's stated purpose ("a pathless URL followed by prose … a later @ becomes the strip point") remains violated for realistic ollama/localhost IPv6 base URLs and punctuation-adjacent ports. — Failure scenario: probe-verified at HEAD: sanitizeProviderBaseUrl('https://[::1]:8443 — contact admin@example.com') → 'https://example.com' (expected unchanged); same for 'http://[::1]:11434 — …', 'https://[::1] — …', 'https://api.example:8443, contact admin@example.com' (trailing comma/./;), and through the warning pipeline — while 'https://api.example:8443/v1, contact admin@example.com' stays unchanged. Verified cheap and test-safe: skipping IPv6 brackets when locating the port colon plus /^\d+[,.;:!?]?$/ for the veto fixes all six inputs and the PR's 58 tests still pass. Please add regression cases mirroring the new tests for 'https://[::1]:8443 — contact admin@example.com' and 'https://api.example:8443, contact admin@example.com' (here or as a follow-up issue so these families aren't silently dropped from the #8136 fix).
— qwen3.8-max via Qwen Code /review (v0.21.5)
The whitespace-bound on findAuthorityEnd (round 2) fixed the portless URL + email corruption but reintroduced three Critical regressions for space-containing passwords (R1-1/R1-2/R1-3): the fallback searched the whole string for '@' and a later prose '@' became the strip point, deleting host/path/prose; a digit-prefix password leaked; and a password containing both '@' and a space was truncated mid-credential. Redesign the strip-point heuristic around two signals: - findAuthorityEnd no longer bounds by whitespace; the fallback uses the last '@' (userinfo terminator) so a password's inner '@' precedes it and prose '@' after the path is excluded. - isLikelyPortlessMisparse vetoes when the segment between the first ':' and the next whitespace is all digits - a port, not a password - so a portless URL + prose email that WHATWG misparses as userinfo is left unchanged instead of corrupted. The veto only fires when the strip candidate '@' is past the colon (real userinfo like user@host:99999 has '@' before the colon). The try/catch branches share one strip-decision ladder (R1-4), and the misleading round-2 comments about new URL() throwing on space-in-userinfo are corrected (R1-5): WHATWG percent-encodes the space and parsing succeeds. KNOWN RESIDUAL (documented tradeoff, QwenLM#8136 R1-2): a password that starts with digits followed by a space (e.g. user:1234 secret@host) is vetoed like a portless URL and leaks. These two input classes are locally indistinguishable at the veto; protecting the common portless-URL-with-prose case is the chosen tradeoff. No-port pathless URLs + email (e.g. ollama.local - contact admin@...) corrupt the same way `main` does (not a regression, R1-6/R1-7).
|
@qwen-code /review --timeout=240 |
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.
Test Plan (not a blocker): 21 tests pass — this review observed 17101 passed.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| let stripPoint = -1; | ||
| if (authorityAtIndex >= authorityStart && !portlessMisparse) { | ||
| stripPoint = authorityAtIndex; | ||
| } |
There was a problem hiding this comment.
[Critical] R2-1: Pathless URLs followed by prose containing '@' are still corrupted by this sanitizer — the wide authority runs to end of segment and the strip point lands on the prose email's '@', because isLikelyPortlessMisparse cannot veto colon-less input (returns false when no colon exists) and cannot veto credential input (the first colon is the userinfo colon, so portCandidate is not all digits). The whole-segment delegation in sanitizeSegmentCredentials is what feeds the prose in. The veto added by the redesign protects only the with-colon all-digit-port subcase; the colon-less and credentialed pathless shapes remain broken, and no test covers them. This is the same corruption class #8136 was filed for, reintroduced for shapes main handled correctly (round 1's review body asked for space-bounding plus pathless/portless tests). — Failure scenario: A/B-verified through the real warning pipeline against the built merge base: 'Cannot reach https://api.example - contact admin@example.com' → 'Cannot reach https://example.com' at HEAD (base: unchanged — the issue's own example sentence minus the port); 'Cannot reach https://user:pass@host.example:8443 - contact admin@example.com' → 'Cannot reach https://example.com' (base: 'Cannot reach https://host.example:8443 - contact admin@example.com'); this PR's own test-5 input 'Failed https://user:pass@h admin@example.com' → 'Failed https://example.com' (base: 'Failed https://h admin@example.com'). The displayed endpoint becomes the email's domain and the contact prose is deleted.
Suggested direction: extend the veto (or the strip-point choice) to these shapes — e.g. when the authority contains whitespace and the text before the first whitespace contains neither ':' nor '@', treat everything past the whitespace as prose; or prefer the last '@' before the first whitespace when one exists (this keeps the space-in-password rows green). Pin both directions with tests ('https://api.example - contact admin@example.com' unchanged, plus a credentialed pathless case).
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const at = baseUrl.lastIndexOf('@'); | ||
| if (at < authorityStart || authorityEnd >= at) { | ||
| if (at < authorityStart) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
[Critical] R2-2: The rewritten findUnescapedUserInfoFallbackAt searches the WHOLE string for the last '@' and dropped the old authorityEnd >= at bound and the colon requirement. When new URL() throws (catch branch) and no '@' sits inside the wide authority, a prose '@' anywhere after the path becomes the strip point, and stripAt deletes host, path, and prose up to the email — the merge base's fallback (colon required, '@' strictly past authorityEnd) returned these inputs unchanged. — Failure scenario: A/B-verified: sanitizeProviderBaseUrl('https://api.example%/v1, contact admin@example.com') → 'https://example.com' at HEAD (the invalid '%' makes new URL() throw; no colon anywhere, so the veto returns false), while the merge base returns the input unchanged; same for 'https://my service/v1 — contact admin@example.com' → 'https://example.com' (base unchanged). The input here is free-form provider error text routed through sanitizeSegmentCredentials, not a validated baseUrl.
Suggested direction: bound the fallback candidate — reject an '@' when the region between the last '/' preceding it and the candidate contains whitespace; or restore the colon requirement on the catch branch (treat "no port information available" as "do not strip"). Please add a regression test for 'https://my service/v1 — contact admin@example.com'.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const spaceIdx = afterColon.search(/\s/); | ||
| const portCandidate = | ||
| spaceIdx === -1 ? afterColon : afterColon.slice(0, spaceIdx); | ||
| return /^\d+$/.test(portCandidate); |
There was a problem hiding this comment.
[Critical] R1-2: Round-1 blocker R1-2 still stands as behaviour. A password starting with digits followed by a space is vetoed like a portless-URL misparse and leaks in full; the redesign documents the leak as KNOWN RESIDUAL and pins the leaking output in a test instead of fixing it. The two input classes (portless-URL-with-prose vs digit-prefix password) are genuinely hard to separate at this veto, and the tradeoff is openly documented — but it has no maintainer sign-off in the record, and it is a credential-exposure regression versus main in the function the issue exists to harden. — Failure scenario: A/B-verified: sanitizeProviderBaseUrl('https://user:1234 secret@host') returns the input unchanged at HEAD (leaking user:1234 secret@ into the serve /status payload via errors[].error / models[].baseUrl / current.baseUrl), while the merge base returned 'https://host'; same for 'https://user:12 34@host' and the with-path variant 'https://user:1234 secret@host.example/v1'. Issue #8136's Expected bullet 2 is that passwords are "removed in full".
Suggested direction: add a discriminator the veto can use (e.g. when new URL() proves userinfo exists, compare the parsed username against host-shape before applying the digit veto — documenting the residual bare-label-host cases such as ollama), or obtain explicit maintainer sign-off for the documented tradeoff plus a follow-up issue, so the pinned leak is a recorded decision rather than an open Critical.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Credentials + pathless + email: WHATWG misparses the whole thing as | ||
| // userinfo and the host becomes the email domain. This is the same | ||
| // corruption `main` has (not a regression); the pathless-with-port shape | ||
| // above is the one this PR protects. |
There was a problem hiding this comment.
[Suggestion] R2-3: This test's comment asserts "This is the same corruption main has (not a regression)" — that parity claim is false, and the test under-asserts: for its own input, main preserved host and prose while HEAD collapses the message to the email's domain. The sole assertion (not.toContain('user:pass@')) passes for either output, and the title's "keeps host and prose" is checked by no assertion. — Concrete cost: A/B-verified — 'Failed https://user:pass@h admin@example.com' → 'Failed https://h admin@example.com' on the merge base vs 'Failed https://example.com' at HEAD. The green test now documents a real regression as accepted pre-existing behavior, which is exactly the justification that will be cited against fixing the pathless corruption finding above.
| // Credentials + pathless + email: WHATWG misparses the whole thing as | |
| // userinfo and the host becomes the email domain. This is the same | |
| // corruption `main` has (not a regression); the pathless-with-port shape | |
| // above is the one this PR protects. | |
| // Credentials + pathless + email: WHATWG misparses the whole thing as | |
| // userinfo and the host becomes the email domain. `main` preserved host | |
| // and prose here (`Failed https://h admin@example.com`); this assertion | |
| // pins only the no-leak property until the corruption is fixed. |
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // A portless URL followed by prose containing '@' (e.g. | ||
| // "https://api.example:8443 - contact admin@example.com") is misparsed by | ||
| // WHATWG as userinfo (username="api.example", password="8443..."). The |
There was a problem hiding this comment.
[Suggestion] R2-4: "portless" contradicts what this veto actually covers. The veto only fires when a colon exists — it protects PATHLESS URLs that HAVE a port (the comment's own example, https://api.example:8443); genuinely portless URLs are precisely the class still corrupted (see the Critical above). The helper name isLikelyPortlessMisparse and "portless-URL-with-prose case" below carry the same drift, while this PR's own sibling wording uses "pathless" (acpModelUtils.test.ts "Pathless URL + prose email", the workspace test's "pathless port URL"). — Concrete cost: probe-verified asymmetry at HEAD: 'https://api.example:8443 - contact admin@example.com' → unchanged (protected), 'https://api.example - contact admin@example.com' → 'https://example.com' (corrupted). A maintainer auditing #8136 coverage reads "portless" and concludes the no-port shape is handled; it is the shape that is broken.
| // A portless URL followed by prose containing '@' (e.g. | |
| // "https://api.example:8443 - contact admin@example.com") is misparsed by | |
| // WHATWG as userinfo (username="api.example", password="8443..."). The | |
| // A pathless URL with a port followed by prose containing '@' (e.g. | |
| // "https://api.example:8443 - contact admin@example.com") is misparsed by | |
| // WHATWG as userinfo (username="api.example", password="8443..."). The |
Please rename isLikelyPortlessMisparse accordingly (e.g. isLikelyPathlessProseMisparse) in the same pass.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| ); | ||
| }); | ||
|
|
||
| it('does not strip a later email @ from a portless URL with a port (#8136)', async () => { |
There was a problem hiding this comment.
[Suggestion] R2-4: This title — "portless URL with a port" — is self-contradictory: the input has port :8443 and no path; the sibling test above correctly says "pathless port URL". The test itself is a near-duplicate of that sibling (byte-identical after normalizing — → -; probe-verified that the dash selects no different code path in HEAD or base). — Concrete cost: a reader scanning the suite sees "portless" and believes the no-port shape is covered — it is not (no test exercises 'https://host - contact admin@example.com', which the code still corrupts); the contradictory title hides that gap.
Suggested fix: rename to describe the actual variant (e.g. 'does not strip a later email @ from a pathless URL with a port (hyphen separator) (#8136)') or merge with the sibling; once the portless behavior is fixed, add a genuinely portless regression case.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if (isLikelyPortlessMisparse(baseUrl, authorityStart, authorityEnd)) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-5: The portless-misparse veto is computed twice with identical arguments per call — this fallback re-runs isLikelyPortlessMisparse(baseUrl, authorityStart, authorityEnd) which its only caller (the unified ladder above) has already computed. When the veto is true the fallback necessarily returns -1 (dead work); when false, the strip decision still lives in two sites. — Concrete cost: the next correction to the strip-point heuristic (which this round's Criticals show is coming) must be applied in two places; missing one reintroduces a per-input-class defect that tests cannot catch, since each site covers a different input class — the same drift mode that produced #8136 and that round-1 R1-4 warned about.
Suggested fix: guard the call site — if (stripPoint === -1 && !portlessMisparse) { stripPoint = findUnescapedUserInfoFallbackAt(...); } — and drop this internal re-check (or pass the computed flag in).
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const colon = baseUrl.indexOf(':', authorityStart); | ||
| if (colon === -1 || colon > authorityEnd) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-7: Round-1 finding R1-7 still stands — this colon search lands inside IPv6 bracket literals (portCandidate becomes ':1]:8443', not all digits) and trailing punctuation defeats the all-digits test, so the #8136 corruption class persists for those families. Not a regression — the merge base corrupts these identically (re-verified this round by A/B probe) — but round 1 asked for a fix or a follow-up issue so these families are not silently dropped from the #8136 fix, and this diff contains neither. — Failure scenario: probe-verified at HEAD (and base alike): sanitizeProviderBaseUrl('https://[::1]:8443 — contact admin@example.com') → 'https://example.com' (expected unchanged); same for 'https://api.example:8443, contact admin@example.com'. Bracketed-IPv6 base URLs (http://[::1]:11434 shapes) are realistic in this codebase's serve/ollama paths.
Suggested direction: skip IPv6 brackets when locating the port colon and accept trailing punctuation in the veto (e.g. /^\d+[,.;:!?]?$/) — round 1 verified this fixes the family with the suite green — or file a follow-up issue tracking these families; add regression cases mirroring the new tests.
— qwen3.8-max via Qwen Code /review (v0.21.5)
…egments Round-2 review found the whitespace-bound redesign still corrupted pathless URLs followed by prose (R2-1) and URL-throwing shapes (R2-2), because the fallback searched the whole string for '@' and a prose email '@' became the strip point. Round-1's digit-prefix password leak (R1-2) also stood as an unresolved credential-exposure regression. Redesign the strip-point heuristic around the URL's path shape: - findUserInfoStripPoint: for pathless URLs (no / ? #, authority runs to end) use the FIRST '@' whose next char is host-shaped (so a prose email '@' is not chosen over the real terminator); for bounded authority use the last '@' within it (a password's inner '@' precedes the terminator). Fall back to the full-string last '@' for passwords containing / ? # (new URL() throws), rejecting when the run between the last '/' and the candidate contains whitespace (prose guard). - isLikelyPathlessPortMisparse: veto when a colon precedes the candidate '@' and the segment up to the first whitespace is all digits (a port), AND the '@' is past the colon (real userinfo like user@host:99999 has '@' before the colon). Protects pathless URL + prose email. - isPathlessProse: when the pathless authority has whitespace and the text before it has neither ':' nor '@', treat as non-credential. sanitizeProviderWarningSegment now delegates to sanitizeProviderBaseUrl for the whole segment first (handles space-containing-credential URLs that URL_LIKE_PATTERN cannot match past whitespace, and the '@'-in-password shape via the last-'@' rule), falling back to per-URL replace. This replaces the old first-'@' + hasCredentialPrefix heuristic that stripped a prose email's '@' on pathless URLs. KNOWN RESIDUAL (R1-2, needs maintainer sign-off): a password starting with digits followed by a space (user:1234 secret@host) is vetoed like a pathless port URL and leaks. These two classes are locally indistinguishable at the veto; the tradeoff is documented and pinned. No-port pathless + email and IPv6/trailing-punctuation families corrupt the same way main does (not regressions, R1-6/R1-7). Fixes QwenLM#8136
|
@qwen-code /review --timeout=240 |
|
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 |
|
@qwen-code /review --timeout=360 |
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.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (isPathless) { | ||
| let at = baseUrl.indexOf('@', authorityStart); | ||
| while (at !== -1 && at < authorityEnd) { |
There was a problem hiding this comment.
[Critical] The pathless branch picks the FIRST host-shaped @ as the userinfo terminator; when the password itself contains @, the strip point lands inside the password and leaks the tail — a regression versus main, which stripped at the authority's last @ (the bounded branch below still does). The doc comment above states the intended rule — "the last '@' within the authority is the terminator" — but this branch contradicts it for inputs without / ? #. — Failure scenario: a configured baseUrl https://user:p@ss@host (pathless — the usual baseUrl shape) → https://ss@host: password fragment ss is emitted into the serve /status payload (models[].baseUrl, current.baseUrl) and ACP responses; https://user:p@ss word@host → https://ss word@host. new URL() parses username user, password p@ss, confirming the true terminator is the LAST @. The bounded sibling (…p@ssw0rd-tail@broken.example/v1) is tested and correct; no test covers the pathless shape. Probe-verified A/B against the merge base.
Suggested direction: when beforeWs contains an @, take the LAST host-shaped @ before the first whitespace; keep the first-@ scan only for the prose shape. Add rows ['https://user:p@ss@host', 'https://host'] and ['https://user:p@ss word@host', 'https://host'].
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const fullAt = baseUrl.lastIndexOf('@'); | ||
| if (fullAt < authorityStart) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
[Critical] The full-string @ fallback dropped the old colon-in-authority precondition (the deleted findUnescapedUserInfoFallbackAt required a : within the authority), so a new URL()-throwing input whose later path/query segment contains an @ with no whitespace between the last / and it is stripped there — host, path and prose deleted. Sibling entrance of R2-2, whose own inputs (whitespace-separated prose) are fixed and pinned. — Failure scenario: sanitizeProviderBaseUrl('https://my service/v1/admin@example.com') → 'https://example.com' (space in host throws; between = /v1/admin has no whitespace, so the prose guard passes); same for '?user=admin@example.com' and 'https://api.example%/v1/contact@admin.example'. A/B-measured against the built merge base, which returns all of these unchanged.
Suggested fix: restore the colon precondition before accepting fullAt — const colon = baseUrl.indexOf(':', authorityStart); if (colon === -1 || colon > authorityEnd) return -1; The shapes this fallback exists for (/ ? # in the password, e.g. https://user:p/x@api.example/v1) all have a userinfo colon, so they still strip.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| authorityEnd, | ||
| ); | ||
| return fallbackAt === -1 ? baseUrl : stripAt(fallbackAt); | ||
| const HOST_SHAPED_CHAR = /[A-Za-z0-9.[\]-]/; |
There was a problem hiding this comment.
[Critical] HOST_SHAPED_CHAR omits _ (and non-ASCII), so a pathless URL whose host starts with a character outside this class is never stripped even though new URL() parsed real userinfo — a full credential leak, and a regression versus the merge base (A/B-measured against the built base: https://user:pass@_host → unchanged at HEAD, https://_host on base). — Failure scenario: a configured internal endpoint https://user:pass@_internal:8080 (underscore-leading hostnames are common internally; IDN hosts likewise) is displayed verbatim in the providers-status payload and model-config warnings instead of stripped.
| const HOST_SHAPED_CHAR = /[A-Za-z0-9.[\]-]/; | |
| const HOST_SHAPED_CHAR = /[A-Za-z0-9._[\]-]/; |
(or, when new URL() proves userinfo exists, strip at the parser-confirmed terminator instead of the char-class heuristic — that would also cover R3-4's whitespace-after-@ sibling). Add a regression row for https://user:pass@_host.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| baseUrl[at + 1] !== undefined && | ||
| HOST_SHAPED_CHAR.test(baseUrl[at + 1]) |
There was a problem hiding this comment.
[Critical] Requiring a host-shaped char AFTER the @ rejects the real terminator when it is followed by whitespace, a tab, or end-of-string — the scan exhausts, stripPoint is -1, and the full credential is returned unchanged. — Failure scenario: sanitizeProviderBaseUrl('https://user:pass@\thost') returns the input unchanged even though WHATWG strips the tab and parses username user, password pass — a credential the sanitizer exists to remove; 'https://user:pass@ host' leaks likewise. Round-2 code stripped both. A tab inside a warning segment is reachable because findUrlSegmentEnd splits on CR/LF only. Probe-verified A/B.
Suggested direction: when the authority region before the @ is userinfo-shaped (contains : between authorityStart and the @) and has no whitespace before it, accept the terminator even if the following char is whitespace/undefined.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const isPathlessProse = | ||
| isPathless && | ||
| firstWs !== -1 && | ||
| !beforeWs.includes(':') && | ||
| !beforeWs.includes('@'); |
There was a problem hiding this comment.
[Critical] isPathlessProse vetoes any pathless URL whose first whitespace-delimited token contains no : and no @, so credentials whose whitespace sits inside the USERNAME (before the first :) leak in full — a regression versus the merge base (A/B probe-measured), and not documented as an accepted tradeoff the way the R1-2 residual is. — Failure scenario: sanitizeProviderBaseUrl('https://user name:pass@host') returns the input unchanged — user name:pass@ leaks into the serve /status and ACP baseUrl payloads — while new URL() parses username user%20name, password pass; merge base returned https://host. Username-only variant https://user pass@host leaks too.
Suggested direction: when new URL() succeeds and parsed.password is non-empty, treat the input as real credentials despite isPathlessProse — kept combined with the port veto, because the R2-1 prose shape (api.example:8443 - contact admin@…) parses with a non-empty password too. https://user pass@host remains locally indistinguishable from prose and needs an explicit tradeoff decision like R1-2's.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const isPathless = authorityEnd === baseUrl.length; | ||
| const beforeWs = firstWs === -1 ? authority : authority.slice(0, firstWs); |
There was a problem hiding this comment.
[Critical] The prose-email guard is gated on isPathless (authorityEnd === baseUrl.length), so any /, ? or # in the trailing prose disables it; the with-path branch below then strips at the PROSE email's @, destroying the real host. Sibling entrance of R2-1, whose own inputs (no delimiter in prose) are fixed and pinned. — Failure scenario: 'Cannot reach https://ollama.local - email admin@example.com or check /var/log/qwen' → 'Cannot reach https://example.com or check /var/log/qwen' — host replaced by the email's domain although no credential existed; probe-verified for ?subject= and #support prose variants too. The base code left these unchanged (hasCredentialPrefix required a : before the first @). The port veto masks only the all-digit-port variant.
Suggested fix: drop the isPathless && gate (veto whenever beforeWs has neither : nor @, regardless of a later delimiter), and in the with-path branch never accept an @ that lies after the first whitespace as the terminator. Add a test for the /var/log prose shape expecting unchanged.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const colon = baseUrl.indexOf(':', authorityStart); | ||
| if (colon === -1 || colon > authorityEnd) { |
There was a problem hiding this comment.
[Suggestion] R1-7: Round-1 finding R1-7 still stands — this colon search lands inside IPv6 bracket literals, and the all-digit portCandidate test below is defeated by trailing punctuation, so the #8136 corruption class survives one punctuation mark away from the pinned tests. — Failure scenario (all probe-verified): 'https://[::1]:8443 - contact admin@example.com' → 'https://example.com' (colon found inside [::1], portCandidate = ':1]:8443', veto never fires); 'https://api.example:8443, contact admin@example.com' → 'https://example.com' ('8443,' fails /^\d+$/); same for 'https://ollama.local: please contact admin@example.com' (empty candidate) and ':8443a'. The veto was rewritten in this diff and its doc comment claims the general class; the merge base corrupted these identically, so this is the class the PR targets rather than a new regression.
Suggested direction: when baseUrl[authorityStart] === '[', locate the port colon after the closing ]; accept a leading digit run terminated by punctuation/whitespace instead of an all-digit token; and mask bracketed sections when computing isPathlessProse.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| function sanitizeProviderWarningSegment( | ||
| segment: string, | ||
| markerLength: number, | ||
| _markerLength: number, |
There was a problem hiding this comment.
[Suggestion] _markerLength is dead: the rewritten body never reads it, but the sole caller (~line 263) still computes and passes next.marker.length. This is a module-private function with exactly one call site, so the _-parked parameter should simply be deleted — the signature currently implies marker-relative offsets are still load-bearing (they were before this diff), which invites a future edit to reintroduce the fragile arithmetic this PR removes. — Concrete cost: misleading signature in a function rewritten three rounds in a row, plus dead computation at the call site.
| function sanitizeProviderWarningSegment( | |
| segment: string, | |
| markerLength: number, | |
| _markerLength: number, | |
| function sanitizeProviderWarningSegment( | |
| segment: string, | |
| ): string { |
(and drop next.marker.length at the call site).
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // #8136: pathless URL + prose email shapes. WHATWG misparses these as | ||
| // userinfo; the veto (all-digit port before first whitespace) protects the | ||
| // with-port shape, and the pathless-prose guard protects the no-colon shape. |
There was a problem hiding this comment.
[Suggestion] Issue #8136's literal bug-1 repro has a PATH ('https://api.example:8443/v1 — contact admin@example.com'), but no added row pins the with-path shape — every #8136 port+prose row here is pathless, which reaches the strip decision through a different mechanism (authority bounded by /, veto's portCandidate sliced at the slash). Behavior is currently correct (probe-verified unchanged), but the maintainer-requested coverage on the issue ("the two repro cases from the issue" pinned) is not met for repro 1. — Concrete cost: a future edit to findAuthorityEnd, the try-branch gate, or the veto could regress the issue's literal repro with no test going red.
Suggested fix: add a pinned row for the verbatim repro, e.g. ['https://api.example:8443/v1, contact admin@example.com', 'https://api.example:8443/v1, contact admin@example.com'].
— qwen3.8-max via Qwen Code /review (v0.21.6)
Round-3 review found six credential-exposure regressions in the sanitizer, all rooted in the host-shaped-char heuristic picking the wrong '@' as the userinfo terminator: - R3-1: pathless URL with '@' in the password (user:p@ss@host) stripped at the FIRST host-shaped '@', leaking the password tail (ss@host). - R3-3: HOST_SHAPED_CHAR omitted '_', so user:pass@_host leaked in full. - R3-4: requiring a host-shaped char AFTER the '@' rejected the real terminator when followed by whitespace/tab (user:pass@\thost leaked). - R3-2: the full-string '@' fallback dropped the colon-in-authority precondition, corrupting prose with '@' in a path segment. Delegate to new URL() instead: when the parser succeeds and reports userinfo, the bounded-authority branch takes the LAST '@' within the authority (a password's inner '@' precedes the terminator); the pathless branch scans only up to the first whitespace, since a real userinfo terminator precedes whitespace while a prose email '@' does not. The catch-branch (parser threw) keeps the fallback with the colon precondition restored. Drop the now-unused HOST_SHAPED_CHAR and the dead _markerLength parameter on sanitizeProviderWarningSegment. Add regression rows for each fixed shape plus the QwenLM#8136 repro-1 with-path case. QwenLM#8136
|
@qwen-code /review --timeout=360 |
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.
Test Plan (not a blocker): 21 tests pass — this review observed 17792 passed.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // KNOWN RESIDUAL: digit-prefix + space password is vetoed like a pathless | ||
| // port URL and leaks. Documented tradeoff (#8136 R1-2). | ||
| ['https://user:1234 secret@host', 'https://user:1234 secret@host'], |
There was a problem hiding this comment.
[Critical] R1-2: Round-1 blocker R1-2 still stands as behaviour. A password starting with digits followed by a space is vetoed like a portless-URL misparse and leaks in full; this row pins the leak as intended behaviour, but the record contains no maintainer sign-off for the tradeoff and no follow-up issue. — Failure scenario: probe-verified A/B against the merge base: sanitizeProviderBaseUrl('https://user:1234 secret@host') and ('https://user:12 34@host') return the input unchanged at HEAD — user:1234 secret@ ships into the serve /status payload (models[].baseUrl, current.baseUrl, errors[].error) and ACP responses — while the merge base returned https://host. WHATWG confirms real userinfo (username user, password 1234%20secret); issue 8136 expects a password to be "removed in full". Suggested direction: add a discriminator the veto can use (e.g. when new URL() proves userinfo exists, compare the parsed username against host-shape before applying the digit veto), or obtain explicit maintainer sign-off plus a follow-up issue so the pinned leak is a recorded decision rather than an open Critical.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const authority = baseUrl.slice(authorityStart, authorityEnd); | ||
| const firstWs = authority.search(/\s/); | ||
| const scanEnd = firstWs === -1 ? authorityEnd : authorityStart + firstWs; | ||
| const at = baseUrl.lastIndexOf('@', scanEnd - 1); |
There was a problem hiding this comment.
[Critical] R3-5: Round-3 blocker R3-5 still stands, with new sibling shapes. Pathless URLs whose userinfo contains whitespace leak (or partially leak) credentials: this scan bounds the '@' search at the first whitespace, so it cannot see a terminator that sits after it — even though new URL() parsed the userinfo authoritatively. — Failure scenario: probe-verified A/B vs the merge base: 'https://user name:pass@host' → unchanged (full leak; the prose veto also fires, contradicting the comment at 285-287); 'https://user:pass word@host' and 'https://user:sec ret@host' → unchanged (full leak — findUserInfoStripPoint computes the correct terminator but this branch discards it); 'https://user:p@ss word@host' → 'https://ss word@host' (password fragment exposed, host destroyed). Base returned https://host for all. The leaks reach the serve /status payload (workspace-providers-status.ts:176/326) and ACP wire messages (acpAgent.ts:1987/6308/6332/10687); no test row pins any of these shapes, and the comments at 178-181 / 200-207 / 263-267 / 285-287 assert the opposite of the executed behaviour. Suggested direction: when new URL() succeeds and reports userinfo, strip at the parser-authoritative terminator (e.g. the last '@' for pathless, veto-free input) instead of the first-whitespace scan — or pin these shapes as KNOWN RESIDUAL rows like the digit-prefix case and fix the comments.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const isPathlessProse = | ||
| isPathless && | ||
| firstWs !== -1 && |
There was a problem hiding this comment.
[Critical] R3-6: Round-3 blocker R3-6 still stands. This prose guard is gated on isPathless, so any /, ? or # in the trailing prose disables it, and the bounded branch then strips at the PROSE email's '@', destroying the real host. New this round: the variant where new URL() succeeds. — Failure scenario: probe-verified through the real warning pipeline, A/B vs base: 'Cannot reach https://ollama.local - email admin@example.com or check /var/log/qwen' → 'Cannot reach https://example.com or check /var/log/qwen' (base: unchanged) — host replaced by the email's domain although no credential existed; colon-less '… - admin@example.com/x' → 'https://example.com/x' (regression vs main, whose deleted hasCredentialPrefix required a colon); ?subject= / #support prose variants corrupt likewise. For '…:8443 - contact admin@example.com/profile' the veto DID fire (stripPoint === -1) but the try-branch never consults it. Suggested direction: drop the isPathless && gate (veto whenever beforeWs has neither ':' nor '@'), consult the veto on the parse-success path (if (stripPoint === -1) return baseUrl;), and never accept an '@' after the first whitespace as terminator in the bounded branch; add a test for the /var/log prose shape expecting unchanged.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Bounded authority or pathless-without-prose: the last '@' within the | ||
| // authority is the terminator. `new URL()` refines this on the try-branch when | ||
| // it parses successfully; the catch-branch (parser threw) uses it directly. | ||
| const at = baseUrl.lastIndexOf('@', authorityEnd - 1); |
There was a problem hiding this comment.
[Critical] The catch-branch strip-point scan is not bounded by the first whitespace for pathless inputs: when new URL() throws, a prose '@' that sits AFTER the first whitespace becomes the strip point, and host + prose are deleted. The try branch bounds its pathless scan at the first whitespace (scanEnd); this path does not mirror that bound. — Failure scenario: probe-verified: 'https://user:pass@host - ping admin@' → 'https://'; 'https://user@host - contact admin@exam%zz.com' → 'https://exam%zz.com'; 'https://user:pass@host - email admin@localhost for help' → 'https://localhost for help' (new URL() throws on each input). The merge base preserved all three messages (credential stripped, host + prose kept). Reachable via the whole-segment delegation in sanitizeProviderWarningSegment (workspace-providers-status.ts:305). Suggested fix:
const scanEnd =
isPathless && firstWs !== -1 ? authorityStart + firstWs : authorityEnd;
const at = baseUrl.lastIndexOf('@', scanEnd - 1);plus regression tests for the three shapes.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // whitespace beyond that point (e.g. `user name:pass@host`) is then | ||
| // indistinguishable from prose and is left as a documented residual. | ||
| const authority = baseUrl.slice(authorityStart, authorityEnd); | ||
| const firstWs = authority.search(/\s/); |
There was a problem hiding this comment.
[Critical] This bound uses the JS /\s/ class, which diverges from WHATWG: tab/CR/LF are stripped pre-parse, and Unicode whitespace (U+00A0, U+2028/U+2029, FF, VT, U+3000, U+202F, U+1680, U+2000–U+200A, U+205F, U+FEFF) is percent-encoded into the password rather than stripped. Pathless userinfo containing any of them is returned unchanged — a full credential leak (merges the round-4 Unicode-whitespace finding). — Failure scenario: probe-verified A/B: 'https://user:pa\tss@host', 'https://user:pass\n@host', and 'https://user:pa\u00a0ss@host' (plus U+2028/U+2029/FF/VT/U+3000/U+202F variants) all leak at HEAD; the merge base stripped every one to https://host. End-to-end: warning 'Cannot reach https://user:pa\u00a0ss@ollama.local' ships the credential in errors[].error (URL_LIKE_PATTERN's [^\s]+ also stops at the char); the tab shape leaks through the real warning pipeline too; nbsp is a common copy-paste artifact from web pages/PDFs into config and error text. Suggested direction: do not derive the bound from /\s/ — when new URL() confirms userinfo, strip via the parser-authoritative terminator, or scan only for WHATWG authority terminators (/ ? #, with tab/CR/LF removal) and treat all other whitespace as valid userinfo content.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const parsed = new URL(baseUrl); | ||
| if (parsed.username || parsed.password) { |
There was a problem hiding this comment.
[Critical] Backslash-userinfo credentials (DOMAIN\user:pass@proxy) leak in full: WHATWG parses \ as an authority terminator on special schemes, so new URL() reports empty username/password and this gate returns the input unchanged; the deleted first-'@' segment strip in sanitizeProviderWarningSegment was the compensation. — Failure scenario: probe-verified end-to-end A/B: ModelsConfig error 'Cannot reach https://CORP\jsmith:secret@proxy.corp:8080 (check proxy)' ships the full corporate credential in errors[].error at HEAD; the merge base stripped it to 'Cannot reach https://proxy.corp:8080 (check proxy)'. Also leaks via current.baseUrl and the modelConfigUtils.ts disambiguation warnings (which re-enter the warning pipeline). Requires \ before the colon; the user-only no-colon shape is base parity. Suggested direction: fall back to the heuristic strip point when the authority contains \ (around this gate), or keep a bounded first-'@' segment fallback for inputs the parser reports as userinfo-less; add a regression test.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| return fullAt; | ||
| } | ||
|
|
||
| function findAuthorityEnd(baseUrl: string, authorityStart: number): number { |
There was a problem hiding this comment.
[Critical] findAuthorityEnd does not treat \ as a WHATWG authority terminator on special schemes, so 'https://user:pass@host\…@prose' is misclassified as pathless and the pathless scan strips at the prose '@' — corrupting the message even though new URL() parsed the userinfo unambiguously. — Failure scenario: probe-verified A/B: 'Failed https://user:pass@host\admin@example.com rest of text' → 'Failed https://example.com rest of text' at HEAD (base: 'Failed https://host\admin@example.com rest of text'); same for 'Cannot reach https://user:pass@proxy.corp:8080\admin@example.com (check proxy)' → 'Cannot reach https://example.com (check proxy)', the no-trailing-whitespace variant, and the non-special foo://… catch-branch variant. new URL() reports username user, host host, pathname /admin@example.com… — only this boundary is wrong (/ and ? controls strip correctly). Suggested fix: also stop at \ here when the scheme is a WHATWG special scheme (http/https/ws/wss/ftp); handle the non-special catch branch separately (e.g. prefer the first '@' when the parser throws); add regression tests for all four shapes.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| function isLikelyPathlessPortMisparse( | ||
| baseUrl: string, | ||
| authorityStart: number, | ||
| authorityEnd: number, |
There was a problem hiding this comment.
[Suggestion] The name and docstring say "pathless", but this veto is invoked unconditionally for bounded authorities too — and that bounded application is load-bearing. — Concrete cost: for 'https://my service:8080/users/admin@example.com' (new URL() throws on the space-in-host), this veto is the only thing returning -1; a mutation scoping it to pathless-only flips the output to 'https://example.com' (probe-verified). A future maintainer "fixing" the function to match its name silently re-introduces the issue 8136 corruption class for bounded shapes. Suggested fix: rename (e.g. isLikelyPortProseMisparse) and add one docstring line: the veto intentionally also applies to bounded authorities, where it protects host:port URLs whose path/query contains '@' when new URL() throws.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // would otherwise report as userinfo. A veto is not final, though: a pathless | ||
| // URL whose userinfo contains whitespace (e.g. `user name:pass@host`) also | ||
| // trips the prose guard but IS a real credential that `new URL()` confirms, | ||
| // so the parser gets the final say below. #8136. |
There was a problem hiding this comment.
[Suggestion] The comments this diff adds describe behaviour the code does not have, and contradict each other about the pathless-whitespace class. — Concrete cost: probe-verified — 'https://user name:pass@host' leaks unchanged although this comment claims the parser "gets the final say", the doc at 285-287 claims it "is not falsely vetoed" (the veto fires), the try-branch comment at 200-207 calls it a "documented residual" while only the digit-prefix shape is pinned (test line 281), and the KNOWN RESIDUAL wording understates the leaking class. A maintainer investigating a credential leak reads these comments and fixes the wrong branch, or treats an intended tradeoff as a regression. Suggested fix: rewrite the comments to state the actual behaviour (pathless credentials with whitespace in userinfo are left unchanged; the parser-override applies only to bounded authorities) and broaden the KNOWN RESIDUAL wording from "digit-prefix + space password" to "pathless URL with whitespace in userinfo" — or make the code match the comments.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const lastSlashBeforeAt = baseUrl.lastIndexOf('/', fullAt); | ||
| const between = | ||
| lastSlashBeforeAt >= authorityStart |
There was a problem hiding this comment.
[Suggestion] The fallback prose guard only rejects candidates whose run from the last '/' contains whitespace, so a whitespace-free path segment embedding an email is stripped there whenever a typo'd non-digit port supplies the authority colon. — Failure scenario: probe-verified: 'https://ollama:1143x/v1/reset/user@example.com' and 'https://localhost:8O80/api,user@example.com' → 'https://example.com' — host, path and prose prefix deleted because between (/reset/user, /api,user) has no whitespace; the pinned sibling survives only because of its space. Merge-base parity (base corrupted both identically) — a residual hole in the guard this diff adds, not a regression. Suggested direction: additionally require userinfo structure in the path run (e.g. a ':' inside between), accepting the user:p/x@host password-with-slash shape as the documented tradeoff — or track this family in a follow-up issue.
— qwen3.8-max via Qwen Code /review (v0.21.6)
Round-4 review found the parser-based terminator still mis-handled several credential shapes. Replace the new URL()-gated heuristic with a single structural scan of the authority: - Terminator is the LAST '@' whose following text is a clean hostname, so whitespace/tab/Unicode-space inside the password (user:pass word@host, user:pa\tss@host, nbsp) still strips correctly (R3-5, R4-2). - Prose shapes are vetoed structurally: a host + trailing email whose first '@' is after whitespace with no ':' before it, and a dotted host + numeric port + prose email (with a dotted-host discriminator that lets a real digit-prefix password through - R1-2). - Whitespace-less multi-'@' prose strips at the FIRST '@' (R4-3); the catch-branch (parser throws) prose '@' after whitespace is no longer a strip point (R4-1); prose with a path no longer corrupts the host (R3-6). - Windows domain\user:pass@ credentials strip as one userinfo run, and '\' terminates the authority for special schemes (R4-4, R4-5). Drops the new URL() gating entirely - WHATWG misparses prose as userinfo and misses backslash credentials, so the structure scan is authoritative. Adds regression rows for every fixed shape. QwenLM#8136
|
@qwen-code /review |
|
@qwen-code /review --timeout=360 |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31243174425)._ |
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 4 by the review time budget.
Test Plan (not a blocker): 21 tests pass — this review observed 18375 passed.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (CLEAN_HOST_AFTER.test(authority.slice(i + 1))) { | ||
| return authorityStart + i; | ||
| } |
There was a problem hiding this comment.
[Critical] Dotless or punctuation-followed host + credentials + a trailing prose email collapses the whole message to the email's domain. The firstHost early return above only protects dotted hosts followed by whitespace/end; for dotless hosts (localhost, ollama, k8s service names) — or dotted hosts followed by punctuation — control falls into this backward scan, whose last @ is the prose email's, and example.com passes CLEAN_HOST_AFTER, so stripAt deletes the real host and all prose. Regression versus the pre-PR wrapper, which kept host + prose (A/B-probe-verified). Reported by 10 of the 14 review lenses. — Failure scenario: sanitizeProviderWarning('Failed https://user:pass@localhost - contact admin@example.com') → 'Failed https://example.com'; same for 'https://user:pass@ollama:11434 - contact admin@example.com', 'https://user@host:8080 - contact admin@example.com', and 'https://user:pass@api.example.com, contact admin@example.com' — the serve /status payload then names the contact's mail domain as the failing endpoint.
Treat the first @ as the terminator also when it is followed by a single-label host (or a dotted host followed by punctuation) + whitespace/end and a later @ exists across the whitespace, without breaking 'user:p@ss word@host' → 'https://host'; add test rows for the shapes above.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| for ( | ||
| let i = authority.lastIndexOf('@'); | ||
| i !== -1; | ||
| i = authority.lastIndexOf('@', i - 1) | ||
| ) { |
There was a problem hiding this comment.
[Critical] Infinite loop when the authority's first character is @: String.lastIndexOf('@', -1) clamps fromIndex to 0 and returns 0 forever, so i never reaches -1. Synchronous busy loop in the serve status path — the daemon's providers-status request hangs and the CLI freezes. The pre-PR code did not hang on these inputs (A/B-probe-verified: base returned immediately; PR side killed by a watchdog past 100,000 iterations). — Failure scenario: any warning or configured baseUrl 'https://@<text>' whose tail is not a single clean host reaches this loop: 'https://@my_host' (underscore not in CLEAN_HOST_AFTER), 'https://@a b', 'https://@host:80a'.
| for ( | |
| let i = authority.lastIndexOf('@'); | |
| i !== -1; | |
| i = authority.lastIndexOf('@', i - 1) | |
| ) { | |
| for ( | |
| let i = authority.lastIndexOf('@'); | |
| i !== -1; | |
| i = i > 0 ? authority.lastIndexOf('@', i - 1) : -1 | |
| ) { |
Flip-check verified: this terminates all three shapes and preserves normal stripping (user:pass@host → https://host, user:p@ss@host → https://host).
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // keeping host + prose. #8136 R4-3. | ||
| return authorityStart + firstAt; |
There was a problem hiding this comment.
[Critical] @ in the password + a host that fails CLEAN_HOST_AFTER (underscore host, bracketed IPv6 literal, non-numeric port): no @ passes the clean-host test above, so this fallback strips at the FIRST @ — inside the password — leaking the password tail. Regression versus the merge base for the direct sanitizeProviderBaseUrl callers (base stripped at the authority's last @ via new URL()). Probe-verified A/B. — Failure scenario: sanitizeProviderWarning('Failed loading provider https://user:p@ssw0rd-tail@broken_host/v1') → '...https://ssw0rd-tail@broken_host/v1' — the password fragment ships into the provider-status payload and ACP responses; also 'https://admin:Secr3t@x@my_service.internal:8443/api' → 'https://x@my_service.internal:8443/api'. The dotted-host sibling (broken.example) strips correctly and is test-covered.
Broaden the terminator test to realistic hosts, e.g. /^(?:[A-Za-z0-9._-]+|\[[^\]]+\])(:\d+)?$/ (keep rejecting prose characters like , ( ) and whitespace so the R4-3 prose fallback still falls through), so the loop finds the true terminator; add a row for 'https://user:p@ssw0rd-tail@broken_host/v1'.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const atBeforeWs = firstWs === -1 || firstAt < firstWs; | ||
| const colonBeforeAt = authority.slice(0, firstAt).includes(':'); | ||
| if (!atBeforeWs && !colonBeforeAt) { |
There was a problem hiding this comment.
[Critical] This whitespace prose veto is defeated by ANY : before the first @ — prose colons (error:, Error:, timestamps) count as userinfo evidence, so a pathless URL + colon-bearing prose + email is truncated to the email's domain. Pre-PR code preserved these messages. Probe-verified A/B. — Failure scenario: sanitizeProviderWarning('Cannot reach https://ollama.local - error: timeout, contact admin@example.com') → 'Cannot reach https://example.com'; 'Cannot reach https://api.internal - Error: contact admin@example.com' → 'https://example.com' (base: both unchanged). The [::1] bracket-colon shape ('https://[::1]:11434 - contact admin@example.com' → 'https://example.com') also corrupts through this entrance (that variant is pre-existing on base).
Scope the colon check to the whitespace-delimited token ending at the @ — a userinfo colon sits inside that token (user name:pass@host), a prose colon does not (- error: contact admin@…):
const tokenStart = authority.slice(0, firstAt).search(/\S+$/);
const colonBeforeAt = authority.slice(tokenStart, firstAt).includes(':');— qwen3.8-max via Qwen Code /review (v0.21.7)
| const beforeColon = baseUrl.slice(authorityStart, colon); | ||
| if (!/\./.test(beforeColon)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Critical] Issue #8136's bug-1 class (URL with an explicit port + a later @ corrupted) survives for bare-label hosts: this veto only fires when the pre-colon host contains a dot, so localhost/ollama/k8s-service hosts with a port followed by prose containing @ are rewritten to the email's domain — although no credential is present. The issue's Expected bullet 1 requires "A URL with a port and no credentials is left byte-for-byte alone"; the reporter explicitly named bare-label hosts ("ollama, a k8s service name"), and http://localhost:11434 is Ollama's default endpoint — the most common real-world instance of the class this PR claims to fix. Probe-verified. — Failure scenario: sanitizeProviderWarning('Cannot reach http://localhost:11434 - contact admin@example.com') → 'Cannot reach http://example.com'; same for 'http://ollama:8080 - contact admin@example.com' and 'Cannot reach http://ollama:11434 - install @scope/pkg' → 'Cannot reach http://scope/pkg'. The dotted sibling (api.example:8443) is protected and test-covered.
Extend the veto to bare-label hosts with a multi-word-prose discriminator (veto when the text between the first whitespace after the all-digit port candidate and the first @ contains further whitespace), keeping 'user:1234 secret@host' stripping intact — or pin the residual as a recorded tradeoff with a test and maintainer sign-off.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const at = baseUrl.indexOf('@', authorityStart); | ||
| if (at !== -1 && at < colon) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This @-before-colon guard branch has no test; the mutation "delete this guard" survives the full suites (acpModelUtils.test.ts 55/55 + workspace-providers-status.test.ts 23/23), while the row below kills it (mutation-verified). — Concrete cost: for 'https://user@host.example:8443 - contact admin@example.com' the current code correctly strips to 'https://host.example:8443 - contact admin@example.com' (username-only credentials + dotted host + port + prose email); if a future refactor drops or reorders this guard, the same warning silently leaks the username and no test fails.
Add to the it.each table:
['https://user@host.example:8443 - contact admin@example.com', 'https://host.example:8443 - contact admin@example.com'],— qwen3.8-max via Qwen Code /review (v0.21.7)
| const stripPoint = findUserInfoStripPoint( | ||
| baseUrl, | ||
| authorityStart, | ||
| authorityEnd, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This PR replaces sanitizeProviderBaseUrl's entire decision procedure — the new URL() path and port-check fallback the maintainer named as the behavior definition — with a new structural-scan stack, deviating from the maintainer-directed scope, while the maintainer-approved minimal PR #8524 for the same issue sits open. — Concrete cost: maintainer scoping in the #8136 thread: "No new host-recognition rules: the authority-scoped logic in sanitizeProviderBaseUrl (including its new URL() path and the port-check fallback) defines the behavior, and the warning wrapper should simply match it — not extend it … anything beyond that belongs in a separate follow-up"; maintainer on #8524: "exactly the minimal shape we had in mind". Four review rounds of Criticals have all lived in exactly this added heuristic stack, and the rewrite changes behavior for every consumer (getRouteEndpointIdentity, acpAgent.ts, modelConfigUtils.ts) with no follow-up vehicle and no sign-off in this PR's thread.
Reduce this PR to the wrapper-only shape (delegate to sanitizeProviderBaseUrl as it exists on main) and move the shared-function changes to a separate follow-up — or obtain explicit maintainer sign-off in this thread and coordinate with #8524 so one of the two lands.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const colonAfter = baseUrl.indexOf(':', backslash + 1); | ||
| const atAfter = baseUrl.indexOf('@', backslash + 1); | ||
| const windowsCred = | ||
| colonAfter !== -1 && atAfter !== -1 && colonAfter < atAfter; |
There was a problem hiding this comment.
[Critical] The windowsCred heuristic scans for : and @ unbounded past whitespace/prose after the backslash, so a backslash in ordinary credential-free prose disables the authority bound at the backslash and expands the authority into the prose — defeating the diff's own documented intent ('\ terminates the authority for prose'). Probe-verified A/B (the merge base preserved the message); flip-check confirmed the bounded fix. — Failure scenario: sanitizeProviderWarning('Cannot reach https://files.local\share - error: contact admin@example.com') → 'Cannot reach https://example.com': colonAfter matches the prose error: colon and atAfter the prose email's @ (both past whitespace), so windowsCred is true and the authority becomes 'files.local\share - error: contact admin@example.com'; the whitespace prose veto is then defeated by colonBeforeAt and the dotted-host early return fires at the email's @. Without this exception the authority bounds at \ and the message stays unchanged.
Restrict the colonAfter/atAfter search to the whitespace-delimited run adjacent to the backslash (or require the backslash to precede the first whitespace). Flip-check: bounding the scan fixes this input, keeps 'DOMAIN\user:pass@proxy' stripping, and all 55 existing tests pass.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // Password containing '@' AND whitespace (bounded authority): the last '@' | ||
| // within the bounded authority is the terminator. | ||
| ['https://user:p@ss word@host.example/v1', 'https://host.example/v1'], |
There was a problem hiding this comment.
[Suggestion] This row is byte-identical to the row at line 278 of the same table; the comment claims a distinct "bounded authority" scenario the duplicate does not cover. — Concrete cost: the same assertion runs twice under the same generated title, and a future editor amending one row for the intended shape will silently leave the other diverging; any genuinely intended distinct variant has zero coverage.
Delete this duplicate row, or replace it with the actually-intended distinct case.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//)?.[0] ?? ''; | ||
| if (/^(https?|wss?|ftp|file):\/\//.test(scheme)) { |
There was a problem hiding this comment.
[Critical] This special-scheme gate is case-sensitive, but URL schemes are case-insensitive — WHATWG/new URL lowercases the scheme, and this function's own scheme regex (the line above) plus the warning path's URL_START_PATTERN both accept uppercase. So HTTPS:// URLs skip the new backslash authority terminator entirely, a \…@domain tail is swallowed into the authority, and the message collapses onto that tail's domain. Probe-verified A/B: WHATWG parses the input with no userinfo (the merge base preserved the message); HTTPS/Https/FTP/WSS variants all corrupt at HEAD while lowercase twins behave; the i-flag flip-check fixes all probe cases with the full suites green. — Failure scenario: sanitizeProviderWarning('Cannot reach HTTPS://api.example:8443\share@evil.com') → 'Cannot reach HTTPS://evil.com' — the displayed endpoint becomes an unrelated tail domain.
| const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//)?.[0] ?? ''; | |
| if (/^(https?|wss?|ftp|file):\/\//.test(scheme)) { | |
| const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//)?.[0] ?? ''; | |
| if (/^(https?|wss?|ftp|file):\/\//i.test(scheme)) { |
— qwen3.8-max via Qwen Code /review (v0.21.7)
…ensitive scheme, leading-@ loop guard)
Round-5 review found several regressions in the structural rewrite:
- R5-3: CLEAN_HOST_AFTER omitted underscore, so user:p@ss@_host leaked the
password fragment. Add underscore to the host charset.
- R5-14: URL schemes are case-insensitive, but the findAuthorityEnd special-
scheme gate matched case-sensitively, so HTTPS://user:pass@host was not
stripped. Match case-insensitively.
- R5-2: the lastIndexOf('@', i-1) loop could loop forever when i reached 0
(leading '@'); guard with i > firstAt so it stops at the first '@'.
- R5-6/R5-7: a dotted username + digit-prefix password + space is locally
indistinguishable from a dotted host + port + prose email; pin it as a
KNOWN RESIDUAL (same tradeoff class as R1-2), pending maintainer sign-off.
Adds regression rows for each fixed and pinned shape. QwenLM#8136
|
@qwen-code /review |
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.
Test Plan (not a blocker): 21 tests pass — this review observed 18384 passed.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // hostname after it is the terminator. #8136 R3-5. | ||
| const afterFirstAt = authority.slice(firstAt + 1); | ||
| const firstHost = afterFirstAt.match(/^([A-Za-z0-9._-]+)(?::\d+)?(?:\s|$)/); | ||
| if (firstHost !== null && firstHost[1]!.includes('.')) { |
There was a problem hiding this comment.
[Critical] R5-1: Round-5 blocker R5-1 still stands at HEAD (probe-verified). Dotless or punctuation-followed hosts + credentials + a trailing prose email collapse the whole message to the email's domain: the firstHost shortcut requires a DOTTED host followed by whitespace/end; otherwise the backward CLEAN_HOST_AFTER scan accepts the prose email's '@'. — Failure scenario: sanitizeProviderBaseUrl('https://user:pass@localhost - contact admin@example.com') → 'https://example.com' at HEAD; same for 'https://user:pass@ollama:11434 - contact admin@example.com', 'https://user@host:8080 - contact admin@example.com', and 'https://user:pass@api.example.com, contact admin@example.com'. The serve /status payload then names the contact's mail domain as the failing endpoint; the round-5 commit changed none of these outputs. Suggested direction: treat a clean host token (dotless included, optional port) followed by whitespace/end after the first '@' as the terminator, or classify the ambiguity as vetoed KNOWN RESIDUAL instead of corrupting the message; add rows for the shapes above.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // before it (no userinfo). A real credential's '@' either precedes whitespace | ||
| // (`user@host`) or has a ':' before it (`user name:pass@host`). #8136 R3-6. | ||
| const atBeforeWs = firstWs === -1 || firstAt < firstWs; | ||
| const colonBeforeAt = authority.slice(0, firstAt).includes(':'); |
There was a problem hiding this comment.
[Critical] R5-4: Round-5 blocker R5-4 still stands at HEAD (probe-verified). This whitespace prose veto treats ANY ':' before the first '@' as userinfo evidence — prose colons (error:, Error:, timestamps, IPv6 bracket colons) count, so a pathless URL + colon-bearing prose + email is truncated to the email's domain. — Failure scenario: sanitizeProviderBaseUrl('https://ollama.local - error: timeout, contact admin@example.com') → 'https://example.com' at HEAD; same for 'https://api.internal - Error: contact admin@example.com' and 'https://[::1]:11434 - contact admin@example.com'. Pre-PR code preserved all three. Suggested direction: scope the colon check to the whitespace-delimited token ending at the '@' — a userinfo colon sits inside that token (name:pass@), a prose colon does not (error: contact admin@); keep user name:pass@host stripping.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| return false; | ||
| } | ||
| const beforeColon = baseUrl.slice(authorityStart, colon); | ||
| if (!/\./.test(beforeColon)) { |
There was a problem hiding this comment.
[Critical] R5-5: Round-5 blocker R5-5 still stands at HEAD (probe-verified). Issue #8136's bug-1 class survives for bare-label hosts: this veto only fires when the pre-colon host contains a dot, so localhost/ollama/k8s-service hosts with a port followed by prose containing '@' are rewritten to the email's domain — although no credential is present. Expected bullet 1 of #8136 requires these byte-for-byte unchanged, and http://localhost:11434-class endpoints are the most common real-world instance of the class this PR claims to fix. — Failure scenario: sanitizeProviderBaseUrl('http://localhost:11434 - contact admin@example.com') → 'http://example.com' at HEAD; same for 'http://ollama:8080 - contact admin@example.com' and 'http://ollama:11434 - install @scope/pkg' → 'http://scope/pkg'. Suggested direction: extend the veto to bare-label hosts with a multi-word-prose discriminator (veto when the text between the first whitespace after the all-digit port candidate and the first '@' contains further whitespace), keeping 'user:1234 secret@host' stripping — or pin the residual as a recorded tradeoff with a test and maintainer sign-off.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // contains '@' (`user:p@ss word@host`), and the LAST '@' with a clean | ||
| // hostname after it is the terminator. #8136 R3-5. | ||
| const afterFirstAt = authority.slice(firstAt + 1); | ||
| const firstHost = afterFirstAt.match(/^([A-Za-z0-9._-]+)(?::\d+)?(?:\s|$)/); |
There was a problem hiding this comment.
[Critical] R5-6: Round-5 blocker R5-6 still stands at HEAD (probe-verified). This dotted-token early return also matches a dotted fragment INSIDE the password, stripping at the password's own '@' and leaking the rest — violating issue #8136 Expected bullet 2 ('A password containing @ is removed in full'). — Failure scenario: sanitizeProviderBaseUrl('https://user:p@ss.word 123@host.example/v1') → 'https://ss.word 123@host.example/v1' at HEAD; 'https://user:john.doe@gmail.com 42@host.example' → 'https://gmail.com 42@host.example' — the password is served in errors[].error to IDE/desktop clients. Suggested direction: before this early return, scan for a subsequent '@' followed by a clean hostname and prefer it as the terminator — that shape means the dotted token was password prose, not the host.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const windowsCred = | ||
| colonAfter !== -1 && atAfter !== -1 && colonAfter < atAfter; |
There was a problem hiding this comment.
[Critical] R5-12: Round-5 blocker R5-12 still stands at HEAD (probe-verified). The windowsCred heuristic scans for ':' and '@' unbounded past whitespace/prose after the backslash, so a backslash in ordinary credential-free prose disables the authority bound at the backslash and expands the authority into the prose — defeating the diff's own documented intent ('\ terminates the authority for prose'). — Failure scenario: sanitizeProviderBaseUrl('https://files.local\share - error: contact admin@example.com') → 'https://example.com' at HEAD; same for 'https://ollama.local (C:\qwen\models) - error: admin@example.com' → 'https://example.com'. The merge base preserved both messages. Suggested direction: restrict the colonAfter/atAfter search to the whitespace-delimited run adjacent to the backslash — a real DOMAIN\user:pass@ run is contiguous.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const at = baseUrl.indexOf('@', authorityStart); | ||
| if (at !== -1 && at < colon) { |
There was a problem hiding this comment.
[Suggestion] R5-10: Round-5 finding R5-10 still stands: this '@'-before-colon guard branch has no test. No table row reaches isLikelyPortProseMisparse with an '@' before the colon on a dotted host + all-digit port, where the guard flips the outcome. — Concrete cost: for 'https://u@h.example:99999 - x@y.z' the guard is the difference between strip-at-first-'@' ('https://h.example:99999 - x@y.z') and veto; the mutation 'delete this guard' survives both changed suites, so a future refactor deleting it ships silently. Suggested fix: add ['https://u@h.example:99999 - x@y.z', 'https://h.example:99999 - x@y.z'] — killed by deleting the guard.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * throws); fall back to the full-string last '@' when the authority has a ':' | ||
| * and the run between the last '/' and the candidate has no whitespace. | ||
| */ | ||
| function findUserInfoStripPoint( |
There was a problem hiding this comment.
[Suggestion] R5-11: Round-5 finding R5-11 still stands: this PR replaces sanitizeProviderBaseUrl's entire decision procedure — the new URL() path and port-check fallback the maintainer named as the behavior definition ('the authority-scoped logic in sanitizeProviderBaseUrl … defines the behavior, and the warning wrapper should simply match it — not extend it', @doudouOUC, 2026-08-04) — with a structural-scan stack. The maintainer-approved wrapper-only approach lives in the open #8524. — Concrete cost: every residual leak/corruption in this PR (R5-1, R5-3…R5-8, R6-1…R6-3) is a consequence of re-deriving strip points structurally instead of matching the ratified behavior, and the deviation forces re-validation of all ~9 single-URL callers that cannot produce prose whenever the heuristics are tuned again. Suggested direction: reconsider the ratified wrapper-only delegation, or obtain explicit maintainer sign-off for the structural-scan replacement and its residual tradeoffs.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ], | ||
| // #8136 R4-4/R4-5: backslash - a Windows domain\user:pass@ credential strips | ||
| // as a single userinfo run, while '\' terminates the authority for prose. | ||
| ['https://DOMAIN\\user:pass@proxy', 'https://proxy'], |
There was a problem hiding this comment.
[Suggestion] R6-4: The new backslash authority-terminator block in findAuthorityEnd has no effective test — deleting the entire special-scheme backslash block leaves both changed suites green (84/84), yet the mutation is observable. — Concrete cost: probe-verified this round — with the block deleted, sanitizeProviderBaseUrl('https://user:p@ss@host\path') returns 'https://ss@host\path' instead of 'https://host\path' (password fragment left behind); the two R4-4/R4-5 backslash rows below are blind to the mutation because their first-'@' fallback yields identical outputs. A future refactor deleting or breaking the block would pass CI. Suggested fix: add this row after the 'https://user:pass@host\path' row:
['https://user:p@ss@host\\path', 'https://host\\path'],(optionally plus a wss:// scheme variant, since the scheme allowlist is otherwise untested).
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (!/\s/.test(between)) { | ||
| return fullAt; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R6-5: This prose whitespace guard in the no-'@' fallback is untested — replacing it with an unconditional return fullAt; leaves both changed suites green (84/84), while a scratch discriminator flips. — Concrete cost: with the guard removed, sanitizeProviderBaseUrl('https://host:abc/v1 - admin@example.com') flips from unchanged to 'https://example.com'; if a later edit drops the guard, prose emails in such warnings are destroyed with no failing test. Suggested fix: add ['https://host:abc/v1 - admin@example.com', 'https://host:abc/v1 - admin@example.com'] to the table.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // match past the whitespace, and the '@'-in-password shape (last '@' wins). | ||
| // The veto in sanitizeProviderBaseUrl leaves pathless-URL + prose-email | ||
| // shapes unchanged, so a prose email's '@' is never stripped. #8136. | ||
| const sanitized = sanitizeProviderBaseUrl(segment); |
There was a problem hiding this comment.
[Suggestion] R6-6: The repo now carries four parallel URL-userinfo removal implementations with divergent edge-case behaviour: this PR's stripping parser, redactUrlCredentials (core/src/extension/redaction.ts), the URL pattern in redactLogCredentials (acp-bridge/src/logRedaction.ts), and redactProxyCredentials (core/src/utils/runtimeFetchOptions.ts). Nothing links them. — Concrete cost: probe-verified this round — the siblings' [^/\s]+@-style regexes all leave 'connect ECONNREFUSED https://user:pass word@host' unchanged (control 'https://user:password@host' redacted by all three), so a credential this PR strips still leaks through channel-worker log forwarding and extension error surfaces; the next fix will be rediscovered and applied to one implementation only. Suggested minimum: a cross-reference comment at sanitizeProviderBaseUrl naming the sibling redactors; optionally a follow-up issue to consolidate on one authority parser.
— qwen3.8-max via Qwen Code /review (v0.21.8)
…indowsCred bound, case-insensitive) Round-6 review found the structural rewrite still mis-handled several prose shapes and one real credential: - R1-7/R6-1: isLikelyPortProseMisparse now skips IPv6 bracket literals (locates the port colon after ']'), accepts a dotless host label, and accepts a digit port followed by one non-alphanumeric char or an empty candidate (ollama.local: please ...). Stops [::1]:8443 — contact admin@ and api.example:8443; contact admin@ being corrupted. - R5-12: findAuthorityEnd's windowsCred scan is now bounded to the first whitespace/path delimiter after the backslash, so a later prose a:b@c is not mistaken for a Windows credential. - R6-2: the no-'@' fallback's colon search skips IPv6 brackets. - R1-7 (prose veto): an IPv6 authority has no userinfo colon before the first '@', so colonBeforeAt is forced false for '['-prefixed authority. R5-1/R5-12/R6-3/R1-2/R5-7 are KNOWN RESIDUAL: a real terminator in the first '@' with a dotless host after it, followed by prose with an '@host', is locally indistinguishable from a password containing '@' + a real terminator after whitespace. Pinned with the actual (leaking) output and documented as pending maintainer sign-off. QwenLM#8136
|
@qwen-code /review |
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 #8408 (fixes issue #8136 — provider warning sanitizer ...: did not run the actual vitest suites ( node_modules absent in this worktree); verified behavior by executing the extracted verbatim functions under node instea…; PR #8408 (fixes issue #8136 — provider warning sanitizer ...: did not exhaustively probe findAuthorityEnd 's Windows-credential scan against tab/Unicode-whitespace separator combinations beyond the traced cases (one trace…; PR #8408 (fixes issue #8136 — provider warning sanitizer ...: could not run the repo's actual vitest suites (node_modules absent in the review worktree); substituted a verbatim standalone copy validated 34/34 against the P….
[Critical] R7-5: The firstAt===-1 fallback's prose guard tests whitespace only in the run between the LAST '/' and the candidate '@' (between), not in the authority. Two failure directions, one root cause: (a) when the last '/' directly precedes the '@' (npm-scoped paths), between === '/' is always whitespace-free, so the guard vacuously ACCEPTS — pathless-URL + prose + scoped path is stripped down to the path tail; (b) a password containing both '/' and whitespace makes between contain whitespace, so the guard REJECTS — full credential leak, regression versus base which stripped it. — Failure scenario: Probe-verified: (a) 'https://registry.example: check /node_modules/@qwen/pkg' → 'https://qwen/pkg'; 'https://ollama.local: please contact /var/log@qwen' → 'https://qwen' (base parity, but the comment claims the between-run guard protects prose — it does not whenever the last slash directly precedes the '@'); (b) 'https://user:pa/ss word@host' returned unchanged at HEAD while base returned 'https://host' (regression). (Inline comment dropped by line-overlap dedup against round-6 comment 3746486439 — the missing-tests finding R6-5 anchored at the same guard; the behavioural defect itself is not on the PR.)
[Critical] Existing blocker 3723377684 (colonless username containing whitespace leaks in full) still stands at HEAD — probe-verified: sanitizeProviderBaseUrl("https://user @host.example/v1") returns the input unchanged while the merge base stripped it to "https://host.example/v1"; the leak reaches models[].baseUrl, current.baseUrl and ACP responses via the direct callers. The vehicle finding is low-confidence (rare trigger — literal-space usernames in configured baseUrls), so it is not posted inline; see the terminal report's Needs Human Review.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // #8136 R5-12: a backslash-free authority with a later prose `a:b@c` is NOT | ||
| // misread as a Windows credential by findAuthorityEnd (R5-12 fixed the | ||
| // windowsCred scan bound). The remaining leak is the R5-1 residual class. | ||
| ['https://user:pass@host a:b@c', 'https://c'], |
There was a problem hiding this comment.
[Critical] R6-3: Round-6 blocker R6-3 still stands as behaviour: password containing '@' + real host + whitespace + prose email with a clean domain — the backward scan accepts the PROSE email's '@' and the message collapses to that domain; pinned by this test row without maintainer sign-off. — Failure scenario: Probe-verified: pipeline 'Failed https://user:pass@host - contact x@y.z' → 'Failed https://y.z' (base: credential stripped, host + prose preserved); the pinned row 'https://user:pass@host a:b@c' → 'https://c' reproduces at the baseUrl level and passes in the green suite — the corrupted output is specified behaviour with no sign-off in the record.
Suggested fix: Same disposition as R5-1: resolve the ambiguity toward the real terminator, or veto the class and pin the vetoed output with an explicit recorded maintainer decision.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // indistinguishable from a dotless host + port + prose email; the veto | ||
| // fires and the credential leaks. Same tradeoff class as R5-7, pending | ||
| // maintainer sign-off. #8136 R1-2. | ||
| ['https://user:1234 secret@host', 'https://user:1234 secret@host'], |
There was a problem hiding this comment.
[Critical] R1-2: Round-1 blocker R1-2 (re-asserted in rounds 2-6) still stands: a password starting with digits followed by a space is vetoed like a port-prose misparse and leaks IN FULL; the PR pins the leak as intended behaviour ('pending maintainer sign-off') with no sign-off in the record — a credential-exposure regression versus the merge base in the function issue #8136 exists to harden (Expected bullet 2: password 'removed in full'). — Failure scenario: Probe-verified A/B: sanitizeProviderBaseUrl('https://user:1234 secret@host') and ('https://foo.bar:1234 secret@host') return the input unchanged at HEAD while the merge base returned 'https://host'; WHATWG confirms real userinfo (password '1234%20secret'). The leak reaches models[].baseUrl, current.baseUrl, errors[].error, and ACP wire responses.
Suggested fix: Add a discriminator the veto can use (e.g. when the token before the colon is not host-shaped, or when the input is a configured baseUrl rather than free prose, prefer the parsed userinfo over the digit veto), or obtain explicit maintainer sign-off plus a follow-up issue so the pinned leak is a recorded decision rather than an open Critical.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| [ | ||
| 'https://user:pass@ollama - contact admin@example.com', | ||
| 'https://example.com', | ||
| ], |
There was a problem hiding this comment.
[Critical] R5-1: Round-6 blocker R5-1 still stands as behaviour: credential + dotless host + trailing prose email collapses the WHOLE message to the email's domain, and this test row pins the corrupted output as intended 'KNOWN RESIDUAL' behaviour 'pending maintainer sign-off' — no sign-off exists in the issue or PR record (independently verified). The wrapper comment added in workspace-providers-status.ts ('a prose email's @ is never stripped') is contradicted by it. Issue #8136: 'The message the user sees is not just redacted, it is wrong.' — Failure scenario: Probe-verified through the real pipeline: sanitizeProviderWarning('Cannot reach https://user:pass@ollama - contact admin@example.com') → 'Cannot reach https://example.com'; merge base stripped the credential and kept host + prose. Same for the localhost variant; 'https://user:pass@host a:b@c' → 'https://c'. The corrupted output is baked into the passing suite as specified behaviour without the recorded maintainer decision the PR's own comment declares required.
Suggested fix: Treat a clean host token (dotless included, optional port) followed by whitespace/end after the first '@' as the terminator; if the ambiguity is judged inseparable, veto the class (return the input unchanged) and pin the vetoed output — do not ship the corrupted output as specified behaviour without an explicit maintainer decision recorded in the thread or a follow-up issue.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const beforeColon = baseUrl.slice(authorityStart, colon); | ||
| // Host-shape: dotted or a single dotless label (not '[' content, which the | ||
| // IPv6 branch already handled). | ||
| if (!/^[A-Za-z0-9._-]+$/.test(beforeColon)) { |
There was a problem hiding this comment.
[Critical] R7-6: The host-shape test in isLikelyPortProseMisparse (/^[A-Za-z0-9._-]+$/ on beforeColon) is ASCII-only, but WHATWG accepts non-ASCII (IDN) hosts; an IDN host before the port colon defeats the veto and the prose email's '@' becomes the strip point. — Failure scenario: Probe-verified: sanitizeProviderBaseUrl('https://api.exämple:8443 - contact admin@example.com') → 'https://example.com' (WHATWG parses the IDN host fine, no userinfo — Expected bullet 1 requires byte-for-byte unchanged). Credential direction unaffected ('https://user:pass@exämple' still strips) — only the veto breaks. Base parity; a gap in this PR's new veto.
Suggested fix: Admit the WHATWG host grammar: /^[^\s@/?#\\]+$/ (any authority run with no terminators) or Unicode classes /^[\p{L}\p{N}._-]+$/u.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // Digit port, optionally followed by one non-alphanumeric char (`,;.` em-dash | ||
| // etc.). An empty candidate (`ollama.local: please ...`) is also a prose | ||
| // shape where the ':' is the prose separator, not a port. #8136 R1-7/R6-1. | ||
| return portCandidate === '' || /^\d+[^A-Za-z0-9]?$/.test(portCandidate); |
There was a problem hiding this comment.
[Critical] R7-2: The empty-portCandidate prose veto (portCandidate === '') also fires for real credentials whose password STARTS WITH WHITESPACE, so the sanitizer returns the input unchanged — full credential leak, regression versus the pre-PR new-URL() try-branch which stripped these. Not among the documented KNOWN RESIDUAL classes. — Failure scenario: Probe-verified A/B: sanitizeProviderBaseUrl('https://user:\tsecret@host') and ('https://user: pass@host') return the input unchanged at HEAD (WHATWG confirms live userinfo, password '%20pass'); base returned 'https://host'. Warning-level 'Failed https://user: pass@host ok' leaks end-to-end because URL_LIKE_PATTERN stops at the space.
Suggested fix: Narrow the empty-candidate branch to genuine prose — veto only when the run between the colon and the first '@' contains more than one whitespace-separated token ('ollama.local: please contact admin@…' → 3 tokens → vetoed; 'user: x@host' → 1 token → strips). Add regression rows for 'https://user:\tsecret@host' / 'https://user: x@host'.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (baseUrl[authorityStart] === '[') { | ||
| const close = baseUrl.indexOf(']', authorityStart); | ||
| if (close !== -1 && close < authorityEnd) { |
There was a problem hiding this comment.
[Suggestion] R7-16: The R6-2 IPv6 bracket skip in the firstAt===-1 fallback has zero test coverage; the only IPv6 row ('https://[::1]:8443 — contact admin@example.com') takes the firstAt !== -1 path and never reaches this branch. — Failure scenario: Mutation-verified: deleting the skip leaves all 90 tests green while 'https://[::1]/v1@admin' corrupts to 'https://admin' (inner-bracket colon read as the userinfo delimiter, ':1]' fails the digit veto, between '/v1' whitespace-free). No test fails if this branch regresses.
Suggested fix: Add rows ['https://[::1]/v1@admin', 'https://[::1]/v1@admin'] and ['https://[::1]:8443/v1@admin', 'https://[::1]:8443/v1@admin'].
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (/^\d+$/.test(colonCandidate)) { | ||
| return -1; // all-digit port, not userinfo | ||
| } |
There was a problem hiding this comment.
[Suggestion] R7-19: The no-'@' fallback's port test (/^\d+$/) diverges from isLikelyPortProseMisparse's (portCandidate === '' || /^\d+[^A-Za-z0-9]?$/), so punctuation-suffixed ('8443,') and empty port colons are not recognized as ports here and flow into the path-'@' strip — the same branch as R7-5's between-guard hole, at a distinct fix point that a between-guard fix alone would not cover. Base parity, a hole in the new code's own fallback. — Failure scenario: Probe-verified: 'https://registry.example:8443, see /docs/@scope/pkg' → 'https://scope/pkg'; 'https://host:/x@y' → 'https://y'. Clean digit ports (host:99999/path@domain) stay protected — that is precisely the divergence.
Suggested fix: Reuse one port-shape predicate in both places — treat colonCandidate as a non-userinfo port when it is empty or matches /^\d+[^A-Za-z0-9]?$/, matching isLikelyPortProseMisparse.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (/^\d+$/.test(colonCandidate)) { | ||
| return -1; // all-digit port, not userinfo | ||
| } | ||
| const lastSlash = baseUrl.lastIndexOf('/', fullAt); |
There was a problem hiding this comment.
[Suggestion] R6-5: Round-6 finding R6-5 still stands: the prose whitespace guard in the no-'@' fallback is untested — all existing fallback rows (user:p?x@…, user:p#x@…, user:p/x@…) have no whitespace between the last '/' and the candidate '@', and the prose rows with whitespace exit earlier via colon === -1. — Failure scenario: Mutation-verified: for 'https://user:p/x - contact admin@example.com' (unchanged today), deleting the guard corrupts the message to 'https://example.com' and all 90 tests still pass. The guard is the only protection for /?#-containing-password URLs followed by a prose email.
Suggested fix: Add ['https://user:p/x - contact admin@example.com', 'https://user:p/x - contact admin@example.com'] to the table.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // admin@example.com`) has its first '@' AFTER the first whitespace and no | ||
| // ':' before it (no userinfo). A real credential's '@' either precedes whitespace | ||
| // (`user@host`), or has a ':' before it with no whitespace between the colon |
There was a problem hiding this comment.
[Suggestion] R7-14: The added comment states a real credential's colon must have 'no whitespace between the colon and @', then its own parenthetical example (user:pass word@host) has whitespace there and is still treated as userinfo. The actual predicate is whitespace-agnostic (authority.slice(0, firstAt).includes(':')). — Failure scenario: A maintainer editing this actively-hardened veto could 'fix' the code to match the comment (adding a no-whitespace requirement); for user:pass word@host-shaped credentials with atBeforeWs false that would fire the prose veto, return -1, and re-leak a credential HEAD correctly strips (pinned green in the suite).
Suggested fix: Reword to match the code: a real credential's '@' either precedes the first whitespace, or has a ':' anywhere before it in the authority — whitespace between the colon and the '@' does not matter.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (close === -1 || close >= authorityStart + firstAt) { | ||
| colonBeforeAt = false; | ||
| } else { |
There was a problem hiding this comment.
[Suggestion] R7-13: Both arms of the inner IPv6 if/else assign colonBeforeAt = false (which is also the initializer), so the close computation and its condition have no effect; the block is equivalent to baseUrl[authorityStart] !== '[' && authority.slice(0, firstAt).includes(':'). Behaviour is correct as-is — the cost is maintenance in a deliberately hardened security parser. — Failure scenario: The condition reads as load-bearing (computes ']' position relative to first '@'), so a future editor may 'fix' one arm to true, which would let 'https://[::1]:8080 text@x'-shapes past the prose veto and corrupt the message.
| if (close === -1 || close >= authorityStart + firstAt) { | |
| colonBeforeAt = false; | |
| } else { | |
| let colonBeforeAt = false; | |
| if (baseUrl[authorityStart] !== '[') { | |
| colonBeforeAt = authority.slice(0, firstAt).includes(':'); | |
| } |
(keeping the why-comment)
— qwen3.8-max via Qwen Code /review (v0.21.8)
…nicode host) Round-7 review found two more real bugs and several residual classes: - R7-3: the port-prose veto regex accepted at most one trailing punctuation char, so api.example:8443,. contact admin@ was corrupted. Accept one or more non-alphanumeric trailing chars. - R7-10: CLEAN_HOST_AFTER was ASCII-only, so a Unicode (IDN) host like user:pass@例子.测试/v1 was not stripped. Accept Unicode letter/number/mark classes (with the u flag). R7-5 (npm scoped /node_modules/@qwen/pkg stripped by the no-'@' fallback), R7-1 (colonless username with whitespace), and the R5-1/R5-6/R6-3/R1-2 families are KNOWN RESIDUAL — locally indistinguishable from real credentials at the character level, base has the same behavior, pending maintainer sign-off. Pinned with the actual output and documented. QwenLM#8136
|
@qwen-code /review |
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 #8408 (fixes issue #8136 — provider warning sanitizer)...: did not re-adjudicate each round-7 veto over-fire Critical (R7-2/R7-3/R7-6) against the current head — the pinned tests suggest several were fixed, but I did no…; PR #8408 (fixes issue #8136 — provider warning sanitizer)...: did not run the repository vitest suites in-worktree (verification done via verbatim-extracted functions under node instead).; PR #8408 (fixes issue #8136 — provider warning sanitizer)...: could not execute the two changed vitest files — the review worktree has no node_modules and vitest fails to start ( ERR_MODULE_NOT_FOUND ); comment/assertion…; PR #8408 (fixes issue #8136 — provider warning sanitizer)...: could not execute the test suites to confirm they are green — this review worktree has no node_modules installed; all branch-reachability and mutation claims ….
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 'https://user:pass@ollama - contact admin@example.com', | ||
| 'https://example.com', |
There was a problem hiding this comment.
[Critical] R5-1: Round-5 blocker R5-1 (re-asserted rounds 6-7) still stands: credential + dotless host + trailing prose email collapses the WHOLE message to the email's domain; pinned as KNOWN RESIDUAL 'pending maintainer sign-off' with no sign-off in the record — the exact corruption class issue #8136 was filed about ('The message the user sees is not just redacted, it is wrong'). — Failure scenario: probe-verified through the real warning pipeline: sanitizeProviderWarning('Cannot reach https://user:pass@ollama - contact admin@example.com') → 'Cannot reach https://example.com' at HEAD, while the merge base emitted 'Cannot reach https://ollama - contact admin@example.com' (credential stripped, host + prose preserved) — a warning-level regression introduced by the whole-segment delegation. The corrupted output is baked into the passing suite without the recorded maintainer decision the PR's own comment declares required. Suggested fix: resolve the ambiguity toward the real terminator, or veto the class and pin the input unchanged — do not pin the corrupted output — and record an explicit maintainer decision first.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // #8136 R7-1 KNOWN RESIDUAL: a colonless username containing whitespace | ||
| // (`user @host`) is indistinguishable from a prose `host @host` shape; | ||
| // the prose veto fires and it leaks. Pending maintainer sign-off. | ||
| ['https://user @host.example/v1', 'https://user @host.example/v1'], |
There was a problem hiding this comment.
[Critical] R7-1: Round-7 blocker R7-1 still stands: a colonless username containing whitespace leaks in full; pinned as KNOWN RESIDUAL pending sign-off; the merge base stripped it. — Failure scenario: probe-verified: sanitizeProviderBaseUrl('https://user @host.example/v1') returns the input unchanged at HEAD while the merge base returned 'https://host.example/v1'; WHATWG reports username 'user%20' (real userinfo, not prose). The leak reaches models[].baseUrl, current.baseUrl, and ACP responses via the direct callers.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| colonBeforeAt = false; | ||
| } | ||
| } else { | ||
| colonBeforeAt = authority.slice(0, firstAt).includes(':'); |
There was a problem hiding this comment.
[Critical] R5-4: Round-5 blocker R5-4 (re-asserted round 6) still stands: ANY colon before the first '@' — including a prose colon AFTER whitespace — counts as userinfo evidence, defeating the prose veto; isLikelyPortProseMisparse also misses (beforeColon contains the space), and the dotted email domain strips — credential-free prose collapses to the email domain. — Failure scenario: probe-verified: 'https://host - note: admin@example.com' → 'https://example.com' (no credentials anywhere — WHATWG misparse username 'host%20-%20note'); 'https://ollama local:8443 - admin@example.com' → 'https://example.com'. Pipeline A/B: HEAD corrupts both; the merge-base pipeline left them unchanged. The pinned sibling 'ollama.local: please contact admin@example.com' is saved only by the empty-candidate branch. Suggested fix: count a colon as userinfo evidence only when it precedes the first whitespace: const searchEnd = firstWs === -1 ? firstAt : Math.min(firstWs, firstAt); colonBeforeAt = authority.slice(0, searchEnd).includes(':'); (flip-verified against all pinned rows).
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // more chars, e.g. `8443,.` `8443;` em-dash). An empty candidate | ||
| // (`ollama.local: please ...`) is also a prose shape where the ':' is the | ||
| // prose separator, not a port. #8136 R1-7/R6-1/R7-3. | ||
| return portCandidate === '' || /^\d+[^A-Za-z0-9]*$/.test(portCandidate); |
There was a problem hiding this comment.
[Critical] R7-2: Round-7 blocker R7-2 still stands: the empty-portCandidate arm (portCandidate === '') also fires for real credentials whose password STARTS WITH WHITESPACE — the sanitizer returns the input unchanged: full credential leak, regression versus the pre-PR try-branch, and NOT among the documented KNOWN RESIDUAL pins. — Failure scenario: probe-verified A/B: sanitizeProviderBaseUrl('https://user: secret@host.example') returns the input unchanged at HEAD while the merge base returned 'https://host.example'; WHATWG parses password '%20secret'. At warning level the full credential leaks where base emitted 'Cannot reach https://host.example'. The leak reaches models[].baseUrl, current.baseUrl, errors[].error, and ACP responses.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // more chars, e.g. `8443,.` `8443;` em-dash). An empty candidate | ||
| // (`ollama.local: please ...`) is also a prose shape where the ':' is the | ||
| // prose separator, not a port. #8136 R1-7/R6-1/R7-3. | ||
| return portCandidate === '' || /^\d+[^A-Za-z0-9]*$/.test(portCandidate); |
There was a problem hiding this comment.
[Critical] R7-3: Round-7 blocker R7-3 only partially fixed: the round-7 commit widened this veto regex for multiple trailing punctuation chars, but port candidates containing LETTERS (explicitly named in R7-3: 'a trailing letter ("8443x")') still fail the test, the veto never fires, and the prose email's '@' becomes the strip point. — Failure scenario: probe-verified: 'https://api.example:8443abc - contact admin@example.com' → 'https://example.com'; 'https://api.example:abc - contact admin@example.com' → 'https://example.com' (WHATWG username 'api.example' — the prose misparse the veto exists to reject). The pinned multi-punctuation control '8443,.' stays unchanged in the same run — only the letter subshapes remain open.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * parser oracle; the leak is a documented tradeoff pending maintainer sign-off. | ||
| * #8136 R1-2/R5-7. | ||
| */ | ||
| function isLikelyPortProseMisparse( |
There was a problem hiding this comment.
[Suggestion] R5-11: Round-5 finding R5-11 still stands (altitude): this PR replaces sanitizeProviderBaseUrl's entire decision procedure — the new URL() path and port-check fallback the maintainer named as the behaviour definition — with a structural-scan stack; the prose-veto machinery serves exactly one caller (the warning segment path), while the other nine call sites (ACP wire, model-config warnings, route identity) can never reach the whitespace-gated branches yet inherit the regressions. Issue #8136 scoped 'Net deletion, no new helper'; the diff is +484/-54 with three new helpers, and the first rewrite attempt (#8137) was closed unmerged for the same failure class. — Concrete cost: every leak/corruption finding in this review lands on all ten call sites; 'sanitized !== segment' is now a cross-module credential-detection protocol — any future normalization in sanitizeProviderBaseUrl silently alters warning output. Suggested direction (matches the recorded maintainer scope): keep new URL() as the userinfo oracle for whitespace-free inputs; contain prose tolerance in the warning sanitizer.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * as `api.example`); IPv6 bracket literals skip past the `]` to find the port | ||
| * colon. #8136 R1-7/R5-1/R5-5/R6-1. | ||
| * | ||
| * KNOWN RESIDUAL: a dotted/dotless USERNAME + digit-prefix password + space |
There was a problem hiding this comment.
[Suggestion] R8-4: Issue fidelity — the KNOWN RESIDUAL posture (pinning credential-leak and message-corruption classes as specified behaviour 'pending maintainer sign-off') is unsupported by the record. Verified against the live GitHub record: no maintainer sign-off exists for any pinned class; maintainer doudouOUC scoped the fix on issue #8136 (2026-08-04) as 'No new host-recognition rules… anything beyond that belongs in a separate follow-up against the shared function', and endorsed the competing minimal PR #8524 (2026-08-05: 'exactly the minimal shape we had in mind' — still OPEN, unmerged; its fix is whitespace-as-authority-terminator without a veto stack). The issue's Suggested fix (delete sanitizeProviderWarningSegment, hasCredentialPrefix, URL_LIKE_PATTERN) is one-third honoured. — Concrete cost: merging as-is blesses fail-open leaks (rows 285/344/379) and bug-1-class corruption (rows 383/390-395) as intended behaviour, foreclosing issue #8136's Expected bullets without the recorded maintainer decision the PR's own comments declare a precondition — while the endorsed minimal shape remains unmerged. Obtain an explicit recorded decision per pinned class, or rebase onto the endorsed shape.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| function findUnescapedUserInfoFallbackAt( | ||
| /** A clean hostname (with optional numeric port) to the end of the authority. | ||
| * Accepts Unicode host labels (IDN) in addition to ASCII. #8136 R7-10. */ | ||
| const CLEAN_HOST_AFTER = /^[A-Za-z0-9._\p{L}\p{N}\p{M}]+(:\d+)?$/u; |
There was a problem hiding this comment.
[Suggestion] R8-5: The R7-10 Unicode fix is never exercised: CLEAN_HOST_AFTER is read only inside the terminator loop (requires ≥2 '@'), and the sole IDN row ('https://user:pass@例子.测试/v1') has one '@' and strips via the final firstAt fallback. — Concrete cost: mutation-verified — an ASCII-only mutant survives the full suite while 'https://user:p@ss@例子.测试' regresses from 'https://例子.测试' to 'https://ss@例子.测试'. Add ['https://user:p@ss@例子.测试', 'https://例子.测试'] to pin the fix the comment claims.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // Host-shape: dotted or a single dotless label (not '[' content, which the | ||
| // IPv6 branch already handled). |
There was a problem hiding this comment.
[Suggestion] R8-7: IPv6 authorities can never be vetoed: the bracket branch above only relocates the port colon, and beforeColon then starts with '[', which fails this host-shape regex — so '[::1]:8443;admin@example.com' (punctuation-glued prose email, no whitespace) collapses to 'https://example.com'. Base parity (WHATWG itself misparses it), but decidable and undocumented; the comment's claim that the IPv6 branch 'already handled' bracket content is misleading. — Concrete cost: probe-verified corruption at HEAD; the whitespace-bearing sibling (R1-7 pin) is saved only by the first veto, which this input bypasses. Accept a bracket literal in the host-shape check or document the residual.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 'https://host.example/v1 - contact admin@example.com', | ||
| ], | ||
| ['https://user:p@ss word@host.example/v1', 'https://host.example/v1'], | ||
| // R1-2: a digit-prefix + space password is stripped (the dotted-host |
There was a problem hiding this comment.
[Suggestion] R8-8: This comment contradicts its own assertion: it says the password 'is stripped', but the assertion five lines below expects 'https://user:1234 secret@host' UNCHANGED, and the intervening KNOWN RESIDUAL note says 'the credential leaks'. There is also no 'dotted-host discriminator' in the code — the veto accepts dotted OR dotless hosts (leftover from an earlier iteration). — Concrete cost: a maintainer editing this region cannot tell which is authoritative; 'fixing' the code to strip breaks the deliberate R1-2 pin, while 'fixing' the assertion blesses a credential leak. Delete these two stale comment lines; the KNOWN RESIDUAL note immediately below already states the pinned behaviour correctly.
— qwen3.8-max via Qwen Code /review (v0.21.8)
…t comment Round-7/8 review: the IPv6 colonBeforeAt if/else assigned false in both arms (dead code), and the R1-2 test comment claimed the password was 'stripped' while the pinned expectation leaked it. - R7-13: collapse to a single boolean (IPv6 authority has no userinfo colon). - R8-8: drop the contradictory 'stripped' half of the R1-2 comment. No behavior change. QwenLM#8136
|
@wenshao — round-8 review (qwen3.8-max) re-asserts the same residual credential/prose classes every round (R1-2, R5-1, R5-4, R7-1, R7-5, etc.). These are locally indistinguishable from real credentials at the character level; I opened #8913 to track these formally and ask for a decision: accept as pinned residuals, or pursue the parser-oracle direction. A sign-off (or a steer toward the oracle path) would let the bot review stop re-asserting these as open Criticals every round. This round also collapsed the dead IPv6 |
|
@qwen-code /review |
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 #8408 (fixes issue #8136): the serve warning sanitizer...: none — I could not run the packages' own vitest suites (no node_modules in the review worktree), so test-pass evidence is from executing the extracted functio…; PR #8408 (fixes issue #8136): the serve warning sanitizer...: did not run the repo's vitest suites in-worktree ( node_modules absent); all behavior claims rest on the 43/43-validated verbatim harness.**; PR #8408 (fixes issue #8136): the serve warning sanitizer...: did not exhaustively probe non-special schemes beyond ftp:// / wss:// spot checks, nor quote-delimited URL_LIKE truncation beyond traced cases.**; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; PR #8408 (fixes issue #8136): the serve warning sanitizer...: could not run the repo's actual vitest suites ( acpModelUtils.test.ts , workspace-providers-status.test.ts ) because dependencies are not installed in this wor…, and 3 more.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const colonCandidate = | ||
| afterWs === -1 ? afterColon : afterColon.slice(0, afterWs); |
There was a problem hiding this comment.
[Critical] R9-1: The no-'@' fallback is no longer gated on new URL() parse failure (the pre-PR code reached its fallback only inside the catch branch). An authority-terminating EMPTY-port colon passes this guard with colonCandidate === '' (the /^\d+$/ test needs at least one digit), so cleanly-parseable URLs whose path embeds an '@' are stripped there although WHATWG reports EMPTY userinfo. — Failure scenario: probe-verified at HEAD: sanitizeProviderBaseUrl('https://host:/x@y.com') → 'https://y.com' (WHATWG: host host, no username/password, href https://host/x@y.com); 'https://registry.internal:/api@evil.com' → 'https://evil.com'; IPv6 sibling 'https://[::1]:/x@y.com' → 'https://y.com' — the displayed endpoint becomes a domain taken from the URL's path, attacker-shapeable in echoed error text. Pre-PR these returned unchanged (try branch saw empty userinfo). The pinned R7-5 row (input throws under WHATWG) is unaffected. Suggested fix (flip-verified: both probes return to unchanged, all 71 pinned rows stay green): also return -1 when afterColon === '' — e.g. if (afterColon === '' || /^\d+$/.test(colonCandidate)) { return -1; } in the guard just below.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // Windows `domain\user:pass@` credential shape, which is a single userinfo | ||
| // run. URL schemes are case-insensitive, so match case-insensitively. #8136 | ||
| // R4-5/R5-14. | ||
| const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//)?.[0] ?? ''; |
There was a problem hiding this comment.
[Suggestion] R9-5: The scheme regex /^[A-Za-z][A-Za-z\d+.-]*:\/\// is matched twice per call — once in sanitizeProviderBaseUrl (line 165) and again here in findAuthorityEnd — and the special-scheme allowlist (https?|wss?|ftp|file) exists only at this second site. — Concrete cost: findAuthorityEnd has exactly one caller, which already holds the match result (authorityStart = scheme[0].length), so the re-match is redundant; more importantly, editing the caller's scheme shape (e.g. accepting a new scheme) silently disables or enables the backslash-authority rule here with no compile error — a divergence hazard sitting in a security-relevant sanitizer. Suggested fix: pass the already-matched scheme text (or a specialScheme boolean) from sanitizeProviderBaseUrl into findAuthorityEnd, or hoist the regex to a module-level const used by both.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| let scanLimit = end; | ||
| for (const sep of separators) { |
There was a problem hiding this comment.
[Suggestion] R9-6: This separator loop is outcome-inert: '/' '?' '#' can never shrink scanLimit (at this point end is already the minimum of the first '/' '?' '#' at/after authorityStart, and backslash < end, so indexOf(sep, backslash + 1) >= end always fails idx < scanLimit), and the ' ' entry is superseded by the !/\s/.test(...) conjunct in windowsCred below. — Concrete cost: mutation-verified — replacing separators with [] leaves both changed suites green (94/94). A maintainer tightening the scan (e.g. adding '\t' to separators) would reasonably expect this loop to be the bounding mechanism and miss that non-space whitespace is only caught by the separate guard — duplicated terminator knowledge in two adjacent blocks. Suggested fix: drop the loop and rely on the !/\s/ guard (noting in a comment that / ? # are already bounded by end), or collapse to the one effective lookup: const space = baseUrl.indexOf(' ', backslash + 1); const scanLimit = space === -1 ? end : Math.min(end, space);
— qwen3.8-max via Qwen Code /review (v0.21.9)
| [ | ||
| 'https://user@host - contact admin@exam%zz.com', | ||
| 'https://host - contact admin@exam%zz.com', | ||
| ], |
There was a problem hiding this comment.
[Suggestion] R9-7: This R4-1 row passes only because %zz fails CLEAN_HOST_AFTER — with any real email domain the same colonless-username shape corrupts, so the row gives false confidence that the shape is protected. — Concrete cost: probe-verified at HEAD: sanitizeProviderBaseUrl('https://user@host - contact admin@example.com') → 'https://example.com' (host + prose destroyed), while the pinned %zz variant strips to 'https://host - contact admin@exam%zz.com' only because the loop rejects the invalid domain. The row's comment also references a 'catch-branch (new URL throws)' mechanism that no longer exists after this rewrite. Suggested fix: pin the real-domain sibling under the same KNOWN RESIDUAL marker as R5-1/R6-3 (so the pending #8913 sign-off covers it explicitly) or fix the shape, and drop the stale catch-branch reference from the comment.
— qwen3.8-max via Qwen Code /review (v0.21.9)
…ose behavior
Round-9 review (4 findings, down from 20+):
- R9-6: the separator loop in findAuthorityEnd's windowsCred bound is
outcome-inert ('/' '?' '#' already bound above; only whitespace is
not covered by findAuthorityEnd). Replace with a single indexOf(' ').
- R9-7: the R4-1 test row only passed because '%zz' fails CLEAN_HOST_AFTER,
spurious. Pin the real-domain variant (user@host - contact
admin@example.com) and mark the prose-email-host-replacement as the same
KNOWN RESIDUAL class as R5-1, pending maintainer sign-off.
- Add the QwenLM#8136 repro-1 em-dash verbatim row (with-path) the maintainer
requested; the ASCII-dash variant was already pinned.
R9-1 (no-'@' fallback not gated on new URL parse failure) is the already-
pinned R7-5 residual (npm-scoped path @ stripped); R9-5 (scheme regex
matched twice) left as a micro-opt nit. QwenLM#8136
|
@qwen-code /review |
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.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (firstHost !== null && firstHost[1]!.includes('.')) { | ||
| return authorityStart + firstAt; | ||
| } |
There was a problem hiding this comment.
[Critical] R5-6: Round-5 blocker R5-6 (re-asserted rounds 6-9, incl. comment 3740534341) still stands: the firstHost dotted-token shortcut trusts the FIRST @ as terminator whenever the text after it starts with a dotted label + whitespace/end; a password containing @ whose post-@ fragment is dotted satisfies the match, so the strip lands inside the password and leaks the remainder — violating issue #8136 Expected bullet 2 ('A password containing @ is removed in full'). — Failure scenario: sanitizeProviderBaseUrl('https://user:p@ss.word @host') returns 'https://ss.word @host' — password fragment ss.word exposed in models[].baseUrl, errors[].error, and ACP responses; the merge base returned 'https://host'.
Witness (A/B probe, HEAD 68d4871 vs merge base 7b7ff19):
HEAD 'https://user:p@ss.word @host' -> 'https://ss.word @host'
base 'https://user:p@ss.word @host' -> 'https://host'
WHATWG user="user" pass="p%40ss.word%20" host="host"
Suggested fix: guard the shortcut against a further @ candidate in the authority — when one exists, fall through to the last-@ loop (the R5-6 comment on this PR proposes the same direction) — and add a killer row for the dotted-fragment shape.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const windowsCred = | ||
| colonAfter !== -1 && | ||
| atAfter !== -1 && | ||
| colonAfter < atAfter && |
There was a problem hiding this comment.
[Critical] R7-7: Round-7 blocker R7-7 (re-asserted round 8) still stands: the windowsCred scan's bound does not terminate at the first backslash itself, so a SECOND backslash between the first \ and the @ still classifies as a Windows credential even though WHATWG terminates the authority at the FIRST backslash (empty userinfo) — corrupting a credential-free URL. This corruption is introduced by this diff. — Failure scenario: sanitizeProviderBaseUrl('https://host\\share\\user:pass@proxy') collapses to 'https://proxy' although no credential is present; the merge base returned the input unchanged.
Witness (A/B probe):
HEAD 'https://host\\share\\user:pass@proxy' -> 'https://proxy'
base 'https://host\\share\\user:pass@proxy' -> unchanged
WHATWG user="" pass="" host="host" (backslash = path separator on special schemes)
Suggested fix: include \ in the scan bound's separator set (stop the windowsCred scan at the next backslash), so a second backslash before the @ ends the candidate userinfo run the way WHATWG ends the authority.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const sanitized = sanitizeProviderBaseUrl(segment); | ||
| if (sanitized !== segment) { | ||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
[Critical] R7-8: Round-7 blocker R7-8 still stands: a Windows-domain credential whose password contains whitespace is never stripped through the serve warning path — the whole-segment delegation returns the segment unchanged (the R7-9 mechanism defeats the windowsCred bound) and the URL_LIKE_PATTERN fallback cannot match past the whitespace — whereas the DELETED pre-PR sanitizeProviderWarningSegment primary path stripped it. The added comment's claim that delegation 'handles space-containing-credential URLs' overclaims for this shape. — Failure scenario: a provider construction error 'Cannot reach https://DOMAIN\\user:pass word@proxy' ships the full credential into the serve /status payload (errors[].error).
Witness (serve-path A/B, verifier drove createWorkspaceProvidersStatusProvider end-to-end on both trees):
HEAD errors[0].error = 'Cannot reach https://DOMAIN\\user:pass word@proxy' (password in payload: true)
base errors[0].error = 'Cannot reach https://proxy' (password in payload: false)
Suggested fix: fix the R7-9 authority truncation at its root (findAuthorityEnd/the no-@ fallback), which restores stripping for this shape through the delegation.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const colonBeforeAt = | ||
| baseUrl[authorityStart] !== '[' && | ||
| authority.slice(0, firstAt).includes(':'); |
There was a problem hiding this comment.
[Critical] R5-4: Round-5 blocker R5-4 (re-asserted rounds 6-9) still stands: any : before the first @ — including a prose colon AFTER whitespace (note:, error:, timestamps) — counts as userinfo evidence (colonBeforeAt), defeating the pathless-prose veto; the scan then strips at the prose email's @ and collapses the message to the email's domain. This directly contradicts the wrapper comment this PR adds ('a prose email's @ is never stripped'). — Failure scenario: sanitizeProviderBaseUrl('https://api.example - note: contact admin@example.com') → 'https://example.com' — the real host is replaced by the prose email's domain and the prose is eaten (base-parity corruption: the claim is that the #8136 class the new veto was added to close survives for colon-bearing prose, unpinned).
Witness (probe + verifier flip check):
HEAD 'https://api.example - note: contact admin@example.com' -> 'https://example.com'
with the suggested fix applied -> unchanged; all 72 pinned rows still green
| const colonBeforeAt = | |
| baseUrl[authorityStart] !== '[' && | |
| authority.slice(0, firstAt).includes(':'); | |
| const colonBound = firstWs === -1 ? firstAt : Math.min(firstAt, firstWs); | |
| const colonBeforeAt = | |
| baseUrl[authorityStart] !== '[' && | |
| authority.slice(0, colonBound).includes(':'); |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // run. URL schemes are case-insensitive, so match case-insensitively. #8136 | ||
| // R4-5/R5-14. | ||
| const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//)?.[0] ?? ''; | ||
| if (/^(https?|wss?|ftp|file):\/\//i.test(scheme)) { |
There was a problem hiding this comment.
[Suggestion] The R5-14 case-insensitive (/i) special-scheme test that gates the new backslash authority-bound rule has no killer test: no added row combines an uppercase scheme with a backslash, so the flag can be silently deleted. — Concrete cost: mutant = drop only the i flag — all 95 tests in the two changed suites stay green (executed); discriminator 'HTTPS://a:b\\c x@d' — HEAD leaves it unchanged, the mutant returns 'HTTPS://d' (whole authority collapsed to the prose fragment), re-opening the #8136 corruption class for uppercase-scheme URLs. The R5-14 uppercase rows contain no backslash and the backslash rows are lowercase, so neither kills the flag.
Suggested fix: add a row pairing an uppercase scheme with a backslash shape, e.g. ['HTTPS://DOMAIN\\user:pass@proxy', 'HTTPS://proxy'] or ['HTTPS://a:b\\c x@d', 'HTTPS://a:b\\c x@d'].
— qwen3.8-max via Qwen Code /review (v0.21.12)
3 participants
What this PR does
Fixes two bugs in
sanitizeProviderWarningthat truncated messages and leaked passwords::8443) followed by a later@(e.g. an email) caused the sanitizer to cut everything between them.@inside a password was matched as the userinfo end, leaking the remainder.Why it's needed
The old
sanitizeProviderWarningSegmentused naiveindexOf(':')andindexOf('@')over the full segment. The correct logic already exists insanitizeProviderBaseUrl(acpModelUtils.ts), which uses authority-scopedlastIndexOf('@')and port detection.What changed
sanitizeProviderWarningSegmentandhasCredentialPrefixsanitizeSegmentCredentialsthat first tries the full segment throughsanitizeProviderBaseUrl(handles spaces in userinfo), then falls back to per-URL-fragment sanitization@in password (no leak)Reviewer Test Plan
Tested on
Risk & Scope
sanitizeProviderBaseUrlfunction that already handles these cases correctly.Linked Issues
Fixes #8136