From 27db0d3c67e7c5198b3a32a455e1b781e82979e9 Mon Sep 17 00:00:00 2001 From: Chris Farhood Date: Sun, 3 May 2026 11:00:02 -0400 Subject: [PATCH 1/2] fix(skills): prevent PAT pollution of bundled skills and orphan secrets on delete Five connected gaps in the original PAT feature, all in company-skills.ts: 1. upsertImportedSkills only protected bundled rows from overwrite when the incoming source claimed to be paperclipai/paperclip. A SKILL.md from any other org/repo whose key resolves to paperclipai/paperclip/ would hijack the bundled row and gain a sourceAuthSecretId. Broadened: any non-bundled incoming is rejected when existing is paperclip_bundled. 2. The metadata-build block preserved sourceAuthSecretId from existing indiscriminately, so any pollution of a bundled row was kept across every ensureBundledSkills re-upsert. Skip preservation when existing is bundled. 3. importFromSource's auth-token loop wrote sourceAuthSecretId for every imported skill including any bundled ones that snuck through. Defense in depth: skip skills with sourceKind === "paperclip_bundled". 4. updateSkillAuth had no guard, so the PATCH /skills/:id/auth route could attach a PAT to a bundled skill via direct API call. Reject explicitly. 5. deleteSkill removed the secret without checking whether any sibling skill still referenced it via metadata.sourceAuthSecretId. Re-imports preserve that reference, so two skills could share a secret and deleting one would orphan the other's reference. Now skip the remove if another skill in the same company still references the secret. --- server/src/services/company-skills.ts | 43 +++++++++++++++++++++------ 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 790dd02d..b9238e06 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -2,7 +2,7 @@ import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { and, asc, eq } from "drizzle-orm"; +import { and, asc, eq, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { companies, companySkills } from "@paperclipai/db"; import { readPaperclipSkillSyncPreference } from "@paperclipai/adapter-utils/server-utils"; @@ -2372,21 +2372,27 @@ export function companySkillService(db: Db) { const incomingOwner = asString(incomingMeta.owner); const incomingRepo = asString(incomingMeta.repo); const incomingKind = asString(incomingMeta.sourceKind); + // Bundled skills are sourced from the server image and re-upserted by + // ensureBundledSkills only. Never let a non-bundled import overwrite a + // bundled row, regardless of which org/repo it claims to be from. if ( existing && existingMeta.sourceKind === "paperclip_bundled" - && incomingKind === "github" - && incomingOwner === "paperclipai" - && incomingRepo === "paperclip" + && incomingKind !== "paperclip_bundled" ) { out.push(existing); continue; } + // Preserve sourceAuthSecretId across re-imports of the same skill. Skip + // bundled rows: they should never carry a PAT reference, and preserving + // one across a bundled re-upsert would re-attach stale data. const metadata = { ...(skill.metadata ?? {}), skillKey: skill.key, - ...(existing?.metadata && typeof (existing.metadata as Record).sourceAuthSecretId === "string" + ...(existing?.metadata + && existingMeta.sourceKind !== "paperclip_bundled" + && typeof (existing.metadata as Record).sourceAuthSecretId === "string" ? { sourceAuthSecretId: (existing.metadata as Record).sourceAuthSecretId } : {}), }; @@ -2464,6 +2470,8 @@ export function companySkillService(db: Db) { if (authToken && imported.length > 0) { for (const skill of imported) { + const skillMeta = skill.metadata as Record | null; + if (skillMeta?.sourceKind === "paperclip_bundled") continue; const secretName = `skill-pat:${skill.id}`; let secretId: string; const existing = await secretsSvc.getByName(companyId, secretName); @@ -2530,10 +2538,24 @@ export function companySkillService(db: Db) { const meta = skill.metadata as Record | null; const secretId = typeof meta?.sourceAuthSecretId === "string" ? meta.sourceAuthSecretId : null; if (secretId) { - try { - await secretsSvc.remove(secretId); - } catch { - // Best-effort: don't fail the skill deletion if secret cleanup fails + // Skip cleanup if another skill in the same company still references this + // secret. The deleted row is already gone from the table, so any result + // here is a sibling skill we shouldn't orphan. + const otherSkillRefs = await db + .select({ id: companySkills.id }) + .from(companySkills) + .where(and( + eq(companySkills.companyId, companyId), + sql`${companySkills.metadata} ->> 'sourceAuthSecretId' = ${secretId}`, + )) + .limit(1); + if (otherSkillRefs.length === 0) { + try { + await secretsSvc.remove(secretId); + } catch { + // Best-effort: don't fail the skill deletion if secret cleanup fails + // (typically blocked by an agent env binding still referencing it). + } } } @@ -2549,6 +2571,9 @@ export function companySkillService(db: Db) { if (!skill) return null; const meta = (skill.metadata ?? {}) as Record; + if (meta.sourceKind === "paperclip_bundled") { + throw unprocessable("Cannot configure auth for bundled paperclip skills"); + } const existingSecretId = typeof meta.sourceAuthSecretId === "string" ? meta.sourceAuthSecretId : null; if (authToken) { From 3ea3020a76d70d90cdafdd5e15fa2baa02fa31ca Mon Sep 17 00:00:00 2001 From: Chris Farhood Date: Sun, 3 May 2026 11:02:16 -0400 Subject: [PATCH 2/2] fix(secrets): include skill metadata references in usages, route, and delete dialog secretsSvc.usages() previously only scanned agent env bindings. Skills can reference a secret via metadata.sourceAuthSecretId (set by the PAT auth feature), so removing a secret without checking those references could orphan a sibling skill's PAT pointer. - Extend usages() to return { agents, skills } with both reference kinds. - Update remove() to block when either array is non-empty. - /secrets/:id/usages now responds with { agents, skills }. - The delete dialog displays both kinds inline so the operator knows which references to detach first. --- server/src/routes/secrets.ts | 4 ++-- server/src/services/secrets.ts | 42 +++++++++++++++++++++++++-------- ui/src/api/secrets.ts | 5 +++- ui/src/pages/CompanySecrets.tsx | 38 ++++++++++++++++++++--------- 4 files changed, 65 insertions(+), 24 deletions(-) diff --git a/server/src/routes/secrets.ts b/server/src/routes/secrets.ts index c7748079..862851fe 100644 --- a/server/src/routes/secrets.ts +++ b/server/src/routes/secrets.ts @@ -141,8 +141,8 @@ export function secretRoutes(db: Db) { return; } assertCompanyAccess(req, existing.companyId); - const agents = await svc.usages(existing.companyId, id); - res.json({ agents }); + const used = await svc.usages(existing.companyId, id); + res.json({ agents: used.agents, skills: used.skills }); }); router.delete("/secrets/:id", async (req, res) => { diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index 77155de7..0b69b262 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -1,6 +1,6 @@ -import { and, desc, eq } from "drizzle-orm"; +import { and, desc, eq, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; -import { companySecrets, companySecretVersions } from "@paperclipai/db"; +import { companySecrets, companySecretVersions, companySkills } from "@paperclipai/db"; import type { AgentEnvConfig, EnvBinding, SecretProvider } from "@paperclipai/shared"; import { envBindingSchema } from "@paperclipai/shared"; import { conflict, notFound, unprocessable } from "../errors.js"; @@ -46,6 +46,8 @@ export function secretService(db: Db) { }; type SecretUsageAgent = { id: string; name: string; envKeys: string[] }; + type SecretUsageSkill = { id: string; name: string; slug: string }; + type SecretUsages = { agents: SecretUsageAgent[]; skills: SecretUsageSkill[] }; async function getById(id: string) { return db @@ -55,10 +57,10 @@ export function secretService(db: Db) { .then((rows) => rows[0] ?? null); } - async function usages(companyId: string, secretId: string): Promise { + async function usages(companyId: string, secretId: string): Promise { const agents = agentService(db); const allAgents = await agents.list(companyId); - const out: SecretUsageAgent[] = []; + const agentRefs: SecretUsageAgent[] = []; for (const agent of allAgents) { const config = asRecord(agent.adapterConfig); if (!config) continue; @@ -73,10 +75,28 @@ export function secretService(db: Db) { } } if (matchingKeys.length > 0) { - out.push({ id: agent.id, name: agent.name, envKeys: matchingKeys }); + agentRefs.push({ id: agent.id, name: agent.name, envKeys: matchingKeys }); } } - return out; + + const skillRows = await db + .select({ + id: companySkills.id, + name: companySkills.name, + slug: companySkills.slug, + }) + .from(companySkills) + .where(and( + eq(companySkills.companyId, companyId), + sql`${companySkills.metadata} ->> 'sourceAuthSecretId' = ${secretId}`, + )); + const skillRefs: SecretUsageSkill[] = skillRows.map((row) => ({ + id: row.id, + name: row.name, + slug: row.slug, + })); + + return { agents: agentRefs, skills: skillRefs }; } async function getByName(companyId: string, name: string) { @@ -315,11 +335,13 @@ export function secretService(db: Db) { const secret = await getById(secretId); if (!secret) return null; const used = await usages(secret.companyId, secretId); - if (used.length > 0) { - const names = used.map((agent) => agent.name).sort((left, right) => left.localeCompare(right)); + if (used.agents.length > 0 || used.skills.length > 0) { + const agentNames = used.agents.map((agent) => agent.name); + const skillNames = used.skills.map((skill) => skill.name); + const names = [...agentNames, ...skillNames].sort((left, right) => left.localeCompare(right)); throw unprocessable( - `Cannot delete secret "${secret.name}" while it is still used by ${names.join(", ")}. Detach it from those agents first.`, - { secretId, usedByAgents: used }, + `Cannot delete secret "${secret.name}" while it is still used by ${names.join(", ")}. Detach it from those references first.`, + { secretId, usedByAgents: used.agents, usedBySkills: used.skills }, ); } await db.delete(companySecrets).where(eq(companySecrets.id, secretId)); diff --git a/ui/src/api/secrets.ts b/ui/src/api/secrets.ts index 397bfd5f..ba30328e 100644 --- a/ui/src/api/secrets.ts +++ b/ui/src/api/secrets.ts @@ -23,5 +23,8 @@ export const secretsApi = { ) => api.patch(`/secrets/${id}`, data), remove: (id: string) => api.delete<{ ok: true }>(`/secrets/${id}`), usages: (id: string) => - api.get<{ agents: { id: string; name: string; envKeys: string[] }[] }>(`/secrets/${id}/usages`), + api.get<{ + agents: { id: string; name: string; envKeys: string[] }[]; + skills: { id: string; name: string; slug: string }[]; + }>(`/secrets/${id}/usages`), }; diff --git a/ui/src/pages/CompanySecrets.tsx b/ui/src/pages/CompanySecrets.tsx index 56ec4354..65834276 100644 --- a/ui/src/pages/CompanySecrets.tsx +++ b/ui/src/pages/CompanySecrets.tsx @@ -421,7 +421,9 @@ function EditSecretDialog({ ); } -type DeleteUsage = { id: string; name: string; envKeys: string[] }; +type DeleteUsageAgent = { id: string; name: string; envKeys: string[] }; +type DeleteUsageSkill = { id: string; name: string; slug: string }; +type DeleteBlock = { agents: DeleteUsageAgent[]; skills: DeleteUsageSkill[] }; function DeleteSecretDialog({ secret, @@ -432,7 +434,7 @@ function DeleteSecretDialog({ onClose: () => void; onDeleted: () => void; }) { - const [blockedBy, setBlockedBy] = useState(null); + const [blockedBy, setBlockedBy] = useState(null); const [errorMessage, setErrorMessage] = useState(null); const remove = useMutation({ @@ -441,12 +443,16 @@ function DeleteSecretDialog({ onError: (error: unknown) => { if (error instanceof ApiError) { const body = error.body as - | { details?: { usedByAgents?: DeleteUsage[] } } + | { details?: { usedByAgents?: DeleteUsageAgent[]; usedBySkills?: DeleteUsageSkill[] } } | null | undefined; - const usedByAgents = body?.details?.usedByAgents; - if (Array.isArray(usedByAgents) && usedByAgents.length > 0) { - setBlockedBy(usedByAgents); + const usedByAgents = body?.details?.usedByAgents ?? []; + const usedBySkills = body?.details?.usedBySkills ?? []; + if ( + (Array.isArray(usedByAgents) && usedByAgents.length > 0) + || (Array.isArray(usedBySkills) && usedBySkills.length > 0) + ) { + setBlockedBy({ agents: usedByAgents, skills: usedBySkills }); setErrorMessage(error.message); return; } @@ -457,6 +463,8 @@ function DeleteSecretDialog({ }, }); + const totalBlockers = blockedBy ? blockedBy.agents.length + blockedBy.skills.length : 0; + return ( !open && onClose()}> @@ -464,25 +472,33 @@ function DeleteSecretDialog({ Delete "{secret.name}" This permanently deletes the secret and all of its versions. Agents - that reference it will fail until the binding is removed. + and skills that reference it will fail until the binding is removed. - {blockedBy && blockedBy.length > 0 ? ( + {blockedBy && totalBlockers > 0 ? (

Cannot delete — still referenced by:

    - {blockedBy.map((agent) => ( -
  • + {blockedBy.agents.map((agent) => ( +
  • {agent.name}{" "} + agent{" "} ({agent.envKeys.join(", ")})
    • ))} + {blockedBy.skills.map((skill) => ( +
  • + {skill.name}{" "} + skill{" "} + ({skill.slug}) +
    • + ))}

- Detach this secret from those agents' environment variables first. + Detach this secret from those references first.

) : errorMessage ? (