From ee7a3a58a5d00bccf70e92a14ef91e6e015bd651 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sat, 20 Jun 2026 00:50:54 -0700 Subject: [PATCH] fix(server): validate instance env settings access --- .../environment-instance-routes.test.ts | 20 +++++++++++- .../instance-settings-routes.test.ts | 31 +++++++++++++++++++ server/src/routes/environments.ts | 16 ++++++---- server/src/routes/instance-settings.ts | 10 ++++++ 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/server/src/__tests__/environment-instance-routes.test.ts b/server/src/__tests__/environment-instance-routes.test.ts index a0aafd1eb95..e146f91fb58 100644 --- a/server/src/__tests__/environment-instance-routes.test.ts +++ b/server/src/__tests__/environment-instance-routes.test.ts @@ -134,6 +134,24 @@ describe("environment instance routes", () => { }); }); + it("allows non-admin board members with company access to read the shared environment catalog", async () => { + mockEnvironmentService.list.mockResolvedValue([createEnvironment()]); + const app = createApp({ + type: "board", + userId: "user-1", + source: "session", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", membershipRole: "member", status: "active" }], + isInstanceAdmin: false, + }); + + const res = await request(app).get("/api/companies/company-1/environments"); + + expect(res.status).toBe(200); + expect(res.body).toHaveLength(1); + expect(mockEnvironmentService.list).toHaveBeenCalledTimes(1); + }); + it("rejects company agents from enumerating the shared environment catalog", async () => { const app = createApp({ type: "agent", @@ -146,7 +164,7 @@ describe("environment instance routes", () => { const res = await request(app).get("/api/companies/company-1/environments"); expect(res.status).toBe(403); - expect(res.body.error).toContain("board operators"); + expect(res.body.error).toContain("Board access required"); }); it("rejects non-admin signed-in board members from mutating instance environments", async () => { diff --git a/server/src/__tests__/instance-settings-routes.test.ts b/server/src/__tests__/instance-settings-routes.test.ts index 969785b40a5..bd6b63a9c4b 100644 --- a/server/src/__tests__/instance-settings-routes.test.ts +++ b/server/src/__tests__/instance-settings-routes.test.ts @@ -15,6 +15,9 @@ const mockHeartbeatService = vi.hoisted(() => ({ buildIssueGraphLivenessAutoRecoveryPreview: vi.fn(), reconcileIssueGraphLiveness: vi.fn(), })); +const mockEnvironmentService = vi.hoisted(() => ({ + getById: vi.fn(), +})); const mockLogActivity = vi.hoisted(() => vi.fn()); function registerModuleMocks() { @@ -23,6 +26,9 @@ function registerModuleMocks() { instanceSettingsService: () => mockInstanceSettingsService, logActivity: mockLogActivity, })); + vi.doMock("../services/environments.js", () => ({ + environmentService: () => mockEnvironmentService, + })); } async function createApp(actor: any) { @@ -59,6 +65,7 @@ describe("instance settings routes", () => { mockInstanceSettingsService.listCompanyIds.mockReset(); mockHeartbeatService.buildIssueGraphLivenessAutoRecoveryPreview.mockReset(); mockHeartbeatService.reconcileIssueGraphLiveness.mockReset(); + mockEnvironmentService.getById.mockReset(); mockLogActivity.mockReset(); mockInstanceSettingsService.get.mockResolvedValue({ id: "instance-settings-1", @@ -162,6 +169,12 @@ describe("instance settings routes", () => { skippedOutsideLookback: 0, escalationIssueIds: ["issue-2"], }); + mockEnvironmentService.getById.mockResolvedValue({ + id: "env-1", + driver: "local", + status: "active", + config: {}, + }); }); it("allows local board users to read and update experimental settings", async () => { @@ -220,6 +233,24 @@ describe("instance settings routes", () => { expect(mockLogActivity).toHaveBeenCalledTimes(2); }); + it("rejects unknown defaultEnvironmentId values with 422", async () => { + mockEnvironmentService.getById.mockResolvedValue(null); + const app = await createApp({ + type: "board", + userId: "local-board", + source: "local_implicit", + isInstanceAdmin: true, + }); + + const res = await request(app) + .patch("/api/instance/settings") + .send({ defaultEnvironmentId: "11111111-1111-4111-8111-111111111111" }); + + expect(res.status).toBe(422); + expect(res.body.error).toContain("Environment not found"); + expect(mockInstanceSettingsService.update).not.toHaveBeenCalled(); + }); + it("allows local board users to update guarded dev-server auto-restart", async () => { const app = await createApp({ type: "board", diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index 22e7b13759d..f0fb87bb978 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -26,7 +26,7 @@ import { probeEnvironment } from "../services/environment-probe.js"; import { secretService } from "../services/secrets.js"; import { listReadyPluginEnvironmentDrivers } from "../services/plugin-environment-driver.js"; import { getConfiguredSecretProvider } from "../secrets/configured-provider.js"; -import { getActorInfo } from "./authz.js"; +import { assertBoardOrgAccess, getActorInfo } from "./authz.js"; import type { PluginWorkerManager } from "../services/plugin-worker-manager.js"; import { environmentService } from "../services/environments.js"; import { executionWorkspaceService } from "../services/execution-workspaces.js"; @@ -58,6 +58,10 @@ export function environmentRoutes( throw forbidden("Instance admin access required"); } + function assertCanReadInstanceEnvironments(req: Request) { + assertBoardOrgAccess(req); + } + async function assertCanReadSecretsForDraftProbe(req: Request, companyId: string) { assertCanAccessInstanceEnvironments(req); return companyId; @@ -155,7 +159,7 @@ export function environmentRoutes( } router.get("/companies/:companyId/environments", async (req, res) => { - assertCanAccessInstanceEnvironments(req); + assertCanReadInstanceEnvironments(req); const rows = await svc.list({ status: req.query.status as string | undefined, driver: req.query.driver as string | undefined, @@ -164,7 +168,7 @@ export function environmentRoutes( }); router.get("/companies/:companyId/environments/capabilities", async (req, res) => { - assertCanAccessInstanceEnvironments(req); + assertCanReadInstanceEnvironments(req); const pluginDrivers = await listReadyPluginEnvironmentDrivers({ db, workerManager: options.pluginWorkerManager, @@ -253,7 +257,7 @@ export function environmentRoutes( res.status(404).json({ error: "Environment not found" }); return; } - assertCanAccessInstanceEnvironments(req); + assertCanReadInstanceEnvironments(req); res.json(environment); }); @@ -263,7 +267,7 @@ export function environmentRoutes( res.status(404).json({ error: "Environment not found" }); return; } - assertCanAccessInstanceEnvironments(req); + assertCanReadInstanceEnvironments(req); const leases = await svc.listLeases(environment.id, { status: req.query.status as string | undefined, }); @@ -276,7 +280,7 @@ export function environmentRoutes( res.status(404).json({ error: "Environment lease not found" }); return; } - assertCanAccessInstanceEnvironments(req); + assertCanReadInstanceEnvironments(req); res.json(lease); }); diff --git a/server/src/routes/instance-settings.ts b/server/src/routes/instance-settings.ts index 45e212e5b55..3e63de6af84 100644 --- a/server/src/routes/instance-settings.ts +++ b/server/src/routes/instance-settings.ts @@ -9,6 +9,8 @@ import { import { forbidden } from "../errors.js"; import { validate } from "../middleware/validate.js"; import { heartbeatService, instanceSettingsService, logActivity } from "../services/index.js"; +import { environmentService } from "../services/environments.js"; +import { assertEnvironmentSelectionForCompany } from "./environment-selection.js"; import { assertBoardOrgAccess, getActorInfo } from "./authz.js"; function assertCanManageInstanceSettings(req: Request) { @@ -24,6 +26,7 @@ function assertCanManageInstanceSettings(req: Request) { export function instanceSettingsRoutes(db: Db) { const router = Router(); const svc = instanceSettingsService(db); + const environments = environmentService(db); const heartbeat = heartbeatService(db); router.get("/instance/settings", async (req, res) => { @@ -36,6 +39,13 @@ export function instanceSettingsRoutes(db: Db) { validate(patchInstanceSettingsSchema), async (req, res) => { assertCanManageInstanceSettings(req); + if (Object.prototype.hasOwnProperty.call(req.body, "defaultEnvironmentId")) { + await assertEnvironmentSelectionForCompany( + environments, + "instance", + typeof req.body.defaultEnvironmentId === "string" ? req.body.defaultEnvironmentId : null, + ); + } const updated = await svc.update(req.body); const actor = getActorInfo(req); const companyIds = await svc.listCompanyIds();