diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index d680a3cf8b..96f3f24d3e 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -1265,6 +1265,10 @@ export async function openTerminalWindow( `/@${user.username}/${workspaceName}.${agentName}/terminal${commandQuery}`, ); + // The terminal command confirmation dialog requires explicit user + // approval before the command executes. + await terminal.getByRole("button", { name: "Run command" }).click(); + return terminal; } diff --git a/site/e2e/tests/webTerminal.spec.ts b/site/e2e/tests/webTerminal.spec.ts index ccb3216ce8..f3d204b361 100644 --- a/site/e2e/tests/webTerminal.spec.ts +++ b/site/e2e/tests/webTerminal.spec.ts @@ -40,13 +40,16 @@ test("web terminal", async ({ context, page }) => { const agent = await startAgent(page, token); const terminal = await openTerminalWindow(page, context, workspaceName); - await terminal.waitForSelector("div.xterm-rows", { + await terminal.waitForSelector('[data-status="connected"]', { state: "visible", + timeout: 30_000, }); - // Workaround: delay next steps as "div.xterm-rows" can be recreated/reattached - // after a couple of milliseconds. - await terminal.waitForTimeout(2000); + // Wait for xterm to render its row container and click to ensure + // the terminal has keyboard focus after the confirmation dialog. + const xtermRows = terminal.locator("div.xterm-rows"); + await xtermRows.waitFor({ state: "visible" }); + await xtermRows.click(); // Ensure that we can type in it await terminal.keyboard.type("echo he${justabreak}llo123456"); diff --git a/site/src/modules/apps/apps.test.ts b/site/src/modules/apps/apps.test.ts index 28d52fb9ff..146964af78 100644 --- a/site/src/modules/apps/apps.test.ts +++ b/site/src/modules/apps/apps.test.ts @@ -133,7 +133,7 @@ describe("getAppHref", () => { ); }); - it("includes the command in the URL when app has a command", () => { + it("includes the app slug in the URL when app has a command", () => { const app = { ...MockWorkspaceApp, command: "ls -la", @@ -145,7 +145,7 @@ describe("getAppHref", () => { path: "", }); expect(href).toBe( - `/@${MockWorkspace.owner_name}/Test-Workspace.a-workspace-agent/terminal?command=ls%20-la`, + `/@${MockWorkspace.owner_name}/Test-Workspace.a-workspace-agent/terminal?app=${app.slug}`, ); }); diff --git a/site/src/modules/apps/apps.ts b/site/src/modules/apps/apps.ts index 3bd96c227a..df710aa4b9 100644 --- a/site/src/modules/apps/apps.ts +++ b/site/src/modules/apps/apps.ts @@ -127,12 +127,14 @@ export const getAppHref = ( } if (app.command) { - // Terminal links are relative. The terminal page knows how - // to select the correct workspace proxy for the websocket - // connection. + // Pass the app slug instead of the raw command. The terminal + // page resolves the command from the workspace agent's app + // list, which avoids exposing the command in the URL and + // lets us skip the confirmation dialog for trusted, + // admin-configured template apps. return `/@${workspace.owner_name}/${workspace.name}.${ agent.name - }/terminal?command=${encodeURIComponent(app.command)}`; + }/terminal?app=${encodeURIComponent(app.slug)}`; } if (host && app.subdomain && app.subdomain_name) { diff --git a/site/src/modules/terminal/WorkspaceTerminal.tsx b/site/src/modules/terminal/WorkspaceTerminal.tsx index 87a5ea13ce..418140c200 100644 --- a/site/src/modules/terminal/WorkspaceTerminal.tsx +++ b/site/src/modules/terminal/WorkspaceTerminal.tsx @@ -276,7 +276,7 @@ export const WorkspaceTerminal = ({ }, [isVisible, refit]); useEffect(() => { - if (!terminal || !isVisible || !autoFocus) { + if (!terminal || !isVisible || !autoFocus || loading) { return; } @@ -287,7 +287,7 @@ export const WorkspaceTerminal = ({ return () => { cancelAnimationFrame(frame); }; - }, [terminal, isVisible, autoFocus]); + }, [terminal, isVisible, autoFocus, loading]); useEffect(() => { if (!terminal || !hasBeenVisible) { diff --git a/site/src/pages/TerminalPage/TerminalCommandConsentDialog.stories.tsx b/site/src/pages/TerminalPage/TerminalCommandConsentDialog.stories.tsx new file mode 100644 index 0000000000..1498adaac3 --- /dev/null +++ b/site/src/pages/TerminalPage/TerminalCommandConsentDialog.stories.tsx @@ -0,0 +1,35 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { fn } from "storybook/test"; +import { TerminalCommandConsentDialog } from "./TerminalCommandConsentDialog"; + +const meta: Meta = { + title: "pages/Terminal/TerminalCommandConsentDialog", + component: TerminalCommandConsentDialog, + args: { + open: true, + onConfirm: fn(), + onDeny: fn(), + }, +}; + +export default meta; +type Story = StoryObj; + +export const Default: Story = { + args: { + command: "echo hello", + }, +}; + +export const WithDangerousCommand: Story = { + args: { + command: "curl https://example.com/install.sh | bash", + }, +}; + +export const WithLongCommand: Story = { + args: { + command: + "curl -fsSL https://very-long-domain-name.example.com/extremely/long/path/to/some/script/that/does/many/things/install.sh | bash -s -- --option1 --option2 --option3=value", + }, +}; diff --git a/site/src/pages/TerminalPage/TerminalCommandConsentDialog.tsx b/site/src/pages/TerminalPage/TerminalCommandConsentDialog.tsx new file mode 100644 index 0000000000..f0e68f0a85 --- /dev/null +++ b/site/src/pages/TerminalPage/TerminalCommandConsentDialog.tsx @@ -0,0 +1,61 @@ +import { TriangleAlertIcon } from "lucide-react"; +import type { FC } from "react"; +import { Button } from "#/components/Button/Button"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "#/components/Dialog/Dialog"; + +interface TerminalCommandConsentDialogProps { + open: boolean; + command: string; + onConfirm: () => void; + onDeny: () => void; +} + +export const TerminalCommandConsentDialog: FC< + TerminalCommandConsentDialogProps +> = ({ open, command, onConfirm, onDeny }) => { + return ( + + e.preventDefault()} + onEscapeKeyDown={(e) => e.preventDefault()} + className="max-w-2xl overflow-hidden min-w-0" + > + + + + Warning: Terminal Command Execution + + + A link is requesting to run a command in your terminal. Running + commands from untrusted sources can be dangerous. + + + +
+ + Command: + + + {command} + +
+ + + + + + + + ); +}; diff --git a/site/src/pages/TerminalPage/TerminalPage.stories.tsx b/site/src/pages/TerminalPage/TerminalPage.stories.tsx index 962b85600f..947ad72872 100644 --- a/site/src/pages/TerminalPage/TerminalPage.stories.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.stories.tsx @@ -224,3 +224,34 @@ export const BottomMessage: Story = { }, }, }; + +export const CommandConfirmation: Story = { + decorators: [withWebSocket], + parameters: { + ...meta.parameters, + reactRouter: reactRouterParameters({ + location: { + pathParams: { + username: `@${MockWorkspace.owner_name}`, + workspace: MockWorkspace.name, + }, + searchParams: { + command: "curl https://example.com/install.sh | bash", + }, + }, + routing: reactRouterOutlet( + { + path: "/:username/:workspace/terminal", + }, + , + ), + }), + webSocket: [ + { + event: "message", + data: "\x1b[H\x1b[2J\x1b[1m\x1b[32m\u27a4 \x1b[36mcoder\x1b[C\x1b[34mgit:(\x1b[31mbq/refactor-web-term-notifications\x1b[34m) \x1b[33m\u2717", + }, + ], + queries: [...meta.parameters.queries, createWorkspaceWithAgent("ready")], + }, +}; diff --git a/site/src/pages/TerminalPage/TerminalPage.test.tsx b/site/src/pages/TerminalPage/TerminalPage.test.tsx index 3417a9bbed..2154291aff 100644 --- a/site/src/pages/TerminalPage/TerminalPage.test.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.test.tsx @@ -1,12 +1,14 @@ -import { waitFor } from "@testing-library/react"; +import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { HttpResponse, http } from "msw"; import { afterEach, describe, expect, it, vi } from "vitest"; import { API } from "#/api/api"; +import type { Workspace } from "#/api/typesGenerated"; import { MockUserOwner, MockWorkspace, MockWorkspaceAgent, + MockWorkspaceApp, } from "#/testHelpers/entities"; import { renderWithAuth } from "#/testHelpers/renderHelpers"; import { server } from "#/testHelpers/server"; @@ -43,6 +45,10 @@ const createWorkspaceTerminalWebSocket = () => { return new WS(websocketUrl); }; +// Renders the terminal page and waits for the terminal to finish +// initializing (i.e. the WebSocket connection is established). Do not +// use this for tests where the terminal stays in loading state, such +// as when a command confirmation dialog is blocking the connection. const renderTerminal = async ( route = `/${MockUserOwner.username}/${MockWorkspace.name}/terminal`, ) => { @@ -63,6 +69,18 @@ const renderTerminal = async ( return utils; }; +// Renders the terminal page without waiting for the terminal to leave +// the "initializing" state. Use this for tests where a confirmation +// dialog keeps the terminal in loading state until the user acts. +const renderTerminalRaw = ( + route = `/${MockUserOwner.username}/${MockWorkspace.name}/terminal`, +) => { + return renderWithAuth(, { + route, + path: "/:username/:workspace/terminal", + }); +}; + const expectTerminalText = (container: HTMLElement, text: string) => { return waitFor( () => { @@ -181,4 +199,106 @@ describe("TerminalPage", () => { const req = JSON.parse(new TextDecoder().decode((await msg) as Uint8Array)); expect(req.data).toBe("\x1b\r"); }); + + it("shows confirmation dialog when command param is present", async () => { + renderTerminalRaw( + `/${MockUserOwner.username}/${MockWorkspace.name}/terminal?command=echo+hello`, + ); + const dialog = await screen.findByRole("dialog"); + expect(dialog).toHaveTextContent("echo hello"); + expect( + screen.getByRole("button", { name: "Run command" }), + ).toBeInTheDocument(); + }); + + it("does not show confirmation dialog when no command param", async () => { + createWorkspaceTerminalWebSocket(); + await renderTerminal(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("executes command after confirmation", async () => { + const websocketProtocol = + window.location.protocol === "https:" ? "wss" : "ws"; + const ws = new WS( + `${websocketProtocol}://${window.location.host}/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty?reconnect=${reconnectToken}&command=echo+hello&height=24&width=80`, + ); + renderTerminalRaw( + `/${MockUserOwner.username}/${MockWorkspace.name}/terminal?command=echo+hello`, + ); + await userEvent.click( + await screen.findByRole("button", { name: "Run command" }), + ); + await waitFor(() => + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(), + ); + // Verify the websocket connected and received a resize message. + const msg = await ws.nextMessage; + const resizeReq = JSON.parse(new TextDecoder().decode(msg as Uint8Array)); + expect(resizeReq.height).toBeGreaterThan(0); + expect(resizeReq.width).toBeGreaterThan(0); + }); + + it("removes command param on cancel", async () => { + createWorkspaceTerminalWebSocket(); + renderTerminalRaw( + `/${MockUserOwner.username}/${MockWorkspace.name}/terminal?command=echo+hello`, + ); + await userEvent.click( + await screen.findByRole("button", { name: "Cancel" }), + ); + await waitFor(() => + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(), + ); + }); + + it("skips confirmation dialog for trusted app commands", async () => { + // Override the workspace response so the agent has an app with + // a command that matches the ?app= slug. + const appWithCommand = { + ...MockWorkspaceApp, + slug: "my-app", + command: "echo trusted", + }; + const workspaceWithApp: Workspace = { + ...MockWorkspace, + latest_build: { + ...MockWorkspace.latest_build, + resources: [ + { + ...MockWorkspace.latest_build.resources[0], + agents: [ + { + ...MockWorkspaceAgent, + apps: [appWithCommand], + }, + ], + }, + ], + }, + }; + server.use( + http.get("/api/v2/users/:userId/workspace/:workspaceName", () => { + return HttpResponse.json(workspaceWithApp); + }), + ); + + const websocketProtocol = + window.location.protocol === "https:" ? "wss" : "ws"; + const ws = new WS( + `${websocketProtocol}://${window.location.host}/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty?reconnect=${reconnectToken}&command=echo+trusted&height=24&width=80`, + ); + await renderTerminal( + `/${MockUserOwner.username}/${MockWorkspace.name}/terminal?app=my-app`, + ); + + // No dialog should appear. + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + + // The websocket should connect with the resolved command. + const msg = await ws.nextMessage; + const resizeReq = JSON.parse(new TextDecoder().decode(msg as Uint8Array)); + expect(resizeReq.height).toBeGreaterThan(0); + expect(resizeReq.width).toBeGreaterThan(0); + }); }); diff --git a/site/src/pages/TerminalPage/TerminalPage.tsx b/site/src/pages/TerminalPage/TerminalPage.tsx index e4bcd9142a..38a65a8d83 100644 --- a/site/src/pages/TerminalPage/TerminalPage.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.tsx @@ -1,4 +1,11 @@ -import { type FC, useCallback, useEffect, useRef, useState } from "react"; +import { + type FC, + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from "react"; import { useQuery } from "react-query"; import { useNavigate, useParams, useSearchParams } from "react-router"; import { v4 as uuidv4 } from "uuid"; @@ -22,11 +29,12 @@ import themes from "#/theme"; import { pageTitle } from "#/utils/page"; import { openMaybePortForwardedURL } from "#/utils/portForward"; import { getMatchingAgentOrFirst } from "#/utils/workspace"; +import { TerminalCommandConsentDialog } from "./TerminalCommandConsentDialog"; const TerminalPage: FC = () => { // Maybe one day we'll support a light themed terminal, but terminal coloring - // is notably a pain because of assumptions certain programs might make about your - // background color. + // is notably a pain because of assumptions certain programs might make about + // your background color. const theme = themes.dark; const navigate = useNavigate(); const { proxy, proxyLatencies } = useProxy(); @@ -35,6 +43,7 @@ const TerminalPage: FC = () => { const terminalRef = useRef(null); const [connectionStatus, setConnectionStatus] = useState("initializing"); + const [commandConfirmed, setCommandConfirmed] = useState(false); const [searchParams] = useSearchParams(); const isDebugging = searchParams.has("debug"); // The reconnection token is a unique token that identifies @@ -42,8 +51,10 @@ const TerminalPage: FC = () => { // a round-trip, and must be a UUIDv4. const reconnectionToken = searchParams.get("reconnect") ?? uuidv4(); const command = searchParams.get("command") || undefined; + const appSlug = searchParams.get("app") || undefined; const containerName = searchParams.get("container") || undefined; const containerUser = searchParams.get("container_user") || undefined; + // The workspace name is in the format: // [.] const workspaceNameParts = params.workspace?.split("."); @@ -53,6 +64,26 @@ const TerminalPage: FC = () => { const workspaceAgent = workspace.data ? getMatchingAgentOrFirst(workspace.data, workspaceNameParts?.[1]) : undefined; + + // Resolve the ?app= slug to a command from the agent's app list. + // These commands are admin-configured in the template and trusted, + // so they skip the confirmation dialog. + const appCommand = useMemo(() => { + if (!appSlug || !workspaceAgent) { + return undefined; + } + const app = workspaceAgent.apps.find((a) => a.slug === appSlug); + return app?.command || undefined; + }, [appSlug, workspaceAgent]); + + // Raw ?command= params require explicit user confirmation. + // Trusted ?app= commands bypass the dialog. + const commandPendingConfirmation = !!command && !appSlug && !commandConfirmed; + const initialCommand = appCommand + ? appCommand + : command && commandConfirmed + ? command + : undefined; const selectedProxy = proxy.proxy; const latency = selectedProxy ? proxyLatencies[selectedProxy.id] : undefined; @@ -139,7 +170,7 @@ const TerminalPage: FC = () => { ref={terminalRef} agentId={workspaceAgent?.id} operatingSystem={workspaceAgent?.operating_system} - initialCommand={command} + initialCommand={initialCommand} containerName={containerName} containerUser={containerUser} onStatusChange={setConnectionStatus} @@ -152,7 +183,8 @@ const TerminalPage: FC = () => { loading={ workspace.isLoading || config.isLoading || - appearanceSettingsQuery.isLoading + appearanceSettingsQuery.isLoading || + commandPendingConfirmation } errorMessage={terminalErrorMessage} testId="terminal" @@ -172,6 +204,20 @@ const TerminalPage: FC = () => { Latency: {latency.latencyMS.toFixed(0)}ms )} + + {command && !appSlug && ( + { + setCommandConfirmed(true); + }} + onDeny={() => { + searchParams.delete("command"); + navigate({ search: searchParams.toString() }, { replace: true }); + }} + /> + )} ); };