fix(server): keep environment secret bindings consistent when re-pointing config secrets (#10576)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents can run inside environments (SSH boxes, sandbox providers); a sandbox environment's config can reference stored company secrets (for example a provider API key) through `format: "secret-ref"` fields > - Environments are instance-scoped and shared by every company on an instance, but `company_secret_bindings` rows are company-scoped, and the environment routes synced config-derived bindings under one guessed "context company" resolved from the environment's existing bindings > - When a save re-pointed a secret-ref field at a secret owned by a different company, the binding sync threw after the config row had already been persisted: the config referenced the new secret, the binding still pointed at the old one, every later lease acquisition failed with `Secret is not bound to environment:<id> at apiKey`, and the stale cross-company binding made every later save fail with a company-context conflict — with no route-level way to recover > - This pull request makes config-derived bindings follow the company that owns each referenced secret, and makes the environment write and its binding syncs atomic > - The benefit is that environment saves can no longer strand an environment in a half-updated state that breaks all of its runs ## Linked Issues or Issue Description Refs #10577 (companion UX change: the editor state that nudges operators into this sequence). **What happened?** Saving an environment whose secret-ref config field points at a secret owned by a different company than the environment's existing binding partially applied: the config row updated, the binding sync failed server-side, and the environment was left referencing a secret it has no binding for. Every run that leased the environment then failed with `lease_acquire_failed: ... Secret is not bound to environment:<id> at apiKey`, and every later save of the environment returned 409 `Environment secret bindings already use a different company context.` — with no route-level way to recover. **Steps to reproduce** 1. On an instance with two companies, create a sandbox environment from company A with a picker-bound API-key secret owned by A (the binding lands in A). 2. From company B, create a new secret and re-point the environment's API-key field at it, then save. 3. The save persists the config but the binding sync throws, so no binding for B's secret exists. 4. Run any agent that uses the environment, or try to save the environment again. **Expected behavior** The save either fully applies (config and bindings consistent) or fully fails. Re-pointing a config secret ref to a secret owned by another company moves the binding with the secret. **Paperclip version** Reproduced on current `master` (also present on recent release images). **Deployment mode** Multi-company server deployment (any mode with more than one company on the instance). ## What Changed - New `secretService.replaceSecretRefsForInstanceTarget`: writes each config-derived binding under the company that owns the referenced secret, replaces all non-`env.*` bindings of the target across every company, and validates every ref (secret exists, not deleted, config-path and projection-class rules) before any row is written. `env.*` env-var bindings stay company-scoped and untouched. - The environment create and update routes now run the environment write and its binding syncs inside one `db.transaction`, threading the transaction through new optional executor seams on `environmentService.create/update` and the existing `SecretBindingDb` seam pattern, so an invalid ref rolls the whole save back instead of leaving a half-updated environment. - `resolveEnvironmentSecretContextCompanyId` no longer lets existing bindings veto the caller's context (the 409s above); it now only picks where new raw-pasted secrets are created and how env-var bindings and probes resolve: explicit route/query company first, then the single company the bindings live in, then the actor's company. ## Verification - `cd server && pnpm vitest run src/__tests__/environment-routes.test.ts src/__tests__/environment-instance-routes.test.ts src/__tests__/secrets-service.test.ts src/__tests__/environment-custom-image-routes.test.ts` (165 tests, includes new coverage below) - New embedded-Postgres tests prove: a re-point moves the binding to the new secret's company and deletes the stale row; refs across several companies each bind under their own secret's company; an unknown secret ref rejects without touching existing bindings; `env.*` rows survive config-ref replacement. - New route tests prove: a cross-company re-point that previously 409'd now saves, with the update and binding replacement on the same transaction executor; a failing ref surfaces as 422. - `cd server && pnpm run typecheck` ## Risks - Behavioral shift: environment saves no longer 409 on a company-context mismatch between the caller and existing bindings; bindings follow the referenced secret's company instead. Environment routes are instance-admin gated, and instance admins already had access to every company's secrets by passing the company explicitly, so this removes an ordering trap rather than widening access. - Runtime lease resolution is unchanged: a run still resolves environment secrets under the run's own company, so an environment referencing company B's secret still only leases for company B runs (fail-closed as before). - The delete route's per-company binding cleanup is unchanged. ## Model Used Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code — extended thinking, agentic tool use (file edits, vitest/tsc runs). No other models involved. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
Devin Foley
GitHub
parent
32c1a8576c
commit
f51cba33fa
|
|
@ -48,6 +48,7 @@ const mockSecretService = vi.hoisted(() => ({
|
|||
resolveSecretValueForEphemeralAccess: vi.fn(),
|
||||
syncEnvBindingsForTarget: vi.fn(),
|
||||
syncSecretRefsForTarget: vi.fn(),
|
||||
replaceSecretRefsForInstanceTarget: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("../services/index.js", () => ({
|
||||
|
|
@ -116,7 +117,9 @@ function createApp(actor: Record<string, unknown>) {
|
|||
(req as typeof req & { actor: Record<string, unknown> }).actor = actor;
|
||||
next();
|
||||
});
|
||||
app.use("/api", environmentRoutes({} as never));
|
||||
app.use("/api", environmentRoutes({
|
||||
transaction: async (fn: (tx: unknown) => Promise<unknown>) => fn({}),
|
||||
} as never));
|
||||
app.use(errorHandler);
|
||||
return app;
|
||||
}
|
||||
|
|
@ -145,6 +148,8 @@ describe("environment instance routes", () => {
|
|||
mockSecretService.resolveSecretValueForEphemeralAccess.mockReset();
|
||||
mockSecretService.syncEnvBindingsForTarget.mockReset();
|
||||
mockSecretService.syncSecretRefsForTarget.mockReset();
|
||||
mockSecretService.replaceSecretRefsForInstanceTarget.mockReset();
|
||||
mockSecretService.replaceSecretRefsForInstanceTarget.mockResolvedValue([]);
|
||||
|
||||
mockInstanceSettingsService.listCompanyIds.mockResolvedValue(["company-1", "company-2"]);
|
||||
mockEnvironmentService.list.mockResolvedValue([]);
|
||||
|
|
@ -252,16 +257,19 @@ describe("environment instance routes", () => {
|
|||
driver: "local",
|
||||
status: "active",
|
||||
}),
|
||||
undefined,
|
||||
{ db: expect.anything() },
|
||||
);
|
||||
expect(mockSecretService.syncSecretRefsForTarget).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
expect(mockSecretService.replaceSecretRefsForInstanceTarget).toHaveBeenCalledWith(
|
||||
{ targetType: "environment", targetId: "env-1" },
|
||||
[],
|
||||
{ db: expect.anything() },
|
||||
);
|
||||
expect(mockSecretService.syncEnvBindingsForTarget).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
{ targetType: "environment", targetId: "env-1" },
|
||||
{},
|
||||
{ db: expect.anything() },
|
||||
);
|
||||
expect(mockLogActivity).toHaveBeenCalledTimes(2);
|
||||
expect(mockLogActivity.mock.calls.map((call) => call[1].companyId)).toEqual(["company-1", "company-2"]);
|
||||
|
|
@ -295,11 +303,16 @@ describe("environment instance routes", () => {
|
|||
envVars,
|
||||
expect.objectContaining({ fieldPath: "envVars" }),
|
||||
);
|
||||
expect(mockEnvironmentService.create).toHaveBeenCalledWith(expect.objectContaining({ envVars }));
|
||||
expect(mockEnvironmentService.create).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ envVars }),
|
||||
undefined,
|
||||
{ db: expect.anything() },
|
||||
);
|
||||
expect(mockSecretService.syncEnvBindingsForTarget).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
{ targetType: "environment", targetId: "env-1" },
|
||||
envVars,
|
||||
{ db: expect.anything() },
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import type { Server } from "node:http";
|
|||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { unprocessable } from "../errors.js";
|
||||
import { environmentRoutes } from "../routes/environments.js";
|
||||
import { errorHandler } from "../middleware/index.js";
|
||||
|
||||
|
|
@ -64,6 +65,7 @@ const mockSecretService = vi.hoisted(() => ({
|
|||
resolveSecretValueForEphemeralAccess: vi.fn(),
|
||||
syncEnvBindingsForTarget: vi.fn(),
|
||||
syncSecretRefsForTarget: vi.fn(),
|
||||
replaceSecretRefsForInstanceTarget: vi.fn(),
|
||||
remove: vi.fn(),
|
||||
}));
|
||||
const mockValidatePluginEnvironmentDriverConfig = vi.hoisted(() => vi.fn());
|
||||
|
|
@ -182,6 +184,14 @@ let currentActor: Record<string, unknown> = {
|
|||
const routeOptions: Record<string, unknown> = {};
|
||||
const originalSecretsProviderEnv = process.env.PAPERCLIP_SECRETS_PROVIDER;
|
||||
|
||||
// The routes open a transaction around environment writes and their binding
|
||||
// syncs. Service calls are mocked, so the executor never runs a real query —
|
||||
// it only needs to be identity-checkable in assertions.
|
||||
const routeDbTx = { __routeDbTx: true };
|
||||
const routeDb = {
|
||||
transaction: async <T>(fn: (tx: unknown) => Promise<T>): Promise<T> => fn(routeDbTx),
|
||||
};
|
||||
|
||||
function createApp(actor: Record<string, unknown>, options: Record<string, unknown> = {}) {
|
||||
currentActor = actor;
|
||||
for (const key of Object.keys(routeOptions)) {
|
||||
|
|
@ -196,7 +206,7 @@ function createApp(actor: Record<string, unknown>, options: Record<string, unkno
|
|||
(req as any).actor = currentActor;
|
||||
next();
|
||||
});
|
||||
app.use("/api", environmentRoutes({} as any, routeOptions as any));
|
||||
app.use("/api", environmentRoutes(routeDb as any, routeOptions as any));
|
||||
app.use(errorHandler);
|
||||
server = app.listen(0);
|
||||
return server;
|
||||
|
|
@ -258,6 +268,7 @@ describe("environment routes", () => {
|
|||
mockSecretService.resolveSecretValueForEphemeralAccess.mockReset();
|
||||
mockSecretService.syncEnvBindingsForTarget.mockReset();
|
||||
mockSecretService.syncSecretRefsForTarget.mockReset();
|
||||
mockSecretService.replaceSecretRefsForInstanceTarget.mockReset();
|
||||
mockSecretService.remove.mockReset();
|
||||
mockSecretService.create.mockResolvedValue({
|
||||
id: "11111111-1111-1111-1111-111111111111",
|
||||
|
|
@ -270,6 +281,7 @@ describe("environment routes", () => {
|
|||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue([]);
|
||||
mockSecretService.syncEnvBindingsForTarget.mockResolvedValue([]);
|
||||
mockSecretService.syncSecretRefsForTarget.mockResolvedValue([]);
|
||||
mockSecretService.replaceSecretRefsForInstanceTarget.mockResolvedValue([]);
|
||||
mockSecretService.remove.mockResolvedValue(null);
|
||||
mockSecretService.resolveSecretValueForEphemeralAccess.mockResolvedValue("resolved-provider-key");
|
||||
delete process.env.PAPERCLIP_SECRETS_PROVIDER;
|
||||
|
|
@ -1039,7 +1051,7 @@ describe("environment routes", () => {
|
|||
status: "active",
|
||||
config: { shell: "zsh" },
|
||||
envVars: {},
|
||||
});
|
||||
}, undefined, { db: routeDbTx });
|
||||
expect(mockLogActivity).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({
|
||||
|
|
@ -1354,7 +1366,7 @@ describe("environment routes", () => {
|
|||
},
|
||||
}),
|
||||
envVars: {},
|
||||
}));
|
||||
}), undefined, { db: routeDbTx });
|
||||
expect(JSON.stringify(mockEnvironmentService.create.mock.calls[0][0])).not.toContain("super-secret-key");
|
||||
expect(mockSecretService.create).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
|
|
@ -1498,7 +1510,7 @@ describe("environment routes", () => {
|
|||
reuseLease: true,
|
||||
},
|
||||
envVars: {},
|
||||
});
|
||||
}, undefined, { db: routeDbTx });
|
||||
expect(mockSecretService.create).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
|
|
@ -1586,7 +1598,7 @@ describe("environment routes", () => {
|
|||
reuseLease: true,
|
||||
},
|
||||
envVars: {},
|
||||
});
|
||||
}, undefined, { db: routeDbTx });
|
||||
expect(JSON.stringify(mockEnvironmentService.create.mock.calls[0][0])).not.toContain("test-provider-key");
|
||||
expect(mockSecretService.create).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
|
|
@ -1667,8 +1679,13 @@ describe("environment routes", () => {
|
|||
reuseLease: true,
|
||||
},
|
||||
envVars: {},
|
||||
});
|
||||
}, undefined, { db: routeDbTx });
|
||||
expect(mockSecretService.create).not.toHaveBeenCalled();
|
||||
expect(mockSecretService.replaceSecretRefsForInstanceTarget).toHaveBeenCalledWith(
|
||||
{ targetType: "environment", targetId: "env-sandbox-secure-plugin" },
|
||||
[{ secretId, configPath: "apiKey", versionSelector: "latest" }],
|
||||
{ db: routeDbTx },
|
||||
);
|
||||
});
|
||||
|
||||
it("uses the configured provider for schema-driven sandbox secret fields", async () => {
|
||||
|
|
@ -1795,7 +1812,7 @@ describe("environment routes", () => {
|
|||
expect(mockEnvironmentService.create).toHaveBeenCalledWith(expect.objectContaining({
|
||||
config: environment.config,
|
||||
envVars: {},
|
||||
}));
|
||||
}), undefined, { db: routeDbTx });
|
||||
});
|
||||
|
||||
it("rejects agent mutations for instance-scoped environments", async () => {
|
||||
|
|
@ -1996,11 +2013,128 @@ describe("environment routes", () => {
|
|||
expect(mockEnvironmentService.update).toHaveBeenCalledWith(environment.id, {
|
||||
driver: "local",
|
||||
config: {},
|
||||
});
|
||||
}, { db: routeDbTx });
|
||||
expect(JSON.stringify(mockEnvironmentService.update.mock.calls[0][1])).not.toContain("super-secret-key");
|
||||
expect(JSON.stringify(mockEnvironmentService.update.mock.calls[0][1])).not.toContain("known-host");
|
||||
});
|
||||
|
||||
it("re-points a sandbox secret ref to another company's secret even when existing bindings disagree", async () => {
|
||||
const oldSecretId = "11111111-1111-1111-1111-111111111111";
|
||||
const newSecretId = "22222222-2222-2222-2222-222222222222";
|
||||
const existing = {
|
||||
...createEnvironment(),
|
||||
id: "env-sandbox",
|
||||
name: "Daytona",
|
||||
driver: "sandbox" as const,
|
||||
config: {
|
||||
provider: "secure-plugin",
|
||||
template: "base",
|
||||
apiKey: oldSecretId,
|
||||
timeoutMs: 450000,
|
||||
reuseLease: true,
|
||||
},
|
||||
};
|
||||
const updated = {
|
||||
...existing,
|
||||
config: { ...existing.config, apiKey: newSecretId },
|
||||
};
|
||||
mockEnvironmentService.getById.mockResolvedValue(existing);
|
||||
mockEnvironmentService.update.mockResolvedValue(updated);
|
||||
mockValidatePluginSandboxProviderConfig.mockImplementation(async ({ config }: { config: Record<string, unknown> }) => ({
|
||||
normalizedConfig: { ...config },
|
||||
pluginId: "plugin-secure",
|
||||
pluginKey: "acme.secure-sandbox-provider",
|
||||
driver: {
|
||||
driverKey: "secure-plugin",
|
||||
kind: "sandbox_provider",
|
||||
displayName: "Secure Sandbox",
|
||||
configSchema: {
|
||||
type: "object",
|
||||
properties: {
|
||||
template: { type: "string" },
|
||||
apiKey: { type: "string", format: "secret-ref" },
|
||||
timeoutMs: { type: "number" },
|
||||
reuseLease: { type: "boolean" },
|
||||
},
|
||||
},
|
||||
},
|
||||
}));
|
||||
// The environment's only binding still lives in another company. Before
|
||||
// bindings moved with the referenced secret, this state made every save
|
||||
// from the caller's company fail, with no route-level way out.
|
||||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue(["company-old"]);
|
||||
const app = createApp({
|
||||
type: "board",
|
||||
userId: "user-1",
|
||||
source: "local_implicit",
|
||||
}, { pluginWorkerManager: {} });
|
||||
|
||||
const res = await request(app)
|
||||
.patch("/api/environments/env-sandbox?companyId=company-new")
|
||||
.send({
|
||||
config: {
|
||||
apiKey: { type: "secret_ref", secretId: newSecretId, version: "latest" },
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockEnvironmentService.update).toHaveBeenCalledWith(
|
||||
"env-sandbox",
|
||||
expect.objectContaining({
|
||||
config: expect.objectContaining({ apiKey: newSecretId }),
|
||||
}),
|
||||
{ db: routeDbTx },
|
||||
);
|
||||
expect(mockSecretService.replaceSecretRefsForInstanceTarget).toHaveBeenCalledWith(
|
||||
{ targetType: "environment", targetId: "env-sandbox" },
|
||||
[{ secretId: newSecretId, configPath: "apiKey", versionSelector: "latest" }],
|
||||
{ db: routeDbTx },
|
||||
);
|
||||
// Explicit caller context wins outright; stale bindings are never consulted.
|
||||
expect(mockSecretService.listBindingCompanyIdsForTarget).not.toHaveBeenCalled();
|
||||
expect(mockSecretService.syncSecretRefsForTarget).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("fails the whole save when a referenced secret cannot be bound", async () => {
|
||||
const existing = {
|
||||
...createEnvironment(),
|
||||
id: "env-sandbox",
|
||||
name: "Daytona",
|
||||
driver: "sandbox" as const,
|
||||
config: {
|
||||
provider: "secure-plugin",
|
||||
template: "base",
|
||||
apiKey: "11111111-1111-1111-1111-111111111111",
|
||||
timeoutMs: 450000,
|
||||
reuseLease: true,
|
||||
},
|
||||
};
|
||||
mockEnvironmentService.getById.mockResolvedValue(existing);
|
||||
mockEnvironmentService.update.mockResolvedValue({
|
||||
...existing,
|
||||
config: { ...existing.config, apiKey: "33333333-3333-3333-3333-333333333333" },
|
||||
});
|
||||
mockSecretService.replaceSecretRefsForInstanceTarget.mockRejectedValue(
|
||||
unprocessable("Secret referenced at apiKey was not found", { code: "secret_missing" }),
|
||||
);
|
||||
const app = createApp({
|
||||
type: "board",
|
||||
userId: "user-1",
|
||||
source: "local_implicit",
|
||||
}, { pluginWorkerManager: {} });
|
||||
|
||||
const res = await request(app)
|
||||
.patch("/api/environments/env-sandbox?companyId=company-1")
|
||||
.send({
|
||||
config: {
|
||||
apiKey: { type: "secret_ref", secretId: "33333333-3333-3333-3333-333333333333", version: "latest" },
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.status).toBe(422);
|
||||
expect(res.body.error).toContain("was not found");
|
||||
});
|
||||
|
||||
it("requires explicit SSH config when switching from local to SSH", async () => {
|
||||
mockEnvironmentService.getById.mockResolvedValue(createEnvironment());
|
||||
const app = createApp({
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import { mkdirSync, rmSync } from "node:fs";
|
|||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { and, eq } from "drizzle-orm";
|
||||
import {
|
||||
activityLog,
|
||||
agents,
|
||||
|
|
@ -148,6 +148,144 @@ describeEmbeddedPostgres("secretService", () => {
|
|||
).rejects.toThrow(/same company/i);
|
||||
});
|
||||
|
||||
it("replaceSecretRefsForInstanceTarget moves the binding to the referenced secret's company", async () => {
|
||||
const companyA = await seedCompany("A");
|
||||
const companyB = await seedCompany("B");
|
||||
const svc = secretService(db);
|
||||
const secretA = await svc.create(companyA, {
|
||||
name: `provider-key-a-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "a",
|
||||
});
|
||||
const secretB = await svc.create(companyB, {
|
||||
name: `provider-key-b-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "b",
|
||||
});
|
||||
const envSecretA = await svc.create(companyA, {
|
||||
name: `env-var-a-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "env",
|
||||
});
|
||||
const environmentId = randomUUID();
|
||||
await svc.createBinding({
|
||||
companyId: companyA,
|
||||
secretId: secretA.id,
|
||||
targetType: "environment",
|
||||
targetId: environmentId,
|
||||
configPath: "apiKey",
|
||||
});
|
||||
// A company-scoped env-var binding on the same environment target must
|
||||
// survive config-ref replacement untouched.
|
||||
await db.insert(companySecretBindings).values({
|
||||
companyId: companyA,
|
||||
secretId: envSecretA.id,
|
||||
targetType: "environment",
|
||||
targetId: environmentId,
|
||||
configPath: "env.EXTRA",
|
||||
versionSelector: "latest",
|
||||
required: true,
|
||||
projectionClass: "unclassified",
|
||||
});
|
||||
|
||||
await svc.replaceSecretRefsForInstanceTarget(
|
||||
{ targetType: "environment", targetId: environmentId },
|
||||
[{ secretId: secretB.id, configPath: "apiKey" }],
|
||||
);
|
||||
|
||||
const rows = await db
|
||||
.select()
|
||||
.from(companySecretBindings)
|
||||
.where(
|
||||
and(
|
||||
eq(companySecretBindings.targetType, "environment"),
|
||||
eq(companySecretBindings.targetId, environmentId),
|
||||
),
|
||||
);
|
||||
const configRows = rows.filter((row) => row.configPath === "apiKey");
|
||||
expect(configRows).toHaveLength(1);
|
||||
expect(configRows[0]?.companyId).toBe(companyB);
|
||||
expect(configRows[0]?.secretId).toBe(secretB.id);
|
||||
expect(rows.filter((row) => row.configPath === "env.EXTRA")).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("replaceSecretRefsForInstanceTarget writes each binding under its own secret's company", async () => {
|
||||
const companyA = await seedCompany("A");
|
||||
const companyB = await seedCompany("B");
|
||||
const svc = secretService(db);
|
||||
const secretA = await svc.create(companyA, {
|
||||
name: `api-key-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "a",
|
||||
});
|
||||
const secretB = await svc.create(companyB, {
|
||||
name: `ssh-key-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "b",
|
||||
});
|
||||
const environmentId = randomUUID();
|
||||
|
||||
const refs = await svc.replaceSecretRefsForInstanceTarget(
|
||||
{ targetType: "environment", targetId: environmentId },
|
||||
[
|
||||
{ secretId: secretA.id, configPath: "apiKey" },
|
||||
{ secretId: secretB.id, configPath: "privateKeySecretRef" },
|
||||
],
|
||||
);
|
||||
|
||||
expect(refs.map((ref) => ref.companyId).sort()).toEqual([companyA, companyB].sort());
|
||||
const rows = await db
|
||||
.select()
|
||||
.from(companySecretBindings)
|
||||
.where(
|
||||
and(
|
||||
eq(companySecretBindings.targetType, "environment"),
|
||||
eq(companySecretBindings.targetId, environmentId),
|
||||
),
|
||||
);
|
||||
expect(rows).toHaveLength(2);
|
||||
expect(rows.find((row) => row.configPath === "apiKey")?.companyId).toBe(companyA);
|
||||
expect(rows.find((row) => row.configPath === "privateKeySecretRef")?.companyId).toBe(companyB);
|
||||
});
|
||||
|
||||
it("replaceSecretRefsForInstanceTarget rejects unknown secrets without touching existing bindings", async () => {
|
||||
const companyA = await seedCompany("A");
|
||||
const svc = secretService(db);
|
||||
const secretA = await svc.create(companyA, {
|
||||
name: `provider-key-${randomUUID()}`,
|
||||
provider: "local_encrypted",
|
||||
value: "a",
|
||||
});
|
||||
const environmentId = randomUUID();
|
||||
await svc.createBinding({
|
||||
companyId: companyA,
|
||||
secretId: secretA.id,
|
||||
targetType: "environment",
|
||||
targetId: environmentId,
|
||||
configPath: "apiKey",
|
||||
});
|
||||
|
||||
await expect(
|
||||
svc.replaceSecretRefsForInstanceTarget(
|
||||
{ targetType: "environment", targetId: environmentId },
|
||||
[{ secretId: randomUUID(), configPath: "apiKey" }],
|
||||
),
|
||||
).rejects.toThrow(/was not found/i);
|
||||
|
||||
const rows = await db
|
||||
.select()
|
||||
.from(companySecretBindings)
|
||||
.where(
|
||||
and(
|
||||
eq(companySecretBindings.targetType, "environment"),
|
||||
eq(companySecretBindings.targetId, environmentId),
|
||||
),
|
||||
);
|
||||
expect(rows).toHaveLength(1);
|
||||
expect(rows[0]?.secretId).toBe(secretA.id);
|
||||
expect(rows[0]?.companyId).toBe(companyA);
|
||||
});
|
||||
|
||||
it("prevents duplicate bindings for a target config path", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const svc = secretService(db);
|
||||
|
|
|
|||
|
|
@ -429,6 +429,17 @@ export function environmentRoutes(
|
|||
return await resolveCustomImageCompanyId(req);
|
||||
}
|
||||
|
||||
/**
|
||||
* Pick the company context used to create new secrets from raw-pasted
|
||||
* values, normalize env-var bindings, and resolve probe secrets. An
|
||||
* explicit route param / query wins, then the single company the
|
||||
* environment's bindings already live in, then the actor's own company.
|
||||
* Bindings must never veto an explicit caller context: config-derived
|
||||
* bindings live in the company that owns each referenced secret (see
|
||||
* `replaceSecretRefsForInstanceTarget`), so an environment's bindings may
|
||||
* legitimately sit in a different company — or several — than the board
|
||||
* that is editing it.
|
||||
*/
|
||||
async function resolveEnvironmentSecretContextCompanyId(
|
||||
req: Request,
|
||||
environmentId: string,
|
||||
|
|
@ -440,18 +451,12 @@ export function environmentRoutes(
|
|||
: typeof req.query.companyId === "string" && req.query.companyId.trim().length > 0
|
||||
? req.query.companyId.trim()
|
||||
: null;
|
||||
if (routeCompanyId) return routeCompanyId;
|
||||
const bindingCompanyIds = await secrets.listBindingCompanyIdsForTarget({
|
||||
targetType: "environment",
|
||||
targetId: environmentId,
|
||||
});
|
||||
if (routeCompanyId && bindingCompanyIds.length > 0 && !bindingCompanyIds.includes(routeCompanyId)) {
|
||||
throw conflict("Environment secret bindings already use a different company context.");
|
||||
}
|
||||
if (routeCompanyId) return routeCompanyId;
|
||||
if (bindingCompanyIds.length === 1) return bindingCompanyIds[0] ?? null;
|
||||
if (bindingCompanyIds.length > 1) {
|
||||
throw conflict("Environment secret bindings span multiple companies and require explicit companyId context.");
|
||||
}
|
||||
if (req.actor.type === "agent" && req.actor.companyId) return req.actor.companyId;
|
||||
if (req.actor.type === "board" && Array.isArray(req.actor.companyIds) && req.actor.companyIds.length === 1) {
|
||||
return req.actor.companyIds[0] ?? null;
|
||||
|
|
@ -887,17 +892,23 @@ export function environmentRoutes(
|
|||
pluginWorkerManager: options.pluginWorkerManager,
|
||||
}),
|
||||
};
|
||||
const environment = await svc.create(input);
|
||||
await secrets.syncSecretRefsForTarget(
|
||||
companyId,
|
||||
{ targetType: "environment", targetId: environment.id },
|
||||
await collectEnvironmentSecretRefs({ db, environment }),
|
||||
);
|
||||
await secrets.syncEnvBindingsForTarget(
|
||||
companyId,
|
||||
{ targetType: "environment", targetId: environment.id },
|
||||
environment.envVars,
|
||||
);
|
||||
// Create the row and its binding rows atomically so an invalid secret
|
||||
// ref cannot leave an environment persisted without its bindings.
|
||||
const environment = await db.transaction(async (tx) => {
|
||||
const created = await svc.create(input, undefined, { db: tx });
|
||||
await secrets.replaceSecretRefsForInstanceTarget(
|
||||
{ targetType: "environment", targetId: created.id },
|
||||
await collectEnvironmentSecretRefs({ db, environment: created }),
|
||||
{ db: tx },
|
||||
);
|
||||
await secrets.syncEnvBindingsForTarget(
|
||||
companyId,
|
||||
{ targetType: "environment", targetId: created.id },
|
||||
created.envVars,
|
||||
{ db: tx },
|
||||
);
|
||||
return created;
|
||||
});
|
||||
await logInstanceEnvironmentActivity({
|
||||
actor,
|
||||
action: "environment.created",
|
||||
|
|
@ -1006,7 +1017,29 @@ export function environmentRoutes(
|
|||
}
|
||||
: {}),
|
||||
};
|
||||
const environment = await svc.update(existing.id, patch);
|
||||
// Persist the config change and its binding rows atomically: a binding
|
||||
// ref that fails validation (e.g. a deleted secret) must roll the whole
|
||||
// save back instead of leaving the config re-pointed with stale bindings.
|
||||
const environment = await db.transaction(async (tx) => {
|
||||
const updated = await svc.update(existing.id, patch, { db: tx });
|
||||
if (!updated) return null;
|
||||
if (patch.config !== undefined || patch.driver !== undefined) {
|
||||
await secrets.replaceSecretRefsForInstanceTarget(
|
||||
{ targetType: "environment", targetId: updated.id },
|
||||
await collectEnvironmentSecretRefs({ db, environment: updated }),
|
||||
{ db: tx },
|
||||
);
|
||||
}
|
||||
if (patch.envVars !== undefined) {
|
||||
await secrets.syncEnvBindingsForTarget(
|
||||
companyIdForSecrets!,
|
||||
{ targetType: "environment", targetId: updated.id },
|
||||
updated.envVars,
|
||||
{ db: tx },
|
||||
);
|
||||
}
|
||||
return updated;
|
||||
});
|
||||
if (!environment) {
|
||||
res.status(404).json({ error: "Environment not found" });
|
||||
return;
|
||||
|
|
@ -1015,11 +1048,6 @@ export function environmentRoutes(
|
|||
ReturnType<typeof customImages.reconcileActiveTemplateForConfigChange>
|
||||
> = { action: "none" };
|
||||
if (patch.config !== undefined || patch.driver !== undefined) {
|
||||
await secrets.syncSecretRefsForTarget(
|
||||
companyIdForSecrets!,
|
||||
{ targetType: "environment", targetId: environment.id },
|
||||
await collectEnvironmentSecretRefs({ db, environment }),
|
||||
);
|
||||
try {
|
||||
customImageReconciliation = await customImages.reconcileActiveTemplateForConfigChange({
|
||||
environmentId: environment.id,
|
||||
|
|
@ -1030,13 +1058,6 @@ export function environmentRoutes(
|
|||
// Reconciliation is best-effort; a failure must not fail the save.
|
||||
}
|
||||
}
|
||||
if (patch.envVars !== undefined) {
|
||||
await secrets.syncEnvBindingsForTarget(
|
||||
companyIdForSecrets!,
|
||||
{ targetType: "environment", targetId: environment.id },
|
||||
environment.envVars,
|
||||
);
|
||||
}
|
||||
await logInstanceEnvironmentActivity({
|
||||
actor,
|
||||
action: "environment.updated",
|
||||
|
|
|
|||
|
|
@ -201,6 +201,9 @@ function countFromRows(rows: Array<{ count: number | string | null | undefined }
|
|||
return Number(rows[0]?.count ?? 0);
|
||||
}
|
||||
|
||||
type DbTransaction = Parameters<Parameters<Db["transaction"]>[0]>[0];
|
||||
type EnvironmentWriteDb = Pick<Db | DbTransaction, "select" | "insert" | "update" | "delete">;
|
||||
|
||||
export function environmentService(db: Db) {
|
||||
/** The single Paperclip-managed sandbox row (`environments_managed_sandbox_idx`), if present. */
|
||||
const findManagedSandboxRow = () =>
|
||||
|
|
@ -528,10 +531,11 @@ export function environmentService(db: Db) {
|
|||
create: async (
|
||||
companyIdOrInput: string | CreateEnvironment,
|
||||
maybeInput?: CreateEnvironment,
|
||||
options?: { db?: EnvironmentWriteDb },
|
||||
): Promise<Environment> => {
|
||||
const input = resolveCreateInput(companyIdOrInput, maybeInput);
|
||||
const now = new Date();
|
||||
const row = await db
|
||||
const row = await (options?.db ?? db)
|
||||
.insert(environments)
|
||||
.values({
|
||||
name: input.name,
|
||||
|
|
@ -561,7 +565,11 @@ export function environmentService(db: Db) {
|
|||
return toEnvironment(row);
|
||||
},
|
||||
|
||||
update: async (id: string, patch: UpdateEnvironment): Promise<Environment | null> => {
|
||||
update: async (
|
||||
id: string,
|
||||
patch: UpdateEnvironment,
|
||||
options?: { db?: EnvironmentWriteDb },
|
||||
): Promise<Environment | null> => {
|
||||
const values: Partial<typeof environments.$inferInsert> = {
|
||||
updatedAt: new Date(),
|
||||
};
|
||||
|
|
@ -575,7 +583,7 @@ export function environmentService(db: Db) {
|
|||
}
|
||||
if (patch.metadata !== undefined) values.metadata = patch.metadata ?? null;
|
||||
|
||||
const row = await db
|
||||
const row = await (options?.db ?? db)
|
||||
.update(environments)
|
||||
.set(values)
|
||||
.where(eq(environments.id, id))
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { randomUUID } from "node:crypto";
|
||||
import { and, desc, eq, inArray, like, ne, notInArray, or, sql } from "drizzle-orm";
|
||||
import { and, desc, eq, inArray, like, ne, notInArray, notLike, or, sql } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import {
|
||||
agents,
|
||||
|
|
@ -4015,6 +4015,111 @@ export function secretService(db: Db) {
|
|||
return normalizedRefs;
|
||||
},
|
||||
|
||||
/**
|
||||
* Replace the config-derived secret bindings of an instance-scoped target
|
||||
* (an environment). Instance-scoped targets are shared across companies,
|
||||
* so each binding is written under the company that owns the referenced
|
||||
* secret rather than a single caller-supplied company context — a
|
||||
* re-point to a secret owned by another company moves the binding with
|
||||
* it. All non-`env.*` bindings of the target are replaced across every
|
||||
* company (env-var bindings stay company-scoped and are managed by
|
||||
* `syncEnvBindingsForTarget`).
|
||||
*
|
||||
* Every referenced secret is loaded and validated before any row is
|
||||
* written, and the delete + insert run on one executor, so an invalid
|
||||
* ref (deleted or unknown secret) fails the whole call without leaving
|
||||
* the target half-bound.
|
||||
*/
|
||||
replaceSecretRefsForInstanceTarget: async (
|
||||
target: { targetType: SecretBindingTargetType; targetId: string },
|
||||
refs: Array<{
|
||||
secretId: string;
|
||||
configPath: string;
|
||||
versionSelector?: SecretVersionSelector;
|
||||
required?: boolean;
|
||||
label?: string | null;
|
||||
projectionClass?: SecretProjectionClass;
|
||||
projectionAllowlistKey?: string | null;
|
||||
}>,
|
||||
options?: { db?: SecretBindingDb },
|
||||
) => {
|
||||
const normalizedRefs: Array<{
|
||||
companyId: string;
|
||||
secretId: string;
|
||||
configPath: string;
|
||||
versionSelector: SecretVersionSelector;
|
||||
required: boolean;
|
||||
label: string | null;
|
||||
projectionClass: SecretProjectionClass;
|
||||
projectionAllowlistKey: string | null;
|
||||
}> = [];
|
||||
const readDb = options?.db ?? db;
|
||||
for (const ref of refs) {
|
||||
const secret = await getById(ref.secretId, readDb);
|
||||
if (!secret || secret.status === "deleted") {
|
||||
throw unprocessable(
|
||||
`Secret referenced at ${ref.configPath} was not found`,
|
||||
{ code: "secret_missing", configPath: ref.configPath },
|
||||
);
|
||||
}
|
||||
assertSecretBindingConfigPath({ targetType: target.targetType, configPath: ref.configPath });
|
||||
const projectionClass = ref.projectionClass ?? "unclassified";
|
||||
const projectionAllowlistKey = ref.projectionAllowlistKey ?? null;
|
||||
assertClass3StaticLeaseAllowed({
|
||||
targetType: target.targetType,
|
||||
configPath: ref.configPath,
|
||||
projectionClass,
|
||||
projectionAllowlistKey,
|
||||
});
|
||||
normalizedRefs.push({
|
||||
companyId: secret.companyId,
|
||||
secretId: ref.secretId,
|
||||
configPath: ref.configPath,
|
||||
versionSelector: ref.versionSelector ?? "latest",
|
||||
required: ref.required ?? true,
|
||||
label: ref.label ?? null,
|
||||
projectionClass,
|
||||
projectionAllowlistKey,
|
||||
});
|
||||
}
|
||||
|
||||
const writeBindings = async (executor: SecretBindingDb) => {
|
||||
await executor
|
||||
.delete(companySecretBindings)
|
||||
.where(
|
||||
and(
|
||||
eq(companySecretBindings.targetType, target.targetType),
|
||||
eq(companySecretBindings.targetId, target.targetId),
|
||||
notLike(companySecretBindings.configPath, "env.%"),
|
||||
),
|
||||
);
|
||||
if (normalizedRefs.length === 0) return;
|
||||
await executor.insert(companySecretBindings).values(
|
||||
normalizedRefs.map((ref) => ({
|
||||
companyId: ref.companyId,
|
||||
secretId: ref.secretId,
|
||||
targetType: target.targetType,
|
||||
targetId: target.targetId,
|
||||
configPath: ref.configPath,
|
||||
versionSelector: String(ref.versionSelector),
|
||||
required: ref.required,
|
||||
label: ref.label,
|
||||
projectionClass: ref.projectionClass,
|
||||
projectionAllowlistKey: ref.projectionAllowlistKey,
|
||||
})),
|
||||
);
|
||||
};
|
||||
|
||||
if (options?.db) {
|
||||
await writeBindings(options.db);
|
||||
} else {
|
||||
await db.transaction(async (tx) => {
|
||||
await writeBindings(tx);
|
||||
});
|
||||
}
|
||||
return normalizedRefs;
|
||||
},
|
||||
|
||||
listBindingCompanyIdsForTarget: async (
|
||||
target: { targetType: SecretBindingTargetType; targetId: string },
|
||||
): Promise<string[]> =>
|
||||
|
|
|
|||
Loading…
Reference in New Issue