fix: block direct skill transfers under moderation (#2282)

* fix: block direct skill transfers under moderation

* fix(api): block accepted transfers for moderated skills

* fix(api): block malware-flagged skill transfers

* fix: cover moderated skill transfer bypasses

* test: support ownership heal transfer sync

* fix: close moderated transfer backfill gaps

* fix: block soft-deleted transfer guard state

---------

Co-authored-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
This commit is contained in:
Vyctor H. Brzezowski
2026-05-27 12:01:51 -07:00
committed by GitHub
co-authored by Patrick Erichsen
parent 8bb6a0d584
commit 6f893b54f4
11 changed files with 497 additions and 84 deletions
+1
View File
@@ -16,6 +16,7 @@
### Fixes
- API: fix `GET /api/v1/skills` pagination so `cursor` advances to the next page instead of repeating the first page for supported non-trending sorts (#2275) (thanks @vyctorbrzezowski, @enerj).
- Security/API: reject direct skill owner transfers when the skill is hidden, suspicious, or malicious (thanks @vyctorbrzezowski).
- Security/API: revalidate package publish actor, owner, and owner publisher active state in the final release insert (thanks @vyctorbrzezowski).
## 0.17.0 - 2026-05-19
+35 -1
View File
@@ -1,5 +1,9 @@
import { describe, expect, it } from "vitest";
import { isSkillReviewFlagged, isSkillSuspicious } from "./skillSafety";
import {
isSkillReviewFlagged,
isSkillSuspicious,
isSkillTransferBlockedByModeration,
} from "./skillSafety";
describe("isSkillSuspicious", () => {
it("returns true when suspicious flag is present", () => {
@@ -39,3 +43,33 @@ describe("isSkillSuspicious", () => {
expect(isSkillReviewFlagged(skill)).toBe(true);
});
});
describe("isSkillTransferBlockedByModeration", () => {
it("blocks scanner malicious reasons even when verdict fields are missing", () => {
expect(
isSkillTransferBlockedByModeration({
moderationStatus: "active",
moderationVerdict: undefined,
isSuspicious: false,
moderationFlags: undefined,
moderationReason: "scanner.vt.malicious",
moderationReasonCodes: undefined,
softDeletedAt: undefined,
}),
).toBe(true);
});
it("blocks legacy hidden skills that only have softDeletedAt", () => {
expect(
isSkillTransferBlockedByModeration({
moderationStatus: undefined,
moderationVerdict: undefined,
isSuspicious: false,
moderationFlags: undefined,
moderationReason: undefined,
moderationReasonCodes: undefined,
softDeletedAt: 123,
}),
).toBe(true);
});
});
+38
View File
@@ -1,10 +1,16 @@
import type { Doc } from "../_generated/dataModel";
import { verdictFromCodes } from "./moderationReasonCodes";
function isScannerSuspiciousReason(reason: string | undefined) {
if (!reason) return false;
return reason.startsWith("scanner.") && reason.endsWith(".suspicious");
}
function isScannerMaliciousReason(reason: string | undefined) {
if (!reason) return false;
return reason.startsWith("scanner.") && reason.endsWith(".malicious");
}
export function isSkillSuspicious(
skill: Pick<Doc<"skills">, "moderationFlags" | "moderationReason">,
) {
@@ -12,6 +18,38 @@ export function isSkillSuspicious(
return isScannerSuspiciousReason(skill.moderationReason);
}
export function isSkillBlockedByMalware(skill: Pick<Doc<"skills">, "moderationFlags">) {
return skill.moderationFlags?.includes("blocked.malware") ?? false;
}
export function isSkillTransferBlockedByModeration(
skill: Pick<
Doc<"skills">,
| "moderationStatus"
| "moderationVerdict"
| "isSuspicious"
| "moderationFlags"
| "moderationReason"
| "moderationReasonCodes"
| "softDeletedAt"
>,
) {
const moderationStatus = skill.moderationStatus ?? "active";
const moderationVerdict =
skill.moderationVerdict ?? verdictFromCodes(skill.moderationReasonCodes ?? []);
return (
skill.softDeletedAt !== undefined ||
moderationStatus !== "active" ||
moderationVerdict === "suspicious" ||
moderationVerdict === "malicious" ||
skill.isSuspicious ||
skill.moderationFlags?.includes("flagged.suspicious") ||
isSkillBlockedByMalware(skill) ||
isSkillSuspicious(skill) ||
isScannerMaliciousReason(skill.moderationReason)
);
}
export function isSkillReviewFlagged(skill: Pick<Doc<"skills">, "moderationFlags">) {
return skill.moderationFlags?.includes("flagged.review") ?? false;
}
+55
View File
@@ -443,6 +443,61 @@ describe("skillTransfers", () => {
);
});
it("acceptTransferInternal rejects skills under moderation before ownership writes", async () => {
const patch = vi.fn(async () => {});
const skill = {
_id: "skills:1",
slug: "demo",
ownerUserId: "users:1",
ownerPublisherId: "publishers:owner",
softDeletedAt: undefined,
moderationStatus: "active",
isSuspicious: false,
moderationReasonCodes: ["suspicious.dynamic_code_execution"],
};
await expect(
acceptTransferInternalHandler(
{
db: {
normalizeId: vi.fn(),
get: vi.fn(async (id: string) => {
if (id === "users:2") return { _id: "users:2", handle: "alice" };
if (id === "skillOwnershipTransfers:1") {
return {
_id: "skillOwnershipTransfers:1",
skillId: "skills:1",
fromUserId: "users:1",
toUserId: "users:2",
status: "pending",
requestedAt: Date.now() - 1_000,
expiresAt: Date.now() + 10_000,
};
}
if (id === "skills:1") return skill;
return null;
}),
query: vi.fn(() => {
throw new Error("unexpected query after moderation guard");
}),
patch,
insert: vi.fn(async () => "auditLogs:1"),
},
} as never,
{
actorUserId: "users:2",
transferId: "skillOwnershipTransfers:1",
} as never,
),
).resolves.toEqual({ ok: false, error: "Skill is under moderation" });
expect(patch).toHaveBeenCalledWith(
"skillOwnershipTransfers:1",
expect.objectContaining({ status: "cancelled" }),
);
expect(patch).not.toHaveBeenCalledWith("skills:1", expect.anything());
});
it("acceptTransferInternal honors publisher-admin source requests", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async () => "auditLogs:1");
+2 -6
View File
@@ -7,6 +7,7 @@ import {
ensurePersonalPublisherForUser,
getActiveUserByHandleOrPersonalPublisher,
} from "./lib/publishers";
import { isSkillTransferBlockedByModeration } from "./lib/skillSafety";
const TRANSFER_EXPIRY_MS = 7 * 24 * 60 * 60 * 1000;
type TransferDoc = Doc<"skillOwnershipTransfers">;
@@ -190,11 +191,7 @@ export const acceptTransferInternal = internalMutation({
const skill = await ctx.db.get(transfer.skillId);
if (!skill || skill.softDeletedAt) throw new Error("Skill not found");
if (
skill.moderationVerdict === "malicious" ||
skill.moderationStatus === "hidden" ||
skill.moderationStatus === "removed"
) {
if (isSkillTransferBlockedByModeration(skill)) {
return await cancelTransfer("Skill is under moderation");
}
const requester = await ctx.db.get(transfer.fromUserId);
@@ -208,7 +205,6 @@ export const acceptTransferInternal = internalMutation({
return await cancelTransfer("Transfer is no longer valid");
}
}
const newPublisher = await ensurePersonalPublisherForUser(ctx, newOwner, {
actorUserId: args.actorUserId,
source: "skill.transfer.accept",
+58
View File
@@ -82,6 +82,7 @@ function createMigrationFixture(params: {
*/
skillSource?: SkillSourceMode;
sourcePersonalLinkedUserId?: string | null;
skillOverrides?: Record<string, unknown>;
}): OrgMigrationFixture {
const now = Date.now();
const patchCalls: Array<{ id: string; value: Record<string, unknown> }> = [];
@@ -261,6 +262,7 @@ function createMigrationFixture(params: {
comments: 0,
versions: 1,
},
...params.skillOverrides,
}),
};
}
@@ -475,6 +477,44 @@ describe("skills.insertVersion owner migration", () => {
expect(embeddingPatches[0]?.value).toMatchObject({ ownerId: "users:caller" });
});
it("rejects owner migration for skills still blocked by legacy reason codes", async () => {
const fixture = createMigrationFixture({
skillSource: "source-org",
sourceMemberships: [
{
_id: "publisherMembers:sourceAdmin",
publisherId: "publishers:sourceOrg",
userId: "users:caller",
role: "admin",
},
{
_id: "publisherMembers:orgAdminCaller",
publisherId: "publishers:org",
userId: "users:caller",
role: "admin",
},
],
skillOverrides: {
moderationReasonCodes: ["malicious.crypto_mining"],
},
});
await expect(
insertVersionHandler(
{ db: fixture.db } as never,
buildPublishArgs({ migrateOwner: true }) as never,
),
).rejects.toThrow("under moderation");
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
expect(skillPatches).toHaveLength(0);
const migrationAudits = fixture.insertCalls.filter(
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
);
expect(migrationAudits).toHaveLength(0);
});
it("migrates ownership when caller moves their OWN personal skill into an org they belong to", async () => {
// Real issue scenario: @cbrunnkvist owns `nano` under their personal
// publisher and wants to republish under `@casualsecurityinc`.
@@ -564,6 +604,24 @@ describe("skills.insertVersion owner migration", () => {
});
});
it("rejects legacy publisher backfill for skills under moderation", async () => {
const fixture = createMigrationFixture({
skillSource: "caller-personal",
sourceMemberships: [],
skillOverrides: {
ownerPublisherId: undefined,
moderationReason: "scanner.vt.malicious",
},
});
await expect(
insertVersionHandler({ db: fixture.db } as never, buildPublishArgs() as never),
).rejects.toThrow("under moderation");
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
expect(skillPatches).toHaveLength(0);
});
it("refuses to migrate a skill out of SOMEONE ELSE'S personal publisher even if caller happens to be a member", async () => {
// Defense-in-depth: addMember currently doesn't forbid adding extra
// members to a user-kind publisher. We must still refuse to let the
+199 -1
View File
@@ -1,11 +1,13 @@
import { describe, expect, it, vi } from "vitest";
import { afterEach, describe, expect, it, vi } from "vitest";
vi.mock("@convex-dev/auth/server", () => ({
getAuthUserId: vi.fn(),
authTables: {},
}));
import { getAuthUserId } from "@convex-dev/auth/server";
import {
changeOwner,
getSkillBySlugInternal,
mergeOwnedSkillIntoCanonicalInternal,
renameOwnedSkillInternal,
@@ -41,6 +43,16 @@ const transferSkillOwnerForUserInternalHandler = (
reason?: string;
}>
)._handler;
const changeOwnerHandler = (
changeOwner as unknown as WrappedHandler<{
skillId: string;
ownerUserId: string;
}>
)._handler;
afterEach(() => {
vi.mocked(getAuthUserId).mockReset();
});
function chainEq(constraints: Record<string, unknown>) {
return {
@@ -416,6 +428,8 @@ describe("skills ownership", () => {
ownerUserId: "users:actor",
ownerPublisherId: "publishers:personal",
softDeletedAt: undefined,
moderationVerdict: "clean",
moderationReasonCodes: ["suspicious.dynamic_code_execution"],
};
const aliases = [
{
@@ -561,6 +575,190 @@ describe("skills ownership", () => {
);
});
it("rejects direct owner transfers for skills under moderation", async () => {
const moderationStates = [
{ moderationStatus: "hidden" },
{ moderationStatus: "removed" },
{ moderationVerdict: "suspicious" },
{ moderationVerdict: "malicious" },
{ isSuspicious: true },
{ moderationFlags: ["flagged.suspicious"] },
{ moderationFlags: ["blocked.malware"] },
{ moderationReason: "scanner.llm.suspicious" },
{ moderationReasonCodes: ["suspicious.dynamic_code_execution"] },
{ moderationReasonCodes: ["malicious.crypto_mining"] },
];
for (const moderationState of moderationStates) {
const patch = vi.fn(async () => {});
const skill = {
_id: "skills:source",
slug: "portable",
displayName: "Portable",
ownerUserId: "users:actor",
ownerPublisherId: "publishers:personal",
softDeletedAt: undefined,
...moderationState,
};
await expect(
transferSkillOwnerForUserInternalHandler(
{
db: {
normalizeId: vi.fn(() => null),
get: vi.fn(async (id: string) => {
if (id === "users:actor") return { _id: "users:actor", role: "user" };
if (id === "publishers:personal") {
return {
_id: "publishers:personal",
kind: "user",
handle: "actor",
linkedUserId: "users:actor",
};
}
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") {
return {
withIndex: (
name: string,
build: (q: ReturnType<typeof chainEq>) => unknown,
) => {
const constraints: Record<string, unknown> = {};
build(chainEq(constraints));
if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`);
return {
unique: async () => (constraints.slug === "portable" ? skill : null),
};
},
};
}
throw new Error(`unexpected table ${table}`);
}),
patch,
insert: vi.fn(async () => "auditLogs:1"),
},
} as never,
{
actorUserId: "users:actor",
slug: "portable",
toOwner: "team",
},
),
).rejects.toThrow("under moderation");
expect(patch).not.toHaveBeenCalledWith("skills:source", expect.anything());
}
});
it("rejects admin owner changes for skills under moderation", async () => {
vi.mocked(getAuthUserId).mockResolvedValue("users:admin" as never);
const patch = vi.fn(async () => {});
await expect(
changeOwnerHandler(
{
db: {
normalizeId: vi.fn(() => null),
system: {},
get: vi.fn(async (id: string) => {
if (id === "users:admin") return { _id: "users:admin", role: "admin" };
if (id === "users:next") return { _id: "users:next", role: "user" };
if (id === "skills:source") {
return {
_id: "skills:source",
slug: "portable",
displayName: "Portable",
ownerUserId: "users:owner",
moderationReasonCodes: ["malicious.crypto_mining"],
};
}
return null;
}),
query: vi.fn(() => {
throw new Error("unexpected query");
}),
patch,
insert: vi.fn(async () => "auditLogs:1"),
},
} as never,
{
skillId: "skills:source",
ownerUserId: "users:next",
},
),
).rejects.toThrow("under moderation");
expect(patch).not.toHaveBeenCalledWith("skills:source", expect.anything());
});
it("checks direct transfer permissions before revealing moderation state", async () => {
const patch = vi.fn(async () => {});
const skill = {
_id: "skills:source",
slug: "portable",
displayName: "Portable",
ownerUserId: "users:owner",
ownerPublisherId: "publishers:personal",
softDeletedAt: undefined,
moderationStatus: "hidden",
};
await expect(
transferSkillOwnerForUserInternalHandler(
{
db: {
normalizeId: vi.fn(() => null),
get: vi.fn(async (id: string) => {
if (id === "users:actor") return { _id: "users:actor", role: "user" };
if (id === "publishers:personal") {
return {
_id: "publishers:personal",
kind: "user",
handle: "owner",
linkedUserId: "users:owner",
};
}
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") {
return {
withIndex: (name: string, build: (q: ReturnType<typeof chainEq>) => unknown) => {
const constraints: Record<string, unknown> = {};
build(chainEq(constraints));
if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`);
return {
unique: async () => (constraints.slug === "portable" ? skill : null),
};
},
};
}
if (table === "publisherMembers") {
return {
withIndex: () => ({
unique: async () => null,
}),
};
}
throw new Error(`unexpected table ${table}`);
}),
patch,
insert: vi.fn(async () => "auditLogs:1"),
},
} as never,
{
actorUserId: "users:actor",
slug: "portable",
toOwner: "team",
},
),
).rejects.toThrow("Forbidden");
expect(patch).not.toHaveBeenCalledWith("skills:source", expect.anything());
});
it("rejects merges that would reserve too many historical slugs for one skill", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async () => "auditLogs:1");
+21 -19
View File
@@ -977,32 +977,34 @@ describe("skills anti-spam guards", () => {
if (table === "skillEmbeddings") {
return {
withIndex: (name: string) => {
if (name !== "by_version") {
throw new Error(`unexpected skillEmbeddings index ${name}`);
if (name === "by_version") {
return {
unique: async () => null,
};
}
return {
unique: async () => null,
};
if (name === "by_skill") {
return {
collect: async () => [],
};
}
throw new Error(`unexpected skillEmbeddings index ${name}`);
},
};
}
if (table === "skillSlugAliases") {
return {
withIndex: (name: string) => {
if (name !== "by_slug") throw new Error(`unexpected skillSlugAliases index ${name}`);
return {
unique: async () => null,
};
},
};
}
if (table === "skillSlugAliases") {
return {
withIndex: (name: string) => {
if (name !== "by_slug") throw new Error(`unexpected skillSlugAliases index ${name}`);
return {
unique: async () => null,
};
if (name === "by_slug") {
return {
unique: async () => null,
};
}
if (name === "by_skill") {
return {
collect: async () => [],
};
}
throw new Error(`unexpected skillSlugAliases index ${name}`);
},
};
}
+52
View File
@@ -195,4 +195,56 @@ describe("skills reclaim ownership transfer", () => {
expect(runAfter).not.toHaveBeenCalled();
expect(patch).not.toHaveBeenCalled();
});
it("rejects transferRootSlugOnly ownership moves for moderated skills", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async () => {});
const runAfter = vi.fn(async () => {});
const existingSkill = {
_id: "skills:1",
slug: "blocked-skill",
ownerUserId: "users:old",
moderationStatus: "active",
moderationReasonCodes: ["malicious.crypto_mining"],
};
const db = {
normalizeId: vi.fn(),
get: vi.fn(async (id: string) => {
if (id === "users:admin") return { _id: "users:admin", role: "admin" };
if (id === "users:new") return { _id: "users:new", role: "user" };
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") {
return {
withIndex: (name: string) => {
if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`);
return { unique: async () => existingSkill };
},
};
}
throw new Error(`unexpected table ${table}`);
}),
patch,
insert,
};
await expect(
reclaimSlugInternalHandler(
{ db, scheduler: { runAfter } } as never,
{
actorUserId: "users:admin",
slug: "blocked-skill",
rightfulOwnerUserId: "users:new",
transferRootSlugOnly: true,
} as never,
),
).rejects.toThrow("under moderation");
expect(runAfter).not.toHaveBeenCalled();
expect(patch).not.toHaveBeenCalledWith("skills:1", expect.anything());
expect(insert).not.toHaveBeenCalled();
});
});
+34 -57
View File
@@ -112,7 +112,12 @@ import {
queueHighlightedWebhook,
} from "./lib/skillPublish";
import { getFrontmatterValue, hashSkillFiles } from "./lib/skills";
import { computeIsSuspicious, isSkillReviewFlagged, isSkillSuspicious } from "./lib/skillSafety";
import {
computeIsSuspicious,
isSkillReviewFlagged,
isSkillSuspicious,
isSkillTransferBlockedByModeration,
} from "./lib/skillSafety";
import {
digestToHydratableSkill,
digestToOwnerInfo,
@@ -8553,23 +8558,11 @@ export const changeOwner = mutation({
if (skill.ownerUserId === args.ownerUserId) return;
const now = Date.now();
await ctx.db.patch(skill._id, {
await transferSkillOwnershipAndEmbeddings(ctx, {
skill,
ownerUserId: args.ownerUserId,
lastReviewedAt: now,
updatedAt: now,
now,
});
await adjustUserSkillStatsForSkillChange(ctx, skill, {
...skill,
ownerUserId: args.ownerUserId,
});
const embeddings = await listSkillEmbeddingsForSkill(ctx, skill._id);
for (const embedding of embeddings) {
await ctx.db.patch(embedding._id, {
ownerId: args.ownerUserId,
updatedAt: now,
});
}
await ctx.db.insert("auditLogs", {
actorUserId: user._id,
@@ -8979,6 +8972,9 @@ async function transferSkillOwnershipAndEmbeddings(
const publisherChanged =
"ownerPublisherId" in params && params.skill.ownerPublisherId !== params.ownerPublisherId;
if (!ownerChanged && !publisherChanged) return;
if (isSkillTransferBlockedByModeration(params.skill)) {
throw new ConvexError("Skill is not eligible for ownership transfer while under moderation");
}
await ctx.db.patch(params.skill._id, patch);
@@ -9060,6 +9056,9 @@ export const transferSkillOwnerForUserInternal = internalMutation({
allowedPublisherRoles: ["admin"],
allowPlatformAdmin: true,
});
if (isSkillTransferBlockedByModeration(skill)) {
throw new ConvexError("Skill is not eligible for ownership transfer while under moderation");
}
const destinationHandle = normalizePublisherHandle(args.toOwner);
if (!destinationHandle) throw new ConvexError("Destination owner is required");
@@ -9887,48 +9886,17 @@ export const insertVersion = internalMutation({
...skill,
ownerPublisherId,
ownerUserId: userId,
lastReviewedAt: now,
updatedAt: now,
};
await ctx.db.patch(skill._id, {
await transferSkillOwnershipAndEmbeddings(ctx, {
skill,
ownerPublisherId,
ownerUserId: userId,
updatedAt: now,
now,
});
// Reassign per-user counters from the previous owner to the new one.
// Without this, `users.publishedSkills / totalStars / totalDownloads`
// would still credit the source owner after an org→org or
// personal→org migration (and double-count once the new owner
// publishes anything else). `adjustUserSkillStatsForSkillChange`
// already handles the cross-owner move cleanly — this mirrors the
// moderator `changeOwner` path above.
await adjustUserSkillStatsForSkillChange(ctx, skill, nextSkill);
// Keep `skillEmbeddings.ownerId` in sync with the skill's owner so
// "authored by" queries/filters and embedding-side access checks
// don't keep resolving to the previous owner after the migration.
const embeddings = await listSkillEmbeddingsForSkill(ctx, skill._id);
for (const embedding of embeddings) {
if (embedding.ownerId === userId) continue;
await ctx.db.patch(embedding._id, {
ownerId: userId,
updatedAt: now,
});
}
// Keep existing slug aliases pointed at the new owner so old URLs still
// resolve correctly while the canonical page moves (the `$owner/$slug`
// loader already redirects to the canonical owner handle on read).
const aliases = await listSkillSlugAliasesForSkill(ctx, skill._id);
for (const alias of aliases) {
await ctx.db.patch(alias._id, {
ownerPublisherId,
ownerUserId: userId,
updatedAt: now,
});
}
await ctx.db.insert("auditLogs", {
actorUserId: userId,
action: "skill.ownership.migrate",
@@ -9974,21 +9942,30 @@ export const insertVersion = internalMutation({
callerProviderAccountId,
)
) {
await ctx.db.patch(skill._id, {
await transferSkillOwnershipAndEmbeddings(ctx, {
skill,
ownerUserId: userId,
ownerPublisherId,
updatedAt: now,
now,
});
skill = { ...skill, ownerUserId: userId, ownerPublisherId };
skill = {
...skill,
ownerUserId: userId,
ownerPublisherId,
lastReviewedAt: now,
updatedAt: now,
};
} else {
throw new ConvexError(slugTakenMessage);
}
} else if (skill && !skill.ownerPublisherId) {
await ctx.db.patch(skill._id, {
await transferSkillOwnershipAndEmbeddings(ctx, {
skill,
ownerUserId: userId,
ownerPublisherId,
updatedAt: now,
now,
});
skill = { ...skill, ownerPublisherId };
skill = { ...skill, ownerPublisherId, lastReviewedAt: now, updatedAt: now };
}
const qualityAssessment = args.qualityAssessment;
+2
View File
@@ -164,6 +164,8 @@ Use dual fields during rollout:
administer both the current owner and destination publisher. User-to-user skill
transfers remain recipient-accepted unless the actor controls the destination
publisher.
- No ownership transfer path should move a skill while it is hidden, removed,
suspicious, or malicious; the artifact must be cleared by moderation first.
## Naming Rules