Skip to content

Commit 6e1169b

Browse files
Merge pull request #29 from devdiv-microsoft/tyler/satisfied-peafowl
MSRC 96762 (CVE-2025-21264)
2 parents 19e0f9e + 7ffa080 commit 6e1169b

File tree

7 files changed

+136
-89
lines changed

7 files changed

+136
-89
lines changed

src/vs/workbench/contrib/chat/browser/chatMarkdownRenderer.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import { IFileService } from '../../../../platform/files/common/files.js';
1616
import { IHoverService } from '../../../../platform/hover/browser/hover.js';
1717
import { IOpenerService } from '../../../../platform/opener/common/opener.js';
1818
import { REVEAL_IN_EXPLORER_COMMAND_ID } from '../../files/browser/fileConstants.js';
19-
import { ITrustedDomainService } from '../../url/browser/trustedDomainService.js';
2019

2120
const allowedHtmlTags = [
2221
'b',
@@ -63,7 +62,6 @@ export class ChatMarkdownRenderer extends MarkdownRenderer {
6362
options: IMarkdownRendererOptions | undefined,
6463
@ILanguageService languageService: ILanguageService,
6564
@IOpenerService openerService: IOpenerService,
66-
@ITrustedDomainService private readonly trustedDomainService: ITrustedDomainService,
6765
@IHoverService private readonly hoverService: IHoverService,
6866
@IFileService private readonly fileService: IFileService,
6967
@ICommandService private readonly commandService: ICommandService,
@@ -74,7 +72,7 @@ export class ChatMarkdownRenderer extends MarkdownRenderer {
7472
override render(markdown: IMarkdownString | undefined, options?: MarkdownRenderOptions, markedOptions?: MarkedOptions): IMarkdownRenderResult {
7573
options = {
7674
...options,
77-
remoteImageIsAllowed: (uri) => this.trustedDomainService.isValid(uri),
75+
remoteImageIsAllowed: (_uri) => false,
7876
sanitizerOptions: {
7977
replaceWithPlaintext: true,
8078
allowedTags: allowedHtmlTags,

src/vs/workbench/contrib/chat/electron-sandbox/tools/fetchPageTool.ts

Lines changed: 14 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55

66
import { CancellationToken } from '../../../../../base/common/cancellation.js';
77
import { MarkdownString } from '../../../../../base/common/htmlContent.js';
8+
import { ResourceSet } from '../../../../../base/common/map.js';
89
import { URI } from '../../../../../base/common/uri.js';
910
import { localize } from '../../../../../nls.js';
1011
import { IWebContentExtractorService } from '../../../../../platform/webContentExtractor/common/webContentExtractor.js';
11-
import { ITrustedDomainService } from '../../../url/browser/trustedDomainService.js';
1212
import { CountTokensCallback, IPreparedToolInvocation, IToolData, IToolImpl, IToolInvocation, IToolResult, IToolResultTextPart, ToolProgress } from '../../common/languageModelToolsService.js';
1313
import { InternalFetchWebPageToolId } from '../../common/tools/tools.js';
1414

@@ -34,11 +34,10 @@ export const FetchWebPageToolData: IToolData = {
3434
};
3535

3636
export class FetchWebPageTool implements IToolImpl {
37-
private _alreadyApprovedDomains = new Set<string>();
37+
private _alreadyApprovedDomains = new ResourceSet();
3838

3939
constructor(
4040
@IWebContentExtractorService private readonly _readerModeService: IWebContentExtractorService,
41-
@ITrustedDomainService private readonly _trustedDomainService: ITrustedDomainService,
4241
) { }
4342

4443
async invoke(invocation: IToolInvocation, _countTokens: CountTokensCallback, _progress: ToolProgress, _token: CancellationToken): Promise<IToolResult> {
@@ -53,9 +52,7 @@ export class FetchWebPageTool implements IToolImpl {
5352
// We approved these via confirmation, so mark them as "approved" in this session
5453
// if they are not approved via the trusted domain service.
5554
for (const uri of validUris) {
56-
if (!this._trustedDomainService.isValid(uri)) {
57-
this._alreadyApprovedDomains.add(uri.toString(true));
58-
}
55+
this._alreadyApprovedDomains.add(uri);
5956
}
6057

6158
const contents = await this._readerModeService.extract(validUris);
@@ -89,7 +86,7 @@ export class FetchWebPageTool implements IToolImpl {
8986
valid.push(uri);
9087
}
9188
});
92-
const urlsNeedingConfirmation = valid.filter(url => !this._trustedDomainService.isValid(url) && !this._alreadyApprovedDomains.has(url.toString(true)));
89+
const urlsNeedingConfirmation = valid.filter(url => !this._alreadyApprovedDomains.has(url));
9390

9491
const pastTenseMessage = invalid.length
9592
? invalid.length > 1
@@ -138,32 +135,17 @@ export class FetchWebPageTool implements IToolImpl {
138135

139136
const result: IPreparedToolInvocation = { invocationMessage, pastTenseMessage };
140137
if (urlsNeedingConfirmation.length) {
141-
const confirmationTitle = urlsNeedingConfirmation.length > 1
142-
? localize('fetchWebPage.confirmationTitle.plural', 'Fetch untrusted web pages?')
143-
: localize('fetchWebPage.confirmationTitle.singular', 'Fetch untrusted web page?');
144-
145-
const managedTrustedDomainsCommand = 'workbench.action.manageTrustedDomain';
146-
const confirmationMessage = new MarkdownString(
147-
urlsNeedingConfirmation.length > 1
148-
? urlsNeedingConfirmation.map(uri => `- ${uri.toString()}`).join('\n')
149-
: urlsNeedingConfirmation[0].toString(),
150-
{
151-
isTrusted: { enabledCommands: [managedTrustedDomainsCommand] },
152-
supportThemeIcons: true
153-
}
154-
);
155-
156-
confirmationMessage.appendMarkdown(
157-
'\n\n$(info) ' + localize(
158-
'fetchWebPage.confirmationMessageManageTrustedDomains',
159-
'You can [manage your trusted domains]({0}) to skip this confirmation in the future.',
160-
`command:${managedTrustedDomainsCommand}`
161-
)
162-
);
163-
164-
result.confirmationMessages = { title: confirmationTitle, message: confirmationMessage, allowAutoConfirm: false };
138+
let confirmationTitle: string;
139+
let confirmationMessage: string | MarkdownString;
140+
if (urlsNeedingConfirmation.length === 1) {
141+
confirmationTitle = localize('fetchWebPage.confirmationTitle.singular', 'Fetch untrusted web page?');
142+
confirmationMessage = urlsNeedingConfirmation[0].toString();
143+
} else {
144+
confirmationTitle = localize('fetchWebPage.confirmationTitle.plural', 'Fetch untrusted web pages?');
145+
confirmationMessage = new MarkdownString(urlsNeedingConfirmation.map(uri => `- ${uri.toString()}`).join('\n'));
146+
}
147+
result.confirmationMessages = { title: confirmationTitle, message: confirmationMessage, allowAutoConfirm: true };
165148
}
166-
167149
return result;
168150
}
169151

src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_remote_images.0.snap

Lines changed: 0 additions & 1 deletion
This file was deleted.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<div class="rendered-markdown"><p></p><div>&lt;img src="http://disallowed.com/image.jpg"&gt;</div><p></p></div>

src/vs/workbench/contrib/chat/test/browser/chatMarkdownRenderer.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import { MarkdownString } from '../../../../../base/common/htmlContent.js';
77
import { assertSnapshot } from '../../../../../base/test/common/snapshot.js';
88
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
99
import { ChatMarkdownRenderer } from '../../browser/chatMarkdownRenderer.js';
10-
import { ITrustedDomainService } from '../../../url/browser/trustedDomainService.js';
11-
import { MockTrustedDomainService } from '../../../url/test/browser/mockTrustedDomainService.js';
1210
import { workbenchInstantiationService } from '../../../../test/browser/workbenchTestServices.js';
1311

1412
suite('ChatMarkdownRenderer', () => {
@@ -17,7 +15,6 @@ suite('ChatMarkdownRenderer', () => {
1715
let testRenderer: ChatMarkdownRenderer;
1816
setup(() => {
1917
const instantiationService = store.add(workbenchInstantiationService(undefined, store));
20-
instantiationService.stub(ITrustedDomainService, new MockTrustedDomainService(['http://allowed.com']));
2118
testRenderer = instantiationService.createInstance(ChatMarkdownRenderer, {});
2219
});
2320

@@ -102,8 +99,8 @@ suite('ChatMarkdownRenderer', () => {
10299
await assertSnapshot(result.element.outerHTML);
103100
});
104101

105-
test('remote images', async () => {
106-
const md = new MarkdownString('<img src="http://allowed.com/image.jpg"> <img src="http://disallowed.com/image.jpg">');
102+
test('remote images are disallowed', async () => {
103+
const md = new MarkdownString('<img src="http://disallowed.com/image.jpg">');
107104
md.supportHtml = true;
108105
const result = store.add(testRenderer.render(md));
109106
await assertSnapshot(result.element.outerHTML);

src/vs/workbench/contrib/url/common/urlGlob.ts

Lines changed: 113 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -5,89 +5,154 @@
55

66
import { URI } from '../../../../base/common/uri.js';
77

8-
// TODO: rewrite this to use URIs directly and validate each part individually
9-
// instead of relying on memoization of the stringified URI.
10-
export const testUrlMatchesGlob = (uri: URI, globUrl: string): boolean => {
11-
let url = uri.with({ query: null, fragment: null }).toString(true);
12-
const normalize = (url: string) => url.replace(/\/+$/, '');
13-
globUrl = normalize(globUrl);
14-
url = normalize(url);
15-
16-
const memo = Array.from({ length: url.length + 1 }).map(() =>
17-
Array.from({ length: globUrl.length + 1 }).map(() => undefined),
18-
);
8+
/**
9+
* Normalizes a URL by removing trailing slashes and query/fragment components.
10+
* @param url The URL to normalize.
11+
* @returns URI - The normalized URI object.
12+
*/
13+
function normalizeURL(url: string | URI): URI {
14+
const uri = typeof url === 'string' ? URI.parse(url) : url;
15+
return uri.with({
16+
// Remove trailing slashes
17+
path: uri.path.replace(/\/+$/, ''),
18+
// Remove query and fragment
19+
query: null,
20+
fragment: null,
21+
});
22+
}
1923

20-
if (/^[^./:]*:\/\//.test(globUrl)) {
21-
return doUrlMatch(memo, url, globUrl, 0, 0);
22-
}
24+
/**
25+
* Checks if a given URL matches a glob URL pattern.
26+
* The glob URL pattern can contain wildcards (*) and subdomain matching (*.)
27+
* @param uri The URL to check.
28+
* @param globUrl The glob URL pattern to match against.
29+
* @returns boolean - True if the URL matches the glob URL pattern, false otherwise.
30+
*/
31+
export function testUrlMatchesGlob(uri: string | URI, globUrl: string): boolean {
32+
const normalizedUrl = normalizeURL(uri);
33+
let normalizedGlobUrl = normalizeURL(globUrl);
2334

24-
const scheme = /^(https?):\/\//.exec(url)?.[1];
25-
if (scheme) {
26-
return doUrlMatch(memo, url, `${scheme}://${globUrl}`, 0, 0);
35+
const globHasScheme = /^[^./:]*:\/\//.test(globUrl);
36+
// if the glob does not have a scheme we assume the scheme is http or https
37+
// so if the url doesn't have a scheme of http or https we return false
38+
if (!globHasScheme) {
39+
if (normalizedUrl.scheme !== 'http' && normalizedUrl.scheme !== 'https') {
40+
return false;
41+
}
42+
normalizedGlobUrl = normalizeURL(`${normalizedUrl.scheme}://${globUrl}`);
2743
}
2844

29-
return false;
30-
};
45+
return (
46+
doMemoUrlMatch(normalizedUrl.scheme, normalizedGlobUrl.scheme) &&
47+
// The authority is the only thing that should do port logic.
48+
doMemoUrlMatch(normalizedUrl.authority, normalizedGlobUrl.authority, true) &&
49+
(
50+
//
51+
normalizedGlobUrl.path === '/' ||
52+
doMemoUrlMatch(normalizedUrl.path, normalizedGlobUrl.path)
53+
)
54+
);
55+
}
56+
57+
/**
58+
* @param normalizedUrlPart The normalized URL part to match.
59+
* @param normalizedGlobUrlPart The normalized glob URL part to match against.
60+
* @param includePortLogic Whether to include port logic in the matching process.
61+
* @returns boolean - True if the URL part matches the glob URL part, false otherwise.
62+
*/
63+
function doMemoUrlMatch(
64+
normalizedUrlPart: string,
65+
normalizedGlobUrlPart: string,
66+
includePortLogic: boolean = false,
67+
) {
68+
const memo = Array.from({ length: normalizedUrlPart.length + 1 }).map(() =>
69+
Array.from({ length: normalizedGlobUrlPart.length + 1 }).map(() => undefined),
70+
);
3171

32-
const doUrlMatch = (
72+
return doUrlPartMatch(memo, includePortLogic, normalizedUrlPart, normalizedGlobUrlPart, 0, 0);
73+
}
74+
75+
/**
76+
* Recursively checks if a URL part matches a glob URL part.
77+
* This function uses memoization to avoid recomputing results for the same inputs.
78+
* It handles various cases such as exact matches, wildcard matches, and port logic.
79+
* @param memo A memoization table to avoid recomputing results for the same inputs.
80+
* @param includePortLogic Whether to include port logic in the matching process.
81+
* @param urlPart The URL part to match with.
82+
* @param globUrlPart The glob URL part to match against.
83+
* @param urlOffset The current offset in the URL part.
84+
* @param globUrlOffset The current offset in the glob URL part.
85+
* @returns boolean - True if the URL part matches the glob URL part, false otherwise.
86+
*/
87+
function doUrlPartMatch(
3388
memo: (boolean | undefined)[][],
34-
url: string,
35-
globUrl: string,
89+
includePortLogic: boolean,
90+
urlPart: string,
91+
globUrlPart: string,
3692
urlOffset: number,
37-
globUrlOffset: number,
38-
): boolean => {
93+
globUrlOffset: number
94+
): boolean {
3995
if (memo[urlOffset]?.[globUrlOffset] !== undefined) {
4096
return memo[urlOffset][globUrlOffset]!;
4197
}
4298

4399
const options = [];
44100

45-
// Endgame.
46-
// Fully exact match
47-
if (urlOffset === url.length) {
48-
return globUrlOffset === globUrl.length;
101+
// We've reached the end of the url.
102+
if (urlOffset === urlPart.length) {
103+
// We're also at the end of the glob url as well so we have an exact match.
104+
if (globUrlOffset === globUrlPart.length) {
105+
return true;
106+
}
107+
108+
if (includePortLogic && globUrlPart[globUrlOffset] + globUrlPart[globUrlOffset + 1] === ':*') {
109+
// any port match. Consume a port if it exists otherwise nothing. Always consume the base.
110+
return globUrlOffset + 2 === globUrlPart.length;
111+
}
112+
113+
return false;
49114
}
50115

51116
// Some path remaining in url
52-
if (globUrlOffset === globUrl.length) {
53-
const remaining = url.slice(urlOffset);
117+
if (globUrlOffset === globUrlPart.length) {
118+
const remaining = urlPart.slice(urlOffset);
54119
return remaining[0] === '/';
55120
}
56121

57-
if (url[urlOffset] === globUrl[globUrlOffset]) {
122+
if (urlPart[urlOffset] === globUrlPart[globUrlOffset]) {
58123
// Exact match.
59-
options.push(doUrlMatch(memo, url, globUrl, urlOffset + 1, globUrlOffset + 1));
124+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset + 1, globUrlOffset + 1));
60125
}
61126

62-
if (globUrl[globUrlOffset] + globUrl[globUrlOffset + 1] === '*.') {
127+
if (globUrlPart[globUrlOffset] + globUrlPart[globUrlOffset + 1] === '*.') {
63128
// Any subdomain match. Either consume one thing that's not a / or : and don't advance base or consume nothing and do.
64-
if (!['/', ':'].includes(url[urlOffset])) {
65-
options.push(doUrlMatch(memo, url, globUrl, urlOffset + 1, globUrlOffset));
129+
if (!['/', ':'].includes(urlPart[urlOffset])) {
130+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset + 1, globUrlOffset));
66131
}
67-
options.push(doUrlMatch(memo, url, globUrl, urlOffset, globUrlOffset + 2));
132+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset, globUrlOffset + 2));
68133
}
69134

70-
if (globUrl[globUrlOffset] === '*') {
135+
if (globUrlPart[globUrlOffset] === '*') {
71136
// Any match. Either consume one thing and don't advance base or consume nothing and do.
72-
if (urlOffset + 1 === url.length) {
137+
if (urlOffset + 1 === urlPart.length) {
73138
// If we're at the end of the input url consume one from both.
74-
options.push(doUrlMatch(memo, url, globUrl, urlOffset + 1, globUrlOffset + 1));
139+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset + 1, globUrlOffset + 1));
75140
} else {
76-
options.push(doUrlMatch(memo, url, globUrl, urlOffset + 1, globUrlOffset));
141+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset + 1, globUrlOffset));
77142
}
78-
options.push(doUrlMatch(memo, url, globUrl, urlOffset, globUrlOffset + 1));
143+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset, globUrlOffset + 1));
79144
}
80145

81-
if (globUrl[globUrlOffset] + globUrl[globUrlOffset + 1] === ':*') {
82-
// any port match. Consume a port if it exists otherwise nothing. Always comsume the base.
83-
if (url[urlOffset] === ':') {
146+
if (includePortLogic && globUrlPart[globUrlOffset] + globUrlPart[globUrlOffset + 1] === ':*') {
147+
// any port match. Consume a port if it exists otherwise nothing. Always consume the base.
148+
if (urlPart[urlOffset] === ':') {
84149
let endPortIndex = urlOffset + 1;
85-
do { endPortIndex++; } while (/[0-9]/.test(url[endPortIndex]));
86-
options.push(doUrlMatch(memo, url, globUrl, endPortIndex, globUrlOffset + 2));
150+
do { endPortIndex++; } while (/[0-9]/.test(urlPart[endPortIndex]));
151+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, endPortIndex, globUrlOffset + 2));
87152
} else {
88-
options.push(doUrlMatch(memo, url, globUrl, urlOffset, globUrlOffset + 2));
153+
options.push(doUrlPartMatch(memo, includePortLogic, urlPart, globUrlPart, urlOffset, globUrlOffset + 2));
89154
}
90155
}
91156

92157
return (memo[urlOffset][globUrlOffset] = options.some(a => a === true));
93-
};
158+
}

src/vs/workbench/contrib/url/test/browser/trustedDomains.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,9 @@ suite('Link protection domain matching', () => {
113113
linkAllowedByRules('https://github.com/login/oauth/authorize?foo=4', ['https://github.com/login/oauth/authorize']);
114114
linkAllowedByRules('https://github.com/login/oauth/authorize#foo', ['https://github.com/login/oauth/authorize']);
115115
});
116+
117+
test('ensure individual parts of url are compared and wildcard does not leak out', () => {
118+
linkNotAllowedByRules('https://x.org/github.com', ['https://*.github.com']);
119+
linkNotAllowedByRules('https://x.org/y.github.com', ['https://*.github.com']);
120+
});
116121
});

0 commit comments

Comments
 (0)