From 90ead239a81215e673446608e7c7c63a5cc4e525 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 31 Jul 2026 16:46:26 -0700 Subject: [PATCH] feat(ui/server): name cross-company environment secret refs instead of calling them missing (#10577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Environment configs (sandbox providers, SSH) can bind stored company secrets through `format: "secret-ref"` fields, picked in the environment editor's secret picker > - Environments are instance-scoped and shared by every company on an instance, but the picker lists only the current company's secrets, so a ref pointing at another company's secret renders as "Missing secret (…)" in destructive styling > - That state is indistinguishable from a genuinely deleted secret, so operators "fix" a healthy binding by creating a duplicate secret in their own company — the exact sequence that used to corrupt bindings before #10576 > - This pull request adds an instance-gated metadata endpoint for an environment's secret refs and teaches the picker to name a cross-company secret and its owner honestly > - The benefit is that operators can tell a healthy cross-company binding from a broken one, and stop creating duplicate secrets ## Linked Issues or Issue Description **Is your feature request related to a problem? Please describe.** In the environment editor, a secret-ref field that points at a secret owned by a different company shows "Missing secret (22095402…)" in red, with "The previously selected secret is no longer available. Pick another or remove the binding." The binding is actually healthy — the current company's picker just cannot list the other company's secrets. Operators react by creating a duplicate secret and re-pointing the field. **Describe the solution you'd like** The editor should know the referenced secret's name, status, and owning company (metadata only, never the value) and present a cross-company ref neutrally, a deleted secret as deleted, and only an unknown id as missing. Related: #10576 (fixes the binding corruption this UI state used to trigger). ## What Changed - New `GET /environments/:id/secret-refs` returns `{ refs: [{ configPath, secretId, name, status, companyId, companyName }] }` for the environment's config-derived secret refs. Values are never returned. The route sits behind `assertCanAccessInstanceEnvironments`, the same gate as environment editing. - New `secretService.describeSecretRefs` loads that metadata across companies; unknown ids are omitted. - `SecretBindingPicker` reads an optional `SecretRefHintsContext` (keyed by secret id). With a hint, a ref the company list cannot show renders as `NAME — Owning Company` with neutral styling and the note "Owned by the … company. The binding keeps working; selecting a secret from this list re-points it here." A hint with `status: "deleted"` reports the secret as deleted. Without hints, behavior is byte-identical to before — agent editors and other picker users are unaffected. - `CompanyEnvironments` fetches descriptors for the environment being edited and provides them through the context. ## Verification - `cd server && pnpm vitest run src/__tests__/environment-routes.test.ts src/__tests__/secrets-service.test.ts` — new endpoint happy path, agent 403 (descriptors never computed), and embedded-Postgres coverage proving cross-company names resolve and unknown ids drop out. - `cd ui && pnpm vitest run src/components/SecretBindingPicker.test.tsx src/components/JsonSchemaForm.test.tsx src/pages/CompanyEnvironments.test.tsx` — hinted cross-company rendering, hinted deleted secret, and unchanged no-hint fallback. - `pnpm run typecheck` in `server` and `ui`. - Manual: edit an environment whose secret-ref field references another company's secret; the field names the secret and its owning company instead of "Missing secret". ## Risks - The endpoint exposes secret names and company names across companies to instance-level environment editors. Those actors already manage instance-shared environments (and instance admins are implicit members of every company), so this reveals no secret material and no new reach; the service method documents that callers must sit behind an instance-level gate. - UI change is additive and context-gated; pickers without a provider render exactly as before. ## 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 --- .../src/__tests__/environment-routes.test.ts | 73 +++++++++ server/src/__tests__/secrets-service.test.ts | 41 +++++ server/src/routes/environments.ts | 16 ++ server/src/routes/openapi.ts | 9 ++ server/src/services/secrets.ts | 46 ++++++ ui/src/api/environments.ts | 11 ++ .../components/SecretBindingPicker.test.tsx | 144 ++++++++++++++++++ ui/src/components/SecretBindingPicker.tsx | 65 +++++++- ui/src/pages/CompanyEnvironments.test.tsx | 2 + ui/src/pages/CompanyEnvironments.tsx | 39 +++++ 10 files changed, 443 insertions(+), 3 deletions(-) create mode 100644 ui/src/components/SecretBindingPicker.test.tsx diff --git a/server/src/__tests__/environment-routes.test.ts b/server/src/__tests__/environment-routes.test.ts index 5a8cc7a5fb..06c04d2d94 100644 --- a/server/src/__tests__/environment-routes.test.ts +++ b/server/src/__tests__/environment-routes.test.ts @@ -66,6 +66,7 @@ const mockSecretService = vi.hoisted(() => ({ syncEnvBindingsForTarget: vi.fn(), syncSecretRefsForTarget: vi.fn(), replaceSecretRefsForInstanceTarget: vi.fn(), + describeSecretRefs: vi.fn(), remove: vi.fn(), })); const mockValidatePluginEnvironmentDriverConfig = vi.hoisted(() => vi.fn()); @@ -269,6 +270,8 @@ describe("environment routes", () => { mockSecretService.syncEnvBindingsForTarget.mockReset(); mockSecretService.syncSecretRefsForTarget.mockReset(); mockSecretService.replaceSecretRefsForInstanceTarget.mockReset(); + mockSecretService.describeSecretRefs.mockReset(); + mockSecretService.describeSecretRefs.mockResolvedValue([]); mockSecretService.remove.mockReset(); mockSecretService.create.mockResolvedValue({ id: "11111111-1111-1111-1111-111111111111", @@ -1224,6 +1227,76 @@ describe("environment routes", () => { expect(mockEnvironmentService.removeIfDeletable).not.toHaveBeenCalled(); }); + it("describes an environment's secret refs with owner metadata", async () => { + const secretId = "22222222-2222-2222-2222-222222222222"; + mockEnvironmentService.getById.mockResolvedValue({ + ...createEnvironment(), + id: "env-sandbox", + name: "Daytona", + driver: "sandbox" as const, + config: { + provider: "secure-plugin", + template: "base", + apiKey: secretId, + timeoutMs: 450000, + reuseLease: true, + }, + }); + mockSecretService.describeSecretRefs.mockResolvedValue([ + { + configPath: "apiKey", + secretId, + name: "DAYTONA_API_KEY", + status: "active", + companyId: "company-2", + companyName: "Other Team", + }, + ]); + const app = createApp({ + type: "board", + userId: "admin-1", + source: "local_implicit", + }); + + const res = await request(app).get("/api/environments/env-sandbox/secret-refs"); + + expect(res.status).toBe(200); + expect(res.body.refs).toEqual([ + { + configPath: "apiKey", + secretId, + name: "DAYTONA_API_KEY", + status: "active", + companyId: "company-2", + companyName: "Other Team", + }, + ]); + expect(mockSecretService.describeSecretRefs).toHaveBeenCalledWith([ + { secretId, configPath: "apiKey", versionSelector: "latest" }, + ]); + }); + + it("denies secret-ref descriptors to agents without instance environment access", async () => { + mockAgentService.getById.mockResolvedValue({ + id: "agent-1", + companyId: "company-1", + role: "engineer", + permissions: { canCreateAgents: false }, + }); + mockAccessService.hasPermission.mockResolvedValue(false); + const app = createApp({ + type: "agent", + agentId: "agent-1", + companyId: "company-1", + source: "agent_key", + }); + + const res = await request(app).get("/api/environments/env-1/secret-refs"); + + expect(res.status).toBe(403); + expect(mockSecretService.describeSecretRefs).not.toHaveBeenCalled(); + }); + it("clears environment selections and secret bindings across all companies when deleting an environment", async () => { const environment = { ...createEnvironment(), diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index a3855f9b72..bd30d873c2 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -286,6 +286,47 @@ describeEmbeddedPostgres("secretService", () => { expect(rows[0]?.companyId).toBe(companyA); }); + it("describeSecretRefs names secrets across companies and omits unknown ids", async () => { + const companyA = await seedCompany("Alpha"); + const companyB = await seedCompany("Beta"); + const svc = secretService(db); + const secretA = await svc.create(companyA, { + name: "PROVIDER_KEY_A", + provider: "local_encrypted", + value: "a", + }); + const secretB = await svc.create(companyB, { + name: "PROVIDER_KEY_B", + provider: "local_encrypted", + value: "b", + }); + + const described = await svc.describeSecretRefs([ + { secretId: secretA.id, configPath: "apiKey" }, + { secretId: secretB.id, configPath: "privateKeySecretRef" }, + { secretId: randomUUID(), configPath: "token" }, + ]); + + expect(described).toEqual([ + { + configPath: "apiKey", + secretId: secretA.id, + name: "PROVIDER_KEY_A", + status: "active", + companyId: companyA, + companyName: "Alpha", + }, + { + configPath: "privateKeySecretRef", + secretId: secretB.id, + name: "PROVIDER_KEY_B", + status: "active", + companyId: companyB, + companyName: "Beta", + }, + ]); + }); + it("prevents duplicate bindings for a target config path", async () => { const companyId = await seedCompany(); const svc = secretService(db); diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index 1ca4259506..087b60c532 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -932,6 +932,22 @@ export function environmentRoutes( res.json(presentEnvironmentForRead(req, environment)); }); + router.get("/environments/:id/secret-refs", async (req, res) => { + assertCanAccessInstanceEnvironments(req); + const environment = await svc.getById(req.params.id as string); + if (!environment) { + res.status(404).json({ error: "Environment not found" }); + return; + } + // Metadata only (name / status / owning company) — never secret values. + // Environments are instance-scoped while secrets are company-scoped, so + // the editor needs this to render refs whose secret a given company's + // picker cannot list. Gated by the same instance-level access check as + // environment editing. + const refs = await collectEnvironmentSecretRefs({ db, environment }); + res.json({ refs: await secrets.describeSecretRefs(refs) }); + }); + router.get("/environments/:id/leases", async (req, res) => { assertCanReadInstanceEnvironments(req); const environment = await svc.getById(req.params.id as string); diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 43fd6a4fe4..ebfd82fd5e 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -4576,6 +4576,15 @@ registry.registerPath({ responses: { 200: r.ok(), 401: r.unauthorized, 403: r.forbidden, 404: r.notFound }, }); +registry.registerPath({ + method: "get", + path: "/api/environments/{id}/secret-refs", + tags: ["environments"], + summary: "Describe an environment's config secret refs (name, status, owning company — never values)", + request: { params: z.object({ id: z.string() }) }, + responses: { 200: r.ok(), 401: r.unauthorized, 403: r.forbidden, 404: r.notFound }, +}); + registry.registerPath({ method: "get", path: "/api/environments/{id}/leases", diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index db97905975..aa0b7124ff 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -3,6 +3,7 @@ import { and, desc, eq, inArray, like, ne, notInArray, notLike, or, sql } from " import type { Db } from "@paperclipai/db"; import { agents, + companies, companySecretBindings, companySecretProviderConfigs, companySecrets, @@ -4120,6 +4121,51 @@ export function secretService(db: Db) { return normalizedRefs; }, + /** + * Describe secret refs (id + config path) with the referenced secret's + * name, status, and owning company. Environments are instance-scoped + * while secrets are company-scoped, so an environment can legitimately + * reference a secret a given company's picker cannot list; this gives + * instance-level readers enough metadata to present such refs honestly. + * Returns names across companies — callers must sit behind an + * instance-level authorization gate. Never returns secret values. + */ + describeSecretRefs: async ( + refs: Array<{ secretId: string; configPath: string }>, + ): Promise> => { + if (refs.length === 0) return []; + const secretIds = [...new Set(refs.map((ref) => ref.secretId))]; + const secretRows = await db + .select() + .from(companySecrets) + .where(inArray(companySecrets.id, secretIds)); + const secretsById = new Map(secretRows.map((row) => [row.id, row])); + const companyIds = [...new Set(secretRows.map((row) => row.companyId))]; + const companyRows = companyIds.length > 0 + ? await db.select().from(companies).where(inArray(companies.id, companyIds)) + : []; + const companyNamesById = new Map(companyRows.map((row) => [row.id, row.name])); + return refs.flatMap((ref) => { + const secret = secretsById.get(ref.secretId); + if (!secret) return []; + return [{ + configPath: ref.configPath, + secretId: secret.id, + name: secret.name, + status: secret.status, + companyId: secret.companyId, + companyName: companyNamesById.get(secret.companyId) ?? null, + }]; + }); + }, + listBindingCompanyIdsForTarget: async ( target: { targetType: SecretBindingTargetType; targetId: string }, ): Promise => diff --git a/ui/src/api/environments.ts b/ui/src/api/environments.ts index 030def1198..e3ebe8a370 100644 --- a/ui/src/api/environments.ts +++ b/ui/src/api/environments.ts @@ -58,11 +58,22 @@ function customImageCompanyQuery(companyId: string): string { return `companyId=${encodeURIComponent(companyId)}`; } +export interface EnvironmentSecretRefDescriptor { + configPath: string; + secretId: string; + name: string; + status: string; + companyId: string; + companyName: string | null; +} + export const environmentsApi = { list: (companyId: string) => api.get(`/companies/${companyId}/environments`), capabilities: (companyId: string) => api.get(`/companies/${companyId}/environments/capabilities`), lease: (leaseId: string) => api.get(`/environment-leases/${leaseId}`), + secretRefs: (environmentId: string) => + api.get<{ refs: EnvironmentSecretRefDescriptor[] }>(`/environments/${environmentId}/secret-refs`), create: (companyId: string, body: { name: string; description?: string | null; diff --git a/ui/src/components/SecretBindingPicker.test.tsx b/ui/src/components/SecretBindingPicker.test.tsx new file mode 100644 index 0000000000..476ab5b5d3 --- /dev/null +++ b/ui/src/components/SecretBindingPicker.test.tsx @@ -0,0 +1,144 @@ +// @vitest-environment jsdom + +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + SecretBindingPicker, + SecretRefHintsContext, + type SecretRefHintsContextValue, +} from "./SecretBindingPicker"; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +const mockSecretsApi = vi.hoisted(() => ({ + list: vi.fn(), + create: vi.fn(), +})); + +vi.mock("../api/secrets", () => ({ + secretsApi: mockSecretsApi, +})); + +vi.mock("../context/CompanyContext", () => ({ + useCompany: () => ({ selectedCompanyId: "company-1" }), +})); + +describe("SecretBindingPicker", () => { + let container: HTMLDivElement; + let root: Root | null = null; + let queryClient: QueryClient; + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + mockSecretsApi.list.mockReset(); + mockSecretsApi.list.mockResolvedValue([]); + }); + + afterEach(() => { + act(() => { + root?.unmount(); + }); + root = null; + container.remove(); + queryClient.clear(); + }); + + async function render(context: SecretRefHintsContextValue | undefined) { + await act(async () => { + root = createRoot(container); + root.render( + + + {}} + /> + + , + ); + }); + // Let the secrets query settle so selectedMissing is based on real data. + await act(async () => { + await Promise.resolve(); + }); + } + + function readyContext(status: string): SecretRefHintsContextValue { + return { + status: "ready", + hints: { + "22222222-2222-2222-2222-222222222222": { + name: "DAYTONA_API_KEY", + status, + companyId: "company-2", + companyName: "Other Team", + }, + }, + }; + } + + it("names an active cross-company secret and its owner instead of calling it missing", async () => { + await render(readyContext("active")); + + expect(container.textContent).toContain("DAYTONA_API_KEY — Other Team"); + expect(container.textContent).toContain("Owned by the Other Team company"); + expect(container.textContent).not.toContain("Missing secret"); + expect(container.querySelector("select")?.className).not.toContain("border-destructive"); + }); + + it("reports a deleted hinted secret as deleted", async () => { + await render(readyContext("deleted")); + + expect(container.textContent).toContain("DAYTONA_API_KEY — Other Team"); + expect(container.textContent).toContain("was deleted"); + expect(container.querySelector("select")?.className).toContain("border-destructive"); + }); + + it("does not present a disabled cross-company secret as working", async () => { + await render(readyContext("disabled")); + + expect(container.textContent).toContain("DAYTONA_API_KEY — Other Team"); + expect(container.textContent).toContain("This secret is disabled"); + expect(container.textContent).not.toContain("keeps working"); + expect(container.querySelector("select")?.className).toContain("border-destructive"); + }); + + it("stays neutral while descriptors are loading", async () => { + await render({ status: "loading", hints: {} }); + + expect(container.textContent).toContain("Checking this secret reference"); + expect(container.textContent).not.toContain("Missing secret"); + expect(container.querySelector("select")?.className).not.toContain("border-destructive"); + }); + + it("stays neutral when the descriptor lookup failed", async () => { + await render({ status: "error", hints: {} }); + + expect(container.textContent).toContain("Could not load this secret reference"); + expect(container.textContent).not.toContain("Missing secret"); + expect(container.querySelector("select")?.className).not.toContain("border-destructive"); + }); + + it("treats an unknown id as missing once descriptors are ready", async () => { + await render({ status: "ready", hints: {} }); + + expect(container.textContent).toContain("Missing secret"); + expect(container.textContent).toContain("no longer available"); + expect(container.querySelector("select")?.className).toContain("border-destructive"); + }); + + it("keeps the generic missing-secret treatment when no hint context exists", async () => { + await render(undefined); + + expect(container.textContent).toContain("Missing secret"); + expect(container.textContent).toContain("no longer available"); + expect(container.querySelector("select")?.className).toContain("border-destructive"); + }); +}); diff --git a/ui/src/components/SecretBindingPicker.tsx b/ui/src/components/SecretBindingPicker.tsx index 1bf33d19d3..a25891f0a4 100644 --- a/ui/src/components/SecretBindingPicker.tsx +++ b/ui/src/components/SecretBindingPicker.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from "react"; +import { createContext, useContext, useMemo, useState } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { AlertCircle, KeyRound, Loader2, Plus, X } from "lucide-react"; import type { CompanySecret, SecretVersionSelector } from "@paperclipai/shared"; @@ -16,6 +16,32 @@ export interface SecretBindingValue { version?: SecretVersionSelector; } +/** + * Metadata for bound secrets the current company's list cannot show — e.g. + * an instance-scoped environment referencing a secret owned by another + * company. Keyed by secret id. Editors that can read instance-level + * secret-ref descriptors provide it; everywhere else the context is absent + * and the picker falls back to its generic missing-secret treatment. + */ +export interface SecretRefHint { + name: string; + status: string; + companyId: string; + companyName: string | null; +} + +/** + * `status` reports the descriptor request itself, so the picker never claims + * a secret is missing while the lookup is still loading or has failed — + * only a `ready` map is authoritative about unknown ids. + */ +export interface SecretRefHintsContextValue { + status: "loading" | "error" | "ready"; + hints: Record; +} + +export const SecretRefHintsContext = createContext(undefined); + interface SecretBindingPickerProps { value: SecretBindingValue | null; onChange: (next: SecretBindingValue | null) => void; @@ -96,6 +122,14 @@ export function SecretBindingPicker({ }, [secretsQuery.data, value]); const selectedMissing = Boolean(value && !selectedSecret); + const hintsContext = useContext(SecretRefHintsContext); + const missingHint = selectedMissing && value ? hintsContext?.hints[value.secretId] : undefined; + // Only an active cross-company secret is healthy: runtime resolution + // rejects disabled/archived/deleted secrets, so those must not be + // presented as working bindings. + const crossCompanyHint = missingHint && missingHint.status === "active" ? missingHint : undefined; + const hintsPending = selectedMissing && !missingHint && hintsContext !== undefined && hintsContext.status !== "ready"; + const calmMissing = Boolean(crossCompanyHint) || hintsPending; const createMutation = useMutation({ mutationFn: () => @@ -146,7 +180,7 @@ export function SecretBindingPicker({