Skip to content

fix: recover mTLS auth after settings races #976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EhabY wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: recover mTLS auth after settings races #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions src/api/authInterceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,21 @@ export class AuthInterceptor implements vscode.Disposable {
throw error;
}

if (error.response?.status !== 401) {
throw error;
}

if (this.shouldRetryAfterAuthConfigChange(error)) {
return this.retryAfterAuthConfigChange(error);
}
Comment on lines +65 to +67
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These feel like they would be natural to be located within recoverFromUnauthorized, so we could see all the possible 401 recovery paths in one place.

That would let us avoid duplicating the _retryAttempted check as well.


if (error.config) {
const config = error.config as { _retryAttempted?: boolean };
if (config._retryAttempted) {
throw error;
}
}

if (error.response?.status !== 401) {
throw error;
}

const baseUrl = this.client.getHost();
if (!baseUrl) {
throw error;
Expand All @@ -78,6 +82,39 @@ export class AuthInterceptor implements vscode.Disposable {
return this.recoverFromUnauthorized(error, hostname);
}

private shouldRetryAfterAuthConfigChange(error: AxiosError): boolean {
if (!error.config) {
return false;
}

const config = error.config as RequestConfigWithMeta & {
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but there must be a way we can convince Typescript these properties exist. I know in code-server we do something similar with Express to pass around things on the request

declare global {
  // eslint-disable-next-line @typescript-eslint/no-namespace
  namespace Express {
    export interface Request {
      args: DefaultedArgs
      heart: Heart
      settings: SettingsProvider<CoderSettings>
      updater: UpdateProvider
      cookieSessionName: string
    }
  }
}

_retryAttempted?: boolean;
_authConfigRetryAttempted?: boolean;
authConfigVersion?: number;
};
// Skip if any retry already happened: caps total attempts at 2.
if (config._retryAttempted || config._authConfigRetryAttempted) {
return false;
}

return this.client.hasAuthConfigChangedSince(config.authConfigVersion);
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense but I wonder if it would be possible to compare the relevant settings instead? If any have changed, then we re-run the request.

Or I suppose the tracker could hash the values and use that as the version.

Hmm but although that might technically cover more cases, this is probably close enough, worst case is we run it once more with values that just failed (like if a user edits and then reverts the edit).

}

private retryAfterAuthConfigChange(error: AxiosError): Promise<unknown> {
if (!error.config) {
throw error;
}

const config = error.config as RequestConfigWithMeta & {
_authConfigRetryAttempted?: boolean;
};
config._authConfigRetryAttempted = true;
this.logger.debug(
"Authentication settings changed during request, retrying once",
);
return this.client.getAxiosInstance().request(config);
}

private recoverFromUnauthorized(
error: AxiosError,
hostname: string,
Expand Down
76 changes: 57 additions & 19 deletions src/api/coderApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
import { Api } from "coder/site/src/api/api";
import * as vscode from "vscode";

import { watchConfigurationChanges } from "../configWatcher";
import {
CONFIG_CHANGE_DEBOUNCE_MS,
watchConfigurationChanges,
} from "../configWatcher";
import { ClientCertificateError } from "../error/clientCertificateError";
import { toError } from "../error/errorUtils";
import { ServerCertificateError } from "../error/serverCertificateError";
Expand All @@ -26,6 +29,7 @@ import {
type RequestConfigWithMeta,
} from "../logging/types";
import { sizeOf } from "../logging/utils";
import { AuthConfigTracker } from "../settings/authConfig";
import { getHeaderCommand } from "../settings/headers";
import {
NOOP_TELEMETRY_REPORTER,
Expand Down Expand Up @@ -98,6 +102,7 @@ export class CoderApi extends Api implements vscode.Disposable {
private readonly output: Logger,
private readonly telemetry: TelemetryReporter,
private readonly httpRequestsTelemetry: HttpRequestsTelemetry,
private readonly authConfigTracker: AuthConfigTracker,
) {
super();
this.configWatcher = this.watchConfigChanges();
Expand All @@ -117,17 +122,27 @@ export class CoderApi extends Api implements vscode.Disposable {
telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER,
): CoderApi {
const httpRequestsTelemetry = new HttpRequestsTelemetry(telemetry);
const client = new CoderApi(output, telemetry, httpRequestsTelemetry);
const authConfigTracker = new AuthConfigTracker();
const client = new CoderApi(
output,
telemetry,
httpRequestsTelemetry,
authConfigTracker,
);
client.setCredentials(baseUrl, token);

setupInterceptors(client, output, httpRequestsTelemetry);
setupInterceptors(client, output, httpRequestsTelemetry, authConfigTracker);
return client;
}

getHost(): string | undefined {
return this.getAxiosInstance().defaults.baseURL;
}

hasAuthConfigChangedSince(version: number | undefined): boolean {
return this.authConfigTracker.hasChangedSince(version);
}

/**
* Set both host and token together. Useful for login/logout/switch to
* avoid triggering multiple reconnection events.
Expand Down Expand Up @@ -172,6 +187,7 @@ export class CoderApi extends Api implements vscode.Disposable {
*/
dispose(): void {
this.configWatcher.dispose();
this.authConfigTracker.dispose();
this.httpRequestsTelemetry.dispose();
for (const socket of this.reconnectingSockets) {
socket.close();
Expand All @@ -189,20 +205,24 @@ export class CoderApi extends Api implements vscode.Disposable {
setting,
getValue: () => vscode.workspace.getConfiguration().get(setting),
}));
return watchConfigurationChanges(settings, () => {
const socketsToReconnect = [...this.reconnectingSockets].filter(
(socket) => socket.state === ConnectionState.DISCONNECTED,
);
if (socketsToReconnect.length) {
this.output.debug(
`Configuration changed, ${socketsToReconnect.length}/${this.reconnectingSockets.size} socket(s) in DISCONNECTED state`,
return watchConfigurationChanges(
settings,
() => {
const socketsToReconnect = [...this.reconnectingSockets].filter(
(socket) => socket.state === ConnectionState.DISCONNECTED,
);
for (const socket of socketsToReconnect) {
this.output.debug(`Reconnecting WebSocket: ${socket.url}`);
socket.reconnect();
if (socketsToReconnect.length) {
this.output.debug(
`Configuration changed, ${socketsToReconnect.length}/${this.reconnectingSockets.size} socket(s) in DISCONNECTED state`,
);
for (const socket of socketsToReconnect) {
this.output.debug(`Reconnecting WebSocket: ${socket.url}`);
socket.reconnect();
}
}
}
});
},
{ debounceMs: CONFIG_CHANGE_DEBOUNCE_MS },
);
}

watchInboxNotifications = async (
Expand Down Expand Up @@ -491,10 +511,16 @@ export class CoderApi extends Api implements vscode.Disposable {
}
}

type RequestConfigWithAuthMeta = RequestConfigWithMeta & {
authConfigVersion?: number;
headerCommandKeys?: string[];
};

function setupInterceptors(
client: CoderApi,
output: Logger,
httpRequestsTelemetry: HttpRequestsTelemetry,
authConfigTracker: AuthConfigTracker,
): void {
addRequestInterceptors(
client.getAxiosInstance(),
Expand All @@ -503,25 +529,37 @@ function setupInterceptors(
);

client.getAxiosInstance().interceptors.request.use(async (config) => {
const configWithAuthMeta = config as RequestConfigWithAuthMeta;

for (const key of configWithAuthMeta.headerCommandKeys ?? []) {
config.headers.delete(key);
}
Comment on lines +534 to +536
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand what is happening, since this could be a retry, we are deleting headers from the previous run of the header command? Since the next run may not output the same headers.

I think this property could use a bit of documentation.


const baseUrl = client.getAxiosInstance().defaults.baseURL;
const headers = await getHeaders(
baseUrl,
getHeaderCommand(vscode.workspace.getConfiguration()),
output,
);
// Add headers from the header command.
for (const [key, value] of Object.entries(headers)) {
config.headers[key] = value;
}
// Skip the session-token header: retryRequest writes it on the same
// config and the retry cleanup would clobber the refreshed token.
configWithAuthMeta.headerCommandKeys = Object.keys(headers).filter(
(k) => k.toLowerCase() !== coderSessionTokenHeader.toLowerCase(),
);

// Configure proxy and TLS.
// Note that by default VS Code overrides the agent. To prevent this, set
// `http.proxySupport` to `on` or `off`.
// VS Code overrides the agent by default; set `http.proxySupport` to
// `on` or `off` to keep ours.
const agent = await createHttpAgent(vscode.workspace.getConfiguration());
config.httpsAgent = agent;
config.httpAgent = agent;
config.proxy = false;

// Stamp last so the version matches the config actually used.
configWithAuthMeta.authConfigVersion = authConfigTracker.version;

return config;
});

Expand Down
53 changes: 42 additions & 11 deletions src/configWatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,74 @@
import { isDeepStrictEqual } from "node:util";
import * as vscode from "vscode";

/**
* Debounce window for config-change reactions that fan out (recovery,
* reconnect, reload prompts). Keeps rapid edits in settings.json from
* flushing each side-effect per keystroke.
*/
export const CONFIG_CHANGE_DEBOUNCE_MS = 250;
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda wild to me that VS Code is not already debouncing setting updates 💀

Copy link
Copy Markdown
Collaborator Author

@EhabY EhabY May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do but it's too short, also Netflix manually modifies the settings one by one so this coalesces them into one batch


export interface WatchedSetting {
setting: string;
getValue: () => unknown;
}

export interface WatchConfigurationChangesOptions {
debounceMs?: number;
}

/**
* Watch for configuration changes and invoke a callback when values change.
* The callback receives a map of changed settings to their new values, so
* consumers can act on the new value without re-reading the configuration.
* Only fires when actual values change, not just when settings are touched.
* Fires only when actual values change. With `debounceMs`, the first event
* opens a fixed collection window; subsequent events during the window are
* coalesced. This bounds latency even when events arrive faster than the
* window length (a reset-style debounce would starve).
Comment on lines +23 to +25
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, a debounce means every time the function is called, it resets the timer.

So this is not actually a debounce? A throttle then? It fires at most once per debounceMs (the window)?

What does it mean to bind latency? Is this just saying we use a throttle to avoid the case where a user keeps editing settings within 250ms over and over, keeping the function from running? Is this really a problem? If the user keeps editing, we really probably do want to avoid using the now-stale values.

*/
export function watchConfigurationChanges(
settings: WatchedSetting[],
onChange: (changes: ReadonlyMap<string, unknown>) => void,
options: WatchConfigurationChangesOptions = {},
): vscode.Disposable {
const appliedValues = new Map(settings.map((s) => [s.setting, s.getValue()]));

return vscode.workspace.onDidChangeConfiguration((e) => {
const detectAndFire = () => {
const changes = new Map<string, unknown>();

for (const { setting, getValue } of settings) {
if (!e.affectsConfiguration(setting)) {
continue;
}

const newValue = getValue();

if (!configValuesEqual(newValue, appliedValues.get(setting))) {
changes.set(setting, newValue);
appliedValues.set(setting, newValue);
}
}

if (changes.size > 0) {
onChange(changes);
}
};

let windowTimer: ReturnType<typeof setTimeout> | undefined;
const listener = vscode.workspace.onDidChangeConfiguration((e) => {
if (!settings.some((s) => e.affectsConfiguration(s.setting))) {
return;
}
if (!options.debounceMs) {
detectAndFire();
return;
}
if (windowTimer) {
return; // already collecting in the open window
}
windowTimer = setTimeout(() => {
Copy link
Copy Markdown
Member

@code-asher code-asher May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this looks like a throttle

I do feel like a debounce would make more sense though

Copy link
Copy Markdown
Collaborator Author

@EhabY EhabY May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it at denounce first but I worried about denouncing for a long time since this can be triggered by editing the file OR the setting through the UI.

I can bring back the denounce but should we limit this to some max time or am I being overly cautious? Do keep in mind that this is used for the CoderApi/Remote settings watch/deployment watch... Etc

Perhaps denounce but reduce it from 250ms?

windowTimer = undefined;
detectAndFire();
}, options.debounceMs);
});

return {
dispose: () => {
clearTimeout(windowTimer);
listener.dispose();
},
};
}

function configValuesEqual(a: unknown, b: unknown): boolean {
Expand Down
Loading
Loading