Skip to content

Commit f690169

Browse files
EhabYEhabY
committed
Handle review comments on new PR
1 parent e5c5c9a commit f690169

File tree

11 files changed

+118
-61
lines changed

11 files changed

+118
-61
lines changed

‎src/core/secretsManager.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import type { Deployment } from "../deployment/types";
88

99
// Each deployment has its own key to ensure atomic operations (multiple windows
1010
// writing to a shared key could drop data) and to receive proper VS Code events.
11-
const SESSION_KEY_PREFIX = "coder.session." as const;
12-
const OAUTH_CLIENT_PREFIX = "coder.oauth.client." as const;
11+
const SESSION_KEY_PREFIX = "coder.session.";
12+
const OAUTH_CLIENT_PREFIX = "coder.oauth.client.";
1313

1414
type SecretKeyPrefix = typeof SESSION_KEY_PREFIX | typeof OAUTH_CLIENT_PREFIX;
1515

@@ -309,14 +309,22 @@ export class SecretsManager {
309309
return;
310310
}
311311

312+
let parsed: OAuthCallbackData;
312313
try {
313314
const data = await this.secrets.get(OAUTH_CALLBACK_KEY);
314-
if (data) {
315-
const parsed = JSON.parse(data) as OAuthCallbackData;
316-
listener(parsed);
315+
if (!data) {
316+
return;
317317
}
318-
} catch {
319-
// Ignore parse errors
318+
parsed = JSON.parse(data) as OAuthCallbackData;
319+
} catch (err) {
320+
this.logger.error("Failed to parse OAuth callback data", err);
321+
return;
322+
}
323+
324+
try {
325+
listener(parsed);
326+
} catch (err) {
327+
this.logger.error("Error in onDidChangeOAuthCallback listener", err);
320328
}
321329
});
322330
}

‎src/deployment/deploymentManager.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export class DeploymentManager implements vscode.Disposable {
140140
this.refreshWorkspaces();
141141

142142
await this.oauthSessionManager.setDeployment(deployment);
143-
await this.oauthInterceptor.setDeployment(deployment.safeHostname);
143+
await this.oauthInterceptor.setDeployment(deployment);
144144
await this.persistDeployment(deployment);
145145
}
146146

‎src/oauth/authorizer.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ import { type SecretsManager } from "../core/secretsManager";
77
import { type Deployment } from "../deployment/types";
88
import { type Logger } from "../logging/logger";
99

