Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions .eslint-allowed-bracket-notation-files
Original file line number Diff line number Diff line change
Expand Up @@ -463,19 +463,6 @@ src/vs/workbench/services/telemetry/common/workbenchCommonProperties.ts
src/vs/workbench/services/telemetry/test/browser/commonProperties.test.ts
src/vs/workbench/services/telemetry/test/node/commonProperties.test.ts

# Remote, tunnels, networking, and authentication (11 files)
extensions/github-authentication/src/test/github.test.ts
extensions/microsoft-authentication/src/common/experimentation.ts
src/vs/platform/github/common/githubTransport.ts
src/vs/platform/remote/node/wsl.ts
src/vs/platform/request/node/requestService.ts
src/vs/platform/tunnel/node/tunnelProxy.ts
src/vs/platform/tunnel/test/node/tunnelProxy.test.ts
src/vs/platform/webContentExtractor/electron-main/webPageLoader.ts
src/vs/platform/webContentExtractor/test/electron-main/webPageLoader.test.ts
src/vs/workbench/contrib/remote/browser/remoteStartEntry.ts
src/vs/workbench/contrib/remoteTunnel/test/electron-browser/remoteTunnel.contribution.test.ts

# Language feature extensions (22 files)
extensions/css-language-features/client/src/node/cssClientMain.ts
extensions/css-language-features/server/src/cssServer.ts
Expand Down
187 changes: 102 additions & 85 deletions extensions/github-authentication/src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,102 @@ function generateSessionId(): string {
return crypto.getRandomValues(new Uint32Array(2)).reduce((prev, curr) => prev += curr.toString(16), '');
}

/**
* Reads and verifies the sessions persisted in the authentication keychain.
*/
export async function readSessionsFromKeychain(
keychain: Pick<Keychain, 'getToken' | 'deleteToken'>,
githubServer: Pick<IGitHubServer, 'getUserInfo'>,
logger: Pick<Log, 'error' | 'info' | 'trace'>,
storeSessions: (sessions: vscode.AuthenticationSession[]) => Promise<void>
): Promise<vscode.AuthenticationSession[]> {
let sessionData: SessionData[];
try {
logger.info('Reading sessions from keychain...');
const storedSessions = await keychain.getToken();
if (!storedSessions) {
return [];
}
logger.info('Got stored sessions!');

try {
sessionData = JSON.parse(storedSessions);
} catch (e) {
await keychain.deleteToken();
throw e;
}
} catch (e) {
logger.error(`Error reading token: ${e}`);
return [];
}

// Unfortunately, we were using a number secretly for the account id for some time... this is due to a bad `any`.
// AuthenticationSession's account id is a string, so we need to detect when there is a number accountId and re-store
// the sessions to migrate away from the bad number usage.
// TODO@TylerLeonhardt: Remove this after we are confident that all users have migrated to the new id.
let seenNumberAccountId: boolean = false;
// TODO: eventually remove this Set because we should only have one session per set of scopes.
const scopesSeen = new Set<string>();
const sessionPromises = sessionData.map(async (session: SessionData): Promise<vscode.AuthenticationSession | undefined> => {
// For GitHub scope list, order doesn't matter so we immediately sort the scopes
const scopesStr = [...session.scopes].sort().join(' ');
let userInfo: IGitHubUserInfo | undefined;
if (!session.account) {
try {
userInfo = await githubServer.getUserInfo(session.accessToken);
logger.info(`Verified session with the following scopes: ${scopesStr}`);
} catch (e) {
if (e.message === 'Unauthorized') {
return undefined;
}
}
}

logger.trace(`Read the following session from the keychain with the following scopes: ${scopesStr}`);
scopesSeen.add(scopesStr);

let accountId: string;
if (session.account?.id) {
if (typeof session.account.id === 'number') {
seenNumberAccountId = true;
}
accountId = `${session.account.id}`;
} else {
accountId = userInfo?.id ?? '<unknown>';
}
const icon = session.account?.icon
? vscode.Uri.from(session.account.icon)
: userInfo?.avatarUrl ? vscode.Uri.parse(userInfo.avatarUrl) : undefined;
return {
id: session.id,
account: {
label: session.account
? session.account.label ?? session.account.displayName ?? '<unknown>'
: (userInfo?.accountName ?? '<unknown>'),
id: accountId,
icon,
},
// we set this to session.scopes to maintain the original order of the scopes requested
// by the extension that called getSession()
scopes: session.scopes,
accessToken: session.accessToken
};
});

const verifiedSessions = (await Promise.allSettled(sessionPromises))
.filter(p => p.status === 'fulfilled')
.map(p => (p as PromiseFulfilledResult<vscode.AuthenticationSession | undefined>).value)
.filter(<T>(p?: T): p is T => Boolean(p));

logger.info(`Got ${verifiedSessions.length} verified sessions.`);
// Account data discovered during reads must not trigger a secret write because web embedders can re-expose accountless sessions.
if (seenNumberAccountId || verifiedSessions.length !== sessionData.length) {
await storeSessions(verifiedSessions);
}

return verifiedSessions;
}

