Validate some untrusted GitHub API responses using validator system

src/platform/configuration/test/common/validator.spec.ts

@@ -159,4 +159,4 @@ describe('vRequired', () => {
 		const result2 = validator.validate({ requiredField: "test", optionalField: null });
 		expect(result2.error).toBeDefined();
 	});
-});
+});

src/platform/github/common/githubAPI.ts

@@ -6,6 +6,7 @@
 import { ILogService } from '../../log/common/logService';
 import { IFetcherService } from '../../networking/common/fetcherService';
 import { ITelemetryService } from '../../telemetry/common/telemetry';
+import { vClosePullRequestResponse } from './githubAPIValidators';
 
 export interface PullRequestSearchItem {
 	id: string;
@@ -78,6 +79,10 @@ export interface PullRequestComment {
 	url: string;
 }
 
+export interface ClosePullRequestResponse {
+	state: string;
+}
+
 export async function makeGitHubAPIRequest(
 	fetcherService: IFetcherService,
 	logService: ILogService,
@@ -353,11 +358,22 @@ export async function closePullRequest(
 		'2022-11-28'
 	);
 
-	const success = result?.state === 'closed';
+	if (!result) {
+		logService.error(`[GitHubAPI] Failed to close pull request ${owner}/${repo}#${pullNumber}: No response`);
+		return false;
+	}
+
+	const validation = vClosePullRequestResponse().validate(result);
+	if (validation.error) {
+		logService.error(`[GitHubAPI] Failed to validate close pull request response: ${validation.error.message}`);
+		return false;
+	}
+
+	const success = validation.content.state === 'closed';
 	if (success) {
 		logService.debug(`[GitHubAPI] Successfully closed pull request ${owner}/${repo}#${pullNumber}`);
 	} else {
-		logService.error(`[GitHubAPI] Failed to close pull request ${owner}/${repo}#${pullNumber}. Its state is ${result?.state}`);
+		logService.error(`[GitHubAPI] Failed to close pull request ${owner}/${repo}#${pullNumber}. Its state is ${validation.content.state}`);
 	}
 	return success;
 }

src/platform/github/common/githubAPIValidators.ts

@@ -0,0 +1,62 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { IValidator, vArray, vNumber, vObj, vString } from '../../configuration/common/validator';
+import { ClosePullRequestResponse } from './githubAPI';
+import { CustomAgentListItem, JobInfo } from './githubService';
+
+const vActor = () => vObj({
+	id: vNumber(),
+	login: vString(),
+});
+
+export const vJobInfo = (): IValidator<JobInfo> => vObj({
+	job_id: vString(),
+	session_id: vString(),
+	problem_statement: vString(),
+	content_filter_mode: vString(),
+	status: vString(),
+	result: vString(),
+	actor: vActor(),
+	created_at: vString(),
+	updated_at: vString(),
+	pull_request: vObj({
+		id: vNumber(),
+		number: vNumber(),
+	}),
+	workflow_run: vObj({
+		id: vNumber(),
+	}),
+	error: vObj({
+		message: vString(),
+	}),
+	event_type: vString(),
+	event_url: vString(),
+	event_identifiers: vArray(vString()),
+});
+
+export const vCustomAgentListItem = (): IValidator<CustomAgentListItem> => vObj({
+	name: vString(),
+	repo_owner_id: vNumber(),
+	repo_owner: vString(),
+	repo_id: vNumber(),
+	repo_name: vString(),
+	display_name: vString(),
+	description: vString(),
+	tools: vArray(vString()),
+	version: vString(),
+});
+
+export interface GetCustomAgentsResponse {
+	agents: CustomAgentListItem[];
+}
+
+export const vGetCustomAgentsResponse = (): IValidator<GetCustomAgentsResponse> => vObj({
+	agents: vArray(vCustomAgentListItem()),
+});
+
+export const vClosePullRequestResponse = (): IValidator<ClosePullRequestResponse> => vObj({
+	state: vString(),
+});

src/platform/github/common/githubService.ts

@@ -11,6 +11,7 @@ import { ILogService } from '../../log/common/logService';
 import { IFetcherService } from '../../networking/common/fetcherService';
 import { ITelemetryService } from '../../telemetry/common/telemetry';
 import { addPullRequestCommentGraphQLRequest, closePullRequest, getPullRequestFromGlobalId, makeGitHubAPIRequest, makeGitHubAPIRequestWithPagination, makeSearchGraphQLRequest, PullRequestComment, PullRequestSearchItem, SessionInfo } from './githubAPI';
+import { vGetCustomAgentsResponse, vJobInfo } from './githubAPIValidators';
 
 export type IGetRepositoryInfoResponseData = Endpoints["GET /repos/{owner}/{repo}"]["response"]["data"];
 
@@ -272,12 +273,16 @@ export class BaseOctoKitService {
 	constructor(
 		private readonly _capiClientService: ICAPIClientService,
 		private readonly _fetcherService: IFetcherService,
-		private readonly _logService: ILogService,
+		protected readonly _logService: ILogService,
 		private readonly _telemetryService: ITelemetryService
 	) { }
 
 	async getCurrentAuthedUserWithToken(token: string): Promise<IOctoKitUser | undefined> {
-		return this._makeGHAPIRequest('user', 'GET', token);
+		const response = await this._makeGHAPIRequest('user', 'GET', token);
+		if (!response) {
+			return undefined;
+		}
+		return response as IOctoKitUser;
 	}
 
 	async getTeamMembershipWithToken(teamId: number, token: string, username: string): Promise<any | undefined> {
@@ -310,11 +315,29 @@ export class BaseOctoKitService {
 	}
 
 	protected async getJobByJobIdWithToken(owner: string, repo: string, jobId: string, userAgent: string, token: string): Promise<JobInfo> {
-		return makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/v1/jobs/${owner}/${repo}/${jobId}`, 'GET', token, undefined, undefined, undefined, userAgent);
+		const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/v1/jobs/${owner}/${repo}/${jobId}`, 'GET', token, undefined, undefined, undefined, userAgent);
+		if (!response) {
+			throw new Error('Failed to get job by job ID');
+		}
+		const validation = vJobInfo().validate(response);
+		if (validation.error) {
+			this._logService.error(`[GitHubService] Failed to validate job info response: ${validation.error.message}`);
+			throw new Error(`Invalid job info response: ${validation.error.message}`);
+		}
+		return validation.content;
 	}
 
 	protected async getJobBySessionIdWithToken(owner: string, repo: string, sessionId: string, userAgent: string, token: string): Promise<JobInfo> {
-		return makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/v1/jobs/${owner}/${repo}/session/${sessionId}`, 'GET', token, undefined, undefined, undefined, userAgent);
+		const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/v1/jobs/${owner}/${repo}/session/${sessionId}`, 'GET', token, undefined, undefined, undefined, userAgent);
+		if (!response) {
+			throw new Error('Failed to get job by session ID');
+		}
+		const validation = vJobInfo().validate(response);
+		if (validation.error) {
+			this._logService.error(`[GitHubService] Failed to validate job info response: ${validation.error.message}`);
+			throw new Error(`Invalid job info response: ${validation.error.message}`);
+		}
+		return validation.content;
 	}
 
 	protected async addPullRequestCommentWithToken(pullRequestId: string, commentBody: string, token: string): Promise<PullRequestComment | null> {
@@ -331,12 +354,24 @@ export class BaseOctoKitService {
 
 	protected async getCustomAgentsWithToken(owner: string, repo: string, token: string): Promise<GetCustomAgentsResponse> {
 		const queryParams = '?exclude_invalid_config=true';
-		return makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/custom-agents/${owner}/${repo}${queryParams}`, 'GET', token, undefined, undefined, 'json', 'vscode-copilot-chat');
+		const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.githubcopilot.com', `agents/swe/custom-agents/${owner}/${repo}${queryParams}`, 'GET', token, undefined, undefined, 'json', 'vscode-copilot-chat');
+		if (!response) {
+			return { agents: [] };
+		}
+		const validation = vGetCustomAgentsResponse().validate(response);
+		if (validation.error) {
+			this._logService.error(`[GitHubService] Failed to validate custom agents response: ${validation.error.message}`);
+			return { agents: [] };
+		}
+		return validation.content;
 	}
 
 	protected async getPullRequestFilesWithToken(owner: string, repo: string, pullNumber: number, token: string): Promise<PullRequestFile[]> {
 		const result = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, this._capiClientService.dotcomAPIURL, `repos/${owner}/${repo}/pulls/${pullNumber}/files`, 'GET', token, undefined, '2022-11-28');
-		return result || [];
+		if (!result) {
+			return [];
+		}
+		return result as PullRequestFile[];
 	}
 
 	protected async closePullRequestWithToken(owner: string, repo: string, pullNumber: number, token: string): Promise<boolean> {
@@ -346,8 +381,13 @@ export class BaseOctoKitService {
 	protected async getFileContentWithToken(owner: string, repo: string, ref: string, path: string, token: string): Promise<string> {
 		const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, this._capiClientService.dotcomAPIURL, `repos/${owner}/${repo}/contents/${path}?ref=${encodeURIComponent(ref)}`, 'GET', token, undefined);
 
-		if (response?.content && response.encoding === 'base64') {
-			return decodeBase64(response.content.replace(/\n/g, '')).toString();
+		if (!response) {
+			return '';
+		}
+
+		const fileContent = response as { content: string; encoding: string };
+		if (fileContent.encoding === 'base64') {
+			return decodeBase64(fileContent.content.replace(/\n/g, '')).toString();
 		} else {
 			return '';
 		}

src/platform/github/common/octoKitServiceImpl.ts

@@ -54,8 +54,11 @@ export class OctoKitService extends BaseOctoKitService implements IOctoKitServic
 			prId,
 			authToken,
 		);
-		const { sessions } = response;
-		return sessions;
+		if (!response) {
+			return [];
+		}
+		const sessionsResponse = response as { sessions: SessionInfo[] };
+		return sessionsResponse.sessions;
 	}
 
 	async getSessionLogs(sessionId: string): Promise<string> {
@@ -67,7 +70,10 @@ export class OctoKitService extends BaseOctoKitService implements IOctoKitServic
 			sessionId,
 			authToken,
 		);
-		return response;
+		if (!response) {
+			return '';
+		}
+		return response as string;
 	}
 
 	async getSessionInfo(sessionId: string): Promise<SessionInfo> {
@@ -79,18 +85,34 @@ export class OctoKitService extends BaseOctoKitService implements IOctoKitServic
 			sessionId,
 			authToken,
 		);
+		if (!response) {
+			throw new Error('No session info response received');
+		}
+
+		// The response might be a string (JSON) or already parsed
+		let parsedResponse: SessionInfo = response as SessionInfo;
 		if (typeof response === 'string') {
-			return JSON.parse(response) as SessionInfo;
+			try {
+				parsedResponse = JSON.parse(response) as SessionInfo;
+			} catch (e) {
+				throw new Error('Failed to parse session info response');
+			}
 		}
-		return response;
+
+		return parsedResponse;
 	}
 
 	async postCopilotAgentJob(owner: string, name: string, apiVersion: string, payload: RemoteAgentJobPayload): Promise<RemoteAgentJobResponse | ErrorResponseWithStatusCode> {
 		const authToken = (await this._authService.getPermissiveGitHubSession({ createIfNone: true }))?.accessToken;
 		if (!authToken) {
 			throw new Error('No authentication token available');
 		}
-		return this.postCopilotAgentJobWithToken(owner, name, apiVersion, 'vscode-copilot-chat', payload, authToken);
+		const response = await this.postCopilotAgentJobWithToken(owner, name, apiVersion, 'vscode-copilot-chat', payload, authToken);
+		if (!response) {
+			throw new Error('No response received from post copilot agent job');
+		}
+
+		return response as RemoteAgentJobResponse | ErrorResponseWithStatusCode;
 	}
 
 	async getJobByJobId(owner: string, repo: string, jobId: string, userAgent: string): Promise<JobInfo> {

src/platform/github/node/githubRepositoryService.ts

@@ -26,7 +26,13 @@ export class GithubRepositoryService implements IGithubRepositoryService {
 	private async _doGetRepositoryInfo(owner: string, repo: string): Promise<IGetRepositoryInfoResponseData | undefined> {
 		const authToken: string | undefined = this._authenticationService.permissiveGitHubSession?.accessToken ?? this._authenticationService.anyGitHubSession?.accessToken;
 
-		return makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.github.com', `repos/${owner}/${repo}`, 'GET', authToken);
+		const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.github.com', `repos/${owner}/${repo}`, 'GET', authToken);
+		if (!response) {
+			return undefined;
+		}
+
+		// IGetRepositoryInfoResponseData is from @octokit/types which is a well-defined external API contract
+		return response as IGetRepositoryInfoResponseData;
 	}
 
 	async getRepositoryInfo(owner: string, repo: string) {
@@ -59,24 +65,22 @@ export class GithubRepositoryService implements IGithubRepositoryService {
 			const encodedPath = path.split('/').map((segment) => encodeURIComponent(segment)).join('/');
 			const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.github.com', `repos/${org}/${repo}/contents/${encodedPath}`, 'GET', authToken);
 
-			if (response.ok) {
-				const data = (await response.json());
-				if (Array.isArray(data)) {
-					for (const child of data) {
-						if ('name' in child && 'path' in child && 'type' in child && 'html_url' in child) {
-							paths.push({ name: child.name, path: child.path, type: child.type, html_url: child.html_url });
-							if (child.type === 'dir') {
-								paths.push(...await this.getRepositoryItems(org, repo, child.path));
-							}
-						}
-					}
-				}
-			} else {
-				console.error(`Failed to fetch contents from ${org}:${repo}:${path}`);
+			if (!response) {
+				this._logService.error(`Failed to fetch contents from ${org}:${repo}:${path}`);
 				return [];
 			}
-		} catch {
-			console.error(`Failed to fetch contents from ${org}:${repo}:${path}`);
+
+			// Response should be an array of repository items
+			const items = response as Array<{ name: string; path: string; type: 'file' | 'dir'; html_url: string }>;
+
+			for (const child of items) {
+				paths.push({ name: child.name, path: child.path, type: child.type, html_url: child.html_url });
+				if (child.type === 'dir') {
+					paths.push(...await this.getRepositoryItems(org, repo, child.path));
+				}
+			}
+		} catch (e) {
+			this._logService.error(`Failed to fetch contents from ${org}:${repo}:${path}: ${e}`);
 			return [];
 		}
 		return paths;
@@ -88,18 +92,17 @@ export class GithubRepositoryService implements IGithubRepositoryService {
 			const encodedPath = path.split('/').map((segment) => encodeURIComponent(segment)).join('/');
 			const response = await makeGitHubAPIRequest(this._fetcherService, this._logService, this._telemetryService, 'https://api.github.com', `repos/${org}/${repo}/contents/${encodedPath}`, 'GET', authToken);
 
-			if (response.ok) {
-
-				const data = (await response.json());
-
-				if ('content' in data) {
-					const content = Buffer.from(data.content, 'base64');
-					return new Uint8Array(content);
-				}
-				throw new Error('Unexpected data from GitHub');
+			if (!response) {
+				this._logService.error(`Failed to fetch contents from ${org}:${repo}:${path}`);
+				return undefined;
 			}
-		} catch {
-			console.error(`Failed to contents from ${org}:${repo}:${path}`);
+
+			const fileContent = response as { content: string };
+			const content = Buffer.from(fileContent.content, 'base64');
+			return new Uint8Array(content);
+		} catch (e) {
+			this._logService.error(`Failed to fetch contents from ${org}:${repo}:${path}: ${e}`);
+			return undefined;
 		}
 	}
 }