From f0e21ccebd011627a423554ed60e0010e2737813 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Thu, 30 Jul 2026 21:09:18 +0800 Subject: [PATCH 01/14] fix(cli): scope warning credential stripping to the URL authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #8136 --- .../serve/workspace-providers-status.test.ts | 57 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 43 ++++---------- 2 files changed, 67 insertions(+), 33 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index c4db8bb309f..17b005614dd 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -562,6 +562,63 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(result.initialized).toBe(false); }); + it('keeps a port and the text after it when the message has no credentials', async () => { + // A `:` in the authority used to be read as the start of a password, so any + // later `@` — an email address, an npm scope — was taken as the end of the + // userinfo and everything between them was deleted. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443/v1 — contact admin@example.com'; + 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( + 'Cannot reach https://api.example:8443/v1 — contact admin@example.com', + ); + }); + + it('strips a password containing an @ instead of splitting on the first one', async () => { + // `indexOf('@')` found the one inside the password, so the cut landed too + // early and the rest of the password was emitted. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:p@ssw0rd-tail@broken.example/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://broken.example/v1', + ); + expect(JSON.stringify(result)).not.toContain('ssw0rd-tail'); + }); + + it('strips credentials from every URL in a message and leaves the rest intact', async () => { + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Moving from https://user:sec ret@a.example/v1 to https://user:other@b.example:8443/v2; see admin@example.com'; + 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( + 'Moving from https://a.example/v1 to https://b.example:8443/v2; see admin@example.com', + ); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index eb9fcce8acf..8463fa36efc 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -247,9 +247,17 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { return ApprovalMode.AUTO; } -const URL_LIKE_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\/[^\s'"`<>]+/g; const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; +/** + * Strips credentials from every URL in a free-text warning or error message. + * + * Each URL-looking span is handed to `sanitizeProviderBaseUrl`, which scopes + * credential detection to the authority. Splitting the message into spans first + * rather than matching URLs with one global regex is what lets a password + * containing a space or a quote — legal in userinfo, but excluded by any + * `[^\s'"]`-style URL pattern — still be found. + */ function sanitizeProviderWarning(warning: string): string { let result = ''; let index = 0; @@ -259,8 +267,7 @@ function sanitizeProviderWarning(warning: string): string { result += warning.slice(index, next.index); 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)); index = segmentEnd; next = findNextUrlStart(warning, index); @@ -295,36 +302,6 @@ function findUrlSegmentEnd( return Math.min(lineEnd, nextUrl?.index ?? value.length); } -function sanitizeProviderWarningSegment( - segment: string, - markerLength: number, -): string { - const at = segment.indexOf('@', markerLength); - if ( - at !== -1 && - hasCredentialPrefix(segment, markerLength, at) && - segment[at + 1] !== undefined && - /[A-Za-z0-9.[\]-]/.test(segment[at + 1]) - ) { - return `${segment.slice(0, markerLength)}${segment.slice(at + 1)}`; - } - - return segment.replace(URL_LIKE_PATTERN, (url) => - sanitizeProviderBaseUrl(url), - ); -} - -function hasCredentialPrefix( - segment: string, - markerLength: number, - at: number, -): boolean { - const colon = segment.indexOf(':', markerLength); - if (colon === -1 || colon > at) return false; - const username = segment.slice(markerLength, colon); - return !/[/?#\s'"`<>]/.test(username); -} - function buildCurrent( authType: AuthType | undefined, modelId: string | undefined, From 98cc5be82c27d1f7adc1378f178bf249d12be419 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Thu, 30 Jul 2026 22:25:37 +0800 Subject: [PATCH 02/14] fix(cli): bound the URL before sanitizing, keeping pathless spans safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../serve/workspace-providers-status.test.ts | 57 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 45 +++++++++++++-- 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 17b005614dd..4d3265ec00a 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -619,6 +619,63 @@ describe('createWorkspaceProvidersStatusProvider', () => { ); }); + it('keeps a pathless URL and the email after it intact', async () => { + // A pathless URL has no `/`, `?` or `#` to bound its authority, so handing + // the trailing prose to the sanitizer let the `@` in an email address read + // as the end of a userinfo: the host was rewritten and the text deleted. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example.com — contact admin@example.com'; + 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( + 'Cannot reach https://api.example.com — contact admin@example.com', + ); + }); + + it('keeps a pathless URL with a port and the email after it intact', async () => { + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example.com:8443 — contact admin@example.com'; + 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( + 'Cannot reach https://api.example.com:8443 — contact admin@example.com', + ); + }); + + it('strips a spaced password from a pathless URL followed by prose', async () => { + // The counterpart to the two above: bounding the URL at the first space + // must not stop a password that legally contains one from being found. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:sec ret@broken.example — retry later'; + 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://broken.example — retry later', + ); + expect(JSON.stringify(result)).not.toContain('sec ret'); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 8463fa36efc..e98b48a6a77 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -249,14 +249,22 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; +/** + * A `user:password@` prefix, where the password may contain spaces. The + * negative lookahead rejects `host:8443 ` so a port is not read as the start of + * 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|$))[^/?#]*@/; + /** * Strips credentials from every URL in a free-text warning or error message. * - * Each URL-looking span is handed to `sanitizeProviderBaseUrl`, which scopes - * credential detection to the authority. Splitting the message into spans first - * rather than matching URLs with one global regex is what lets a password - * containing a space or a quote — legal in userinfo, but excluded by any - * `[^\s'"]`-style URL pattern — still be found. + * Each URL is handed to `sanitizeProviderBaseUrl`, which scopes credential + * detection to the authority. Splitting the message into spans first rather than + * matching URLs with one global regex is what lets a password containing a space + * or a quote — legal in userinfo, but excluded by any `[^\s'"]`-style URL + * pattern — still be found. */ function sanitizeProviderWarning(warning: string): string { let result = ''; @@ -267,7 +275,10 @@ function sanitizeProviderWarning(warning: string): string { result += warning.slice(index, next.index); const segmentEnd = findUrlSegmentEnd(warning, next.index, next.marker); - result += sanitizeProviderBaseUrl(warning.slice(next.index, segmentEnd)); + const segment = warning.slice(next.index, segmentEnd); + const urlEnd = findUrlEnd(segment, next.marker.length); + result += + sanitizeProviderBaseUrl(segment.slice(0, urlEnd)) + segment.slice(urlEnd); index = segmentEnd; next = findNextUrlStart(warning, index); @@ -276,6 +287,28 @@ function sanitizeProviderWarning(warning: string): string { return result + warning.slice(index); } +/** + * Where the URL ends inside a span that may continue into prose. + * + * A span runs to the next URL or end of line, so it can carry trailing text. + * Handing that text to `sanitizeProviderBaseUrl` would let an `@` in it — an + * email address, say — be read as the end of a userinfo, since a pathless URL + * has no delimiter to bound the authority. So the URL 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 the whole reason + * the message is split into spans rather than matched with a URL regex. + */ +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; +} + function findNextUrlStart( value: string, from: number, From da101de1d2d3b15ddba638416c9ceaa3186dce99 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Thu, 30 Jul 2026 23:59:11 +0800 Subject: [PATCH 03/14] fix(cli): pick the userinfo delimiter by what follows it, not by greediness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../serve/workspace-providers-status.test.ts | 82 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 37 +++++++-- 2 files changed, 114 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 4d3265ec00a..fb1a47baba1 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -676,6 +676,88 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('sec ret'); }); + it('keeps the host when a pathless URL carries credentials and prose has an email', async () => { + // Both `@`s are candidates for the end of the userinfo. Taking the later one + // deleted the prose and rewrote the host from it, so the credentials were + // stripped but the message named the wrong server. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://user:pass@host.example — contact admin@example.com'; + 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( + 'Cannot reach https://host.example — contact admin@example.com', + ); + expect(JSON.stringify(result)).not.toContain('pass@'); + }); + + it('strips a password that begins with digits and a space', async () => { + // `user:123 ` is shaped exactly like `host:8443 `, so the guard that stops a + // port being read as a password rejected this password and leaked it. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:123 secret@broken.example/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://broken.example/v1', + ); + expect(JSON.stringify(result)).not.toContain('123 secret'); + }); + + it('strips a password containing both an at sign and a space', async () => { + // The two failure modes meet here: a greedy tail runs past the host into the + // prose, a lazy one stops at the `@` inside the password and leaves `s s@` + // behind. Only choosing the `@` by what follows it handles both. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:p@s s@broken.example/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://broken.example/v1', + ); + expect(JSON.stringify(result)).not.toContain('p@s s'); + expect(JSON.stringify(result)).not.toContain('s s@'); + }); + + it('keeps a pathless URL on localhost with a port and the email after it intact', async () => { + // `localhost` and a dotted IPv4 are hosts too, so the port heuristic has to + // survive them as well as a dotted name. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://localhost:8443 — contact admin@example.com'; + 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( + 'Cannot reach https://localhost:8443 — contact admin@example.com', + ); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index e98b48a6a77..2b036db5e63 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -250,12 +250,33 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; /** - * A `user:password@` prefix, where the password may contain spaces. The - * negative lookahead rejects `host:8443 ` so a port is not read as the start of - * a password — that is the only way a colon in the authority can be followed by - * whitespace and still not be credentials. + * A host a provider base URL can address: a dotted name, a bracketed IPv6 + * 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 CREDENTIAL_PREFIX_PATTERN = /^[^\s/?#'"`<>]+:(?!\d+(?:\s|$))[^/?#]*@/; +const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; + +/** + * A `user:password@` prefix, where the password may contain spaces. + * + * Two decisions are encoded here, and each is load-bearing on its own. + * + * Whether there is a userinfo at all: a colon must appear before any + * whitespace, and the negative lookahead rejects digits that read as a port + * rather than the start of a password. `:8443 ` alone is not enough to tell + * the two apart — a password may also begin with digits and a space + * (`user:123 secret@host`) — so the digits count as a port only at end of span + * or when a further space follows before the `@`, which is prose rather than a + * one-space password. + * + * Which `@` ends the userinfo: neither greediness setting is right, because a + * greedy tail runs past the host into an `@` in trailing prose while a lazy one + * stops at an `@` inside the password. So the `@` is chosen structurally — it + * 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})`, +); /** * Strips credentials from every URL in a free-text warning or error message. @@ -297,6 +318,12 @@ function sanitizeProviderWarning(warning: string): string { * except that a recognised `user:password@` prefix is consumed first: a * password may legally contain spaces, and finding those is the whole reason * the message is split into spans rather than matched with a URL regex. + * + * Consuming that prefix is also what keeps the host intact. Were the whole span + * handed over, `sanitizeProviderBaseUrl` would strip at the last `@` in the + * authority it computes and rewrite the host from the prose — turning + * `https://user:pass@host.com — contact admin@example.com` into + * `https://example.com`. */ function findUrlEnd(segment: string, markerLength: number): number { const credentials = CREDENTIAL_PREFIX_PATTERN.exec( From 7ab462b493104335e0a6d57e0740856b34beea48 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 01:45:25 +0800 Subject: [PATCH 04/14] fix(cli): recognise the hosts and delimiters a real message actually 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. --- .../serve/workspace-providers-status.test.ts | 105 ++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 25 ++++- 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index fb1a47baba1..ba120d3da80 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -758,6 +758,111 @@ describe('createWorkspaceProvidersStatusProvider', () => { ); }); + it('strips a spaced password from a URL with a bare single-label host', async () => { + // Requiring a dot in the host excluded exactly the hosts an intranet uses: + // a proxy name, a container name like `ollama`, a k8s service. Not + // recognising the host means not recognising the userinfo, and the fallback + // then cut the URL at the first space -- inside the password. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:my password@internalhost/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://internalhost/v1', + ); + expect(JSON.stringify(result)).not.toContain('my password'); + }); + + it('keeps a single-label host and the prose after it when the URL is pathless', async () => { + // The other half of the same omission: with the real `@` unrecognised, the + // lazy tail ran on into the prose and stripped at the email's `@` instead, + // reporting a host the user never configured and deleting the contact line. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://user:pass@intranet — contact admin@example.com'; + 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( + 'Cannot reach https://intranet — contact admin@example.com', + ); + expect(JSON.stringify(result)).not.toContain('pass@'); + }); + + it('does not read a port as a password when punctuation follows it', async () => { + // A port was only recognised at end of message or before a space, so the + // comma in `:8443, contact` left `8443` looking like the start of a + // password -- and the tail then found the email's `@`. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443, contact admin@example.com'; + 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( + 'Cannot reach https://api.example:8443, contact admin@example.com', + ); + }); + + it('strips a spaced password when the URL ends a sentence', async () => { + // The characters allowed to follow a host were enumerated, and `.` was not + // among them, so a URL at the end of a sentence -- the most ordinary shape + // an error message has -- was not recognised, and the password leaked. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:my pass@host.example. Retry later'; + 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 https://host.example. Retry later', + ); + expect(JSON.stringify(result)).not.toContain('my pass'); + }); + + it('keeps the host and the following sentence when a period ends the URL', async () => { + // Same omission, corrupting instead of leaking: the trailing period made + // the real `@` invisible, so the strip landed on the email in the next + // sentence and both the host and that sentence were rewritten away. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://user:pass@host.example. Contact admin@example.com'; + 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( + 'Cannot reach https://host.example. Contact admin@example.com', + ); + expect(JSON.stringify(result)).not.toContain('pass@'); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 2b036db5e63..24e197764fa 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -250,11 +250,23 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; /** - * A host a provider base URL can address: a dotted name, a bracketed IPv6 - * 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. + * A host a provider base URL can address, with an optional port. + * + * Three shapes, because a single pattern for all of them would be wrong at the + * edges. A bracketed IPv6 literal. A dotted name, whose first label may be a + * single character (`a.example`). Or a bare label — intranet proxies, container + * names like `ollama`, and k8s service names have no dot — required to be at + * least two characters, which is what keeps a one-letter fragment of a password + * from passing for a host. + * + * What follows the host is required only to be something a host cannot continue + * into, rather than a fixed list of delimiters. Enumerating them meant a URL + * ending a sentence was not recognised, since `.`, `,` and `)` were not on the + * list — and the consequence of not recognising a host is a credential leak, so + * 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:.]+\]|localhost|[A-Za-z\d-]+(?:\.[A-Za-z\d-]+)+)(?::\d+)?(?:[/?#\s]|$)`; +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-])`; /** * A `user:password@` prefix, where the password may contain spaces. @@ -265,7 +277,8 @@ const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d * whitespace, and the negative lookahead rejects digits that read as a port * rather than the start of a password. `:8443 ` alone is not enough to tell * the two apart — a password may also begin with digits and a space - * (`user:123 secret@host`) — so the digits count as a port only at end of span + * (`user:123 secret@host`) — so the digits count as a port when they end the + * span, when a non-space follows them (sentence punctuation: `:8443, contact`), * or when a further space follows before the `@`, which is prose rather than a * one-space password. * @@ -275,7 +288,7 @@ const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|localhost|[A-Za-z\d * 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})`, + String.raw`^[^\s/?#'"\`<>]+:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, ); /** From 556ffd4cf23831cd778727cbb0b8953e93c983e5 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 03:24:04 +0800 Subject: [PATCH 05/14] fix(cli): recognise a host by its delimiter, not only by its length 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. --- .../serve/workspace-providers-status.test.ts | 100 ++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 34 +++++- 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index ba120d3da80..b83b8aa2845 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -863,6 +863,106 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('pass@'); }); + it('strips a spaced password from a URL whose host is a single character', async () => { + // The two-character floor on a bare host excluded a one-character one. The + // floor is only needed where a label has no delimiter after it; here the `/` + // says the label is an authority rather than a word, so no length is needed. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = 'Failed https://user:my pass@h/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 https://h/v1'); + expect(JSON.stringify(result)).not.toContain('my pass'); + }); + + it('strips a spaced password when the username is empty', async () => { + // `https://:token@host` is a legal URL and the shape a token-only config + // produces, but requiring a character before the colon meant no userinfo was + // recognised and the fallback cut inside the password. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://:my pass@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('my pass'); + }); + + it('strips credentials from a bracketed IPv6 base URL', async () => { + // Pins the IPv6 branch, which every other case here leaves untouched: it is + // the one host shape whose own characters include a colon, so a change to + // the port or delimiter rules could break it while the rest stay green. + 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'); + }); + + it('leaves a password whose tail reads as a host, and says so', async () => { + // Not a fix: a record of where the boundary is. `word@realhost.example` is + // indistinguishable from a userinfo followed by a host, so `word` survives + // -- as it did before this PR. Pinned so that a later widening of the host + // rules has to decide this case deliberately rather than by accident. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:p@ss word@realhost.example/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 https://ss word@realhost.example/v1', + ); + }); + + it('leaves a two-space password beginning with digits, and says so', async () => { + // The flip side of the port heuristic, and the other half of the tradeoff + // documented on CREDENTIAL_PREFIX_PATTERN: a second space before the `@` is + // what tells `:8443 — contact admin@…` from a password, so a password that + // has one reads as prose. Pinned for the same reason as the case above. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:123 secret word@host.example/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 https://user:123 secret word@host.example/v1', + ); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 24e197764fa..64583b3a5f4 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -250,7 +250,8 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; /** - * A host a provider base URL can address, with an optional port. + * A host with nothing after it to mark where it ends, so its own shape is the + * only evidence that it is one. * * Three shapes, because a single pattern for all of them would be wrong at the * edges. A bracketed IPv6 literal. A dotted name, whose first label may be a @@ -266,7 +267,24 @@ const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; * 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-])`; +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-])`; + +/** + * A host followed by a path, query or fragment, which may be a single label. + * + * The two-character floor above exists only because a bare label at the end of + * a span has nothing to distinguish it from a word — but a label followed by + * `/`, `?` or `#` is not a word, it is an authority with a path. That evidence + * is structural rather than a length, so it can admit `@h/v1` without also + * admitting the `s` in `p@s s@host.example`, whose only delimiter is a space. + * + * Raising the floor instead would have traded one leak for another: at three + * characters, a two-character host leaks, and so on up. + */ +const DELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d][A-Za-z\d-]*(?:\.[A-Za-z\d-]+)*)(?::\d+)?[/?#]`; + +/** A host addressable after a userinfo, delimited or not. */ +const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST})`; /** * A `user:password@` prefix, where the password may contain spaces. @@ -286,9 +304,19 @@ const HOST_AFTER_USERINFO = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d](?:[A-Za- * greedy tail runs past the host into an `@` in trailing prose while a lazy one * stops at an `@` inside the password. So the `@` is chosen structurally — it * must be followed by something addressable as a host. + * + * The username may be empty. `https://:password@host` is a legal URL, and one + * that appears in configs where only a token is needed, so requiring a character + * before the colon left its password unstripped. + * + * Two shapes are knowingly left unstripped, because nothing in them says which + * reading is right: `p@ss word@host.example`, where the password's tail is a + * valid host, and `123 secret word@host`, where a second space before the `@` is + * the same evidence that tells a port from a password. Both are pinned by test + * 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})`, + String.raw`^[^\s/?#'"\`<>]*:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, ); /** From ce14a6d9844ba51996e13495eeb5b97f3d091db6 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 08:30:28 +0800 Subject: [PATCH 06/14] fix(cli): let a port and a non-ASCII name be evidence of a host 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. --- .../serve/workspace-providers-status.test.ts | 101 ++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 62 ++++++++--- 2 files changed, 149 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index b83b8aa2845..2428428b939 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -963,6 +963,107 @@ describe('createWorkspaceProvidersStatusProvider', () => { ); }); + it('keeps a single-character host that carries a port', async () => { + // A one-character host fell through every host shape: the bare-label branch + // needs two characters and the delimited branch needs a path, so `@h:8443` + // was not recognised and the `@` was taken from the contact line instead -- + // rewriting the host to `example.com` and deleting the prose. A port is the + // same structural evidence of an authority that a path is. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://user:pass@h:8443 — contact admin@example.com'; + 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( + 'Cannot reach https://h:8443 — contact admin@example.com', + ); + expect(JSON.stringify(result)).not.toContain('pass@'); + }); + + it('strips a spaced password whose first word begins with digits', async () => { + // `123abc` is not a port -- a port is digits and nothing else -- but the + // heuristic accepted any non-digit as the character after one, so a letter + // ended the "port" and the credentials were left in the message. PORT_END + // lists what actually closes a port instead. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:123abc secret@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('123abc'); + }); + + it('strips a spaced password whose first word contains a URL-legal symbol', async () => { + // The same defect through a different character: `%` and `_` are legal in a + // password, so "any non-digit ends the port" left `123%abc secret` in place. + // Guards against PORT_END being rewritten as a negated class. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:123%abc secret@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('123%abc'); + }); + + it('strips credentials before a non-ASCII host', async () => { + // An internationalized host arrives here un-punycoded when it comes from a + // config someone typed. The ASCII-only host classes did not recognise it, so + // the credential prefix was not found, the URL was cut at the space inside + // the password, and `user:my pass` was shown. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:my pass@münchen.example/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(JSON.stringify(result)).not.toContain('my pass'); + expect(result.errors?.[0]?.error).toContain('/v1'); + }); + + it('reads a port followed by one word ending in @ as a userinfo, and says so', async () => { + // The third documented tradeoff, and the only one that is not a regression: + // `:8443 support@example.com` is character-for-character a one-space + // password, and the deleted code produced this same output. Pinned because + // it is the case a future loosening of PORT_END would silently change. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443 support@example.com'; + 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('Cannot reach https://example.com'); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 64583b3a5f4..35b669643e3 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -249,16 +249,30 @@ function resolveApprovalMode(settings: Settings): ApprovalMode { const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; +/** + * What a host label is made of. + * + * Unicode rather than `[A-Za-z\d]`: an internationalized host reaches this code + * un-punycoded when it comes from a config file a person typed, and an + * ASCII-only class does not recognise it. Not recognising a host is what leaks a + * credential, so `münchen.de` had to be a host here for the same reason + * `example.com` does. + */ +const HOST_CHAR = String.raw`[\p{L}\p{N}\p{M}]`; +const LABEL_CHAR = String.raw`[\p{L}\p{N}\p{M}-]`; + /** * A host with nothing after it to mark where it ends, so its own shape is the * only evidence that it is one. * - * Three shapes, because a single pattern for all of them would be wrong at the + * Four shapes, because a single pattern for all of them would be wrong at the * edges. A bracketed IPv6 literal. A dotted name, whose first label may be a - * single character (`a.example`). Or a bare label — intranet proxies, container + * single character (`a.example`). A bare label — intranet proxies, container * names like `ollama`, and k8s service names have no dot — required to be at * least two characters, which is what keeps a one-letter fragment of a password - * from passing for a host. + * from passing for a host. Or a single character carrying a port, since `:8443` + * is structural evidence of an authority in the same way a path is, and without + * it `@h:8443` fell through every shape and the `@` was taken from later prose. * * What follows the host is required only to be something a host cannot continue * into, rather than a fixed list of delimiters. Enumerating them meant a URL @@ -267,7 +281,12 @@ const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; * 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-])`; +const UNDELIMITED_HOST = + String.raw`(?:\[[0-9A-Fa-f:.]+\]` + + String.raw`|${HOST_CHAR}(?:${LABEL_CHAR}*\.)+${LABEL_CHAR}+` + + String.raw`|${HOST_CHAR}${LABEL_CHAR}+` + + String.raw`|${HOST_CHAR}:\d+)` + + String.raw`(?::\d+)?(?!${LABEL_CHAR})`; /** * A host followed by a path, query or fragment, which may be a single label. @@ -281,11 +300,22 @@ const UNDELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d](?:[A-Za-z\d * Raising the floor instead would have traded one leak for another: at three * characters, a two-character host leaks, and so on up. */ -const DELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|[A-Za-z\d][A-Za-z\d-]*(?:\.[A-Za-z\d-]+)*)(?::\d+)?[/?#]`; +const DELIMITED_HOST = String.raw`(?:\[[0-9A-Fa-f:.]+\]|${HOST_CHAR}${LABEL_CHAR}*(?:\.${LABEL_CHAR}+)*)(?::\d+)?[/?#]`; /** A host addressable after a userinfo, delimited or not. */ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST})`; +/** + * How a port ends, and so what tells `:8443` from a password beginning `123`. + * + * A real port is only digits, so what follows a port is never a letter — which + * is the whole distinction. Written as an explicit closing set rather than + * "any non-digit", because a negated class also admits `_`, `=` and `%`, all + * 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`[,.;:!?)\]}]`; + /** * A `user:password@` prefix, where the password may contain spaces. * @@ -296,9 +326,8 @@ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST}) * rather than the start of a password. `:8443 ` alone is not enough to tell * the two apart — a password may also begin with digits and a space * (`user:123 secret@host`) — so the digits count as a port when they end the - * span, when a non-space follows them (sentence punctuation: `:8443, contact`), - * or when a further space follows before the `@`, which is prose rather than a - * one-space password. + * span, when a sentence closes on them (`:8443, contact`), or when a further + * space follows before the `@`, which is prose rather than a one-space password. * * Which `@` ends the userinfo: neither greediness setting is right, because a * greedy tail runs past the host into an `@` in trailing prose while a lazy one @@ -309,14 +338,19 @@ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST}) * that appears in configs where only a token is needed, so requiring a character * before the colon left its password unstripped. * - * Two shapes are knowingly left unstripped, because nothing in them says which - * reading is right: `p@ss word@host.example`, where the password's tail is a - * valid host, and `123 secret word@host`, where a second space before the `@` is - * the same evidence that tells a port from a password. Both are pinned by test - * so that widening either rule has to choose deliberately. + * Three shapes are knowingly left as they are, because nothing in them says + * which reading is right: `p@ss word@host.example`, where the password's tail is + * a valid host; `123 secret word@host`, where a second space before the `@` is + * the same evidence that tells a port from a password; and `:8443 support@x.com`, + * where a port is followed by exactly one word ending in `@`, which is + * indistinguishable from a one-space password. All three are pinned by test so + * that changing any rule has to choose deliberately. */ const CREDENTIAL_PREFIX_PATTERN = new RegExp( - String.raw`^[^\s/?#'"\`<>]*:(?!\d+(?:$|[^\s\d]|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, + // `\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<>]*:(?!\d+(?:$|${PORT_END}|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, + 'u', ); /** From c7976a32048de34ef68fd4ef20e3dde69f22d3e7 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 09:39:17 +0800 Subject: [PATCH 07/14] fix(cli): let a balanced quote around a URL end its port 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. --- .../serve/workspace-providers-status.test.ts | 70 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 45 +++++++++++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 2428428b939..a22f91b9aac 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1064,6 +1064,76 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(result.errors?.[0]?.error).toBe('Cannot reach https://example.com'); }); + it.each([ + ['"', '"'], + ["'", "'"], + ['`', '`'], + ['<', '>'], + ['(', ')'], + ['[', ']'], + ])( + 'keeps the host and the prose when a URL ending in a port is wrapped in %s%s', + async (open, close) => { + // A closing delimiter sits where the port heuristic expects a sentence + // character or a space, so the digits read as the start of a password, the + // `@` in the prose beyond was taken as the end of a userinfo, and both the + // real host and the contact text were deleted. + const message = `Cannot reach ${open}https://api.example:8443${close} — contact admin@example.com`; + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = message; + 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(message); + }, + ); + + it('still strips a credential from inside a quoted URL', async () => { + // The closing quote bounds the span; it must not stop the credential in the + // authority from being found, including a password containing a space. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach "https://user:123 secret@host.example/v1" — retry'; + 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( + 'Cannot reach "https://host.example/v1" — retry', + ); + expect(JSON.stringify(result)).not.toContain('secret'); + }); + + it.each(['"', "'", '('])( + 'reads %s in a password as a password character, not as a closing delimiter', + async (char) => { + // The fix must stay asymmetric: a delimiter only ends a port when the + // matching opener precedes the URL. Widening PORT_END to hold these + // characters instead would leave this password in the message. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = `Failed https://user:123${char}abc secret@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('secret'); + }, + ); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 35b669643e3..7ee727e5557 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -313,6 +313,11 @@ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST}) * "any non-digit", because a negated class also admits `_`, `=` and `%`, all * 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 + * `"https://api.example:8443"` puts one right after a port. That case is handled + * where the evidence is unambiguous — a *balanced* delimiter, in + * `findUrlSegmentEnd` — because `"` alone is as legal in a password as `%` is. */ const PORT_END = String.raw`[,.;:!?)\]}]`; @@ -345,6 +350,10 @@ const PORT_END = String.raw`[,.;:!?)\]}]`; * where a port is followed by exactly one word ending in `@`, which is * indistinguishable from a one-space password. All three are pinned by test so * that changing any rule has to choose deliberately. + * + * An unbalanced closer is a fourth: `:8443(ECONNREFUSED) — contact a@b.com` has + * no opening paren before the URL, so nothing distinguishes it from a password + * beginning `123(`. Wrapped in a balanced pair it is handled; bare it is not. */ const CREDENTIAL_PREFIX_PATTERN = new RegExp( // `\x60` is the backtick, spelled as an escape because a literal one cannot @@ -370,7 +379,12 @@ function sanitizeProviderWarning(warning: string): string { while (next) { result += warning.slice(index, next.index); - const segmentEnd = findUrlSegmentEnd(warning, next.index, next.marker); + const segmentEnd = findUrlSegmentEnd( + warning, + next.index, + next.marker, + warning.slice(0, next.index), + ); const segment = warning.slice(next.index, segmentEnd); const urlEnd = findUrlEnd(segment, next.marker.length); result += @@ -420,10 +434,33 @@ function findNextUrlStart( return match ? { index: match.index, marker: match[0] } : undefined; } +/** + * A quote that opened immediately before a URL, and so closes it. + * + * `"https://api.example:8443"` puts the closing quote right after the port, + * where the port heuristic expects a sentence character or a space and so reads + * the digits as the start of a password — matching the `@` in the prose beyond + * and deleting the real host. The closer is the evidence that the URL ended. + * + * Only a *balanced* delimiter counts, which is what makes this safe: adding + * these characters to `PORT_END` instead would let a password beginning `123"` + * read as a port, and `:123"abc secret@host` would keep its password. Here the + * same character is inert unless the matching opener sits before the URL. + */ +const URL_QUOTE_PAIRS = new Map([ + ['"', '"'], + ["'", "'"], + ['`', '`'], + ['<', '>'], + ['(', ')'], + ['[', ']'], +]); + function findUrlSegmentEnd( value: string, start: number, marker: string, + before: string, ): number { const afterMarker = start + marker.length; const carriageReturn = value.indexOf('\r', afterMarker); @@ -432,6 +469,12 @@ function findUrlSegmentEnd( if (carriageReturn !== -1) lineEnd = Math.min(lineEnd, carriageReturn); if (lineFeed !== -1) lineEnd = Math.min(lineEnd, lineFeed); + 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); + } + const nextUrl = findNextUrlStart(value, afterMarker); return Math.min(lineEnd, nextUrl?.index ?? value.length); From f43af06e591b58054528f9632e7ab31fe06a2c0e Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 11:23:51 +0800 Subject: [PATCH 08/14] fix(cli): require a port before a balanced quote ends a URL span MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../serve/workspace-providers-status.test.ts | 74 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 34 +++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index a22f91b9aac..5aef700dec9 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1134,6 +1134,80 @@ describe('createWorkspaceProvidersStatusProvider', () => { }, ); + it.each([ + ['"', '"'], + ["'", "'"], + ['`', '`'], + ['<', '>'], + ['(', ')'], + ['[', ']'], + ])( + 'strips a password containing %s%s from a URL wrapped in the same pair', + async (open, close) => { + // The closing character is legal in a password, so the first one in the + // span may be the credential's rather than the URL's. Ending the span + // there leaves no `@` behind it, nothing is recognised as a userinfo, and + // the password stays in the message — so a closer only ends a span when it + // is the last one on the line and a port sits immediately before it. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = `Cannot reach ${open}https://user:pa${close}ss@host.example/v1${close} — retry`; + 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( + `Cannot reach ${open}https://host.example/v1${close} — retry`, + ); + expect(JSON.stringify(result)).not.toContain(`pa${close}ss`); + }, + ); + + it('strips a quoted URL whose password itself looks like a port', async () => { + // `:8443"` is the exact shape the delimiter rule looks for, but here it is a + // password, not a port. The port must be what the closer *closes on* — it is + // measured against the span the closer would cut, so digits that merely + // start a password do not qualify. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach "https://user:8443"abc secret@host.example/v1" — retry'; + 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( + 'Cannot reach "https://host.example/v1" — retry', + ); + expect(JSON.stringify(result)).not.toContain('secret'); + }); + + it('strips a quoted URL whose closing quote is on a later line', async () => { + // The span still ends at the newline, so the only closer available is the + // one inside the password. Taking it would end the span mid-credential. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach "https://user:pa"ss@host.example/v1\nsee the log" — retry'; + 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( + 'Cannot reach "https://host.example/v1\nsee the log" — retry', + ); + expect(JSON.stringify(result)).not.toContain('pa"ss'); + }); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 7ee727e5557..aa5995e9cd8 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -446,6 +446,16 @@ function findNextUrlStart( * these characters to `PORT_END` instead would let a password beginning `123"` * read as a port, and `:123"abc secret@host` would keep its password. Here the * same character is inert unless the matching opener sits before the URL. + * + * Balance alone is still not enough, because the same character is legal in a + * password: in `"https://user:pa"ss@host.example/v1"` the first closer sits + * inside the credential, and cutting there leaves no `@` in the span, so nothing + * is stripped and the password survives in full. Which is why a closer must + * carry two signals, in `findUrlSegmentEnd` — it must be the *last* one within + * the span, and a port must sit immediately before it. Either test alone leaks: + * without the port, `:8443"abc secret@host` cuts at the closer inside the + * password; without `lastIndexOf`, a closer in the password ends the span + * whenever no later one exists on the line. */ const URL_QUOTE_PAIRS = new Map([ ['"', '"'], @@ -456,6 +466,16 @@ const URL_QUOTE_PAIRS = new Map([ ['[', ']'], ]); +/** + * A port sitting at the end of what the closer would cut. + * + * This is the second half of the evidence, and it is what keeps a closer that + * happens to fall inside a password from ending the span: the reason a balanced + * delimiter matters at all is that the port heuristic cannot see past it, so the + * closer only carries information when a port is what precedes it. + */ +const PORT_AT_END = /:\d+$/; + function findUrlSegmentEnd( value: string, start: number, @@ -469,15 +489,19 @@ function findUrlSegmentEnd( if (carriageReturn !== -1) lineEnd = Math.min(lineEnd, carriageReturn); if (lineFeed !== -1) lineEnd = Math.min(lineEnd, lineFeed); + 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.indexOf(closer, afterMarker); - if (closed !== -1) lineEnd = Math.min(lineEnd, closed); + 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; + } } - const nextUrl = findNextUrlStart(value, afterMarker); - - return Math.min(lineEnd, nextUrl?.index ?? value.length); + return bound; } function buildCurrent( From 8ab9eefb4dea7d7df9c938e1ca1a34f45bc45bb0 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 13:13:59 +0800 Subject: [PATCH 09/14] test(cli): pin the shared host grammar the free-text sanitizer depends 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. --- .../serve/workspace-providers-status.test.ts | 30 +++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 6 ++++ packages/cli/src/utils/acpModelUtils.ts | 13 ++++++++ 3 files changed, 49 insertions(+) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 5aef700dec9..23c0bbc3484 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1208,6 +1208,36 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('pa"ss'); }); + it.each([ + ['?k=v', 'query'], + ['#f', 'fragment'], + ])( + 'keeps the host when a pathless URL carries credentials and its %s holds an @', + async (tail) => { + // The counterpart to the prose-email case above, and the one that pins + // this file's notion of "host" to `sanitizeProviderBaseUrl`'s. A pathless + // URL is bounded by `?` or `#` as well as by `/`; drop either from + // `findAuthorityEnd` in utils/acpModelUtils.ts and the later `@` becomes + // the userinfo terminator, rewriting the host to `evil.com`. The + // credentials are still stripped either way, so only an assertion on the + // surviving host catches it. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = `Cannot reach https://user:secret@host.example:8443${tail}@evil.com — retry`; + 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( + `Cannot reach https://host.example:8443${tail}@evil.com — retry`, + ); + expect(JSON.stringify(result)).not.toContain('secret'); + }, + ); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index aa5995e9cd8..731cf1e9c73 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -354,6 +354,12 @@ const PORT_END = String.raw`[,.;:!?)\]}]`; * An unbalanced closer is a fourth: `:8443(ECONNREFUSED) — contact a@b.com` has * no opening paren before the URL, so nothing distinguishes it from a password * beginning `123(`. Wrapped in a balanced pair it is handled; bare it is not. + * + * This grammar decides only how much of the message is URL; the slice is then + * handed to `sanitizeProviderBaseUrl` in `utils/acpModelUtils.ts`, which locates + * the userinfo within it. The two must agree on where an authority ends, and + * neither can see the other — see that function's comment for the host rewrite + * that a disagreement produces. */ const CREDENTIAL_PREFIX_PATTERN = new RegExp( // `\x60` is the backtick, spelled as an escape because a literal one cannot diff --git a/packages/cli/src/utils/acpModelUtils.ts b/packages/cli/src/utils/acpModelUtils.ts index de14c6f0aad..f130512be21 100644 --- a/packages/cli/src/utils/acpModelUtils.ts +++ b/packages/cli/src/utils/acpModelUtils.ts @@ -161,6 +161,19 @@ export function getCurrentAcpModelId( : formatAcpModelId(modelId, authType); } +/** + * Strips the userinfo from a URL, scoping the search to the authority. + * + * `serve/workspace-providers-status.ts` re-implements a parallel host grammar + * (`CREDENTIAL_PREFIX_PATTERN` / `HOST_AFTER_USERINFO`) to decide how much of a + * free-text message is URL, then feeds that slice here — so where this function + * 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 + * `holds an @` cases in `serve/workspace-providers-status.test.ts` are what + * catch it. + */ export function sanitizeProviderBaseUrl(baseUrl: string): string { const scheme = baseUrl.match(/^[A-Za-z][A-Za-z\d+.-]*:\/\//); if (!scheme) { From 9ae36aced0f76804c06da89f3be63ec8a3f04f8e Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 15:48:55 +0800 Subject: [PATCH 10/14] fix(cli): strip a spaced password before a one-character host `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. --- .../serve/workspace-providers-status.test.ts | 83 ++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 86 ++++++++++++++++++- 2 files changed, 165 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 23c0bbc3484..517006bdc47 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1025,6 +1025,89 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('123%abc'); }); + it('strips a spaced password before a bare one-character host', async () => { + // `HOST_AFTER_USERINFO` requires two characters of a bare label, so `@h` + // matched no shape, `CREDENTIAL_PREFIX_PATTERN` declined, and the fallback + // cut at the first space — inside the password. The slice handed to + // `sanitizeProviderBaseUrl` then had no `@` at all, so nothing was stripped + // and `user:my pass` was re-appended as prose. One-char hosts are real: + // container names and `/etc/hosts` aliases are often a single letter. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:my pass@h retry later'; + 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 https://h retry later'); + expect(JSON.stringify(result)).not.toContain('my pass'); + }); + + it('strips a spaced password before a bare one-character host with an empty username', async () => { + // `https://:token@host` is a legal URL and appears in configs where only a + // token is needed. Pinned alongside the case above because the fallback + // pattern allows an empty username for the same reason the primary one does. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = 'Failed https://:my pass@h retry later'; + 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 https://h retry later'); + expect(JSON.stringify(result)).not.toContain('my pass'); + }); + + it('does not let the one-character fallback host swallow a password fragment', async () => { + // The boundary of the fallback, and the reason the two-character floor + // exists in the primary pattern: in `p@s s@host.example` the `s` after the + // first `@` is a single character, so admitting one-char hosts everywhere + // would cut there and leave ` s@host.example` as the host. The primary + // pattern matches this span, so the fallback is never consulted — this test + // fails if that ordering is ever inverted. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:p@s s@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('p@s s'); + }); + + it('leaves a message with no credentials alone when a one-char word precedes an email', async () => { + // The fallback drops the two-character host floor, so it must not turn a + // port plus prose into a userinfo. Without the port lookahead being kept in + // the fallback, `:8443 — contact a@b` would cut at the email's `@` and + // rewrite the host. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443 — contact a@b'; + 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( + 'Cannot reach https://api.example:8443 — contact a@b', + ); + }); + it('strips credentials before a non-ASCII host', async () => { // An internationalized host arrives here un-punycoded when it comes from a // config someone typed. The ASCII-only host classes did not recognise it, so diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 731cf1e9c73..f29e7cbd561 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -321,6 +321,27 @@ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST}) */ const PORT_END = String.raw`[,.;:!?)\]}]`; +/** + * Digits after a colon that are a port rather than the start of a password. + * + * Three shapes qualify, and each is a different kind of evidence: the digits end + * the span, a sentence closes on them (`:8443, contact`), or a further space + * follows before any `@`, which is prose rather than a one-space password. + * + * The sentence-closing shape needs the same "and no userinfo follows" test the + * space shape makes, because `PORT_END` on its own does not tell `:8443. See + * admin@x` from `:123. secret@host.example/v1`. Without it the latter read as a + * port, so no credential prefix was found at all and `findUrlEnd` cut at the + * first space — inside the password — putting `user:123. secret@` in the + * `/status` payload. `CREDENTIAL_FALLBACK_PATTERN` cannot cover this one: its + * host is a single character by construction, and the host here is + * `host.example`. + * + * The test is scoped to the rest of the span rather than to the token, since the + * `@` that resolves the ambiguity is in the word after the space. + */ +const PORT_DIGITS = String.raw`\d+(?:$|${PORT_END}(?![^\s]*\s[^@\s]*@)|\s(?:[^@\s]*\s))`; + /** * A `user:password@` prefix, where the password may contain spaces. * @@ -364,7 +385,50 @@ const PORT_END = String.raw`[,.;:!?)\]}]`; 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<>]*:(?!\d+(?:$|${PORT_END}|\s(?:[^@\s]*\s)))[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, + String.raw`^[^\s/?#'"\x60<>]*:(?!${PORT_DIGITS})[^/?#]*?@(?=${HOST_AFTER_USERINFO})`, + 'u', +); + +/** + * A bare one-character host, admitted only by the fallback. + * + * `UNDELIMITED_HOST` requires two characters of a bare label, because at the end + * of a span a single letter is indistinguishable from a fragment of a password — + * the `s` in `p@s s@host.example`. That floor is right for the primary pattern, + * where a wrong guess rewrites a host. It is wrong for the fallback, where the + * alternative is not "leave the host alone" but "leave the password in", because + * the fallback only runs once the first-space cut is known to land inside a + * credential. + */ +const FALLBACK_HOST = String.raw`${HOST_CHAR}(?!${LABEL_CHAR})`; + +/** + * A `user:password@` prefix recognised on weaker evidence, used only when + * `CREDENTIAL_PREFIX_PATTERN` has already declined. + * + * It differs 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 + * to strip and cutting at the email's `@` would rewrite the host. + * + * Widening the host rule is safe here in a way it is not in the primary pattern, + * because the two are consulted in different states. There, a one-character + * "host" that is really a password fragment rewrites a host that was fine. Here, + * the primary pattern has already declined, so `findUrlEnd`'s other branch is + * about to cut at the first space — which, when a colon-then-`@` is present, is + * *inside* the password by construction. The slice then reaching + * `sanitizeProviderBaseUrl` has no `@` at all, so nothing is stripped and the + * credential is re-appended verbatim as prose. Between rewriting a host and + * emitting a password, the choice is not close. + * + * That asymmetry is the general point, and it is why this is a second pattern + * rather than a third alternative in `HOST_AFTER_USERINFO`: a sanitizer should + * resolve an ambiguity it cannot settle by removing more, not less, but only + * where the cost of guessing wrong is a less informative message rather than a + * different host. + */ +const CREDENTIAL_FALLBACK_PATTERN = new RegExp( + String.raw`^[^\s/?#'"\x60<>]*:(?!${PORT_DIGITS})[^/?#]*?@(?=${FALLBACK_HOST})`, 'u', ); @@ -419,11 +483,25 @@ function sanitizeProviderWarning(warning: string): string { * authority it computes and rewrite the host from the prose — turning * `https://user:pass@host.com — contact admin@example.com` into * `https://example.com`. + * + * When the prefix is *not* recognised, cutting at the first space is the unsafe + * direction, and that is the route every leak this function has had takes: the + * grammar declines a `user:…@` that is really there, the cut lands inside the + * password, the slice handed to `sanitizeProviderBaseUrl` contains no `@` at all, + * and the credential is re-appended verbatim as prose. + * + * `CREDENTIAL_FALLBACK_PATTERN` closes that route for one spelling — a bare + * one-character host — and not for the shapes where a port and a password are + * genuinely indistinguishable, which stay as `CREDENTIAL_PREFIX_PATTERN` + * documents them. */ function findUrlEnd(segment: string, markerLength: number): number { - const credentials = CREDENTIAL_PREFIX_PATTERN.exec( - segment.slice(markerLength), - ); + const body = segment.slice(markerLength); + const credentials = CREDENTIAL_PREFIX_PATTERN.exec(body); + if (!credentials) { + const fallback = CREDENTIAL_FALLBACK_PATTERN.exec(body); + if (fallback) return markerLength + fallback[0].length; + } const from = credentials ? markerLength + credentials[0].length : markerLength; From 7d6336a8035295f8157ee8795d633a4322603f22 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 16:02:41 +0800 Subject: [PATCH 11/14] test(cli): pin the sentence-closing port digits both ways 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. --- .../serve/workspace-providers-status.test.ts | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 517006bdc47..46aa369f86c 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1025,6 +1025,50 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('123%abc'); }); + it('strips a spaced password whose first word is digits closing a sentence', async () => { + // `123.` is digits followed by a `PORT_END` character, which is exactly how a + // port ending a sentence looks (`:8443. See ...`), so the colon was rejected + // as a port, no credential prefix was found, and `findUrlEnd` cut at the + // first space — inside the password. What separates the two is whether a + // userinfo `@` follows: a port that ends a sentence is not followed by one. + // The fallback pattern cannot cover this case, because its host is a single + // character by construction and the host here is `host.example`. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed https://user:123. secret@host.example/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 https://host.example/v1'); + expect(JSON.stringify(result)).not.toContain('secret'); + }); + + it('still reads a port that closes a sentence as a port', async () => { + // The other side of the rule above, and the reason it tests for a following + // `@` rather than dropping `PORT_END` from the lookahead: with no credential + // in the message, treating `8443` as a password start would take the `@` from + // the contact line and rewrite the host to `example.com`. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443. Contact admin@example.com'; + 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( + 'Cannot reach https://api.example:8443. Contact admin@example.com', + ); + }); + it('strips a spaced password before a bare one-character host', async () => { // `HOST_AFTER_USERINFO` requires two characters of a bare label, so `@h` // matched no shape, `CREDENTIAL_PREFIX_PATTERN` declined, and the fallback From 68fe634bafa2b5f2f32a86db90890a346e160cdd Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 16:13:05 +0800 Subject: [PATCH 12/14] fix(cli): consult the fallback credential pattern even when the primary matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../serve/workspace-providers-status.test.ts | 24 +++++++++++ .../src/serve/workspace-providers-status.ts | 40 +++++++++++++++++-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 46aa369f86c..9abcd4414e2 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1025,6 +1025,30 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('123%abc'); }); + it('keeps a one-character host when prose after it contains an @', async () => { + // The other half of the one-character-host defect, and it needs a different + // fix: here `CREDENTIAL_PREFIX_PATTERN` does not decline, it matches at the + // *wrong* `@`. A one-char host is invisible to `HOST_AFTER_USERINFO`, so the + // lazy tail runs past it to the email's `@` -- the credential is stripped, + // but the host becomes `example.com` and the contact prose is deleted. So the + // fallback has to be consulted even when the primary pattern succeeds. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://user:pass@h — contact admin@example.com'; + 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( + 'Cannot reach https://h — contact admin@example.com', + ); + expect(JSON.stringify(result)).not.toContain('pass@'); + }); + it('strips a spaced password whose first word is digits closing a sentence', async () => { // `123.` is digits followed by a `PORT_END` character, which is exactly how a // port ending a sentence looks (`:8443. See ...`), so the colon was rejected diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index f29e7cbd561..e487d22bff0 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -494,13 +494,31 @@ function sanitizeProviderWarning(warning: string): string { * one-character host — and not for the shapes where a port and a password are * genuinely indistinguishable, which stay as `CREDENTIAL_PREFIX_PATTERN` * documents them. + * + * The fallback is also consulted when the primary pattern *succeeds*, because + * succeeding is not the same as being right. A one-character host is invisible to + * `HOST_AFTER_USERINFO`, so in `user:pass@h — contact admin@example.com` the lazy + * tail runs past it and matches at the email's `@` instead: the credential is + * still stripped, but the host is rewritten from `h` to `example.com` and the + * prose is deleted. The fallback finds the right `@`; it just has to be asked. + * + * Which of the two to believe is decided by how much whitespace separates them. + * Two or more spaces is prose — the primary reached out of the URL to find a host + * — so 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 (`p@s s@host.example`, pinned by test), so the primary keeps it. + * That is the same "a second space means prose" test `PORT_DIGITS` makes, applied + * to the other ambiguity. */ function findUrlEnd(segment: string, markerLength: number): number { const body = segment.slice(markerLength); const credentials = CREDENTIAL_PREFIX_PATTERN.exec(body); - if (!credentials) { - const fallback = CREDENTIAL_FALLBACK_PATTERN.exec(body); - if (fallback) return markerLength + fallback[0].length; + const fallback = CREDENTIAL_FALLBACK_PATTERN.exec(body); + if ( + fallback && + (!credentials || prefersFallback(body, credentials, fallback)) + ) { + return markerLength + fallback[0].length; } const from = credentials ? markerLength + credentials[0].length @@ -509,6 +527,22 @@ function findUrlEnd(segment: string, markerLength: number): number { return space === -1 ? segment.length : from + space; } +/** + * Whether the fallback's earlier `@` is the better of two matches. + * + * Only when it is genuinely earlier, and only when what lies between the two is + * prose rather than a password: two spaces or more. See `findUrlEnd`. + */ +function prefersFallback( + body: string, + credentials: RegExpExecArray, + fallback: RegExpExecArray, +): boolean { + 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; +} + function findNextUrlStart( value: string, from: number, From f5c513e34963326b1a8f512b8e0c3cbc96e2dffc Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Fri, 31 Jul 2026 16:41:19 +0800 Subject: [PATCH 13/14] fix(cli): look past a later quoted span for the one that closes the URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../serve/workspace-providers-status.test.ts | 24 +++++++++ .../src/serve/workspace-providers-status.ts | 54 +++++++++++++++++-- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index 9abcd4414e2..ff2523a49da 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1339,6 +1339,30 @@ describe('createWorkspaceProvidersStatusProvider', () => { expect(JSON.stringify(result)).not.toContain('secret'); }); + it('keeps the host when a quoted email follows a quoted credentialed URL', async () => { + // `lastIndexOf` finds the *email's* closing quote, not the URL's. The text + // between then fails the port test, the closer carries nothing, and the span + // runs to end-of-line, where a later `:` becomes the username and the prose + // `@` its terminator — rewriting the host and deleting the contact text. The + // credential is stripped either way, so only asserting on the surviving host + // catches this. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Error at "https://admin:s3cr3t@internal:8443" - email "ops@company.com" for help'; + 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( + 'Error at "https://internal:8443" - email "ops@company.com" for help', + ); + expect(JSON.stringify(result)).not.toContain('s3cr3t'); + }); + it('strips a quoted URL whose closing quote is on a later line', async () => { // The span still ends at the newline, so the only closer available is the // one inside the password. Taking it would end the span mid-credential. diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index e487d22bff0..6441674d265 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -574,6 +574,11 @@ function findNextUrlStart( * without the port, `:8443"abc secret@host` cuts at the closer inside the * password; without `lastIndexOf`, a closer in the password ends the span * whenever no later one exists on the line. + * + * A closer to the left of the last one can still be this URL's, when a second + * quoted span follows on the same line. That case is admitted separately, under + * the stricter test in `CREDENTIALED_AUTHORITY_AT_END` — the two signals above + * are what a *last* closer must carry, not what any closer must. */ const URL_QUOTE_PAIRS = new Map([ ['"', '"'], @@ -594,6 +599,34 @@ const URL_QUOTE_PAIRS = new Map([ */ const PORT_AT_END = /:\d+$/; +/** + * A whole credentialed authority ending in a port: `userinfo@host:port`. + * + * The `lastIndexOf` above is the *last* closer on the line, which is the wrong + * one as soon as a second quoted span follows — a quoted email being the case + * that occurs. Its span then holds an `@`, `PORT_AT_END` fails on the text + * between, and the closer carries no information, so the span runs to + * end-of-line and a later `:` becomes the username. + * + * Looking further left needs a stricter test than `PORT_AT_END` and the absence + * of an `@`, because the credentialed URL's own span contains an `@` and a + * password can end in `:digits` — `user:8443` is character-for-character a + * `host:port`. Requiring the userinfo *and* the host separates them: `user:8443` + * has no `@` at all, so only a span that is a complete authority qualifies. + * + * Which leaves the uncredentialed `"https://internal:8443" … "ops@company.com"` + * still wrong, as it is on base. There is no signal to separate its + * `internal:8443` from the pinned `user:8443`: identical shapes, and what + * follows the closer is prose in one case and the rest of a credential in the + * other. Widening this to accept a bare `host:port` fixes that message and + * leaves the password in the pinned one, which is the worse trade. + */ +const CREDENTIALED_AUTHORITY_AT_END = new RegExp( + String.raw`^[^\s/?#'"\x60<>]*:[^/?#]*?@` + + String.raw`${HOST_CHAR}(?:[\p{L}\p{N}\p{M}.-]*${HOST_CHAR})?:\d+$`, + 'u', +); + function findUrlSegmentEnd( value: string, start: number, @@ -612,10 +645,23 @@ function findUrlSegmentEnd( 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; + const last = value.lastIndexOf(closer, bound - 1); + if (last >= afterMarker) { + const upTo = value.slice(afterMarker, last); + if (PORT_AT_END.test(upTo) && !upTo.includes('@')) return last; + + // The last closer was not this URL's — a second quoted span follows. Walk + // left, but only a complete `userinfo@host:port` may end the span here: a + // closer inside a password reaches this walk too, and `PORT_AT_END` alone + // would cut at it. + let at = last - 1; + while (at >= afterMarker) { + const closed = value.lastIndexOf(closer, at); + if (closed < afterMarker) break; + const span = value.slice(afterMarker, closed); + if (CREDENTIALED_AUTHORITY_AT_END.test(span)) return closed; + at = closed - 1; + } } } From b3db8ed93a0d3c30c1b92d520983f59d4bd2ba80 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Sun, 2 Aug 2026 22:16:08 +0800 Subject: [PATCH 14/14] fix(cli): recognise the hosts and passwords that leaked a credential 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. --- .../serve/workspace-providers-status.test.ts | 133 ++++++++++++++++++ .../src/serve/workspace-providers-status.ts | 106 ++++++++++++-- packages/cli/src/utils/acpModelUtils.test.ts | 23 +++ 3 files changed, 251 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/serve/workspace-providers-status.test.ts b/packages/cli/src/serve/workspace-providers-status.test.ts index ff2523a49da..8fb8bf061cb 100644 --- a/packages/cli/src/serve/workspace-providers-status.test.ts +++ b/packages/cli/src/serve/workspace-providers-status.test.ts @@ -1413,6 +1413,139 @@ describe('createWorkspaceProvidersStatusProvider', () => { }, ); + it.each([ + ['_host', 'an underscore'], + ['_svc.local', 'an underscore and a dot'], + ['-host', 'a leading dash'], + ['.host', 'a leading dot'], + ])( + 'strips a spaced password before a host starting with %s', + async (host) => { + // A host whose first character is one a *valid* host cannot start with was + // matched by neither credential pattern, so the URL was cut at the space + // inside the password and `user:my pass` was emitted as prose. `_host` was + // already leaking before this PR; `-host` and `.host` are shapes the + // replaced code stripped, because it tested one character class rather than + // matching a host grammar. Both directions are covered here so the grammar + // cannot narrow back to only what is addressable. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = `Failed loading provider https://user:my pass@${host}/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://${host}/v1`, + ); + expect(JSON.stringify(result)).not.toContain('my pass'); + }, + ); + + it('strips a spaced password containing a slash', async () => { + // A slash is legal in a password, but both primary patterns spell the + // userinfo tail `[^/?#]*?@` and so declined the prefix: the cut landed at the + // space inside the password and `b/c@host.example/v1` came back as prose. + // `SLASHED_PASSWORD_PATTERN` crosses the slash, gated on a space appearing + // before the `@` — which is the precondition for the leak and the one thing a + // path cannot contain. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Failed loading provider https://user:a b/c@host.example/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://host.example/v1', + ); + expect(JSON.stringify(result)).not.toContain('a b/c'); + }); + + it('keeps a host whose path holds an @ when no credential is present', async () => { + // The cost of crossing a slash, and why the crossing is gated. A path may + // legally contain an `@`; taking it as the end of a userinfo would rewrite the + // host from the text beyond it. The space gate is what excludes this — a space + // ends the URL, so a path cannot hold one — and without it this reports + // `https://thing`. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://host.example/path@thing — retry'; + 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( + 'Cannot reach https://host.example/path@thing — retry', + ); + }); + + it('keeps a port with a path and the email after it intact', async () => { + // The other cost of crossing a slash. `PORT_DIGITS` does not treat a slash as + // ending a port, because until the slashed-password pattern existed nothing + // could cross one and `:8443/v1` was unreachable. Reachable, it reads + // `8443/v1 contact admin@` as a userinfo and reports the host as `example.com` + // — the failure this file works hardest to avoid — so that pattern excludes + // `\d+[/?#]` itself rather than loosening `PORT_DIGITS` for the other two. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = + 'Cannot reach https://api.example:8443/v1 contact admin@example.com'; + 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( + 'Cannot reach https://api.example:8443/v1 contact admin@example.com', + ); + }); + + it.each([ + ['123"abc secret word', 'a second space before the @'], + ['123"ab c secret', 'a space inside the first word'], + ])( + 'strips a password whose digits are followed by a quote and %s', + async (password) => { + // Why `PORT_END` holds `)`, `]` and `}` but not `"` or `'`. The brackets are + // carried by the `no userinfo follows` lookahead in `PORT_DIGITS`, not by + // their own shape, so adding quotes for symmetry looks safe and passes every + // other test here. These two spellings are where the lookahead does not + // fire: with quotes in the set the digits read as a port, no prefix is + // found, and the password is emitted. Nothing else in this file catches it. + coreMock.throwModelsConfigError = true; + coreMock.modelsConfigErrorMessage = `Failed loading provider https://user:${password}@host.example/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); + + // Asserted on the message rather than on `JSON.stringify(result)`, which + // every other test here can use: the password holds a `"`, which stringify + // escapes, so a `not.toContain` on the dump passes whatever the sanitizer + // did and pins nothing. + expect(result.errors?.[0]?.error).toBe( + 'Failed loading provider https://host.example/v1', + ); + }, + ); + async function writeUserSettings(settings: Record) { await fs.writeFile( path.join(qwenHome, 'settings.json'), diff --git a/packages/cli/src/serve/workspace-providers-status.ts b/packages/cli/src/serve/workspace-providers-status.ts index 6441674d265..9e37ab1bd4b 100644 --- a/packages/cli/src/serve/workspace-providers-status.ts +++ b/packages/cli/src/serve/workspace-providers-status.ts @@ -257,9 +257,24 @@ const URL_START_PATTERN = /\b[A-Za-z][A-Za-z\d+.-]*:\/\//g; * ASCII-only class does not recognise it. Not recognising a host is what leaks a * credential, so `münchen.de` had to be a host here for the same reason * `example.com` does. + * + * `HOST_CHAR` admits the characters a *valid* host cannot start with — `_`, `-` + * and `.` — for that same reason, which is the one place this grammar must not + * be a validator. Rejecting `@_host` did not make the URL invalid, it made the + * authority unrecognised: no credential prefix matched, `findUrlEnd` cut at the + * first space *inside* the password instead, and `sanitizeProviderBaseUrl` was + * handed an `@`-less slice it returned verbatim, so the whole credential landed + * in the `/status` payload. `_` is legal in practice besides — container and k8s + * service names use it, and this grammar exists to cover exactly those bare + * intranet labels. + * + * Being liberal here cannot leak in the other direction: what a match does is + * *strip* the userinfo ahead of it, so admitting too much removes text from the + * payload rather than adding it. The tighter guards that keep prose from reading + * as a host are the two-character floor and the delimiter evidence below. */ -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}_-]`; /** * A host with nothing after it to mark where it ends, so its own shape is the @@ -314,10 +329,24 @@ const HOST_AFTER_USERINFO = String.raw`(?:${UNDELIMITED_HOST}|${DELIMITED_HOST}) * 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 - * `"https://api.example:8443"` puts one right after a port. That case is handled - * where the evidence is unambiguous — a *balanced* delimiter, in - * `findUrlSegmentEnd` — because `"` alone is as legal in a password as `%` is. + * The closing brackets `)`, `]` and `}` are in the set even though they are as + * legal in a password as `%` is, and what makes them safe is not their own shape + * but the lookahead that follows in `PORT_DIGITS`: `:123)abc secret@host` still + * finds its prefix, because a `@` later in the span rejects the port reading. A + * bracket therefore only ends a port where there is no credential to keep. + * + * Quotes are absent, and that is the one place the set is not interchangeable + * with a wider one. `"https://api.example:8443"` puts a quote right after a port, + * so adding `"` and `'` looks symmetric with the brackets and passes every test + * in this file — but the lookahead is what carries the brackets, and a quote can + * appear where the lookahead does not fire. `:123"abc secret word@host` has a + * second space before the `@`, and `:123"ab c secret@host` has one inside the + * first word; both fail the `[^@\s]*\s[^@\s]*@` shape, so with quotes in the set + * the digits read as a port, no prefix is found, and the password is emitted. + * Measured on both spellings before writing this down. + * + * The quoted-port case is handled where the evidence is unambiguous instead — a + * *balanced* delimiter, in `findUrlSegmentEnd`. */ const PORT_END = String.raw`[,.;:!?)\]}]`; @@ -432,6 +461,51 @@ const CREDENTIAL_FALLBACK_PATTERN = new RegExp( 'u', ); +/** + * A `user:password@` prefix whose password contains a slash. + * + * Both patterns above spell the userinfo tail `[^/?#]*?@`, which cannot cross a + * `/` — and it must not, in general, because a path may legally contain an `@`: + * crossing one in `https://host.example/path@thing` would take the `@` from the + * path and rewrite the host from the text beyond it. A slash is genuinely a + * stronger authority delimiter than a space. + * + * But a slash is also legal in a password, and there the same class is what + * declines the prefix: `https://user:a b/c@host.example/v1` matched neither + * pattern, so `findUrlEnd` cut at the first space — inside the password — and the + * `@`-less slice `https://user:a` came back from `sanitizeProviderBaseUrl` + * verbatim with `b/c@host.example/v1` re-appended as prose. This is the same leak + * route `findUrlEnd` documents, reached through the one delimiter the other two + * patterns treat as absolute. + * + * Two gates are what make crossing it safe here, and each rules out one half of + * the ambiguity: + * + * A space must precede the `@`. That is not a heuristic about passwords, it is + * the precondition for the leak: with no space before it, the first-space cut + * cannot land inside the credential, so there is nothing to fix and no reason to + * guess. It is also what a path cannot have — a space ends the URL — so the + * `path@thing` case above is excluded rather than traded away. + * + * The port lookahead gains `\d+[/?#]`. `PORT_DIGITS` does not treat a slash as + * ending a port, because until now nothing could cross one, and `:8443/v1` was + * therefore unreachable. Crossing makes it reachable, and it is the common + * spelling of a base URL: without this, `provider https://api.example:8443/v1 + * contact admin@example.com` reads `8443/v1 contact admin@` as a userinfo and + * reports the host as `example.com`, deleting the prose. Rewriting a host is the + * failure this file works hardest to avoid, so the exclusion belongs here rather + * than in `PORT_DIGITS`, where it would loosen the other two patterns for a case + * they cannot reach. + * + * Consulted only when both patterns above have declined, so it can neither + * override a match nor change which `@` a recognised prefix ends at. + */ +const SLASHED_PASSWORD_PATTERN = new RegExp( + String.raw`^[^\s/?#'"\x60<>]*:(?!${PORT_DIGITS}|\d+[/?#])` + + String.raw`(?=[^?#@]*\s)[^?#]*?@(?=${HOST_AFTER_USERINFO})`, + 'u', +); + /** * Strips credentials from every URL in a free-text warning or error message. * @@ -491,10 +565,16 @@ function sanitizeProviderWarning(warning: string): string { * and the credential is re-appended verbatim as prose. * * `CREDENTIAL_FALLBACK_PATTERN` closes that route for one spelling — a bare - * one-character host — and not for the shapes where a port and a password are + * one-character host — and `SLASHED_PASSWORD_PATTERN` for another, a password + * containing a slash. Neither closes the shapes where a port and a password are * genuinely indistinguishable, which stay as `CREDENTIAL_PREFIX_PATTERN` * documents them. * + * The three are consulted in decreasing order of evidence, and only the first two + * compete: the slashed-password pattern runs last because crossing a `/` is the + * weakest inference of the three, and it must not be able to move an `@` that a + * better-evidenced pattern already found. + * * The fallback is also consulted when the primary pattern *succeeds*, because * succeeding is not the same as being right. A one-character host is invisible to * `HOST_AFTER_USERINFO`, so in `user:pass@h — contact admin@example.com` the lazy @@ -520,9 +600,8 @@ function findUrlEnd(segment: string, markerLength: number): number { ) { return markerLength + fallback[0].length; } - const from = credentials - ? markerLength + credentials[0].length - : markerLength; + const prefix = credentials ?? SLASHED_PASSWORD_PATTERN.exec(body); + const from = prefix ? markerLength + prefix[0].length : markerLength; const space = segment.slice(from).search(/\s/); return space === -1 ? segment.length : from + space; } @@ -620,10 +699,15 @@ const PORT_AT_END = /:\d+$/; * follows the closer is prose in one case and the rest of a credential in the * other. Widening this to accept a bare `host:port` fixes that message and * leaves the password in the pinned one, which is the worse trade. + * + * The host middle is composed from `LABEL_CHAR` plus `.` rather than spelled + * out, so that widening the shared classes reaches here too. Spelled inline it + * did not: `HOST_CHAR` gained `_` and this pattern kept rejecting `@_host:8443`, + * which is the divergence that leaks a credential rather than a cosmetic one. */ const CREDENTIALED_AUTHORITY_AT_END = new RegExp( String.raw`^[^\s/?#'"\x60<>]*:[^/?#]*?@` + - String.raw`${HOST_CHAR}(?:[\p{L}\p{N}\p{M}.-]*${HOST_CHAR})?:\d+$`, + String.raw`${HOST_CHAR}(?:(?:${LABEL_CHAR}|\.)*${HOST_CHAR})?:\d+$`, 'u', ); diff --git a/packages/cli/src/utils/acpModelUtils.test.ts b/packages/cli/src/utils/acpModelUtils.test.ts index 9291ec67238..5083e931a72 100644 --- a/packages/cli/src/utils/acpModelUtils.test.ts +++ b/packages/cli/src/utils/acpModelUtils.test.ts @@ -249,6 +249,29 @@ describe('acpModelUtils', () => { ['https://user:p?x@api.example/v1', 'https://api.example/v1'], ['https://user:p#x@api.example/v1', 'https://api.example/v1'], ['https://user:secret@api.example', 'https://api.example'], + // 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 + // either from it and the later `@` becomes the userinfo terminator, so the + // credential is still stripped but the host is reported as `evil.com`. Only an + // assertion on the surviving host catches that, which is why the expectation + // keeps the query and fragment verbatim. + [ + 'https://user:secret@api.example?k=v@evil.com', + 'https://api.example?k=v@evil.com', + ], + [ + 'https://user:secret@api.example#f@evil.com', + 'https://api.example#f@evil.com', + ], + [ + 'https://user:secret@api.example:8443?k=v@evil.com', + 'https://api.example:8443?k=v@evil.com', + ], + // The same two shapes with no credential to strip, so a widening of the + // authority cannot quietly start rewriting a host that was already clean. + ['https://api.example?k=v@evil.com', 'https://api.example?k=v@evil.com'], + ['https://api.example#f@evil.com', 'https://api.example#f@evil.com'], ])('sanitizes provider base URL credentials for %s', (input, expected) => { expect(sanitizeProviderBaseUrl(input)).toBe(expected); });