Skip to content

Commit

Permalink
Update trusted domains based on workspaace remotes
Browse files Browse the repository at this point in the history 
Closes #97532
  • Loading branch information
Jackson Kearl committed May 12, 2020
1 parent 1636e54 commit 288852d
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 14 deletions.
45 changes: 43 additions & 2 deletions src/vs/workbench/contrib/url/browser/trustedDomains.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import { IStorageService, StorageScope } from 'vs/platform/storage/common/storag
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IAuthenticationService } from 'vs/workbench/services/authentication/browser/authenticationService';
import { IFileService } from 'vs/platform/files/common/files';
import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles';
import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';

const TRUSTED_DOMAINS_URI = URI.parse('trustedDomains:/Trusted Domains');

Expand Down Expand Up @@ -117,7 +120,42 @@ export async function configureOpenerTrustedDomainsHandler(
return [];
}

export async function readTrustedDomains(storageService: IStorageService, productService: IProductService, authenticationService: IAuthenticationService) {
async function getRemotes(fileService: IFileService, textFileService: ITextFileService, contextService: IWorkspaceContextService): Promise<string[]> {
const workspaceUris = contextService.getWorkspace().folders.map(folder => folder.uri);
const domains = await Promise.all<string[]>(workspaceUris.map(async workspaceUri => {
const path = workspaceUri.path;
const uri = workspaceUri.with({ path: `${path !== '/' ? path : ''}/.git/config` });
const exists = await fileService.exists(uri);
if (!exists) {
return [];
}
const content = (await (textFileService.read(uri, { acceptTextOnly: true }).catch(() => ({ value: '' })))).value;
const domains = new Set<string>();
let match: RegExpExecArray | null;

const RemoteMatcher = /^\s*url\s*=\s*(?:git@|https:\/\/)([^:\/]*)(?::|\/)([^.]*)\.git\s*$/mg;
while (match = RemoteMatcher.exec(content)) {
const [domain, repo] = [match[1], match[2]];
if (domain && repo) {
domains.add(`https://${domain}/${repo}/`);
}
}
return [...domains];
}));

const set = domains.reduce((set, list) => list.reduce((set, item) => set.add(item), set), new Set<string>());
return [...set];
}

export async function readTrustedDomains(accessor: ServicesAccessor) {

const storageService = accessor.get(IStorageService);
const productService = accessor.get(IProductService);
const authenticationService = accessor.get(IAuthenticationService);
const fileService = accessor.get(IFileService);
const textFileService = accessor.get(ITextFileService);
const workspaceContextService = accessor.get(IWorkspaceContextService);

const defaultTrustedDomains: string[] = productService.linkProtectionTrustedDomains
? [...productService.linkProtectionTrustedDomains]
: [];
Expand All @@ -134,9 +172,12 @@ export async function readTrustedDomains(storageService: IStorageService, produc
.map(session => session.account.displayName)
.map(username => `https://github.com/${username}/`);

const workspaceDomains = await getRemotes(fileService, textFileService, workspaceContextService);

return {
defaultTrustedDomains,
trustedDomains,
userDomains
userDomains,
workspaceDomains
};
}
22 changes: 14 additions & 8 deletions src/vs/workbench/contrib/url/browser/trustedDomainsFileSystemProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ import { IStorageService, StorageScope } from 'vs/platform/storage/common/storag
import { IWorkbenchContribution } from 'vs/workbench/common/contributions';
import { VSBuffer } from 'vs/base/common/buffer';
import { readTrustedDomains, TRUSTED_DOMAINS_CONTENT_STORAGE_KEY, TRUSTED_DOMAINS_STORAGE_KEY } from 'vs/workbench/contrib/url/browser/trustedDomains';
import { IProductService } from 'vs/platform/product/common/productService';
import { IStorageKeysSyncRegistryService } from 'vs/platform/userDataSync/common/storageKeys';
import { IAuthenticationService } from 'vs/workbench/services/authentication/browser/authenticationService';
import { assertIsDefined } from 'vs/base/common/types';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';

const TRUSTED_DOMAINS_SCHEMA = 'trustedDomains';

Expand Down Expand Up @@ -47,7 +46,7 @@ const CONFIG_PLACEHOLDER_TEXT = `[
// "https://microsoft.com"
]`;

function computeTrustedDomainContent(defaultTrustedDomains: string[], trustedDomains: string[], userTrustedDomains: string[]) {
function computeTrustedDomainContent(defaultTrustedDomains: string[], trustedDomains: string[], userTrustedDomains: string[], workspaceTrustedDomains: string[]) {
let content = CONFIG_HELP_TEXT_PRE;

if (defaultTrustedDomains.length > 0) {
Expand All @@ -58,13 +57,21 @@ function computeTrustedDomainContent(defaultTrustedDomains: string[], trustedDom
} else {
content += `// By default, VS Code trusts "localhost".\n`;
}

if (userTrustedDomains.length) {
content += `//\n// Additionally, the following domains are trusted based on your current GitHub login:\n`;
userTrustedDomains.forEach(d => {
content += `// - "${d}"\n`;
});
}

if (workspaceTrustedDomains.length) {
content += `//\n// Further, the following domains are trusted based on your workspace configuration:\n`;
workspaceTrustedDomains.forEach(d => {
content += `// - "${d}"\n`;
});
}

content += CONFIG_HELP_TEXT_AFTER;

if (trustedDomains.length === 0) {
Expand All @@ -85,9 +92,8 @@ export class TrustedDomainsFileSystemProvider implements IFileSystemProviderWith
constructor(
@IFileService private readonly fileService: IFileService,
@IStorageService private readonly storageService: IStorageService,
@IProductService private readonly productService: IProductService,
@IInstantiationService private readonly instantiationService: IInstantiationService,
@IStorageKeysSyncRegistryService private readonly storageKeysSyncRegistryService: IStorageKeysSyncRegistryService,
@IAuthenticationService private readonly authenticationService: IAuthenticationService,
) {
this.fileService.registerProvider(TRUSTED_DOMAINS_SCHEMA, this);

Expand All @@ -105,14 +111,14 @@ export class TrustedDomainsFileSystemProvider implements IFileSystemProviderWith
StorageScope.GLOBAL
);

const { defaultTrustedDomains, trustedDomains, userDomains } = await readTrustedDomains(this.storageService, this.productService, this.authenticationService);
const { defaultTrustedDomains, trustedDomains, userDomains, workspaceDomains } = await this.instantiationService.invokeFunction(readTrustedDomains);
if (
!trustedDomainsContent ||
trustedDomainsContent.indexOf(CONFIG_HELP_TEXT_PRE) === -1 ||
trustedDomainsContent.indexOf(CONFIG_HELP_TEXT_AFTER) === -1 ||
[...defaultTrustedDomains, ...trustedDomains, ...userDomains].some(d => !assertIsDefined(trustedDomainsContent).includes(d))
[...defaultTrustedDomains, ...trustedDomains, ...userDomains, ...workspaceDomains].some(d => !assertIsDefined(trustedDomainsContent).includes(d))
) {
trustedDomainsContent = computeTrustedDomainContent(defaultTrustedDomains, trustedDomains, userDomains);
trustedDomainsContent = computeTrustedDomainContent(defaultTrustedDomains, trustedDomains, userDomains, workspaceDomains);
}

const buffer = VSBuffer.fromString(trustedDomainsContent).buffer;
Expand Down
8 changes: 4 additions & 4 deletions src/vs/workbench/contrib/url/browser/trustedDomainsValidator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
import { IClipboardService } from 'vs/platform/clipboard/common/clipboardService';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IAuthenticationService } from 'vs/workbench/services/authentication/browser/authenticationService';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';

type TrustedDomainsDialogActionClassification = {
action: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
Expand All @@ -36,7 +36,7 @@ export class OpenerValidatorContributions implements IWorkbenchContribution {
@IEditorService private readonly _editorService: IEditorService,
@IClipboardService private readonly _clipboardService: IClipboardService,
@ITelemetryService private readonly _telemetryService: ITelemetryService,
@IAuthenticationService private readonly _authenticationService: IAuthenticationService,
@IInstantiationService private readonly _instantiationService: IInstantiationService,
) {
this._openerService.registerValidator({ shouldOpen: r => this.validateLink(r) });
}
Expand All @@ -52,8 +52,8 @@ export class OpenerValidatorContributions implements IWorkbenchContribution {
const { scheme, authority, path, query, fragment } = resource;

const domainToOpen = `${scheme}://${authority}`;
const { defaultTrustedDomains, trustedDomains, userDomains } = await readTrustedDomains(this._storageService, this._productService, this._authenticationService);
const allTrustedDomains = [...defaultTrustedDomains, ...trustedDomains, ...userDomains];
const { defaultTrustedDomains, trustedDomains, userDomains, workspaceDomains } = await this._instantiationService.invokeFunction(readTrustedDomains);
const allTrustedDomains = [...defaultTrustedDomains, ...trustedDomains, ...userDomains, ...workspaceDomains];

if (isURLDomainTrusted(resource, allTrustedDomains)) {
return true;
Expand Down

8 comments on commit 288852d

@JacksonKearl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @joaomoreno not sure if this would be better in the git extension? 288852d#diff-5251dce8e7eb39c156561693ac352eafR123-R148

We do similar stuff in

vscode/src/vs/workbench/contrib/tags/electron-browser/workspaceTags.ts

Line 105 in 5b33ba2

private reportRemoteDomains(workspaceUris: URI[]): void {
and

vscode/src/vs/workbench/contrib/tags/electron-browser/workspaceTagsService.ts

Line 139 in 7bc587d

getHashedRemotesFromUri(workspaceUri: URI, stripEndingDotGit: boolean = false): Promise<string[]> {

@joaomoreno
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacksonKearl The git extension already exposes an API, including the remotes of its repos.

@JacksonKearl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno could you possibly point me to how I can use that?

@joaomoreno
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... now that I read this better... you can't, since it's an extension API, so you'd have to access it from an extension.

I could give you a hacky getRemotes command via

vscode/extensions/git/src/api/api1.ts

Line 307 in 94428f8

export function registerAPICommands(extension: GitExtension): Disposable {

But not sure... maybe your current approach is good enough. I definitely would not like to add a dependency from the core to the git extension.

@JacksonKearl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can just leave it as-is. thanks!

@eamodio
Copy link
Contributor

@eamodio eamodio commented on 288852d Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really be looking up remotes on every attempt to open a url? Seems quite heavy imo. Also with custom FileSystemProviders, this can be problematic if the fs provider require authentication (which also requires opening a url). I hit this with implementing authentication for the new GitHub serverless fs provider. To deal with it, I've added a short timeout for now, but this needs a better fix: e3de1b8

@JacksonKearl
Copy link
Contributor

@JacksonKearl JacksonKearl commented on 288852d Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could cache per-workspace/session pretty easily, but that doesn't get around the auth problem.

I think it an approach like your patch could be fine if the results were cached and the timeout were made much smaller?

Though the whole point of this was to make codespaces easier to use, so if we cant access the fs without auth there... that's not a great situation.

@eamodio
Copy link
Contributor

@eamodio eamodio commented on 288852d Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a smaller timeout would be better, but it would be a strange user experience if sometimes urls would prompt and other times not. Maybe not a big deal if we cached the results (even if we stopped waiting for that instance), then in theory only the first time you could get prompted. Although, the whole trusted domains thing feels strange to me, I'm not sure what security is gained by not opening a url (especially if it is http/https)

Please sign in to comment.