-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(serve): use authority-scoped credential stripping in provider warning sanitizer #8408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0bbcee4
5722231
456a597
9622a32
e901f90
69b153a
03b1141
81fe680
b431df7
46b93f7
4f77712
26578a1
bbccdc4
7758092
68d4871
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -260,7 +260,7 @@ function sanitizeProviderWarning(warning: string): string { | |
|
|
||
| const segmentEnd = findUrlSegmentEnd(warning, next.index, next.marker); | ||
| const segment = warning.slice(next.index, segmentEnd); | ||
| result += sanitizeProviderWarningSegment(segment, next.marker.length); | ||
| result += sanitizeProviderWarningSegment(segment); | ||
|
|
||
| index = segmentEnd; | ||
| next = findNextUrlStart(warning, index); | ||
|
|
@@ -295,36 +295,22 @@ 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)}`; | ||
| function sanitizeProviderWarningSegment(segment: string): string { | ||
| // When the whole segment is a URL that sanitizeProviderBaseUrl confirms | ||
| // carries real userinfo (it changes the segment), use its result directly. | ||
| // This handles space-containing-credential URLs that URL_LIKE_PATTERN cannot | ||
| // match past the whitespace, and the '@'-in-password shape (last '@' wins). | ||
| // The veto in sanitizeProviderBaseUrl leaves pathless-URL + prose-email | ||
| // shapes unchanged, so a prose email's '@' is never stripped. #8136. | ||
| const sanitized = sanitizeProviderBaseUrl(segment); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-6: The repo now carries four parallel URL-userinfo removal implementations with divergent edge-case behaviour: this PR's stripping parser, β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| if (sanitized !== segment) { | ||
| return sanitized; | ||
|
Comment on lines
+305
to
+307
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Whole-segment delegation strips at a whitespace-less prose '@' in multi-'@' segments β for both pathless AND bounded authorities. β Failure scenario: probe-verified A/B through the real pipeline: β qwen3.8-max via Qwen Code /review (v0.21.6) |
||
| } | ||
|
Comment on lines
+305
to
308
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R7-8: Round-7 blocker R7-8 still stands: a Windows-domain credential whose password contains whitespace is never stripped through the serve warning path β the whole-segment delegation returns the segment unchanged (the R7-9 mechanism defeats the windowsCred bound) and the Witness (serve-path A/B, verifier drove Suggested fix: fix the R7-9 authority truncation at its root ( β qwen3.8-max via Qwen Code /review (v0.21.12) |
||
|
|
||
| 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, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,6 +249,174 @@ 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'], | ||
| // #8136: pathless URL + prose email shapes. WHATWG misparses these as | ||
| // userinfo; the veto (all-digit port before first whitespace) protects the | ||
| // with-port shape, and the pathless-prose guard protects the no-colon shape. | ||
|
Comment on lines
+252
to
+254
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Issue #8136's literal bug-1 repro has a PATH ( Suggested fix: add a pinned row for the verbatim repro, e.g. β qwen3.8-max via Qwen Code /review (v0.21.6) |
||
| [ | ||
| 'https://api.example:8443 - contact admin@example.com', | ||
| 'https://api.example:8443 - contact admin@example.com', | ||
| ], | ||
| [ | ||
| 'https://api.example - contact admin@example.com', | ||
| 'https://api.example - contact admin@example.com', | ||
| ], | ||
| [ | ||
| 'https://ollama.local - contact admin@example.com', | ||
| 'https://ollama.local - contact admin@example.com', | ||
| ], | ||
| // Credentials + pathless + email: strip the credential, keep host + prose. | ||
| [ | ||
| 'https://user:pass@host.example:8443 - contact admin@example.com', | ||
| 'https://host.example:8443 - contact admin@example.com', | ||
| ], | ||
| // Space-in-password: the last '@' within the bounded authority is the real | ||
| // userinfo terminator (the password's '@' precedes it). #8136 R1-1/R1-3. | ||
| [ | ||
| 'https://user:sec ret@host.example/v1 - contact admin@example.com', | ||
| 'https://host.example/v1 - contact admin@example.com', | ||
| ], | ||
| ['https://user:p@ss word@host.example/v1', 'https://host.example/v1'], | ||
| // R1-2 KNOWN RESIDUAL: digit-prefix + space password is locally | ||
| // indistinguishable from a dotless host + port + prose email; the veto | ||
| // fires and the credential leaks. Same tradeoff class as R5-7, pending | ||
| // maintainer sign-off. #8136 R1-2. | ||
| ['https://user:1234 secret@host', 'https://user:1234 secret@host'], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R1-2: Round-1 blocker R1-2 (re-asserted in rounds 2-6) still stands: a password starting with digits followed by a space is vetoed like a port-prose misparse and leaks IN FULL; the PR pins the leak as intended behaviour ('pending maintainer sign-off') with no sign-off in the record β a credential-exposure regression versus the merge base in the function issue #8136 exists to harden (Expected bullet 2: password 'removed in full'). β Failure scenario: Probe-verified A/B: sanitizeProviderBaseUrl('https://user:1234 secret@host') and ('https://foo.bar:1234 secret@host') return the input unchanged at HEAD while the merge base returned 'https://host'; WHATWG confirms real userinfo (password '1234%20secret'). The leak reaches models[].baseUrl, current.baseUrl, errors[].error, and ACP wire responses. Suggested fix: Add a discriminator the veto can use (e.g. when the token before the colon is not host-shaped, or when the input is a configured baseUrl rather than free prose, prefer the parsed userinfo over the digit veto), or obtain explicit maintainer sign-off plus a follow-up issue so the pinned leak is a recorded decision rather than an open Critical. β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| // #8136 R3: passwords/hostnames the previous host-shaped-char heuristic | ||
| // mishandled. The structural terminator scan resolves these. | ||
| // Password containing '@' (pathless): strip at the LAST '@', not the first. | ||
| ['https://user:p@ss@host', 'https://host'], | ||
| // Underscore-leading host (previously outside HOST_SHAPED_CHAR): strip. | ||
| ['https://user:pass@_host', 'https://_host'], | ||
| // Tab immediately after '@' (WHATWG strips it as userinfo terminator): strip. | ||
| [`https://user:pass@\thost`, 'https://\thost'], | ||
| // Password containing '@' AND whitespace (bounded authority): the last '@' | ||
| // within the bounded authority is the terminator. | ||
| ['https://user:p@ss word@host.example/v1', 'https://host.example/v1'], | ||
|
Comment on lines
+292
to
+294
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This row is byte-identical to the row at line 278 of the same table; the comment claims a distinct "bounded authority" scenario the duplicate does not cover. β Concrete cost: the same assertion runs twice under the same generated title, and a future editor amending one row for the intended shape will silently leave the other diverging; any genuinely intended distinct variant has zero coverage. Delete this duplicate row, or replace it with the actually-intended distinct case. β qwen3.8-max via Qwen Code /review (v0.21.7) |
||
| // Whitespace in the password (pathless): the '@' whose following text is a | ||
| // clean hostname is the terminator, so the password's whitespace does not | ||
| // end the scan. #8136 R3-5. | ||
| ['https://user:pass word@host', 'https://host'], | ||
| ['https://user:p@ss word@host', 'https://host'], | ||
| // Unicode whitespace in the password (WHATWG percent-encodes it): strip. | ||
| ['https://user:paΒ ss@host', 'https://host'], | ||
| // #8136 R3-6: prose with a path - the prose email's '@' must not destroy the | ||
| // host. The pathless prose veto fires regardless of a later delimiter. | ||
| [ | ||
| 'https://ollama.local - email admin@example.com or check /var/log/qwen', | ||
| 'https://ollama.local - email admin@example.com or check /var/log/qwen', | ||
| ], | ||
| // #8136 R4-1: catch-branch (new URL throws) prose '@' after whitespace must | ||
| // not become the strip point - strip the credential, keep host + prose. | ||
| ['https://user:pass@host - ping admin@', 'https://host - ping admin@'], | ||
| // R9-7 KNOWN RESIDUAL: a real host + prose email with a VALID email domain | ||
| // is indistinguishable from a real credential whose terminator is that | ||
| // email's '@' - the prose email's host replaces the real host. (A '%zz' | ||
| // invalid domain makes CLEAN_HOST_AFTER fail and passes spuriously; this | ||
| // pins the real-domain behavior instead.) Same class as R5-1, pending | ||
| // maintainer sign-off. | ||
| ['https://user@host - contact admin@example.com', 'https://example.com'], | ||
| // #8136 R4-3: whitespace-less multi-'@' prose - the FIRST '@' ends the | ||
| // userinfo; the prose email's '@' is not a terminator. | ||
| [ | ||
| 'https://u:p@h,see(admin@example.com)', | ||
| 'https://h,see(admin@example.com)', | ||
| ], | ||
| [ | ||
| 'https://u:p@h,see(admin@example.com)/x', | ||
| 'https://h,see(admin@example.com)/x', | ||
| ], | ||
| // #8136 R4-4/R4-5: backslash - a Windows domain\user:pass@ credential strips | ||
| // as a single userinfo run, while '\' terminates the authority for prose. | ||
| ['https://DOMAIN\\user:pass@proxy', 'https://proxy'], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-4: The new backslash authority-terminator block in ['https://user:p@ss@host\\path', 'https://host\\path'],(optionally plus a β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| ['https://user:pass@host\\path', 'https://host\\path'], | ||
| // #8136 R5-3: an '@' in the password with an underscore host still strips - | ||
| // CLEAN_HOST_AFTER accepts underscore-leading hosts. | ||
| ['https://user:p@ss@_host', 'https://_host'], | ||
| ['https://user:pass@_host:8080', 'https://_host:8080'], | ||
| // #8136 R5-14: URL schemes are case-insensitive; uppercase must still strip. | ||
| ['HTTPS://user:pass@host/v1', 'HTTPS://host/v1'], | ||
| ['HTTP://user:pass@host', 'HTTP://host'], | ||
| // #8136 R5-2: a leading '@' (empty userinfo) does not loop and is a no-op. | ||
| ['https://@host', 'https://host'], | ||
| // #8136 R5-1/R5-7 KNOWN RESIDUAL: a dotted username + digit-prefix password | ||
| // followed by space is locally indistinguishable from a dotted host + port | ||
| // + prose email; the veto fires and the credential leaks. Same tradeoff as | ||
| // R1-2, pending maintainer sign-off. | ||
| ['https://foo.bar:1234 secret@host', 'https://foo.bar:1234 secret@host'], | ||
| // #8136 R1-7: IPv6 bracket + port + prose email must stay unchanged. The | ||
| // prose veto skips the bracket's inner colons and accepts an em-dash/empty | ||
| // port candidate. | ||
| [ | ||
| 'https://[::1]:8443 β contact admin@example.com', | ||
| 'https://[::1]:8443 β contact admin@example.com', | ||
| ], | ||
| [ | ||
| 'https://ollama.local: please contact admin@example.com', | ||
| 'https://ollama.local: please contact admin@example.com', | ||
| ], | ||
| // #8136 R6-1: a port followed by punctuation (`;`/`,`/`.`) + prose email. | ||
| [ | ||
| 'https://api.example:8443; contact admin@example.com', | ||
| 'https://api.example:8443; contact admin@example.com', | ||
| ], | ||
| // #8136 R7-3: a port followed by multiple punctuation chars + prose email. | ||
| [ | ||
| 'https://api.example:8443,. contact admin@example.com', | ||
| 'https://api.example:8443,. contact admin@example.com', | ||
| ], | ||
| // #8136 R7-10: a Unicode (IDN) host is a clean hostname β strip the credential. | ||
| ['https://user:pass@δΎε.ζ΅θ―/v1', 'https://δΎε.ζ΅θ―/v1'], | ||
| // #8136 R7-5 KNOWN RESIDUAL: an '@' in the path (npm scoped) with a | ||
| // host:port-shaped authority before it is stripped by the no-'@' fallback | ||
| // (base has the same behavior β the between-run has no whitespace). Same | ||
| // tradeoff class, pending maintainer sign-off. | ||
| [ | ||
| 'https://registry.example: check /node_modules/@qwen/pkg', | ||
| 'https://qwen/pkg', | ||
| ], | ||
| // #8136 R7-1 KNOWN RESIDUAL: a colonless username containing whitespace | ||
| // (`user @host`) is indistinguishable from a prose `host @host` shape; | ||
| // the prose veto fires and it leaks. Pending maintainer sign-off. | ||
| ['https://user @host.example/v1', 'https://user @host.example/v1'], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R7-1: Round-7 blocker R7-1 still stands: a colonless username containing whitespace leaks in full; pinned as KNOWN RESIDUAL pending sign-off; the merge base stripped it. β Failure scenario: probe-verified: sanitizeProviderBaseUrl('https://user @host.example/v1') returns the input unchanged at HEAD while the merge base returned 'https://host.example/v1'; WHATWG reports username 'user%20' (real userinfo, not prose). The leak reaches models[].baseUrl, current.baseUrl, and ACP responses via the direct callers. β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| // #8136 R5-12: a backslash-free authority with a later prose `a:b@c` is NOT | ||
| // misread as a Windows credential by findAuthorityEnd (R5-12 fixed the | ||
| // windowsCred scan bound). The remaining leak is the R5-1 residual class. | ||
| ['https://user:pass@host a:b@c', 'https://c'], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R6-3: Round-6 blocker R6-3 still stands as behaviour: password containing '@' + real host + whitespace + prose email with a clean domain β the backward scan accepts the PROSE email's '@' and the message collapses to that domain; pinned by this test row without maintainer sign-off. β Failure scenario: Probe-verified: pipeline 'Failed https://user:pass@host - contact x@y.z' β 'Failed https://y.z' (base: credential stripped, host + prose preserved); the pinned row 'https://user:pass@host a:b@c' β 'https://c' reproduces at the baseUrl level and passes in the green suite β the corrupted output is specified behaviour with no sign-off in the record. Suggested fix: Same disposition as R5-1: resolve the ambiguity toward the real terminator, or veto the class and pin the vetoed output with an explicit recorded maintainer decision. β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| // #8136 R5-1/R6-3 KNOWN RESIDUAL: a real terminator in the first '@' with a | ||
| // dotless host after it, followed by prose with an '@host', is | ||
| // indistinguishable from a password containing '@' + a real terminator after | ||
| // whitespace (`user:p@ss word@host` -> `host`). The prose shape's host gets | ||
| // replaced by the prose email's domain; same tradeoff class as R1-2, | ||
| // pending maintainer sign-off. | ||
| [ | ||
| 'https://user:pass@ollama - contact admin@example.com', | ||
| 'https://example.com', | ||
|
Comment on lines
+392
to
+393
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R5-1: Round-5 blocker R5-1 (re-asserted rounds 6-7) still stands: credential + dotless host + trailing prose email collapses the WHOLE message to the email's domain; pinned as KNOWN RESIDUAL 'pending maintainer sign-off' with no sign-off in the record β the exact corruption class issue #8136 was filed about ('The message the user sees is not just redacted, it is wrong'). β Failure scenario: probe-verified through the real warning pipeline: sanitizeProviderWarning('Cannot reach https://user:pass@ollama - contact admin@example.com') β 'Cannot reach https://example.com' at HEAD, while the merge base emitted 'Cannot reach https://ollama - contact admin@example.com' (credential stripped, host + prose preserved) β a warning-level regression introduced by the whole-segment delegation. The corrupted output is baked into the passing suite without the recorded maintainer decision the PR's own comment declares required. Suggested fix: resolve the ambiguity toward the real terminator, or veto the class and pin the input unchanged β do not pin the corrupted output β and record an explicit maintainer decision first. β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| ], | ||
|
Comment on lines
+391
to
+394
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R5-1: Round-6 blocker R5-1 still stands as behaviour: credential + dotless host + trailing prose email collapses the WHOLE message to the email's domain, and this test row pins the corrupted output as intended 'KNOWN RESIDUAL' behaviour 'pending maintainer sign-off' β no sign-off exists in the issue or PR record (independently verified). The wrapper comment added in workspace-providers-status.ts ('a prose email's @ is never stripped') is contradicted by it. Issue #8136: 'The message the user sees is not just redacted, it is wrong.' β Failure scenario: Probe-verified through the real pipeline: sanitizeProviderWarning('Cannot reach https://user:pass@ollama - contact admin@example.com') β 'Cannot reach https://example.com'; merge base stripped the credential and kept host + prose. Same for the localhost variant; 'https://user:pass@host a:b@c' β 'https://c'. The corrupted output is baked into the passing suite as specified behaviour without the recorded maintainer decision the PR's own comment declares required. Suggested fix: Treat a clean host token (dotless included, optional port) followed by whitespace/end after the first '@' as the terminator; if the ambiguity is judged inseparable, veto the class (return the input unchanged) and pin the vetoed output β do not ship the corrupted output as specified behaviour without an explicit maintainer decision recorded in the thread or a follow-up issue. β qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| [ | ||
| 'https://user:p@ss word@host.example - contact admin@example.com', | ||
| 'https://example.com', | ||
| ], | ||
| // #8136 repro-1 (with-path port + prose email): must stay unchanged - the | ||
| // path bounds the authority, so the prose '@' is never the strip point. | ||
| [ | ||
| 'https://api.example:8443/v1 - contact admin@example.com', | ||
| 'https://api.example:8443/v1 - contact admin@example.com', | ||
| ], | ||
| // #8136 repro-1 verbatim (em-dash as in the issue) also stays unchanged. | ||
| [ | ||
| 'https://api.example:8443/v1 β contact admin@example.com', | ||
| 'https://api.example:8443/v1 β contact admin@example.com', | ||
| ], | ||
| // URL-throwing shapes (invalid %, space in host) + prose email: do not | ||
| // strip the prose '@'. #8136 R2-2. | ||
| [ | ||
| 'https://api.example%/v1, contact admin@example.com', | ||
| 'https://api.example%/v1, contact admin@example.com', | ||
| ], | ||
| [ | ||
| 'https://my service/v1 - contact admin@example.com', | ||
| 'https://my service/v1 - contact admin@example.com', | ||
| ], | ||
| ])('sanitizes provider base URL credentials for %s', (input, expected) => { | ||
| expect(sanitizeProviderBaseUrl(input)).toBe(expected); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R7-20: This added comment asserts 'a prose email's @ is never stripped', but for credential + dotless-host + prose-email segments sanitizeProviderBaseUrl DOES change the segment and the strip point IS the prose email's '@' β the wrapper returns that result directly (the PR's own pinned test reproduces the collapse). β Failure scenario: 'Cannot reach https://user:pass@ollama - contact admin@example.com' β 'Cannot reach https://example.com' (probe-verified). A maintainer reasoning about what the segment pass can emit β e.g. while fixing the R5-1 residuals β would conclude from this comment that the email domain can never replace the host; demonstrably false.
β qwen3.8-max via Qwen Code /review (v0.21.8)