diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 8eb1402923..7b02b9b24b 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -49,15 +49,22 @@ import { import { SseConnection } from "../websocket/sseConnection"; import { getRefreshCommand, refreshCertificates } from "./certificateRefresh"; +import { + parseApiResponse, + VALIDATED_RESPONSES, + type ValidatedMethods, +} from "./responseValidation"; import { createHttpAgent } from "./utils"; import type { GetInboxNotificationResponse, + ProvisionerJob, ProvisionerJobLog, ServerSentEvent, Workspace, WorkspaceAgent, WorkspaceAgentLog, + WorkspaceBuild, } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; @@ -114,6 +121,7 @@ export class CoderApi extends Api implements vscode.Disposable { private readonly authConfigTracker: AuthConfigTracker, ) { super(); + wrapWithValidation(this); this.configWatcher = this.watchConfigChanges(); } @@ -149,6 +157,30 @@ export class CoderApi extends Api implements vscode.Disposable { return this.getAxiosInstance().defaults.baseURL; } + /** + * Reimplemented because the SDK version polls inside a voided IIFE that + * swallows errors, hanging callers forever if a poll throws (e.g. on + * failed response validation). + */ + override waitForBuild = async ( + build: WorkspaceBuild, + ): Promise => { + while (true) { + const { job } = await this.getWorkspaceBuildByNumber( + build.workspace_owner_name, + build.workspace_name, + build.build_number, + ); + if (job.status === "failed") { + throw new Error(`Build ${build.build_number} failed`); + } + if (job.status === "succeeded" || job.status === "canceled") { + return job; + } + await new Promise((resolve) => setTimeout(resolve, 1000)); + } + }; + hasAuthConfigChangedSince(version: number | undefined): boolean { return this.authConfigTracker.hasChangedSince(version); } @@ -747,6 +779,21 @@ function wrapResponseTransform( ]; } +/** + * Validate the fields the extension reads on each response, since the SDK + * casts bodies to the generated types with no runtime check. The methods + * are instance arrow properties, so wrapping is by reassignment; + * `override` fields would depend on declaration order. + */ +function wrapWithValidation(api: CoderApi): void { + const methods: ValidatedMethods = api; + for (const [name, schema] of VALIDATED_RESPONSES) { + const method = methods[name]; + methods[name] = async (...args) => + parseApiResponse(schema, await method(...args), name, api.getHost()); + } +} + function getSize(headers: AxiosHeaders, data: unknown): number | undefined { const contentLength = headers["content-length"] as unknown; if (typeof contentLength === "string") { diff --git a/src/api/responseValidation.ts b/src/api/responseValidation.ts new file mode 100644 index 0000000000..7991c75fb4 --- /dev/null +++ b/src/api/responseValidation.ts @@ -0,0 +1,122 @@ +import { z } from "zod"; + +import type { CoderApi } from "./coderApi"; + +/** + * Thrown when a 2xx response body does not match the shape the extension + * needs, which almost always means the URL does not point at a Coder + * deployment (a proxy error page, a different service, a partial body). + */ +export class InvalidApiResponseError extends Error { + constructor( + public readonly endpoint: string, + url: string | undefined, + options?: { cause?: unknown }, + ) { + super( + `${url ?? "The deployment"} did not return a valid Coder API response ` + + `for ${endpoint}. Check that the URL points to a Coder deployment.`, + options, + ); + this.name = "InvalidApiResponseError"; + } +} + +/** + * Validate a response body, returning the original value with the caller's + * type. + * + * Schemas list only the fields the extension reads and use looseObject so + * unknown fields pass through. Require a field only if every deployment back + * to Coder 0.25 sends it; anything newer must be .optional(). + * + * @throws {InvalidApiResponseError} naming the endpoint when validation fails. + */ +export function parseApiResponse( + schema: z.ZodType, + data: T, + endpoint: string, + url?: string, +): T { + const result = schema.safeParse(data); + if (!result.success) { + throw new InvalidApiResponseError(endpoint, url, { cause: result.error }); + } + return data; +} + +export const UserSchema = z.looseObject({ + id: z.string(), + username: z.string(), + roles: z.array(z.looseObject({ name: z.string() })), +}); + +const WorkspaceAgentSchema = z.looseObject({ + id: z.string(), + name: z.string(), + status: z.string(), + operating_system: z.string(), +}); + +const WorkspaceResourceSchema = z.looseObject({ + agents: z.array(WorkspaceAgentSchema).nullable().optional(), +}); + +export const WorkspaceSchema = z.looseObject({ + id: z.string(), + name: z.string(), + owner_name: z.string(), + template_id: z.string(), + latest_build: z.looseObject({ + id: z.string(), + status: z.string(), + template_version_id: z.string(), + resources: z.array(WorkspaceResourceSchema), + }), +}); + +/** waitForBuild reads the identifiers to poll and the job status to stop. */ +export const WorkspaceBuildSchema = z.looseObject({ + workspace_owner_name: z.string(), + workspace_name: z.string(), + build_number: z.number(), + job: z.looseObject({ status: z.string() }), +}); + +export const TemplateSchema = z.looseObject({ + active_version_id: z.string(), +}); + +export const WorkspaceResourcesSchema = z.array(WorkspaceResourceSchema); + +export const SSHConfigResponseSchema = z.looseObject({ + ssh_config_options: z.record(z.string(), z.string()), +}); + +/** + * The schema each SDK method's response must match, applied by CoderApi. Add a + * pair to validate another method; the name doubles as the endpoint in the + * error. Pairs, not an object, so iterating keeps the names as literal types. + * OAuth endpoints are plain axios calls and pass their schema at the call site. + */ +export const VALIDATED_RESPONSES = [ + ["getAuthenticatedUser", UserSchema], + ["getDeploymentSSHConfig", SSHConfigResponseSchema], + ["getTemplate", TemplateSchema], + ["getTemplateVersionResources", WorkspaceResourcesSchema], + ["getWorkspace", WorkspaceSchema], + ["getWorkspaceByOwnerAndName", WorkspaceSchema], + ["getWorkspaceBuildByNumber", WorkspaceBuildSchema], + ["startWorkspace", WorkspaceBuildSchema], + ["stopWorkspace", WorkspaceBuildSchema], +] as const satisfies ReadonlyArray; + +/** + * The methods above, reduced to what the wrapper needs. CoderApi satisfies this + * with no assertion: `never` parameters accept any signature, and one uniform + * value type is what allows assigning by a name held in a variable. + */ +export type ValidatedMethods = Record< + (typeof VALIDATED_RESPONSES)[number][0], + (...args: never[]) => Promise +>; diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 864ca08a47..b22bb90a70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -17,6 +17,11 @@ import { generateState, toUrlSearchParams, } from "./utils"; +import { + OAuth2ClientRegistrationResponseSchema, + OAuth2TokenResponseSchema, + parseOAuthResponse, +} from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -173,16 +178,22 @@ export class OAuthAuthorizer implements vscode.Disposable { registrationRequest, ); + const registrationResponse = parseOAuthResponse( + OAuth2ClientRegistrationResponseSchema, + response.data, + metadata.registration_endpoint, + ); + await this.secretsManager.setOAuthClientRegistration( deployment.safeHostname, - response.data, + registrationResponse, ); this.logger.debug( "Saved OAuth client registration:", - response.data.client_id, + registrationResponse.client_id, ); - return response.data; + return registrationResponse; } /** @@ -360,7 +371,11 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("Token exchange successful"); - return response.data; + return parseOAuthResponse( + OAuth2TokenResponseSchema, + response.data, + metadata.token_endpoint, + ); } public dispose(): void { diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index add545fbd8..b6b5b4e24f 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -1,3 +1,5 @@ +import { parseApiResponse } from "../api/responseValidation"; + import { AUTH_GRANT_TYPE, PKCE_CHALLENGE_METHOD, @@ -5,6 +7,7 @@ import { RESPONSE_TYPE, TOKEN_ENDPOINT_AUTH_METHOD, } from "./constants"; +import { OAuth2AuthorizationServerMetadataSchema } from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -69,9 +72,13 @@ export class OAuthMetadataClient { OAUTH_DISCOVERY_ENDPOINT, ); - const metadata = response.data; + const metadata = parseApiResponse( + OAuth2AuthorizationServerMetadataSchema, + response.data, + OAUTH_DISCOVERY_ENDPOINT, + this.axiosInstance.defaults.baseURL, + ); - this.validateRequiredEndpoints(metadata); this.validateGrantTypes(metadata); this.validateResponseTypes(metadata); this.validateAuthMethods(metadata); @@ -87,21 +94,6 @@ export class OAuthMetadataClient { return metadata; } - private validateRequiredEndpoints( - metadata: OAuth2AuthorizationServerMetadata, - ): void { - if ( - !metadata.authorization_endpoint || - !metadata.token_endpoint || - !metadata.issuer - ) { - throw new Error( - "OAuth server metadata missing required endpoints: " + - JSON.stringify(metadata), - ); - } - } - private validateGrantTypes( metadata: OAuth2AuthorizationServerMetadata, ): void { diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 395a4411e7..5ccc122aa5 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -8,6 +8,7 @@ import { DEFAULT_OAUTH_SCOPES, REFRESH_GRANT_TYPE } from "./constants"; import { OAuthError, parseOAuthError } from "./errors"; import { OAuthMetadataClient } from "./metadataClient"; import { buildOAuthTokenData, toUrlSearchParams } from "./utils"; +import { OAuth2TokenResponseSchema, parseOAuthResponse } from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -421,17 +422,23 @@ export class OAuthSessionManager implements vscode.Disposable { this.logger.debug("Token refresh successful"); + const tokenResponse = parseOAuthResponse( + OAuth2TokenResponseSchema, + response.data, + metadata.token_endpoint, + ); + await this.secretsManager.setSessionAuth(deployment.safeHostname, { url: deployment.url, - token: response.data.access_token, + token: tokenResponse.access_token, username: await this.fetchUsername( deployment, - response.data.access_token, + tokenResponse.access_token, ), - oauth: buildOAuthTokenData(response.data), + oauth: buildOAuthTokenData(tokenResponse), }); - return response.data; + return tokenResponse; }, ); } catch (error) { diff --git a/src/oauth/validation.ts b/src/oauth/validation.ts new file mode 100644 index 0000000000..b16c88f8f2 --- /dev/null +++ b/src/oauth/validation.ts @@ -0,0 +1,53 @@ +import { z } from "zod"; + +import { parseApiResponse } from "../api/responseValidation"; + +/** + * parseApiResponse for OAuth endpoints, whose absolute URLs come from server + * metadata and may live on a different origin than the deployment. The schemas + * below follow the same rules. + */ +export function parseOAuthResponse( + schema: z.ZodType, + data: T, + endpoint: string, +): T { + const { origin, pathname } = new URL(endpoint); + return parseApiResponse(schema, data, pathname, origin); +} + +/** An empty endpoint or identifier is as unusable as a missing one. */ +const REQUIRED_STRING = z.string().min(1); + +/** + * Plain strings rather than the generated enums, so a server adding a value + * does not fail validation. Absent means the RFC 8414 default applies. + */ +const CAPABILITIES = z.array(z.string()).optional(); + +export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ + issuer: REQUIRED_STRING, + authorization_endpoint: REQUIRED_STRING, + token_endpoint: REQUIRED_STRING, + // Callers report these as unsupported when absent, so no .min(1) here. + registration_endpoint: z.string().optional(), + revocation_endpoint: z.string().optional(), + grant_types_supported: CAPABILITIES, + response_types_supported: CAPABILITIES, + token_endpoint_auth_methods_supported: CAPABILITIES, + code_challenge_methods_supported: CAPABILITIES, + scopes_supported: CAPABILITIES, +}); + +export const OAuth2ClientRegistrationResponseSchema = z.looseObject({ + client_id: REQUIRED_STRING, + client_secret: z.string().optional(), + redirect_uris: z.array(z.string()).optional(), +}); + +export const OAuth2TokenResponseSchema = z.looseObject({ + access_token: REQUIRED_STRING, + token_type: z.string(), + refresh_token: z.string().optional(), + expires_in: z.number().optional(), +}); diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index 16c2847dd9..c79b438699 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -22,6 +22,11 @@ import { refreshCertificates, } from "@/api/certificateRefresh"; import { CoderApi, DEFAULT_REQUEST_TIMEOUT_MS } from "@/api/coderApi"; +import { + InvalidApiResponseError, + VALIDATED_RESPONSES, + type ValidatedMethods, +} from "@/api/responseValidation"; import { createHttpAgent } from "@/api/utils"; import { CONFIG_CHANGE_DEBOUNCE_MS } from "@/configWatcher"; import { ClientCertificateError } from "@/error/clientCertificateError"; @@ -35,10 +40,14 @@ import { ReconnectingWebSocket } from "@/websocket/reconnectingWebSocket"; import { createMockLogger, + createMockUser, MockConfigurationProvider, } from "../../mocks/testHelpers"; -import type { ProvisionerJobLog } from "coder/site/src/api/typesGenerated"; +import type { + ProvisionerJobLog, + WorkspaceBuild, +} from "coder/site/src/api/typesGenerated"; import type { RequestConfigWithMeta } from "@/logging/types"; @@ -846,6 +855,173 @@ describe("CoderApi", () => { ); }); + describe("response validation", () => { + const mockResponse = (data: unknown) => { + mockAdapter.mockResolvedValueOnce({ + data, + status: 200, + statusText: "OK", + headers: {}, + config: {}, + }); + }; + + const VALID_WORKSPACE = { + id: "ws-1", + name: "dev", + owner_name: "developer", + template_id: "tpl-1", + latest_build: { + id: "build-1", + status: "running", + template_version_id: "version-1", + resources: [], + }, + }; + + const VALID_BUILD = { + workspace_owner_name: "developer", + workspace_name: "dev", + build_number: 1, + job: { status: "succeeded" }, + }; + + /** One realistic body per validated method, keyed by the method name. */ + const CASES: ReadonlyArray<{ + method: keyof ValidatedMethods; + call: (api: CoderApi) => Promise; + valid: unknown; + }> = [ + { + method: "getAuthenticatedUser", + call: (api) => api.getAuthenticatedUser(), + valid: createMockUser({ username: "developer" }), + }, + { + method: "getDeploymentSSHConfig", + call: (api) => api.getDeploymentSSHConfig(), + valid: { ssh_config_options: { ConnectTimeout: "30" } }, + }, + { + method: "getTemplate", + call: (api) => api.getTemplate("tpl-1"), + valid: { active_version_id: "version-1" }, + }, + { + method: "getTemplateVersionResources", + call: (api) => api.getTemplateVersionResources("version-1"), + valid: [{ id: "res-1", agents: null }], + }, + { + method: "getWorkspace", + call: (api) => api.getWorkspace("ws-1"), + valid: VALID_WORKSPACE, + }, + { + method: "getWorkspaceByOwnerAndName", + call: (api) => api.getWorkspaceByOwnerAndName("developer", "dev"), + valid: VALID_WORKSPACE, + }, + { + method: "getWorkspaceBuildByNumber", + call: (api) => api.getWorkspaceBuildByNumber("developer", "dev", 1), + valid: VALID_BUILD, + }, + { + method: "startWorkspace", + call: (api) => api.startWorkspace("ws-1", "version-1"), + valid: VALID_BUILD, + }, + { + method: "stopWorkspace", + call: (api) => api.stopWorkspace("ws-1"), + valid: VALID_BUILD, + }, + ]; + + it("exercises every validated method", () => { + expect(CASES.map((testCase) => testCase.method).sort()).toEqual( + VALIDATED_RESPONSES.map(([method]) => method).sort(), + ); + }); + + it.each(CASES)( + "$method passes a valid body through", + async ({ call, valid }) => { + api = createApi(); + mockResponse(valid); + + await expect(call(api)).resolves.toEqual(valid); + }, + ); + + it.each(CASES)( + "$method rejects a body that is not from Coder", + async ({ method, call }) => { + api = createApi(); + mockResponse("Bad Gateway"); + + await expect(call(api)).rejects.toThrow( + `${CODER_URL} did not return a valid Coder API response for ${method}`, + ); + }, + ); + + it("reports an unparseable response as InvalidApiResponseError", async () => { + api = createApi(); + mockResponse({ id: "user-1" }); + + await expect(api.getAuthenticatedUser()).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); + }); + }); + + describe("waitForBuild", () => { + const BUILD = { + workspace_owner_name: "developer", + workspace_name: "dev", + build_number: 1, + job: { status: "succeeded" }, + } as WorkspaceBuild; + + const mockPoll = (job: unknown) => { + mockAdapter.mockResolvedValueOnce({ + data: { ...BUILD, job }, + status: 200, + statusText: "OK", + headers: {}, + config: {}, + }); + }; + + it("returns the job once the build settles", async () => { + api = createApi(); + mockPoll({ status: "succeeded" }); + + await expect(api.waitForBuild(BUILD)).resolves.toEqual({ + status: "succeeded", + }); + }); + + it("throws when the build failed", async () => { + api = createApi(); + mockPoll({ status: "failed" }); + + await expect(api.waitForBuild(BUILD)).rejects.toThrow("Build 1 failed"); + }); + + // The SDK version swallows poll errors, leaving callers hanging forever. + it("surfaces a validation error instead of polling forever", async () => { + api = createApi(); + mockPoll({}); + + await expect(api.waitForBuild(BUILD)).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); + }); + }); + describe("getHost/getSessionToken", () => { it("returns current host and token", () => { const api = createApi(CODER_URL, AXIOS_TOKEN); diff --git a/test/unit/api/responseValidation.test.ts b/test/unit/api/responseValidation.test.ts new file mode 100644 index 0000000000..d64e01ddcb --- /dev/null +++ b/test/unit/api/responseValidation.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it } from "vitest"; +import { ZodError, type z } from "zod"; + +import { + InvalidApiResponseError, + parseApiResponse, + SSHConfigResponseSchema, + TemplateSchema, + UserSchema, + WorkspaceBuildSchema, + WorkspaceResourcesSchema, + WorkspaceSchema, +} from "@/api/responseValidation"; + +import { createMockUser } from "../../mocks/testHelpers"; + +const ENDPOINT = "/api/v2/users/me"; +const DEPLOYMENT_URL = "https://coder.example.com"; + +/** + * The smallest body each schema must keep accepting, as Coder 0.25 sends it. + * Breaking one of these breaks old deployments; make the new field .optional(). + */ +const SCHEMAS: ReadonlyArray<{ + name: string; + schema: z.ZodType; + minimal: unknown; + /** The minimal body with one required field taken away. */ + incomplete: unknown; +}> = [ + { + name: "UserSchema", + schema: UserSchema, + minimal: { id: "user-1", username: "dev", roles: [] }, + incomplete: { id: "user-1", username: "dev" }, + }, + { + name: "WorkspaceSchema", + schema: WorkspaceSchema, + minimal: { + id: "ws-1", + name: "dev", + owner_name: "developer", + template_id: "tpl-1", + latest_build: { + id: "build-1", + status: "running", + template_version_id: "version-1", + resources: [], + }, + }, + incomplete: { + id: "ws-1", + name: "dev", + owner_name: "developer", + template_id: "tpl-1", + }, + }, + { + name: "WorkspaceBuildSchema", + schema: WorkspaceBuildSchema, + minimal: { + workspace_owner_name: "developer", + workspace_name: "dev", + build_number: 1, + job: { status: "succeeded" }, + }, + incomplete: { + workspace_owner_name: "developer", + workspace_name: "dev", + build_number: 1, + }, + }, + { + name: "WorkspaceResourcesSchema", + schema: WorkspaceResourcesSchema, + minimal: [{}], + incomplete: { resources: [] }, + }, + { + name: "TemplateSchema", + schema: TemplateSchema, + minimal: { active_version_id: "version-1" }, + incomplete: {}, + }, + { + name: "SSHConfigResponseSchema", + schema: SSHConfigResponseSchema, + minimal: { ssh_config_options: {} }, + incomplete: { hostname_prefix: "coder." }, + }, +]; + +describe("parseApiResponse", () => { + it("returns the body as-is, unknown fields included", () => { + const body = { ...createMockUser(), future_field: { nested: [1, 2, 3] } }; + + expect(parseApiResponse(UserSchema, body, ENDPOINT, DEPLOYMENT_URL)).toBe( + body, + ); + }); + + it("throws InvalidApiResponseError naming the endpoint and URL", () => { + expect(() => + parseApiResponse(UserSchema, {}, ENDPOINT, DEPLOYMENT_URL), + ).toThrow( + `${DEPLOYMENT_URL} did not return a valid Coder API response for ${ENDPOINT}`, + ); + }); + + it("keeps the Zod failure as the cause", () => { + try { + parseApiResponse(UserSchema, {}, ENDPOINT, DEPLOYMENT_URL); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(InvalidApiResponseError); + expect((error as InvalidApiResponseError).cause).toBeInstanceOf(ZodError); + } + }); + + it("names the deployment generically when no URL is known", () => { + expect(() => parseApiResponse(UserSchema, {}, ENDPOINT)).toThrow( + "The deployment did not return a valid Coder API response", + ); + }); + + // What a misdirected URL actually returns: a login page, an empty body, + // or JSON from some other service. + it.each([ + ["an HTML page", "Login"], + ["null", null], + ["undefined", undefined], + ["an unrelated object", { message: "not found" }], + ])("rejects %s", (_name, body) => { + expect(() => parseApiResponse(UserSchema, body, ENDPOINT)).toThrow( + InvalidApiResponseError, + ); + }); +}); + +describe.each(SCHEMAS)("$name", ({ schema, minimal, incomplete }) => { + it("accepts the minimal body an old deployment sends", () => { + expect(schema.safeParse(minimal).success).toBe(true); + }); + + it("rejects a body missing a required field", () => { + expect(schema.safeParse(incomplete).success).toBe(false); + }); +}); + +describe("WorkspaceSchema", () => { + it("accepts resources whose agents are null or absent", () => { + const workspace = { + id: "ws-1", + name: "dev", + owner_name: "developer", + template_id: "tpl-1", + latest_build: { + id: "build-1", + status: "running", + template_version_id: "version-1", + resources: [{ id: "res-1", agents: null }, { id: "res-2" }], + }, + }; + + expect(WorkspaceSchema.safeParse(workspace).success).toBe(true); + }); +}); diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 976a8d4069..2f95f7ae8c 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -1,10 +1,12 @@ import { describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; +import { InvalidApiResponseError } from "@/api/responseValidation"; import { getHeaders } from "@/headers"; import { OAuthAuthorizer } from "@/oauth/authorizer"; import { + createMockUser, MockCancellationToken, MockProgress, setupAxiosMockRoutes, @@ -115,19 +117,17 @@ async function waitForBrowserToOpen(): Promise<{ describe("OAuthAuthorizer", () => { describe("login flow", () => { it("completes full OAuth login flow successfully", async () => { - const { mockAdapter, oauthCallback, secretsManager, authorizer } = + const { setupOAuthRoutes, oauthCallback, secretsManager, authorizer } = createTestContext(); - setupAxiosMockRoutes(mockAdapter, { - "/.well-known/oauth-authorization-server": - createMockOAuthMetadata(TEST_URL), + setupOAuthRoutes(undefined, { "/oauth2/register": createMockClientRegistration({ client_id: "registered-client-id", }), "/oauth2/token": createMockTokenResponse({ access_token: "oauth-access-token", }), - "/api/v2/users/me": { username: "oauth-user" }, + "/api/v2/users/me": createMockUser({ username: "oauth-user" }), }); const deployment = createTestDeployment(); @@ -161,7 +161,7 @@ describe("OAuthAuthorizer", () => { }); it("uses existing client registration when redirect URI matches", async () => { - const { mockAdapter, oauthCallback, secretsManager, authorizer } = + const { setupOAuthRoutes, oauthCallback, secretsManager, authorizer } = createTestContext(); // Pre-store a client registration with matching redirect URI @@ -174,12 +174,8 @@ describe("OAuthAuthorizer", () => { ); // Registration endpoint should throw if called (existing registration should be reused) - setupAxiosMockRoutes(mockAdapter, { + setupOAuthRoutes(undefined, { "/oauth2/register": new Error("Should not re-register"), - "/.well-known/oauth-authorization-server": - createMockOAuthMetadata(TEST_URL), - "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, }); const loginPromise = authorizer.login( @@ -200,7 +196,7 @@ describe("OAuthAuthorizer", () => { }); it("re-registers client when redirect URI has changed", async () => { - const { mockAdapter, oauthCallback, secretsManager, authorizer } = + const { setupOAuthRoutes, oauthCallback, secretsManager, authorizer } = createTestContext(); // Pre-store a client registration with different redirect URI @@ -213,14 +209,10 @@ describe("OAuthAuthorizer", () => { ); // Server will return new registration - setupAxiosMockRoutes(mockAdapter, { - "/.well-known/oauth-authorization-server": - createMockOAuthMetadata(TEST_URL), + setupOAuthRoutes(undefined, { "/oauth2/register": createMockClientRegistration({ client_id: "new-client-id", }), - "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, }); const loginPromise = authorizer.login( @@ -507,5 +499,34 @@ describe("OAuthAuthorizer", () => { ), ).rejects.toThrow("Server does not support dynamic client registration"); }); + + it("rejects a token response without an access token", async () => { + const { setupOAuthRoutes, startLogin, completeLogin } = + createTestContext(); + setupOAuthRoutes(undefined, { + "/oauth2/token": { token_type: "Bearer" }, + }); + + const { loginPromise, state } = await startLogin(); + await completeLogin(state); + + await expect(loginPromise).rejects.toThrow(InvalidApiResponseError); + }); + + it("rejects a registration response without a client_id", async () => { + const { setupOAuthRoutes, authorizer } = createTestContext(); + setupOAuthRoutes(undefined, { + "/oauth2/register": { client_secret: "no-id" }, + }); + + // Fails before the browser opens, so there is no callback to complete. + await expect( + authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ), + ).rejects.toThrow(InvalidApiResponseError); + }); }); }); diff --git a/test/unit/oauth/metadataClient.test.ts b/test/unit/oauth/metadataClient.test.ts index db63886b70..79390076d5 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -75,23 +75,30 @@ describe("OAuthMetadataClient", () => { expect(result).toEqual(metadata); }); - describe("required endpoints validation", () => { - it.each(["authorization_endpoint", "token_endpoint", "issuer"])( - "throws when %s missing", - async (field) => { - const { mockAdapter, client } = createTestContext(); - - setupAxiosMockRoutes(mockAdapter, { - "/.well-known/oauth-authorization-server": createMockOAuthMetadata( - TEST_URL, - { [field]: undefined }, - ), - }); - - await expect(client.getMetadata()).rejects.toThrow( - "OAuth server metadata missing required endpoints", - ); + it.each<[string, Record]>([ + [ + "authorization_endpoint is missing", + { authorization_endpoint: undefined }, + ], + ["token_endpoint is missing", { token_endpoint: undefined }], + ["issuer is missing", { issuer: undefined }], + ["issuer is empty", { issuer: "" }], + [ + "a *_supported field is not an array", + { grant_types_supported: "authorization_code refresh_token" }, + ], + ])("throws when %s", async (_name, overrides) => { + const { mockAdapter, client } = createTestContext(); + + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": { + ...createMockOAuthMetadata(TEST_URL), + ...overrides, }, + }); + + await expect(client.getMetadata()).rejects.toThrow( + "did not return a valid Coder API response", ); }); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 1951317f83..be11270d44 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -5,6 +5,7 @@ import { } from "axios"; import { describe, expect, it, vi } from "vitest"; +import { InvalidApiResponseError } from "@/api/responseValidation"; import { type SessionAuth } from "@/core/secretsManager"; import { DEFAULT_OAUTH_SCOPES } from "@/oauth/constants"; import { OAuthSessionManager } from "@/oauth/sessionManager"; @@ -229,6 +230,21 @@ describe("OAuthSessionManager", () => { username: "existing-user", }); }); + + it("rejects a token response without an access token", async () => { + const { manager, setupForOAuthOperation } = createTestContext(); + await setupForOAuthOperation( + { + "/oauth2/token": { token_type: "Bearer" }, + "/api/v2/users/me": createMockUser(), + }, + { token: "old-token" }, + ); + + await expect(manager.refreshToken()).rejects.toThrow( + InvalidApiResponseError, + ); + }); }); describe("getStoredTokens validation", () => { diff --git a/test/unit/oauth/testUtils.ts b/test/unit/oauth/testUtils.ts index e0fb85614d..8729aa4291 100644 --- a/test/unit/oauth/testUtils.ts +++ b/test/unit/oauth/testUtils.ts @@ -8,6 +8,7 @@ import { OAuthCallback } from "@/oauth/oauthCallback"; import { createMockLogger, + createMockUser, getAxiosMockAdapter, InMemoryMemento, InMemorySecretStorage, @@ -136,17 +137,22 @@ export function createBaseTestContext() { ); const oauthCallback = new OAuthCallback(secretStorage, logger); - /** Sets up OAuth routes, defaulting to metadata for TEST_URL. */ + /** + * Sets up OAuth routes, defaulting to metadata for TEST_URL. Pass overrides + * to replace individual route responses with a failure or a custom body. + */ const setupOAuthRoutes = ( metadata: OAuth2AuthorizationServerMetadata = createMockOAuthMetadata( TEST_URL, ), + overrides: Record = {}, ) => { setupAxiosMockRoutes(mockAdapter, { "/.well-known/oauth-authorization-server": metadata, "/oauth2/register": createMockClientRegistration(), "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, + "/api/v2/users/me": createMockUser(), + ...overrides, }); };