From 12627ce9ad221cc48beba73a047a070ce1e3267b Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sun, 9 Aug 2026 12:48:54 +0000 Subject: [PATCH 1/6] fix(api): validate API responses at the boundary instead of trusting generated types The SDK casts 2xx response bodies to generated TypeScript types with no runtime check. A non-Coder service at the configured URL, a proxy HTML error page, or a partial body would flow in as if valid and crash far from the cause (e.g. user.roles.some -> 'Cannot read properties of undefined'). Add a zod-based parseApiResponse helper and InvalidApiResponseError that names the endpoint and deployment URL. Validate on CoderApi overrides for the login/connect paths (users/me, workspace, workspace build, template version resources, deployment SSH config) and at the OAuth entry points (metadata, client registration, token exchange/refresh). Schemas are permissive looseObjects requiring only the fields the extension reads, so newer deployments adding fields never break login. A roles-less /users/me now hard-fails login with a clear error instead of silently treating the user as non-owner. Fixes #1050 --- src/api/coderApi.ts | 121 +++++++++++++++ src/api/responseValidation.ts | 96 ++++++++++++ src/oauth/authorizer.ts | 25 ++- src/oauth/metadataClient.ts | 10 +- src/oauth/sessionManager.ts | 17 ++- src/oauth/validation.ts | 27 ++++ test/unit/api/coderApi.test.ts | 136 +++++++++++++++++ test/unit/api/responseValidation.test.ts | 186 +++++++++++++++++++++++ test/unit/oauth/authorizer.test.ts | 49 +++++- test/unit/oauth/metadataClient.test.ts | 2 +- test/unit/oauth/sessionManager.test.ts | 16 ++ test/unit/oauth/testUtils.ts | 3 +- 12 files changed, 674 insertions(+), 14 deletions(-) create mode 100644 src/api/responseValidation.ts create mode 100644 src/oauth/validation.ts create mode 100644 test/unit/api/responseValidation.test.ts diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 8eb1402923..712f1f8ab1 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -49,15 +49,28 @@ import { import { SseConnection } from "../websocket/sseConnection"; import { getRefreshCommand, refreshCertificates } from "./certificateRefresh"; +import { + parseApiResponse, + SSHConfigResponseSchema, + UserSchema, + WorkspaceBuildSchema, + WorkspaceResourcesSchema, + WorkspaceSchema, +} from "./responseValidation"; import { createHttpAgent } from "./utils"; import type { GetInboxNotificationResponse, ProvisionerJobLog, ServerSentEvent, + SSHConfigResponse, + User, Workspace, WorkspaceAgent, WorkspaceAgentLog, + WorkspaceBuild, + WorkspaceOptions, + WorkspaceResource, } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; @@ -149,6 +162,97 @@ export class CoderApi extends Api implements vscode.Disposable { return this.getAxiosInstance().defaults.baseURL; } + /** + * The SDK casts response bodies to the generated types with no runtime + * check, so a 2xx from a non-Coder service (or a proxy error page) would + * flow in as if valid and crash far from the cause. These overrides + * validate the fields the extension reads and throw + * InvalidApiResponseError, naming the endpoint, at the boundary instead. + * + * The SDK methods are instance arrow properties assigned during super(), + * and field initializers run in declaration order before the constructor + * body, so capturing `this.X` here reads the base implementation before + * the shadowing overrides below are assigned. `super.X` is unavailable + * for parent class fields (ts(2855)), and TS2729 on `this.X` is a false + * positive since the base constructor already ran. + */ + private readonly baseMethods = captureBaseMethods(this); + + override getAuthenticatedUser = async (): Promise => { + return parseApiResponse( + UserSchema, + await this.baseMethods.getAuthenticatedUser(), + "/api/v2/users/me", + this.getHost(), + ); + }; + + override getWorkspace = async ( + workspaceId: string, + params?: WorkspaceOptions, + ): Promise => { + return parseApiResponse( + WorkspaceSchema, + await this.baseMethods.getWorkspace(workspaceId, params), + `/api/v2/workspaces/${workspaceId}`, + this.getHost(), + ); + }; + + override getWorkspaceByOwnerAndName = async ( + username: string, + workspaceName: string, + params?: WorkspaceOptions, + ): Promise => { + return parseApiResponse( + WorkspaceSchema, + await this.baseMethods.getWorkspaceByOwnerAndName( + username, + workspaceName, + params, + ), + `/api/v2/users/${username}/workspace/${workspaceName}`, + this.getHost(), + ); + }; + + override getWorkspaceBuildByNumber = async ( + username: string, + workspaceName: string, + buildNumber: number, + ): Promise => { + return parseApiResponse( + WorkspaceBuildSchema, + await this.baseMethods.getWorkspaceBuildByNumber( + username, + workspaceName, + buildNumber, + ), + `/api/v2/users/${username}/workspace/${workspaceName}/builds/${buildNumber}`, + this.getHost(), + ); + }; + + override getTemplateVersionResources = async ( + versionId: string, + ): Promise => { + return parseApiResponse( + WorkspaceResourcesSchema, + await this.baseMethods.getTemplateVersionResources(versionId), + `/api/v2/templateversions/${versionId}/resources`, + this.getHost(), + ); + }; + + override getDeploymentSSHConfig = async (): Promise => { + return parseApiResponse( + SSHConfigResponseSchema, + await this.baseMethods.getDeploymentSSHConfig(), + "/api/v2/deployment/ssh", + this.getHost(), + ); + }; + hasAuthConfigChangedSince(version: number | undefined): boolean { return this.authConfigTracker.hasChangedSince(version); } @@ -747,6 +851,23 @@ function wrapResponseTransform( ]; } +/** + * Capture the base SDK method implementations before the validating + * overrides on CoderApi shadow them. Reads `this.X` while the base class + * arrow-function fields are still in place (field initializers run in + * declaration order, and this field is declared before the overrides). + */ +function captureBaseMethods(instance: Api) { + return { + getAuthenticatedUser: instance.getAuthenticatedUser, + getWorkspace: instance.getWorkspace, + getWorkspaceByOwnerAndName: instance.getWorkspaceByOwnerAndName, + getWorkspaceBuildByNumber: instance.getWorkspaceBuildByNumber, + getTemplateVersionResources: instance.getTemplateVersionResources, + getDeploymentSSHConfig: instance.getDeploymentSSHConfig, + }; +} + 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..2a3a8ccaa5 --- /dev/null +++ b/src/api/responseValidation.ts @@ -0,0 +1,96 @@ +import { ZodError, z } from "zod"; + +/** + * 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. ` + + "See Output > Coder for details.", + options, + ); + this.name = "InvalidApiResponseError"; + } +} + +/** + * Validate a response body against a permissive schema, returning the + * original value with the caller's type. Schemas must use looseObject so + * unknown fields pass through: the point is to catch "this is not the object + * we expect", not to mirror the full API, so newer deployments adding fields + * never break. + * + * @throws {InvalidApiResponseError} naming the endpoint when validation fails. + */ +export function parseApiResponse( + schema: z.ZodType, + data: T, + endpoint: string, + url?: string, +): T { + try { + schema.parse(data); + } catch (error) { + if (error instanceof ZodError) { + throw new InvalidApiResponseError(endpoint, url, { cause: error }); + } + throw error; + } + return data; +} + +/** + * Only the fields the extension reads are required; everything else passes + * through untouched. + */ +export const UserSchema = z.looseObject({ + id: z.string(), + username: z.string(), + roles: z.array(z.looseObject({ name: z.string() })), + organization_ids: z.array(z.string()), +}); + +const WorkspaceAgentSchema = z.looseObject({ + id: z.string(), + name: z.string(), + status: z.string(), + operating_system: z.string(), + architecture: 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 polls `job.status`; a malformed job would loop forever. */ +export const WorkspaceBuildSchema = z.looseObject({ + job: z.looseObject({ status: z.string() }), +}); + +export const WorkspaceResourcesSchema = z.array(WorkspaceResourceSchema); + +export const SSHConfigResponseSchema = z.looseObject({ + hostname_suffix: z.string(), + ssh_config_options: z.record(z.string(), z.string()), +}); diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 864ca08a47..85c66ce894 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; +import { parseApiResponse } from "../api/responseValidation"; import { resolveCoderDashboardUrl } from "../util/uri"; import { @@ -17,6 +18,10 @@ import { generateState, toUrlSearchParams, } from "./utils"; +import { + OAuth2ClientRegistrationResponseSchema, + OAuth2TokenResponseSchema, +} from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -173,16 +178,23 @@ export class OAuthAuthorizer implements vscode.Disposable { registrationRequest, ); + const registrationResponse = parseApiResponse( + OAuth2ClientRegistrationResponseSchema, + response.data, + metadata.registration_endpoint, + axiosInstance.defaults.baseURL, + ); + 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 +372,12 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("Token exchange successful"); - return response.data; + return parseApiResponse( + OAuth2TokenResponseSchema, + response.data, + metadata.token_endpoint, + axiosInstance.defaults.baseURL, + ); } public dispose(): void { diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index add545fbd8..583cc4de90 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,7 +72,12 @@ 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); diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 395a4411e7..434f985891 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -1,4 +1,5 @@ import { CoderApi } from "../api/coderApi"; +import { parseApiResponse } from "../api/responseValidation"; import { AuthTelemetry, type AuthTokenRefreshTrigger, @@ -8,6 +9,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 } from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -421,17 +423,24 @@ export class OAuthSessionManager implements vscode.Disposable { this.logger.debug("Token refresh successful"); + const tokenResponse = parseApiResponse( + OAuth2TokenResponseSchema, + response.data, + metadata.token_endpoint, + axiosInstance.defaults.baseURL, + ); + 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..b0a8c9fa1c --- /dev/null +++ b/src/oauth/validation.ts @@ -0,0 +1,27 @@ +import { z } from "zod"; + +/** + * Permissive schemas for the OAuth endpoints hit directly via axios during + * login, before any session exists. Only the fields the flow reads are + * required; unknown fields pass through so newer deployments never break. + */ +export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ + issuer: z.string(), + authorization_endpoint: z.string(), + token_endpoint: z.string(), + registration_endpoint: z.string().optional(), + revocation_endpoint: z.string().optional(), +}); + +export const OAuth2ClientRegistrationResponseSchema = z.looseObject({ + client_id: z.string(), + client_secret: z.string().optional(), + redirect_uris: z.array(z.string()).optional(), +}); + +export const OAuth2TokenResponseSchema = z.looseObject({ + access_token: z.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..b44c297ffe 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -22,6 +22,7 @@ import { refreshCertificates, } from "@/api/certificateRefresh"; import { CoderApi, DEFAULT_REQUEST_TIMEOUT_MS } from "@/api/coderApi"; +import { InvalidApiResponseError } from "@/api/responseValidation"; import { createHttpAgent } from "@/api/utils"; import { CONFIG_CHANGE_DEBOUNCE_MS } from "@/configWatcher"; import { ClientCertificateError } from "@/error/clientCertificateError"; @@ -846,6 +847,141 @@ describe("CoderApi", () => { ); }); + describe("Response Validation", () => { + const mockResponse = (data: unknown) => { + mockAdapter.mockResolvedValueOnce({ + data, + status: 200, + statusText: "OK", + headers: {}, + config: {}, + }); + }; + + const validUser = { + id: "user-1", + username: "developer", + roles: [{ name: "owner" }], + organization_ids: ["org-1"], + email: "dev@example.com", + }; + + const validWorkspace = { + 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: [], + }, + }; + + it("returns the user when /users/me is valid", async () => { + api = createApi(); + mockResponse(validUser); + + const user = await api.getAuthenticatedUser(); + + expect(user.username).toBe("developer"); + expect(user.roles[0]?.name).toBe("owner"); + }); + + it("still performs the underlying HTTP request", async () => { + api = createApi(); + mockResponse(validUser); + + await api.getAuthenticatedUser(); + + expect(mockAdapter).toHaveBeenCalledWith( + expect.objectContaining({ url: "/api/v2/users/me" }), + ); + }); + + it.each([ + ["an HTML error page", "Bad Gateway"], + ["null", null], + ["an empty object", {}], + ["a user missing roles", { id: "user-1", username: "developer" }], + ])( + "rejects /users/me returning %s with the endpoint and URL", + async (_description, body) => { + api = createApi(); + mockResponse(body); + + const promise = api.getAuthenticatedUser(); + await expect(promise).rejects.toBeInstanceOf(InvalidApiResponseError); + await expect(promise).rejects.toThrow( + `${CODER_URL} did not return a valid Coder API response for /api/v2/users/me`, + ); + }, + ); + + it("preserves unknown fields on validated responses", async () => { + api = createApi(); + mockResponse({ ...validUser, future_field: { nested: true } }); + + const user = await api.getAuthenticatedUser(); + + expect((user as unknown as Record).future_field).toEqual( + { nested: true }, + ); + }); + + it("validates getWorkspace and getWorkspaceByOwnerAndName", async () => { + api = createApi(); + mockResponse(validWorkspace); + await expect(api.getWorkspace("ws-1")).resolves.toEqual(validWorkspace); + + mockResponse({ name: "not-a-workspace" }); + await expect( + api.getWorkspaceByOwnerAndName("me", "dev"), + ).rejects.toBeInstanceOf(InvalidApiResponseError); + }); + + it("validates the build job status for waitForBuild polling", async () => { + api = createApi(); + mockResponse({ job: { status: "succeeded" } }); + await expect( + api.getWorkspaceBuildByNumber("me", "dev", 1), + ).resolves.toEqual({ job: { status: "succeeded" } }); + + mockResponse({ id: "build-without-job" }); + await expect( + api.getWorkspaceBuildByNumber("me", "dev", 1), + ).rejects.toBeInstanceOf(InvalidApiResponseError); + }); + + it("validates template version resources", async () => { + api = createApi(); + mockResponse([{ id: "res-1", agents: null }]); + await expect( + api.getTemplateVersionResources("version-1"), + ).resolves.toEqual([{ id: "res-1", agents: null }]); + + mockResponse({ resources: "not-an-array" }); + await expect( + api.getTemplateVersionResources("version-1"), + ).rejects.toBeInstanceOf(InvalidApiResponseError); + }); + + it("validates deployment SSH config", async () => { + api = createApi(); + mockResponse({ hostname_suffix: ".coder", ssh_config_options: {} }); + await expect(api.getDeploymentSSHConfig()).resolves.toEqual({ + hostname_suffix: ".coder", + ssh_config_options: {}, + }); + + mockResponse({}); + await expect(api.getDeploymentSSHConfig()).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..fe6233ad51 --- /dev/null +++ b/test/unit/api/responseValidation.test.ts @@ -0,0 +1,186 @@ +import { describe, expect, it } from "vitest"; +import { ZodError } from "zod"; + +import { + InvalidApiResponseError, + parseApiResponse, + SSHConfigResponseSchema, + UserSchema, + WorkspaceSchema, +} from "@/api/responseValidation"; + +const validUser = { + id: "user-1", + username: "developer", + roles: [{ name: "owner", display_name: "Owner" }], + organization_ids: ["org-1"], + email: "dev@example.com", + status: "active", +}; + +describe("parseApiResponse", () => { + it("returns the value when it matches the schema", () => { + const result = parseApiResponse( + UserSchema, + validUser, + "/api/v2/users/me", + "https://coder.example.com", + ); + expect(result).toEqual(validUser); + }); + + it("preserves unknown fields not in the schema", () => { + const withExtras = { ...validUser, future_field: { nested: [1, 2, 3] } }; + const result = parseApiResponse( + UserSchema, + withExtras, + "/api/v2/users/me", + "https://coder.example.com", + ); + expect(result).toEqual(withExtras); + }); + + it("throws InvalidApiResponseError naming the endpoint and URL", () => { + let caught: unknown; + try { + parseApiResponse( + UserSchema, + { id: "user-1" }, + "/api/v2/users/me", + "https://coder.example.com", + ); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(InvalidApiResponseError); + const error = caught as InvalidApiResponseError; + expect(error.message).toContain("https://coder.example.com"); + expect(error.message).toContain("/api/v2/users/me"); + expect(error.message).toContain( + "Check that the URL points to a Coder deployment", + ); + expect(error.cause).toBeInstanceOf(ZodError); + }); + + it("rejects a string body, e.g. an HTML proxy error page", () => { + expect(() => + parseApiResponse( + UserSchema, + "Login", + "/api/v2/users/me", + "https://proxy.example.com", + ), + ).toThrow(InvalidApiResponseError); + }); + + it("rejects null and empty objects", () => { + for (const body of [null, undefined, {}]) { + expect(() => + parseApiResponse(UserSchema, body, "/api/v2/users/me"), + ).toThrow(InvalidApiResponseError); + } + }); + + it("rejects a user with missing roles", () => { + const { roles: _roles, ...noRoles } = validUser; + expect(() => + parseApiResponse(UserSchema, noRoles, "/api/v2/users/me"), + ).toThrow(InvalidApiResponseError); + }); + + it("omits the URL from the message when not provided", () => { + let caught: unknown; + try { + parseApiResponse(UserSchema, {}, "/api/v2/users/me"); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(InvalidApiResponseError); + expect((caught as Error).message).toContain("The deployment"); + }); +}); + +describe("WorkspaceSchema", () => { + const validWorkspace = { + 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: [ + { + id: "agent-1", + name: "main", + status: "connected", + operating_system: "linux", + architecture: "amd64", + }, + ], + }, + ], + }, + }; + + it("accepts a workspace with agents", () => { + expect(() => + parseApiResponse( + WorkspaceSchema, + validWorkspace, + "/api/v2/workspaces/ws-1", + ), + ).not.toThrow(); + }); + + it("accepts resources with null or missing agents", () => { + const workspace = { + ...validWorkspace, + latest_build: { + ...validWorkspace.latest_build, + resources: [{ id: "res-1", agents: null }, { id: "res-2" }], + }, + }; + expect(() => + parseApiResponse(WorkspaceSchema, workspace, "/api/v2/workspaces/ws-1"), + ).not.toThrow(); + }); + + it("rejects a workspace without latest_build", () => { + const { latest_build: _lb, ...noBuild } = validWorkspace; + expect(() => + parseApiResponse(WorkspaceSchema, noBuild, "/api/v2/workspaces/ws-1"), + ).toThrow(InvalidApiResponseError); + }); +}); + +describe("SSHConfigResponseSchema", () => { + it("accepts a valid config with extra fields", () => { + const config = { + hostname_prefix: "coder.", + hostname_suffix: ".coder", + ssh_config_options: { ConnectTimeout: "30" }, + something_new: true, + }; + const result = parseApiResponse( + SSHConfigResponseSchema, + config, + "/api/v2/deployment/ssh", + ); + expect(result).toEqual(config); + }); + + it("rejects a config without hostname_suffix", () => { + expect(() => + parseApiResponse( + SSHConfigResponseSchema, + { ssh_config_options: {} }, + "/api/v2/deployment/ssh", + ), + ).toThrow(InvalidApiResponseError); + }); +}); diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 976a8d4069..866e3625f5 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, @@ -127,7 +129,7 @@ describe("OAuthAuthorizer", () => { "/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(); @@ -179,7 +181,7 @@ describe("OAuthAuthorizer", () => { "/.well-known/oauth-authorization-server": createMockOAuthMetadata(TEST_URL), "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, + "/api/v2/users/me": createMockUser(), }); const loginPromise = authorizer.login( @@ -220,7 +222,7 @@ describe("OAuthAuthorizer", () => { client_id: "new-client-id", }), "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, + "/api/v2/users/me": createMockUser(), }); const loginPromise = authorizer.login( @@ -507,5 +509,46 @@ describe("OAuthAuthorizer", () => { ), ).rejects.toThrow("Server does not support dynamic client registration"); }); + + it("throws when the token response has no access token", async () => { + const { mockAdapter, oauthCallback, authorizer } = createTestContext(); + + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": + createMockOAuthMetadata(TEST_URL), + "/oauth2/register": createMockClientRegistration(), + "/oauth2/token": { token_type: "Bearer" }, + "/api/v2/users/me": createMockUser(), + }); + + const loginPromise = authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ); + + const { state } = await waitForBrowserToOpen(); + await oauthCallback.send({ state, code: "auth-code-123", error: null }); + + await expect(loginPromise).rejects.toThrow(InvalidApiResponseError); + }); + + it("throws when the registration response has no client_id", async () => { + const { mockAdapter, authorizer } = createTestContext(); + + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": + createMockOAuthMetadata(TEST_URL), + "/oauth2/register": { client_secret: "no-id" }, + }); + + 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..5306d46ad7 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -89,7 +89,7 @@ describe("OAuthMetadataClient", () => { }); await expect(client.getMetadata()).rejects.toThrow( - "OAuth server metadata missing required endpoints", + "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..217235a67d 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, @@ -146,7 +147,7 @@ export function createBaseTestContext() { "/.well-known/oauth-authorization-server": metadata, "/oauth2/register": createMockClientRegistration(), "/oauth2/token": createMockTokenResponse(), - "/api/v2/users/me": { username: "test-user" }, + "/api/v2/users/me": createMockUser(), }); }; From bd02369cdd8253a74ac3d8f31187af8f1f5ea6c9 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sun, 9 Aug 2026 13:04:04 +0000 Subject: [PATCH 2/6] refactor(api): simplify response validation code and tests Add a private CoderApi.validate helper supplying the host so each override is a single call. Trim doc comments to the essential rationale and merge redundant passthrough tests into one identity assertion. --- src/api/coderApi.ts | 51 ++++++++++-------------- src/api/responseValidation.ts | 8 ++-- src/oauth/validation.ts | 5 +-- test/unit/api/responseValidation.test.ts | 36 +++++------------ 4 files changed, 38 insertions(+), 62 deletions(-) diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 712f1f8ab1..402c300b03 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -73,6 +73,7 @@ import type { WorkspaceResource, } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; +import type { z } from "zod"; import type { Logger } from "../logging/logger"; import type { @@ -164,26 +165,29 @@ export class CoderApi extends Api implements vscode.Disposable { /** * The SDK casts response bodies to the generated types with no runtime - * check, so a 2xx from a non-Coder service (or a proxy error page) would - * flow in as if valid and crash far from the cause. These overrides - * validate the fields the extension reads and throw - * InvalidApiResponseError, naming the endpoint, at the boundary instead. - * - * The SDK methods are instance arrow properties assigned during super(), - * and field initializers run in declaration order before the constructor - * body, so capturing `this.X` here reads the base implementation before - * the shadowing overrides below are assigned. `super.X` is unavailable - * for parent class fields (ts(2855)), and TS2729 on `this.X` is a false - * positive since the base constructor already ran. + * check, so a 2xx from a non-Coder service would crash far from the + * cause. The overrides below validate the fields the extension reads and + * throw InvalidApiResponseError at the boundary instead. Base + * implementations are captured here because they are instance arrow + * properties assigned during super(); this field is declared before the + * overrides, so field initialization order reads the base versions. + * `super.X` is unavailable for parent class fields (ts(2855)). */ private readonly baseMethods = captureBaseMethods(this); + private validate( + schema: z.ZodType, + data: T, + endpoint: string, + ): T { + return parseApiResponse(schema, data, endpoint, this.getHost()); + } + override getAuthenticatedUser = async (): Promise => { - return parseApiResponse( + return this.validate( UserSchema, await this.baseMethods.getAuthenticatedUser(), "/api/v2/users/me", - this.getHost(), ); }; @@ -191,11 +195,10 @@ export class CoderApi extends Api implements vscode.Disposable { workspaceId: string, params?: WorkspaceOptions, ): Promise => { - return parseApiResponse( + return this.validate( WorkspaceSchema, await this.baseMethods.getWorkspace(workspaceId, params), `/api/v2/workspaces/${workspaceId}`, - this.getHost(), ); }; @@ -204,7 +207,7 @@ export class CoderApi extends Api implements vscode.Disposable { workspaceName: string, params?: WorkspaceOptions, ): Promise => { - return parseApiResponse( + return this.validate( WorkspaceSchema, await this.baseMethods.getWorkspaceByOwnerAndName( username, @@ -212,7 +215,6 @@ export class CoderApi extends Api implements vscode.Disposable { params, ), `/api/v2/users/${username}/workspace/${workspaceName}`, - this.getHost(), ); }; @@ -221,7 +223,7 @@ export class CoderApi extends Api implements vscode.Disposable { workspaceName: string, buildNumber: number, ): Promise => { - return parseApiResponse( + return this.validate( WorkspaceBuildSchema, await this.baseMethods.getWorkspaceBuildByNumber( username, @@ -229,27 +231,24 @@ export class CoderApi extends Api implements vscode.Disposable { buildNumber, ), `/api/v2/users/${username}/workspace/${workspaceName}/builds/${buildNumber}`, - this.getHost(), ); }; override getTemplateVersionResources = async ( versionId: string, ): Promise => { - return parseApiResponse( + return this.validate( WorkspaceResourcesSchema, await this.baseMethods.getTemplateVersionResources(versionId), `/api/v2/templateversions/${versionId}/resources`, - this.getHost(), ); }; override getDeploymentSSHConfig = async (): Promise => { - return parseApiResponse( + return this.validate( SSHConfigResponseSchema, await this.baseMethods.getDeploymentSSHConfig(), "/api/v2/deployment/ssh", - this.getHost(), ); }; @@ -851,12 +850,6 @@ function wrapResponseTransform( ]; } -/** - * Capture the base SDK method implementations before the validating - * overrides on CoderApi shadow them. Reads `this.X` while the base class - * arrow-function fields are still in place (field initializers run in - * declaration order, and this field is declared before the overrides). - */ function captureBaseMethods(instance: Api) { return { getAuthenticatedUser: instance.getAuthenticatedUser, diff --git a/src/api/responseValidation.ts b/src/api/responseValidation.ts index 2a3a8ccaa5..270c1cf7d0 100644 --- a/src/api/responseValidation.ts +++ b/src/api/responseValidation.ts @@ -22,11 +22,9 @@ export class InvalidApiResponseError extends Error { } /** - * Validate a response body against a permissive schema, returning the - * original value with the caller's type. Schemas must use looseObject so - * unknown fields pass through: the point is to catch "this is not the object - * we expect", not to mirror the full API, so newer deployments adding fields - * never break. + * Validate a response body, returning the original value with the caller's + * type. Schemas must use looseObject so unknown fields pass through and + * newer deployments adding fields never break. * * @throws {InvalidApiResponseError} naming the endpoint when validation fails. */ diff --git a/src/oauth/validation.ts b/src/oauth/validation.ts index b0a8c9fa1c..3480f2ad3a 100644 --- a/src/oauth/validation.ts +++ b/src/oauth/validation.ts @@ -1,9 +1,8 @@ import { z } from "zod"; /** - * Permissive schemas for the OAuth endpoints hit directly via axios during - * login, before any session exists. Only the fields the flow reads are - * required; unknown fields pass through so newer deployments never break. + * Schemas for the OAuth endpoints hit directly via axios during login, + * before any session exists. Only the fields the flow reads are required. */ export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ issuer: z.string(), diff --git a/test/unit/api/responseValidation.test.ts b/test/unit/api/responseValidation.test.ts index fe6233ad51..78d3e83f3b 100644 --- a/test/unit/api/responseValidation.test.ts +++ b/test/unit/api/responseValidation.test.ts @@ -19,17 +19,7 @@ const validUser = { }; describe("parseApiResponse", () => { - it("returns the value when it matches the schema", () => { - const result = parseApiResponse( - UserSchema, - validUser, - "/api/v2/users/me", - "https://coder.example.com", - ); - expect(result).toEqual(validUser); - }); - - it("preserves unknown fields not in the schema", () => { + it("returns the value unchanged, preserving unknown fields", () => { const withExtras = { ...validUser, future_field: { nested: [1, 2, 3] } }; const result = parseApiResponse( UserSchema, @@ -37,27 +27,28 @@ describe("parseApiResponse", () => { "/api/v2/users/me", "https://coder.example.com", ); - expect(result).toEqual(withExtras); + expect(result).toBe(withExtras); }); it("throws InvalidApiResponseError naming the endpoint and URL", () => { - let caught: unknown; - try { + const call = () => parseApiResponse( UserSchema, { id: "user-1" }, "/api/v2/users/me", "https://coder.example.com", ); + + let caught: unknown; + try { + call(); } catch (error) { caught = error; } expect(caught).toBeInstanceOf(InvalidApiResponseError); const error = caught as InvalidApiResponseError; - expect(error.message).toContain("https://coder.example.com"); - expect(error.message).toContain("/api/v2/users/me"); expect(error.message).toContain( - "Check that the URL points to a Coder deployment", + "https://coder.example.com did not return a valid Coder API response for /api/v2/users/me", ); expect(error.cause).toBeInstanceOf(ZodError); }); @@ -89,14 +80,9 @@ describe("parseApiResponse", () => { }); it("omits the URL from the message when not provided", () => { - let caught: unknown; - try { - parseApiResponse(UserSchema, {}, "/api/v2/users/me"); - } catch (error) { - caught = error; - } - expect(caught).toBeInstanceOf(InvalidApiResponseError); - expect((caught as Error).message).toContain("The deployment"); + expect(() => parseApiResponse(UserSchema, {}, "/api/v2/users/me")).toThrow( + "The deployment did not return a valid Coder API response", + ); }); }); From bae1e2af6e099ed21e91d7227a296ae4bccf2531 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sun, 9 Aug 2026 13:08:13 +0000 Subject: [PATCH 3/6] test(api): assert passthrough via toEqual instead of an as-cast --- test/unit/api/coderApi.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index b44c297ffe..dfbb9a8cf7 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -921,13 +921,15 @@ describe("CoderApi", () => { it("preserves unknown fields on validated responses", async () => { api = createApi(); - mockResponse({ ...validUser, future_field: { nested: true } }); + const withExtra = { + ...validUser, + future_field: { nested: true }, + }; + mockResponse(withExtra); const user = await api.getAuthenticatedUser(); - expect((user as unknown as Record).future_field).toEqual( - { nested: true }, - ); + expect(user).toEqual(withExtra); }); it("validates getWorkspace and getWorkspaceByOwnerAndName", async () => { From 4f1103b7237f277b5f1bbdb59589765b6934048c Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 10 Aug 2026 15:41:06 +0300 Subject: [PATCH 4/6] fix(api): harden response validation at the boundary - Drop hostname_suffix, organization_ids, and architecture from the schemas; nothing reads them and older deployments do not send them - Replace the six validation overrides and captureBaseMethods with a typed wrap helper reassigned in the constructor - Reimplement waitForBuild, whose SDK version swallows errors inside a voided IIFE and would hang callers on a validation failure - Validate getTemplate, stopWorkspace, and startWorkspace responses too - Validate the *_supported arrays in OAuth metadata and blame the endpoint origin, not the deployment, in OAuth validation errors - Fold validateRequiredEndpoints into the OAuth metadata schema - Use safeParse and drop the misleading Output > Coder pointer - Pin the minimal old-deployment body each schema must keep accepting --- src/api/coderApi.ts | 176 ++++++++++------------- src/api/responseValidation.ts | 32 ++--- src/oauth/authorizer.ts | 8 +- src/oauth/metadataClient.ts | 16 --- src/oauth/sessionManager.ts | 6 +- src/oauth/validation.ts | 26 +++- test/unit/api/coderApi.test.ts | 72 ++++++++-- test/unit/api/responseValidation.test.ts | 68 +++++++-- test/unit/oauth/metadataClient.test.ts | 15 ++ 9 files changed, 253 insertions(+), 166 deletions(-) diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 402c300b03..cb0a5a5f5b 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -52,6 +52,7 @@ import { getRefreshCommand, refreshCertificates } from "./certificateRefresh"; import { parseApiResponse, SSHConfigResponseSchema, + TemplateSchema, UserSchema, WorkspaceBuildSchema, WorkspaceResourcesSchema, @@ -61,16 +62,13 @@ import { createHttpAgent } from "./utils"; import type { GetInboxNotificationResponse, + ProvisionerJob, ProvisionerJobLog, ServerSentEvent, - SSHConfigResponse, - User, Workspace, WorkspaceAgent, WorkspaceAgentLog, WorkspaceBuild, - WorkspaceOptions, - WorkspaceResource, } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; import type { z } from "zod"; @@ -128,6 +126,7 @@ export class CoderApi extends Api implements vscode.Disposable { private readonly authConfigTracker: AuthConfigTracker, ) { super(); + wrapWithValidation(this); this.configWatcher = this.watchConfigChanges(); } @@ -164,92 +163,27 @@ export class CoderApi extends Api implements vscode.Disposable { } /** - * The SDK casts response bodies to the generated types with no runtime - * check, so a 2xx from a non-Coder service would crash far from the - * cause. The overrides below validate the fields the extension reads and - * throw InvalidApiResponseError at the boundary instead. Base - * implementations are captured here because they are instance arrow - * properties assigned during super(); this field is declared before the - * overrides, so field initialization order reads the base versions. - * `super.X` is unavailable for parent class fields (ts(2855)). + * 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). */ - private readonly baseMethods = captureBaseMethods(this); - - private validate( - schema: z.ZodType, - data: T, - endpoint: string, - ): T { - return parseApiResponse(schema, data, endpoint, this.getHost()); - } - - override getAuthenticatedUser = async (): Promise => { - return this.validate( - UserSchema, - await this.baseMethods.getAuthenticatedUser(), - "/api/v2/users/me", - ); - }; - - override getWorkspace = async ( - workspaceId: string, - params?: WorkspaceOptions, - ): Promise => { - return this.validate( - WorkspaceSchema, - await this.baseMethods.getWorkspace(workspaceId, params), - `/api/v2/workspaces/${workspaceId}`, - ); - }; - - override getWorkspaceByOwnerAndName = async ( - username: string, - workspaceName: string, - params?: WorkspaceOptions, - ): Promise => { - return this.validate( - WorkspaceSchema, - await this.baseMethods.getWorkspaceByOwnerAndName( - username, - workspaceName, - params, - ), - `/api/v2/users/${username}/workspace/${workspaceName}`, - ); - }; - - override getWorkspaceBuildByNumber = async ( - username: string, - workspaceName: string, - buildNumber: number, - ): Promise => { - return this.validate( - WorkspaceBuildSchema, - await this.baseMethods.getWorkspaceBuildByNumber( - username, - workspaceName, - buildNumber, - ), - `/api/v2/users/${username}/workspace/${workspaceName}/builds/${buildNumber}`, - ); - }; - - override getTemplateVersionResources = async ( - versionId: string, - ): Promise => { - return this.validate( - WorkspaceResourcesSchema, - await this.baseMethods.getTemplateVersionResources(versionId), - `/api/v2/templateversions/${versionId}/resources`, - ); - }; - - override getDeploymentSSHConfig = async (): Promise => { - return this.validate( - SSHConfigResponseSchema, - await this.baseMethods.getDeploymentSSHConfig(), - "/api/v2/deployment/ssh", - ); + 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 { @@ -850,15 +784,61 @@ function wrapResponseTransform( ]; } -function captureBaseMethods(instance: Api) { - return { - getAuthenticatedUser: instance.getAuthenticatedUser, - getWorkspace: instance.getWorkspace, - getWorkspaceByOwnerAndName: instance.getWorkspaceByOwnerAndName, - getWorkspaceBuildByNumber: instance.getWorkspaceBuildByNumber, - getTemplateVersionResources: instance.getTemplateVersionResources, - getDeploymentSSHConfig: instance.getDeploymentSSHConfig, - }; +/** + * 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 wrap = + ( + name: string, + schema: z.ZodType, + method: (...args: Args) => Promise, + ) => + async (...args: Args): Promise => { + const url = api.getHost(); + return parseApiResponse(schema, await method(...args), name, url); + }; + + api.getAuthenticatedUser = wrap( + "getAuthenticatedUser", + UserSchema, + api.getAuthenticatedUser, + ); + api.getWorkspace = wrap("getWorkspace", WorkspaceSchema, api.getWorkspace); + api.getWorkspaceByOwnerAndName = wrap( + "getWorkspaceByOwnerAndName", + WorkspaceSchema, + api.getWorkspaceByOwnerAndName, + ); + api.getWorkspaceBuildByNumber = wrap( + "getWorkspaceBuildByNumber", + WorkspaceBuildSchema, + api.getWorkspaceBuildByNumber, + ); + api.getTemplateVersionResources = wrap( + "getTemplateVersionResources", + WorkspaceResourcesSchema, + api.getTemplateVersionResources, + ); + api.getDeploymentSSHConfig = wrap( + "getDeploymentSSHConfig", + SSHConfigResponseSchema, + api.getDeploymentSSHConfig, + ); + api.getTemplate = wrap("getTemplate", TemplateSchema, api.getTemplate); + api.stopWorkspace = wrap( + "stopWorkspace", + WorkspaceBuildSchema, + api.stopWorkspace, + ); + api.startWorkspace = wrap( + "startWorkspace", + WorkspaceBuildSchema, + api.startWorkspace, + ); } function getSize(headers: AxiosHeaders, data: unknown): number | undefined { diff --git a/src/api/responseValidation.ts b/src/api/responseValidation.ts index 270c1cf7d0..36adc3cbc8 100644 --- a/src/api/responseValidation.ts +++ b/src/api/responseValidation.ts @@ -1,4 +1,4 @@ -import { ZodError, z } from "zod"; +import { z } from "zod"; /** * Thrown when a 2xx response body does not match the shape the extension @@ -13,8 +13,7 @@ export class InvalidApiResponseError extends Error { ) { super( `${url ?? "The deployment"} did not return a valid Coder API response ` + - `for ${endpoint}. Check that the URL points to a Coder deployment. ` + - "See Output > Coder for details.", + `for ${endpoint}. Check that the URL points to a Coder deployment.`, options, ); this.name = "InvalidApiResponseError"; @@ -34,26 +33,22 @@ export function parseApiResponse( endpoint: string, url?: string, ): T { - try { - schema.parse(data); - } catch (error) { - if (error instanceof ZodError) { - throw new InvalidApiResponseError(endpoint, url, { cause: error }); - } - throw error; + const result = schema.safeParse(data); + if (!result.success) { + throw new InvalidApiResponseError(endpoint, url, { cause: result.error }); } return data; } /** - * Only the fields the extension reads are required; everything else passes - * through untouched. + * Only fields the extension reads appear here. A field is required only if + * every deployment version sends it; newer fields must be .optional() and + * their consumers must handle the absence. */ export const UserSchema = z.looseObject({ id: z.string(), username: z.string(), roles: z.array(z.looseObject({ name: z.string() })), - organization_ids: z.array(z.string()), }); const WorkspaceAgentSchema = z.looseObject({ @@ -61,7 +56,6 @@ const WorkspaceAgentSchema = z.looseObject({ name: z.string(), status: z.string(), operating_system: z.string(), - architecture: z.string(), }); const WorkspaceResourceSchema = z.looseObject({ @@ -81,14 +75,20 @@ export const WorkspaceSchema = z.looseObject({ }), }); -/** waitForBuild polls `job.status`; a malformed job would loop forever. */ +/** 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({ - hostname_suffix: z.string(), ssh_config_options: z.record(z.string(), z.string()), }); diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 85c66ce894..b22bb90a70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -1,7 +1,6 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; -import { parseApiResponse } from "../api/responseValidation"; import { resolveCoderDashboardUrl } from "../util/uri"; import { @@ -21,6 +20,7 @@ import { import { OAuth2ClientRegistrationResponseSchema, OAuth2TokenResponseSchema, + parseOAuthResponse, } from "./validation"; import type { AxiosInstance } from "axios"; @@ -178,11 +178,10 @@ export class OAuthAuthorizer implements vscode.Disposable { registrationRequest, ); - const registrationResponse = parseApiResponse( + const registrationResponse = parseOAuthResponse( OAuth2ClientRegistrationResponseSchema, response.data, metadata.registration_endpoint, - axiosInstance.defaults.baseURL, ); await this.secretsManager.setOAuthClientRegistration( @@ -372,11 +371,10 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("Token exchange successful"); - return parseApiResponse( + return parseOAuthResponse( OAuth2TokenResponseSchema, response.data, metadata.token_endpoint, - axiosInstance.defaults.baseURL, ); } diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index 583cc4de90..b6b5b4e24f 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -79,7 +79,6 @@ export class OAuthMetadataClient { this.axiosInstance.defaults.baseURL, ); - this.validateRequiredEndpoints(metadata); this.validateGrantTypes(metadata); this.validateResponseTypes(metadata); this.validateAuthMethods(metadata); @@ -95,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 434f985891..5ccc122aa5 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -1,5 +1,4 @@ import { CoderApi } from "../api/coderApi"; -import { parseApiResponse } from "../api/responseValidation"; import { AuthTelemetry, type AuthTokenRefreshTrigger, @@ -9,7 +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 } from "./validation"; +import { OAuth2TokenResponseSchema, parseOAuthResponse } from "./validation"; import type { AxiosInstance } from "axios"; import type { @@ -423,11 +422,10 @@ export class OAuthSessionManager implements vscode.Disposable { this.logger.debug("Token refresh successful"); - const tokenResponse = parseApiResponse( + const tokenResponse = parseOAuthResponse( OAuth2TokenResponseSchema, response.data, metadata.token_endpoint, - axiosInstance.defaults.baseURL, ); await this.secretsManager.setSessionAuth(deployment.safeHostname, { diff --git a/src/oauth/validation.ts b/src/oauth/validation.ts index 3480f2ad3a..f577b6a06c 100644 --- a/src/oauth/validation.ts +++ b/src/oauth/validation.ts @@ -1,17 +1,37 @@ import { z } from "zod"; +import { parseApiResponse } from "../api/responseValidation"; + /** * Schemas for the OAuth endpoints hit directly via axios during login, * before any session exists. Only the fields the flow reads are required. */ export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ - issuer: z.string(), - authorization_endpoint: z.string(), - token_endpoint: z.string(), + issuer: z.string().min(1), + authorization_endpoint: z.string().min(1), + token_endpoint: z.string().min(1), registration_endpoint: z.string().optional(), revocation_endpoint: z.string().optional(), + grant_types_supported: z.array(z.string()).optional(), + response_types_supported: z.array(z.string()).optional(), + token_endpoint_auth_methods_supported: z.array(z.string()).optional(), + code_challenge_methods_supported: z.array(z.string()).optional(), + scopes_supported: z.array(z.string()).optional(), }); +/** + * parseApiResponse for OAuth endpoints, which are absolute URLs from + * server metadata and may live on a different origin than the deployment. + */ +export function parseOAuthResponse( + schema: z.ZodType, + data: T, + endpoint: string, +): T { + const { origin, pathname } = new URL(endpoint); + return parseApiResponse(schema, data, pathname, origin); +} + export const OAuth2ClientRegistrationResponseSchema = z.looseObject({ client_id: z.string(), client_secret: z.string().optional(), diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index dfbb9a8cf7..052eee18e4 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -36,10 +36,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"; @@ -858,13 +862,10 @@ describe("CoderApi", () => { }); }; - const validUser = { - id: "user-1", + const validUser = createMockUser({ username: "developer", - roles: [{ name: "owner" }], - organization_ids: ["org-1"], - email: "dev@example.com", - }; + roles: [{ name: "owner", display_name: "Owner" }], + }); const validWorkspace = { id: "ws-1", @@ -914,7 +915,7 @@ describe("CoderApi", () => { const promise = api.getAuthenticatedUser(); await expect(promise).rejects.toBeInstanceOf(InvalidApiResponseError); await expect(promise).rejects.toThrow( - `${CODER_URL} did not return a valid Coder API response for /api/v2/users/me`, + `${CODER_URL} did not return a valid Coder API response for getAuthenticatedUser`, ); }, ); @@ -943,12 +944,19 @@ describe("CoderApi", () => { ).rejects.toBeInstanceOf(InvalidApiResponseError); }); + const validBuild = { + workspace_owner_name: "me", + workspace_name: "dev", + build_number: 1, + job: { status: "succeeded" }, + } as WorkspaceBuild; + it("validates the build job status for waitForBuild polling", async () => { api = createApi(); - mockResponse({ job: { status: "succeeded" } }); + mockResponse(validBuild); await expect( api.getWorkspaceBuildByNumber("me", "dev", 1), - ).resolves.toEqual({ job: { status: "succeeded" } }); + ).resolves.toEqual(validBuild); mockResponse({ id: "build-without-job" }); await expect( @@ -956,6 +964,45 @@ describe("CoderApi", () => { ).rejects.toBeInstanceOf(InvalidApiResponseError); }); + it("waitForBuild resolves settled jobs and throws on failed ones", async () => { + api = createApi(); + mockResponse(validBuild); + await expect(api.waitForBuild(validBuild)).resolves.toEqual({ + status: "succeeded", + }); + + mockResponse({ ...validBuild, job: { status: "failed" } }); + await expect(api.waitForBuild(validBuild)).rejects.toThrow( + "Build 1 failed", + ); + }); + + it("waitForBuild surfaces validation errors instead of hanging", async () => { + api = createApi(); + mockResponse({ ...validBuild, job: {} }); + await expect(api.waitForBuild(validBuild)).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); + }); + + it("validates getTemplate, stopWorkspace, and startWorkspace", async () => { + api = createApi(); + mockResponse({ active_version_id: "v1" }); + await expect(api.getTemplate("tpl-1")).resolves.toEqual({ + active_version_id: "v1", + }); + + mockResponse("Bad Gateway"); + await expect(api.stopWorkspace("ws-1")).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); + + mockResponse({ id: "build-without-job" }); + await expect(api.startWorkspace("ws-1", "v1")).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); + }); + it("validates template version resources", async () => { api = createApi(); mockResponse([{ id: "res-1", agents: null }]); @@ -971,10 +1018,9 @@ describe("CoderApi", () => { it("validates deployment SSH config", async () => { api = createApi(); - mockResponse({ hostname_suffix: ".coder", ssh_config_options: {} }); + mockResponse({ ssh_config_options: { ConnectTimeout: "30" } }); await expect(api.getDeploymentSSHConfig()).resolves.toEqual({ - hostname_suffix: ".coder", - ssh_config_options: {}, + ssh_config_options: { ConnectTimeout: "30" }, }); mockResponse({}); diff --git a/test/unit/api/responseValidation.test.ts b/test/unit/api/responseValidation.test.ts index 78d3e83f3b..b2603c689d 100644 --- a/test/unit/api/responseValidation.test.ts +++ b/test/unit/api/responseValidation.test.ts @@ -1,22 +1,20 @@ import { describe, expect, it } from "vitest"; -import { ZodError } from "zod"; +import { ZodError, type z } from "zod"; import { InvalidApiResponseError, parseApiResponse, SSHConfigResponseSchema, + TemplateSchema, UserSchema, + WorkspaceBuildSchema, + WorkspaceResourcesSchema, WorkspaceSchema, } from "@/api/responseValidation"; -const validUser = { - id: "user-1", - username: "developer", - roles: [{ name: "owner", display_name: "Owner" }], - organization_ids: ["org-1"], - email: "dev@example.com", - status: "active", -}; +import { createMockUser } from "../../mocks/testHelpers"; + +const validUser = createMockUser(); describe("parseApiResponse", () => { it("returns the value unchanged, preserving unknown fields", () => { @@ -160,13 +158,61 @@ describe("SSHConfigResponseSchema", () => { expect(result).toEqual(config); }); - it("rejects a config without hostname_suffix", () => { + it("rejects a config without ssh_config_options", () => { expect(() => parseApiResponse( SSHConfigResponseSchema, - { ssh_config_options: {} }, + { hostname_prefix: "coder." }, "/api/v2/deployment/ssh", ), ).toThrow(InvalidApiResponseError); }); }); + +/** + * Pins the oldest body each schema must keep accepting; a new field that + * breaks one of these must be .optional() instead. + */ +describe("backward compatibility", () => { + it.each<[string, z.ZodType, unknown]>([ + ["UserSchema", UserSchema, { id: "u", username: "dev", roles: [] }], + [ + "WorkspaceSchema", + WorkspaceSchema, + { + id: "ws", + name: "dev", + owner_name: "me", + template_id: "tpl", + latest_build: { + id: "b", + status: "running", + template_version_id: "v", + resources: [], + }, + }, + ], + [ + "WorkspaceBuildSchema", + WorkspaceBuildSchema, + { + workspace_owner_name: "me", + workspace_name: "dev", + build_number: 1, + job: { status: "ok" }, + }, + ], + ["WorkspaceResourcesSchema", WorkspaceResourcesSchema, [{}]], + ["TemplateSchema", TemplateSchema, { active_version_id: "v" }], + [ + "SSHConfigResponseSchema", + SSHConfigResponseSchema, + { ssh_config_options: {} }, + ], + ])( + "%s accepts the minimal body an old deployment sends", + (_, schema, body) => { + expect(schema.safeParse(body).success).toBe(true); + }, + ); +}); diff --git a/test/unit/oauth/metadataClient.test.ts b/test/unit/oauth/metadataClient.test.ts index 5306d46ad7..f3508b8255 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -95,6 +95,21 @@ describe("OAuthMetadataClient", () => { ); }); + it("throws when a *_supported field is not an array", async () => { + const { mockAdapter, client } = createTestContext(); + + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": { + ...createMockOAuthMetadata(TEST_URL), + grant_types_supported: "authorization_code refresh_token", + }, + }); + + await expect(client.getMetadata()).rejects.toThrow( + "did not return a valid Coder API response", + ); + }); + describe("grant type validation", () => { it("accepts metadata with required grant types", async () => { const { mockAdapter, client } = createTestContext(); From 8dfa12616e6286a04a822d3c9a77934c41189d3b Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 11 Aug 2026 14:08:03 +0300 Subject: [PATCH 5/6] refactor(api): drive response validation from a schema map Replace the nine hand-written wrapper assignments with a single VALIDATED_RESPONSES map next to the schemas it references, so adding a validated method is a one-line change and the key serves as both the method to wrap and the endpoint name in the error. Mirror the layout on the OAuth side (helper first, schemas below) and collapse the repeated field expressions. Rework the tests around one table per concern, including a guard that fails when a method is added to the map without a case covering it. --- src/api/coderApi.ts | 68 +----- src/api/responseValidation.ts | 33 ++- src/oauth/validation.ts | 47 ++-- test/unit/api/coderApi.test.ts | 231 +++++++++--------- test/unit/api/responseValidation.test.ts | 295 ++++++++++------------- test/unit/oauth/authorizer.test.ts | 54 ++--- test/unit/oauth/metadataClient.test.ts | 36 ++- test/unit/oauth/testUtils.ts | 7 +- 8 files changed, 334 insertions(+), 437 deletions(-) diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index cb0a5a5f5b..24b67347d4 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -49,15 +49,7 @@ import { import { SseConnection } from "../websocket/sseConnection"; import { getRefreshCommand, refreshCertificates } from "./certificateRefresh"; -import { - parseApiResponse, - SSHConfigResponseSchema, - TemplateSchema, - UserSchema, - WorkspaceBuildSchema, - WorkspaceResourcesSchema, - WorkspaceSchema, -} from "./responseValidation"; +import { parseApiResponse, VALIDATED_RESPONSES } from "./responseValidation"; import { createHttpAgent } from "./utils"; import type { @@ -71,7 +63,6 @@ import type { WorkspaceBuild, } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; -import type { z } from "zod"; import type { Logger } from "../logging/logger"; import type { @@ -791,54 +782,15 @@ function wrapResponseTransform( * `override` fields would depend on declaration order. */ function wrapWithValidation(api: CoderApi): void { - const wrap = - ( - name: string, - schema: z.ZodType, - method: (...args: Args) => Promise, - ) => - async (...args: Args): Promise => { - const url = api.getHost(); - return parseApiResponse(schema, await method(...args), name, url); - }; - - api.getAuthenticatedUser = wrap( - "getAuthenticatedUser", - UserSchema, - api.getAuthenticatedUser, - ); - api.getWorkspace = wrap("getWorkspace", WorkspaceSchema, api.getWorkspace); - api.getWorkspaceByOwnerAndName = wrap( - "getWorkspaceByOwnerAndName", - WorkspaceSchema, - api.getWorkspaceByOwnerAndName, - ); - api.getWorkspaceBuildByNumber = wrap( - "getWorkspaceBuildByNumber", - WorkspaceBuildSchema, - api.getWorkspaceBuildByNumber, - ); - api.getTemplateVersionResources = wrap( - "getTemplateVersionResources", - WorkspaceResourcesSchema, - api.getTemplateVersionResources, - ); - api.getDeploymentSSHConfig = wrap( - "getDeploymentSSHConfig", - SSHConfigResponseSchema, - api.getDeploymentSSHConfig, - ); - api.getTemplate = wrap("getTemplate", TemplateSchema, api.getTemplate); - api.stopWorkspace = wrap( - "stopWorkspace", - WorkspaceBuildSchema, - api.stopWorkspace, - ); - api.startWorkspace = wrap( - "startWorkspace", - WorkspaceBuildSchema, - api.startWorkspace, - ); + const methods = api as unknown as Record< + string, + (...args: unknown[]) => Promise + >; + for (const [name, schema] of Object.entries(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 { diff --git a/src/api/responseValidation.ts b/src/api/responseValidation.ts index 36adc3cbc8..21778e323f 100644 --- a/src/api/responseValidation.ts +++ b/src/api/responseValidation.ts @@ -1,5 +1,7 @@ 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 @@ -22,8 +24,12 @@ export class InvalidApiResponseError extends Error { /** * Validate a response body, returning the original value with the caller's - * type. Schemas must use looseObject so unknown fields pass through and - * newer deployments adding fields never break. + * type. + * + * Every schema passed here lists only the fields the extension reads and uses + * looseObject so unknown fields pass through. A field may be required only if + * every supported deployment sends it (Coder 0.25 and up, see featureSet); + * anything newer must be .optional() with its consumers handling the absence. * * @throws {InvalidApiResponseError} naming the endpoint when validation fails. */ @@ -40,11 +46,6 @@ export function parseApiResponse( return data; } -/** - * Only fields the extension reads appear here. A field is required only if - * every deployment version sends it; newer fields must be .optional() and - * their consumers must handle the absence. - */ export const UserSchema = z.looseObject({ id: z.string(), username: z.string(), @@ -92,3 +93,21 @@ export const WorkspaceResourcesSchema = z.array(WorkspaceResourceSchema); export const SSHConfigResponseSchema = z.looseObject({ ssh_config_options: z.record(z.string(), z.string()), }); + +/** + * The schema each validated SDK method's response must match, applied by + * CoderApi. Add an entry to validate another method; the key doubles as the + * endpoint name in the error. The OAuth endpoints are plain axios calls + * rather than SDK methods, so they 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 Readonly>>; diff --git a/src/oauth/validation.ts b/src/oauth/validation.ts index f577b6a06c..16aaef81af 100644 --- a/src/oauth/validation.ts +++ b/src/oauth/validation.ts @@ -2,26 +2,10 @@ import { z } from "zod"; import { parseApiResponse } from "../api/responseValidation"; -/** - * Schemas for the OAuth endpoints hit directly via axios during login, - * before any session exists. Only the fields the flow reads are required. - */ -export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ - issuer: z.string().min(1), - authorization_endpoint: z.string().min(1), - token_endpoint: z.string().min(1), - registration_endpoint: z.string().optional(), - revocation_endpoint: z.string().optional(), - grant_types_supported: z.array(z.string()).optional(), - response_types_supported: z.array(z.string()).optional(), - token_endpoint_auth_methods_supported: z.array(z.string()).optional(), - code_challenge_methods_supported: z.array(z.string()).optional(), - scopes_supported: z.array(z.string()).optional(), -}); - /** * parseApiResponse for OAuth endpoints, which are absolute URLs from * server metadata and may live on a different origin than the deployment. + * The schemas below follow the same rules as parseApiResponse's. */ export function parseOAuthResponse( schema: z.ZodType, @@ -32,14 +16,39 @@ export function parseOAuthResponse( 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); + +/** + * Advertised capabilities, kept as plain strings rather than the generated + * enums so a server adding a value does not fail validation. Absent means the + * caller applies the RFC 8414 default. + */ +const CAPABILITIES = z.array(z.string()).optional(); + +export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ + issuer: REQUIRED_STRING, + authorization_endpoint: REQUIRED_STRING, + token_endpoint: REQUIRED_STRING, + // Absence is how a server signals it does not support these, so an empty + // string stays valid here and the callers report the missing capability. + 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: z.string(), + client_id: REQUIRED_STRING, client_secret: z.string().optional(), redirect_uris: z.array(z.string()).optional(), }); export const OAuth2TokenResponseSchema = z.looseObject({ - access_token: z.string(), + 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 052eee18e4..1164607fde 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -22,7 +22,10 @@ import { refreshCertificates, } from "@/api/certificateRefresh"; import { CoderApi, DEFAULT_REQUEST_TIMEOUT_MS } from "@/api/coderApi"; -import { InvalidApiResponseError } from "@/api/responseValidation"; +import { + InvalidApiResponseError, + VALIDATED_RESPONSES, +} from "@/api/responseValidation"; import { createHttpAgent } from "@/api/utils"; import { CONFIG_CHANGE_DEBOUNCE_MS } from "@/configWatcher"; import { ClientCertificateError } from "@/error/clientCertificateError"; @@ -851,7 +854,7 @@ describe("CoderApi", () => { ); }); - describe("Response Validation", () => { + describe("response validation", () => { const mockResponse = (data: unknown) => { mockAdapter.mockResolvedValueOnce({ data, @@ -862,12 +865,7 @@ describe("CoderApi", () => { }); }; - const validUser = createMockUser({ - username: "developer", - roles: [{ name: "owner", display_name: "Owner" }], - }); - - const validWorkspace = { + const VALID_WORKSPACE = { id: "ws-1", name: "dev", owner_name: "developer", @@ -880,151 +878,144 @@ describe("CoderApi", () => { }, }; - it("returns the user when /users/me is valid", async () => { - api = createApi(); - mockResponse(validUser); - - const user = await api.getAuthenticatedUser(); - - expect(user.username).toBe("developer"); - expect(user.roles[0]?.name).toBe("owner"); - }); - - it("still performs the underlying HTTP request", async () => { - api = createApi(); - mockResponse(validUser); + const VALID_BUILD = { + workspace_owner_name: "developer", + workspace_name: "dev", + build_number: 1, + job: { status: "succeeded" }, + }; - await api.getAuthenticatedUser(); + /** One realistic body per validated method, keyed by the method name. */ + const CASES: ReadonlyArray<{ + method: keyof typeof VALIDATED_RESPONSES; + 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, + }, + ]; - expect(mockAdapter).toHaveBeenCalledWith( - expect.objectContaining({ url: "/api/v2/users/me" }), + it("exercises every validated method", () => { + expect(CASES.map((testCase) => testCase.method).sort()).toEqual( + Object.keys(VALIDATED_RESPONSES).sort(), ); }); - it.each([ - ["an HTML error page", "Bad Gateway"], - ["null", null], - ["an empty object", {}], - ["a user missing roles", { id: "user-1", username: "developer" }], - ])( - "rejects /users/me returning %s with the endpoint and URL", - async (_description, body) => { + it.each(CASES)( + "$method passes a valid body through", + async ({ call, valid }) => { api = createApi(); - mockResponse(body); + mockResponse(valid); - const promise = api.getAuthenticatedUser(); - await expect(promise).rejects.toBeInstanceOf(InvalidApiResponseError); - await expect(promise).rejects.toThrow( - `${CODER_URL} did not return a valid Coder API response for getAuthenticatedUser`, - ); + await expect(call(api)).resolves.toEqual(valid); }, ); - it("preserves unknown fields on validated responses", async () => { - api = createApi(); - const withExtra = { - ...validUser, - future_field: { nested: true }, - }; - mockResponse(withExtra); - - const user = await api.getAuthenticatedUser(); + it.each(CASES)( + "$method rejects a body that is not from Coder", + async ({ method, call }) => { + api = createApi(); + mockResponse("Bad Gateway"); - expect(user).toEqual(withExtra); - }); + await expect(call(api)).rejects.toThrow( + `${CODER_URL} did not return a valid Coder API response for ${method}`, + ); + }, + ); - it("validates getWorkspace and getWorkspaceByOwnerAndName", async () => { + it("reports an unparseable response as InvalidApiResponseError", async () => { api = createApi(); - mockResponse(validWorkspace); - await expect(api.getWorkspace("ws-1")).resolves.toEqual(validWorkspace); + mockResponse({ id: "user-1" }); - mockResponse({ name: "not-a-workspace" }); - await expect( - api.getWorkspaceByOwnerAndName("me", "dev"), - ).rejects.toBeInstanceOf(InvalidApiResponseError); + await expect(api.getAuthenticatedUser()).rejects.toBeInstanceOf( + InvalidApiResponseError, + ); }); + }); - const validBuild = { - workspace_owner_name: "me", + describe("waitForBuild", () => { + const BUILD = { + workspace_owner_name: "developer", workspace_name: "dev", build_number: 1, job: { status: "succeeded" }, } as WorkspaceBuild; - it("validates the build job status for waitForBuild polling", async () => { - api = createApi(); - mockResponse(validBuild); - await expect( - api.getWorkspaceBuildByNumber("me", "dev", 1), - ).resolves.toEqual(validBuild); - - mockResponse({ id: "build-without-job" }); - await expect( - api.getWorkspaceBuildByNumber("me", "dev", 1), - ).rejects.toBeInstanceOf(InvalidApiResponseError); - }); - - it("waitForBuild resolves settled jobs and throws on failed ones", async () => { - api = createApi(); - mockResponse(validBuild); - await expect(api.waitForBuild(validBuild)).resolves.toEqual({ - status: "succeeded", + const mockPoll = (job: unknown) => { + mockAdapter.mockResolvedValueOnce({ + data: { ...BUILD, job }, + status: 200, + statusText: "OK", + headers: {}, + config: {}, }); + }; - mockResponse({ ...validBuild, job: { status: "failed" } }); - await expect(api.waitForBuild(validBuild)).rejects.toThrow( - "Build 1 failed", - ); - }); - - it("waitForBuild surfaces validation errors instead of hanging", async () => { + it("returns the job once the build settles", async () => { api = createApi(); - mockResponse({ ...validBuild, job: {} }); - await expect(api.waitForBuild(validBuild)).rejects.toBeInstanceOf( - InvalidApiResponseError, - ); - }); + mockPoll({ status: "succeeded" }); - it("validates getTemplate, stopWorkspace, and startWorkspace", async () => { - api = createApi(); - mockResponse({ active_version_id: "v1" }); - await expect(api.getTemplate("tpl-1")).resolves.toEqual({ - active_version_id: "v1", + await expect(api.waitForBuild(BUILD)).resolves.toEqual({ + status: "succeeded", }); - - mockResponse("Bad Gateway"); - await expect(api.stopWorkspace("ws-1")).rejects.toBeInstanceOf( - InvalidApiResponseError, - ); - - mockResponse({ id: "build-without-job" }); - await expect(api.startWorkspace("ws-1", "v1")).rejects.toBeInstanceOf( - InvalidApiResponseError, - ); }); - it("validates template version resources", async () => { + it("throws when the build failed", async () => { api = createApi(); - mockResponse([{ id: "res-1", agents: null }]); - await expect( - api.getTemplateVersionResources("version-1"), - ).resolves.toEqual([{ id: "res-1", agents: null }]); - - mockResponse({ resources: "not-an-array" }); - await expect( - api.getTemplateVersionResources("version-1"), - ).rejects.toBeInstanceOf(InvalidApiResponseError); + mockPoll({ status: "failed" }); + + await expect(api.waitForBuild(BUILD)).rejects.toThrow("Build 1 failed"); }); - it("validates deployment SSH config", async () => { + // The SDK version swallows poll errors, leaving callers hanging forever. + it("surfaces a validation error instead of polling forever", async () => { api = createApi(); - mockResponse({ ssh_config_options: { ConnectTimeout: "30" } }); - await expect(api.getDeploymentSSHConfig()).resolves.toEqual({ - ssh_config_options: { ConnectTimeout: "30" }, - }); + mockPoll({}); - mockResponse({}); - await expect(api.getDeploymentSSHConfig()).rejects.toBeInstanceOf( + await expect(api.waitForBuild(BUILD)).rejects.toBeInstanceOf( InvalidApiResponseError, ); }); diff --git a/test/unit/api/responseValidation.test.ts b/test/unit/api/responseValidation.test.ts index b2603c689d..1b9b476fee 100644 --- a/test/unit/api/responseValidation.test.ts +++ b/test/unit/api/responseValidation.test.ts @@ -14,205 +14,156 @@ import { import { createMockUser } from "../../mocks/testHelpers"; -const validUser = createMockUser(); +const ENDPOINT = "/api/v2/users/me"; +const DEPLOYMENT_URL = "https://coder.example.com"; + +/** + * The smallest body each schema must keep accepting: what Coder 0.25, the + * oldest supported deployment, sends. Making one of these fail is a breaking + * change, so a newly required field belongs in the schema as .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 value unchanged, preserving unknown fields", () => { - const withExtras = { ...validUser, future_field: { nested: [1, 2, 3] } }; - const result = parseApiResponse( - UserSchema, - withExtras, - "/api/v2/users/me", - "https://coder.example.com", + 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, ); - expect(result).toBe(withExtras); }); it("throws InvalidApiResponseError naming the endpoint and URL", () => { - const call = () => - parseApiResponse( - UserSchema, - { id: "user-1" }, - "/api/v2/users/me", - "https://coder.example.com", - ); + expect(() => + parseApiResponse(UserSchema, {}, ENDPOINT, DEPLOYMENT_URL), + ).toThrow( + `${DEPLOYMENT_URL} did not return a valid Coder API response for ${ENDPOINT}`, + ); + }); - let caught: unknown; + it("keeps the Zod failure as the cause", () => { try { - call(); + parseApiResponse(UserSchema, {}, ENDPOINT, DEPLOYMENT_URL); + expect.unreachable("should have thrown"); } catch (error) { - caught = error; + expect(error).toBeInstanceOf(InvalidApiResponseError); + expect((error as InvalidApiResponseError).cause).toBeInstanceOf(ZodError); } - expect(caught).toBeInstanceOf(InvalidApiResponseError); - const error = caught as InvalidApiResponseError; - expect(error.message).toContain( - "https://coder.example.com did not return a valid Coder API response for /api/v2/users/me", - ); - expect(error.cause).toBeInstanceOf(ZodError); }); - it("rejects a string body, e.g. an HTML proxy error page", () => { - expect(() => - parseApiResponse( - UserSchema, - "Login", - "/api/v2/users/me", - "https://proxy.example.com", - ), - ).toThrow(InvalidApiResponseError); + 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", + ); }); - it("rejects null and empty objects", () => { - for (const body of [null, undefined, {}]) { - expect(() => - parseApiResponse(UserSchema, body, "/api/v2/users/me"), - ).toThrow(InvalidApiResponseError); - } + // The bodies a misdirected URL realistically returns: a proxy login page, a + // 204-style 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, + ); }); +}); - it("rejects a user with missing roles", () => { - const { roles: _roles, ...noRoles } = validUser; - expect(() => - parseApiResponse(UserSchema, noRoles, "/api/v2/users/me"), - ).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("omits the URL from the message when not provided", () => { - expect(() => parseApiResponse(UserSchema, {}, "/api/v2/users/me")).toThrow( - "The deployment did not return a valid Coder API response", - ); + it("rejects a body missing a required field", () => { + expect(schema.safeParse(incomplete).success).toBe(false); }); }); describe("WorkspaceSchema", () => { - const validWorkspace = { - 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: [ - { - id: "agent-1", - name: "main", - status: "connected", - operating_system: "linux", - architecture: "amd64", - }, - ], - }, - ], - }, - }; - - it("accepts a workspace with agents", () => { - expect(() => - parseApiResponse( - WorkspaceSchema, - validWorkspace, - "/api/v2/workspaces/ws-1", - ), - ).not.toThrow(); - }); - - it("accepts resources with null or missing agents", () => { + it("accepts resources whose agents are null or absent", () => { const workspace = { - ...validWorkspace, + id: "ws-1", + name: "dev", + owner_name: "developer", + template_id: "tpl-1", latest_build: { - ...validWorkspace.latest_build, + id: "build-1", + status: "running", + template_version_id: "version-1", resources: [{ id: "res-1", agents: null }, { id: "res-2" }], }, }; - expect(() => - parseApiResponse(WorkspaceSchema, workspace, "/api/v2/workspaces/ws-1"), - ).not.toThrow(); - }); - it("rejects a workspace without latest_build", () => { - const { latest_build: _lb, ...noBuild } = validWorkspace; - expect(() => - parseApiResponse(WorkspaceSchema, noBuild, "/api/v2/workspaces/ws-1"), - ).toThrow(InvalidApiResponseError); + expect(WorkspaceSchema.safeParse(workspace).success).toBe(true); }); }); - -describe("SSHConfigResponseSchema", () => { - it("accepts a valid config with extra fields", () => { - const config = { - hostname_prefix: "coder.", - hostname_suffix: ".coder", - ssh_config_options: { ConnectTimeout: "30" }, - something_new: true, - }; - const result = parseApiResponse( - SSHConfigResponseSchema, - config, - "/api/v2/deployment/ssh", - ); - expect(result).toEqual(config); - }); - - it("rejects a config without ssh_config_options", () => { - expect(() => - parseApiResponse( - SSHConfigResponseSchema, - { hostname_prefix: "coder." }, - "/api/v2/deployment/ssh", - ), - ).toThrow(InvalidApiResponseError); - }); -}); - -/** - * Pins the oldest body each schema must keep accepting; a new field that - * breaks one of these must be .optional() instead. - */ -describe("backward compatibility", () => { - it.each<[string, z.ZodType, unknown]>([ - ["UserSchema", UserSchema, { id: "u", username: "dev", roles: [] }], - [ - "WorkspaceSchema", - WorkspaceSchema, - { - id: "ws", - name: "dev", - owner_name: "me", - template_id: "tpl", - latest_build: { - id: "b", - status: "running", - template_version_id: "v", - resources: [], - }, - }, - ], - [ - "WorkspaceBuildSchema", - WorkspaceBuildSchema, - { - workspace_owner_name: "me", - workspace_name: "dev", - build_number: 1, - job: { status: "ok" }, - }, - ], - ["WorkspaceResourcesSchema", WorkspaceResourcesSchema, [{}]], - ["TemplateSchema", TemplateSchema, { active_version_id: "v" }], - [ - "SSHConfigResponseSchema", - SSHConfigResponseSchema, - { ssh_config_options: {} }, - ], - ])( - "%s accepts the minimal body an old deployment sends", - (_, schema, body) => { - expect(schema.safeParse(body).success).toBe(true); - }, - ); -}); diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 866e3625f5..2f95f7ae8c 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -117,12 +117,10 @@ 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", }), @@ -163,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 @@ -176,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": createMockUser(), }); const loginPromise = authorizer.login( @@ -202,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 @@ -215,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": createMockUser(), }); const loginPromise = authorizer.login( @@ -510,38 +500,26 @@ describe("OAuthAuthorizer", () => { ).rejects.toThrow("Server does not support dynamic client registration"); }); - it("throws when the token response has no access token", async () => { - const { mockAdapter, oauthCallback, authorizer } = createTestContext(); - - setupAxiosMockRoutes(mockAdapter, { - "/.well-known/oauth-authorization-server": - createMockOAuthMetadata(TEST_URL), - "/oauth2/register": createMockClientRegistration(), + it("rejects a token response without an access token", async () => { + const { setupOAuthRoutes, startLogin, completeLogin } = + createTestContext(); + setupOAuthRoutes(undefined, { "/oauth2/token": { token_type: "Bearer" }, - "/api/v2/users/me": createMockUser(), }); - const loginPromise = authorizer.login( - createTestDeployment(), - new MockProgress(), - new MockCancellationToken(), - ); - - const { state } = await waitForBrowserToOpen(); - await oauthCallback.send({ state, code: "auth-code-123", error: null }); + const { loginPromise, state } = await startLogin(); + await completeLogin(state); await expect(loginPromise).rejects.toThrow(InvalidApiResponseError); }); - it("throws when the registration response has no client_id", async () => { - const { mockAdapter, authorizer } = createTestContext(); - - setupAxiosMockRoutes(mockAdapter, { - "/.well-known/oauth-authorization-server": - createMockOAuthMetadata(TEST_URL), + 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(), diff --git a/test/unit/oauth/metadataClient.test.ts b/test/unit/oauth/metadataClient.test.ts index f3508b8255..79390076d5 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -75,33 +75,25 @@ 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( - "did not return a valid Coder API response", - ); - }, - ); - }); - - it("throws when a *_supported field is not an array", async () => { + 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), - grant_types_supported: "authorization_code refresh_token", + ...overrides, }, }); diff --git a/test/unit/oauth/testUtils.ts b/test/unit/oauth/testUtils.ts index 217235a67d..8729aa4291 100644 --- a/test/unit/oauth/testUtils.ts +++ b/test/unit/oauth/testUtils.ts @@ -137,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": createMockUser(), + ...overrides, }); }; From f6cc4b04ec32804adf2d23afffe7915a3c6f19e3 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 11 Aug 2026 17:20:55 +0300 Subject: [PATCH 6/6] refactor(api): type the validation wrapper instead of casting The map-driven wrapper reached the SDK methods through an `as unknown as`, which asserted the shape rather than checking it. Store the schemas as `as const` pairs so iterating keeps each method name as a literal type, and assign the instance to a `ValidatedMethods` record that CoderApi satisfies structurally. `never` parameters accept any signature and the uniform value type permits assigning by a name held in a variable, so the whole wrap is compiler-checked with no assertions. Also tighten the new comments. --- src/api/coderApi.ts | 13 ++++--- src/api/responseValidation.ts | 47 ++++++++++++++---------- src/oauth/validation.ts | 14 +++---- test/unit/api/coderApi.test.ts | 5 ++- test/unit/api/responseValidation.test.ts | 9 ++--- 5 files changed, 48 insertions(+), 40 deletions(-) diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 24b67347d4..7b02b9b24b 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -49,7 +49,11 @@ import { import { SseConnection } from "../websocket/sseConnection"; import { getRefreshCommand, refreshCertificates } from "./certificateRefresh"; -import { parseApiResponse, VALIDATED_RESPONSES } from "./responseValidation"; +import { + parseApiResponse, + VALIDATED_RESPONSES, + type ValidatedMethods, +} from "./responseValidation"; import { createHttpAgent } from "./utils"; import type { @@ -782,11 +786,8 @@ function wrapResponseTransform( * `override` fields would depend on declaration order. */ function wrapWithValidation(api: CoderApi): void { - const methods = api as unknown as Record< - string, - (...args: unknown[]) => Promise - >; - for (const [name, schema] of Object.entries(VALIDATED_RESPONSES)) { + 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()); diff --git a/src/api/responseValidation.ts b/src/api/responseValidation.ts index 21778e323f..7991c75fb4 100644 --- a/src/api/responseValidation.ts +++ b/src/api/responseValidation.ts @@ -26,10 +26,9 @@ export class InvalidApiResponseError extends Error { * Validate a response body, returning the original value with the caller's * type. * - * Every schema passed here lists only the fields the extension reads and uses - * looseObject so unknown fields pass through. A field may be required only if - * every supported deployment sends it (Coder 0.25 and up, see featureSet); - * anything newer must be .optional() with its consumers handling the absence. + * 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. */ @@ -95,19 +94,29 @@ export const SSHConfigResponseSchema = z.looseObject({ }); /** - * The schema each validated SDK method's response must match, applied by - * CoderApi. Add an entry to validate another method; the key doubles as the - * endpoint name in the error. The OAuth endpoints are plain axios calls - * rather than SDK methods, so they pass their schema at the call site. + * 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 Readonly>>; +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/validation.ts b/src/oauth/validation.ts index 16aaef81af..b16c88f8f2 100644 --- a/src/oauth/validation.ts +++ b/src/oauth/validation.ts @@ -3,9 +3,9 @@ import { z } from "zod"; import { parseApiResponse } from "../api/responseValidation"; /** - * parseApiResponse for OAuth endpoints, which are absolute URLs from - * server metadata and may live on a different origin than the deployment. - * The schemas below follow the same rules as parseApiResponse's. + * 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, @@ -20,9 +20,8 @@ export function parseOAuthResponse( const REQUIRED_STRING = z.string().min(1); /** - * Advertised capabilities, kept as plain strings rather than the generated - * enums so a server adding a value does not fail validation. Absent means the - * caller applies the RFC 8414 default. + * 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(); @@ -30,8 +29,7 @@ export const OAuth2AuthorizationServerMetadataSchema = z.looseObject({ issuer: REQUIRED_STRING, authorization_endpoint: REQUIRED_STRING, token_endpoint: REQUIRED_STRING, - // Absence is how a server signals it does not support these, so an empty - // string stays valid here and the callers report the missing capability. + // 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, diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index 1164607fde..c79b438699 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -25,6 +25,7 @@ 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"; @@ -887,7 +888,7 @@ describe("CoderApi", () => { /** One realistic body per validated method, keyed by the method name. */ const CASES: ReadonlyArray<{ - method: keyof typeof VALIDATED_RESPONSES; + method: keyof ValidatedMethods; call: (api: CoderApi) => Promise; valid: unknown; }> = [ @@ -940,7 +941,7 @@ describe("CoderApi", () => { it("exercises every validated method", () => { expect(CASES.map((testCase) => testCase.method).sort()).toEqual( - Object.keys(VALIDATED_RESPONSES).sort(), + VALIDATED_RESPONSES.map(([method]) => method).sort(), ); }); diff --git a/test/unit/api/responseValidation.test.ts b/test/unit/api/responseValidation.test.ts index 1b9b476fee..d64e01ddcb 100644 --- a/test/unit/api/responseValidation.test.ts +++ b/test/unit/api/responseValidation.test.ts @@ -18,9 +18,8 @@ const ENDPOINT = "/api/v2/users/me"; const DEPLOYMENT_URL = "https://coder.example.com"; /** - * The smallest body each schema must keep accepting: what Coder 0.25, the - * oldest supported deployment, sends. Making one of these fail is a breaking - * change, so a newly required field belongs in the schema as .optional(). + * 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; @@ -125,8 +124,8 @@ describe("parseApiResponse", () => { ); }); - // The bodies a misdirected URL realistically returns: a proxy login page, a - // 204-style empty body, or JSON from some other service. + // 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],