Skip to content

Commit fd3a090

Browse files
committed
confirm repo-provided setup script before running
1 parent bf27356 commit fd3a090

7 files changed

Lines changed: 121 additions & 7 deletions

File tree

packages/core/src/task-detail/taskCreationHost.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,12 @@ export interface ITaskCreationHost {
128128
): Promise<string[]>;
129129
setProvisioningActive(taskId: string): void;
130130
clearProvisioning(taskId: string): void;
131+
confirmEnvironmentSetup(args: {
132+
repoPath: string;
133+
environmentId: string;
134+
name: string;
135+
script: string;
136+
}): Promise<boolean>;
131137
dispatchSetupAction(args: SetupActionDispatch): void;
132138
track(event: string, props?: Record<string, unknown>): void;
133139
importClaudeCliSession(args: {

packages/core/src/task-detail/taskCreationSaga.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ const mockHost = vi.hoisted(() => ({
2929
uploadRunAttachments: vi.fn(),
3030
setProvisioningActive: vi.fn(),
3131
clearProvisioning: vi.fn(),
32+
confirmEnvironmentSetup: vi.fn(async () => true),
3233
dispatchSetupAction: vi.fn(),
3334
importClaudeCliSession: vi.fn(),
3435
deleteClaudeCliImport: vi.fn(),

packages/core/src/task-detail/taskCreationSaga.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,9 +637,17 @@ export class TaskCreationSaga extends Saga<
637637
): void {
638638
this.deps.host
639639
.getEnvironment({ repoPath, id: environmentId })
640-
.then((env) => {
640+
.then(async (env) => {
641641
if (!env?.setup?.script) return;
642642

643+
const approved = await this.deps.host.confirmEnvironmentSetup({
644+
repoPath,
645+
environmentId,
646+
name: env.name,
647+
script: env.setup.script,
648+
});
649+
if (!approved) return;
650+
643651
this.deps.host.dispatchSetupAction({
644652
taskId,
645653
command: env.setup.script,
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { render } from "@testing-library/react";
2+
import { describe, expect, it, vi } from "vitest";
3+
4+
const useEnvironments = vi.fn();
5+
6+
vi.mock("./useEnvironments", () => ({
7+
useEnvironments: (repoPath: string | null) => useEnvironments(repoPath),
8+
}));
9+
10+
import { EnvironmentSelector } from "./EnvironmentSelector";
11+
12+
describe("EnvironmentSelector", () => {
13+
it("never selects a repo-provided environment on its own", () => {
14+
useEnvironments.mockReturnValue({
15+
data: [
16+
{ id: "env-1", name: "Malicious" },
17+
{ id: "env-2", name: "Other" },
18+
],
19+
});
20+
const onChange = vi.fn();
21+
22+
render(
23+
<EnvironmentSelector repoPath="/repo" value={null} onChange={onChange} />,
24+
);
25+
26+
expect(onChange).not.toHaveBeenCalled();
27+
});
28+
});

packages/ui/src/features/environments/EnvironmentSelector.tsx

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
ComboboxListFooter,
1111
ComboboxTrigger,
1212
} from "@posthog/quill";
13-
import { useEffect, useRef, useState } from "react";
13+
import { useRef, useState } from "react";
1414
import { useEnvironments } from "./useEnvironments";
1515

1616
interface EnvironmentSelectorProps {
@@ -35,11 +35,7 @@ export function EnvironmentSelector({
3535

3636
const { data: environments = [] } = useEnvironments(repoPath);
3737

38-
useEffect(() => {
39-
if (value === null && environments.length > 0) {
40-
onChange(environments[0].id);
41-
}
42-
}, [value, environments, onChange]);
38+
// Never auto-select: environments are repo-provided and run setup scripts.
4339

4440
const selectedEnvironment = environments.find((env) => env.id === value);
4541
const displayText = selectedEnvironment?.name ?? "No environment";
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
3+
vi.mock("@posthog/di/container", () => ({
4+
resolveService: vi.fn(),
5+
}));
6+
7+
vi.mock("../../shell/analytics", () => ({
8+
track: vi.fn(),
9+
captureException: vi.fn(),
10+
}));
11+
12+
import { TrpcTaskCreationHost } from "./taskCreationHostImpl";
13+
14+
const args = {
15+
repoPath: "/repo",
16+
environmentId: "env-1",
17+
name: "Dev",
18+
script: "npm run setup",
19+
};
20+
21+
describe("TrpcTaskCreationHost.confirmEnvironmentSetup", () => {
22+
const host = new TrpcTaskCreationHost();
23+
24+
afterEach(() => {
25+
vi.restoreAllMocks();
26+
});
27+
28+
it.each([
29+
{ answer: true, expected: true },
30+
{ answer: false, expected: false },
31+
])(
32+
"returns $expected when the user answers $answer",
33+
async ({ answer, expected }) => {
34+
vi.spyOn(window, "confirm").mockReturnValue(answer);
35+
36+
await expect(host.confirmEnvironmentSetup(args)).resolves.toBe(expected);
37+
},
38+
);
39+
40+
it("prompts on every run so an earlier answer cannot be reused", async () => {
41+
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true);
42+
43+
await host.confirmEnvironmentSetup(args);
44+
await host.confirmEnvironmentSetup(args);
45+
46+
expect(confirmSpy).toHaveBeenCalledTimes(2);
47+
});
48+
49+
it("warns that the script can execute other files in the repository", async () => {
50+
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
51+
52+
await host.confirmEnvironmentSetup(args);
53+
54+
expect(confirmSpy.mock.calls[0][0]).toContain(
55+
"can execute other files in the repository",
56+
);
57+
});
58+
});

packages/ui/src/features/task-detail/taskCreationHostImpl.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,23 @@ export class TrpcTaskCreationHost implements ITaskCreationHost {
194194
useProvisioningStore.getState().clear(taskId);
195195
}
196196

197+
async confirmEnvironmentSetup(args: {
198+
repoPath: string;
199+
environmentId: string;
200+
name: string;
201+
script: string;
202+
}): Promise<boolean> {
203+
// Asked every run, never remembered: the script text does not determine
204+
// what the command does, so a stored approval for "npm run setup" would
205+
// still hold after the repo changes what that runs.
206+
return window.confirm(
207+
`The environment "${args.name}" in ${args.repoPath} wants to run this ` +
208+
`setup script on your machine:\n\n${args.script}\n\n` +
209+
`It runs with your permissions and can execute other files in the ` +
210+
`repository. Run it?`,
211+
);
212+
}
213+
197214
dispatchSetupAction(args: SetupActionDispatch): void {
198215
const actionId = `setup-${args.taskId}-${Date.now()}`;
199216
usePanelLayoutStore

0 commit comments

Comments
 (0)