From 697b30ea6cd71acc78e388c4d143cbbcc39be57a Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 4 Aug 2023 14:04:52 -0800 Subject: [PATCH 1/8] Fix equals being lost in SSH config value This was causing issues with the header flag as those contain equal signs. --- .eslintrc.json | 8 +++++++- src/sshSupport.test.ts | 2 ++ src/sshSupport.ts | 10 +++++++--- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 879aff28..c2294838 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -35,7 +35,13 @@ }], "import/no-unresolved": ["error", { "ignore": ["vscode"] - }] + }], + "@typescript-eslint/no-unused-vars": [ + "error", + { + "varsIgnorePattern": "^_" + } + ] }, "ignorePatterns": [ "out", diff --git a/src/sshSupport.test.ts b/src/sshSupport.test.ts index 8bf2ac58..e723eff9 100644 --- a/src/sshSupport.test.ts +++ b/src/sshSupport.test.ts @@ -29,6 +29,7 @@ it("computes the config for a host", () => { Host coder-vscode--* StrictHostKeyChecking no Another=true + ProxyCommand=/tmp/coder --header="X-FOO=bar" coder.dev # --- END CODER VSCODE --- `, ) @@ -36,5 +37,6 @@ Host coder-vscode--* expect(properties).toEqual({ Another: "true", StrictHostKeyChecking: "yes", + ProxyCommand: '/tmp/coder --header="X-FOO=bar" coder.dev', }) }) diff --git a/src/sshSupport.ts b/src/sshSupport.ts index 12b65dd3..8e30d0cf 100644 --- a/src/sshSupport.ts +++ b/src/sshSupport.ts @@ -52,7 +52,11 @@ export function computeSSHProperties(host: string, config: string): Record Date: Fri, 4 Aug 2023 12:09:06 -0800 Subject: [PATCH 2/8] Add header command setting This will be called before requests and added to the SSH config. --- package.json | 5 ++++ src/commands.ts | 4 ++- src/extension.ts | 17 ++++++++++-- src/headers.test.ts | 57 ++++++++++++++++++++++++++++++++++++++++ src/headers.ts | 64 +++++++++++++++++++++++++++++++++++++++++++++ src/remote.ts | 10 ++++++- src/storage.ts | 5 ++++ 7 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 src/headers.test.ts create mode 100644 src/headers.ts diff --git a/package.json b/package.json index db63b8ab..326b408e 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,11 @@ "markdownDescription": "The full path of the directory into which the Coder CLI will be downloaded. Defaults to the extension's global storage directory.", "type": "string", "default": "" + }, + "coder.headerCommand": { + "markdownDescription": "An external command that outputs additional HTTP headers added to all requests. The command must output each header as `key=value` on its own line. The following environment variables will be available to the process: `CODER_URL`.", + "type": "string", + "default": "" } } }, diff --git a/src/commands.ts b/src/commands.ts index 4372d6b6..683e4f08 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -70,8 +70,10 @@ export class Commands { severity: vscode.InputBoxValidationSeverity.Error, } } + // This could be something like the header command erroring or an + // invalid session token. return { - message: "Invalid session token! (" + message + ")", + message: "Failed to authenticate: " + message, severity: vscode.InputBoxValidationSeverity.Error, } }) diff --git a/src/extension.ts b/src/extension.ts index 720ab53e..08c6eace 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -59,6 +59,17 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { const storage = new Storage(output, ctx.globalState, ctx.secrets, ctx.globalStorageUri, ctx.logUri) await storage.init() + // Add headers from the header command. + axios.interceptors.request.use(async (config) => { + return { + ...config, + headers: { + ...(await storage.getHeaders()), + ...creds.headers, + }, + } + }) + const myWorkspacesProvider = new WorkspaceProvider(WorkspaceQuery.Mine, storage) const allWorkspacesProvider = new WorkspaceProvider(WorkspaceQuery.All, storage) @@ -74,8 +85,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { } } }) - .catch(() => { - // Not authenticated! + .catch((error) => { + // This should be a failure to make the request, like the header command + // errored. + vscodeProposed.window.showErrorMessage("Failed to check user authentication: " + error.message) }) .finally(() => { vscode.commands.executeCommand("setContext", "coder.loaded", true) diff --git a/src/headers.test.ts b/src/headers.test.ts new file mode 100644 index 00000000..ed3dde22 --- /dev/null +++ b/src/headers.test.ts @@ -0,0 +1,57 @@ +import * as os from "os" +import { it, expect } from "vitest" +import { getHeaders } from "./headers" + +const logger = { + writeToCoderOutputChannel() { + // no-op + }, +} + +it("should return no headers", async () => { + await expect(getHeaders(undefined, undefined, logger)).resolves.toStrictEqual({}) + await expect(getHeaders("localhost", undefined, logger)).resolves.toStrictEqual({}) + await expect(getHeaders(undefined, "command", logger)).resolves.toStrictEqual({}) + await expect(getHeaders("localhost", "", logger)).resolves.toStrictEqual({}) + await expect(getHeaders("", "command", logger)).resolves.toStrictEqual({}) + await expect(getHeaders("localhost", " ", logger)).resolves.toStrictEqual({}) + await expect(getHeaders(" ", "command", logger)).resolves.toStrictEqual({}) +}) + +it("should return headers", async () => { + await expect(getHeaders("localhost", "printf foo=bar'\n'baz=qux", logger)).resolves.toStrictEqual({ + foo: "bar", + baz: "qux", + }) + await expect(getHeaders("localhost", "printf foo=bar'\r\n'baz=qux", logger)).resolves.toStrictEqual({ + foo: "bar", + baz: "qux", + }) + await expect(getHeaders("localhost", "printf foo=bar'\r\n'", logger)).resolves.toStrictEqual({ foo: "bar" }) + await expect(getHeaders("localhost", "printf foo=bar", logger)).resolves.toStrictEqual({ foo: "bar" }) + await expect(getHeaders("localhost", "printf foo=bar=", logger)).resolves.toStrictEqual({ foo: "bar=" }) + await expect(getHeaders("localhost", "printf foo=bar=baz", logger)).resolves.toStrictEqual({ foo: "bar=baz" }) + await expect(getHeaders("localhost", "printf foo=", logger)).resolves.toStrictEqual({ foo: "" }) +}) + +it("should error on malformed or empty lines", async () => { + await expect(getHeaders("localhost", "printf foo=bar'\r\n\r\n'", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf '\r\n'foo=bar", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf =foo", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf foo", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf ' =foo'", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf 'foo =bar'", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf 'foo foo=bar'", logger)).rejects.toMatch(/Malformed/) + await expect(getHeaders("localhost", "printf ''", logger)).rejects.toMatch(/Malformed/) +}) + +it("should have access to environment variables", async () => { + const coderUrl = "dev.coder.com" + await expect( + getHeaders(coderUrl, os.platform() === "win32" ? "printf url=%CODER_URL" : "printf url=$CODER_URL", logger), + ).resolves.toStrictEqual({ url: coderUrl }) +}) + +it("should error on non-zero exit", async () => { + await expect(getHeaders("localhost", "exit 10", logger)).rejects.toMatch(/exited unexpectedly with code 10/) +}) diff --git a/src/headers.ts b/src/headers.ts new file mode 100644 index 00000000..f9da6168 --- /dev/null +++ b/src/headers.ts @@ -0,0 +1,64 @@ +import * as cp from "child_process" +import * as util from "util" + +export interface Logger { + writeToCoderOutputChannel(message: string): void +} + +interface ExecException { + code?: number + stderr?: string + stdout?: string +} + +function isExecException(err: unknown): err is ExecException { + return typeof (err as ExecException).code !== "undefined" +} + +// TODO: getHeaders might make more sense to directly implement on Storage +// but it is difficult to test Storage right now since we use vitest instead of +// the standard extension testing framework which would give us access to vscode +// APIs. We should revert the testing framework then consider moving this. + +// getHeaders executes the header command and parses the headers from stdout. +// Both stdout and stderr are logged on error but stderr is otherwise ignored. +// Throws an error if the process exits with non-zero or the JSON is invalid. +// Returns undefined if there is no header command set. No effort is made to +// validate the JSON other than making sure it can be parsed. +export async function getHeaders( + url: string | undefined, + command: string | undefined, + logger: Logger, +): Promise> { + const headers: Record = {} + if (typeof url === "string" && url.trim().length > 0 && typeof command === "string" && command.trim().length > 0) { + let result: { stdout: string; stderr: string } + try { + result = await util.promisify(cp.exec)(command, { + env: { + ...process.env, + CODER_URL: url, + }, + }) + } catch (error) { + if (isExecException(error)) { + logger.writeToCoderOutputChannel(`Header command exited unexpectedly with code ${error.code}`) + logger.writeToCoderOutputChannel(`stdout: ${error.stdout}`) + logger.writeToCoderOutputChannel(`stderr: ${error.stderr}`) + throw new Error(`Header command exited unexpectedly with code ${error.code}`) + } + throw new Error(`Header command exited unexpectedly: ${error}`) + } + const lines = result.stdout.replace(/\r?\n$/, "").split(/\r?\n/) + for (let i = 0; i < lines.length; ++i) { + const [key, value] = lines[i].split(/=(.*)/) + // Header names cannot be blank or contain whitespace and the Coder CLI + // requires that there be an equals sign (the value can be blank though). + if (key.length === 0 || key.indexOf(" ") !== -1 || typeof value === "undefined") { + throw new Error(`Malformed line from header command: [${lines[i]}] (out: ${result.stdout})`) + } + headers[key] = value + } + } + return headers +} diff --git a/src/remote.ts b/src/remote.ts index 53483d32..6ee82f1c 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -508,9 +508,17 @@ export class Remote { } const escape = (str: string): string => `"${str.replace(/"/g, '\\"')}"` + + // Add headers from the header command. + let headerArg = "" + const headerCommand = vscode.workspace.getConfiguration().get("coder.headerCommand") + if (typeof headerCommand === "string" && headerCommand.trim().length > 0) { + headerArg = ` --header-command "${escape(headerCommand)}"` + } + const sshValues: SSHValues = { Host: `${Remote.Prefix}*`, - ProxyCommand: `${escape(binaryPath)} vscodessh --network-info-dir ${escape( + ProxyCommand: `${escape(binaryPath)}${headerArg} vscodessh --network-info-dir ${escape( this.storage.getNetworkInfoPath(), )} --session-token-file ${escape(this.storage.getSessionTokenPath())} --url-file ${escape( this.storage.getURLPath(), diff --git a/src/storage.ts b/src/storage.ts index 993a82ba..5a59d6cf 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -11,6 +11,7 @@ import os from "os" import path from "path" import prettyBytes from "pretty-bytes" import * as vscode from "vscode" +import { getHeaders } from "./headers" export class Storage { public workspace?: Workspace @@ -391,6 +392,10 @@ export class Storage { await fs.rm(this.getSessionTokenPath(), { force: true }) } } + + public async getHeaders(url = this.getURL()): Promise | undefined> { + return getHeaders(url, vscode.workspace.getConfiguration().get("coder.headerCommand"), this) + } } // goos returns the Go format for the current platform. From f9b416afc2901489c9a9ece880e82fecac41aa68 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 16:41:36 -0800 Subject: [PATCH 3/8] Check for missing base URL I got this once, not sure how to reproduce it now though. --- src/error.ts | 4 ++-- src/extension.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/error.ts b/src/error.ts index 181ee67c..aafe9d0e 100644 --- a/src/error.ts +++ b/src/error.ts @@ -40,8 +40,8 @@ export class CertificateError extends Error { // maybeWrap returns a CertificateError if the code is a certificate error // otherwise it returns the original error. - static async maybeWrap(err: T, address: string, logger: Logger): Promise { - if (axios.isAxiosError(err)) { + static async maybeWrap(err: T, address: string | undefined, logger: Logger): Promise { + if (address && axios.isAxiosError(err)) { switch (err.code) { case X509_ERR_CODE.UNABLE_TO_VERIFY_LEAF_SIGNATURE: // "Unable to verify" can mean different things so we will attempt to diff --git a/src/extension.ts b/src/extension.ts index 08c6eace..9884a431 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -46,7 +46,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { axios.interceptors.response.use( (r) => r, async (err) => { - throw await CertificateError.maybeWrap(err, err.config.baseURL, storage) + throw await CertificateError.maybeWrap(err, err?.config?.baseURL, storage) }, ) From 8a56af9c9aa0e40526966d83f71effffaa8fa68d Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 16:42:24 -0800 Subject: [PATCH 4/8] A bit too many quotes Forgot escape adds its own wrapping quotes. --- src/remote.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/remote.ts b/src/remote.ts index 6ee82f1c..ab64197d 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -513,7 +513,7 @@ export class Remote { let headerArg = "" const headerCommand = vscode.workspace.getConfiguration().get("coder.headerCommand") if (typeof headerCommand === "string" && headerCommand.trim().length > 0) { - headerArg = ` --header-command "${escape(headerCommand)}"` + headerArg = ` --header-command ${escape(headerCommand)}` } const sshValues: SSHValues = { From c67bca29653dabd88b7f8b4905b25e11e96073a1 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 16:52:05 -0800 Subject: [PATCH 5/8] Fix merging headers into config object First, I replaced the wrong line, needed to replace the second one. Secondly, seems you cannot actually spread like this because config.headers actually has a bunch of functions on it. --- src/extension.ts | 11 ++++------- src/storage.ts | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 9884a431..1f75667d 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -61,13 +61,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { // Add headers from the header command. axios.interceptors.request.use(async (config) => { - return { - ...config, - headers: { - ...(await storage.getHeaders()), - ...creds.headers, - }, - } + Object.entries(await storage.getHeaders()).forEach(([key, value]) => { + config.headers[key] = value + }) + return config }) const myWorkspacesProvider = new WorkspaceProvider(WorkspaceQuery.Mine, storage) diff --git a/src/storage.ts b/src/storage.ts index 5a59d6cf..e568e5d7 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -393,7 +393,7 @@ export class Storage { } } - public async getHeaders(url = this.getURL()): Promise | undefined> { + public async getHeaders(url = this.getURL()): Promise> { return getHeaders(url, vscode.workspace.getConfiguration().get("coder.headerCommand"), this) } } From 37118f9c4e4bd575b0d3e594d68929556bf5b914 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 17:00:46 -0800 Subject: [PATCH 6/8] Better fix for getting base URL --- src/error.ts | 4 ++-- src/extension.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/error.ts b/src/error.ts index aafe9d0e..181ee67c 100644 --- a/src/error.ts +++ b/src/error.ts @@ -40,8 +40,8 @@ export class CertificateError extends Error { // maybeWrap returns a CertificateError if the code is a certificate error // otherwise it returns the original error. - static async maybeWrap(err: T, address: string | undefined, logger: Logger): Promise { - if (address && axios.isAxiosError(err)) { + static async maybeWrap(err: T, address: string, logger: Logger): Promise { + if (axios.isAxiosError(err)) { switch (err.code) { case X509_ERR_CODE.UNABLE_TO_VERIFY_LEAF_SIGNATURE: // "Unable to verify" can mean different things so we will attempt to diff --git a/src/extension.ts b/src/extension.ts index 1f75667d..27ce6bf9 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -46,7 +46,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { axios.interceptors.response.use( (r) => r, async (err) => { - throw await CertificateError.maybeWrap(err, err?.config?.baseURL, storage) + throw await CertificateError.maybeWrap(err, axios.getUri(err.config), storage) }, ) From 59237693e4e4423ea82f3482124684df576c9645 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 22:12:00 -0800 Subject: [PATCH 7/8] Avoid logging in when no url has been set To avoid an error like "127.0.0.1:80 ECONNREFUSED". Before we did not log the error so this did not matter so much but now we do to catch header issues. --- src/extension.ts | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 27ce6bf9..2c48aa04 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -73,23 +73,28 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { vscode.window.registerTreeDataProvider("myWorkspaces", myWorkspacesProvider) vscode.window.registerTreeDataProvider("allWorkspaces", allWorkspacesProvider) - getAuthenticatedUser() - .then(async (user) => { - if (user) { - vscode.commands.executeCommand("setContext", "coder.authenticated", true) - if (user.roles.find((role) => role.name === "owner")) { - await vscode.commands.executeCommand("setContext", "coder.isOwner", true) + const url = storage.getURL() + if (url) { + getAuthenticatedUser() + .then(async (user) => { + if (user) { + vscode.commands.executeCommand("setContext", "coder.authenticated", true) + if (user.roles.find((role) => role.name === "owner")) { + await vscode.commands.executeCommand("setContext", "coder.isOwner", true) + } } - } - }) - .catch((error) => { - // This should be a failure to make the request, like the header command - // errored. - vscodeProposed.window.showErrorMessage("Failed to check user authentication: " + error.message) - }) - .finally(() => { - vscode.commands.executeCommand("setContext", "coder.loaded", true) - }) + }) + .catch((error) => { + // This should be a failure to make the request, like the header command + // errored. + vscodeProposed.window.showErrorMessage("Failed to check user authentication: " + error.message) + }) + .finally(() => { + vscode.commands.executeCommand("setContext", "coder.loaded", true) + }) + } else { + vscode.commands.executeCommand("setContext", "coder.loaded", true) + } vscode.window.registerUriHandler({ handleUri: async (uri) => { From a7f21beb6cca31e56c562abc0d8b2020e1d5e93c Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 17 Aug 2023 22:51:59 -0800 Subject: [PATCH 8/8] Fix getting headers on login At this stage the URL may not be set. Or it could be set to the previous URL. We need to use the URL of the actual request. --- src/extension.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension.ts b/src/extension.ts index 2c48aa04..931f2995 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -61,7 +61,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { // Add headers from the header command. axios.interceptors.request.use(async (config) => { - Object.entries(await storage.getHeaders()).forEach(([key, value]) => { + Object.entries(await storage.getHeaders(config.baseURL || axios.getUri(config))).forEach(([key, value]) => { config.headers[key] = value }) return config