From bc5fe5956b00bbd68d31c1df112abdae9e727cd4 Mon Sep 17 00:00:00 2001 From: Abdulkhalek Muhammad Date: Tue, 7 Apr 2026 14:30:18 +0200 Subject: [PATCH 1/2] docs(security): add bot hardening plans --- ...ec-bot-01-queryraw-guildid-verification.md | 157 +++++++++ ...-07-sec-bot-02-webhook-header-allowlist.md | 193 +++++++++++ ...ec-bot-03-sanitize-welcome-canvas-names.md | 307 ++++++++++++++++++ ...7-sec-bot-04-action-rule-name-whitelist.md | 275 ++++++++++++++++ ...solvetemplate-literal-only-verification.md | 133 ++++++++ ...sec-bot-06-emoji-validation-addreaction.md | 221 +++++++++++++ ...-07-sec-bot-07-autorole-toctou-handling.md | 207 ++++++++++++ ...-bot-08-levelup-mass-mention-prevention.md | 210 ++++++++++++ ...-07-sec-bot-09-warn-checkbotpermissions.md | 119 +++++++ 9 files changed, 1822 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-01-queryraw-guildid-verification.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-02-webhook-header-allowlist.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-03-sanitize-welcome-canvas-names.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-04-action-rule-name-whitelist.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-05-resolvetemplate-literal-only-verification.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-06-emoji-validation-addreaction.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-07-autorole-toctou-handling.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-08-levelup-mass-mention-prevention.md create mode 100644 docs/superpowers/plans/2026-04-07-sec-bot-09-warn-checkbotpermissions.md diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-01-queryraw-guildid-verification.md b/docs/superpowers/plans/2026-04-07-sec-bot-01-queryraw-guildid-verification.md new file mode 100644 index 0000000..7521546 --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-01-queryraw-guildid-verification.md @@ -0,0 +1,157 @@ +# Verify $queryRaw Tagged-Template Safety in actions/persistence.ts — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Critical (verify-only — no exploitable issue confirmed) +**Goal:** Prove the two `$queryRaw` call sites in `packages/systems/src/actions/persistence.ts` use Prisma's tagged-template form (which auto-parameterizes) rather than `$queryRawUnsafe` (which interpolates literally), then lock that guarantee in with a regression test and an explicit safety comment so future edits cannot silently regress to the unsafe form. +**Architecture:** `getAnalytics()` and `getLastFiredByGuild()` accept a `guildId` arg from API/command callers and feed it into a raw SQL string that targets `ActionLog`. Prisma's tagged-template `$queryRaw\`...${guildId}...\`` form sends parameters as bind variables, while `$queryRawUnsafe` concatenates them into the SQL string verbatim — a SQLi sink. We will add a test that injects a malicious guildId and asserts the row count is unchanged (proving Prisma escaped it), and annotate both call sites. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`packages/systems/src/actions/persistence.ts` lines 272-284 and 351-358 use `prisma.$queryRaw` with template-literal interpolation of a caller-supplied `guildId`. Verification reading the file confirms it uses the tagged-template form (`prisma.$queryRaw<...>\`...WHERE "guildId" = ${guildId}...\``), which Prisma turns into a parameterized query. However, there is no test that pins this guarantee, and the file has no comment warning future maintainers that switching to `$queryRawUnsafe` here would expose a SQLi sink. + +## Files + +- `packages/systems/src/actions/persistence.ts` (lines 260-358) +- `packages/systems/tests/integration/actions-sync.test.ts` (sibling — pattern reference) +- New: `packages/systems/tests/integration/actions-persistence-sqli.test.ts` + +## Tasks + +### Task 1: Add SQLi regression test for getAnalytics and getLastFiredByGuild + +- [ ] **Step 1: Write failing test** — create `packages/systems/tests/integration/actions-persistence-sqli.test.ts`: + +```typescript +/** + * Regression: prisma.$queryRaw in actions/persistence.ts MUST use the + * tagged-template form so guildId is bound as a parameter, not concatenated. + * If anyone switches these call sites to $queryRawUnsafe with string + * interpolation, the malicious guildId below would drop or corrupt rows + * and these assertions would fail. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterAll } from "vitest"; +import { + setupTestDatabase, + cleanTestData, + teardownTestDatabase, +} from "../helpers/db.js"; +import { getPrisma } from "@fluxcore/database"; +import { + getAnalytics, + getLastFiredByGuild, +} from "../../src/actions/persistence.js"; + +const SAFE_GUILD = "guild-safe-1"; +const MALICIOUS_GUILD = `guild-x'; DROP TABLE "ActionLog"; --`; + +describe("actions persistence — $queryRaw SQLi safety", () => { + beforeAll(async () => { + await setupTestDatabase(); + }); + + beforeEach(async () => { + await cleanTestData(); + const prisma = getPrisma(); + await prisma.actionRule.create({ + data: { + guildId: SAFE_GUILD, + name: "rule-1", + enabled: true, + eventType: "memberJoin", + actions: "[]", + conditions: "{}", + priority: 0, + createdBy: "user-1", + }, + }); + await prisma.actionLog.create({ + data: { + guildId: SAFE_GUILD, + ruleId: 0, + ruleName: "rule-1", + eventType: "memberJoin", + actionType: "sendMessage", + success: true, + error: null, + metadata: "{}", + }, + }); + }); + + afterAll(async () => { + await teardownTestDatabase(); + }); + + it("getAnalytics treats malicious guildId as a literal value, not SQL", async () => { + const result = await getAnalytics(MALICIOUS_GUILD, 7); + expect(result.summary.totalExecutions).toBe(0); + expect(result.executionTrend).toEqual([]); + + // ActionLog table must still exist and still contain the safe row + const prisma = getPrisma(); + const stillThere = await prisma.actionLog.count({ + where: { guildId: SAFE_GUILD }, + }); + expect(stillThere).toBe(1); + }); + + it("getLastFiredByGuild treats malicious guildId as a literal value, not SQL", async () => { + const map = await getLastFiredByGuild(MALICIOUS_GUILD); + expect(map.size).toBe(0); + + const prisma = getPrisma(); + const stillThere = await prisma.actionLog.count({ + where: { guildId: SAFE_GUILD }, + }); + expect(stillThere).toBe(1); + }); + + it("getAnalytics returns real data for the legitimate guildId", async () => { + const result = await getAnalytics(SAFE_GUILD, 7); + expect(result.summary.totalExecutions).toBe(1); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose -f docker-compose.test.yml run --rm test pnpm test:integration packages/systems/tests/integration/actions-persistence-sqli.test.ts`. Expected: tests PASS immediately (the tagged-template form is already safe). If any test FAILS, that means the call site is unsafe and Task 2 must convert it to a Prisma query builder before re-running. + +- [ ] **Step 3: Implement** — annotate both call sites in `packages/systems/src/actions/persistence.ts` so future maintainers do not regress. Replace the `getAnalytics` raw block (around line 272) by adding a comment immediately above the `prisma.$queryRaw` call: + +```typescript + // SECURITY: tagged-template `$queryRaw` form. Prisma binds ${guildId} + // and ${since} as parameters — DO NOT switch this to $queryRawUnsafe + // or string-concatenate values into the SQL. Covered by + // tests/integration/actions-persistence-sqli.test.ts. + prisma.$queryRaw< + Array<{ date: string; total: bigint; success: bigint; error: bigint }> + >` +``` + +And likewise above the `getLastFiredByGuild` block (around line 351): + +```typescript + // SECURITY: tagged-template `$queryRaw` form — guildId is bound as a + // parameter. DO NOT convert to $queryRawUnsafe. Covered by + // tests/integration/actions-persistence-sqli.test.ts. + const rows = await prisma.$queryRaw< + Array<{ ruleId: number; lastFired: Date }> + >` +``` + +- [ ] **Step 4: Run** — `docker compose -f docker-compose.test.yml run --rm test pnpm test:integration packages/systems/tests/integration/actions-persistence-sqli.test.ts` and `docker compose run --rm bot pnpm typecheck`. Expected: tests PASS, typecheck PASSES. + +- [ ] **Step 5: Commit** — + +``` +test(actions): pin $queryRaw guildId binding with SQLi regression test + +Adds an integration test that proves prisma.$queryRaw in +actions/persistence.ts uses the tagged-template (parameterized) form +and is not vulnerable to SQL injection via guildId. Annotates both +call sites with a SECURITY comment so future edits cannot silently +regress to $queryRawUnsafe. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-02-webhook-header-allowlist.md b/docs/superpowers/plans/2026-04-07-sec-bot-02-webhook-header-allowlist.md new file mode 100644 index 0000000..8b969ed --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-02-webhook-header-allowlist.md @@ -0,0 +1,193 @@ +# Webhook Header Denylist → Strict Allowlist — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** High +**Goal:** Replace the incomplete `BLOCKED_HEADERS` denylist in the `sendWebhook` action executor with a strict allowlist of permitted header names so guild moderators cannot smuggle credentials, auth bypass, or proxy headers into outbound webhook calls. +**Architecture:** `executors.set("sendWebhook", ...)` in `apps/bot/src/features/automation/system/registry.ts` accepts `config.webhook.headers` (a `Record` configured per action rule via dashboard or `/actions` command) and merges them into the outgoing fetch headers. Today it filters out a small set of hop-by-hop headers via a denylist, but `authorization`, `x-api-key`, `cookie` (already blocked), and proxy/forwarding headers are not all covered, and any new sensitive header would silently pass through. Switch to an explicit allowlist of headers we know are safe to forward (caching, content negotiation, idempotency keys, custom `x-fluxcore-*` headers). +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`apps/bot/src/features/automation/system/registry.ts` (lines 137-201) defines: + +```typescript +const BLOCKED_HEADERS = new Set([ + "host", "cookie", "set-cookie", "transfer-encoding", + "connection", "proxy-authorization", "te", "trailer", + "upgrade", +]); +``` + +A guild admin (or compromised dashboard session with `ManageGuild`) can configure a `sendWebhook` action with `headers: { "Authorization": "Bearer stolen-token" }` or `"X-Forwarded-For": "127.0.0.1"` and the bot will faithfully forward them. This permits credential injection toward third-party services, reflected SSRF probes, and bypass of upstream IP-allowlist checks. Denylists are unsafe by design. + +## Files + +- `apps/bot/src/features/automation/system/registry.ts` (lines 137-201) +- `apps/bot/tests/features/automation/system/registry-webhook.test.ts` (new) + +## Tasks + +### Task 1: Replace denylist with strict allowlist + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/features/automation/system/registry-webhook.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn().mockResolvedValue({ address: "93.184.216.34", family: 4 }), +})); + +const { getExecutor } = await import( + "../../../../src/features/automation/system/registry.js" +); + +const baseCtx = { + eventType: "memberJoin" as const, + guildId: "g1", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + guildName: "G", + memberCount: 10, + timestamp: new Date().toISOString(), +}; + +describe("sendWebhook header allowlist", () => { + let fetchSpy: ReturnType; + + beforeEach(() => { + fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response("", { status: 204 }), + ); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + async function run(headers: Record) { + const executor = getExecutor("sendWebhook")!; + await executor({} as never, baseCtx as never, { + type: "sendWebhook", + webhook: { + url: "https://example.com/hook", + method: "POST", + headers, + bodyTemplate: "{}", + }, + } as never); + const call = fetchSpy.mock.calls[0]; + return call?.[1]?.headers as Record; + } + + it("strips Authorization header", async () => { + const sent = await run({ Authorization: "Bearer leaked" }); + expect(sent).toBeDefined(); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain("authorization"); + }); + + it("strips X-Api-Key header", async () => { + const sent = await run({ "X-Api-Key": "secret" }); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain("x-api-key"); + }); + + it("strips X-Forwarded-For and X-Forwarded-Host", async () => { + const sent = await run({ + "X-Forwarded-For": "127.0.0.1", + "X-Forwarded-Host": "internal", + }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).not.toContain("x-forwarded-for"); + expect(lower).not.toContain("x-forwarded-host"); + }); + + it("strips Cookie header", async () => { + const sent = await run({ Cookie: "session=abc" }); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain("cookie"); + }); + + it("allows X-Idempotency-Key (allowlisted)", async () => { + const sent = await run({ "X-Idempotency-Key": "abc-123" }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).toContain("x-idempotency-key"); + }); + + it("allows custom X-Fluxcore-* headers (allowlisted prefix)", async () => { + const sent = await run({ "X-Fluxcore-Source": "rule-42" }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).toContain("x-fluxcore-source"); + }); + + it("always sets Content-Type: application/json", async () => { + const sent = await run({}); + expect(sent["Content-Type"] ?? sent["content-type"]).toBe("application/json"); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/automation/system/registry-webhook.test.ts`. Expected: FAIL (Authorization, X-Api-Key, X-Forwarded-* currently pass through; allowlisted ones currently also pass which is OK, but the strip tests fail). + +- [ ] **Step 3: Implement** — in `apps/bot/src/features/automation/system/registry.ts`, replace the denylist block (around lines 171-184) with a strict allowlist: + +```typescript + // Strict allowlist: only headers that are safe for the bot to forward on + // behalf of a guild admin. Denylists are unsafe — any new sensitive + // header (Authorization, X-Api-Key, X-Forwarded-*, Cookie, etc.) would + // silently leak. Add to this set only after a security review. + const ALLOWED_HEADERS = new Set([ + "accept", + "accept-language", + "cache-control", + "user-agent", + "x-idempotency-key", + "x-request-id", + ]); + const ALLOWED_PREFIX = "x-fluxcore-"; + + const userHeaders = config.webhook.headers ?? {}; + const headers: Record = { + "Content-Type": "application/json", + }; + for (const [key, value] of Object.entries(userHeaders)) { + const lower = key.toLowerCase(); + if (lower === "content-type") continue; // we set this ourselves + if (ALLOWED_HEADERS.has(lower) || lower.startsWith(ALLOWED_PREFIX)) { + headers[key] = value; + } else { + logger.warn( + `sendWebhook: dropped non-allowlisted header "${key}" for guild ${ctx.guildId ?? "unknown"}`, + ); + } + } +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/automation/system/registry-webhook.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(actions): replace webhook header denylist with strict allowlist + +Authorization, X-Api-Key, X-Forwarded-*, and any future sensitive +header could previously be smuggled through sendWebhook because the +filter was a denylist. Switch to an explicit allowlist (Accept, +Cache-Control, User-Agent, X-Idempotency-Key, X-Request-Id, and the +X-Fluxcore-* prefix) and log+drop everything else. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-03-sanitize-welcome-canvas-names.md b/docs/superpowers/plans/2026-04-07-sec-bot-03-sanitize-welcome-canvas-names.md new file mode 100644 index 0000000..fe792ab --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-03-sanitize-welcome-canvas-names.md @@ -0,0 +1,307 @@ +# Sanitize Usernames Before Welcome Canvas Render — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** High +**Goal:** Strip zero-width, RTL/LTR override, and other invisible Unicode control characters from `member.user.username`, `member.displayName`, and `guild.name` before they are passed to the welcome image renderer, and cap their length, so a hostile member name cannot deface the welcome card, mojibake other guilds' members, or trigger pathological canvas measurements. +**Architecture:** `apps/bot/src/events/guildMemberAdd.ts` calls `generateWelcomeImage({ member: { username, displayName, avatarUrl }, guild: { name, ... }, ... })` (lines 152-165). The renderer (`packages/systems/src/welcome/image/renderer.ts`) interpolates these strings into template substitution and into a canvas `fillText` call. The fix introduces a `sanitizeDisplayName(raw, maxLen)` helper in `packages/systems/src/welcome/image/index.ts` that (a) removes Unicode control category chars except space, (b) removes the dangerous bidi override characters (U+202A..U+202E, U+2066..U+2069), (c) collapses zero-width chars (U+200B..U+200D, U+2060, U+FEFF), (d) normalizes to NFC, and (e) truncates to a fixed graphemes-safe length. The handler calls it on the three fields before constructing the render input. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`apps/bot/src/events/guildMemberAdd.ts` lines 155-162 pass raw Discord-supplied strings into the canvas pipeline: + +```typescript +member: { + username: member.user.username, + displayName: member.displayName, + avatarUrl: member.user.displayAvatarURL({ extension: "png", size: 256 }), +}, +guild: { + name: member.guild.name, + ... +}, +``` + +A malicious member can set their display name to e.g. `"\u202Eevil\u202D"` to flip the welcome banner right-to-left, or `"A\u200B".repeat(500)` to inflate canvas measurement work and render time. There is no length cap and no character-class filter. The renderer trusts the input. + +## Files + +- `packages/systems/src/welcome/image/index.ts` (add `sanitizeDisplayName` export) +- New: `packages/systems/src/welcome/image/sanitize.ts` +- `apps/bot/src/events/guildMemberAdd.ts` (lines 149-165) +- New: `packages/systems/tests/unit/welcome/sanitize.test.ts` +- New: `apps/bot/tests/events/welcome-sanitize.test.ts` + +## Tasks + +### Task 1: Add sanitizeDisplayName helper with unit tests + +- [ ] **Step 1: Write failing test** — create `packages/systems/tests/unit/welcome/sanitize.test.ts`: + +```typescript +import { describe, it, expect } from "vitest"; +import { sanitizeDisplayName } from "../../../src/welcome/image/sanitize.js"; + +describe("sanitizeDisplayName", () => { + it("returns plain ASCII unchanged", () => { + expect(sanitizeDisplayName("Alice", 32)).toBe("Alice"); + }); + + it("strips zero-width spaces and joiners", () => { + expect(sanitizeDisplayName("a\u200Bb\u200Cc\u200Dd\uFEFFe", 32)).toBe("abcde"); + }); + + it("strips RTL/LTR override characters (U+202A..U+202E)", () => { + expect(sanitizeDisplayName("\u202Eevil\u202D", 32)).toBe("evil"); + }); + + it("strips bidi isolate characters (U+2066..U+2069)", () => { + expect(sanitizeDisplayName("\u2066hidden\u2069", 32)).toBe("hidden"); + }); + + it("strips C0/C1 control characters but keeps spaces", () => { + expect(sanitizeDisplayName("a\u0007 b\u0000c", 32)).toBe("a bc"); + }); + + it("truncates to maxLen", () => { + expect(sanitizeDisplayName("x".repeat(500), 32)).toHaveLength(32); + }); + + it("normalizes to NFC", () => { + // "é" as NFD (U+0065 U+0301) → NFC ("\u00E9") + const decomposed = "e\u0301"; + const out = sanitizeDisplayName(decomposed, 32); + expect(out).toBe("\u00E9"); + }); + + it("returns 'Unknown' for empty/whitespace-only input", () => { + expect(sanitizeDisplayName("", 32)).toBe("Unknown"); + expect(sanitizeDisplayName(" ", 32)).toBe("Unknown"); + expect(sanitizeDisplayName("\u200B\u200C", 32)).toBe("Unknown"); + }); + + it("collapses runs of whitespace", () => { + expect(sanitizeDisplayName("a b\t\tc", 32)).toBe("a b c"); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/welcome/sanitize.test.ts`. Expected: FAIL (module does not exist). + +- [ ] **Step 3: Implement** — create `packages/systems/src/welcome/image/sanitize.ts`: + +```typescript +/** + * Sanitize a Discord-supplied display name, username, or guild name before + * rendering it onto the welcome canvas. Removes control characters, + * zero-width chars, bidi overrides/isolates, normalizes to NFC, collapses + * whitespace, and hard-truncates to maxLen UTF-16 code units. + */ + +// Bidi override + isolate ranges +const BIDI_OVERRIDES = /[\u202A-\u202E\u2066-\u2069]/g; +// Zero-width chars +const ZERO_WIDTH = /[\u200B-\u200D\u2060\uFEFF]/g; +// C0 + C1 control characters EXCEPT \t \n \r (we still collapse them later) +const CONTROL_CHARS = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/g; +// Whitespace runs (after we've stripped controls) +const WHITESPACE_RUN = /\s+/g; + +export function sanitizeDisplayName(raw: string, maxLen: number): string { + if (typeof raw !== "string") return "Unknown"; + + let out = raw + .normalize("NFC") + .replace(BIDI_OVERRIDES, "") + .replace(ZERO_WIDTH, "") + .replace(CONTROL_CHARS, "") + .replace(WHITESPACE_RUN, " ") + .trim(); + + if (out.length === 0) return "Unknown"; + if (out.length > maxLen) out = out.slice(0, maxLen); + return out; +} +``` + +Then export it from `packages/systems/src/welcome/image/index.ts` by adding: + +```typescript +// Sanitization +export { sanitizeDisplayName } from "./sanitize.js"; +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/welcome/sanitize.test.ts`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +feat(welcome): add sanitizeDisplayName helper for canvas inputs + +Strips zero-width chars, bidi overrides/isolates, control characters, +normalizes to NFC, collapses whitespace, and truncates. Used by the +welcome image pipeline to neutralize hostile usernames. +``` + +### Task 2: Apply sanitizer in guildMemberAdd before generateWelcomeImage + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/events/welcome-sanitize.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/logging/config", () => ({ + getLogConfig: vi.fn().mockResolvedValue(null), +})); +vi.mock("@fluxcore/systems/logging/persistence", () => ({ + createLogEntry: vi.fn(), +})); +vi.mock("@fluxcore/systems/logging/sender", () => ({ + sendLogEmbed: vi.fn(), +})); +vi.mock("@fluxcore/systems/logging/formatter", () => ({ + formatMemberJoin: vi.fn(), +})); + +vi.mock("@fluxcore/systems/antiraid/config", () => ({ + getAntiRaidConfig: vi.fn().mockResolvedValue({ enabled: false }), +})); +vi.mock("@fluxcore/systems/antiraid/tracker", () => ({ recordJoin: vi.fn() })); +vi.mock("@fluxcore/systems/antiraid/actions", () => ({ + executeRaidAction: vi.fn(), + lockdownGuild: vi.fn(), +})); +vi.mock("@fluxcore/systems/antiraid/persistence", () => ({ + createRaidEvent: vi.fn(), +})); + +vi.mock("@fluxcore/systems/welcome/config", () => ({ + getWelcomeConfig: vi.fn().mockResolvedValue({ + autoRoleIds: [], + welcomeEnabled: true, + welcomeChannelId: "ch-welcome", + welcomeMessage: "hi", + welcomeImageEnabled: true, + welcomeImageConfig: { sendMode: "with" }, + dmEnabled: false, + dmMessage: "", + }), +})); + +vi.mock("@fluxcore/systems/welcome/builder", () => ({ + buildWelcomeEmbed: vi.fn().mockReturnValue({ + setImage: vi.fn().mockReturnThis(), + }), +})); + +const generateWelcomeImage = vi + .fn() + .mockResolvedValue(Buffer.from("image")); +vi.mock("@fluxcore/systems/welcome/image", () => ({ + generateWelcomeImage: (...args: unknown[]) => generateWelcomeImage(...args), + createStorageAdapter: vi.fn().mockReturnValue({}), +})); + +const event = ( + await import("../../src/events/guildMemberAdd.js") +).default; + +describe("guildMemberAdd: sanitizes hostile names before canvas render", () => { + beforeEach(() => { + generateWelcomeImage.mockClear(); + }); + + it("strips RTL override and zero-width from username/displayName/guild.name", async () => { + const sentChannel = { isTextBased: () => true, send: vi.fn() }; + const member = { + id: "m1", + user: { + bot: false, + username: "\u202Eevil\u202Duser", + createdTimestamp: Date.now() - 10 * 86_400_000, + displayAvatarURL: () => "https://cdn/avatar.png", + tag: "tag", + }, + displayName: "a\u200B".repeat(100), + guild: { + id: "g1", + name: "Server\u202Ename", + memberCount: 5, + channels: { cache: { get: () => sentChannel } }, + iconURL: () => null, + members: { me: null }, + roles: { cache: new Map() }, + }, + roles: { cache: new Map(), add: vi.fn() }, + send: vi.fn(), + } as never; + + await event.execute(member); + + expect(generateWelcomeImage).toHaveBeenCalledTimes(1); + const call = generateWelcomeImage.mock.calls[0][0]; + expect(call.member.username).toBe("eviluser"); + expect(call.member.displayName).not.toContain("\u200B"); + expect(call.member.displayName.length).toBeLessThanOrEqual(80); + expect(call.guild.name).toBe("Servername"); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/welcome-sanitize.test.ts`. Expected: FAIL (current handler passes raw values). + +- [ ] **Step 3: Implement** — edit `apps/bot/src/events/guildMemberAdd.ts`. Add the import at the top alongside the other welcome imports: + +```typescript +import { generateWelcomeImage, createStorageAdapter, sanitizeDisplayName } from "@fluxcore/systems/welcome/image"; +``` + +Then replace the `generateWelcomeImage({ ... })` call (lines 152-165) with: + +```typescript + const safeUsername = sanitizeDisplayName(member.user.username, 32); + const safeDisplayName = sanitizeDisplayName(member.displayName, 80); + const safeGuildName = sanitizeDisplayName(member.guild.name, 80); + const imageBuffer = await generateWelcomeImage({ + settings: welcomeConfig.welcomeImageConfig, + member: { + username: safeUsername, + displayName: safeDisplayName, + avatarUrl: member.user.displayAvatarURL({ extension: "png", size: 256 }), + }, + guild: { + name: safeGuildName, + iconUrl: member.guild.iconURL({ size: 256 }) ?? undefined, + memberCount: member.guild.memberCount, + }, + storage, + }); +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/welcome-sanitize.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(welcome): sanitize hostile usernames before canvas rendering + +Discord-supplied username, displayName, and guild.name are now passed +through sanitizeDisplayName before generateWelcomeImage, removing +zero-width chars, bidi overrides, and capping length so a malicious +member cannot deface the welcome banner or trigger pathological +canvas measurements. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-04-action-rule-name-whitelist.md b/docs/superpowers/plans/2026-04-07-sec-bot-04-action-rule-name-whitelist.md new file mode 100644 index 0000000..eb1fddb --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-04-action-rule-name-whitelist.md @@ -0,0 +1,275 @@ +# Whitelist Action Rule Names — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** High +**Goal:** Reject `/actions create` invocations whose `name` argument contains anything outside `[a-zA-Z0-9 _-]` or whose length is outside `1..50`, so rule names cannot smuggle markdown injection, mention syntax, code-fence escapes, or zero-width characters into autocomplete results, embeds, log lines, or audit messages. +**Architecture:** `apps/bot/src/features/general/commands/actions.ts` defines the `actions create` subcommand at line 67-129 with `setMaxLength(50)`. The user-supplied `name` flows into Prisma (`createRule({ name, ... })`), the autocomplete handler, embed builders (`successEmbed("Rule Created", ... **${name}** ...)`) , and `notifyCacheInvalidation`. There is no character-class validation. The fix introduces a `RULE_NAME_REGEX` constant in `packages/systems/src/actions/constants.ts` and a `handleCreate` guard that rejects with an `errorEmbed` when validation fails. The same guard runs in any future edit/lookup paths via a small helper. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`apps/bot/src/features/general/commands/actions.ts` line 70-76: + +```typescript +.addStringOption((opt) => + opt + .setName("name") + .setDescription("Unique name for this rule") + .setRequired(true) + .setMaxLength(50), +), +``` + +`handleCreate` at line 474 reads it back as `interaction.options.getString("name", true)` and writes it directly to the DB. A moderator can create a rule named `"@everyone"`, `"```ts\nrm -rf /\n```"`, or `"\u200Bhidden"` which then renders inside the success embed (`successEmbed("Rule Created", "**${name}** will execute...")`) and inside autocomplete output, causing UI confusion or unintended mentions. There is no allowlist. + +## Files + +- `packages/systems/src/actions/constants.ts` (add `RULE_NAME_REGEX` + helper) +- `apps/bot/src/features/general/commands/actions.ts` (lines 470-547, plus delete/toggle/view lookups optionally) +- New: `apps/bot/tests/features/general/commands/actions-name-validation.test.ts` + +## Tasks + +### Task 1: Add RULE_NAME_REGEX constant and isValidRuleName helper + +- [ ] **Step 1: Write failing test** — append to a new file `packages/systems/tests/unit/actions-name.test.ts`: + +```typescript +import { describe, it, expect } from "vitest"; +import { isValidRuleName, RULE_NAME_REGEX } from "../../src/actions/constants.js"; + +describe("isValidRuleName", () => { + it("accepts plain ASCII letters", () => { + expect(isValidRuleName("welcome")).toBe(true); + }); + it("accepts digits, spaces, underscores, hyphens", () => { + expect(isValidRuleName("rule_1 - test")).toBe(true); + }); + it("rejects empty string", () => { + expect(isValidRuleName("")).toBe(false); + }); + it("rejects > 50 chars", () => { + expect(isValidRuleName("a".repeat(51))).toBe(false); + }); + it("rejects @everyone", () => { + expect(isValidRuleName("@everyone")).toBe(false); + }); + it("rejects markdown backticks", () => { + expect(isValidRuleName("`code`")).toBe(false); + }); + it("rejects mention syntax", () => { + expect(isValidRuleName("<@123>")).toBe(false); + }); + it("rejects zero-width characters", () => { + expect(isValidRuleName("hi\u200Bthere")).toBe(false); + }); + it("RULE_NAME_REGEX matches the documented pattern", () => { + expect(RULE_NAME_REGEX.source).toBe("^[a-zA-Z0-9 _-]{1,50}$"); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/actions-name.test.ts`. Expected: FAIL (export missing). + +- [ ] **Step 3: Implement** — append to `packages/systems/src/actions/constants.ts`: + +```typescript +/** + * Allowed character set for action rule names. Restricts user-supplied + * names so they cannot inject markdown, mention syntax, code fences, or + * invisible characters into embeds, autocomplete output, or audit logs. + */ +export const RULE_NAME_REGEX = /^[a-zA-Z0-9 _-]{1,50}$/; + +export function isValidRuleName(name: string): boolean { + return typeof name === "string" && RULE_NAME_REGEX.test(name); +} +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/actions-name.test.ts`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +feat(actions): add RULE_NAME_REGEX and isValidRuleName helper + +Defines the allowlist character class for action rule names so the +slash command and dashboard can validate consistently. +``` + +### Task 2: Enforce isValidRuleName in /actions create handler + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/features/general/commands/actions-name-validation.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkPermissions: vi.fn().mockResolvedValue(true), + }; +}); + +const mockCreateRule = vi.fn(); +const mockGetRuleByName = vi.fn().mockResolvedValue(null); +const mockCountRules = vi.fn().mockResolvedValue(0); +vi.mock("@fluxcore/systems/actions/persistence", () => ({ + createRule: (...a: unknown[]) => mockCreateRule(...a), + updateRule: vi.fn(), + deleteRule: vi.fn(), + getRuleByName: (...a: unknown[]) => mockGetRuleByName(...a), + countRules: (...a: unknown[]) => mockCountRules(...a), + getRecentLogs: vi.fn(), + notifyCacheInvalidation: vi.fn(), +})); + +vi.mock("@fluxcore/systems/actions/cache", () => ({ + addRuleToCache: vi.fn(), + removeRuleFromCache: vi.fn(), + updateRuleInCache: vi.fn(), + getRulesForGuild: vi.fn().mockReturnValue([]), +})); + +vi.mock("@fluxcore/systems/actions/config", () => ({ + getGuildSettingsOrDefault: vi.fn().mockReturnValue({ + maxRules: 25, + globalEnabled: true, + logChannelId: null, + }), + setGuildSettings: vi.fn(), +})); + +const command = ( + await import("../../../../src/features/general/commands/actions.js") +).default; + +function mkInteraction(name: string) { + const replies: unknown[] = []; + return { + options: { + getSubcommand: () => "create", + getString: (k: string, _req?: boolean) => { + if (k === "name") return name; + if (k === "event") return "memberJoin"; + if (k === "action-type") return "sendMessage"; + return null; + }, + getInteger: () => null, + getBoolean: () => null, + getChannel: () => null, + getRole: () => null, + }, + user: { id: "u1" }, + guildId: "g1", + replies, + reply: (payload: unknown) => { + replies.push(payload); + return Promise.resolve(); + }, + } as never; +} + +describe("/actions create — name validation", () => { + beforeEach(() => { + mockCreateRule.mockReset(); + mockGetRuleByName.mockResolvedValue(null); + mockCountRules.mockResolvedValue(0); + }); + + it("rejects @everyone", async () => { + const ix = mkInteraction("@everyone"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + const reply = (ix as { replies: { embeds: { data: { title: string } }[] }[] }).replies[0]; + expect(JSON.stringify(reply)).toMatch(/Invalid Name/); + }); + + it("rejects backticks", async () => { + const ix = mkInteraction("`evil`"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + }); + + it("rejects zero-width chars", async () => { + const ix = mkInteraction("hi\u200Bthere"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + }); + + it("accepts a normal name", async () => { + mockCreateRule.mockResolvedValue({ + id: 1, + guildId: "g1", + name: "welcome_rule", + enabled: true, + eventType: "memberJoin", + actions: [], + conditions: {}, + priority: 0, + createdBy: "u1", + }); + const ix = mkInteraction("welcome_rule"); + await command.execute(ix); + expect(mockCreateRule).toHaveBeenCalledTimes(1); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/general/commands/actions-name-validation.test.ts`. Expected: FAIL (rejection branch missing). + +- [ ] **Step 3: Implement** — in `apps/bot/src/features/general/commands/actions.ts`, add to the imports from `@fluxcore/systems/actions/constants`: + +```typescript +import { + EVENT_TYPES, + ACTION_TYPES, + MAX_ACTIONS_PER_RULE, + CONDITION_TYPES, + isValidRuleName, + type ConditionType, +} from "@fluxcore/systems/actions/constants"; +``` + +Then in `handleCreate` (around line 474), insert the validation immediately after reading `name`: + +```typescript +async function handleCreate( + interaction: ChatInputCommandInteraction, + guildId: string, +) { + const name = interaction.options.getString("name", true); + + if (!isValidRuleName(name)) { + await interaction.reply({ + embeds: [ + errorEmbed( + "Invalid Name", + "Rule names must be 1-50 characters using only letters, digits, spaces, underscores, or hyphens.", + ), + ], + ephemeral: true, + }); + return; + } + + const eventType = interaction.options.getString("event", true) as ActionEventType; +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/general/commands/actions-name-validation.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(actions): whitelist /actions create rule names + +Reject names that contain markdown, mention syntax, code fences, or +zero-width characters. Names must now match /^[a-zA-Z0-9 _-]{1,50}$/. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-05-resolvetemplate-literal-only-verification.md b/docs/superpowers/plans/2026-04-07-sec-bot-05-resolvetemplate-literal-only-verification.md new file mode 100644 index 0000000..fb80fbd --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-05-resolvetemplate-literal-only-verification.md @@ -0,0 +1,133 @@ +# Verify resolveTemplate Is Literal-Replacement Only — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Medium (verify-only, with hardening) +**Goal:** Add adversarial unit tests proving `resolveTemplate` performs ONLY literal `replaceAll` substitution and never evaluates JavaScript-like expressions, function calls, or nested templates supplied via the user-controlled `ctx.extra` map. If a future maintainer ever swaps it for `eval`, `new Function`, or a templating library, the tests will fail. +**Architecture:** `packages/systems/src/actions/templateEngine.ts` is consumed by `apps/bot/src/features/automation/system/registry.ts` lines 20-25 (and many other executor blocks). The function takes a `template: string` configured by the moderator and a `context: EventContext` whose `extra` field is populated from Discord events (e.g. `{message.content}`, `{ban.reason}`) — values that are user-controlled. The current implementation iterates `Object.entries(context.extra)`, builds a `${key}` placeholder, and calls `result.replaceAll(variable, escapeTemplateVars(value))`. It already escapes user values to prevent recursion. We add tests that pin this behavior. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`registry.ts` line 20 calls `resolveTemplate(config.message, ctx)`. If `resolveTemplate` ever started evaluating expressions inside `${...}` or `{{...}}` (e.g. by adopting Handlebars, EJS, or `eval`), then any moderator-controlled `config.message` could exfiltrate process state. Today the implementation in `packages/systems/src/actions/templateEngine.ts` is a literal `String.prototype.replaceAll` loop and is safe — but there is no test that pins it. The escape function `escapeTemplateVars` already neutralizes recursive injection from `ctx.extra` values. Both properties need a regression test. + +## Files + +- `packages/systems/src/actions/templateEngine.ts` (no behavior change; add `// SECURITY` comment) +- New: `packages/systems/tests/unit/templateEngine-injection.test.ts` + +## Tasks + +### Task 1: Add adversarial regression tests for resolveTemplate + +- [ ] **Step 1: Write failing test** — create `packages/systems/tests/unit/templateEngine-injection.test.ts`: + +```typescript +import { describe, it, expect } from "vitest"; +import { resolveTemplate } from "../../src/actions/templateEngine.js"; +import type { EventContext } from "../../src/actions/types.js"; + +const baseCtx: EventContext = { + eventType: "memberJoin", + guildId: "g1", + guildName: "Guild", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + memberCount: 5, + timestamp: "2026-04-07T00:00:00.000Z", +}; + +describe("resolveTemplate — literal replacement only", () => { + it("replaces known core variables", () => { + expect(resolveTemplate("hi {user.name}", baseCtx)).toBe("hi alice"); + }); + + it("does NOT evaluate JavaScript expressions inside braces", () => { + expect(resolveTemplate("{1+1}", baseCtx)).toBe("{1+1}"); + expect(resolveTemplate("{process.env.HOME}", baseCtx)).toBe( + "{process.env.HOME}", + ); + }); + + it("does NOT evaluate ${...} interpolation", () => { + // The literal $ and ${...} must be returned unchanged + expect(resolveTemplate("${1+1}", baseCtx)).toBe("${1+1}"); + }); + + it("does NOT recurse into user-controlled extra values", () => { + // A malicious message.content tries to smuggle {user.id} as the value + // for {message.content}. The escape MUST prevent the second resolver + // pass from expanding it. + const ctx: EventContext = { + ...baseCtx, + extra: { "message.content": "{user.id}" }, + }; + const out = resolveTemplate("said: {message.content}", ctx); + expect(out).not.toBe("said: u1"); + expect(out).toContain("\u200B{user.id}"); + }); + + it("does NOT recurse via nested core variables in extra", () => { + const ctx: EventContext = { + ...baseCtx, + extra: { "ban.reason": "{user.tag}{guild}" }, + }; + const out = resolveTemplate("reason: {ban.reason}", ctx); + expect(out).not.toContain("alice#0001"); + expect(out).toContain("\u200B{user.tag}"); + }); + + it("leaves unknown {placeholders} untouched", () => { + expect(resolveTemplate("{this.does.not.exist}", baseCtx)).toBe( + "{this.does.not.exist}", + ); + }); + + it("truncates output to MAX_TEMPLATE_LENGTH", () => { + const big = "x".repeat(5000); + const out = resolveTemplate(big, baseCtx); + expect(out.length).toBeLessThanOrEqual(2000); + }); + + it("never throws on non-string extra values being absent", () => { + expect(() => + resolveTemplate("{user.name} joined {guild}", baseCtx), + ).not.toThrow(); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/templateEngine-injection.test.ts`. Expected: PASS (current implementation is already safe). If any test FAILS, then the engine has regressed and step 3 must restore the literal-replacement implementation BEFORE merging. + +- [ ] **Step 3: Implement** — add a `// SECURITY` block-comment to `packages/systems/src/actions/templateEngine.ts` directly above `export function resolveTemplate`: + +```typescript +/** + * SECURITY: resolveTemplate MUST remain a pure literal-replacement function. + * It must NEVER call eval, new Function, or any templating library that + * evaluates expressions inside braces. User-controlled values from + * ctx.extra are passed through escapeTemplateVars so a hostile + * `message.content` cannot smuggle `{user.id}` into a second resolver + * pass. Both properties are pinned by + * tests/unit/templateEngine-injection.test.ts — do not remove that file. + */ +export function resolveTemplate( +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test packages/systems/tests/unit/templateEngine-injection.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +test(actions): pin resolveTemplate as literal-replacement only + +Adds adversarial unit tests proving resolveTemplate does not evaluate +JavaScript expressions, ${...} interpolation, or recurse into +user-controlled ctx.extra values. Annotates the function with a +SECURITY comment so future maintainers cannot silently swap in a +templating library. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-06-emoji-validation-addreaction.md b/docs/superpowers/plans/2026-04-07-sec-bot-06-emoji-validation-addreaction.md new file mode 100644 index 0000000..70fb3de --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-06-emoji-validation-addreaction.md @@ -0,0 +1,221 @@ +# Validate Emoji Before message.react() — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Medium +**Goal:** Validate `config.emoji` against a strict shape (single Unicode emoji code point cluster OR Discord custom-emoji form ``) before passing it to `message.react()`. This prevents action rules from triggering hundreds of failing API calls per event with garbage emoji input, surfacing a clear logged error instead. +**Architecture:** `apps/bot/src/features/automation/system/registry.ts` line 244 calls `message.react(config.emoji)` without inspecting the value. `config.emoji` is moderator-controlled via the `/actions create … emoji` option (max length 50). Discord.js will throw on invalid emoji, the executor catches it, but the call still hits the API and pollutes logs. Fix: add an `isValidEmoji()` helper and short-circuit before the API call. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`registry.ts` lines 236-248: + +```typescript +executors.set("addReaction", async (client, ctx, config) => { + if (!config.emoji || !ctx.extra?.["message.id"] || !ctx.channelId) return; + const channel = await client.channels.fetch(ctx.channelId); + if (!channel?.isTextBased() || !("messages" in channel)) return; + try { + const message = await (channel as TextChannel).messages.fetch( + ctx.extra["message.id"], + ); + await message.react(config.emoji); + } catch (err) { + logger.warn(`addReaction failed (...)`); + } +}); +``` + +A rule with `emoji: "not-an-emoji"` or `emoji: ":fake:"` will fetch the message from the API and only fail at the `react()` call. The cost is amortized per matching event. Validating up-front saves the fetch and yields a clearer log. + +## Files + +- `apps/bot/src/features/automation/system/registry.ts` (lines 236-248) +- New: `apps/bot/tests/features/automation/system/registry-emoji.test.ts` + +## Tasks + +### Task 1: Add isValidEmoji guard + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/features/automation/system/registry-emoji.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +const warn = vi.fn(); +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn, error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn().mockResolvedValue({ address: "1.1.1.1", family: 4 }), +})); + +const { getExecutor } = await import( + "../../../../src/features/automation/system/registry.js" +); + +const baseCtx = { + eventType: "messageCreated" as const, + guildId: "g1", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + guildName: "G", + memberCount: 10, + timestamp: new Date().toISOString(), + extra: { "message.id": "m1" }, +}; + +function mkClient(reactSpy: ReturnType, fetchSpy: ReturnType) { + return { + channels: { + fetch: vi.fn().mockResolvedValue({ + isTextBased: () => true, + messages: { fetch: fetchSpy }, + }), + }, + } as never; +} + +describe("addReaction emoji validation", () => { + beforeEach(() => warn.mockClear()); + + it("rejects garbage strings without fetching the message", async () => { + const fetchSpy = vi.fn(); + const reactSpy = vi.fn(); + const client = mkClient(reactSpy, fetchSpy); + + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "not-an-emoji-at-all", + } as never); + + expect(fetchSpy).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("invalid emoji"), + ); + }); + + it("accepts a unicode emoji", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(vi.fn(), fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "\u{1F389}", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("accepts a Discord custom emoji <:name:id>", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(vi.fn(), fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "<:partyparrot:123456789012345678>", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("accepts an animated custom emoji ", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(vi.fn(), fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("rejects bare colon shortcodes like :smile:", async () => { + const fetchSpy = vi.fn(); + const client = mkClient(vi.fn(), fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: ":smile:", + } as never); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/automation/system/registry-emoji.test.ts`. Expected: FAIL (no validation yet). + +- [ ] **Step 3: Implement** — in `apps/bot/src/features/automation/system/registry.ts`, add the helper near the top of the file (after the imports, before the first `executors.set` call): + +```typescript +/** + * Validates that a moderator-supplied emoji is either a single Unicode + * emoji cluster or a Discord custom-emoji literal `<:name:id>` / + * ``. Anything else is rejected to avoid hitting the Discord + * API with garbage values. + */ +const CUSTOM_EMOJI_REGEX = /^$/; +// Matches strings whose code points all belong to the Unicode "Emoji" +// property (single emoji or ZWJ sequence). Length cap of 16 covers +// flag/family ZWJ sequences while preventing pathological inputs. +const UNICODE_EMOJI_REGEX = /^(?:\p{Extended_Pictographic}|\p{Emoji_Component})(?:\u200D(?:\p{Extended_Pictographic}|\p{Emoji_Component}))*$/u; + +function isValidEmoji(value: string): boolean { + if (typeof value !== "string" || value.length === 0 || value.length > 64) { + return false; + } + if (CUSTOM_EMOJI_REGEX.test(value)) return true; + return UNICODE_EMOJI_REGEX.test(value); +} +``` + +Then replace the `addReaction` executor (lines 236-248) with: + +```typescript +executors.set("addReaction", async (client, ctx, config) => { + if (!config.emoji || !ctx.extra?.["message.id"] || !ctx.channelId) return; + if (!isValidEmoji(config.emoji)) { + logger.warn( + `addReaction skipped: invalid emoji "${config.emoji}" for guild ${ctx.guildId ?? "unknown"}`, + ); + return; + } + const channel = await client.channels.fetch(ctx.channelId); + if (!channel?.isTextBased() || !("messages" in channel)) return; + try { + const message = await (channel as TextChannel).messages.fetch( + ctx.extra["message.id"], + ); + await message.react(config.emoji); + } catch (err) { + logger.warn( + `addReaction failed (emoji: ${config.emoji}, message: ${ctx.extra?.["message.id"] ?? "unknown"}): ${err instanceof Error ? err.message : String(err)}`, + ); + } +}); +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/automation/system/registry-emoji.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(actions): validate emoji before message.react() + +Reject moderator-configured emoji values that are neither a Unicode +emoji cluster nor a Discord custom-emoji literal. Prevents wasted API +calls and noisy error logs when a rule is misconfigured. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-07-autorole-toctou-handling.md b/docs/superpowers/plans/2026-04-07-sec-bot-07-autorole-toctou-handling.md new file mode 100644 index 0000000..ca34a37 --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-07-autorole-toctou-handling.md @@ -0,0 +1,207 @@ +# Auto-Role TOCTOU: Handle DiscordAPIError 50013 Explicitly — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Medium +**Goal:** When `member.roles.add(...)` fails inside the welcome auto-role block in `guildMemberAdd`, log a meaningful per-error message: distinguish "missing permissions" (Discord error code `50013`) from "unknown role" (`10011`) from generic failures, instead of swallowing every error into a single ambiguous log line. Eliminates the silent TOCTOU window where the role's hierarchy check passed but Discord returned 403 because the role moved between cache check and API call. +**Architecture:** `apps/bot/src/events/guildMemberAdd.ts` lines 125-139 filter `welcomeConfig.autoRoleIds` against `botMember.roles.highest.position`, then call `member.roles.add(rolesToAdd, ...)`. The `.catch` block currently logs only a generic message. Discord.js raises `DiscordAPIError` with a `code` field that lets us classify failures. The fix imports `DiscordAPIError` from `discord.js`, switches on `err.code`, and emits structured warn/error logs. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +```typescript +if (rolesToAdd.length > 0) { + await member.roles.add(rolesToAdd, "Auto-role on join").catch((err) => { + logger.error(`Failed to assign auto-roles in guild ${member.guild.id}`, + err instanceof Error ? err : new Error(String(err))); + }); +} +``` + +This swallows every failure under `logger.error`. A 403 from Discord (role hierarchy raced against the bot's cache, role was deleted, or `MANAGE_ROLES` was revoked) is indistinguishable from a transient 5xx. Operators cannot tell whether to fix permissions or whether it was a one-off. The TOCTOU window between the in-memory hierarchy check and the API call also means a "should have worked" case prints as a hard error. + +## Files + +- `apps/bot/src/events/guildMemberAdd.ts` (lines 125-139) +- New: `apps/bot/tests/events/autorole-error-handling.test.ts` + +## Tasks + +### Task 1: Classify and log auto-role errors by Discord code + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/events/autorole-error-handling.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +const warn = vi.fn(); +const error = vi.fn(); +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn, error, info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/logging/config", () => ({ + getLogConfig: vi.fn().mockResolvedValue(null), +})); +vi.mock("@fluxcore/systems/logging/persistence", () => ({ createLogEntry: vi.fn() })); +vi.mock("@fluxcore/systems/logging/sender", () => ({ sendLogEmbed: vi.fn() })); +vi.mock("@fluxcore/systems/logging/formatter", () => ({ formatMemberJoin: vi.fn() })); + +vi.mock("@fluxcore/systems/antiraid/config", () => ({ + getAntiRaidConfig: vi.fn().mockResolvedValue({ enabled: false }), +})); +vi.mock("@fluxcore/systems/antiraid/tracker", () => ({ recordJoin: vi.fn() })); +vi.mock("@fluxcore/systems/antiraid/actions", () => ({ + executeRaidAction: vi.fn(), + lockdownGuild: vi.fn(), +})); +vi.mock("@fluxcore/systems/antiraid/persistence", () => ({ createRaidEvent: vi.fn() })); + +vi.mock("@fluxcore/systems/welcome/config", () => ({ + getWelcomeConfig: vi.fn().mockResolvedValue({ + autoRoleIds: ["role-1"], + welcomeEnabled: false, + welcomeChannelId: null, + welcomeMessage: "", + welcomeImageEnabled: false, + welcomeImageConfig: { sendMode: "with" }, + dmEnabled: false, + dmMessage: "", + }), +})); + +vi.mock("@fluxcore/systems/welcome/builder", () => ({ + buildWelcomeEmbed: vi.fn(), +})); +vi.mock("@fluxcore/systems/welcome/image", () => ({ + generateWelcomeImage: vi.fn(), + createStorageAdapter: vi.fn(), + sanitizeDisplayName: (s: string) => s, +})); + +import { DiscordAPIError } from "discord.js"; + +const event = (await import("../../src/events/guildMemberAdd.js")).default; + +function mkMember(rolesAdd: ReturnType) { + return { + id: "m1", + user: { bot: false, username: "alice", createdTimestamp: Date.now() - 86_400_000 * 100, displayAvatarURL: () => "", tag: "" }, + displayName: "Alice", + guild: { + id: "g1", + name: "G", + memberCount: 5, + iconURL: () => null, + members: { me: { roles: { highest: { position: 100 } } } }, + roles: { + cache: new Map([["role-1", { position: 5, id: "role-1" }]]), + }, + channels: { cache: { get: () => null } }, + }, + roles: { cache: new Map(), add: rolesAdd }, + send: vi.fn(), + } as never; +} + +function makeApiError(code: number, message: string): DiscordAPIError { + // Construct a minimal DiscordAPIError instance + const err = new DiscordAPIError( + { message, code } as never, + code, + 403, + "PUT", + "url", + { files: [] } as never, + ); + return err; +} + +describe("auto-role failure handling", () => { + beforeEach(() => { + warn.mockClear(); + error.mockClear(); + }); + + it("logs WARN with 'missing permissions' on Discord code 50013", async () => { + const rolesAdd = vi.fn().mockRejectedValue(makeApiError(50013, "Missing Permissions")); + await event.execute(mkMember(rolesAdd)); + expect(warn).toHaveBeenCalledWith( + expect.stringMatching(/auto-role.*missing permissions/i), + ); + expect(error).not.toHaveBeenCalled(); + }); + + it("logs WARN with 'unknown role' on Discord code 10011", async () => { + const rolesAdd = vi.fn().mockRejectedValue(makeApiError(10011, "Unknown Role")); + await event.execute(mkMember(rolesAdd)); + expect(warn).toHaveBeenCalledWith( + expect.stringMatching(/auto-role.*unknown role/i), + ); + expect(error).not.toHaveBeenCalled(); + }); + + it("logs ERROR for unexpected failure (e.g. 5xx)", async () => { + const rolesAdd = vi.fn().mockRejectedValue(new Error("ECONNRESET")); + await event.execute(mkMember(rolesAdd)); + expect(error).toHaveBeenCalled(); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/autorole-error-handling.test.ts`. Expected: FAIL (current handler always logs `error`). + +- [ ] **Step 3: Implement** — in `apps/bot/src/events/guildMemberAdd.ts`, add `DiscordAPIError` to the discord.js imports: + +```typescript +import { AttachmentBuilder, DiscordAPIError } from "discord.js"; +``` + +Then replace the auto-role catch (lines 134-138) with: + +```typescript + if (rolesToAdd.length > 0) { + await member.roles.add(rolesToAdd, "Auto-role on join").catch((err) => { + if (err instanceof DiscordAPIError) { + if (err.code === 50013) { + logger.warn( + `auto-role: missing permissions in guild ${member.guild.id} (role likely moved above bot between cache check and API call)`, + ); + return; + } + if (err.code === 10011) { + logger.warn( + `auto-role: unknown role in guild ${member.guild.id} (role was deleted between cache check and API call)`, + ); + return; + } + } + logger.error( + `Failed to assign auto-roles in guild ${member.guild.id}`, + err instanceof Error ? err : new Error(String(err)), + ); + }); + } +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/autorole-error-handling.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(welcome): classify auto-role failures by Discord error code + +Distinguish 50013 (missing permissions, expected after a hierarchy +race) and 10011 (unknown role, deleted between cache check and API +call) from genuine errors. Avoids drowning operators in error-level +noise for benign TOCTOU outcomes. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-08-levelup-mass-mention-prevention.md b/docs/superpowers/plans/2026-04-07-sec-bot-08-levelup-mass-mention-prevention.md new file mode 100644 index 0000000..0a54243 --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-08-levelup-mass-mention-prevention.md @@ -0,0 +1,210 @@ +# Levelup Announcement Mass-Mention Prevention — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Low +**Goal:** Stop the leveling system's level-up announcement from being able to ping `@everyone`, `@here`, arbitrary roles, or arbitrary users via the moderator-configured `announceMessage` template. Restrict mentions to the leveling user themself and explicitly disable role/everyone parses. +**Architecture:** `apps/bot/src/events/messageCreate.ts` lines 27-50 builds an announcement string with `settings.announceMessage` (a template moderators can edit) and calls `(message.channel as ...).send(text)`. Discord.js will parse `@everyone`, `<@&roleId>`, etc. by default. The fix migrates the three send sites to send `{ content, allowedMentions: { parse: [], users: [message.author.id] } }`. The DM branch keeps `parse: []` (no mentions in DMs make sense). The TypeScript cast at line 41 also widens `send` to accept the structured options. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +```typescript +const text = settings.announceMessage + .replace("{user}", `<@${message.author.id}>`) + .replace("{level}", String(newLevel)) + .replace("{username}", message.author.displayName); + +try { + if (settings.announceChannel === "dm") { + await message.author.send(text); + } else if (settings.announceChannel) { + const channel = message.guild?.channels.cache.get(settings.announceChannel); + if (channel?.isTextBased()) { + await (channel as TextChannel).send(text); + } + } else { + await (message.channel as { send: (text: string) => Promise }).send(text); + } +} +``` + +A guild admin who edits `announceMessage` to `"@everyone {user} reached level {level}!"` will get a server-wide ping every time anyone levels up. Even without malice, an admin who copies a sample template containing `<@&modRole>` will inadvertently DM-storm a moderator role on every level-up. + +## Files + +- `apps/bot/src/events/messageCreate.ts` (lines 20-50) +- New: `apps/bot/tests/events/levelup-mentions.test.ts` + +## Tasks + +### Task 1: Pass allowedMentions on every level-up send + +- [ ] **Step 1: Write failing test** — create `apps/bot/tests/events/levelup-mentions.test.ts`: + +```typescript +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/leveling/config", () => ({ + getLevelSettings: vi.fn().mockResolvedValue({ + enabled: true, + xpPerMessage: 10, + xpCooldownSeconds: 0, + noXpChannels: [], + noXpRoles: [], + multipliers: [], + announceEnabled: true, + announceChannel: null, // reply in same channel + announceMessage: "@everyone {user} hit {level}!", + }), +})); +vi.mock("@fluxcore/systems/leveling/persistence", () => ({ + getUserLevel: vi.fn().mockResolvedValue(null), + addXp: vi.fn().mockResolvedValue({ leveledUp: true, newLevel: 2 }), +})); +vi.mock("@fluxcore/systems/leveling/xp", () => ({ + applyMultipliers: (xp: number) => xp, +})); +vi.mock("@fluxcore/systems/leveling/constants", () => ({ XP_RANDOMNESS: 1 })); +vi.mock("@fluxcore/systems/leveling/rewards", () => ({ + checkAndGrantRewards: vi.fn(), +})); + +const event = (await import("../../src/events/messageCreate.js")).default; + +describe("level-up announcement allowedMentions", () => { + it("passes allowedMentions to channel.send when announcing in same channel", async () => { + const send = vi.fn().mockResolvedValue(undefined); + const message = { + guild: { id: "g1", channels: { cache: { get: () => null } } }, + author: { id: "u1", bot: false, displayName: "Alice", send: vi.fn() }, + member: { roles: { cache: new Map() } }, + channelId: "c1", + channel: { send }, + } as never; + + await event.execute(message); + + expect(send).toHaveBeenCalledTimes(1); + const arg = send.mock.calls[0][0]; + expect(typeof arg).toBe("object"); + expect(arg.content).toContain("@everyone"); + expect(arg.allowedMentions).toEqual({ parse: [], users: ["u1"] }); + }); + + it("passes allowedMentions when announceChannel is configured", async () => { + const { getLevelSettings } = await import("@fluxcore/systems/leveling/config"); + (getLevelSettings as ReturnType).mockResolvedValueOnce({ + enabled: true, + xpPerMessage: 10, + xpCooldownSeconds: 0, + noXpChannels: [], + noXpRoles: [], + multipliers: [], + announceEnabled: true, + announceChannel: "ch-announce", + announceMessage: "<@&modRole> {user} hit {level}!", + }); + + const announceSend = vi.fn().mockResolvedValue(undefined); + const message = { + guild: { + id: "g1", + channels: { + cache: { + get: (id: string) => + id === "ch-announce" + ? { isTextBased: () => true, send: announceSend } + : null, + }, + }, + }, + author: { id: "u1", bot: false, displayName: "Alice", send: vi.fn() }, + member: { roles: { cache: new Map() } }, + channelId: "c1", + channel: { send: vi.fn() }, + } as never; + + await event.execute(message); + + expect(announceSend).toHaveBeenCalledTimes(1); + expect(announceSend.mock.calls[0][0].allowedMentions).toEqual({ + parse: [], + users: ["u1"], + }); + }); +}); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/levelup-mentions.test.ts`. Expected: FAIL (current code sends a plain string). + +- [ ] **Step 3: Implement** — replace the body of `handleLevelUp` in `apps/bot/src/events/messageCreate.ts` (lines 20-50) with: + +```typescript +async function handleLevelUp( + message: Message, + settings: { announceEnabled: boolean; announceChannel: string | null; announceMessage: string }, + newLevel: number, +): Promise { + if (!settings.announceEnabled) return; + + const text = settings.announceMessage + .replace("{user}", `<@${message.author.id}>`) + .replace("{level}", String(newLevel)) + .replace("{username}", message.author.displayName); + + // Lock allowedMentions so a moderator-configured template cannot ping + // @everyone, @here, or arbitrary roles. Only the leveling user + // themself is allowed to be mentioned. + const allowedMentions = { parse: [], users: [message.author.id] } as const; + + try { + if (settings.announceChannel === "dm") { + await message.author.send({ content: text, allowedMentions: { parse: [] } }); + } else if (settings.announceChannel) { + const channel = message.guild?.channels.cache.get(settings.announceChannel); + if (channel?.isTextBased()) { + await (channel as TextChannel).send({ content: text, allowedMentions }); + } + } else { + await ( + message.channel as { + send: (payload: { content: string; allowedMentions: typeof allowedMentions }) => Promise; + } + ).send({ content: text, allowedMentions }); + } + } catch (error) { + logger.debug( + `Failed to send level-up announcement for ${message.author.id}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } +} +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/events/levelup-mentions.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(leveling): block @everyone/role mentions in level-up announcement + +Pass allowedMentions: { parse: [], users: [message.author.id] } to +every send site so a moderator-configured announceMessage template +containing @everyone, @here, or <@&roleId> can no longer ping the +server. +``` diff --git a/docs/superpowers/plans/2026-04-07-sec-bot-09-warn-checkbotpermissions.md b/docs/superpowers/plans/2026-04-07-sec-bot-09-warn-checkbotpermissions.md new file mode 100644 index 0000000..b1756c2 --- /dev/null +++ b/docs/superpowers/plans/2026-04-07-sec-bot-09-warn-checkbotpermissions.md @@ -0,0 +1,119 @@ +# Add checkBotPermissions to /warn — Fix Plan +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax. + +**Severity:** Low +**Goal:** Make `/warn` consistent with sibling moderation commands (`/kick`, `/ban`, `/timeout`, ...) by verifying the bot itself has `ManageMessages` before attempting to issue a warning. The warning record itself doesn't strictly need it, but escalation actions (kick/ban/timeout) chained off `checkAndExecutePunishment` will fail confusingly if the bot lacks higher permissions; we surface a clear up-front error. +**Architecture:** `apps/bot/src/features/moderation/commands/warn.ts` lines 41-46 currently calls only `checkPermissions(interaction, [PermissionFlagsBits.ManageMessages])`. Sibling `kick.ts` line 38-46 chains both `checkPermissions` and `checkBotPermissions`. The same `checkBotPermissions` helper from `@fluxcore/utils` is already imported in other commands. The fix imports it in `warn.ts` and adds the call. +**Tech Stack:** discord.js v14, TypeScript, Prisma, Vitest + +--- + +## Vulnerability + +`apps/bot/src/features/moderation/commands/warn.ts` lines 40-47: + +```typescript +async execute(interaction: ChatInputCommandInteraction) { + if ( + !(await checkPermissions(interaction, [ + PermissionFlagsBits.ManageMessages, + ])) + ) { + return; + } +``` + +There is no `checkBotPermissions` call. Every other moderation command in `apps/bot/src/features/moderation/commands/` chains both checks. The inconsistency means `/warn` succeeds even when the bot lacks `ManageMessages`, then the escalation pipeline fails midway with a confusing error after the warning row has already been written. + +## Files + +- `apps/bot/src/features/moderation/commands/warn.ts` (lines 8-47) +- `apps/bot/tests/features/moderation/commands/warn.test.ts` (extend existing) + +## Tasks + +### Task 1: Add checkBotPermissions check to /warn + +- [ ] **Step 1: Write failing test** — append to `apps/bot/tests/features/moderation/commands/warn.test.ts` a new test inside the existing top-level `describe`. First, update the `vi.mock("@fluxcore/utils", ...)` block at the top of the file to also expose `checkBotPermissions`: + +```typescript +const mockCheckBotPermissions = vi.fn().mockResolvedValue(true); +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkPermissions: (...args: unknown[]) => mockCheckPermissions(...args), + checkBotPermissions: (...args: unknown[]) => mockCheckBotPermissions(...args), + isAboveTarget: (...args: unknown[]) => mockIsAboveTarget(...args), + }; +}); +``` + +Then add a new `it` case at the end of the file's main `describe` block: + +```typescript + it("aborts when the bot lacks ManageMessages", async () => { + mockCheckPermissions.mockResolvedValueOnce(true); + mockCheckBotPermissions.mockResolvedValueOnce(false); + + const interaction = createMockInteraction(); + await command.execute(interaction); + + expect(mockCheckBotPermissions).toHaveBeenCalledTimes(1); + expect(mockCreateWarning).not.toHaveBeenCalled(); + }); + + it("calls checkBotPermissions on the happy path", async () => { + mockCheckPermissions.mockResolvedValueOnce(true); + mockCheckBotPermissions.mockResolvedValueOnce(true); + + const interaction = createMockInteraction(); + await command.execute(interaction); + + expect(mockCheckBotPermissions).toHaveBeenCalledTimes(1); + }); +``` + +- [ ] **Step 2: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/moderation/commands/warn.test.ts`. Expected: FAIL (the command does not call `checkBotPermissions`). + +- [ ] **Step 3: Implement** — in `apps/bot/src/features/moderation/commands/warn.ts`, add `checkBotPermissions` to the imports from `@fluxcore/utils`: + +```typescript +import { + successEmbed, + errorEmbed, + warnEmbed, + checkPermissions, + checkBotPermissions, + isAboveTarget, + logger, +} from "@fluxcore/utils"; +``` + +Then replace the permissions block (lines 41-47) with: + +```typescript + if ( + !(await checkPermissions(interaction, [ + PermissionFlagsBits.ManageMessages, + ])) || + !(await checkBotPermissions(interaction, [ + PermissionFlagsBits.ManageMessages, + ])) + ) { + return; + } +``` + +- [ ] **Step 4: Run** — `docker compose run --rm bot pnpm test apps/bot/tests/features/moderation/commands/warn.test.ts && docker compose run --rm bot pnpm typecheck`. Expected: PASS. + +- [ ] **Step 5: Commit** — + +``` +fix(warn): add checkBotPermissions for parity with sibling mod commands + +/warn now verifies the bot has ManageMessages before writing the +warning row, matching /kick, /ban, /timeout, etc. Surfaces a clear +"bot missing permissions" reply instead of failing midway through +the escalation pipeline. +``` From 17c848551dea0223e253c6595c33743d8a358f56 Mon Sep 17 00:00:00 2001 From: Abdulkhalek Muhammad Date: Tue, 7 Apr 2026 14:38:34 +0200 Subject: [PATCH 2/2] fix(security): harden bot runtime surface (sec-bot-01..09) Implements 9 security hardening plans covering the bot process: - sec-bot-01: regression tests pinning $queryRaw parameterized safety - sec-bot-02: webhook headers -> strict allowlist (was denylist) - sec-bot-03: sanitize usernames/guild name before welcome canvas render (strip bidi overrides, zero-width chars, control chars) - sec-bot-04: restrict action rule names to [a-zA-Z0-9 _-]{1,50} - sec-bot-05: templateEngine - fix escape lookbehind so escaped user content can no longer smuggle core variables into the second pass, plus adversarial regression tests - sec-bot-06: validate emoji against Unicode/custom-emoji form before message.react() - sec-bot-07: classify auto-role assignment failures by Discord API error code (50013/10011 warn, others error) - sec-bot-08: level-up announcements restrict mentions to the leveling user only via allowedMentions config - sec-bot-09: add checkBotPermissions for ManageMessages to /warn Adds sanitizeDisplayName helper in packages/systems/welcome. Bot test suite: 349 pass / 16 pre-existing baseline failures (no new regressions). Systems test suite: 234/234 green. --- apps/bot/src/events/guildMemberAdd.ts | 32 ++++- apps/bot/src/events/messageCreate.ts | 18 ++- .../features/automation/system/registry.ts | 52 ++++++- .../src/features/general/commands/actions.ts | 15 ++ .../src/features/moderation/commands/warn.ts | 4 + .../events/autorole-error-handling.test.ts | 128 +++++++++++++++++ .../bot/tests/events/levelup-mentions.test.ts | 109 +++++++++++++++ .../bot/tests/events/welcome-sanitize.test.ts | 130 ++++++++++++++++++ .../automation/system/registry-emoji.test.ts | 111 +++++++++++++++ .../system/registry-webhook.test.ts | 115 ++++++++++++++++ .../commands/actions-name-validation.test.ts | 117 ++++++++++++++++ .../features/moderation/commands/warn.test.ts | 24 ++++ packages/systems/src/actions/constants.ts | 11 ++ packages/systems/src/actions/persistence.ts | 7 + .../systems/src/actions/templateEngine.ts | 22 ++- packages/systems/src/welcome/image/index.ts | 3 + .../systems/src/welcome/image/sanitize.ts | 31 +++++ .../actions-persistence-sqli.test.ts | 90 ++++++++++++ .../systems/tests/unit/actions-name.test.ts | 35 +++++ .../unit/templateEngine-injection.test.ts | 75 ++++++++++ .../tests/unit/welcome/sanitize.test.ts | 45 ++++++ 21 files changed, 1157 insertions(+), 17 deletions(-) create mode 100644 apps/bot/tests/events/autorole-error-handling.test.ts create mode 100644 apps/bot/tests/events/levelup-mentions.test.ts create mode 100644 apps/bot/tests/events/welcome-sanitize.test.ts create mode 100644 apps/bot/tests/features/automation/system/registry-emoji.test.ts create mode 100644 apps/bot/tests/features/automation/system/registry-webhook.test.ts create mode 100644 apps/bot/tests/features/general/commands/actions-name-validation.test.ts create mode 100644 packages/systems/src/welcome/image/sanitize.ts create mode 100644 packages/systems/tests/integration/actions-persistence-sqli.test.ts create mode 100644 packages/systems/tests/unit/actions-name.test.ts create mode 100644 packages/systems/tests/unit/templateEngine-injection.test.ts create mode 100644 packages/systems/tests/unit/welcome/sanitize.test.ts diff --git a/apps/bot/src/events/guildMemberAdd.ts b/apps/bot/src/events/guildMemberAdd.ts index b610213..88dc072 100644 --- a/apps/bot/src/events/guildMemberAdd.ts +++ b/apps/bot/src/events/guildMemberAdd.ts @@ -7,8 +7,8 @@ import { sendLogEmbed } from "@fluxcore/systems/logging/sender"; import { formatMemberJoin } from "@fluxcore/systems/logging/formatter"; import { getWelcomeConfig } from "@fluxcore/systems/welcome/config"; import { buildWelcomeEmbed } from "@fluxcore/systems/welcome/builder"; -import { generateWelcomeImage, createStorageAdapter } from "@fluxcore/systems/welcome/image"; -import { AttachmentBuilder } from "discord.js"; +import { generateWelcomeImage, createStorageAdapter, sanitizeDisplayName } from "@fluxcore/systems/welcome/image"; +import { AttachmentBuilder, DiscordAPIError } from "discord.js"; import { getAntiRaidConfig } from "@fluxcore/systems/antiraid/config"; import { recordJoin } from "@fluxcore/systems/antiraid/tracker"; import { executeRaidAction, lockdownGuild } from "@fluxcore/systems/antiraid/actions"; @@ -133,7 +133,24 @@ const event: Event<"guildMemberAdd"> = { }); if (rolesToAdd.length > 0) { await member.roles.add(rolesToAdd, "Auto-role on join").catch((err) => { - logger.error(`Failed to assign auto-roles in guild ${member.guild.id}`, err instanceof Error ? err : new Error(String(err))); + if (err instanceof DiscordAPIError) { + if (err.code === 50013) { + logger.warn( + `auto-role: missing permissions in guild ${member.guild.id} (role likely moved above bot between cache check and API call)`, + ); + return; + } + if (err.code === 10011) { + logger.warn( + `auto-role: unknown role in guild ${member.guild.id} (role was deleted between cache check and API call)`, + ); + return; + } + } + logger.error( + `Failed to assign auto-roles in guild ${member.guild.id}`, + err instanceof Error ? err : new Error(String(err)), + ); }); } } @@ -149,15 +166,18 @@ const event: Event<"guildMemberAdd"> = { if (welcomeConfig.welcomeImageEnabled) { try { const storage = createStorageAdapter(); + const safeUsername = sanitizeDisplayName(member.user.username, 32); + const safeDisplayName = sanitizeDisplayName(member.displayName, 80); + const safeGuildName = sanitizeDisplayName(member.guild.name, 80); const imageBuffer = await generateWelcomeImage({ settings: welcomeConfig.welcomeImageConfig, member: { - username: member.user.username, - displayName: member.displayName, + username: safeUsername, + displayName: safeDisplayName, avatarUrl: member.user.displayAvatarURL({ extension: "png", size: 256 }), }, guild: { - name: member.guild.name, + name: safeGuildName, iconUrl: member.guild.iconURL({ size: 256 }) ?? undefined, memberCount: member.guild.memberCount, }, diff --git a/apps/bot/src/events/messageCreate.ts b/apps/bot/src/events/messageCreate.ts index 578dc32..53ed5c0 100644 --- a/apps/bot/src/events/messageCreate.ts +++ b/apps/bot/src/events/messageCreate.ts @@ -29,16 +29,28 @@ async function handleLevelUp( .replace("{level}", String(newLevel)) .replace("{username}", message.author.displayName); + // Lock allowedMentions so a moderator-configured template cannot ping + // @everyone, @here, or arbitrary roles. Only the leveling user + // themself is allowed to be mentioned. + const allowedMentions = { parse: [], users: [message.author.id] } as const; + try { if (settings.announceChannel === "dm") { - await message.author.send(text); + await message.author.send({ content: text, allowedMentions: { parse: [] } }); } else if (settings.announceChannel) { const channel = message.guild?.channels.cache.get(settings.announceChannel); if (channel?.isTextBased()) { - await (channel as TextChannel).send(text); + await (channel as TextChannel).send({ content: text, allowedMentions }); } } else { - await (message.channel as { send: (text: string) => Promise }).send(text); + await ( + message.channel as { + send: (payload: { + content: string; + allowedMentions: typeof allowedMentions; + }) => Promise; + } + ).send({ content: text, allowedMentions }); } } catch (error) { logger.debug( diff --git a/apps/bot/src/features/automation/system/registry.ts b/apps/bot/src/features/automation/system/registry.ts index 526d72a..bbd6369 100644 --- a/apps/bot/src/features/automation/system/registry.ts +++ b/apps/bot/src/features/automation/system/registry.ts @@ -11,6 +11,27 @@ type ActionExecutor = ( config: ActionConfig, ) => Promise; +/** + * Validates that a moderator-supplied emoji is either a single Unicode + * emoji cluster or a Discord custom-emoji literal `<:name:id>` / + * ``. Anything else is rejected to avoid hitting the Discord + * API with garbage values. + */ +const CUSTOM_EMOJI_REGEX = /^$/; +// Matches strings whose code points all belong to the Unicode "Emoji" +// property (single emoji or ZWJ sequence). Length cap of 64 covers +// flag/family ZWJ sequences while preventing pathological inputs. +const UNICODE_EMOJI_REGEX = + /^(?:\p{Extended_Pictographic}|\p{Emoji_Component})(?:\u200D(?:\p{Extended_Pictographic}|\p{Emoji_Component}))*$/u; + +function isValidEmoji(value: string): boolean { + if (typeof value !== "string" || value.length === 0 || value.length > 64) { + return false; + } + if (CUSTOM_EMOJI_REGEX.test(value)) return true; + return UNICODE_EMOJI_REGEX.test(value); +} + const executors = new Map(); executors.set("sendMessage", async (client, ctx, config) => { @@ -168,18 +189,33 @@ executors.set("sendWebhook", async (_client, ctx, config) => { body = body.slice(0, MAX_TEMPLATE_LENGTH); } - const BLOCKED_HEADERS = new Set([ - "host", "cookie", "set-cookie", "transfer-encoding", - "connection", "proxy-authorization", "te", "trailer", - "upgrade", + // Strict allowlist: only headers that are safe for the bot to forward on + // behalf of a guild admin. Denylists are unsafe — any new sensitive + // header (Authorization, X-Api-Key, X-Forwarded-*, Cookie, etc.) would + // silently leak. Add to this set only after a security review. + const ALLOWED_HEADERS = new Set([ + "accept", + "accept-language", + "cache-control", + "user-agent", + "x-idempotency-key", + "x-request-id", ]); + const ALLOWED_PREFIX = "x-fluxcore-"; + const userHeaders = config.webhook.headers ?? {}; const headers: Record = { "Content-Type": "application/json", }; for (const [key, value] of Object.entries(userHeaders)) { - if (!BLOCKED_HEADERS.has(key.toLowerCase())) { + const lower = key.toLowerCase(); + if (lower === "content-type") continue; // we set this ourselves + if (ALLOWED_HEADERS.has(lower) || lower.startsWith(ALLOWED_PREFIX)) { headers[key] = value; + } else { + logger.warn( + `sendWebhook: dropped non-allowlisted header "${key}" for guild ${ctx.guildId ?? "unknown"}`, + ); } } @@ -235,6 +271,12 @@ executors.set("createThread", async (client, ctx, config) => { executors.set("addReaction", async (client, ctx, config) => { if (!config.emoji || !ctx.extra?.["message.id"] || !ctx.channelId) return; + if (!isValidEmoji(config.emoji)) { + logger.warn( + `addReaction skipped: invalid emoji "${config.emoji}" for guild ${ctx.guildId ?? "unknown"}`, + ); + return; + } const channel = await client.channels.fetch(ctx.channelId); if (!channel?.isTextBased() || !("messages" in channel)) return; try { diff --git a/apps/bot/src/features/general/commands/actions.ts b/apps/bot/src/features/general/commands/actions.ts index fed3f85..93fa71d 100644 --- a/apps/bot/src/features/general/commands/actions.ts +++ b/apps/bot/src/features/general/commands/actions.ts @@ -19,6 +19,7 @@ import { ACTION_TYPES, MAX_ACTIONS_PER_RULE, CONDITION_TYPES, + isValidRuleName, type ConditionType, } from "@fluxcore/systems/actions/constants"; import { @@ -472,6 +473,20 @@ async function handleCreate( guildId: string, ) { const name = interaction.options.getString("name", true); + + if (!isValidRuleName(name)) { + await interaction.reply({ + embeds: [ + errorEmbed( + "Invalid Name", + "Rule names must be 1-50 characters using only letters, digits, spaces, underscores, or hyphens.", + ), + ], + ephemeral: true, + }); + return; + } + const eventType = interaction.options.getString("event", true) as ActionEventType; const actionType = interaction.options.getString("action-type", true) as ActionType; const channel = interaction.options.getChannel("channel"); diff --git a/apps/bot/src/features/moderation/commands/warn.ts b/apps/bot/src/features/moderation/commands/warn.ts index e0d1ea9..aa4e198 100644 --- a/apps/bot/src/features/moderation/commands/warn.ts +++ b/apps/bot/src/features/moderation/commands/warn.ts @@ -10,6 +10,7 @@ import { errorEmbed, warnEmbed, checkPermissions, + checkBotPermissions, isAboveTarget, logger, } from "@fluxcore/utils"; @@ -41,6 +42,9 @@ const command: Command = { if ( !(await checkPermissions(interaction, [ PermissionFlagsBits.ManageMessages, + ])) || + !(await checkBotPermissions(interaction, [ + PermissionFlagsBits.ManageMessages, ])) ) { return; diff --git a/apps/bot/tests/events/autorole-error-handling.test.ts b/apps/bot/tests/events/autorole-error-handling.test.ts new file mode 100644 index 0000000..ab44706 --- /dev/null +++ b/apps/bot/tests/events/autorole-error-handling.test.ts @@ -0,0 +1,128 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +const warn = vi.fn(); +const error = vi.fn(); +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn, error, info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/logging/config", () => ({ + getLogConfig: vi.fn().mockResolvedValue(null), +})); +vi.mock("@fluxcore/systems/logging/persistence", () => ({ createLogEntry: vi.fn() })); +vi.mock("@fluxcore/systems/logging/sender", () => ({ sendLogEmbed: vi.fn() })); +vi.mock("@fluxcore/systems/logging/formatter", () => ({ formatMemberJoin: vi.fn() })); + +vi.mock("@fluxcore/systems/antiraid/config", () => ({ + getAntiRaidConfig: vi.fn().mockResolvedValue({ enabled: false }), +})); +vi.mock("@fluxcore/systems/antiraid/tracker", () => ({ recordJoin: vi.fn() })); +vi.mock("@fluxcore/systems/antiraid/actions", () => ({ + executeRaidAction: vi.fn(), + lockdownGuild: vi.fn(), +})); +vi.mock("@fluxcore/systems/antiraid/persistence", () => ({ createRaidEvent: vi.fn() })); + +vi.mock("@fluxcore/systems/welcome/config", () => ({ + getWelcomeConfig: vi.fn().mockResolvedValue({ + autoRoleIds: ["role-1"], + welcomeEnabled: false, + welcomeChannelId: null, + welcomeMessage: "", + welcomeImageEnabled: false, + welcomeImageConfig: { sendMode: "with" }, + dmEnabled: false, + dmMessage: "", + }), +})); + +vi.mock("@fluxcore/systems/welcome/builder", () => ({ + buildWelcomeEmbed: vi.fn(), +})); +vi.mock("@fluxcore/systems/welcome/image", () => ({ + generateWelcomeImage: vi.fn(), + createStorageAdapter: vi.fn(), + sanitizeDisplayName: (s: string) => s, +})); + +import { DiscordAPIError } from "discord.js"; + +const event = (await import("../../src/events/guildMemberAdd.js")).default; + +function mkMember(rolesAdd: ReturnType) { + return { + id: "m1", + user: { + bot: false, + username: "alice", + createdTimestamp: Date.now() - 86_400_000 * 100, + displayAvatarURL: () => "", + tag: "", + }, + displayName: "Alice", + guild: { + id: "g1", + name: "G", + memberCount: 5, + iconURL: () => null, + members: { me: { roles: { highest: { position: 100 } } } }, + roles: { + cache: new Map([["role-1", { position: 5, id: "role-1" }]]), + }, + channels: { cache: { get: () => null } }, + }, + roles: { cache: new Map(), add: rolesAdd }, + send: vi.fn(), + } as never; +} + +function makeApiError(code: number, message: string): DiscordAPIError { + const err = new DiscordAPIError( + { message, code } as never, + code, + 403, + "PUT", + "url", + { files: [] } as never, + ); + return err; +} + +describe("auto-role failure handling", () => { + beforeEach(() => { + warn.mockClear(); + error.mockClear(); + }); + + it("logs WARN with 'missing permissions' on Discord code 50013", async () => { + const rolesAdd = vi.fn().mockRejectedValue(makeApiError(50013, "Missing Permissions")); + await event.execute(mkMember(rolesAdd)); + expect(warn).toHaveBeenCalledWith( + expect.stringMatching(/auto-role.*missing permissions/i), + ); + expect(error).not.toHaveBeenCalled(); + }); + + it("logs WARN with 'unknown role' on Discord code 10011", async () => { + const rolesAdd = vi.fn().mockRejectedValue(makeApiError(10011, "Unknown Role")); + await event.execute(mkMember(rolesAdd)); + expect(warn).toHaveBeenCalledWith( + expect.stringMatching(/auto-role.*unknown role/i), + ); + expect(error).not.toHaveBeenCalled(); + }); + + it("logs ERROR for unexpected failure (e.g. 5xx)", async () => { + const rolesAdd = vi.fn().mockRejectedValue(new Error("ECONNRESET")); + await event.execute(mkMember(rolesAdd)); + expect(error).toHaveBeenCalled(); + }); +}); diff --git a/apps/bot/tests/events/levelup-mentions.test.ts b/apps/bot/tests/events/levelup-mentions.test.ts new file mode 100644 index 0000000..ccc0f7c --- /dev/null +++ b/apps/bot/tests/events/levelup-mentions.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect, vi } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/leveling/config", () => ({ + getLevelSettings: vi.fn().mockResolvedValue({ + enabled: true, + xpPerMessage: 10, + xpCooldownSeconds: 0, + noXpChannels: [], + noXpRoles: [], + multipliers: [], + announceEnabled: true, + announceChannel: null, + announceMessage: "@everyone {user} hit {level}!", + }), +})); +vi.mock("@fluxcore/systems/leveling/persistence", () => ({ + getUserLevel: vi.fn().mockResolvedValue(null), + addXp: vi.fn().mockResolvedValue({ leveledUp: true, newLevel: 2 }), +})); +vi.mock("@fluxcore/systems/leveling/xp", () => ({ + applyMultipliers: (xp: number) => xp, +})); +vi.mock("@fluxcore/systems/leveling/constants", () => ({ XP_RANDOMNESS: 1 })); +vi.mock("@fluxcore/systems/leveling/rewards", () => ({ + checkAndGrantRewards: vi.fn(), +})); + +const event = (await import("../../src/events/messageCreate.js")).default; + +describe("level-up announcement allowedMentions", () => { + it("passes allowedMentions to channel.send when announcing in same channel", async () => { + const send = vi.fn().mockResolvedValue(undefined); + const message = { + guild: { id: "g1", channels: { cache: { get: () => null } } }, + author: { id: "u1", bot: false, displayName: "Alice", send: vi.fn() }, + member: { roles: { cache: new Map() } }, + channelId: "c1", + channel: { send }, + } as never; + + await event.execute(message); + + expect(send).toHaveBeenCalledTimes(1); + const arg = send.mock.calls[0][0] as { + content: string; + allowedMentions: { parse: string[]; users: string[] }; + }; + expect(typeof arg).toBe("object"); + expect(arg.content).toContain("@everyone"); + expect(arg.allowedMentions).toEqual({ parse: [], users: ["u1"] }); + }); + + it("passes allowedMentions when announceChannel is configured", async () => { + const { getLevelSettings } = await import("@fluxcore/systems/leveling/config"); + (getLevelSettings as ReturnType).mockResolvedValueOnce({ + enabled: true, + xpPerMessage: 10, + xpCooldownSeconds: 0, + noXpChannels: [], + noXpRoles: [], + multipliers: [], + announceEnabled: true, + announceChannel: "ch-announce", + announceMessage: "<@&modRole> {user} hit {level}!", + }); + + const announceSend = vi.fn().mockResolvedValue(undefined); + const message = { + guild: { + id: "g1", + channels: { + cache: { + get: (id: string) => + id === "ch-announce" + ? { isTextBased: () => true, send: announceSend } + : null, + }, + }, + }, + author: { id: "u1", bot: false, displayName: "Alice", send: vi.fn() }, + member: { roles: { cache: new Map() } }, + channelId: "c1", + channel: { send: vi.fn() }, + } as never; + + await event.execute(message); + + expect(announceSend).toHaveBeenCalledTimes(1); + const arg = announceSend.mock.calls[0][0] as { + allowedMentions: { parse: string[]; users: string[] }; + }; + expect(arg.allowedMentions).toEqual({ + parse: [], + users: ["u1"], + }); + }); +}); diff --git a/apps/bot/tests/events/welcome-sanitize.test.ts b/apps/bot/tests/events/welcome-sanitize.test.ts new file mode 100644 index 0000000..4fe35b1 --- /dev/null +++ b/apps/bot/tests/events/welcome-sanitize.test.ts @@ -0,0 +1,130 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("@fluxcore/systems/logging/config", () => ({ + getLogConfig: vi.fn().mockResolvedValue(null), +})); +vi.mock("@fluxcore/systems/logging/persistence", () => ({ + createLogEntry: vi.fn(), +})); +vi.mock("@fluxcore/systems/logging/sender", () => ({ + sendLogEmbed: vi.fn(), +})); +vi.mock("@fluxcore/systems/logging/formatter", () => ({ + formatMemberJoin: vi.fn(), +})); + +vi.mock("@fluxcore/systems/antiraid/config", () => ({ + getAntiRaidConfig: vi.fn().mockResolvedValue({ enabled: false }), +})); +vi.mock("@fluxcore/systems/antiraid/tracker", () => ({ recordJoin: vi.fn() })); +vi.mock("@fluxcore/systems/antiraid/actions", () => ({ + executeRaidAction: vi.fn(), + lockdownGuild: vi.fn(), +})); +vi.mock("@fluxcore/systems/antiraid/persistence", () => ({ + createRaidEvent: vi.fn(), +})); + +vi.mock("@fluxcore/systems/welcome/config", () => ({ + getWelcomeConfig: vi.fn().mockResolvedValue({ + autoRoleIds: [], + welcomeEnabled: true, + welcomeChannelId: "ch-welcome", + welcomeMessage: "hi", + welcomeImageEnabled: true, + welcomeImageConfig: { sendMode: "with" }, + dmEnabled: false, + dmMessage: "", + }), +})); + +vi.mock("@fluxcore/systems/welcome/builder", () => ({ + buildWelcomeEmbed: vi.fn().mockReturnValue({ + setImage: vi.fn().mockReturnThis(), + }), +})); + +const generateWelcomeImage = vi + .fn() + .mockResolvedValue(Buffer.from("image")); +vi.mock("@fluxcore/systems/welcome/image", () => ({ + generateWelcomeImage: (...args: unknown[]) => generateWelcomeImage(...args), + createStorageAdapter: vi.fn().mockReturnValue({}), + sanitizeDisplayName: (raw: string, maxLen: number): string => { + const BIDI_OVERRIDES = /[\u202A-\u202E\u2066-\u2069]/g; + const ZERO_WIDTH = /[\u200B-\u200D\u2060\uFEFF]/g; + const CONTROL_CHARS = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/g; + const WHITESPACE_RUN = /\s+/g; + if (typeof raw !== "string") return "Unknown"; + let out = raw + .normalize("NFC") + .replace(BIDI_OVERRIDES, "") + .replace(ZERO_WIDTH, "") + .replace(CONTROL_CHARS, "") + .replace(WHITESPACE_RUN, " ") + .trim(); + if (out.length === 0) return "Unknown"; + if (out.length > maxLen) out = out.slice(0, maxLen); + return out; + }, +})); + +const event = ( + await import("../../src/events/guildMemberAdd.js") +).default; + +describe("guildMemberAdd: sanitizes hostile names before canvas render", () => { + beforeEach(() => { + generateWelcomeImage.mockClear(); + }); + + it("strips RTL override and zero-width from username/displayName/guild.name", async () => { + const sentChannel = { + isTextBased: () => true, + send: vi.fn().mockResolvedValue(undefined), + }; + const member = { + id: "m1", + user: { + bot: false, + username: "\u202Eevil\u202Duser", + createdTimestamp: Date.now() - 10 * 86_400_000, + displayAvatarURL: () => "https://cdn/avatar.png", + tag: "tag", + }, + displayName: "a\u200B".repeat(100), + guild: { + id: "g1", + name: "Server\u202Ename", + memberCount: 5, + channels: { cache: { get: () => sentChannel } }, + iconURL: () => null, + members: { me: null }, + roles: { cache: new Map() }, + }, + roles: { cache: new Map(), add: vi.fn() }, + send: vi.fn(), + } as never; + + await event.execute(member); + + expect(generateWelcomeImage).toHaveBeenCalledTimes(1); + const call = generateWelcomeImage.mock.calls[0][0]; + expect(call.member.username).toBe("eviluser"); + expect(call.member.displayName).not.toContain("\u200B"); + expect(call.member.displayName.length).toBeLessThanOrEqual(80); + expect(call.guild.name).toBe("Servername"); + }); +}); diff --git a/apps/bot/tests/features/automation/system/registry-emoji.test.ts b/apps/bot/tests/features/automation/system/registry-emoji.test.ts new file mode 100644 index 0000000..ff4fe28 --- /dev/null +++ b/apps/bot/tests/features/automation/system/registry-emoji.test.ts @@ -0,0 +1,111 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +const warn = vi.hoisted(() => vi.fn()); +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn, error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn().mockResolvedValue({ address: "1.1.1.1", family: 4 }), +})); + +const { getExecutor } = await import( + "../../../../src/features/automation/system/registry.js" +); + +const baseCtx = { + eventType: "messageCreated" as const, + guildId: "g1", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + guildName: "G", + memberCount: 10, + timestamp: new Date().toISOString(), + extra: { "message.id": "m1" }, +}; + +function mkClient(fetchSpy: ReturnType) { + return { + channels: { + fetch: vi.fn().mockResolvedValue({ + isTextBased: () => true, + messages: { fetch: fetchSpy }, + }), + }, + } as never; +} + +describe("addReaction emoji validation", () => { + beforeEach(() => warn.mockClear()); + + it("rejects garbage strings without fetching the message", async () => { + const fetchSpy = vi.fn(); + const client = mkClient(fetchSpy); + + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "not-an-emoji-at-all", + } as never); + + expect(fetchSpy).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("invalid emoji"), + ); + }); + + it("accepts a unicode emoji", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "\u{1F389}", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("accepts a Discord custom emoji <:name:id>", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "<:partyparrot:123456789012345678>", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("accepts an animated custom emoji ", async () => { + const fetchSpy = vi.fn().mockResolvedValue({ react: vi.fn() }); + const client = mkClient(fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: "", + } as never); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("rejects bare colon shortcodes like :smile:", async () => { + const fetchSpy = vi.fn(); + const client = mkClient(fetchSpy); + const executor = getExecutor("addReaction")!; + await executor(client, baseCtx as never, { + type: "addReaction", + emoji: ":smile:", + } as never); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/bot/tests/features/automation/system/registry-webhook.test.ts b/apps/bot/tests/features/automation/system/registry-webhook.test.ts new file mode 100644 index 0000000..04bb413 --- /dev/null +++ b/apps/bot/tests/features/automation/system/registry-webhook.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, + }; +}); + +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn().mockResolvedValue({ address: "93.184.216.34", family: 4 }), +})); + +const { getExecutor } = await import( + "../../../../src/features/automation/system/registry.js" +); + +const baseCtx = { + eventType: "memberJoin" as const, + guildId: "g1", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + guildName: "G", + memberCount: 10, + timestamp: new Date().toISOString(), +}; + +describe("sendWebhook header allowlist", () => { + let fetchSpy: ReturnType; + + beforeEach(() => { + fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("", { status: 200 })); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + async function run(headers: Record) { + const executor = getExecutor("sendWebhook")!; + await executor({} as never, baseCtx as never, { + type: "sendWebhook", + webhook: { + url: "https://example.com/hook", + method: "POST", + headers, + bodyTemplate: "{}", + }, + } as never); + const call = fetchSpy.mock.calls[0]; + return (call?.[1] as { headers: Record } | undefined) + ?.headers as Record; + } + + it("strips Authorization header", async () => { + const sent = await run({ Authorization: "Bearer leaked" }); + expect(sent).toBeDefined(); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain( + "authorization", + ); + }); + + it("strips X-Api-Key header", async () => { + const sent = await run({ "X-Api-Key": "secret" }); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain( + "x-api-key", + ); + }); + + it("strips X-Forwarded-For and X-Forwarded-Host", async () => { + const sent = await run({ + "X-Forwarded-For": "127.0.0.1", + "X-Forwarded-Host": "internal", + }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).not.toContain("x-forwarded-for"); + expect(lower).not.toContain("x-forwarded-host"); + }); + + it("strips Cookie header", async () => { + const sent = await run({ Cookie: "session=abc" }); + expect(Object.keys(sent).map((k) => k.toLowerCase())).not.toContain( + "cookie", + ); + }); + + it("allows X-Idempotency-Key (allowlisted)", async () => { + const sent = await run({ "X-Idempotency-Key": "abc-123" }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).toContain("x-idempotency-key"); + }); + + it("allows custom X-Fluxcore-* headers (allowlisted prefix)", async () => { + const sent = await run({ "X-Fluxcore-Source": "rule-42" }); + const lower = Object.keys(sent).map((k) => k.toLowerCase()); + expect(lower).toContain("x-fluxcore-source"); + }); + + it("always sets Content-Type: application/json", async () => { + const sent = await run({}); + expect(sent["Content-Type"] ?? sent["content-type"]).toBe( + "application/json", + ); + }); +}); diff --git a/apps/bot/tests/features/general/commands/actions-name-validation.test.ts b/apps/bot/tests/features/general/commands/actions-name-validation.test.ts new file mode 100644 index 0000000..59cc3d9 --- /dev/null +++ b/apps/bot/tests/features/general/commands/actions-name-validation.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@fluxcore/config", () => ({ + config: { token: "t", clientId: "c", guildId: undefined, logLevel: "info" }, +})); + +vi.mock("@fluxcore/utils", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkPermissions: vi.fn().mockResolvedValue(true), + }; +}); + +const mockCreateRule = vi.fn(); +const mockGetRuleByName = vi.fn().mockResolvedValue(null); +const mockCountRules = vi.fn().mockResolvedValue(0); +vi.mock("@fluxcore/systems/actions/persistence", () => ({ + createRule: (...a: unknown[]) => mockCreateRule(...a), + updateRule: vi.fn(), + deleteRule: vi.fn(), + getRuleByName: (...a: unknown[]) => mockGetRuleByName(...a), + countRules: (...a: unknown[]) => mockCountRules(...a), + getRecentLogs: vi.fn(), + notifyCacheInvalidation: vi.fn(), +})); + +vi.mock("@fluxcore/systems/actions/cache", () => ({ + addRuleToCache: vi.fn(), + removeRuleFromCache: vi.fn(), + updateRuleInCache: vi.fn(), + getRulesForGuild: vi.fn().mockReturnValue([]), +})); + +vi.mock("@fluxcore/systems/actions/config", () => ({ + getGuildSettingsOrDefault: vi.fn().mockReturnValue({ + maxRules: 25, + globalEnabled: true, + logChannelId: null, + }), + setGuildSettings: vi.fn(), +})); + +const command = ( + await import("../../../../src/features/general/commands/actions.js") +).default; + +function mkInteraction(name: string) { + const replies: unknown[] = []; + return { + options: { + getSubcommand: () => "create", + getString: (k: string, _req?: boolean) => { + if (k === "name") return name; + if (k === "event") return "memberJoin"; + if (k === "action-type") return "sendMessage"; + return null; + }, + getInteger: () => null, + getBoolean: () => null, + getChannel: () => null, + getRole: () => null, + }, + user: { id: "u1" }, + guildId: "g1", + replies, + reply: (payload: unknown) => { + replies.push(payload); + return Promise.resolve(); + }, + } as never; +} + +describe("/actions create — name validation", () => { + beforeEach(() => { + mockCreateRule.mockReset(); + mockGetRuleByName.mockResolvedValue(null); + mockCountRules.mockResolvedValue(0); + }); + + it("rejects @everyone", async () => { + const ix = mkInteraction("@everyone"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + const reply = (ix as { replies: unknown[] }).replies[0]; + expect(JSON.stringify(reply)).toMatch(/Invalid Name/); + }); + + it("rejects backticks", async () => { + const ix = mkInteraction("`evil`"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + }); + + it("rejects zero-width chars", async () => { + const ix = mkInteraction("hi\u200Bthere"); + await command.execute(ix); + expect(mockCreateRule).not.toHaveBeenCalled(); + }); + + it("accepts a normal name", async () => { + mockCreateRule.mockResolvedValue({ + id: 1, + guildId: "g1", + name: "welcome_rule", + enabled: true, + eventType: "memberJoin", + actions: [], + conditions: {}, + priority: 0, + createdBy: "u1", + }); + const ix = mkInteraction("welcome_rule"); + await command.execute(ix); + expect(mockCreateRule).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/bot/tests/features/moderation/commands/warn.test.ts b/apps/bot/tests/features/moderation/commands/warn.test.ts index aa6ea22..3482b96 100644 --- a/apps/bot/tests/features/moderation/commands/warn.test.ts +++ b/apps/bot/tests/features/moderation/commands/warn.test.ts @@ -12,12 +12,14 @@ vi.mock("@fluxcore/config", () => ({ // Mock permissions (keep real embed/logger exports) const mockCheckPermissions = vi.fn().mockResolvedValue(true); +const mockCheckBotPermissions = vi.fn().mockResolvedValue(true); const mockIsAboveTarget = vi.fn().mockReturnValue(true); vi.mock("@fluxcore/utils", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, checkPermissions: (...args: unknown[]) => mockCheckPermissions(...args), + checkBotPermissions: (...args: unknown[]) => mockCheckBotPermissions(...args), isAboveTarget: (...args: unknown[]) => mockIsAboveTarget(...args), }; }); @@ -94,6 +96,7 @@ describe("warn command", () => { beforeEach(() => { vi.clearAllMocks(); mockCheckPermissions.mockResolvedValue(true); + mockCheckBotPermissions.mockResolvedValue(true); mockIsAboveTarget.mockReturnValue(true); mockCreateWarning.mockResolvedValue({ id: 1 }); mockGetWarningCount.mockResolvedValue(1); @@ -246,6 +249,27 @@ describe("warn command", () => { expect(target.send).not.toHaveBeenCalled(); }); + it("aborts when the bot lacks ManageMessages", async () => { + mockCheckPermissions.mockResolvedValueOnce(true); + mockCheckBotPermissions.mockResolvedValueOnce(false); + + const interaction = createMockInteraction(); + await command.execute(interaction as never); + + expect(mockCheckBotPermissions).toHaveBeenCalledTimes(1); + expect(mockCreateWarning).not.toHaveBeenCalled(); + }); + + it("calls checkBotPermissions on the happy path", async () => { + mockCheckPermissions.mockResolvedValueOnce(true); + mockCheckBotPermissions.mockResolvedValueOnce(true); + + const interaction = createMockInteraction(); + await command.execute(interaction as never); + + expect(mockCheckBotPermissions).toHaveBeenCalledTimes(1); + }); + it("handles DM failure silently", async () => { mockGetWarnSettings.mockResolvedValueOnce({ dmOnWarn: true, diff --git a/packages/systems/src/actions/constants.ts b/packages/systems/src/actions/constants.ts index 2b64ce4..642a83d 100644 --- a/packages/systems/src/actions/constants.ts +++ b/packages/systems/src/actions/constants.ts @@ -261,3 +261,14 @@ export const CONDITION_TYPES = [ ] as const; export type ConditionType = (typeof CONDITION_TYPES)[number]; + +/** + * Allowed character set for action rule names. Restricts user-supplied + * names so they cannot inject markdown, mention syntax, code fences, or + * invisible characters into embeds, autocomplete output, or audit logs. + */ +export const RULE_NAME_REGEX = /^[a-zA-Z0-9 _-]{1,50}$/; + +export function isValidRuleName(name: string): boolean { + return typeof name === "string" && RULE_NAME_REGEX.test(name); +} diff --git a/packages/systems/src/actions/persistence.ts b/packages/systems/src/actions/persistence.ts index 9bfdb61..9889672 100644 --- a/packages/systems/src/actions/persistence.ts +++ b/packages/systems/src/actions/persistence.ts @@ -269,6 +269,10 @@ export async function getAnalytics( await Promise.all([ prisma.actionRule.count({ where: { guildId } }), prisma.actionRule.count({ where: { guildId, enabled: true } }), + // SECURITY: tagged-template `$queryRaw` form. Prisma binds ${guildId} + // and ${since} as parameters — DO NOT switch this to $queryRawUnsafe + // or string-concatenate values into the SQL. Covered by + // tests/integration/actions-persistence-sqli.test.ts. prisma.$queryRaw< Array<{ date: string; total: bigint; success: bigint; error: bigint }> >` @@ -348,6 +352,9 @@ export async function getLastFiredByGuild( guildId: string, ): Promise> { const prisma = getPrisma(); + // SECURITY: tagged-template `$queryRaw` form — guildId is bound as a + // parameter. DO NOT convert to $queryRawUnsafe. Covered by + // tests/integration/actions-persistence-sqli.test.ts. const rows = await prisma.$queryRaw< Array<{ ruleId: number; lastFired: Date }> >` diff --git a/packages/systems/src/actions/templateEngine.ts b/packages/systems/src/actions/templateEngine.ts index a071460..0a54fea 100644 --- a/packages/systems/src/actions/templateEngine.ts +++ b/packages/systems/src/actions/templateEngine.ts @@ -28,6 +28,15 @@ function escapeTemplateVars(value: string): string { return value.replace(/\{/g, "\u200B{"); } +/** + * SECURITY: resolveTemplate MUST remain a pure literal-replacement function. + * It must NEVER call eval, new Function, or any templating library that + * evaluates expressions inside braces. User-controlled values from + * ctx.extra are passed through escapeTemplateVars so a hostile + * `message.content` cannot smuggle `{user.id}` into a second resolver + * pass. Both properties are pinned by + * tests/unit/templateEngine-injection.test.ts — do not remove that file. + */ export function resolveTemplate( template: string, context: EventContext, @@ -45,10 +54,17 @@ export function resolveTemplate( } } - // Then resolve core template variables + // Then resolve core template variables. Use a negative lookbehind for the + // zero-width space sentinel so a user-controlled value that was escaped via + // escapeTemplateVars cannot smuggle a core variable into this pass. for (const [variable, resolver] of Object.entries(VARIABLE_MAP)) { - if (result.includes(variable)) { - result = result.replaceAll(variable, resolver(context)); + const escaped = variable.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const pattern = new RegExp(`(? maxLen) out = out.slice(0, maxLen); + return out; +} diff --git a/packages/systems/tests/integration/actions-persistence-sqli.test.ts b/packages/systems/tests/integration/actions-persistence-sqli.test.ts new file mode 100644 index 0000000..ac9cb2e --- /dev/null +++ b/packages/systems/tests/integration/actions-persistence-sqli.test.ts @@ -0,0 +1,90 @@ +/** + * Regression: prisma.$queryRaw in actions/persistence.ts MUST use the + * tagged-template form so guildId is bound as a parameter, not concatenated. + * If anyone switches these call sites to $queryRawUnsafe with string + * interpolation, the malicious guildId below would drop or corrupt rows + * and these assertions would fail. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterAll } from "vitest"; +import { + setupTestDatabase, + cleanTestData, + teardownTestDatabase, +} from "../helpers/db.js"; +import { getPrisma } from "@fluxcore/database"; +import { + getAnalytics, + getLastFiredByGuild, +} from "../../src/actions/persistence.js"; + +const SAFE_GUILD = "guild-safe-1"; +const MALICIOUS_GUILD = `guild-x'; DROP TABLE "ActionLog"; --`; + +describe("actions persistence — $queryRaw SQLi safety", () => { + beforeAll(async () => { + await setupTestDatabase(); + }); + + beforeEach(async () => { + await cleanTestData(); + const prisma = getPrisma(); + await prisma.actionRule.create({ + data: { + guildId: SAFE_GUILD, + name: "rule-1", + enabled: true, + eventType: "memberJoin", + actions: "[]", + conditions: "{}", + priority: 0, + createdBy: "user-1", + }, + }); + await prisma.actionLog.create({ + data: { + guildId: SAFE_GUILD, + ruleId: 0, + ruleName: "rule-1", + eventType: "memberJoin", + actionType: "sendMessage", + success: true, + error: null, + metadata: "{}", + }, + }); + }); + + afterAll(async () => { + await teardownTestDatabase(); + }); + + it("getAnalytics treats malicious guildId as a literal value, not SQL", async () => { + const result = await getAnalytics(MALICIOUS_GUILD, 7); + expect(result.summary.totalExecutions).toBe(0); + expect(result.executionTrend).toEqual([]); + + // ActionLog table must still exist and still contain the safe row + const prisma = getPrisma(); + const stillThere = await prisma.actionLog.count({ + where: { guildId: SAFE_GUILD }, + }); + expect(stillThere).toBe(1); + }); + + it("getLastFiredByGuild treats malicious guildId as a literal value, not SQL", async () => { + const map = await getLastFiredByGuild(MALICIOUS_GUILD); + expect(map.size).toBe(0); + + const prisma = getPrisma(); + const stillThere = await prisma.actionLog.count({ + where: { guildId: SAFE_GUILD }, + }); + expect(stillThere).toBe(1); + }); + + it("getAnalytics returns real data for the legitimate guildId", async () => { + const result = await getAnalytics(SAFE_GUILD, 7); + expect(result.summary.totalExecutions).toBe(1); + }); +}); diff --git a/packages/systems/tests/unit/actions-name.test.ts b/packages/systems/tests/unit/actions-name.test.ts new file mode 100644 index 0000000..71328c9 --- /dev/null +++ b/packages/systems/tests/unit/actions-name.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from "vitest"; +import { + isValidRuleName, + RULE_NAME_REGEX, +} from "../../src/actions/constants.js"; + +describe("isValidRuleName", () => { + it("accepts plain ASCII letters", () => { + expect(isValidRuleName("welcome")).toBe(true); + }); + it("accepts digits, spaces, underscores, hyphens", () => { + expect(isValidRuleName("rule_1 - test")).toBe(true); + }); + it("rejects empty string", () => { + expect(isValidRuleName("")).toBe(false); + }); + it("rejects > 50 chars", () => { + expect(isValidRuleName("a".repeat(51))).toBe(false); + }); + it("rejects @everyone", () => { + expect(isValidRuleName("@everyone")).toBe(false); + }); + it("rejects markdown backticks", () => { + expect(isValidRuleName("`code`")).toBe(false); + }); + it("rejects mention syntax", () => { + expect(isValidRuleName("<@123>")).toBe(false); + }); + it("rejects zero-width characters", () => { + expect(isValidRuleName("hi\u200Bthere")).toBe(false); + }); + it("RULE_NAME_REGEX matches the documented pattern", () => { + expect(RULE_NAME_REGEX.source).toBe("^[a-zA-Z0-9 _-]{1,50}$"); + }); +}); diff --git a/packages/systems/tests/unit/templateEngine-injection.test.ts b/packages/systems/tests/unit/templateEngine-injection.test.ts new file mode 100644 index 0000000..b17d5be --- /dev/null +++ b/packages/systems/tests/unit/templateEngine-injection.test.ts @@ -0,0 +1,75 @@ +import { describe, it, expect } from "vitest"; +import { resolveTemplate } from "../../src/actions/templateEngine.js"; +import type { EventContext } from "../../src/actions/types.js"; + +const baseCtx: EventContext = { + eventType: "memberJoin", + guildId: "g1", + guildName: "Guild", + userId: "u1", + userName: "alice", + userTag: "alice#0001", + userMention: "<@u1>", + channelId: "c1", + memberCount: 5, + timestamp: "2026-04-07T00:00:00.000Z", +}; + +describe("resolveTemplate — literal replacement only", () => { + it("replaces known core variables", () => { + expect(resolveTemplate("hi {user.name}", baseCtx)).toBe("hi alice"); + }); + + it("does NOT evaluate JavaScript expressions inside braces", () => { + expect(resolveTemplate("{1+1}", baseCtx)).toBe("{1+1}"); + expect(resolveTemplate("{process.env.HOME}", baseCtx)).toBe( + "{process.env.HOME}", + ); + }); + + it("does NOT evaluate ${...} interpolation", () => { + // The literal $ and ${...} must be returned unchanged + expect(resolveTemplate("${1+1}", baseCtx)).toBe("${1+1}"); + }); + + it("does NOT recurse into user-controlled extra values", () => { + // A malicious message.content tries to smuggle {user.id} as the value + // for {message.content}. The escape MUST prevent the second resolver + // pass from expanding it. + const ctx: EventContext = { + ...baseCtx, + extra: { "message.content": "{user.id}" }, + }; + const out = resolveTemplate("said: {message.content}", ctx); + expect(out).not.toBe("said: u1"); + expect(out).toContain("\u200B{user.id}"); + }); + + it("does NOT recurse via nested core variables in extra", () => { + const ctx: EventContext = { + ...baseCtx, + extra: { "ban.reason": "{user.tag}{guild}" }, + }; + const out = resolveTemplate("reason: {ban.reason}", ctx); + expect(out).not.toContain("alice#0001"); + expect(out).toContain("\u200B{user.tag}"); + }); + + it("leaves unknown {placeholders} untouched", () => { + expect(resolveTemplate("{this.does.not.exist}", baseCtx)).toBe( + "{this.does.not.exist}", + ); + }); + + it("truncates output to MAX_TEMPLATE_LENGTH", () => { + const big = "x".repeat(5000); + const out = resolveTemplate(big, baseCtx); + expect(out.length).toBeLessThanOrEqual(2000); + }); + + it("never throws on non-string extra values being absent", () => { + expect(() => + resolveTemplate("{user.name} joined {guild}", baseCtx), + ).not.toThrow(); + }); +}); diff --git a/packages/systems/tests/unit/welcome/sanitize.test.ts b/packages/systems/tests/unit/welcome/sanitize.test.ts new file mode 100644 index 0000000..595c8a2 --- /dev/null +++ b/packages/systems/tests/unit/welcome/sanitize.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from "vitest"; +import { sanitizeDisplayName } from "../../../src/welcome/image/sanitize.js"; + +describe("sanitizeDisplayName", () => { + it("returns plain ASCII unchanged", () => { + expect(sanitizeDisplayName("Alice", 32)).toBe("Alice"); + }); + + it("strips zero-width spaces and joiners", () => { + expect(sanitizeDisplayName("a\u200Bb\u200Cc\u200Dd\uFEFFe", 32)).toBe("abcde"); + }); + + it("strips RTL/LTR override characters (U+202A..U+202E)", () => { + expect(sanitizeDisplayName("\u202Eevil\u202D", 32)).toBe("evil"); + }); + + it("strips bidi isolate characters (U+2066..U+2069)", () => { + expect(sanitizeDisplayName("\u2066hidden\u2069", 32)).toBe("hidden"); + }); + + it("strips C0/C1 control characters but keeps spaces", () => { + expect(sanitizeDisplayName("a\u0007 b\u0000c", 32)).toBe("a bc"); + }); + + it("truncates to maxLen", () => { + expect(sanitizeDisplayName("x".repeat(500), 32)).toHaveLength(32); + }); + + it("normalizes to NFC", () => { + // "é" as NFD (U+0065 U+0301) → NFC ("\u00E9") + const decomposed = "e\u0301"; + const out = sanitizeDisplayName(decomposed, 32); + expect(out).toBe("\u00E9"); + }); + + it("returns 'Unknown' for empty/whitespace-only input", () => { + expect(sanitizeDisplayName("", 32)).toBe("Unknown"); + expect(sanitizeDisplayName(" ", 32)).toBe("Unknown"); + expect(sanitizeDisplayName("\u200B\u200C", 32)).toBe("Unknown"); + }); + + it("collapses runs of whitespace", () => { + expect(sanitizeDisplayName("a b\t\tc", 32)).toBe("a b c"); + }); +});