export class GitHubAuthenticationProvider implements vscode.AuthenticationProvider, vscode.Disposable {
private readonly _sessionChangeEmitter = new vscode.EventEmitter<vscode.AuthenticationProviderAuthenticationSessionsChangeEvent>();
private readonly _logger: Log;
Expand Down Expand Up @@ -617,91 +713,12 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid
}

private async readSessions(): Promise<vscode.AuthenticationSession[]> {
let sessionData: SessionData[];
try {
this._logger.info('Reading sessions from keychain...');
const storedSessions = await this._keychain.getToken();
if (!storedSessions) {
return [];
}
this._logger.info('Got stored sessions!');

try {
sessionData = JSON.parse(storedSessions);
} catch (e) {
await this._keychain.deleteToken();
throw e;
}
} catch (e) {
this._logger.error(`Error reading token: ${e}`);
return [];
}

// Unfortunately, we were using a number secretly for the account id for some time... this is due to a bad `any`.
// AuthenticationSession's account id is a string, so we need to detect when there is a number accountId and re-store
// the sessions to migrate away from the bad number usage.
// TODO@TylerLeonhardt: Remove this after we are confident that all users have migrated to the new id.
let seenNumberAccountId: boolean = false;
// TODO: eventually remove this Set because we should only have one session per set of scopes.
const scopesSeen = new Set<string>();
const sessionPromises = sessionData.map(async (session: SessionData): Promise<vscode.AuthenticationSession | undefined> => {
// For GitHub scope list, order doesn't matter so we immediately sort the scopes
const scopesStr = [...session.scopes].sort().join(' ');
let userInfo: IGitHubUserInfo | undefined;
if (!session.account) {
try {
userInfo = await this._githubServer.getUserInfo(session.accessToken);
this._logger.info(`Verified session with the following scopes: ${scopesStr}`);
} catch (e) {
if (e.message === 'Unauthorized') {
return undefined;
}
}
}

this._logger.trace(`Read the following session from the keychain with the following scopes: ${scopesStr}`);
scopesSeen.add(scopesStr);

let accountId: string;
if (session.account?.id) {
if (typeof session.account.id === 'number') {
seenNumberAccountId = true;
}
accountId = `${session.account.id}`;
} else {
accountId = userInfo?.id ?? '<unknown>';
}
const icon = session.account?.icon
? vscode.Uri.from(session.account.icon)
: userInfo?.avatarUrl ? vscode.Uri.parse(userInfo.avatarUrl) : undefined;
return {
id: session.id,
account: {
label: session.account
? session.account.label ?? session.account.displayName ?? '<unknown>'
: (userInfo?.accountName ?? '<unknown>'),
id: accountId,
icon,
},
// we set this to session.scopes to maintain the original order of the scopes requested
// by the extension that called getSession()
scopes: session.scopes,
accessToken: session.accessToken
};
});

const verifiedSessions = (await Promise.allSettled(sessionPromises))
.filter(p => p.status === 'fulfilled')
.map(p => (p as PromiseFulfilledResult<vscode.AuthenticationSession | undefined>).value)
.filter(<T>(p?: T): p is T => Boolean(p));

this._logger.info(`Got ${verifiedSessions.length} verified sessions.`);
// Account data discovered during reads must not trigger a secret write because web embedders can re-expose accountless sessions.
if (seenNumberAccountId || verifiedSessions.length !== sessionData.length) {
await this.storeSessions(verifiedSessions);
}

return verifiedSessions;
return readSessionsFromKeychain(
this._keychain,
this._githubServer,
this._logger,
sessions => this.storeSessions(sessions)
);
}

