fix: propagate proxy settings to SSH environment #1012
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import * as vscode from "vscode"; | ||
|
|
||
| import type { Memento } from "vscode"; | ||
|
|
||
| const DISMISS = "Don't Show Again"; | ||
|
|
||
| /** globalState keys under which "Don't Show Again" dismissals are stored. */ | ||
| export const DISMISSIBLE_NOTIFICATION_KEYS = [ | ||
| "coder.proxyUseLocalServerWarningDismissed", | ||
| ] as const; | ||
|
|
||
| export type DismissibleNotificationKey = | ||
| (typeof DISMISSIBLE_NOTIFICATION_KEYS)[number]; | ||
|
|
||
| export class DismissibleNotifier { | ||
| public constructor(private readonly globalState: Memento) {} | ||
|
|
||
| /** | ||
| * Show a warning notification with a "Don't Show Again" button that persists | ||
| * dismissal under `key`. Returns the chosen action, or undefined if dismissed, | ||
| * closed, or already dismissed before. Pass `modal` to block until the user | ||
| * answers; non-modal toasts can auto-dismiss, so blocking callers must set it. | ||
| */ | ||
| public async showDismissible( | ||
| key: DismissibleNotificationKey, | ||
| message: string, | ||
| { | ||
| actions = [], | ||
| modal = false, | ||
| }: { actions?: string[]; modal?: boolean } = {}, | ||
| ): Promise<string | undefined> { | ||
| if (this.globalState.get<boolean>(key)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const choice = await vscode.window.showWarningMessage( | ||
| message, | ||
| { modal }, | ||
| ...actions, | ||
| DISMISS, | ||
| ); | ||
|
|
||
| if (choice === DISMISS) { | ||
| await this.globalState.update(key, true); | ||
| return undefined; | ||
| } | ||
| return choice; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import { getProxyForUrl } from "../api/proxy"; | ||
|
|
||
| import type { WorkspaceConfiguration } from "vscode"; | ||
|
|
||
| type Environment = Record<string, string | undefined>; | ||
| type PreviousValue = [key: string, existed: boolean, value: string | undefined]; | ||
| type SshEnvironment = Partial< | ||
| Record<"HTTP_PROXY" | "HTTPS_PROXY" | "NO_PROXY", string> | ||
| >; | ||
|
|
||
| /** | ||
| * The settings {@link getSshProxyEnvironment} reads, paired with display titles. | ||
| * Watch these to prompt for a reload when the SSH proxy environment changes. | ||
| */ | ||
| export const SSH_PROXY_SETTINGS: ReadonlyArray<{ | ||
| setting: string; | ||
| title: string; | ||
| }> = [ | ||
| { setting: "http.proxy", title: "HTTP Proxy" }, | ||
| { setting: "http.noProxy", title: "HTTP No Proxy" }, | ||
| { setting: "coder.proxyBypass", title: "Proxy Bypass" }, | ||
| ]; | ||
|
|
||
| /** | ||
| * Sets SSH-related environment variables on this extension host's process.env so | ||
| * the spawned `coder ssh` ProxyCommand inherits them. For now that is just the | ||
| * proxy configuration (HTTP_PROXY/HTTPS_PROXY/NO_PROXY): the coder CLI reads them | ||
| * like any Go HTTP client and has no proxy flag. We mutate process.env rather | ||
| * than baking values into the SSH config so no credentialed proxy URL is written | ||
| * to disk and multiple windows onto the same workspace stay independent. | ||
| * | ||
| * Best-effort: only processes spawned afterwards inherit the change. MS VS Code | ||
| * with `remote.SSH.useLocalServer=false` spawns ssh off a path that does not | ||
| * inherit, so propagation there needs `useLocalServer=true`. Returns a disposable | ||
| * that restores the previous values. | ||
|
Comment on lines
+32
to
+35
Member
There was a problem hiding this comment. How would the location of the SSH binary change whether environment variables are inherited? Or by path do you mean the code path and not the binary path? If I understand correctly, And then |
||
| */ | ||
| export function applySshEnvironment( | ||
| baseUrl: string, | ||
| cfg: Pick<WorkspaceConfiguration, "get">, | ||
| env: Environment = process.env, | ||
| ): { dispose(): void } { | ||
| return applyEnvironment(getSshProxyEnvironment(baseUrl, cfg), env); | ||
| } | ||
|
|
||
| /** | ||
| * The proxy portion of the SSH environment. Exposed so callers can check whether | ||
| * a proxy actually applies to a deployment via `.HTTP_PROXY`. | ||
| */ | ||
| export function getSshProxyEnvironment( | ||
| baseUrl: string, | ||
| cfg: Pick<WorkspaceConfiguration, "get">, | ||
| ): SshEnvironment { | ||
| const httpProxy = getSetting(cfg, "http.proxy"); | ||
|
Member
There was a problem hiding this comment. nbd but could just do |
||
| const noProxy = getSetting(cfg, "coder.proxyBypass") ?? getHttpNoProxy(cfg); | ||
| const proxy = httpProxy | ||
| ? getProxyForUrl(baseUrl, httpProxy, noProxy, undefined) | ||
|
Comment on lines
+55
to
+56
Member
There was a problem hiding this comment. Why do we call this differently here than in the plugin? We could just pass the fallback as the fourth option to get the same behavior, right? I think maybe it is so we can use Actually, there is no need to even set Or, maybe better to skip |
||
| : ""; | ||
|
|
||
| return { | ||
| ...(proxy ? { HTTP_PROXY: proxy, HTTPS_PROXY: proxy } : {}), | ||
|
Member
There was a problem hiding this comment. Is it right to set both |
||
| ...(noProxy ? { NO_PROXY: noProxy } : {}), | ||
| }; | ||
| } | ||
|
|
||
| function applyEnvironment( | ||
| values: SshEnvironment, | ||
| env: Environment, | ||
| ): { dispose(): void } { | ||
| const previous: PreviousValue[] = []; | ||
| for (const [key, value] of Object.entries(values)) { | ||
| if (value === undefined) { | ||
| continue; | ||
| } | ||
| for (const envKey of getEnvKeys(env, key)) { | ||
| previous.push([envKey, Object.hasOwn(env, envKey), env[envKey]]); | ||
| env[envKey] = value; | ||
| } | ||
| } | ||
|
|
||
| let disposed = false; | ||
| return { | ||
| dispose: () => { | ||
| if (disposed) { | ||
| return; | ||
| } | ||
| disposed = true; | ||
| for (let i = previous.length - 1; i >= 0; i--) { | ||
|
Member
There was a problem hiding this comment. Is this not using a |
||
| const [key, existed, value] = previous[i]; | ||
| if (existed) { | ||
| env[key] = value; | ||
| } else { | ||
| delete env[key]; | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| function getEnvKeys(env: Environment, key: string): string[] { | ||
| const keys = Object.keys(env).filter( | ||
| (envKey) => envKey.toLowerCase() === key.toLowerCase(), | ||
|
Member
There was a problem hiding this comment. Environment variables are case-sensitive (at least, in Linux, Windows might be different) so I think we should not compare with lowercase or we technically cause collisions. |
||
| ); | ||
| return keys.length > 0 ? keys : [key]; | ||
| } | ||
|
|
||
| function getSetting( | ||
| cfg: Pick<WorkspaceConfiguration, "get">, | ||
| setting: string, | ||
| ): string | undefined { | ||
| const value = cfg.get<string | null>(setting); | ||
| return typeof value === "string" ? value.trim() || undefined : undefined; | ||
| } | ||
|
|
||
| function getHttpNoProxy( | ||
| cfg: Pick<WorkspaceConfiguration, "get">, | ||
| ): string | undefined { | ||
| return ( | ||
| cfg | ||
| .get<string[]>("http.noProxy", []) | ||
| .map((value) => value.trim()) | ||
| .filter(Boolean) | ||
| .join(",") || undefined | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
enum? This looks basically like an enum.Or, if the idea is that we want to pass string literals then we could just do
type DISMISSIBLE_NOTIFICATION_KEYS = "key" | "key2"since it does not look like we use the const for anything right now.But if we will need to iterate over it or something like that in the future, then ignore me.