diff --git a/src/commands.ts b/src/commands.ts index 0a0903302..a91ab70b1 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -1348,7 +1348,6 @@ export class Commands { if (!vscode.workspace.workspaceFolders?.length) { newWindow = false; } - if (!folderPath && useDefaultDirectory) { folderPath = agent.expanded_directory; } @@ -1366,7 +1365,6 @@ export class Commands { // reference a workspace without an agent name, which will be missed. (opened) => opened.folderUri?.authority === remoteAuthority, ); - // openRecent will always use the most recent. Otherwise, if there are // multiple we ask the user which to use. if (opened.length === 1 || (opened.length > 1 && openRecent)) { @@ -1399,6 +1397,13 @@ export class Commands { this.logger.error(`IPC ping failed for ${remoteAuthority}`, err); }); + this.logger.info("Opening remote workspace", { + remoteAuthority, + workspace: `${workspace.owner_name}/${workspace.name}`, + agent: agent.name, + handoff: folderPath ? "folder" : "empty_window", + }); + if (folderPath) { await vscode.commands.executeCommand( "vscode.openFolder", diff --git a/src/remote/remote.ts b/src/remote/remote.ts index d64c76ede..d5420d57b 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -135,13 +135,22 @@ export class Remote { startupMode: StartupMode, remoteSshExtensionId: string, ): Promise { - const parts = parseRemoteAuthority(remoteAuthority); + let parts: AuthorityParts | null; + try { + parts = parseRemoteAuthority(remoteAuthority); + } catch (error) { + this.logger.warn("Failed to parse remote authority", { + remoteAuthority, + error: toError(error).message, + }); + throw error; + } if (!parts) { - // Not a Coder host. return; } - this.logger.debug("Setting up remote connection", { + this.logger.info("Setting up remote connection", { + remoteAuthority, hostname: parts.safeHostname, workspace: `${parts.username}/${parts.workspace}`, agent: parts.agent || "(default)", diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index d6772cda8..1e5765264 100644 --- a/src/uri/uriHandler.ts +++ b/src/uri/uriHandler.ts @@ -22,6 +22,7 @@ interface UriRouteContext extends UriHandlerDeps { } type UriRouteHandler = (ctx: UriRouteContext) => Promise; +type CoderUriRoute = "open" | "openDevContainer"; const routes: Readonly> = { "/open": handleOpen, @@ -48,7 +49,10 @@ export function registerUriHandler(deps: UriHandlerDeps): vscode.Disposable { }); } catch (error) { const message = errToStr(error, "No error message was provided"); - output.warn(`Failed to handle URI ${uri.toString()}: ${message}`); + output.warn("Failed to handle URI", { + ...summarizeUri(uri), + error: message, + }); vscodeProposed.window.showErrorMessage("Failed to handle URI", { detail: message, modal: true, @@ -67,6 +71,17 @@ function getRequiredParam(params: URLSearchParams, name: string): string { return value; } +function summarizeUri(uri: vscode.Uri): Record { + const params = new URLSearchParams(uri.query); + return { + path: uri.path, + hasOwner: params.has("owner"), + hasWorkspace: params.has("workspace"), + hasAgent: params.has("agent"), + hasToken: params.has("token"), + }; +} + async function handleOpen(ctx: UriRouteContext): Promise { const { params, serviceContainer, deploymentManager, commands } = ctx; @@ -78,7 +93,7 @@ async function handleOpen(ctx: UriRouteContext): Promise { params.has("openRecent") && (!params.get("openRecent") || params.get("openRecent") === "true"); - await setupDeployment(params, serviceContainer, deploymentManager); + await setupDeployment("open", params, serviceContainer, deploymentManager); await commands.open({ workspaceOwner: owner, @@ -108,7 +123,12 @@ async function handleOpenDevContainer(ctx: UriRouteContext): Promise { ); } - await setupDeployment(params, serviceContainer, deploymentManager); + await setupDeployment( + "openDevContainer", + params, + serviceContainer, + deploymentManager, + ); await commands.openDevContainer( owner, @@ -126,6 +146,7 @@ async function handleOpenDevContainer(ctx: UriRouteContext): Promise { * and token storage. Throws if user cancels URL input or login fails. */ async function setupDeployment( + route: CoderUriRoute, params: URLSearchParams, serviceContainer: ServiceContainer, deploymentManager: Pick, @@ -154,6 +175,14 @@ async function setupDeployment( } const safeHostname = toSafeHost(url); + const owner = params.get("owner") ?? ""; + const workspace = params.get("workspace") ?? ""; + serviceContainer.getLogger().info("Handling Coder URI", { + route, + safeHostname, + workspace: owner && workspace ? `${owner}/${workspace}` : "", + agent: params.get("agent") ?? "(unspecified)", + }); const token: string | undefined = params.get("token") ?? undefined; const result = await authTelemetry.traceLogin("uri", () => diff --git a/src/util.ts b/src/util.ts index 375d6a29c..5ea37b8c1 100644 --- a/src/util.ts +++ b/src/util.ts @@ -58,9 +58,9 @@ export function findPort(text: string): number | null { * SSH host names created by this extension match the format: * coder-vscode.----(.) * - * If this is not a Coder host, return null. + * If this is not a Coder authority, return null. * - * Throw an error if the host is invalid. + * Throw an error if a Coder authority is invalid. */ export function parseRemoteAuthority(authority: string): AuthorityParts | null { const authorityParts = authority.split("+"); diff --git a/test/unit/uri/uriHandler.test.ts b/test/unit/uri/uriHandler.test.ts index 82d186e57..c6c084e0f 100644 --- a/test/unit/uri/uriHandler.test.ts +++ b/test/unit/uri/uriHandler.test.ts @@ -363,11 +363,43 @@ describe("uriHandler", () => { await handleUri(createMockUri("/open", "workspace=w")); expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining("Failed to handle URI"), + "Failed to handle URI", + expect.objectContaining({ + error: expect.stringContaining("owner must be specified"), + }), ); expect(showErrorMessage).toHaveBeenCalled(); }); + it("does not log URI tokens on failure", async () => { + const { handleUri, commands, logger } = createTestContext(); + commands.open.mockRejectedValue(new Error("Connection failed")); + + await handleUri( + createMockUri( + "/open", + `owner=o&workspace=w&url=${encodeURIComponent(TEST_URL)}&token=secret-token`, + ), + ); + + expect(logger.warn).toHaveBeenCalledWith( + "Failed to handle URI", + expect.objectContaining({ + error: "Connection failed", + }), + ); + const logged = [ + ...vi.mocked(logger.debug).mock.calls, + ...vi.mocked(logger.info).mock.calls, + ...vi.mocked(logger.warn).mock.calls, + ] + .map((call) => JSON.stringify(call)) + .join("\n"); + expect(logged).not.toContain("secret-token"); + expect(logged).not.toContain("token="); + expect(logged).not.toContain("vscode://coder.coder-remote"); + }); + it("propagates command errors", async () => { const { handleUri, commands, showErrorMessage } = createTestContext(); commands.open.mockRejectedValue(new Error("Connection failed")); diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 39daa59dc..952ee35af 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -12,6 +12,7 @@ import { openInBrowser, parseRemoteAuthority, resolveUiUrl, + toRemoteAuthority, toSafeHost, } from "@/util"; @@ -42,13 +43,17 @@ describe("parseRemoteAuthority", () => { it.each([ { - label: "missing user and workspace", + label: "missing username and workspace", sshHost: "coder-vscode.dev.coder.com", }, { label: "missing workspace", sshHost: "coder-vscode.dev.coder.com--foo", }, + { + label: "manual host using Coder prefix", + sshHost: "coder-vscode.personal-host", + }, { label: "empty username", sshHost: "coder-vscode.dev.coder.com----bar", @@ -88,6 +93,26 @@ describe("parseRemoteAuthority", () => { username?: string; } + it("round trips generated remote authorities", () => { + const authority = toRemoteAuthority( + "https://ほげ", + "alice", + "workspace", + "main", + ); + + expect(authority).toBe( + "ssh-remote+coder-vscode.xn--18j4d--alice--workspace.main", + ); + expect(parseRemoteAuthority(authority)).toStrictEqual({ + agent: "main", + sshHost: "coder-vscode.xn--18j4d--alice--workspace.main", + safeHostname: "xn--18j4d", + username: "alice", + workspace: "workspace", + } satisfies AuthorityParts); + }); + it.each([ { label: "hostname without agent",