Resolve the environment secret companyId context on first save (#11291)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Environments give agent runs an execution target, and their env vars can carry secrets, so writes need a company scope for secret bindings > - `PATCH /environments/:id` resolves that scope from a query param, existing bindings, or a single-membership actor — and fails closed otherwise > - A fresh environment has no bindings yet, so an admin whose memberships cannot pin one company gets a 422 on the very first env var save and can never create the first binding > - #11200 made this reachable from the UI: it opened the envVars-only edit on the managed sandbox row, but the UI never sends the company it already knows > - This pull request sends the page's selected company on environment updates and adds a server fallback to the instance's only company > - The benefit is that the first env var save works on fresh environments, while multi-company ambiguity still fails closed ## Linked Issues or Issue Description Refs #11200 **What happened?** On a fresh platform-managed sandbox environment, the first "Save environment variables" in the UI fails with HTTP 422: "Environment secret management requires a companyId context during the instance-scoped transition." The same failure hits any environment update that touches `envVars` or `config` when the environment has no secret bindings yet and the actor's memberships do not name exactly one company (for example an instance admin provisioned without a membership row, or a user in two companies). **Expected behavior** The save succeeds. The client tells the server which company scopes the secret bindings, and on a single-company instance the server can resolve the only possible scope by itself. **Steps to reproduce** 1. Provision a platform-managed sandbox environment (managed-config `environments` entry) so the row exists with no secret bindings. 2. Sign in as an instance admin whose memberships do not resolve to exactly one company. 3. Open Environments, edit the managed row, add an env var, and save. 4. The PATCH returns 422 with the companyId-context error. ## What Changed - `ui/src/api/environments.ts`: `environmentsApi.update` accepts an optional `companyId` and sends it as the `companyId` query param the server already reads. Renamed the `customImageCompanyQuery` helper to `companyIdQuery` since it now serves plain updates and probes too. - `ui/src/pages/CompanyEnvironments.tsx`: both the managed envVars-only save and the general environment save pass `selectedCompanyId`. - `server/src/routes/environments.ts`: `resolveEnvironmentSecretContextCompanyId` falls back to the instance's only company when exactly one exists, mirroring the existing fallback in `resolveCustomImageCompanyId`. Multi-company instances still fail closed. - Tests: three new route tests (explicit query context for a multi-company actor, single-company-instance fallback for a membership-less admin, fail-closed regression on a multi-company instance), and updated the SSH-probe and probe-ambiguity tests for the new fallback. ## Verification - `cd server && pnpm vitest run src/__tests__/environment-routes.test.ts` — 78 pass. - `cd ui && pnpm vitest run src/pages/CompanyEnvironments.test.tsx` — 22 pass. - Full `server` and `ui` vitest suites and `tsc --noEmit` on both packages pass locally. - Manual: on a managed instance, edit the managed sandbox environment, add an env var, save. Before: 422. After: the save persists and the PATCH carries `?companyId=`. ## Risks - Low risk. The explicit query param path already existed on the server; the UI now uses it. - Behavioral shift: on single-company instances, environment secret-context resolution (including `POST /environments/:id/probe`) now resolves the only company instead of returning no context. That lets probes resolve secret-backed config where they previously returned a 422 asking for an explicit companyId. Multi-company instances keep the fail-closed behavior, covered by a regression test. - Self-hosted single-company instances gain the same first-save fix; no schema or config changes. ## Model Used Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code — extended thinking, agentic tool use. ## 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
f1931d0e14
commit
ff5fd62d07
|
|
@ -552,6 +552,81 @@ describe("environment routes", () => {
|
|||
expect(res.body.envVars).toEqual({ MY_AGENT_TOOL_SETTING: "updated-value", EXTRA: "added" });
|
||||
});
|
||||
|
||||
it("scopes the envVars patch to an explicit companyId query for a multi-company actor", async () => {
|
||||
const row = createPlatformSandboxEnvironment();
|
||||
mockEnvironmentService.getById.mockResolvedValue(row);
|
||||
mockEnvironmentService.update.mockResolvedValue({ ...row, envVars: { A: "1" } });
|
||||
// No prior bindings and two memberships: neither inference path can
|
||||
// pin a company, so the explicit query context must carry the save.
|
||||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue([]);
|
||||
const app = createApp({
|
||||
...ownerAdminActor,
|
||||
companyIds: ["company-1", "company-2"],
|
||||
memberships: [
|
||||
{ companyId: "company-1", status: "active", membershipRole: "owner" },
|
||||
{ companyId: "company-2", status: "active", membershipRole: "member" },
|
||||
],
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.patch("/api/environments/env-managed-1?companyId=company-1")
|
||||
.send({ envVars: { A: "1" } });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockSecretService.normalizeEnvBindingsForPersistence).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
{ A: "1" },
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to the instance's only company when the actor's memberships cannot pin one", async () => {
|
||||
const row = createPlatformSandboxEnvironment();
|
||||
mockEnvironmentService.getById.mockResolvedValue(row);
|
||||
mockEnvironmentService.update.mockResolvedValue({ ...row, envVars: { A: "1" } });
|
||||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue([]);
|
||||
// An instance admin provisioned without membership rows — the
|
||||
// owner-as-admin shape on managed stacks.
|
||||
const app = createApp({
|
||||
...ownerAdminActor,
|
||||
companyIds: [],
|
||||
memberships: [],
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.patch("/api/environments/env-managed-1")
|
||||
.send({ envVars: { A: "1" } });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockSecretService.normalizeEnvBindingsForPersistence).toHaveBeenCalledWith(
|
||||
"company-1",
|
||||
{ A: "1" },
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it("still fails closed when no companyId context is resolvable on a multi-company instance", async () => {
|
||||
mockEnvironmentService.getById.mockResolvedValue(createPlatformSandboxEnvironment());
|
||||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue([]);
|
||||
mockInstanceSettingsService.listCompanyIds.mockResolvedValue(["company-1", "company-2"]);
|
||||
const app = createApp({
|
||||
...ownerAdminActor,
|
||||
companyIds: ["company-1", "company-2"],
|
||||
memberships: [
|
||||
{ companyId: "company-1", status: "active", membershipRole: "owner" },
|
||||
{ companyId: "company-2", status: "active", membershipRole: "member" },
|
||||
],
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.patch("/api/environments/env-managed-1")
|
||||
.send({ envVars: { A: "1" } });
|
||||
|
||||
expect(res.status).toBe(422);
|
||||
expect(res.body.error).toContain("requires a companyId context");
|
||||
expect(mockEnvironmentService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects a patch that mixes envVars with any other field on the managed sandbox row", async () => {
|
||||
mockEnvironmentService.getById.mockResolvedValue(createPlatformSandboxEnvironment());
|
||||
const app = createApp(ownerAdminActor);
|
||||
|
|
@ -2497,7 +2572,9 @@ describe("environment routes", () => {
|
|||
expect(res.status).toBe(200);
|
||||
expect(res.body.ok).toBe(true);
|
||||
expect(mockProbeEnvironment).toHaveBeenCalledWith(expect.anything(), environment, {
|
||||
companyId: null,
|
||||
// The instance has exactly one company, so the secret-context fallback
|
||||
// resolves it even though the actor carries no memberships.
|
||||
companyId: "company-1",
|
||||
pluginWorkerManager: undefined,
|
||||
applyCustomImageTemplate: false,
|
||||
acquireSandboxRuntimeLease: false,
|
||||
|
|
@ -2538,6 +2615,9 @@ describe("environment routes", () => {
|
|||
};
|
||||
mockEnvironmentService.getById.mockResolvedValue(environment);
|
||||
mockSecretService.listBindingCompanyIdsForTarget.mockResolvedValue([]);
|
||||
// A multi-company instance keeps the context genuinely ambiguous — a
|
||||
// single-company instance would resolve via the instance fallback.
|
||||
mockInstanceSettingsService.listCompanyIds.mockResolvedValue(["company-1", "company-2"]);
|
||||
const app = createApp({
|
||||
type: "board",
|
||||
userId: "user-1",
|
||||
|
|
|
|||
|
|
@ -498,7 +498,8 @@ export function environmentRoutes(
|
|||
* 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.
|
||||
* environment's bindings already live in, then the actor's own company,
|
||||
* then the instance's only company (when exactly one exists).
|
||||
* 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
|
||||
|
|
@ -526,6 +527,14 @@ export function environmentRoutes(
|
|||
if (req.actor.type === "board" && Array.isArray(req.actor.companyIds) && req.actor.companyIds.length === 1) {
|
||||
return req.actor.companyIds[0] ?? null;
|
||||
}
|
||||
// Single-company instances have exactly one possible secret scope, so an
|
||||
// actor whose memberships cannot pin a company (none, or several — e.g. an
|
||||
// instance admin provisioned without a membership row) still resolves.
|
||||
// Mirrors the fallback in `resolveCustomImageCompanyId`.
|
||||
const instanceCompanyIds = await instanceSettings.listCompanyIds();
|
||||
if (instanceCompanyIds.length === 1 && instanceCompanyIds[0]) {
|
||||
return instanceCompanyIds[0];
|
||||
}
|
||||
if (!options.required) return null;
|
||||
throw unprocessable(
|
||||
"Environment secret management requires a companyId context during the instance-scoped transition.",
|
||||
|
|
|
|||
|
|
@ -54,7 +54,7 @@ export interface EnvironmentCustomImageRollbackResult {
|
|||
supersededTemplate: EnvironmentCustomImageTemplate;
|
||||
}
|
||||
|
||||
function customImageCompanyQuery(companyId: string): string {
|
||||
function companyIdQuery(companyId: string): string {
|
||||
return `companyId=${encodeURIComponent(companyId)}`;
|
||||
}
|
||||
|
||||
|
|
@ -91,11 +91,21 @@ export const environmentsApi = {
|
|||
// write floor admits envVars-only patches there).
|
||||
envVars?: Environment["envVars"];
|
||||
metadata?: Record<string, unknown> | null;
|
||||
}) => api.patch<EnvironmentUpdateResult>(`/environments/${environmentId}`, body),
|
||||
// Secret-context company for env var / config writes. Without it the
|
||||
// server can only infer a company from existing bindings or a
|
||||
// single-membership actor, and fails closed otherwise — a fresh
|
||||
// environment with no bindings needs the explicit context.
|
||||
}, companyId?: string | null) =>
|
||||
api.patch<EnvironmentUpdateResult>(
|
||||
companyId
|
||||
? `/environments/${environmentId}?${companyIdQuery(companyId)}`
|
||||
: `/environments/${environmentId}`,
|
||||
body,
|
||||
),
|
||||
probe: (environmentId: string, companyId?: string | null) =>
|
||||
api.post<EnvironmentProbeResult>(
|
||||
companyId
|
||||
? `/environments/${environmentId}/probe?${customImageCompanyQuery(companyId)}`
|
||||
? `/environments/${environmentId}/probe?${companyIdQuery(companyId)}`
|
||||
: `/environments/${environmentId}/probe`,
|
||||
{},
|
||||
),
|
||||
|
|
@ -108,7 +118,7 @@ export const environmentsApi = {
|
|||
}) => api.post<EnvironmentProbeResult>(`/companies/${companyId}/environments/probe-config`, body),
|
||||
customImageTemplate: (environmentId: string, companyId: string) =>
|
||||
api.get<EnvironmentCustomImageOverview>(
|
||||
`/environments/${environmentId}/custom-image-template?${customImageCompanyQuery(companyId)}`,
|
||||
`/environments/${environmentId}/custom-image-template?${companyIdQuery(companyId)}`,
|
||||
),
|
||||
startCustomImageSetupSession: (
|
||||
environmentId: string,
|
||||
|
|
@ -116,7 +126,7 @@ export const environmentsApi = {
|
|||
body: StartEnvironmentCustomImageSetupSession = {},
|
||||
) =>
|
||||
api.post<EnvironmentCustomImageSetupSessionResult>(
|
||||
`/environments/${environmentId}/custom-image-setup-sessions?${customImageCompanyQuery(companyId)}`,
|
||||
`/environments/${environmentId}/custom-image-setup-sessions?${companyIdQuery(companyId)}`,
|
||||
body,
|
||||
),
|
||||
customImageSetupSession: (sessionId: string) =>
|
||||
|
|
@ -149,7 +159,7 @@ export const environmentsApi = {
|
|||
),
|
||||
rollbackCustomImageTemplate: (environmentId: string, companyId: string) =>
|
||||
api.post<EnvironmentCustomImageRollbackResult>(
|
||||
`/environments/${environmentId}/custom-image-template/rollback?${customImageCompanyQuery(companyId)}`,
|
||||
`/environments/${environmentId}/custom-image-template/rollback?${companyIdQuery(companyId)}`,
|
||||
{},
|
||||
),
|
||||
disableCustomImageTemplate: (
|
||||
|
|
@ -158,6 +168,6 @@ export const environmentsApi = {
|
|||
options: { deleteProviderTemplate?: boolean } = {},
|
||||
) =>
|
||||
api.delete<EnvironmentCustomImageTemplate>(
|
||||
`/environments/${environmentId}/custom-image-template?${customImageCompanyQuery(companyId)}&deleteProviderTemplate=${options.deleteProviderTemplate === true ? "true" : "false"}`,
|
||||
`/environments/${environmentId}/custom-image-template?${companyIdQuery(companyId)}&deleteProviderTemplate=${options.deleteProviderTemplate === true ? "true" : "false"}`,
|
||||
),
|
||||
};
|
||||
|
|
|
|||
|
|
@ -685,6 +685,9 @@ describe("CompanyEnvironments — test provider button", () => {
|
|||
driver: "sandbox",
|
||||
envVars: { API_TOKEN: { type: "plain", value: "draft-token" } },
|
||||
}),
|
||||
// The secret-context company must ride along so the server can scope
|
||||
// bindings even when the environment has none yet.
|
||||
"company-1",
|
||||
);
|
||||
expect(getEnvironmentFormPage()).toBeNull();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1258,7 +1258,7 @@ export function CompanyEnvironments({ mode = "list" }: CompanyEnvironmentsProps)
|
|||
const managedEnvironmentEnvVarsMutation = useMutation({
|
||||
mutationFn: async (envVars: EnvironmentFormState["envVars"]) => {
|
||||
if (!editingEnvironmentId) throw new Error("No environment selected");
|
||||
return await environmentsApi.update(editingEnvironmentId, { envVars });
|
||||
return await environmentsApi.update(editingEnvironmentId, { envVars }, selectedCompanyId);
|
||||
},
|
||||
onSuccess: async (environment) => {
|
||||
if (selectedCompanyId) {
|
||||
|
|
@ -1291,7 +1291,7 @@ export function CompanyEnvironments({ mode = "list" }: CompanyEnvironmentsProps)
|
|||
const body = buildEnvironmentPayload(form);
|
||||
|
||||
if (editingEnvironmentId) {
|
||||
return await environmentsApi.update(editingEnvironmentId, body);
|
||||
return await environmentsApi.update(editingEnvironmentId, body, selectedCompanyId);
|
||||
}
|
||||
|
||||
if (!selectedCompanyId) throw new Error("Select a company to create environments");
|
||||
|
|
|
|||
Loading…
Reference in New Issue