Harden environment cleanup paths

This commit is contained in:
Devin Foley
2026-04-23 19:02:19 -07:00
parent 7d1eb6fe0c
commit 193f55d916
5 changed files with 163 additions and 31 deletions

View File

@@ -1036,6 +1036,31 @@ describe("environment routes", () => {
);
});
it("skips SSH secret cleanup gracefully when stored SSH config no longer parses", async () => {
const environment = {
...createEnvironment(),
name: "SSH Fixture",
driver: "ssh" as const,
config: {
host: "",
username: "ssh-user",
},
};
mockEnvironmentService.getById.mockResolvedValue(environment);
mockEnvironmentService.remove.mockResolvedValue(environment);
const app = createApp({
type: "board",
userId: "user-1",
source: "local_implicit",
});
const res = await request(app).delete(`/api/environments/${environment.id}`);
expect(res.status).toBe(200);
expect(mockEnvironmentService.remove).toHaveBeenCalledWith(environment.id);
expect(mockSecretService.remove).not.toHaveBeenCalled();
});
it("returns 404 when deleting a missing environment", async () => {
mockEnvironmentService.getById.mockResolvedValue(null);
const app = createApp({

View File

@@ -5,6 +5,7 @@ import path from "node:path";
import { randomUUID } from "node:crypto";
import { promisify } from "node:util";
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
import { inArray } from "drizzle-orm";
import {
companies,
createDb,
@@ -244,6 +245,104 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => {
]));
});
it("clears matching environment selections transactionally without touching other workspaces", async () => {
const companyId = randomUUID();
const projectId = randomUUID();
const matchingWorkspaceId = randomUUID();
const otherWorkspaceId = randomUUID();
const untouchedWorkspaceId = randomUUID();
const environmentId = randomUUID();
await db.insert(companies).values({
id: companyId,
name: "Paperclip",
issuePrefix: "PAP",
requireBoardApprovalForNewAgents: false,
});
await db.insert(projects).values({
id: projectId,
companyId,
name: "Workspace cleanup",
status: "in_progress",
executionWorkspacePolicy: {
enabled: true,
},
});
await db.insert(executionWorkspaces).values([
{
id: matchingWorkspaceId,
companyId,
projectId,
mode: "isolated_workspace",
strategyType: "directory",
name: "Matching workspace",
status: "active",
providerType: "local_fs",
cwd: "/tmp/workspace-a",
metadata: {
source: "manual",
config: {
environmentId,
cleanupCommand: "echo clean",
},
},
},
{
id: otherWorkspaceId,
companyId,
projectId,
mode: "isolated_workspace",
strategyType: "directory",
name: "Different environment",
status: "active",
providerType: "local_fs",
cwd: "/tmp/workspace-b",
metadata: {
source: "manual",
config: {
environmentId: randomUUID(),
},
},
},
{
id: untouchedWorkspaceId,
companyId,
projectId,
mode: "isolated_workspace",
strategyType: "directory",
name: "No environment",
status: "active",
providerType: "local_fs",
cwd: "/tmp/workspace-c",
metadata: {
source: "manual",
},
},
]);
const cleared = await svc.clearEnvironmentSelection(companyId, environmentId);
expect(cleared).toBe(1);
const rows = await db
.select({
id: executionWorkspaces.id,
metadata: executionWorkspaces.metadata,
})
.from(executionWorkspaces)
.where(inArray(executionWorkspaces.id, [matchingWorkspaceId, otherWorkspaceId, untouchedWorkspaceId]));
const byId = new Map(rows.map((row) => [row.id, row.metadata as Record<string, unknown> | null]));
expect(readExecutionWorkspaceConfig(byId.get(matchingWorkspaceId) ?? null)).toMatchObject({
environmentId: null,
cleanupCommand: "echo clean",
});
expect(readExecutionWorkspaceConfig(byId.get(otherWorkspaceId) ?? null)).toMatchObject({
environmentId: expect.any(String),
});
expect(readExecutionWorkspaceConfig(byId.get(untouchedWorkspaceId) ?? null)).toBeNull();
});
it("warns about dirty and unmerged git worktrees and reports cleanup actions", async () => {
const repoRoot = await createTempRepo();
tempDirs.add(repoRoot);

View File

@@ -22,6 +22,7 @@ import {
normalizeEnvironmentConfigForPersistence,
normalizeEnvironmentConfigForProbe,
parseEnvironmentDriverConfig,
readSshEnvironmentPrivateKeySecretId,
type ParsedEnvironmentConfig,
} from "../services/environment-config.js";
import { probeEnvironment } from "../services/environment-probe.js";
@@ -319,14 +320,9 @@ export function environmentRoutes(db: Db) {
res.status(404).json({ error: "Environment not found" });
return;
}
if (existing.driver === "ssh") {
const parsed = parseEnvironmentDriverConfig(existing);
if (parsed.driver === "ssh") {
const secretId = parsed.config.privateKeySecretRef?.secretId;
if (secretId) {
await secrets.remove(secretId);
}
}
const secretId = readSshEnvironmentPrivateKeySecretId(existing);
if (secretId) {
await secrets.remove(secretId);
}
const actor = getActorInfo(req);
await logActivity(db, {

View File

@@ -206,6 +206,15 @@ export async function resolveEnvironmentDriverConfigForRuntime(
return parsed;
}
export function readSshEnvironmentPrivateKeySecretId(
environment: Pick<Environment, "driver" | "config">,
): string | null {
if (environment.driver !== "ssh") return null;
const parsed = sshEnvironmentConfigSchema.safeParse(parseObject(environment.config));
if (!parsed.success) return null;
return parsed.data.privateKeySecretRef?.secretId ?? null;
}
export function parseEnvironmentDriverConfig(
environment: Pick<Environment, "driver" | "config">,
): ParsedEnvironmentConfig {

View File

@@ -745,31 +745,34 @@ export function executionWorkspaceService(db: Db) {
},
clearEnvironmentSelection: async (companyId: string, environmentId: string) => {
const rows = await db
.select({
id: executionWorkspaces.id,
metadata: executionWorkspaces.metadata,
})
.from(executionWorkspaces)
.where(eq(executionWorkspaces.companyId, companyId));
let cleared = 0;
for (const row of rows) {
const metadata = (row.metadata as Record<string, unknown> | null) ?? null;
const config = readExecutionWorkspaceConfig(metadata);
if (config?.environmentId !== environmentId) continue;
await db
.update(executionWorkspaces)
.set({
metadata: mergeExecutionWorkspaceConfig(metadata, { environmentId: null }),
updatedAt: new Date(),
return db.transaction(async (tx) => {
const rows = await tx
.select({
id: executionWorkspaces.id,
metadata: executionWorkspaces.metadata,
})
.where(eq(executionWorkspaces.id, row.id));
cleared += 1;
}
.from(executionWorkspaces)
.where(eq(executionWorkspaces.companyId, companyId));
return cleared;
let cleared = 0;
const updatedAt = new Date();
for (const row of rows) {
const metadata = (row.metadata as Record<string, unknown> | null) ?? null;
const config = readExecutionWorkspaceConfig(metadata);
if (config?.environmentId !== environmentId) continue;
await tx
.update(executionWorkspaces)
.set({
metadata: mergeExecutionWorkspaceConfig(metadata, { environmentId: null }),
updatedAt,
})
.where(eq(executionWorkspaces.id, row.id));
cleared += 1;
}
return cleared;
});
},
};
}