10+
import {
11+
AUTH_GRANT_TYPE,
12+
PKCE_CHALLENGE_METHOD,
13+
RESPONSE_TYPE,
14+
TOKEN_ENDPOINT_AUTH_METHOD,
15+
} from "./constants";
1016
import { OAuthMetadataClient } from "./metadataClient";
1117
import {
1218
CALLBACK_PATH,
@@ -23,10 +29,6 @@ import type {
2329
TokenResponse,
2430
} from "./types";
2531

26-
const AUTH_GRANT_TYPE = "authorization_code";
27-
const RESPONSE_TYPE = "code";
28-
const PKCE_CHALLENGE_METHOD = "S256";
29-
3032
/**
3133
* Minimal scopes required by the VS Code extension.
3234
*/
@@ -152,11 +154,10 @@ export class OAuthAuthorizer implements vscode.Disposable {
152154

153155
const registrationRequest: ClientRegistrationRequest = {
154156
redirect_uris: [redirectUri],
155-
application_type: "web",
156-
grant_types: ["authorization_code"],
157-
response_types: ["code"],
157+
grant_types: [AUTH_GRANT_TYPE],
158+
response_types: [RESPONSE_TYPE],
158159
client_name: `Coder for ${vscode.env.appName}`,
159-
token_endpoint_auth_method: "client_secret_post",
160+
token_endpoint_auth_method: TOKEN_ENDPOINT_AUTH_METHOD,
160161
};
161162

162163
const response = await axiosInstance.post<ClientRegistrationResponse>(
@@ -241,7 +242,10 @@ export class OAuthAuthorizer implements vscode.Disposable {
241242

242243
const callbackPromise = new Promise<{ code: string; verifier: string }>(
243244
(resolve, reject) => {
244-
// Track reject for disposal
245+
// Reject any existing pending auth before starting a new one
246+
if (this.pendingAuthReject) {
247+
this.pendingAuthReject(new Error("New OAuth flow started"));
248+
}
245249
this.pendingAuthReject = reject;
246250

247251
const timeoutMins = 5;
@@ -258,6 +262,10 @@ export class OAuthAuthorizer implements vscode.Disposable {
258262
const listener = this.secretsManager.onDidChangeOAuthCallback(
259263
({ state: callbackState, code, error }) => {
260264
if (callbackState !== state) {
265+
this.logger.warn(
266+
"Ignoring OAuth callback with mismatched state",
267+
{ expected: state, received: callbackState },
268+
);
261269
return;
262270
}
263271

‎src/oauth/axiosInterceptor.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type * as vscode from "vscode";
44

55
import type { CoderApi } from "../api/coderApi";
66
import type { SecretsManager } from "../core/secretsManager";
7+
import type { Deployment } from "../deployment/types";
78
import type { Logger } from "../logging/logger";
89
import type { RequestConfigWithMeta } from "../logging/types";
910

@@ -53,12 +54,12 @@ export class OAuthInterceptor implements vscode.Disposable {
5354
return instance;
5455
}
5556

56-
public async setDeployment(safeHostname: string): Promise<void> {
57-
if (this.safeHostname === safeHostname) {
57+
public async setDeployment(deployment: Deployment): Promise<void> {
58+
if (this.safeHostname === deployment.safeHostname) {
5859
return;
5960
}
6061

61-
this.safeHostname = safeHostname;
62+
this.safeHostname = deployment.safeHostname;
6263
this.detach();
6364
this.setupTokenListener();
6465
await this.syncWithTokenState();
@@ -94,9 +95,9 @@ export class OAuthInterceptor implements vscode.Disposable {
9495
*/
9596
private async syncWithTokenState(): Promise<void> {
9697
const isOAuth = await this.oauthSessionManager.isLoggedInWithOAuth();
97-
if (isOAuth && this.interceptorId === null) {
98+
if (isOAuth) {
9899
this.attach();
99-
} else if (!isOAuth && this.interceptorId !== null) {
100+
} else {
100101
this.detach();
101102
}
102103
}

‎src/oauth/constants.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// OAuth 2.1 Grant Types
2+
export const AUTH_GRANT_TYPE = "authorization_code";
3+
export const REFRESH_GRANT_TYPE = "refresh_token";
4+
5+
// OAuth 2.1 Response Types
6+
export const RESPONSE_TYPE = "code";
7+
8+
// Token Endpoint Authentication Methods
9+
export const TOKEN_ENDPOINT_AUTH_METHOD = "client_secret_post";
10+
11+
// PKCE Code Challenge Methods (OAuth 2.1 requires S256)
12+
export const PKCE_CHALLENGE_METHOD = "S256";

‎src/oauth/metadataClient.ts‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
import {
2+
AUTH_GRANT_TYPE,
3+
PKCE_CHALLENGE_METHOD,
4+
REFRESH_GRANT_TYPE,
5+
RESPONSE_TYPE,
6+
TOKEN_ENDPOINT_AUTH_METHOD,
7+
} from "./constants";
8+
19
import type { AxiosInstance } from "axios";
210

311
import type { Logger } from "../logging/logger";
@@ -11,20 +19,17 @@ import type {
1119

1220
const OAUTH_DISCOVERY_ENDPOINT = "/.well-known/oauth-authorization-server";
1321

14-
const AUTH_GRANT_TYPE = "authorization_code" as const;
15-
const REFRESH_GRANT_TYPE = "refresh_token" as const;
16-
const RESPONSE_TYPE = "code" as const;
17-
const OAUTH_METHOD = "client_secret_post" as const;
18-
const PKCE_CHALLENGE_METHOD = "S256" as const;
19-
20-
const REQUIRED_GRANT_TYPES = [AUTH_GRANT_TYPE, REFRESH_GRANT_TYPE] as const;
22+
const REQUIRED_GRANT_TYPES: readonly string[] = [
23+
AUTH_GRANT_TYPE,
24+
REFRESH_GRANT_TYPE,
25+
];
2126

2227
// RFC 8414 defaults when fields are omitted
23-
const DEFAULT_GRANT_TYPES = [AUTH_GRANT_TYPE] as GrantType[];
24-
const DEFAULT_RESPONSE_TYPES = [RESPONSE_TYPE] as ResponseType[];
25-
const DEFAULT_AUTH_METHODS = [
28+
const DEFAULT_GRANT_TYPES: readonly GrantType[] = [AUTH_GRANT_TYPE];
29+
const DEFAULT_RESPONSE_TYPES: readonly ResponseType[] = [RESPONSE_TYPE];
30+
const DEFAULT_AUTH_METHODS: readonly TokenEndpointAuthMethod[] = [
2631
"client_secret_basic",
27-
] as TokenEndpointAuthMethod[];
32+
];
2833

2934
/**
3035
* Client for discovering and validating OAuth server metadata.
@@ -95,7 +100,7 @@ export class OAuthMetadataClient {
95100
const supported = metadata.grant_types_supported ?? DEFAULT_GRANT_TYPES;
96101
if (!includesAllTypes(supported, REQUIRED_GRANT_TYPES)) {
97102
throw new Error(
98-
`Server does not support required grant types: ${REQUIRED_GRANT_TYPES.join(", ")}. Supported: ${supported.join(", ")}`,
103+
`Server does not support required grant types: ${REQUIRED_GRANT_TYPES.join(", ")}. Supported: ${formatSupported(supported)}`,
99104
);
100105
}
101106
}
@@ -105,17 +110,17 @@ export class OAuthMetadataClient {
105110
metadata.response_types_supported ?? DEFAULT_RESPONSE_TYPES;
106111
if (!includesAllTypes(supported, [RESPONSE_TYPE])) {
107112
throw new Error(
108-
`Server does not support required response type: ${RESPONSE_TYPE}. Supported: ${supported.join(", ")}`,
113+
`Server does not support required response type: ${RESPONSE_TYPE}. Supported: ${formatSupported(supported)}`,
109114
);
110115
}
111116
}
112117

113118
private validateAuthMethods(metadata: OAuthServerMetadata): void {
114119
const supported =
115120
metadata.token_endpoint_auth_methods_supported ?? DEFAULT_AUTH_METHODS;
116-
if (!includesAllTypes(supported, [OAUTH_METHOD])) {
121+
if (!includesAllTypes(supported, [TOKEN_ENDPOINT_AUTH_METHOD])) {
117122
throw new Error(
118-
`Server does not support required auth method: ${OAUTH_METHOD}. Supported: ${supported.join(", ")}`,
123+
`Server does not support required auth method: ${TOKEN_ENDPOINT_AUTH_METHOD}. Supported: ${formatSupported(supported)}`,
119124
);
120125
}
121126
}
@@ -125,7 +130,7 @@ export class OAuthMetadataClient {
125130
const supported = metadata.code_challenge_methods_supported ?? [];
126131
if (!includesAllTypes(supported, [PKCE_CHALLENGE_METHOD])) {
127132
throw new Error(
128-
`Server does not support required PKCE method: ${PKCE_CHALLENGE_METHOD}. Supported: ${supported.length > 0 ? supported.join(", ") : "none"}`,
133+
`Server does not support required PKCE method: ${PKCE_CHALLENGE_METHOD}. Supported: ${formatSupported(supported)}`,
129134
);
130135
}
131136
}
@@ -140,3 +145,7 @@ function includesAllTypes(
140145
): boolean {
141146
return requiredTypes.every((type) => arr.includes(type));
142147
}
148+
149+
function formatSupported(supported: readonly string[]): string {
150+
return supported.length > 0 ? supported.join(", ") : "none";
151+
}

‎src/oauth/sessionManager.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { type Deployment } from "../deployment/types";
1111
import { type Logger } from "../logging/logger";
1212
import { type LoginCoordinator } from "../login/loginCoordinator";
1313

14+
import { REFRESH_GRANT_TYPE } from "./constants";
1415
import {
1516
type OAuthError,
1617
parseOAuthError,
@@ -29,8 +30,6 @@ import type {
2930
TokenRevocationRequest,
3031
} from "./types";
3132

32-
const REFRESH_GRANT_TYPE = "refresh_token";
33-
3433
/**
3534
* Token refresh threshold: refresh when token expires in less than this time.
3635
*/
@@ -73,6 +72,7 @@ type StoredTokens = OAuthTokenData & {
7372
*/
7473
export class OAuthSessionManager implements vscode.Disposable {
7574
private refreshPromise: Promise<TokenResponse> | null = null;
75+
private refreshAbortController: AbortController | null = null;
7676
private lastRefreshAttempt = 0;
7777
private refreshTimer: NodeJS.Timeout | undefined;
7878
private tokenChangeListener: vscode.Disposable | undefined;
@@ -171,8 +171,11 @@ export class OAuthSessionManager implements vscode.Disposable {
171171

172172
/**
173173
* Clear all refresh-related state: in-flight promise, throttle, timer, and listener.
174+
* Aborts any in-flight refresh request to prevent stale token updates.
174175
*/
175176
private clearRefreshState(): void {
177+
this.refreshAbortController?.abort();
178+
this.refreshAbortController = null;
176179
this.refreshPromise = null;
177180
this.lastRefreshAttempt = 0;
178181
if (this.refreshTimer) {
@@ -386,6 +389,9 @@ export class OAuthSessionManager implements vscode.Disposable {
386389
private async executeTokenRefresh(
387390
deployment: Deployment,
388391
): Promise<TokenResponse> {
392+
const abortController = new AbortController();
393+
this.refreshAbortController = abortController;
394+
389395
try {
390396
const storedTokens = await this.getStoredTokens();
391397
if (!storedTokens?.refresh_token) {
@@ -418,9 +424,15 @@ export class OAuthSessionManager implements vscode.Disposable {
418424
headers: {
419425
"Content-Type": "application/x-www-form-urlencoded",
420426
},
427+
signal: abortController.signal,
421428
},
422429
);
423430

431+
// Check if aborted between response and save
432+
if (abortController.signal.aborted) {
433+
throw new Error("Token refresh aborted");
434+
}
435+
424436
this.logger.debug("Token refresh successful");
425437

426438
const oauthData = buildOAuthTokenData(response.data);
@@ -435,6 +447,9 @@ export class OAuthSessionManager implements vscode.Disposable {
435447
this.handleOAuthError(error);
436448
throw error;
437449
} finally {
450+
if (this.refreshAbortController === abortController) {
451+
this.refreshAbortController = null;
452+
}
438453
this.refreshPromise = null;
439454
}
440455
}

‎src/oauth/types.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ export type TokenEndpointAuthMethod =
1313
| "client_secret_basic"
1414
| "none";
1515

16-
// Application Types
17-
export type ApplicationType = "native" | "web";
18-
1916
// PKCE Code Challenge Methods (OAuth 2.1 requires S256)
2017
export type CodeChallengeMethod = "S256";
2118

@@ -26,7 +23,6 @@ export type TokenType = "Bearer" | "DPoP";
2623
export interface ClientRegistrationRequest {
2724
redirect_uris: string[];
2825
token_endpoint_auth_method: TokenEndpointAuthMethod;
29-
application_type: ApplicationType;
3026
grant_types: GrantType[];
3127
response_types: ResponseType[];
3228
client_name?: string;
@@ -49,7 +45,6 @@ export interface ClientRegistrationResponse {
4945
client_secret_expires_at?: number;
5046
redirect_uris: string[];
5147
token_endpoint_auth_method: TokenEndpointAuthMethod;
52-
application_type?: ApplicationType;
5348
grant_types: GrantType[];
5449
response_types: ResponseType[];
5550
client_name?: string;

‎src/oauth/utils.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ import type { TokenResponse } from "./types";
1010
export const CALLBACK_PATH = "/oauth/callback";
1111

1212
/**
13-
* Default expiry time for OAuth access tokens when the server doesn't provide one.
13+
* Fallback expiry time for access tokens when the server omits expires_in.
14+
* RFC 6749 recommends but doesn't require expires_in and specifies no default.
1415
*/
1516
const ACCESS_TOKEN_DEFAULT_EXPIRY_MS = 60 * 60 * 1000;
1617

‎test/mocks/testHelpers.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,8 @@ export function setupAxiosMockRoutes(
695695
if (value instanceof Error) {
696696
throw value;
697697
}
698-
const data = typeof value === "function" ? await value() : value;
698+
const data =
699+
typeof value === "function" ? await value(config) : value;
699700
return {
700701
data,
701702
status: 200,

0 commit comments

Comments
 (0)