private async storeSessions(sessions: vscode.AuthenticationSession[]): Promise<void> {
Expand Down
77 changes: 30 additions & 47 deletions extensions/github-authentication/src/test/github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,9 @@ import { AccountLinks } from '../common/accountLinks';
import { IGitHubUserInfo } from '../common/gitHubAccount';
import { Log } from '../common/logger';
import { EntraTokenExchangeError, EntraTokenExchangeFailure, IEntraRenewal, IEntraRenewedToken } from '../entraTokenExchange';
import { AuthProviderType, GitHubAuthenticationProvider } from '../github';
import { AuthProviderType, GitHubAuthenticationProvider, readSessionsFromKeychain } from '../github';
import { TestMemento } from './testMemento';

interface TestGitHubAuthenticationProvider {
readonly _keychain: {
getToken(): Promise<string>;
deleteToken(): Promise<void>;
};
readonly _githubServer: {
getUserInfo(token: string): Promise<{ id: string; accountName: string; avatarUrl: string | undefined }>;
};
readonly _logger: {
error(message: string): void;
info(message: string): void;
trace(message: string): void;
};
readSessions(): Promise<vscode.AuthenticationSession[]>;
storeSessions(sessions: vscode.AuthenticationSession[]): Promise<void>;
}

suite('GitHub session persistence', () => {
test('does not loop when Codespaces secret storage drops hydrated account data', async () => {
const storedSessions = JSON.stringify([{
Expand All @@ -40,38 +23,38 @@ suite('GitHub session persistence', () => {
let secretWrites = 0;
const secretChangeReads: Promise<vscode.AuthenticationSession[]>[] = [];

const provider: TestGitHubAuthenticationProvider = {
_keychain: {
getToken: async () => storedSessions,
deleteToken: async () => { }
},
_githubServer: {
getUserInfo: async _token => {
userInfoRequests++;
return {
id: 'account-id',
accountName: 'octocat',
avatarUrl: 'https://avatars.githubusercontent.com/u/1'
};
}
},
_logger: {
error: _message => { },
info: _message => { },
trace: _message => { }
},
readSessions: GitHubAuthenticationProvider.prototype['readSessions'],
storeSessions: async _sessions => {
secretWrites++;
// Browser secret storage fires a change event for every write. The Codespaces
// provider then exposes the original accountless session on the next read.
if (secretWrites === 1) {
secretChangeReads.push(provider.readSessions());
}
const keychain = {
getToken: async () => storedSessions,
deleteToken: async () => { }
};
const githubServer = {
getUserInfo: async (_token: string) => {
userInfoRequests++;
return {
id: 'account-id',
accountName: 'octocat',
avatarUrl: 'https://avatars.githubusercontent.com/u/1'
};
}
};
const logger = {
error: (_message: string) => { },
info: (_message: string) => { },
trace: (_message: string) => { }
};
async function storeSessions(_sessions: vscode.AuthenticationSession[]): Promise<void> {
secretWrites++;
// Browser secret storage fires a change event for every write. The Codespaces
// provider then exposes the original accountless session on the next read.
if (secretWrites === 1) {
secretChangeReads.push(readSessions());
}
}
function readSessions(): Promise<vscode.AuthenticationSession[]> {
return readSessionsFromKeychain(keychain, githubServer, logger, storeSessions);
}

const sessions = await provider.readSessions();
const sessions = await readSessions();
await Promise.all(secretChangeReads);

assert.deepStrictEqual({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export async function createExperimentationService(
isPreRelease: boolean,
): Promise<IExperimentationService> {
const id = context.extension.id;
const version = context.extension.packageJSON['version'];
const version = context.extension.packageJSON.version;

const service = getExperimentationService(
id,
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/github/common/githubTransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export class GitHubTransport extends Disposable implements IGitHubTransport {
'X-GitHub-Api-Version': defaultApiVersion,
};
if (authenticated) {
headers['Authorization'] = `Bearer ${token}`;
headers.Authorization = `Bearer ${token}`;
}
let response: Response;
try {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/remote/node/wsl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ async function testWSLFeatureInstalled(): Promise<boolean> {
}

function getSystem32Path(subPath: string): string | undefined {
const systemRoot = process.env['SystemRoot'];
const systemRoot = process.env.SystemRoot;
if (systemRoot) {
const is32ProcessOn64Windows = process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432');
return join(systemRoot, is32ProcessOn64Windows ? 'Sysnative' : 'System32', subPath);
Expand Down
4 changes: 2 additions & 2 deletions src/vs/platform/request/node/requestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ async function nodeRequestAttempt(options: NodeRequestOptions, token: Cancellati

const req = rawRequest(opts, (res: http.IncomingMessage) => {
const followRedirects: number = isNumber(options.followRedirects) ? options.followRedirects : 3;
if (res.statusCode && res.statusCode >= 300 && res.statusCode < 400 && followRedirects > 0 && res.headers['location']) {
if (res.statusCode && res.statusCode >= 300 && res.statusCode < 400 && followRedirects > 0 && res.headers.location) {
nodeRequest({
...options,
url: res.headers['location'],
url: res.headers.location,
followRedirects: followRedirects - 1
}, token).then(resolve, reject);
} else {
Expand Down
14 changes: 7 additions & 7 deletions src/vs/platform/tunnel/node/tunnelProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,21 +345,21 @@ export class TunnelProxy extends Disposable {
// An intermediary MUST parse the Connection header and remove any
// fields named in it, then remove Connection itself. It SHOULD
// also remove other known hop-by-hop headers.
const connectionTokens = (headers['connection'] ?? '')
const connectionTokens = (headers.connection ?? '')
.toString()
.split(',')
.map(t => t.trim().toLowerCase())
.filter(t => t.length > 0);
for (const token of connectionTokens) {
delete headers[token];
}
delete headers['connection'];
delete headers.connection;
delete headers['keep-alive'];
delete headers['proxy-authorization'];
delete headers['proxy-connection'];
delete headers['te'];
delete headers.te;
delete headers['transfer-encoding'];
delete headers['upgrade'];
delete headers.upgrade;

const proxyReq = http.request({
agent: this._tunnelAgent,
Expand Down Expand Up @@ -472,12 +472,12 @@ export class TunnelProxy extends Disposable {
// Handle IPv6 bracket notation [::1]:port
const bracketMatch = /^\[(?<host>[^\]]+)\]:(?<port>\d+)$/.exec(address);
if (bracketMatch?.groups) {
host = bracketMatch.groups['host'];
port = parseInt(bracketMatch.groups['port'], 10);
host = bracketMatch.groups.host;
port = parseInt(bracketMatch.groups.port, 10);
} else {
const bracketOnly = /^\[(?<host>[^\]]+)\]$/.exec(address);
if (bracketOnly?.groups) {
host = bracketOnly.groups['host'];
host = bracketOnly.groups.host;
port = defaultPort;
} else {
const lastColon = address.lastIndexOf(':');
Expand Down
Loading