From 005f32ee1cc1e32b7e3da575b8c5a3c3cea526ec Mon Sep 17 00:00:00 2001 From: vyctorbrzezowski Date: Fri, 15 May 2026 23:16:12 -0300 Subject: [PATCH 1/5] fix: abort stale unban skill restore batches --- CHANGELOG.md | 1 + convex/skills.banBatches.test.ts | 207 +++++++++++++++++++++++++++++++ convex/skills.ts | 23 +++- specs/security-moderation.md | 14 +++ 4 files changed, 244 insertions(+), 1 deletion(-) create mode 100644 convex/skills.banBatches.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e3ef4568ed..8198234866 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,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). ## 0.17.0 - 2026-05-19 diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts new file mode 100644 index 0000000000..1f00b2e8bf --- /dev/null +++ b/convex/skills.banBatches.test.ts @@ -0,0 +1,207 @@ +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 } + > +)._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 }), + }), + }), + }; + } + 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("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("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("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 bd4d36aadd..52a37a4f1c 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6711,7 +6711,22 @@ export const applyBanToOwnedSkillsBatchInternal = internalMutation({ let hiddenCount = 0; for (const skill of page) { - if (skill.softDeletedAt) continue; + if (skill.softDeletedAt) { + if ( + skill.moderationStatus === "hidden" && + 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. @@ -6828,6 +6843,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 @@ -6844,6 +6864,7 @@ export const restoreOwnedSkillsForUnbanBatchInternal = internalMutation({ if ( !skill.softDeletedAt || skill.softDeletedAt !== args.bannedAt || + skill.moderationStatus !== "hidden" || skill.moderationReason !== "user.banned" ) { continue; diff --git a/specs/security-moderation.md b/specs/security-moderation.md index ce59f661e2..f9c72dad06 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). @@ -192,11 +202,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 From 11cbb4719523573e21bfa226190e762854f8d6c0 Mon Sep 17 00:00:00 2001 From: vyctorbrzezowski Date: Sat, 16 May 2026 19:22:50 -0300 Subject: [PATCH 2/5] fix(api): keep ban restore markers monotonic --- convex/skills.banBatches.test.ts | 31 +++++++++++++++++++++++++++++++ convex/skills.ts | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts index 1f00b2e8bf..dcbf13c1b5 100644 --- a/convex/skills.banBatches.test.ts +++ b/convex/skills.banBatches.test.ts @@ -133,6 +133,37 @@ describe("skills ban/unban batches", () => { 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 unban restore pages when the owner was banned again", async () => { const { ctx, patch, query, scheduler } = makeCtx({ user: { _id: "users:owner", deletedAt: 2_000 }, diff --git a/convex/skills.ts b/convex/skills.ts index 52a37a4f1c..e54ad1d56d 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6715,7 +6715,7 @@ export const applyBanToOwnedSkillsBatchInternal = internalMutation({ if ( skill.moderationStatus === "hidden" && skill.moderationReason === "user.banned" && - skill.softDeletedAt !== args.bannedAt + skill.softDeletedAt < args.bannedAt ) { await ctx.db.patch(skill._id, { softDeletedAt: args.bannedAt, From cb1143e133a6390a4eea8ae2086e66eb0f38f2c1 Mon Sep 17 00:00:00 2001 From: vyctorbrzezowski Date: Mon, 18 May 2026 17:44:12 -0300 Subject: [PATCH 3/5] fix(api): restore legacy ban-hidden skills on unban --- convex/skills.banBatches.test.ts | 46 ++++++++++++++++++++++++++++++++ convex/skills.ts | 2 +- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts index dcbf13c1b5..5cc68ffb2d 100644 --- a/convex/skills.banBatches.test.ts +++ b/convex/skills.banBatches.test.ts @@ -40,6 +40,13 @@ function makeCtx({ }), }; } + if (table === "skillEmbeddings") { + return { + withIndex: () => ({ + collect: async () => [], + }), + }; + } throw new Error(`Unexpected table ${table}`); }); const scheduler = { runAfter: vi.fn() }; @@ -212,6 +219,45 @@ describe("skills ban/unban batches", () => { 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 }, diff --git a/convex/skills.ts b/convex/skills.ts index e54ad1d56d..50dbee4140 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6864,7 +6864,7 @@ export const restoreOwnedSkillsForUnbanBatchInternal = internalMutation({ if ( !skill.softDeletedAt || skill.softDeletedAt !== args.bannedAt || - skill.moderationStatus !== "hidden" || + skill.moderationStatus === "removed" || skill.moderationReason !== "user.banned" ) { continue; From f8f9e8198ee95d2bc2657143cffb0bf73dd4a76e Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Wed, 27 May 2026 13:07:05 -0500 Subject: [PATCH 4/5] fix: retimestamp legacy ban-hidden skills on re-ban --- convex/skills.banBatches.test.ts | 69 ++++++++++++++++++++++++++++++++ convex/skills.ts | 4 +- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts index 5cc68ffb2d..9934fa1a81 100644 --- a/convex/skills.banBatches.test.ts +++ b/convex/skills.banBatches.test.ts @@ -111,6 +111,75 @@ describe("skills ban/unban batches", () => { 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 }, diff --git a/convex/skills.ts b/convex/skills.ts index 466cad3c45..8189a0c871 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6992,8 +6992,10 @@ export const applyBanToOwnedSkillsBatchInternal = internalMutation({ let hiddenCount = 0; for (const skill of page) { if (skill.softDeletedAt) { + const isBanHiddenStatus = + skill.moderationStatus === "hidden" || skill.moderationStatus === undefined; if ( - skill.moderationStatus === "hidden" && + isBanHiddenStatus && skill.moderationReason === "user.banned" && skill.softDeletedAt < args.bannedAt ) { From 81c95fe84e2d516ca08b631a597e73341bdf5fc7 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Wed, 27 May 2026 13:48:18 -0500 Subject: [PATCH 5/5] fix: abort stale scheduled skill ban pages --- convex/skills.banBatches.test.ts | 36 +++++++++++++++++++++++++++++++- convex/skills.ts | 7 +++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/convex/skills.banBatches.test.ts b/convex/skills.banBatches.test.ts index 9934fa1a81..017131ab0b 100644 --- a/convex/skills.banBatches.test.ts +++ b/convex/skills.banBatches.test.ts @@ -18,7 +18,7 @@ const restoreUnbanHandler = ( const applyBanHandler = ( applyBanToOwnedSkillsBatchInternal as unknown as WrappedHandler< { ownerUserId: string; bannedAt: number; hiddenBy?: string; cursor?: string }, - { hiddenCount: number; scheduled: boolean } + { hiddenCount: number; scheduled: boolean; aborted?: boolean } > )._handler; @@ -240,6 +240,40 @@ describe("skills ban/unban batches", () => { 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 }, diff --git a/convex/skills.ts b/convex/skills.ts index 8189a0c871..7e7a8b4a4a 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -6980,6 +6980,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))