mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): require confirmation before executing terminal command from URL (#24650)
The terminal page auto-executed commands from the `?command=` query
parameter
on page load without user confirmation. Because session auth uses
`SameSite=Lax`
cookies, an attacker could craft a link (phishing email, Slack DM,
external page)
that executes arbitrary commands in a victim's workspace when clicked.
Adds a `ConfirmDialog` that shows the exact command and requires
explicit user
approval before it is passed to the terminal WebSocket. Canceling
removes the
`command` parameter from the URL and opens a plain terminal.
<details>
<summary>Implementation details</summary>
### Data flow (before)
`TerminalPage.tsx` reads `searchParams.get("command")` and passes it
directly
as `initialCommand` to `WorkspaceTerminal`, which embeds it in the
WebSocket
URL. `proxy.go` forwards it to the agent, which runs `bash -c
"<command>"`
immediately.
### Fix
- Added `commandConfirmed` state and `commandPendingConfirmation` flag
in
`TerminalPage.tsx`.
- The `loading` prop passed to `WorkspaceTerminal` includes
`commandPendingConfirmation`, keeping the terminal in loading state
until
the user confirms or cancels.
- The command is only passed as `initialCommand` after the user clicks
"Run command" in the confirmation dialog.
- Trusted `?app=` commands (resolved from agent apps) bypass the dialog.
- Cancel removes the `?command=` parameter from the URL entirely.
- No backend changes needed; the frontend gates the command before it
reaches the WebSocket.
### Terminal focus after dialog
`WorkspaceTerminal`'s autoFocus effect previously depended on
`[terminal, isVisible, autoFocus]` but not `loading`. It fired while the
Radix dialog's focus trap was active, so `terminal.focus()` was
intercepted. When `loading` became false after confirming the dialog,
the
effect did not re-fire. Fixed by adding `loading` to the effect deps and
skipping focus while `loading` is true.
### Files changed
| File | Change |
|------|--------|
| `site/src/pages/TerminalPage/TerminalPage.tsx` | Confirmation dialog,
`commandPendingConfirmation` in loading prop |
| `site/src/pages/TerminalPage/TerminalCommandConsentDialog.tsx` | New
dialog component |
| `site/src/pages/TerminalPage/TerminalCommandConsentDialog.stories.tsx`
| Storybook story for dialog |
| `site/src/pages/TerminalPage/TerminalPage.stories.tsx` |
`CommandConfirmation` story |
| `site/src/pages/TerminalPage/TerminalPage.test.tsx` | 4 new dialog
tests, `renderTerminalRaw` helper for non-blocking render |
| `site/src/modules/terminal/WorkspaceTerminal.tsx` | Add `loading` to
autoFocus effect deps |
| `site/e2e/helpers.ts` | Dismiss dialog in `openTerminalWindow` helper
|
| `site/e2e/tests/webTerminal.spec.ts` | Wait for
`data-status="connected"` + click terminal for focus |
</details>
> 🤖 Generated by Coder Agents
---------
Co-authored-by: Jakub Domeracki <jakub@coder.com>
This commit is contained in:
co-authored by
Jakub Domeracki
parent
d5a5be116d
commit
66abd8a271
@@ -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}`,
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user