forked from farhoodlabs/paperclip
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/<slug> 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.
This commit is contained in:
@@ -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<string, unknown>).sourceAuthSecretId === "string"
|
||||
...(existing?.metadata
|
||||
&& existingMeta.sourceKind !== "paperclip_bundled"
|
||||
&& typeof (existing.metadata as Record<string, unknown>).sourceAuthSecretId === "string"
|
||||
? { sourceAuthSecretId: (existing.metadata as Record<string, unknown>).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<string, unknown> | 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<string, unknown> | 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<string, unknown>;
|
||||
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) {
|
||||
|
||||
Reference in New Issue
Block a user