From 652c948b85ff951ced7178212e6bc914ff8c7aaf Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 15 May 2024 22:03:17 -0700 Subject: [PATCH] Check for allowed names in the participant fullName (#212849) * Check for allowed names in the participant fullName * Validate provider name and fullName * Fix --- src/vs/base/common/strings.ts | 21 +++++++++++++++++++ src/vs/base/test/common/strings.test.ts | 16 ++++++++++++++ .../contrib/chat/browser/chatAgentHover.ts | 6 +++--- .../chatMarkdownDecorationsRenderer.ts | 2 +- .../browser/chatParticipantContributions.ts | 17 +++++++++++++++ .../browser/contrib/chatInputCompletions.ts | 2 +- .../contrib/chat/common/chatAgents.ts | 16 +++++++++++--- .../contrib/chat/common/chatViewModel.ts | 20 ++++++++++++------ 8 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/vs/base/common/strings.ts b/src/vs/base/common/strings.ts index 79d22ee0e6d..70bded5a67d 100644 --- a/src/vs/base/common/strings.ts +++ b/src/vs/base/common/strings.ts @@ -1249,6 +1249,16 @@ export class AmbiguousCharacters { return this.confusableDictionary.has(codePoint); } + public containsAmbiguousCharacter(str: string): boolean { + for (let i = 0; i < str.length; i++) { + const codePoint = str.codePointAt(i); + if (typeof codePoint === 'number' && this.isAmbiguous(codePoint)) { + return true; + } + } + return false; + } + /** * Returns the non basic ASCII code point that the given code point can be confused, * or undefined if such code point does note exist. @@ -1281,6 +1291,17 @@ export class InvisibleCharacters { return InvisibleCharacters.getData().has(codePoint); } + public static containsInvisibleCharacter(str: string): boolean { + for (let i = 0; i < str.length; i++) { + const codePoint = str.codePointAt(i); + if (typeof codePoint === 'number' && InvisibleCharacters.isInvisibleCharacter(codePoint)) { + return true; + } + } + return false; + + } + public static get codePoints(): ReadonlySet { return InvisibleCharacters.getData(); } diff --git a/src/vs/base/test/common/strings.test.ts b/src/vs/base/test/common/strings.test.ts index bb9e5db1d51..4be439f4656 100644 --- a/src/vs/base/test/common/strings.test.ts +++ b/src/vs/base/test/common/strings.test.ts @@ -558,6 +558,22 @@ suite('Strings', () => { assert.strictEqual(strings.count('hello world', 'foo'), 0); }); + test('containsAmbiguousCharacter', () => { + assert.strictEqual(strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter('abcd'), false); + assert.strictEqual(strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter('üå'), false); + assert.strictEqual(strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter('(*&^)'), false); + + assert.strictEqual(strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter('ο'), true); + assert.strictEqual(strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter('abɡc'), true); + }); + + test('containsInvisibleCharacter', () => { + assert.strictEqual(strings.InvisibleCharacters.containsInvisibleCharacter('abcd'), false); + assert.strictEqual(strings.InvisibleCharacters.containsInvisibleCharacter(' '), true); + assert.strictEqual(strings.InvisibleCharacters.containsInvisibleCharacter('a\u{e004e}b'), true); + assert.strictEqual(strings.InvisibleCharacters.containsInvisibleCharacter('a\u{e015a}\u000bb'), true); + }); + ensureNoDisposablesAreLeakedInTestSuite(); }); diff --git a/src/vs/workbench/contrib/chat/browser/chatAgentHover.ts b/src/vs/workbench/contrib/chat/browser/chatAgentHover.ts index 6216dc2248d..7e738c0ccfe 100644 --- a/src/vs/workbench/contrib/chat/browser/chatAgentHover.ts +++ b/src/vs/workbench/contrib/chat/browser/chatAgentHover.ts @@ -15,7 +15,7 @@ import { ThemeIcon } from 'vs/base/common/themables'; import { URI } from 'vs/base/common/uri'; import { localize } from 'vs/nls'; import { ICommandService } from 'vs/platform/commands/common/commands'; -import { IChatAgentData, IChatAgentNameService, IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents'; +import { getFullyQualifiedId, IChatAgentData, IChatAgentNameService, IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents'; import { showExtensionsWithIdsCommandId } from 'vs/workbench/contrib/extensions/browser/extensionsActions'; import { verifiedPublisherIcon } from 'vs/workbench/contrib/extensions/browser/extensionsIcons'; import { IExtensionsWorkbenchService } from 'vs/workbench/contrib/extensions/common/extensions'; @@ -87,7 +87,8 @@ export class ChatAgentHover extends Disposable { this.domNode.classList.toggle('noExtensionName', !!agent.isDynamic); - this.name.textContent = `@${agent.name}`; + const isAllowed = this.chatAgentNameService.getAgentNameRestriction(agent); + this.name.textContent = isAllowed ? `@${agent.name}` : getFullyQualifiedId(agent); this.extensionName.textContent = agent.extensionDisplayName; this.publisherName.textContent = agent.publisherDisplayName ?? agent.extensionPublisherId; @@ -99,7 +100,6 @@ export class ChatAgentHover extends Disposable { } this.description.textContent = description; - const isAllowed = this.chatAgentNameService.getAgentNameRestriction(agent).get(); this.domNode.classList.toggle('allowedName', isAllowed); this.domNode.classList.toggle('verifiedPublisher', false); diff --git a/src/vs/workbench/contrib/chat/browser/chatMarkdownDecorationsRenderer.ts b/src/vs/workbench/contrib/chat/browser/chatMarkdownDecorationsRenderer.ts index b62beef0cf4..25c4ac91e44 100644 --- a/src/vs/workbench/contrib/chat/browser/chatMarkdownDecorationsRenderer.ts +++ b/src/vs/workbench/contrib/chat/browser/chatMarkdownDecorationsRenderer.ts @@ -40,7 +40,7 @@ export function agentToMarkdown(agent: IChatAgentData, isClickable: boolean, acc const chatAgentNameService = accessor.get(IChatAgentNameService); const chatAgentService = accessor.get(IChatAgentService); - const isAllowed = chatAgentNameService.getAgentNameRestriction(agent).get(); + const isAllowed = chatAgentNameService.getAgentNameRestriction(agent); let name = `${isAllowed ? agent.name : getFullyQualifiedId(agent)}`; const isDupe = isAllowed && chatAgentService.getAgentsByName(agent.name).length > 1; if (isDupe) { diff --git a/src/vs/workbench/contrib/chat/browser/chatParticipantContributions.ts b/src/vs/workbench/contrib/chat/browser/chatParticipantContributions.ts index d05ea9d07e5..e9e074f0cfb 100644 --- a/src/vs/workbench/contrib/chat/browser/chatParticipantContributions.ts +++ b/src/vs/workbench/contrib/chat/browser/chatParticipantContributions.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { isNonEmptyArray } from 'vs/base/common/arrays'; +import * as strings from 'vs/base/common/strings'; import { Codicon } from 'vs/base/common/codicons'; import { Disposable, DisposableMap, DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { localize, localize2 } from 'vs/nls'; @@ -159,6 +160,22 @@ export class ChatExtensionPointHandler implements IWorkbenchContribution { chatParticipantExtensionPoint.setHandler((extensions, delta) => { for (const extension of delta.added) { for (const providerDescriptor of extension.value) { + if (!providerDescriptor.name.match(/^[\w0-9_-]+$/)) { + this.logService.error(`Extension '${extension.description.identifier.value}' CANNOT register participant with invalid name: ${providerDescriptor.name}. Name must match /^[\\w0-9_-]+$/.`); + continue; + } + + if (providerDescriptor.fullName && strings.AmbiguousCharacters.getInstance(new Set()).containsAmbiguousCharacter(providerDescriptor.fullName)) { + this.logService.error(`Extension '${extension.description.identifier.value}' CANNOT register participant with fullName that contains ambiguous characters: ${providerDescriptor.fullName}.`); + continue; + } + + // Spaces are allowed but considered "invisible" + if (providerDescriptor.fullName && strings.InvisibleCharacters.containsInvisibleCharacter(providerDescriptor.fullName.replace(/ /g, ''))) { + this.logService.error(`Extension '${extension.description.identifier.value}' CANNOT register participant with fullName that contains invisible characters: ${providerDescriptor.fullName}.`); + continue; + } + if (providerDescriptor.isDefault && !isProposedApiEnabled(extension.description, 'defaultChatParticipant')) { this.logService.error(`Extension '${extension.description.identifier.value}' CANNOT use API proposal: defaultChatParticipant.`); continue; diff --git a/src/vs/workbench/contrib/chat/browser/contrib/chatInputCompletions.ts b/src/vs/workbench/contrib/chat/browser/contrib/chatInputCompletions.ts index da4a7aea5f5..5e0dc593588 100644 --- a/src/vs/workbench/contrib/chat/browser/contrib/chatInputCompletions.ts +++ b/src/vs/workbench/contrib/chat/browser/contrib/chatInputCompletions.ts @@ -393,7 +393,7 @@ class VariableCompletions extends Disposable { Registry.as(WorkbenchExtensions.Workbench).registerWorkbenchContribution(VariableCompletions, LifecyclePhase.Eventually); function getAgentCompletionDetails(agent: IChatAgentData, otherAgents: IChatAgentData[], chatAgentNameService: IChatAgentNameService): { label: string; isDupe: boolean } { - const isAllowed = chatAgentNameService.getAgentNameRestriction(agent).get(); + const isAllowed = chatAgentNameService.getAgentNameRestriction(agent); const agentLabel = `${chatAgentLeader}${isAllowed ? agent.name : getFullyQualifiedId(agent)}`; const isDupe = isAllowed && !!otherAgents.find(other => other.name === agent.name && other.id !== agent.id); diff --git a/src/vs/workbench/contrib/chat/common/chatAgents.ts b/src/vs/workbench/contrib/chat/common/chatAgents.ts index 9b48a3f2529..3fc918226a5 100644 --- a/src/vs/workbench/contrib/chat/common/chatAgents.ts +++ b/src/vs/workbench/contrib/chat/common/chatAgents.ts @@ -389,7 +389,7 @@ interface IChatParticipantRegistryResponse { export interface IChatAgentNameService { _serviceBrand: undefined; - getAgentNameRestriction(chatAgentData: IChatAgentData): IObservable; + getAgentNameRestriction(chatAgentData: IChatAgentData): boolean; } export class ChatAgentNameService implements IChatAgentNameService { @@ -454,10 +454,20 @@ export class ChatAgentNameService implements IChatAgentNameService { this.storageService.store(ChatAgentNameService.StorageKey, JSON.stringify(registry), StorageScope.APPLICATION, StorageTarget.MACHINE); } - getAgentNameRestriction(chatAgentData: IChatAgentData): IObservable { + /** + * Returns true if the agent is allowed to use this name + */ + getAgentNameRestriction(chatAgentData: IChatAgentData): boolean { + // TODO would like to use observables here but nothing uses it downstream and I'm not sure how to combine these two + const nameAllowed = this.checkAgentNameRestriction(chatAgentData.name, chatAgentData).get(); + const fullNameAllowed = !chatAgentData.fullName || this.checkAgentNameRestriction(chatAgentData.fullName.replace(/\s/g, ''), chatAgentData).get(); + return nameAllowed && fullNameAllowed; + } + + private checkAgentNameRestriction(name: string, chatAgentData: IChatAgentData): IObservable { // Registry is a map of name to an array of extension publisher IDs or extension IDs that are allowed to use it. // Look up the list of extensions that are allowed to use this name - const allowList = this.registry.map(registry => registry[chatAgentData.name.toLowerCase()]); + const allowList = this.registry.map(registry => registry[name.toLowerCase()]); return allowList.map(allowList => { if (!allowList) { return true; diff --git a/src/vs/workbench/contrib/chat/common/chatViewModel.ts b/src/vs/workbench/contrib/chat/common/chatViewModel.ts index 8d4161ff747..130ef76979b 100644 --- a/src/vs/workbench/contrib/chat/common/chatViewModel.ts +++ b/src/vs/workbench/contrib/chat/common/chatViewModel.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { Emitter, Event } from 'vs/base/common/event'; +import { IMarkdownString } from 'vs/base/common/htmlContent'; import { Disposable } from 'vs/base/common/lifecycle'; import { marked } from 'vs/base/common/marked/marked'; import { ThemeIcon } from 'vs/base/common/themables'; @@ -11,13 +12,12 @@ import { URI } from 'vs/base/common/uri'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { ILogService } from 'vs/platform/log/common/log'; import { annotateVulnerabilitiesInText } from 'vs/workbench/contrib/chat/common/annotations'; -import { IChatAgentCommand, IChatAgentData, IChatAgentResult } from 'vs/workbench/contrib/chat/common/chatAgents'; +import { getFullyQualifiedId, IChatAgentCommand, IChatAgentData, IChatAgentNameService, IChatAgentResult } from 'vs/workbench/contrib/chat/common/chatAgents'; import { ChatModelInitState, IChatModel, IChatRequestModel, IChatResponseModel, IChatTextEditGroup, IChatWelcomeMessageContent, IResponse } from 'vs/workbench/contrib/chat/common/chatModel'; import { IParsedChatRequest } from 'vs/workbench/contrib/chat/common/chatParserTypes'; -import { IChatCommandButton, IChatConfirmation, IChatContentReference, IChatFollowup, IChatProgressMessage, IChatResponseErrorDetails, IChatResponseProgressFileTreeData, IChatTask, IChatUsedContext, IChatWarningMessage, ChatAgentVoteDirection } from 'vs/workbench/contrib/chat/common/chatService'; +import { ChatAgentVoteDirection, IChatCommandButton, IChatConfirmation, IChatContentReference, IChatFollowup, IChatProgressMessage, IChatResponseErrorDetails, IChatResponseProgressFileTreeData, IChatTask, IChatUsedContext, IChatWarningMessage } from 'vs/workbench/contrib/chat/common/chatService'; import { countWords } from 'vs/workbench/contrib/chat/common/chatWordCounter'; import { CodeBlockModelCollection } from './codeBlockModelCollection'; -import { IMarkdownString } from 'vs/base/common/htmlContent'; export function isRequestVM(item: unknown): item is IChatRequestViewModel { return !!item && typeof item === 'object' && 'message' in item; @@ -369,9 +369,16 @@ export class ChatResponseViewModel extends Disposable implements IChatResponseVi } get username() { - return this.agent ? - (this.agent.fullName || this.agent.name) : - this._model.username; + if (this.agent) { + const isAllowed = this.chatAgentNameService.getAgentNameRestriction(this.agent); + if (isAllowed) { + return this.agent.fullName || this.agent.name; + } else { + return getFullyQualifiedId(this.agent); + } + } + + return this._model.username; } get avatarIcon() { @@ -471,6 +478,7 @@ export class ChatResponseViewModel extends Disposable implements IChatResponseVi constructor( private readonly _model: IChatResponseModel, @ILogService private readonly logService: ILogService, + @IChatAgentNameService private readonly chatAgentNameService: IChatAgentNameService, ) { super();