diff --git a/CHANGELOG.md b/CHANGELOG.md index 34beb41708..db51965c6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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). +- Web: stop stale unban restore batches from reactivating skills after the owner is banned again or deactivated (thanks @vyctorbrzezowski). - 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). diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts new file mode 100644 index 0000000000..017131ab0b --- /dev/null +++ b/convex/skills.banBatches.test.ts @@ -0,0 +1,387 @@ +import { describe, expect, it, vi } from "vitest"; +import { + applyBanToOwnedSkillsBatchInternal, + restoreOwnedSkillsForUnbanBatchInternal, +} from "./skills"; + +type WrappedHandler = { + _handler: (ctx: unknown, args: TArgs) => Promise; +}; + +const restoreUnbanHandler = ( + restoreOwnedSkillsForUnbanBatchInternal as unknown as WrappedHandler< + { ownerUserId: string; bannedAt: number; cursor?: string }, + { restoredCount: number; scheduled: boolean; aborted?: boolean } + > +)._handler; + +const applyBanHandler = ( + applyBanToOwnedSkillsBatchInternal as unknown as WrappedHandler< + { ownerUserId: string; bannedAt: number; hiddenBy?: string; cursor?: string }, + { hiddenCount: number; scheduled: boolean; aborted?: boolean } + > +)._handler; + +function makeCtx({ + user, + skills = [], +}: { + user: Record | null; + skills?: Array>; +}) { + const patch = vi.fn(); + const query = vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: () => ({ + order: () => ({ + paginate: async () => ({ page: skills, isDone: true, continueCursor: null }), + }), + }), + }; + } + if (table === "skillEmbeddings") { + return { + withIndex: () => ({ + collect: async () => [], + }), + }; + } + throw new Error(`Unexpected table ${table}`); + }); + const scheduler = { runAfter: vi.fn() }; + return { + ctx: { + db: { + get: vi.fn(async (id: string) => (id === "users:owner" ? user : null)), + insert: vi.fn(), + patch, + replace: vi.fn(), + delete: vi.fn(), + query, + normalizeId: vi.fn(), + }, + scheduler, + } as never, + patch, + query, + scheduler, + }; +} + +describe("skills ban/unban batches", () => { + it("retimestamps earlier ban-hidden skills during a later ban", async () => { + const { ctx, patch, scheduler } = makeCtx({ + user: { _id: "users:owner", deletedAt: 2_000 }, + skills: [ + { + _id: "skills:hidden", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: "hidden", + moderationReason: "user.banned", + hiddenAt: 1_000, + hiddenBy: "users:first-moderator", + }, + ], + }); + + await expect( + applyBanHandler(ctx, { + ownerUserId: "users:owner", + bannedAt: 2_000, + hiddenBy: "users:second-moderator", + }), + ).resolves.toEqual({ + ok: true, + hiddenCount: 0, + scheduled: false, + }); + + expect(patch).toHaveBeenCalledWith( + "skills:hidden", + expect.objectContaining({ + softDeletedAt: 2_000, + hiddenAt: 2_000, + hiddenBy: "users:second-moderator", + lastReviewedAt: 2_000, + updatedAt: 2_000, + }), + ); + expect(scheduler.runAfter).not.toHaveBeenCalled(); + }); + + it("retimestamps legacy ban-hidden skills so a final unban restores them after a re-ban", async () => { + const legacySkill = { + _id: "skills:legacy-hidden", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: undefined, + moderationReason: "user.banned", + hiddenAt: 1_000, + hiddenBy: "users:first-moderator", + stats: { + downloads: 0, + stars: 0, + comments: 0, + versions: 1, + installsCurrent: 0, + installsAllTime: 0, + }, + }; + const { ctx: banCtx, patch: banPatch } = makeCtx({ + user: { _id: "users:owner", deletedAt: 2_000 }, + skills: [legacySkill], + }); + + await expect( + applyBanHandler(banCtx, { + ownerUserId: "users:owner", + bannedAt: 2_000, + hiddenBy: "users:second-moderator", + }), + ).resolves.toEqual({ + ok: true, + hiddenCount: 0, + scheduled: false, + }); + + expect(banPatch).toHaveBeenCalledWith( + "skills:legacy-hidden", + expect.objectContaining({ + softDeletedAt: 2_000, + hiddenAt: 2_000, + hiddenBy: "users:second-moderator", + lastReviewedAt: 2_000, + updatedAt: 2_000, + }), + ); + + const retimestampPatch = banPatch.mock.calls[0]?.[1] ?? {}; + const { ctx: unbanCtx, patch: unbanPatch } = makeCtx({ + user: { _id: "users:owner", deletedAt: undefined, deactivatedAt: undefined }, + skills: [{ ...legacySkill, ...retimestampPatch }], + }); + + await expect( + restoreUnbanHandler(unbanCtx, { ownerUserId: "users:owner", bannedAt: 2_000 }), + ).resolves.toMatchObject({ + restoredCount: 1, + scheduled: false, + }); + + expect(unbanPatch).toHaveBeenCalledWith( + "skills:legacy-hidden", + expect.objectContaining({ + softDeletedAt: undefined, + moderationStatus: "active", + moderationReason: "restored.unban", + }), + ); + }); + + it("does not retimestamp removed ban-hidden skills during a later ban", async () => { + const { ctx, patch } = makeCtx({ + user: { _id: "users:owner", deletedAt: 2_000 }, + skills: [ + { + _id: "skills:removed", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: "removed", + moderationReason: "user.banned", + hiddenAt: 1_000, + }, + ], + }); + + await expect( + applyBanHandler(ctx, { + ownerUserId: "users:owner", + bannedAt: 2_000, + hiddenBy: "users:second-moderator", + }), + ).resolves.toMatchObject({ + hiddenCount: 0, + scheduled: false, + }); + + expect(patch).not.toHaveBeenCalledWith("skills:removed", expect.anything()); + }); + + it("does not roll newer ban markers back when stale ban pages run late", async () => { + const { ctx, patch } = makeCtx({ + user: { _id: "users:owner", deletedAt: 1_000 }, + skills: [ + { + _id: "skills:hidden", + ownerUserId: "users:owner", + softDeletedAt: 2_000, + moderationStatus: "hidden", + moderationReason: "user.banned", + hiddenAt: 2_000, + hiddenBy: "users:second-moderator", + }, + ], + }); + + await expect( + applyBanHandler(ctx, { + ownerUserId: "users:owner", + bannedAt: 1_000, + hiddenBy: "users:first-moderator", + }), + ).resolves.toEqual({ + ok: true, + hiddenCount: 0, + scheduled: false, + }); + + expect(patch).not.toHaveBeenCalledWith("skills:hidden", expect.anything()); + }); + + it("aborts stale scheduled ban pages after the owner is unbanned", async () => { + const { ctx, patch, query, scheduler } = makeCtx({ + user: { _id: "users:owner", deletedAt: undefined, deactivatedAt: undefined }, + skills: [ + { + _id: "skills:hidden", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: "hidden", + moderationReason: "user.banned", + hiddenAt: 1_000, + }, + ], + }); + + await expect( + applyBanHandler(ctx, { + ownerUserId: "users:owner", + bannedAt: 2_000, + hiddenBy: "users:second-moderator", + cursor: "next-page", + }), + ).resolves.toEqual({ + ok: true, + hiddenCount: 0, + scheduled: false, + aborted: true, + }); + + expect(query).not.toHaveBeenCalled(); + expect(patch).not.toHaveBeenCalled(); + expect(scheduler.runAfter).not.toHaveBeenCalled(); + }); + + it("aborts stale unban restore pages when the owner was banned again", async () => { + const { ctx, patch, query, scheduler } = makeCtx({ + user: { _id: "users:owner", deletedAt: 2_000 }, + skills: [ + { + _id: "skills:hidden", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationReason: "user.banned", + }, + ], + }); + + await expect( + restoreUnbanHandler(ctx, { + ownerUserId: "users:owner", + bannedAt: 1_000, + cursor: "next-page", + }), + ).resolves.toEqual({ + ok: true, + restoredCount: 0, + scheduled: false, + aborted: true, + }); + + expect(query).not.toHaveBeenCalled(); + expect(patch).not.toHaveBeenCalled(); + expect(scheduler.runAfter).not.toHaveBeenCalled(); + }); + + it("continues unban restore pages while the owner is active", async () => { + const { ctx, query, scheduler } = makeCtx({ + user: { _id: "users:owner", deletedAt: undefined, deactivatedAt: undefined }, + }); + + await expect( + restoreUnbanHandler(ctx, { ownerUserId: "users:owner", bannedAt: 1_000 }), + ).resolves.toEqual({ + ok: true, + restoredCount: 0, + scheduled: false, + }); + + expect(query).toHaveBeenCalledWith("skills"); + expect(scheduler.runAfter).not.toHaveBeenCalled(); + }); + + it("restores legacy ban-hidden skills without moderationStatus", async () => { + const { ctx, patch } = makeCtx({ + user: { _id: "users:owner", deletedAt: undefined, deactivatedAt: undefined }, + skills: [ + { + _id: "skills:legacy-hidden", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: undefined, + moderationReason: "user.banned", + stats: { + downloads: 0, + stars: 0, + comments: 0, + versions: 1, + installsCurrent: 0, + installsAllTime: 0, + }, + }, + ], + }); + + await expect( + restoreUnbanHandler(ctx, { ownerUserId: "users:owner", bannedAt: 1_000 }), + ).resolves.toMatchObject({ + restoredCount: 1, + scheduled: false, + }); + + expect(patch).toHaveBeenCalledWith( + "skills:legacy-hidden", + expect.objectContaining({ + softDeletedAt: undefined, + moderationStatus: "active", + moderationReason: "restored.unban", + }), + ); + }); + + it("does not restore removed ban-hidden skills", async () => { + const { ctx, patch } = makeCtx({ + user: { _id: "users:owner", deletedAt: undefined, deactivatedAt: undefined }, + skills: [ + { + _id: "skills:removed", + ownerUserId: "users:owner", + softDeletedAt: 1_000, + moderationStatus: "removed", + moderationReason: "user.banned", + }, + ], + }); + + await expect( + restoreUnbanHandler(ctx, { ownerUserId: "users:owner", bannedAt: 1_000 }), + ).resolves.toMatchObject({ + restoredCount: 0, + scheduled: false, + }); + + expect(patch).not.toHaveBeenCalledWith("skills:removed", expect.anything()); + }); +}); diff --git a/convex/skills.ts b/convex/skills.ts index 832815748d..a923627f85 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6985,6 +6985,13 @@ export const applyBanToOwnedSkillsBatchInternal = internalMutation({ cursor: v.optional(v.string()), }, handler: async (ctx, args) => { + if (args.cursor) { + const owner = await ctx.db.get(args.ownerUserId); + if (!owner || owner.deletedAt !== args.bannedAt || owner.deactivatedAt) { + return { ok: true as const, hiddenCount: 0, scheduled: false, aborted: true }; + } + } + const { page, isDone, continueCursor } = await ctx.db .query("skills") .withIndex("by_owner", (q) => q.eq("ownerUserId", args.ownerUserId)) @@ -6996,7 +7003,24 @@ export const applyBanToOwnedSkillsBatchInternal = internalMutation({ let hiddenCount = 0; for (const skill of page) { - if (skill.softDeletedAt) continue; + if (skill.softDeletedAt) { + const isBanHiddenStatus = + skill.moderationStatus === "hidden" || skill.moderationStatus === undefined; + if ( + isBanHiddenStatus && + skill.moderationReason === "user.banned" && + skill.softDeletedAt < args.bannedAt + ) { + await ctx.db.patch(skill._id, { + softDeletedAt: args.bannedAt, + hiddenAt: args.bannedAt, + hiddenBy: args.hiddenBy, + lastReviewedAt: args.bannedAt, + updatedAt: args.bannedAt, + }); + } + continue; + } // Only overwrite moderation fields for active skills. Keep existing hidden/removed // moderation reasons intact. @@ -7113,6 +7137,11 @@ export const restoreOwnedSkillsForUnbanBatchInternal = internalMutation({ cursor: v.optional(v.string()), }, handler: async (ctx, args) => { + const user = await ctx.db.get(args.ownerUserId); + if (!user || user.deletedAt || user.deactivatedAt) { + return { ok: true as const, restoredCount: 0, scheduled: false, aborted: true }; + } + const now = Date.now(); const { page, isDone, continueCursor } = await ctx.db @@ -7129,6 +7158,7 @@ export const restoreOwnedSkillsForUnbanBatchInternal = internalMutation({ if ( !skill.softDeletedAt || skill.softDeletedAt !== args.bannedAt || + skill.moderationStatus === "removed" || skill.moderationReason !== "user.banned" ) { continue; diff --git a/specs/security-moderation.md b/specs/security-moderation.md index 6c1a9de2cd..923eb41c58 100644 --- a/specs/security-moderation.md +++ b/specs/security-moderation.md @@ -16,6 +16,16 @@ See also: [acceptable-usage.md](./acceptable-usage.md) for the marketplace polic - moderator: hide/restore skills, view hidden skills, unhide, soft-delete, ban users (except admins). - admin: all moderator actions + hard delete skills, change owners, change roles. +## Ban + unban batches + +- Ban/unban skill batches are paginated and may continue after the mutation that + started them has committed. +- Unban restore pages must re-read the owner before processing. If the owner is + missing, banned again, or deactivated, the stale page must abort without + restoring skills or scheduling another page. +- Restore pages only clear the exact `softDeletedAt` timestamp from the ban + being lifted and only for skills hidden with `moderationReason = "user.banned"`. + ## Reporting + auto-hide - Reports are unique per user + target (skill/comment/package). @@ -200,11 +210,15 @@ See also: [acceptable-usage.md](./acceptable-usage.md) for the marketplace polic - Banning a user: - hard-deletes all owned skills + - retimestamps already ban-hidden owned skills to the current ban marker so + a later matching unban can restore them - soft-deletes all authored skill comments + soul comments - revokes API tokens - sets `deletedAt` on the user - Admins can manually unban (`deletedAt` + `banReason` cleared); revoked API tokens stay revoked and should be recreated by the user. +- Stale unban restore batches must stop if the user was banned again before a + later page runs. - Optional ban reason is stored in `users.banReason` and audit logs. - Admins can reclassify an existing ban reason without unbanning or restoring content. This preserves the ban while removing users from remediation flows