fix(cli): scope warning credential stripping to the URL authority - #8137
fix(cli): scope warning credential stripping to the URL authority#8137LHMQ878 wants to merge 14 commits into
Conversation
`sanitizeProviderWarningSegment` located the userinfo with `indexOf(':')` and
`indexOf('@')` across the whole span, with no idea where the authority ended.
Two consequences:
- A port was read as the start of a password, so a later `@` anywhere in the
message was taken as the end of the userinfo and everything between them was
deleted. `Cannot reach https://api.example:8443/v1 — contact admin@example.com`
became `Cannot reach https://example.com`.
- `indexOf('@')` found the one inside a password, cutting too early and emitting
the rest of it: `https://user:p@ssw0rd-tail@host/v1` kept `ssw0rd-tail`.
`sanitizeProviderBaseUrl` already gets both right — it bounds the search with
`findAuthorityEnd` and takes `lastIndexOf('@')` inside the authority, with a port
check for inputs `new URL()` rejects. Delegate to it and delete the bespoke
heuristics.
The span-splitting stays: it is what lets a password containing a space or a
quote be found at all, since no `[^\s'"]`-style URL pattern can match one.
Closes QwenLM#8136
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR — and for the thorough issue write-up in #8136. Template looks good ✓ Problem: observed bug with a clear reproduction. The issue includes before/after output for both failure modes (port-induced truncation and Direction: aligned. Credential sanitization in the Size: not applicable — no core paths touched. 43 production lines (10 added, 33 deleted), 57 test lines. Net deletion of production logic. Approach: the scope feels right. Deleting the bespoke Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！也感谢 #8136 中详尽的问题描述。 模板完整 ✓ 问题：已观测到的 bug，有明确的复现。issue 中包含了两种故障模式（端口导致截断、密码中含 方向：对齐。 规模：不适用——未触及核心路径。生产代码 43 行（新增 10 行，删除 33 行），测试 57 行。生产逻辑净删除。 方案：范围合理。删除自定义的 风险：无升级风险信号——未匹配高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal first: given a bug where The PR does exactly this. No simpler path exists. Traced the logic through all three new test cases:
No correctness issues, no security concerns, no convention violations. The diff is a net deletion of production logic with no new abstractions. Testing
The main CI suite ( The author reports 22 pass in Sandboxed verification would settle this: 中文说明代码审查独立方案：既然 bug 在于 PR 正是这样做的。不存在更简路径。 手动追踪了三个新测试用例的逻辑，均正确。无正确性问题、无安全隐患、无规范违反。diff 是生产逻辑的净删除，无新抽象。 测试主 CI 套件（ 作者报告 沙盒验证可以确认： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal fix that reuses existing well-tested logic; only gap is that CI hasn't executed yet (fork workflow approval pending). This is what a good bugfix looks like: the problem is real and reproduced, the fix deletes more code than it adds, and the correct logic already existed in the codebase — the PR just wires it up. I traced the authority-bounded The only reservation is procedural: the main CI suite hasn't run because fork workflow runs need a maintainer's approval first. The code review is clean enough that I'm approving now, but a maintainer should still approve the CI run and confirm green before merging. 中文说明置信度：4/5 ——干净、最小的修复，复用了已有的经过充分测试的逻辑；唯一的缺口是 CI 尚未执行（fork 工作流等待批准）。 这是一个好的 bugfix 应有的样子：问题真实且有复现，修复删除的代码比新增的多，正确的逻辑已经存在于代码库中——PR 只是将其接入。我手动追踪了 authority 范围限定的 唯一的保留是程序性的：主 CI 套件尚未运行，因为 fork 工作流运行需要先获得维护者批准。代码审查足够干净，我现在批准，但维护者仍应批准 CI 运行并在合并前确认全绿。 — 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.
LGTM, looks ready to ship. ✅
| const segmentEnd = findUrlSegmentEnd(warning, next.index, next.marker); | ||
| const segment = warning.slice(next.index, segmentEnd); | ||
| result += sanitizeProviderWarningSegment(segment, next.marker.length); | ||
| result += sanitizeProviderBaseUrl(warning.slice(next.index, segmentEnd)); |
There was a problem hiding this comment.
[Critical] Passing the full segment (URL + trailing non-URL text) to sanitizeProviderBaseUrl over-strips when the URL has no path component and the trailing text contains @. findAuthorityEnd extends the "authority" to end-of-string when no /, ?, or # exists after the scheme, so lastIndexOf('@') lands on an @ in trailing prose. new URL() succeeds (WHATWG percent-encodes spaces), parsed.username is truthy, and the code strips at that @. This is a regression: the removed hasCredentialPrefix rejected candidates whose "username" span contained spaces, preserving such messages.
Failure scenario: Cannot reach https://api.example.com — contact admin@example.com → Cannot reach https://example.com — the host mutates from api.example.com to example.com, and the email address and surrounding text are deleted. Confirmed by probe: sanitizeProviderBaseUrl('https://api.example.com — contact admin@example.com') returns 'https://example.com'. All three new tests use URLs with paths (/v1, /v2), so this pathless case is untested.
| result += sanitizeProviderBaseUrl(warning.slice(next.index, segmentEnd)); | |
| const rawSegment = warning.slice(next.index, segmentEnd); | |
| const urlOnly = rawSegment.match(/^[^\s'"`<>]+/)?.[0] ?? rawSegment; | |
| result += sanitizeProviderBaseUrl(urlOnly) + rawSegment.slice(urlOnly.length); |
Alternatively, add a whitespace guard in findAuthorityEnd so the authority stops at the first space — but that would change behavior for the existing (correct) space-in-password handling, so a segment-level fix is safer.
— qwen3.8-max-preview via Qwen Code /review
Handing the whole span to sanitizeProviderBaseUrl over-stripped when the URL had no path and the trailing text contained an `@`. A pathless URL has no `/`, `?` or `#` to bound its authority, so findAuthorityEnd ran to end-of-span and the `@` in an email address was read as the end of a userinfo: Cannot reach https://api.example.com — contact admin@example.com -> Cannot reach https://example.com The host mutated and the rest of the message was deleted. All three tests added with the delegation used URLs with paths, so the case was untested. The URL now ends at the first space, except that a recognised `user:password@` prefix is consumed first — a password may legally contain spaces, and finding those is why the message is split into spans at all. Bounding the URL at the first space unconditionally would have leaked every spaced password, including the one the existing 'sanitizes credentials from provider construction errors' test covers. The prefix pattern rejects `host:8443 ` via a negative lookahead, so a port is still not read as the start of a password.
|
Fixed in Your diagnosis of the mechanism is right: with no I did not take the suggested diff, because it leaks passwords. Bounding the span at The first of those is the pre-existing What I did instead — same idea (bound the URL before handing it over), but the bound consumes a recognised credential prefix first: const CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*@/;
function findUrlEnd(segment: string, markerLength: number): number {
const credentials = CREDENTIAL_PREFIX_PATTERN.exec(segment.slice(markerLength));
const from = credentials ? markerLength + credentials[0].length : markerLength;
const space = segment.slice(from).search(/\s/);
return space === -1 ? segment.length : from + space;
}So the URL ends at the first space after any I also considered your alternative of adding a whitespace guard inside Verification. 56 tests pass across Beyond the suite I ran 19 probe cases split into 9 must-strip and 10 must-preserve, including |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| * a password — that is the only way a colon in the authority can be followed by | ||
| * whitespace and still not be credentials. | ||
| */ | ||
| const CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*@/; |
There was a problem hiding this comment.
[Suggestion] The greedy [^/?#]*@ matches to the LAST @ in the span. When a pathless URL carries credentials AND is followed by prose containing an @ (e.g. an email address), the credential prefix consumes the prose, findUrlEnd returns the whole segment, and sanitizeProviderBaseUrl strips at that last @ — deleting the prose and rewriting the host. — Failure scenario: Cannot reach https://user:pass@host.com — contact admin@example.com → Cannot reach https://example.com (host mutates host.com → example.com, the prose — contact admin@ is deleted); the pre-diff first-@ code produced the correct Cannot reach https://host.com — contact admin@example.com. Credentials are still stripped (no leak), but this is a message-mangling regression of the exact class this PR sets out to fix, and the added pathless tests cover only the no-credential case. Confirmed by probe.
| const CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*@/; | |
| const CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*?@/; |
— qwen3.8-max-preview via Qwen Code /review
| * a password — that is the only way a colon in the authority can be followed by | ||
| * whitespace and still not be credentials. | ||
| */ | ||
| const CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*@/; |
There was a problem hiding this comment.
[Suggestion] The negative lookahead (?!\d+(?:\s|$)) rejects any colon + digits + whitespace as a port, including a legitimate credential whose password starts with digits followed by a space — so the password is never stripped and leaks into the status payload. — Failure scenario: a provider base URL https://user:123 secret@host.com/v1 in a ModelsConfig error message → the pattern fails to match (123 + space trips the port heuristic), findUrlEnd cuts the URL at the space after user:123, sanitizeProviderBaseUrl parses https://user:123 as host user port 123 with no userinfo, and the prose secret@host.com/v1 is appended verbatim — the password 123 secret is shown in the UI (confirmed by probe). The pre-diff code (hasCredentialPrefix + first-@) stripped this to https://host.com/v1, so this is a credential-leak regression; the comment's premise that a port is "the only way" a colon in the authority can be followed by whitespace without being credentials is not correct. The trigger is a rare password shape (hence Suggestion, not Critical), but please consider tightening the lookahead so it rejects digits only when no @ follows before the next //?/#, and/or correct the comment and add a test documenting the gap.
— qwen3.8-max-preview via Qwen Code /review
…diness Both Suggestions on the previous head are real; a probe reproduced each before anything changed, and turned up a third case of the same shape. The `@` that ends the userinfo was being chosen by the tail's greediness, and neither setting is correct. A greedy tail runs past the host into an `@` in trailing prose, so `https://user:pass@host.example — contact admin@example.com` became `https://example.com`: credentials stripped, but the prose deleted and the host rewritten from it. Making the tail lazy fixes that case and breaks the mirror image — it stops at an `@` inside the password, so `https://user:p@s s@broken.example/v1` leaves `s s@` behind in the message. The delimiter is therefore chosen structurally instead: the `@` must be followed by something a provider base URL can address — a dotted name, a bracketed IPv6 literal, or `localhost`, with an optional port. A bare single label is excluded so the `s` in `p@s s@host.example` cannot pass for a host. The port guard was also too eager. `user:123 secret@host` is locally identical to `host:8443 ` — colon, digits, space — so rejecting the latter rejected the former and the password `123 secret` reached the status payload verbatim. The digits now count as a port only at end of span, or when another space follows before the `@`, which is prose rather than a one-space password. The comment claiming a port is "the only way a colon in the authority can be followed by whitespace and still not be credentials" was wrong and is gone. Both halves are load-bearing: dropping the port guard and keeping only the host lookahead re-breaks the four pathless-URL-plus-port cases, including the one the previous round's Critical was about. 46 probe cases pass, covering every combination of {path, no path}, {port, no port}, {credentials, none}, {password containing a space, an `@`, both, leading digits} and {trailing prose with an email, without}. 60 tests pass across workspace-providers-status.test.ts and acpModelUtils.test.ts. Two of the four added tests fail without the source change; the other two pass either way and are there to pin that the new delimiter rule did not narrow what already worked — `p@s s@` was handled correctly by the greedy tail, and the localhost case checks the port heuristic still holds for a host that is not a dotted name. Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
|
Both Suggestions are real. Fixed in I did not take the suggested
The So the delimiter is now chosen structurally — the const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`;A bare single label is deliberately excluded, or the On Suggestion 2, your correction of my comment is right and I've removed the claim. I wrote that a port is "the only way a colon in the authority can be followed by whitespace and still not be credentials". It isn't — (?!\d+(?:$|\s(?:[^@\s]*\s)))That distinguishes prose ( Both halves are load-bearing — I checked rather than assuming. Dropping the port guard and keeping only the host lookahead re-breaks four pathless-URL-plus-port cases, including the one last round's Critical was about: Verification. 46 probe cases across every combination of {path, no path} × {port, no port} × {credentials, none} × {password with a space, an Two of the four added tests are regression tests; two are not, and I'd rather say so than imply otherwise. Stashing only the source file and re-running: The two that fail are the two Suggestions. The two that pass either way pin that the new delimiter rule did not narrow what already worked — |
| * literal, or `localhost`, with an optional port. A bare single label is not | ||
| * included, so the `s` in `p@s s@host.example` cannot pass for one. | ||
| */ | ||
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; |
There was a problem hiding this comment.
[Critical] HOST_AFTER_USERINFO excludes bare single-label hosts (other than localhost), so CREDENTIAL_PREFIX_PATTERN never recognises a credential prefix before a single-label host. This causes two distinct failures:
(A) Credential leak — when the password contains a space, findUrlEnd falls back to cutting at the first space after the scheme marker (inside the password), and sanitizeProviderBaseUrl receives a truncated https://user:pass fragment with no @, which it returns unchanged. The full credentials appear in the output.
(B) Message corruption — when a pathless credentialed URL with a single-label host is followed by prose containing an email, the lazy [^/?#]*? crosses the space into the prose and selects the email's @, so sanitizeProviderBaseUrl strips there — rewriting the host and deleting the prose.
— Failure scenario A: Failed https://user:my password@internalhost/v1 → credentials fully visible in the status payload. The removed indexOf('@') code stripped this correctly.
— Failure scenario B: Cannot reach https://user:pass@intranet — contact admin@example.com → Cannot reach https://example.com — the oncall sees a wrong server name and the contact line is deleted.
Single-label hosts are common for intranet proxies, ollama, redis, and k8s service names. Confirmed by probe.
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; | |
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d][A-Za-z\d-]*(?:\.[A-Za-z\d-]+)*)(?::\d+)?(?:[/?#\s]|$)`; |
This adds [A-Za-z\d][A-Za-z\d-]*(?:\.[A-Za-z\d-]+)* — a label starting with an alphanumeric, optionally followed by dotted labels — which matches both single-label hosts (intranet, ollama) and dotted names, while the two-character minimum ([A-Za-z\d][A-Za-z\d-]*) still rejects the one-character s in p@s s@host.example.
— qwen3.8-max-preview via Qwen Code /review
| * must be followed by something addressable as a host. | ||
| */ | ||
| const CREDENTIAL_PREFIX_PATTERN = new RegExp( | ||
| String.raw`^[^\s/?#'"\`<>]+:(?!\d+(?:$|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, |
There was a problem hiding this comment.
[Critical] The port-detection negative lookahead (?!\d+(?:$|\s(?:[^@\s]*\s))) only rejects digits followed by end-of-string or space-word-space. Digits followed by punctuation (comma, semicolon, paren) pass through and are treated as the start of a password, so the regex consumes prose up to the next @ and rewrites the host from an email address.
— Failure scenario: Cannot reach https://api.example:8443, contact admin@example.com → \d+ matches 8443 but the next char is , (not $ or \s), so the lookahead succeeds and credential detection proceeds. [^/?#]*?@ expands through 8443, contact admin to the email's @, and HOST_AFTER_USERINFO matches example.com. Output: Cannot reach https://example.com — the real host api.example:8443 and the contact prose are both lost. Confirmed by probe.
The existing tests use :8443/v1 (slash stops the match) and :8443 — (space triggers the \s branch), so neither covers punctuation immediately after port digits.
| String.raw`^[^\s/?#'"\`<>]+:(?!\d+(?:$|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, | |
| String.raw`^[^\s/?#'"\`<>]+:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, |
Adding [^\s\d] to the lookahead rejects digits immediately followed by any non-space, non-digit character (comma, period, semicolon, paren, etc.), which covers sentence punctuation while still accepting user:123 secret@host (space after digits, not punctuation).
— qwen3.8-max-preview via Qwen Code /review
| * literal, or `localhost`, with an optional port. A bare single label is not | ||
| * included, so the `s` in `p@s s@host.example` cannot pass for one. | ||
| */ | ||
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; |
There was a problem hiding this comment.
[Critical] HOST_AFTER_USERINFO's trailing delimiter (?:[/?#\s]|$) rejects common sentence punctuation (., ,, ;, !, )) after an otherwise-valid dotted host, so the credential-prefix lookahead fails when a URL ends a sentence — leaking spaced passwords and corrupting messages even for fully valid dotted hosts. This is a distinct defect from the single-label host exclusion (different part of the same regex; fixing one does not fix the other).
— Failure scenario A (credential leak): Failed https://user:my pass@host.example. Retry later → the trailing . after host.example is not in [/?#\s] and not $, so the lookahead fails at the only @. findUrlEnd falls back to the first space, cutting at https://user:my; credentials fully visible. Confirmed by probe.
— Failure scenario B (message corruption): Cannot reach https://user:pass@host.example. Contact admin@example.com → the lazy [^/?#]*? skips the first @ (trailing . fails the lookahead) and lands on the email's @; output is Cannot reach https://example.com. Confirmed by probe.
A trailing period is one of the most common characters in prose error messages, yet the existing tests only use space-delimited hosts (host.example — retry).
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; | |
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s.,;:!?\])}]|$)`; |
Widening the trailing delimiter class to include common sentence/clause punctuation lets the lookahead succeed when a URL ends a sentence. The punctuation is not consumed (it's inside a lookahead), so it does not affect the host match itself.
— qwen3.8-max-preview via Qwen Code /review
…contains The userinfo delimiter is chosen by what follows the `@` -- it must be something addressable as a host -- so whatever that host pattern fails to recognise, the sanitizer fails to sanitize. Two omissions in it, plus one in the port guard, each producing both a credential leak and a corrupted message: A host was required to contain a dot, so a bare label was not one. Those are exactly the hosts an intranet has: a proxy name, a container name like `ollama`, a k8s service. `https://user:my password@internalhost/v1` left the credentials fully visible, because with the `@` unrecognised the URL was cut at the first space -- inside the password. Pathless, the lazy tail instead ran into the prose and stripped at an email's `@`, turning `https://user:pass@intranet - contact admin@example.com` into `https://intranet` with the contact line deleted. What may follow a host was enumerated as `[/?#\s]|$`, which does not include a period, so a URL ending a sentence was not recognised either -- the most ordinary shape an error message has. Replaced with a lookahead for anything a host cannot continue into, so the list cannot be incomplete again; the trailing dot is left to the sentence. A port was only recognised at end of span or before a space, so the comma in `:8443, contact` made `8443` read as the start of a password and the tail found the email's `@`. Digits followed by a non-space are now a port. Five tests added, one per confirmed failure. Reverting the source file fails exactly those five and leaves the other 29 passing.
|
All three were real, and all five scenarios reproduce. Fixed in I ran every one of them against the actual code before changing anything, since a claim about a regex is cheap to make and cheap to check. Temporarily exporting Two full credential leaks, three messages where the host is replaced by a domain the user never configured and the following prose is deleted. After the fix, with the same probe: and all nine regression cases ( The common cause, which is why I didn't take the three suggestions verbatimThese are three symptoms of one thing. The So rather than lengthening the list to For the single-label host I used
The port lookahead I took as suggested ( TestsFive added, one per confirmed failure, each commented with what it caught rather than what it asserts. One note on the review comments themselves, since it may be worth knowing: C1 and C3 were filed as separate |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| * the list being incomplete was not a cosmetic problem. A trailing dot is | ||
| * allowed to belong to the sentence rather than the name. | ||
| */ | ||
| const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d](?:[A-Za-z\d-]*\.)+[A-Za-z\d-]+|[A-Za-z\d][A-Za-z\d-]+)(?::\d+)?(?![A-Za-z\d-])`; |
There was a problem hiding this comment.
[Suggestion] The structured credential detection is deliberately more conservative than the removed indexOf heuristic — which is the right call: bounding the search to the authority is exactly what fixes the host-rewrite and over-strip bugs this PR targets. The tradeoff is that a few pathological shapes now leak that the old code stripped. All are narrow and live in the same-principal display path (credentials from the user's own config, surfaced in their own /status payload), so none is blocking — but they're worth pinning with tests and a one-line doc note so a future tweak doesn't silently move the boundary:
- A genuine single-character host with a spaced password —
Failed https://user:my pass@h/v1— is no longer stripped: the bare-label alternative needs at least two chars, so@h/isn't recognised. The removedhasCredentialPrefixstripped this tohttps://h/v1(narrow regression). - An empty-username URL with a spaced password —
Failed https://:my pass@host.example/v1— isn't recognised either, because the username class on line 291 requires at least one char before:. Also a narrow regression;+to*on that class would close it. - A 2+char password fragment can still read as a bare host —
https://user:p@ss word@realhost.example/v1leaksword(pre-existing; the old code leaked it too). - The documented port tradeoff (the comment above this constant) has the same root; its two-space variant
https://user:123 secret word@hostis the untested flip side of theuser:123 secret@hostcase already pinned in the tests.
One caution on the fix space: raising the bare-label minimum to 3+ chars (a natural response to the third bullet) widens the first bullet's leak to 2-char hosts — the length lever trades one leak for another. If short hosts must be supported, a structural signal (e.g. requiring a / or delimiter after the candidate host) is the robust route.
— qwen3.8-max-preview via Qwen Code /review
| expect(result.initialized).toBe(false); | ||
| }); | ||
|
|
||
| it('keeps a port and the text after it when the message has no credentials', async () => { |
There was a problem hiding this comment.
[Suggestion] The bracketed-IPv6 branch of HOST_AFTER_USERINFO (\[[0-9A-Fa-f:.]+\], workspace-providers-status.ts:269) isn't exercised by any of the new tests. It works today — Failed loading provider https://user:pass@[::1]:8443/v1 correctly strips to https://[::1]:8443/v1 — so this is a coverage gap rather than a live bug, but a future edit to that branch would ship undetected. A case alongside these would pin it, e.g.:
it('strips credentials from a bracketed IPv6 base URL', async () => {
coreMock.throwModelsConfigError = true;
coreMock.modelsConfigErrorMessage =
'Failed loading provider https://user:pass@[::1]:8443/v1';
const provider = createWorkspaceProvidersStatusProvider({ env: {} });
await writeUserSettings({
security: { auth: { selectedType: 'openai' } },
modelProviders: { openai: [{ id: 'model-a', name: 'Model A' }] },
});
const result = await provider(workspace, true);
expect(result.errors?.[0]?.error).toBe(
'Failed loading provider https://[::1]:8443/v1',
);
expect(JSON.stringify(result)).not.toContain('pass');
});— qwen3.8-max-preview via Qwen Code /review
The bare-label alternative in HOST_AFTER_USERINFO required two characters, so `https://user:my pass@h/v1` was not recognised as credentialed and the fallback cut the URL at the first space -- inside the password. Raising or lowering that floor only moves the leak: at two characters a one-character host leaks, at three a two-character host does. The floor is only needed where a label has nothing after it, which is the case the constant was written for. A label followed by `/`, `?` or `#` is not a word that happens to look like a host; it is an authority with a path. So the two cases are now separate constants -- UNDELIMITED_HOST keeps the length rule, DELIMITED_HOST replaces it with the delimiter -- and `@h/v1` is recognised without also recognising the `s` in `p@s s@host.example`, whose only boundary is a space. Also allows an empty username. `https://:token@host` is a legal URL and the shape a token-only config produces, but the userinfo class required a character before the colon, so that password was left in the status payload too. Two shapes stay unstripped and are now pinned by test rather than left to be rediscovered: `p@ss word@host.example`, where the password's tail is itself a valid host, and `123 secret word@host`, where the second space before the `@` is the same evidence that distinguishes a port from a password. Neither is decidable from the text, so a test records the behaviour and the comment says why, which makes a later change to either rule a deliberate one. Adds a case for the bracketed IPv6 branch, which no test covered.
|
Both suggestions taken, in The two narrow leaks are fixed, without the length leverI probed all four shapes named in the review before touching anything:
That's the whole shape of it. Any floor N leaks hosts of N-1 characters, so I took the structural route: the two-character minimum exists only because a bare label at the end of a span has nothing to distinguish it from a word. A label followed by So the constant splits in two. The empty-username case was a one-character change ( The other two are recorded rather than fixed
So both now have a test that asserts the current output and a name that says it is not a fix — IPv6 coverageAdded, essentially as written. It is worth having for a reason beyond coverage: the bracketed literal is the one host shape whose own characters include a colon, so a future change to the port or delimiter rules could break it while every other case here stays green. Verification39 tests in The three characterization/coverage tests passing either way is intended — they pin behaviour that already held. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| * the list being incomplete was not a cosmetic problem. A trailing dot is | ||
| * allowed to belong to the sentence rather than the name. | ||
| */ | ||
| const UNDELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d](?:[A-Za-z\d-]*\.)+[A-Za-z\d-]+|[A-Za-z\d][A-Za-z\d-]+)(?::\d+)?(?![A-Za-z\d-])`; |
There was a problem hiding this comment.
[Suggestion] A single-character host with a port but no path (@h:8443) falls through both UNDELIMITED_HOST (bare-label branch requires ≥ 2 chars) and DELIMITED_HOST (requires /, ?, or #), so CREDENTIAL_PREFIX_PATTERN fails to recognise the real @ and extends to a later @ in trailing prose, corrupting the message. — Failure scenario: Cannot reach https://user:pass@h:8443 — contact admin@example.com → Cannot reach https://example.com (wrong host, contact line deleted; credentials stripped, no leak). The old code handled this case via indexOf('@'). The two-char host hh:8443 works correctly; the gap is specifically single-char + port + no path + prose containing @.
Consider adding a port-qualified single-character alternative (e.g. |[A-Za-z\d](?::\d+) before the closing ) of the host alternation), or pinning this case with a test to document the boundary deliberately.
— qwen3.8-max-preview via Qwen Code /review
| const CREDENTIAL_PREFIX_PATTERN = new RegExp( | ||
| String.raw`^[^\s/?#'"\`<>]*:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The port heuristic's third alternative (\s(?:[^@\s]*\s)) requires two spaces before the @, so a port followed by a single word ending in @ (e.g. :8443 support@example.com) is misread as a password, rewriting the host and deleting the prose. — Failure scenario: Cannot reach https://api.example:8443 support@example.com → Cannot reach https://example.com. The old deleted code produced the same output (not a regression), but this shape is neither documented in the comment nor pinned by a test, unlike the two analogous tradeoff cases (p@ss word@host.example and 123 secret word@host).
Consider adding a pinning test alongside the existing two, and a sentence to the CREDENTIAL_PREFIX_PATTERN comment naming this shape.
— qwen3.8-max-preview via Qwen Code /review
| * so that widening either rule has to choose deliberately. | ||
| */ | ||
| const CREDENTIAL_PREFIX_PATTERN = new RegExp( | ||
| String.raw`^[^\s/?#'"\`<>]*:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, |
There was a problem hiding this comment.
[Suggestion] The port-detection lookahead's [^\s\d] alternative treats a digit run followed by any non-space/non-digit (including a letter) as a port, but a real port is pure digits — so a spaced password beginning with \d+[A-Za-z] is misread as a port and its credentials are never stripped. — Failure scenario: Failed https://user:123abc secret@host.example/v1 → \d+ matches 123, [^\s\d] matches a, the negative lookahead fires, CREDENTIAL_PREFIX_PATTERN returns null, findUrlEnd cuts at the first space, and sanitizeProviderBaseUrl receives https://user:123abc (no @) — username and first password word leaked. The old code stripped at the first @ and produced https://host.example/v1, so this is a regression.
Consider restricting the punctuation alternative so it cannot match a letter continuation — e.g. [^\s\da-zA-Z] — or at minimum pin this shape with a test so the leak is a deliberate, documented tradeoff.
— qwen3.8-max-preview via Qwen Code /review
| * the list being incomplete was not a cosmetic problem. A trailing dot is | ||
| * allowed to belong to the sentence rather than the name. | ||
| */ | ||
| const UNDELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d](?:[A-Za-z\d-]*\.)+[A-Za-z\d-]+|[A-Za-z\d][A-Za-z\d-]+)(?::\d+)?(?![A-Za-z\d-])`; |
There was a problem hiding this comment.
[Suggestion] HOST_AFTER_USERINFO uses ASCII-only character classes ([A-Za-z\d]), so a non-ASCII (IDN) host after @ is never recognised; when the password also contains a space, findUrlEnd fails to consume the credential prefix, bounds the URL at the space inside the password, and sanitizeProviderBaseUrl receives a truncated URL with no @ — leaking the username and first password word. — Failure scenario: Failed https://user:my pass@münchen.de/v1 → CREDENTIAL_PREFIX_PATTERN returns null (ü fails the ASCII host lookahead), output leaks user:my pass. The base code used indexOf('@') with a single-char check and handled IDN hosts whose first character is ASCII, producing https://münchen.de/v1. This is a regression.
Consider widening the host label classes to include Unicode letters/digits (e.g. [\p{L}\p{N}] with the u flag), or adding a fallback in findUrlEnd for authorities containing a : before the first space and a later @.
— qwen3.8-max-preview via Qwen Code /review
Three shapes reached the sanitizer and were not recognised as authorities, so the credential prefix was not found in them. Two leaked the credentials they exist to strip; one rewrote the host from an email address in the trailing prose. A one-character host carrying a port fell through every alternative: the bare label branch requires two characters and the delimited branch requires a path. `@h:8443` was therefore not a host, the `@` was taken from the contact line after it, and `https://user:pass@h:8443 - contact admin@example.com` became `https://example.com`. A port is the same structural evidence of an authority that a path is, so it now qualifies a single character the same way. The port heuristic accepted any non-digit as the character following a port, so a password beginning `123abc` read as port `123` and was left in the message whole. A port is digits and nothing else; what closes one is now an explicit set rather than a negated class, because a negated class also admits `_`, `=` and `%`, all legal password characters -- `123%abc secret` leaked through the same gap. An internationalized host arrives here un-punycoded when it comes from a config file someone typed, and the ASCII-only classes did not match it. With no recognised prefix the URL was cut at the space inside the password and `user:my pass` was displayed. `:8443 support@example.com` is left as it is and pinned by test instead. A port followed by exactly one word ending in `@` is character-for-character a one-space password; the deleted code produced the same output, so this is a boundary to document rather than a regression to fix.
|
Five findings from the review on A one-character host carrying a port was not a host. The real host and the contact line are both gone. A port is the same structural evidence of an authority that a path is, so The port heuristic read a letter as the end of a port. A non-ASCII host was not recognised. An internationalized host reaches this code un-punycoded when it comes from a config someone typed, and
The earlier findings on Verification: Four of the five new tests fail without the source change; the fifth is the pinning test, which passes either way by design -- it records a boundary rather than a fix. The IPv6 coverage gap from the review on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| coreMock.throwModelsConfigError = true; | ||
| coreMock.modelsConfigErrorMessage = | ||
| 'Cannot reach https://api.example:8443/v1 — contact admin@example.com'; |
There was a problem hiding this comment.
[Suggestion] 25 new tests repeat an identical 7-line setup (set mock flag, set mock message, create provider, write settings, call provider); only the message string and the assertions differ. — Concrete cost: a change to the test setup — a new required field in the settings shape, a change to createWorkspaceProvidersStatusProvider's signature, or a different mock mechanism — requires 25 identical edits instead of one.
| coreMock.throwModelsConfigError = true; | |
| coreMock.modelsConfigErrorMessage = | |
| 'Cannot reach https://api.example:8443/v1 — contact admin@example.com'; | |
| async function sanitizedErrorMessage(message: string) { | |
| coreMock.throwModelsConfigError = true; | |
| coreMock.modelsConfigErrorMessage = message; | |
| const provider = createWorkspaceProvidersStatusProvider({ env: {} }); | |
| await writeUserSettings({ | |
| security: { auth: { selectedType: 'openai' } }, | |
| modelProviders: { openai: [{ id: 'model-a', name: 'Model A' }] }, | |
| }); | |
| return provider(workspace, true); | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| * legal in a password: `:123%abc secret@host` was read as a port and its | ||
| * password left in the message. | ||
| */ | ||
| const PORT_END = String.raw`[,.;:!?)\]}]`; |
There was a problem hiding this comment.
[Suggestion] PORT_END omits quote characters (', ", backtick) and opening paren (, so a port followed by a closing quote or parenthetical is not recognised as a port, and the credential pattern treats the digits as the start of a password. — Failure scenario: Cannot reach "https://api.example:8443" — contact admin@example.com → the " after 8443 matches none of $, PORT_END, or \s(…), so the port is not detected; the pattern treats 8443" — contact admin as a password, matches the @ before example.com, and the output becomes Cannot reach "https://example.com — the real host and contact prose are deleted. Similarly Cannot connect https://api.internal:8443(ECONNREFUSED) — contact admin@example.com → Cannot connect https://example.com. Confirmed by probe.
| const PORT_END = String.raw`[,.;:!?)\]}]`; | |
| const PORT_END = String.raw`[,.;:!?)\]}'"(]`; |
— qwen3.8-max-preview via Qwen Code /review
A closing delimiter sits exactly where the port heuristic expects a sentence character or a space, so `"https://api.example:8443"` read `8443"` as the start of a password, matched the `@` in the prose beyond, and deleted both the real host and the text after it: Cannot reach "https://api.example:8443" - contact admin@example.com -> Cannot reach "https://example.com Adding these characters to PORT_END would fix that case and open a leak, which is the same trade the `%` case already rejected: `"` is as legal in a password as `%` is, so `:123"abc secret@host` would then read as a port and keep its password. Verified by probe before choosing. Only a *balanced* delimiter is evidence, and that evidence is available where the span is cut rather than in the pattern: the character before the URL is already in hand there. So `findUrlSegmentEnd` bounds the span at the matching closer when an opener precedes the URL, and PORT_END is untouched. The same character stays inert inside a password, where nothing opened it. An unbalanced closer is still ambiguous - `:8443(ECONNREFUSED)` has no opening paren, so it cannot be told from a password beginning `123(`. Documented as a fourth tradeoff alongside the existing three. Tests: 6 delimiter pairs pinned, 3 password characters pinned as non-delimiters, and one case proving a credential inside a quoted URL is still stripped. 4 of the 11 fail without the change; the `()` and `[]` pairs pass either way because `)` and `]` are already in PORT_END, so those two guard existing behaviour.
|
Thanks — one of the four is a real bug, and it took a different fix than the suggested one. The live one: a delimiter after a port destroys the hostConfirmed exactly as described, and it is worse than the two reported cases — every quote and bracket does it: Why I did not take the suggested
|
| const closer = URL_QUOTE_PAIRS.get(before.slice(-1)); | ||
| if (closer !== undefined) { | ||
| const closed = value.indexOf(closer, afterMarker); | ||
| if (closed !== -1) lineEnd = Math.min(lineEnd, closed); |
There was a problem hiding this comment.
[Critical] The balanced-delimiter bounding cuts the URL span at the first occurrence of the closing quote — including one that appears inside the password. When a URL is wrapped in a quote pair and the password contains that same character, the span is truncated before the @, CREDENTIAL_PREFIX_PATTERN sees no credential prefix, and sanitizeProviderBaseUrl receives a userinfo-less fragment it returns unchanged — so the password is emitted into the /status payload in full.
— Failure scenario: Cannot reach "https://user:pa"ss@host.example/v1" — retry. The " before https sets the closer to "; value.indexOf('"', afterMarker) then lands on the " inside pa"ss, so the segment becomes https://user:pa. No @ survives in it, nothing is stripped, and the output is the input unchanged — password pa"ss fully visible (probe-confirmed). This is a regression: the pre-PR code scanned to end-of-line and stripped this to https://host.example/v1. The existing 'still strips a credential from inside a quoted URL' test uses a spaced password but not a quote character in the password, so this path is untested.
One possible fix — bound the closer search by the credential prefix, so a closer inside the password is skipped:
const closer = URL_QUOTE_PAIRS.get(before.slice(-1));
if (closer !== undefined) {
const creds = CREDENTIAL_PREFIX_PATTERN.exec(value.slice(start + marker.length));
const closerFrom = creds ? start + marker.length + creds[0].length : afterMarker;
const closed = value.indexOf(closer, closerFrom);
if (closed !== -1) lineEnd = Math.min(lineEnd, closed);
}— qwen3.8-max-preview via Qwen Code /review
The balanced-delimiter rule cut the span at the first matching closer, but that character is legal in a password. In Cannot reach "https://user:pa"ss@host.example/v1" — retry the first `"` sits inside the credential, so the span ended at `https://user:pa`, no `@` remained in it, nothing was recognised as a userinfo, and the password stayed in the message in full — a regression against the pre-fix behaviour, which scanned to end of line and stripped this correctly. All six delimiter pairs leaked. Bounding the closer search by the credential pattern fixes the leaks but undoes the bug the closer exists for: it asks the port heuristic to validate the span, and that heuristic's failure on `:8443"` is the whole reason a closer is consulted. Measured, it turns `"https://api.example:8443" — contact admin@example.com` back into `"https://example.com`, destroying the host. So a closer now has to carry two signals: it must be the last one within the span bound, and a port must sit immediately before it with no `@` in between. Either test alone still leaks — without the port, `:8443"abc secret@host` cuts inside the password; without lastIndexOf, a closer in the password ends the span whenever no later one exists on the line. Together they hold, and the port test is the principled half: the closer only carries information about where the URL ended when a port is what precedes it. Computing the line/next-URL bound before the closer search also stops a closer on a later line from ending this span.
|
Confirmed, and fixed in The finding, verifiedI probed all six delimiter pairs on the reviewed head
All six leaked in full. The mechanism is exactly yours: the span ends at Why I did not take the suggested patchBounding the closer search by The host is destroyed and the prose is eaten -- which is the case the closer rule was added for. It's circular: it asks the port heuristic to decide where the span ends, and that heuristic's failure on What the fix does insteadA closer now has to carry two signals -- it must be the last one within the already-computed span bound, and a port must sit immediately before it with no const nextUrl = findNextUrlStart(value, afterMarker);
const bound = Math.min(lineEnd, nextUrl?.index ?? value.length);
const closer = URL_QUOTE_PAIRS.get(before.slice(-1));
if (closer !== undefined) {
const closed = value.lastIndexOf(closer, bound - 1);
if (closed >= afterMarker) {
const upTo = value.slice(afterMarker, closed);
if (PORT_AT_END.test(upTo) && !upTo.includes('@')) return closed;
}
}
return bound;Both halves are load-bearing; I measured each alone leaking a case the other catches:
The port test is also the principled half rather than a patch: the reason a balanced delimiter matters at all is that the port heuristic cannot see past it, so a closer only carries information about where the URL ended when a port is what precedes it. Moving the line/next-URL TestsYou were right that the existing
Revert control, on the reviewed head with the new tests applied: 8 failed (6 pairs + 2), while the original 6-pair One case knowingly unchanged
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| result += sanitizeProviderWarningSegment(segment, next.marker.length); | ||
| const urlEnd = findUrlEnd(segment, next.marker.length); | ||
| result += | ||
| sanitizeProviderBaseUrl(segment.slice(0, urlEnd)) + segment.slice(urlEnd); |
There was a problem hiding this comment.
[Suggestion] The free-text sanitizer (CREDENTIAL_PREFIX_PATTERN / HOST_AFTER_USERINFO in this file) and sanitizeProviderBaseUrl in utils/acpModelUtils.ts encode related assumptions about URL authority structure — what counts as a host and where the userinfo ends — but share no code, no shared constant, and no cross-reference; acpModelUtils.ts has no pointer back to this file. — Concrete cost: a future change to sanitizeProviderBaseUrl's authority-bounding or @-selection (e.g. in findAuthorityEnd / findUnescapedUserInfoFallbackAt) could silently violate the assumptions findUrlEnd relies on and reproduce exactly the credential-leak / host-rewrite bugs this PR fixes, with only acpModelUtils.test.ts (a different directory) to catch it. Not a present-day bug — the two agree on every realistic input today — but the invariant is load-bearing and undocumented. Add a one-line cross-reference at both sites naming it, e.g. at sanitizeProviderBaseUrl: // The free-text sanitizer in serve/workspace-providers-status.ts (CREDENTIAL_PREFIX_PATTERN / HOST_AFTER_USERINFO) re-implements a parallel host grammar whose cut is fed into this function; keep the two notions of "host" consistent or a credential leaks / a host is rewritten., plus a matching back-pointer in the CREDENTIAL_PREFIX_PATTERN doc comment.
— qwen3.8-max-preview via Qwen Code /review
…s on `sanitizeProviderWarning` decides how much of a message is URL, then hands the slice to `sanitizeProviderBaseUrl` in utils/acpModelUtils.ts, which locates the userinfo within it. The two encode the same notion of where an authority ends, share no code, and neither referenced the other. That invariant turned out to be both load-bearing and untested. Narrowing `findAuthorityEnd` to stop only at `/` -- dropping the `?` and `#` cases, the kind of simplification made when assuming a URL has a path -- leaves both existing suites fully green, 64 and 31 tests, while changing the output of 18 of 54 pathless-URL shapes. The credentials are still stripped in every one of them; what breaks is the host. `https://user:secret@host.example:8443?k=v@evil.com` reports `evil.com`, because the later `@` becomes the userinfo terminator. That is the host-rewrite bug this PR already fixes for prose, reachable again through a query or fragment. Since only an assertion on the surviving host can see it -- a `not.toContain(password)` check passes either way -- this adds two cases, covering `?` and `#`. Under the narrowed `findAuthorityEnd` they are the only two failures in the file, and acpModelUtils.test.ts still passes all 31, which is what shows the coupling is invisible from its own side. Both files now name the other, with the concrete failure recorded rather than a bare pointer, so a future change to either has the reason in front of it.
|
Accepted, and taken further than a comment — the invariant turned out to be testable, so it's pinned rather than just described. The coupling is real, and it's worse than "only
|
| String.raw`^[^\s/?#'"\x60<>]*:(?!\d+(?:$|${PORT_END}|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, | ||
| 'u', |
There was a problem hiding this comment.
[Critical] Credential leak: a spaced password whose first word is digits immediately followed by a PORT_END punctuation char (. , ; ! ? ) ] }) is misread as a port, so CREDENTIAL_PREFIX_PATTERN never matches and the full credential is emitted into the /status payload.
The negative lookahead (?!\d+(?:$|${PORT_END}|…)) rejects the colon in user:123. secret@host because \d+ matches 123 and PORT_END matches .. With no credential match, findUrlEnd falls back to cutting at the first space — inside the password — handing https://user:123. (no @) to sanitizeProviderBaseUrl, which returns it unchanged; the remainder is appended as prose.
— Failure scenario: Failed https://user:123. secret@host.example/v1 → Failed https://user:123. secret@host.example/v1 (full username + password + host visible). Probe-confirmed; the same leak occurs for the 123, 123) 123! 123; and 1. variants. This is a regression — base stripped it to https://host.example/v1. The pinned control user:123 secret@… (no punctuation) still strips correctly; appending a single PORT_END char to the digit word is what opens the leak. Note the symmetry with the PORT_END comment: the closing set was chosen to fix the :123%abc secret@host leak, but including . / , / ; / etc. creates this one.
Fix direction: read leading digits as a port only when that reading actually terminates the authority — before rejecting on PORT_END, confirm there is no later @(?=HOST_AFTER_USERINFO) in the span. A naive "look for a later @" flip breaks the pinned :8443, contact admin@example.com test, so the port reading needs to require the digits-plus-punctuation to sit at the URL boundary (space / EOL / closer), not mid-password. Pin with user:123. secret@host.example/v1 → https://host.example/v1, password absent.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Confirmed and fixed in 7d6336a (test) on top of 9ae36ac (fix). Reproduced through createWorkspaceProvidersStatusProvider rather than against the regex alone, since the leak is in the interaction between the pattern and findUrlEnd's fallback:
in Failed https://user:123. secret@host.example/v1
before Failed https://user:123. secret@host.example/v1 <- credential kept verbatim
after Failed https://host.example/v1
Your reading of the mechanism is exactly right, including the part that matters most: the failure is not "the pattern matches too little", it is that not matching routes the span to the first-space cut, which lands inside the password and leaves a slice with no @ for sanitizeProviderBaseUrl to strip.
Two things I checked before choosing the fix.
The obvious fix is wrong. Dropping PORT_END from the lookahead does fix this case, and it also breaks two others — the new port-guard test and the pre-existing :8443, contact one both start rewriting the host to example.com. I kept it as a mutation:
| mutation | result |
|---|---|
restore the bare ${PORT_END} arm |
leak test fails (1) |
delete the ${PORT_END} arm outright |
port-guard test + :8443, contact fail (2) |
So there are two tests, not one. The second pins the direction the ambiguity is resolved in, which is the part a later change could flip silently.
What separates the two readings turned out to be already in the pattern: a port that closes a sentence is not followed by a userinfo @, while a password's remaining words are. That is the same test the neighbouring \s(?:[^@\s]*\s) arm makes, so the PORT_END arm now makes it too — scoped to the rest of the span rather than the token, because the deciding @ is in the word after the space.
CREDENTIAL_FALLBACK_PATTERN cannot cover this one, which is worth stating since it was added for the sibling finding in the same batch. Its host is a single character by construction (FALLBACK_HOST) and the host here is host.example, so it never matches. I verified that rather than assuming it — with the fallback in place and the lookahead reverted, the leak still reproduces.
Since the change nests a lookahead inside a negative lookahead, I also scaled it rather than assuming it was cheap: linear on digit runs, punctuation runs and long space-separated tails up to n=2000, worst point 1ms → 3ms over 200 iterations. Worth measuring because a different candidate I tried while working this — a forward scan for a later @, in the host rule rather than here — turned out quadratic at 388ms/n=3200 against 6ms for an anchored equivalent, and nothing about reading it suggested that.
There was a problem hiding this comment.
Correction to my reply above, on the one point that changes what a reviewer should conclude: I described this as a defect in the pattern without confirming your "this is a regression" note. You were right, and I have now measured it — base strips Failed https://user:123. secret@host.example/v1 to https://host.example/v1 correctly, and the leak arrives with ce14a6d, one of my own earlier commits on this branch.
Full end-to-end walk of the branch, for this case and the two siblings in the same batch:
| revision | 123. secret@host.example |
my pass@h |
pass@h — contact a@b.com |
|---|---|---|---|
base 6a432ad |
stripped | stripped | host kept |
ce14a6d (mine) |
leak | leak | rewritten |
f43af06, 8ab9eef |
leak | leak | rewritten |
9ae36ac |
stripped | stripped | rewritten |
7d6336a + 68fe634 |
stripped | stripped | host kept |
Nothing about the fix or the mutation results changes. What changes is the framing: this is not a gap in a new grammar, it is one regression with three spellings that my own earlier commits introduced, and the branch was net-negative against base on these inputs for several commits. The PORT_END symmetry you pointed out is the mechanism — the closing set was widened to fix the :123%abc leak and that widening opened this one, which is the same trade the other two findings sit on.
I have written the fuller version of this on the sibling comment (#discussion_r3689070573), including whether the accumulated grammar should be restructured rather than widened a sixth time.
| String.raw`|${HOST_CHAR}${LABEL_CHAR}+` + | ||
| String.raw`|${HOST_CHAR}:\d+)` + | ||
| String.raw`(?::\d+)?(?!${LABEL_CHAR})`; |
There was a problem hiding this comment.
[Critical] Credential leak (and host rewrite) for a bare single-character host. HOST_AFTER_USERINFO admits a single char only with a port (HOST_CHAR:\d+) or with a path (DELIMITED_HOST); a bare one-char host (@h) with neither matches no alternative, so CREDENTIAL_PREFIX_PATTERN cannot match at that @.
Two consequences, depending on what follows:
- Leak — spaced password, no later
@:Failed https://user:my pass@h retry later→ the pattern fails outright,findUrlEndcuts at the first space (inside the password:https://user:my), and the full credentialuser:my passsurvives verbatim. Base stripped this tohttps://h retry later. (The unspaceduser:pass@hcase still strips, because the whole token has no internal space andsanitizeProviderBaseUrl/new URLhandles it — the leak needs a spaced password.) - Host rewrite — a prose
@after:Cannot reach https://user:pass@h — contact admin@example.com→ the lazy match falls back to the email's@and rewrites the host tohttps://example.com(base kepthttps://h …, prose intact).
Both probe-confirmed; controls (@intranet, @h/v1, @h:8443) strip correctly, isolating the bare single-char gap. This is the narrower single-char case behind the already-fixed single-label blocker — multi-char bare labels (intranet, internalhost) work, one-char ones do not.
Fix direction: admit a one-char label as a host only when it is the target of a user:password@ prefix and no later @ resolves to a properly-delimited host (prefer the better-evidenced later @). Lowering the two-character floor outright breaks the pinned p@s s@host.example tradeoff (the s after p@ is a single char), so the distinguisher must be the presence of a later viable @, not length alone.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Both halves confirmed, and they needed two different fixes — which I only found because I re-measured your second bullet against the first fix instead of assuming one commit covered the comment.
Leak half — fixed in 9ae36ac:
in Failed https://user:my pass@h retry later
before Failed https://user:my pass@h retry later <- credential kept verbatim
after Failed https://h retry later
Host-rewrite half — still broken after that commit; fixed in 68fe634:
in Cannot reach https://user:pass@h — contact admin@example.com
after 9ae36ac Cannot reach https://example.com <- host rewritten, prose deleted
after 68fe634 Cannot reach https://h — contact admin@example.com
The reason the first fix does not reach it is worth recording, because it looks like it should. CREDENTIAL_FALLBACK_PATTERN was consulted only when the primary pattern declined. In the leak case it declines, so the fallback runs. In the rewrite case it does not decline — it matches at the email's @, because a one-character host is invisible to HOST_AFTER_USERINFO and the lazy tail simply runs past it. Succeeding at the wrong @ and failing outright look the same from the outside but take different branches:
body = user:pass@h — contact admin@example.com
primary : "user:pass@h — contact admin@" <- matches, so the fallback is skipped
fallback : "user:pass@" <- had the right answer all along
So the fallback is now consulted either way, and when both match, which to believe is decided by the whitespace between the two @s.
Your fix direction was right about the discriminator but I could not use length or @-presence for it. I tried three:
| rule | result |
|---|---|
| prefer the shorter match | fixes this, breaks pinned p@s s@host.example |
prefer the fallback when a space lies between the two @s |
fixes this, breaks pinned p@s s@host.example |
| prefer the fallback when two or more spaces lie between | all 27 cases pass |
Two spaces is the line because that is already this file's notion of prose — it is the same test PORT_DIGITS's \s(?:[^@\s]*\s) arm makes to tell :8443 — contact from a one-space password. So the pinned tradeoff survives for the reason it was pinned (one space is genuinely ambiguous, and the later host is better evidenced) rather than by special-casing.
Three mutations, each killed by a different set, which is what convinced me the guard is load-bearing rather than decorative:
| mutation | failures |
|---|---|
| consult the fallback only when the primary declines | 1 |
| always prefer the fallback | 2 |
| treat one space as enough | 2 |
The last two both "fix" the reported case and both break p@s s@host.example, so without the guard this would have been a trade rather than a fix.
Your controls reproduced as stated — @intranet, @h/v1 and @h:8443 all strip correctly, before and after — which is what isolated this to the bare one-char case. Also confirming your parenthetical: the unspaced user:pass@h does strip on base, so the leak does need a spaced password.
One thing I should add, because it is not visible from any single comment and it is not flattering to this branch. I measured every revision end-to-end against all three cases in this batch, and all three are regressions my own earlier commits on this branch introduced — base handles all three correctly:
| revision | 123. secret@host.example |
my pass@h |
pass@h — contact a@b.com |
|---|---|---|---|
base 6a432ad |
stripped | stripped | host kept |
ce14a6d (mine) |
leak | leak | rewritten |
f43af06, 8ab9eef |
leak | leak | rewritten |
9ae36ac |
stripped | stripped | rewritten |
68fe634 |
stripped | stripped | host kept |
So this is one regression with three spellings, introduced by a commit whose message claims to fix host recognition, and it took four further commits to get back to parity with base on these inputs. The cause is structural rather than careless: each of those commits widened the host grammar to recognise a shape that was being missed, and every widening moves the boundary between "host" and "password fragment" — which is exactly where these three inputs sit.
The reason I am stating it here rather than leaving it in per-comment replies: the branch is a sequence of grammar widenings, and if the review reads each finding as an isolated miss, the pattern that produced them is invisible. If you would rather see this restructured — the accumulated grammar replaced by something closer to base's new URL-based authority parsing, which never had to guess at these boundaries — that is a reasonable call and I would rather make it now than add a sixth widening later.
71 tests green. Three files in packages/cli/src/serve fail on my machine — fast-path, daemon-logger, capabilities-docs-contract — with EPERM: operation not permitted, symlink, which is Windows without symlink privilege and not related to this change.
| const closed = value.lastIndexOf(closer, bound - 1); | ||
| if (closed >= afterMarker) { | ||
| const upTo = value.slice(afterMarker, closed); | ||
| if (PORT_AT_END.test(upTo) && !upTo.includes('@')) return closed; |
There was a problem hiding this comment.
[Suggestion] Host rewrite when a balanced-quoted credentialed URL is followed later on the same line by a second quoted span containing @ (e.g. a quoted email). lastIndexOf(closer, bound - 1) lands on the LAST quote on the line (the email's closing quote), PORT_AT_END fails on the text between, so the closer shortcut gives up and the span runs to end-of-line; the greedy username class [^\s/?#'"\x60<>]*: then backtracks to a later : and sanitizeProviderBaseUrl strips at a prose @.
— Concrete cost: Error at "https://admin:s3cr3t@internal:8443" — email "ops@company.com" for help → Error at "https://company.com" for help — the host is rewritten from internal:8443 to company.com and the contact prose is deleted (the credential is still stripped, so this is corruption, not a leak). Regression: base preserved https://internal:8443 with the prose intact. The uncredentialed variant ("https://internal:8443" with no credentials) produces the same wrong output on base as well, so that one is pre-existing; the credentialed case is where this diff regresses. It also sits in tension with the CREDENTIAL_PREFIX_PATTERN comment's claim that a URL "wrapped in a balanced pair … is handled" — it is, unless a second quoted span on the line steals lastIndexOf.
Fix direction: search backward through closer occurrences and return the rightmost one where PORT_AT_END matches and the span carries a recognizable credential prefix. The naive backward search guarded by !upTo.includes('@') does not flip this case (the credentialed URL's own span contains @) and breaks the password itself looks like a port test, so the closer needs to be validated against the credential prefix rather than the mere absence of @.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Confirmed on both halves, exactly as you split them — fixed in f5c513e for the credentialed case, and I am leaving the uncredentialed one broken on purpose, with the reason recorded in the code.
in Error at "https://admin:s3cr3t@internal:8443" - email "ops@company.com" for help
before Error at "https://company.com" for help
after Error at "https://internal:8443" - email "ops@company.com" for help
Your warning about the naive backward search was the whole difficulty, and it cost me three candidates before I read it as the constraint it is. !upTo.includes('@') cannot gate the walk, because the credentialed span carries its own @; but PORT_AT_END alone cannot either, because a closer inside a password reaches the walk too and user:8443 is character-for-character a host:port. What separates them is that user:8443 has no @ at all, so the walk requires a complete userinfo@host:port:
"admin:s3cr3t@internal:8443" -> accepted
"user:8443" -> rejected (no userinfo)
"user:8443\"abc secret@host.example/v1" -> rejected
"api.example:8443" -> rejected (no userinfo; handled by the last-closer rule)
The walk also starts at last - 1, not last, so the last closer keeps the existing rule verbatim and nothing that works today changes route.
The uncredentialed variant I did not fix, and I do not think it should be fixed this way. You are right that it is pre-existing; what I want to add is that its input is indistinguishable from a pinned one:
"https://internal:8443" … "ops@company.com" want: keep internal:8443
"https://user:8443"abc secret@host.example/v1" want: strip the password
internal:8443 and user:8443 are the same shape, and what follows the closer is prose in one and the rest of a credential in the other — so widening the walk to accept a bare host:port fixes the first message and leaves the password in the second. That is a leak traded for a corruption, so I left it and wrote the reasoning into the comment instead of quietly declining it.
Three mutations:
| mutation | failures |
|---|---|
| walk accepts any port-at-end | 1 — the pinned password itself looks like a port, and it leaks |
| walk deleted | 1 — the new test only |
last closer no longer requires the absence of @ |
0 |
The third surviving is worth stating rather than hiding. It changes the span on three inputs I constructed but the output on none of fourteen probed end-to-end, so after the walk exists it looks redundant — I kept it because "no test covers it" and "it is safe to delete" are different claims and I only measured the first.
Revision walk, credentialed / uncredentialed:
| revision | credentialed | uncredentialed |
|---|---|---|
base 6a432ad |
internal:8443 kept |
company.com (broken here too) |
ce14a6d (mine) |
company.com |
company.com |
8ab9eef, 9ae36ac, 68fe634 |
company.com |
company.com |
f5c513e |
internal:8443 kept |
company.com |
Which matches your note precisely, and makes this the fourth finding in this batch that traces to ce14a6d. It also answers your point about the CREDENTIAL_PREFIX_PATTERN comment: the claim that a balanced pair "is handled" was true only for a line with one quoted span, and I have amended the URL_QUOTE_PAIRS doc, which asserted a closer "must be the last one within the span" — that is now what a last closer must satisfy, not what any closer must.
72 tests green, prettier/eslint/tsc clean. The EPERM: operation not permitted, symlink failures in fast-path, daemon-logger and capabilities-docs-contract are Windows without symlink privilege, unrelated to this change.
The standing offer on #discussion_r3689070573 applies here too, and this finding strengthens it: four regressions from one commit, each fixed by a further narrowing of the same accumulated grammar. If you would rather see this branch restructured around base's new URL authority parsing than widened again, say so and I will do that instead of adding a fifth patch.
`HOST_AFTER_USERINFO` requires two characters of a bare label, so `@h` matched no alternative and `CREDENTIAL_PREFIX_PATTERN` declined a userinfo that was really there. `findUrlEnd` then fell back to cutting at the first space, which is inside the password, and the slice handed to `sanitizeProviderBaseUrl` had no `@` at all -- so nothing was stripped and the credential came back as prose: in Failed https://user:my pass@h retry later out Failed https://user:my pass@h retry later <- full credential base Failed https://h retry later A regression against the branch point, and the same for an empty username (`https://:my pass@h`). One-character hosts are real: container names and /etc/hosts aliases are often a single letter. `CREDENTIAL_FALLBACK_PATTERN` is consulted only after the primary pattern has declined, and differs from it in exactly one place -- the `@` may be followed by a one-character bare host. The port lookahead is kept verbatim, because that is what tells a userinfo from `:8443 - contact admin@example.com`, where there is no credential and cutting at the email's `@` would rewrite the host. Widening the host rule is safe in that state in a way it is not in the primary pattern. There, a one-character "host" that is really a password fragment rewrites a host that was fine -- which is why the two-character floor exists, and why `p@s s@host.example` still resolves through the primary pattern. Here, the alternative is not "leave the host alone" but "emit the password". Four tests: the two leaks, the `p@s s` boundary that the floor exists for, and a no-credential message ending in a one-character word before an email, which fails if the port lookahead is ever dropped from the fallback. With the fallback disabled, exactly the two leak tests fail and the two boundary tests still pass. Not fixed here, because a port and a password are genuinely indistinguishable in them: `user:123. secret@host` (digits then a PORT_END char) and the quoted-span closer case. Both need the colon in `host:port` told apart from the colon in `user:password`, which is the ambiguity `CREDENTIAL_PREFIX_PATTERN` already documents; a fix guessing either way trades one leak for another.
|
All three findings on I reproduced each against the head before touching anything, and confirmed the "regression" characterisation by running the same three inputs against the branch point:
So both Criticals are real leaks and regressions against base. Confirmed. Fixed: the bare one-character host
Widening the host rule is safe in that state in a way it is not in the primary pattern. There, a one-char "host" that is really a password fragment rewrites a host that was fine — which is why the two-character floor exists, and Four tests: the two leaks ( 68 tests in the file pass; Not fixed, and why the suggested fixes reopen leaksI tried both suggested directions and measured what they do. Reporting the failures because they bound the fix space.
Four inputs, one structure, two opposite correct answers. Any rule keyed on this shape moves the three no-credential cases, and those three are pinned tests — The quoted-span closer. The suggestion is to search closers backward and validate against the credential prefix instead of But it reopens exactly the leak the
Adding "or the span carries a credential prefix" fixes that one and breaks the pinned The common blocker: both need the colon in I would rather ship the one case that has an unambiguous discriminator than land a fourth iteration that moves the leak somewhere the tests do not look. If you would prefer these two fixed in this PR anyway, say so and I will take the version that keeps the leaks pinned as explicit tests rather than silently. Note: the working tree here also has an in-progress |
Two cases for the `PORT_DIGITS` rule, one on each side of it, because the rule
resolves an ambiguity rather than computing an answer and a test on only one
side would be satisfied by the naive fix.
`https://user:123. secret@host.example/v1` is a password whose first word is
digits closing on a `PORT_END` character, which is character-for-character how a
port ending a sentence looks. Read as a port, the credential prefix was not
recognised, `findUrlEnd` cut at the first space -- inside the password -- and
the slice reaching `sanitizeProviderBaseUrl` had no `@` left to strip, so
`user:123. secret@` was re-emitted verbatim in the `/status` payload.
`CREDENTIAL_FALLBACK_PATTERN` does not reach it either: its host is a single
character by construction and this host is `host.example`.
Measured through `createWorkspaceProvidersStatusProvider`:
in Failed https://user:123. secret@host.example/v1
before Failed https://user:123. secret@host.example/v1 <- credential kept
after Failed https://host.example/v1
The second case is the port that really is a port. Each kills a different
mutation:
- restoring the bare `${PORT_END}` arm fails the leak test only;
- deleting the `${PORT_END}` arm outright -- the naive fix for the leak -- fails
the new port-guard test *and* the pre-existing `:8443, contact` one, both of
which rewrite the host to `example.com`.
So the guard is not redundant with the leak test; it pins which direction the
ambiguity is resolved in, which is the part a later change could quietly flip.
…ry matches The one-character-host defect has a second half that 9ae36ac does not close, because there the primary pattern does not decline -- it matches at the *wrong* `@`. A one-char host is invisible to `HOST_AFTER_USERINFO`, so in Cannot reach https://user:pass@h — contact admin@example.com the lazy tail runs past `h` and matches at the email's `@`. The credential is still stripped, so this is corruption rather than a leak, but the host is rewritten from `h` to `example.com` and the contact prose is deleted: before Cannot reach https://example.com after Cannot reach https://h — contact admin@example.com `CREDENTIAL_FALLBACK_PATTERN` finds the right `@` already; it was only consulted when the primary pattern failed. It is now consulted either way, and when both match, which to believe is decided by the whitespace between them: - two spaces or more is prose, so the primary reached out of the URL to find a host and the earlier `@` wins; - one space is a one-space password, which this file already treats as ambiguous and resolves in favour of the better-evidenced later host -- the pinned `p@s s@host.example` case -- so the primary keeps it. That is the same "a second space means prose" test `PORT_DIGITS` makes, applied to the other ambiguity, rather than a new heuristic. Three mutations, each killed by a different set: | mutation | failures | |---|---| | consult the fallback only when the primary declines | 1 | | always prefer the fallback (drop the two-space guard) | 2 | | treat one space as enough | 2 | The last two matter: both would fix the reported case, and both break the pinned `p@s s@host.example` tradeoff, so the guard is what keeps this from being a trade of one defect for another.
`findUrlSegmentEnd` took `lastIndexOf(closer, bound - 1)` — the last closer on the line, which is the wrong one as soon as a second quoted span follows. For `Error at "https://admin:s3cr3t@internal:8443" - email "ops@company.com"` it lands on the email's quote, the port test fails on the text between, and the closer carries no information; the span then runs to end-of-line, a later `:` becomes the username and the prose `@` its terminator, rewriting the host to `company.com` and deleting the contact text. The credential is still stripped, so this is corruption rather than a leak. So when the last closer does not qualify, walk left through the remaining ones. The test there has to be stricter than a port at the end and no `@`: the credentialed span holds an `@` of its own, and a password can end in `:digits` — `user:8443` is character-for-character a `host:port`. Requiring the userinfo *and* the host separates them, since `user:8443` has no `@` at all, and keeps the pinned password-looks-like-a-port case stripping. The uncredentialed `"https://internal:8443"` followed by a quoted email is still wrong, as it is on base. There is no signal that separates its `internal:8443` from that password: same shape, and what follows the closer is prose in one case and the rest of a credential in the other. Accepting a bare `host:port` here would fix the message and leave the password in the other, which is the worse trade — recorded in the comment rather than taken. Measured across the branch, credentialed / uncredentialed: base 6a432ad internal:8443 kept / company.com (latter broken here too) ce14a6d company.com / company.com (regression) 8ab9eef, 9ae36ac, 68fe634 unchanged this commit internal:8443 kept / company.com Three mutations: accepting any port-at-end in the walk leaks the pinned password (1 failure), deleting the walk fails only the new test (1), and dropping the first closer's `@` test fails nothing — it changes the span on three inputs but not the output on any of fourteen probed, so it is kept as pre-existing behaviour rather than removed on no evidence.
| const HOST_CHAR = String.raw`[\p{L}\p{N}\p{M}]`; | ||
| const LABEL_CHAR = String.raw`[\p{L}\p{N}\p{M}-]`; |
There was a problem hiding this comment.
[Critical] Credential leak: the host grammar admits no host whose first character is outside [\p{L}\p{N}\p{M}] (notably _, and also - and .). A credentialed URL with such a host is matched by neither CREDENTIAL_PREFIX_PATTERN nor CREDENTIAL_FALLBACK_PATTERN; when the password also contains a space, findUrlEnd falls through to the first-space cut inside the password, and sanitizeProviderBaseUrl receives an @-less slice it returns unchanged — so the full credential is re-appended verbatim into the /status payload.
— Failure scenario: Failed https://user:my pass@_host/v1 → Failed https://user:my pass@_host/v1 (password my pass visible). Reproduced against the real code through createWorkspaceProvidersStatusProvider; the same leak hits @_svc.local, @-host, @.host, and a slash-containing spaced password (https://user:a b/c@host.example/v1). The control https://user:my pass@host.example/v1 strips correctly. The -/./slash variants are regressions versus base (base stripped them); _ is a never-handled gap that this file's own guarantee — "Not recognising a host is what leaks a credential" — and the bare-label support added for intranet/k8s/container names should now cover.
| const HOST_CHAR = String.raw`[\p{L}\p{N}\p{M}]`; | |
| const LABEL_CHAR = String.raw`[\p{L}\p{N}\p{M}-]`; | |
| const HOST_CHAR = String.raw`[\p{L}\p{N}\p{M}_]`; | |
| const LABEL_CHAR = String.raw`[\p{L}\p{N}\p{M}_-]`; |
Admitting _ lets @_host/@_svc.local match so the primary pattern consumes the userinfo. The -/.-led and slash-in-password variants are the same class and deserve a deliberate decision too; at minimum pin the underscore case with a regression test. Note CREDENTIALED_AUTHORITY_AT_END (line 626) spells its middle class inline and will not pick up this widening — see the separate suggestion there.
— qwen3.8-max-preview via Qwen Code /review
| * legal in a password: `:123%abc secret@host` was read as a port and its | ||
| * password left in the message. | ||
| * | ||
| * Quotes and brackets are deliberately absent for the same reason, even though |
There was a problem hiding this comment.
[Suggestion] The comment says "Quotes and brackets are deliberately absent" but PORT_END = [,.;:!?)\]}] on line 322 includes three closing brackets — ), ], }. Comment and code contradict each other on a security-sensitive regex grammar. — Concrete cost: those closing brackets are load-bearing (via PORT_DIGITS they let :8443) / :8443] / :8443} read as a port, which is what stops a following prose @ such as :8443) contact admin@example.com from being taken as a userinfo terminator). A maintainer who trusts "brackets are deliberately absent" and removes them reintroduces exactly the host-rewrite/leak the surrounding code works to prevent.
| * Quotes and brackets are deliberately absent for the same reason, even though | |
| * Quotes and opening brackets are deliberately absent for the same reason, even though |
— qwen3.8-max-preview via Qwen Code /review
| * reports `evil.com`. Both suites pass under that change; the two | ||
| * `holds an @` cases in `serve/workspace-providers-status.test.ts` are what | ||
| * catch it. |
There was a problem hiding this comment.
[Suggestion] This comment admits the gap but does not close it: the collocated sanitizeProviderBaseUrl cases in utils/acpModelUtils.test.ts have no case where ? or # bounds the authority with a credential @ before it and an @ after it (the https://u:p@host.example:8443?k=v@evil.com shape this comment names). — Concrete cost: verified empirically — temporarily narrowing findAuthorityEnd (dropping the ?/# bounds) leaves acpModelUtils.test.ts green at 31/31, and only the two holds an @ cases in serve/workspace-providers-status.test.ts (a different directory) fail. A developer changing findAuthorityEnd who runs only the collocated suite sees green while the host-rewrite regression slips through.
| * reports `evil.com`. Both suites pass under that change; the two | |
| * `holds an @` cases in `serve/workspace-providers-status.test.ts` are what | |
| * catch it. | |
| * reports `evil.com`. Both suites pass under that change; the two | |
| * `holds an @` cases in `serve/workspace-providers-status.test.ts` are what | |
| * catch it. Adding the `?k=v@` / `#f@` shapes to `acpModelUtils.test.ts` would | |
| * let the collocated suite catch it too. |
— qwen3.8-max-preview via Qwen Code /review
| */ | ||
| const CREDENTIALED_AUTHORITY_AT_END = new RegExp( | ||
| String.raw`^[^\s/?#'"\x60<>]*:[^/?#]*?@` + | ||
| String.raw`${HOST_CHAR}(?:[\p{L}\p{N}\p{M}.-]*${HOST_CHAR})?:\d+$`, |
There was a problem hiding this comment.
[Suggestion] CREDENTIALED_AUTHORITY_AT_END is the only host pattern that spells the Unicode class inline ([\p{L}\p{N}\p{M}.-]*) instead of composing it from LABEL_CHAR; every other host pattern (UNDELIMITED_HOST, DELIMITED_HOST, FALLBACK_HOST) builds on the shared HOST_CHAR/LABEL_CHAR constants. — Concrete cost: the natural fix for the credential leak above (add _ to HOST_CHAR/LABEL_CHAR) propagates to the primary/fallback patterns and HOST_AFTER_USERINFO automatically but silently leaves this pattern rejecting _ mid-host, so a quoted "https://user:pass@my_host:8443" followed by a second quoted span still mis-bounds the segment after that fix. No independent present-day bug (today _ is rejected uniformly), so this is a coupling hazard rather than a blocker.
| String.raw`${HOST_CHAR}(?:[\p{L}\p{N}\p{M}.-]*${HOST_CHAR})?:\d+$`, | |
| String.raw`${HOST_CHAR}(?:${HOST_MIDDLE_CHAR}*${HOST_CHAR})?:\d+$`, |
(where HOST_MIDDLE_CHAR is LABEL_CHAR plus the dot the cross-label match needs, e.g. const HOST_MIDDLE_CHAR = String.raw`(?:${LABEL_CHAR}|\.)`; — keeps the intentional dot admission while letting any future widening of LABEL_CHAR propagate.)
— qwen3.8-max-preview via Qwen Code /review
🩺 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 |
A host whose first character is one a valid host cannot start with was matched by neither credential pattern, so `findUrlEnd` fell through to cutting at the first space -- inside the password -- and the `@`-less slice came back from `sanitizeProviderBaseUrl` verbatim with the rest of the credential re-appended as prose. `_host` leaked before this branch; `-host` and `.host` are shapes the replaced code stripped, because it tested one character class instead of matching a host grammar. This is the one place the grammar must not be a validator: rejecting a host does not make the URL invalid, it makes the authority unrecognised, and being liberal here can only remove more text from the payload. `CREDENTIALED_AUTHORITY_AT_END` now composes its host middle from the shared classes rather than spelling them inline, which is why it kept rejecting `@_host:8443` after `HOST_CHAR` gained `_`. A slash in the password reached the same leak through the one delimiter both patterns treat as absolute: their `[^/?#]*?@` tail cannot cross it, and it must not in general, since a path may legally contain an `@`. `SLASHED_PASSWORD_PATTERN` crosses it under two gates and is consulted only after both others decline -- a space must precede the `@`, which is the precondition for the leak and the one thing a path cannot hold, and the port lookahead excludes `\d+[/?#]` so that `:8443/v1`, newly reachable, is not read as a userinfo. `PORT_END`'s comment claimed brackets were absent while `)`, `]` and `}` were in the set. They are safe, but not for the reason given: the `no userinfo follows` lookahead is what carries them. Quotes are the one addition that is not interchangeable, and two spellings where the lookahead does not fire are now pinned -- both pass with quotes added if asserted on `JSON.stringify`, which escapes the quote in the expected password. Also pins `?` and `#` as the authority terminator in `sanitizeProviderBaseUrl`, with and without a credential to strip. Measured: 7 new tests in workspace-providers-status.test.ts are red on the previous revision of this branch (5 failed / 74 passed) and green with the fix (81 passed); the two quote tests are red with `"` and `'` added to `PORT_END`. acpModelUtils.test.ts: 36 passed. The wider `serve`/`utils` selection fails the same 80 tests before and after -- failure sets diffed, not counts. The one test that differed, `server.test.ts > ... refreshes cached serve features`, fails intermittently on the unmodified tree as well, once in two runs.
|
Fixed in
Three of the five were strictly worse than the code I replaced. Base got them right for a reason worth recording: it did not match a host grammar at all, it took A host grammar in a sanitizer must not be a validator. That is the actual lesson, and I had it backwards. One divergence was invisible until I composed it away: The slash case is a different mechanism, and the one I found least comfortable to fix. Both patterns spell the userinfo tail So
On the Which then made the symmetric question answerable instead of hand-waved, and the answer surprised me. I added Both fail the Also added the missing Measurements. 7 new tests are red on the previous revision of this branch and green with the fix; the two quote tests are red with quotes added to The wider |
| const SLASHED_PASSWORD_PATTERN = new RegExp( | ||
| String.raw`^[^\s/?#'"\x60<>]*:(?!${PORT_DIGITS}|\d+[/?#])` + | ||
| String.raw`(?=[^?#@]*\s)[^?#]*?@(?=${HOST_AFTER_USERINFO})`, |
There was a problem hiding this comment.
[Critical] SLASHED_PASSWORD_PATTERN (added by this PR) still leaks three classes of spaced/slashed credential that the removed code stripped. Each is probe- and A/B-confirmed against the merge base:
(a) Spaced password before a pathless one-character host — the host lookahead (?=${HOST_AFTER_USERINFO}) rejects a bare one-char host and the fallback cannot cross /, so nothing matches and findUrlEnd cuts inside the password. Failed loading provider https://user:a b/c@h retry later → unchanged (full credential); base → https://h retry later.
(b) # or ? inside a spaced password — every tail ([^/?#]*?@ / [^?#]*?@) cannot cross #/?, and unlike / nothing crosses them. Cannot reach https://user:pa#ss word@host.example/v1 retry → unchanged; base → https://host.example/v1 retry (the ? spelling leaks identically).
(c) First word is digits-then-/ — the \d+[/?#] port exclusion declines it. Failed loading provider https://user:123/abc secret@host.example/v1 → unchanged; base → https://host.example/v1. This exclusion is load-bearing (it protects :8443/v1 contact admin@), so it cannot simply be deleted.
— Failure scenario: each input reaches the /status payload with the full credential visible — the leak direction this file exists to prevent. None is among the documented tradeoffs.
Suggested fix: (a) give SLASHED_PASSWORD_PATTERN the fallback's one-char-host escape hatch (FALLBACK_HOST) under the same two gates; (b) give it (or a sibling) a userinfo tail that crosses #/? under the same gates; (c) gate \d+[/?#] the way PORT_DIGITS gates its bracket case — read it as a port only when no spaced credential follows it. Pin each input with a regression test.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| if (fallback[0].length >= credentials[0].length) return false; | ||
| const between = body.slice(fallback[0].length, credentials[0].length); | ||
| return (between.match(/\s/g) ?? []).length >= 2; |
There was a problem hiding this comment.
[Critical] prefersFallback's "≥2 spaces means prose" rule rewrites the host for a pathless single-character host followed by exactly one prose word and an email. Cannot reach https://user:pass@h admin@example.com → Cannot reach https://example.com (host rewritten h→example.com, contact prose deleted); base produced https://h admin@example.com. A/B-confirmed regression.
This is not the documented one-space tradeoff: the pinned p@s s@host.example case resolves in favour of a later host that has structural evidence (a path via DELIMITED_HOST), whereas here the later "host" example.com is a prose email with no path/port/query. This one-word shape is not test-pinned — the closest test ('keeps a one-character host when prose after it contains an @') uses — contact between @h and admin@, i.e. ≥2 spaces, a different code path.
— Failure scenario: the oncall sees a wrong server name (example.com) and the contact line is deleted.
Suggested fix: also prefer the fallback when its host candidate is a one-character bare label immediately followed by whitespace (the shape the rest of this PR deliberately supports), rather than gating purely on a ≥2-space count; at minimum pin this spelling so the one-space resolution is a deliberate choice.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| const CREDENTIAL_PREFIX_PATTERN = new RegExp( | ||
| // `\x60` is the backtick, spelled as an escape because a literal one cannot | ||
| // appear in the template and `\`` is not a valid escape under the `u` flag. | ||
| String.raw`^[^\s/?#'"\x60<>]*:(?!${PORT_DIGITS})[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, |
There was a problem hiding this comment.
[Critical] The credential patterns' username class [^\s/?#'"\x60<>]* admits : and @, so the greedy quantifier consumes past the credential's own @ and re-anchors on a later colon; a trailing prose email's @ then becomes the strip boundary — rewriting the host and deleting prose. Two probe- and A/B-confirmed triggers:
(a) Bracketed-IPv6 internal colons: Error at "https://admin:s3cr3t@[::1]:8443" - email "ops@company.com" for help → Error at "https://company.com" for help; base preserved [::1]:8443.
(b) A quoted credentialed URL with a port followed by an unquoted prose email — the closer's first check bails on upTo.includes('@') and the walk-left finds no second closer, so the whole span reaches findUrlEnd and the greedy username swallows user:token@host.example via the port colon: "https://user:token@host.example:8443" — contact admin@example.com → "https://example.com; base preserved https://host.example:8443. The quoted-email sibling is test-pinned (line ~1351); this is the unquoted-email hole in that same guard.
— Failure scenario: the reported host becomes the email's domain and the contact prose is deleted — "the failure this file works hardest to avoid."
Suggested fix: exclude @ (and :) from the username class in all three credential patterns and CREDENTIALED_AUTHORITY_AT_END — [^\s/?#'"\x60<>:@]*. Verified: this flips both outputs to correct and all 81 tests still pass. (For (b) you may additionally admit CREDENTIALED_AUTHORITY_AT_END to the closer's first check so the span is truncated before any pattern sees the prose.)
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| const prefix = credentials ?? SLASHED_PASSWORD_PATTERN.exec(body); | ||
| const from = prefix ? markerLength + prefix[0].length : markerLength; | ||
| const space = segment.slice(from).search(/\s/); |
There was a problem hiding this comment.
[Critical] A password whose first character is / is recognised by no credential pattern; findUrlEnd falls to the first-space cut and hands the whole URL to sanitizeProviderBaseUrl, where new URL parses without throwing (hostname app, pathname /secret@…, empty username/password), so the if (parsed.username || parsed.password) branch is skipped and the URL is returned unchanged — full leak.
failed: https://app:/secret@svc.local:8443/api retry → unchanged (credential app:/secret present); base stripped to https://svc.local:8443/api retry. A/B-confirmed regression; the same leak hits https://user:/token@host.example/v1. Non-leading-slash passwords still strip (there new URL throws and the catch-path fallback fires). No test covers a slash-leading password.
— Failure scenario: the full credential reaches the /status payload.
Suggested fix: in sanitizeProviderBaseUrl (acpModelUtils.ts), do not unconditionally return baseUrl when new URL succeeds with empty userinfo — also consult the already-present findUnescapedUserInfoFallbackAt(baseUrl, authorityStart, authorityEnd) in that branch and stripAt when it returns an index (guarding its /^\d+$/ port test so user:123/…@host is not mistaken for host:port). Verified to flip the output without breaking existing cases.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| // A pathless authority bounded by `?` or `#` rather than `/`. The rows above | ||
| // pin `?`/`#` inside the *userinfo*; these pin them as the authority | ||
| // terminator, which is the direction `findAuthorityEnd` exists for. Drop |
There was a problem hiding this comment.
[Critical] findAuthorityEnd bounds the authority on /, ?, # only and misses \, which WHATWG treats as / for special schemes. The new warning-path delegation to sanitizeProviderBaseUrl makes the resulting host rewrite newly reachable from free-text messages.
Failed loading provider https://user:secret@api.example\@evil.com/ now → Failed loading provider https://evil.com/ now (host rewritten api.example→evil.com); base preserved the host (https://api.example\@evil.com/ now). A/B-confirmed regression introduced by the delegation: the deleted sanitizeProviderWarningSegment cut at the first @ and never consulted sanitizeProviderBaseUrl for this input. (The direct model.baseUrl path was already exposed pre-PR; the warning path is the new reachability.) These new test rows pin ?/# as authority terminators but not \, so neither suite catches it.
— Failure scenario: the reported host becomes evil.com — the exact failure the JSDoc this PR adds to sanitizeProviderBaseUrl calls load-bearing.
Suggested fix: in findAuthorityEnd, also min in baseUrl.indexOf('\\', authorityStart); add a pin row such as ['https://user:secret@api.example\\@evil.com/', 'https://api.example\\@evil.com/'] alongside the ?/# rows. Verified: flips the output and keeps all 117 tests green.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| expect(result.errors?.[0]?.error).toBe( | ||
| `Cannot reach ${open}https://host.example/v1${close} — retry`, | ||
| ); | ||
| expect(JSON.stringify(result)).not.toContain(`pa${close}ss`); |
There was a problem hiding this comment.
[Suggestion] The not.toContain credential-leak guard is vacuous for the " parametrization: JSON.stringify escapes " to \", so a regression leaking pa"ss renders as pa\"ss and not.toContain('pa"ss') (searching for the unescaped substring) still passes — probe-confirmed. The test is saved only by its exact .toBe on the full error string; if that were ever relaxed, the " leak would ship uncaught. Affects this line and the hardcoded not.toContain('pa"ss') at line 1383.
— Concrete cost: the assertion pins nothing for the " case. The other five parametrizations (', backtick, <, (, [) are not escaped by JSON.stringify, so their guards work correctly.
Suggested fix: assert on the message directly — expect(result.errors?.[0]?.error).not.toContain(pa${close}ss) (and likewise on result.errors?.[0]?.error at line 1383) — or drop the redundant line since the .toBe already covers it.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| * thinks the authority ends is load-bearing for a caller in another directory. | ||
| * Narrowing `findAuthorityEnd` still strips the credentials, but it rewrites the | ||
| * host from whatever follows: `https://u:p@host.example:8443?k=v@evil.com` | ||
| * reports `evil.com`. Both suites pass under that change; the two |
There was a problem hiding this comment.
[Suggestion] This comment claims "Both suites pass under that change" (narrowing findAuthorityEnd to drop the ?/# bounds), but the acpModelUtils.test.ts rows added in this same diff fail (3 tests) under that change — mutation-confirmed. A maintainer following the guidance would observe failures the comment implies cannot occur, and would misunderstand which tests guard the cross-file invariant.
— Concrete cost: misleading mutation-testing guidance on a security-sensitive contract; the comment names only the two holds an @ cases in serve/workspace-providers-status.test.ts as the catch set, when the collocated acpModelUtils.test.ts rows catch it too.
Suggested fix: reword to note that the rows added here also catch it, e.g. "Before the rows above were added, both suites passed under that change; those rows and the two holds an @ cases in serve/workspace-providers-status.test.ts are what catch it."
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
I reproduced all five [Critical] findings and confirmed the A/B claim on each. They are all real, and they are all regressions against the merge base. I'm not going to attempt a seventh fix round — I think the right call is to close this PR, and I'd rather say that myself than have it argued out of me. Verification. I drove all ten inputs through the public path (
Ten for ten: clean at base, broken at HEAD. Six are outright credential leaks into the The suite does not catch any of them. 117 tests pass at HEAD across both files. So the tests I added over six rounds encode the cases I thought of and are blind to every one of these — which is the strongest argument that the tests aren't the problem. Net effect is negative, and I checked that rather than assuming. The two bugs this PR was opened for are real, and base does get them wrong:
So this is 2 fixed against 6 new leaks and 4 host rewrites. Trading two known failures for ten unknown ones is a bad trade in a sanitizer, and it's the trade currently on the table. Why I think it can't be patched into shape. The shape of the last six rounds is the argument: 14 commits and +1478/−31 to replace 31 lines, and round 6 still finds five Criticals of the same kind as rounds 1–5. Each fix widens a regex or adds a gate, and each widening creates a new interaction with the others — What I'd suggest instead, if the two original bugs are worth fixing:
Closing this. Thanks to the reviewer for the A/B discipline — attaching the base comparison to every finding is what made this diagnosable instead of arguable, and it's why I could confirm rather than debate all five. Happy to open a narrow PR for (1) if that's wanted; I'll leave (2) to whoever owns this file, since it's a policy decision about over- vs under-redaction rather than a bug fix. |
2 participants
What this PR does
Makes the provider warning sanitizer find credentials the same way its sibling already does: by bounding the search to the URL's authority instead of scanning the whole message with
indexOf. The bespoke credential heuristics are deleted and each URL-looking span is handed tosanitizeProviderBaseUrl, which already computes where the authority ends and takeslastIndexOf('@')inside it. Net deletion of production logic, no new helper.Why it's needed
sanitizeProviderWarningSegmentlocated the userinfo withindexOf(':', markerLength)andindexOf('@', markerLength)over the entire span, with no idea where the authority stopped. That produced two distinct failures.A port made it delete the text after the URL. The
:before the port was read as the start of a password, so any later@in the message was taken as the end of the userinfo and everything between them was cut:The port, the path, the em-dash and
contactare all gone. Any warning naming a URL with an explicit port and containing a later@— an email address, an npm scope, auser@host— is mangled this way. This is not over-redaction; the message the user is shown is factually wrong about which endpoint failed.An
@inside the password made it leak the rest of the password.indexOf('@')found the one inside the password, so the cut landed too early:sanitizeProviderBaseUrlgets both of these right already — it bounds the search withfindAuthorityEndand falls back to a port check for inputsnew URL()rejects. The warning path only reached it in the fallback branch, which the heuristics pre-empted.Both call sites are live in the
/statusprovider:workspace-providers-status.ts:207for everyresolvedCliConfig.warningsentry, and thecatchat:223for anyModelsConfigconstruction error.The span-splitting loop is kept deliberately. It is what allows a password containing a space or a quote to be found at all — those are legal in userinfo but excluded by any
[^\s'"]-style URL pattern, so a single global regex cannot match them. The existinghttps://user:p ass@…test case depends on this.Reviewer Test Plan
How to verify
To see the bugs, revert
workspace-providers-status.tsalone and keep the tests:Two of the three added cases fail, with exactly the outputs quoted above. The third (
strips credentials from every URL in a message and leaves the rest intact) passes onmaintoo — it is there as a guard, not as a repro.Evidence (Before & After)
Not user-visible in the TUI; this is the
/statusJSON payload. Behaviour measured directly:mainCannot reach https://api.example:8443/v1 — contact admin@example.comCannot reach https://example.comSet https://registry.example:4873/ then install @scope/pkgSet https://scope/pkgFailed loading provider https://user:p@ssw0rd-tail@broken.example/v1…https://ssw0rd-tail@broken.example/v1…https://broken.example/v1Auth failed for https://user:p ass@api.example/v1Auth failed for https://api.example/v1Invalid baseUrl "https://api.example/v1" — contact admin@example.comTest results:
workspace-providers-status.test.ts— 22 pass (19 existing + 3 added).acpModelUtils.test.ts— 31 pass, untouched;sanitizeProviderBaseUrl's own behaviour does not change.npx eslint … --max-warnings 0— clean.npx prettier --check— clean.tsc -p packages/cli/tsconfig.json --noEmit— no errors in the touched file. The pre-existing errors on this checkout are stale-distresolution failures in unrelated packages (acpAgent.ts,Session.test.ts, …), identical before and after.grepconfirms nothing else referencedsanitizeProviderWarningSegment,hasCredentialPrefix, orURL_LIKE_PATTERN.Tested on
Environment (optional)
Unit tests only, via
npx vitest run. No platform-specific code in the diff.Risk & Scope
sanitizeProviderBaseUrlitself. Its own gaps are separate and untouched here — notably it leaves a URL alone when a scheme is not at position 0 (a leading space defeats the^-anchored match) and it does not redact credentials passed as query parameters (?api-key=…). Neither is reachable from the two warning call sites in a way this change affects, so I have kept them out. Happy to file them separately if useful.Linked Issues
Closes #8136