From b4f18537bbf6787d18a9abafce53c557ac36f475 Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Mon, 1 Jun 2026 23:36:37 +0300 Subject: [PATCH] fix(automations): enforce tenant isolation on caller-supplied contactId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The automation engine executes steps via the service-role client, which bypasses RLS. POST /api/automations/engine reads contact_id straight from the request body, so an authenticated attacker could supply another tenant's contact UUID and have the engine modify it (update_contact_field), tag it, or read it back through a condition — bypassing the contacts RLS policy. (GHSA-63cv-2c49-m5v3) - Primary: verify the supplied contactId belongs to the dispatching account before any step runs; refuse silently otherwise. - Defense in depth: scope the service-role contacts writes/reads (update_contact_field, contact_field condition) by account_id. Tag steps rely on the entry-point guard since contact_tags has no account_id column — documented inline. - Add engine.test.ts covering the guard (PoC inverted), the happy path, and the account-scoped write. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/automations/engine.test.ts | 170 +++++++++++++++++++++++++++++ src/lib/automations/engine.ts | 40 +++++++ 2 files changed, 210 insertions(+) create mode 100644 src/lib/automations/engine.test.ts diff --git a/src/lib/automations/engine.test.ts b/src/lib/automations/engine.test.ts new file mode 100644 index 0000000..03684d9 --- /dev/null +++ b/src/lib/automations/engine.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; + +// Shared mock state for the service-role client. Lives in a hoisted block +// so the vi.mock factory below can close over it. +const h = vi.hoisted(() => ({ + state: { + owned: null as { id: string } | null, + automations: [] as Record[], + steps: [] as Record[], + fromCalls: [] as string[], + updateCalls: [] as { table: string; filters: [string, string, unknown][] }[], + }, +})); + +vi.mock("./admin-client", () => { + const { state } = h; + + function resolve(ops: { + table: string; + type: string; + filters: [string, string, unknown][]; + }) { + const { table, type } = ops; + if (table === "contacts") { + if (type === "update") { + state.updateCalls.push({ table, filters: ops.filters }); + return { data: null, error: null }; + } + // ownership guard / condition read + return { data: state.owned, error: null }; + } + if (table === "automations") return { data: state.automations, error: null }; + if (table === "automation_logs") { + if (type === "insert") return { data: { id: "log1" }, error: null }; + if (type === "update") return { data: null, error: null }; + return { data: { steps_executed: [], status: "success" }, error: null }; + } + if (table === "automation_steps") return { data: state.steps, error: null }; + return { data: null, error: null }; + } + + function builder(table: string) { + const ops = { + table, + type: "select", + payload: undefined as unknown, + filters: [] as [string, string, unknown][], + }; + const b: Record = { + select: () => b, + insert: (p: unknown) => ((ops.type = "insert"), (ops.payload = p), b), + update: (p: unknown) => ((ops.type = "update"), (ops.payload = p), b), + delete: () => ((ops.type = "delete"), b), + upsert: (p: unknown) => ((ops.type = "upsert"), (ops.payload = p), b), + eq: (k: string, v: unknown) => (ops.filters.push(["eq", k, v]), b), + gte: () => b, + is: () => b, + order: () => b, + limit: () => b, + single: () => Promise.resolve(resolve(ops)), + maybeSingle: () => Promise.resolve(resolve(ops)), + then: (onF: (v: unknown) => unknown, onR?: (e: unknown) => unknown) => + Promise.resolve(resolve(ops)).then(onF, onR), + }; + return b; + } + + return { + supabaseAdmin: () => ({ + from: (t: string) => { + state.fromCalls.push(t); + return builder(t); + }, + rpc: () => Promise.resolve({ error: null }), + }), + }; +}); + +vi.mock("./meta-send", () => ({ + engineSendText: vi.fn(async () => ({ whatsapp_message_id: "m1" })), + engineSendTemplate: vi.fn(async () => ({ whatsapp_message_id: "m1" })), +})); + +import { runAutomationsForTrigger } from "./engine"; + +const ACCOUNT = "acct-1"; + +beforeEach(() => { + h.state.owned = null; + h.state.automations = []; + h.state.steps = []; + h.state.fromCalls = []; + h.state.updateCalls = []; +}); + +describe("runAutomationsForTrigger — tenant isolation", () => { + it("refuses to dispatch when the contact is not in the account (GHSA-63cv-2c49-m5v3)", async () => { + // Ownership lookup returns nothing — the contact belongs to another tenant. + h.state.owned = null; + // If the guard failed, this automation would run an update_contact_field step. + h.state.automations = [automationWithUpdateStep()]; + h.state.steps = [updateStep()]; + + await runAutomationsForTrigger({ + accountId: ACCOUNT, + triggerType: "new_message_received", + contactId: "victim-contact-uuid", + context: { message_text: "manual trigger" }, + }); + + // Bailed at the guard: never fetched automations, never wrote a contact. + expect(h.state.fromCalls).toContain("contacts"); + expect(h.state.fromCalls).not.toContain("automations"); + expect(h.state.updateCalls).toHaveLength(0); + }); + + it("proceeds past the guard when the contact belongs to the account", async () => { + h.state.owned = { id: "c1" }; + h.state.automations = []; // no matching automations; just prove we got past the guard + + await runAutomationsForTrigger({ + accountId: ACCOUNT, + triggerType: "new_message_received", + contactId: "c1", + context: {}, + }); + + expect(h.state.fromCalls).toContain("automations"); + }); + + it("scopes the update_contact_field write to the automation's account", async () => { + h.state.owned = { id: "c1" }; + h.state.automations = [automationWithUpdateStep()]; + h.state.steps = [updateStep()]; + + await runAutomationsForTrigger({ + accountId: ACCOUNT, + triggerType: "new_message_received", + contactId: "c1", + context: {}, + }); + + expect(h.state.updateCalls).toHaveLength(1); + const filters = h.state.updateCalls[0].filters; + expect(filters).toContainEqual(["eq", "id", "c1"]); + expect(filters).toContainEqual(["eq", "account_id", ACCOUNT]); + }); +}); + +function automationWithUpdateStep() { + return { + id: "a1", + account_id: ACCOUNT, + user_id: "u1", + trigger_type: "new_message_received", + trigger_config: {}, + is_active: true, + }; +} + +function updateStep() { + return { + id: "s1", + automation_id: "a1", + step_type: "update_contact_field", + position: 0, + parent_step_id: null, + step_config: { field: "company", value: "pwned-by-automation" }, + }; +} diff --git a/src/lib/automations/engine.ts b/src/lib/automations/engine.ts index 8a3ab93..e91b45d 100644 --- a/src/lib/automations/engine.ts +++ b/src/lib/automations/engine.ts @@ -57,6 +57,31 @@ export interface DispatchInput { export async function runAutomationsForTrigger(input: DispatchInput): Promise { try { const db = supabaseAdmin() + + // Tenant isolation. `contactId` can be caller-supplied (the manual + // POST /api/automations/engine entrypoint reads it straight from the + // request body), and every step below runs through the service-role + // client, which bypasses RLS. So before any step can touch the + // contact, verify it actually belongs to this account. A foreign or + // forged id is refused silently — callers are fire-and-forget, and a + // distinct error would leak whether a given contact UUID exists. + if (input.contactId) { + const { data: owned, error: ownErr } = await db + .from('contacts') + .select('id') + .eq('id', input.contactId) + .eq('account_id', input.accountId) + .maybeSingle() + if (ownErr) { + console.error('[automations] contact ownership check failed:', ownErr) + return + } + if (!owned) { + console.warn('[automations] contact not in account, refusing dispatch', input.contactId) + return + } + } + const { data: automations, error } = await db .from('automations') .select('*') @@ -368,6 +393,9 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise } case 'add_tag': { + // contact_tags has no account_id column; cross-tenant protection for + // the attacker-supplied contactId comes from the ownership guard in + // runAutomationsForTrigger. const cfg = step.step_config as TagStepConfig if (!args.contactId || !cfg.tag_id) throw new Error('add_tag needs contact + tag_id') await db @@ -380,6 +408,8 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise } case 'remove_tag': { + // See add_tag: tenant scoping relies on the runAutomationsForTrigger + // ownership guard, since contact_tags carries no account_id. const cfg = step.step_config as TagStepConfig if (!args.contactId || !cfg.tag_id) throw new Error('remove_tag needs contact + tag_id') await db @@ -421,10 +451,14 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise if (!allowed.has(cfg.field)) { return `field ${cfg.field} not writable from automations` } + // Defense in depth: scope the service-role write to the account so + // a future caller that skips the entry-point ownership guard still + // cannot write across tenants. await db .from('contacts') .update({ [cfg.field]: cfg.value, updated_at: new Date().toISOString() }) .eq('id', args.contactId) + .eq('account_id', args.automation.account_id) return `${cfg.field} updated` } @@ -517,6 +551,9 @@ async function evaluateCondition(cfg: ConditionStepConfig, args: ExecuteArgs): P switch (cfg.subject) { case 'tag_presence': { if (!args.contactId || !cfg.operand) return false + // contact_tags has no account_id column (its RLS keys off the parent + // contact), so tenant scoping here relies on the contact-ownership + // guard in runAutomationsForTrigger. const { count } = await db .from('contact_tags') .select('id', { count: 'exact', head: true }) @@ -526,10 +563,13 @@ async function evaluateCondition(cfg: ConditionStepConfig, args: ExecuteArgs): P } case 'contact_field': { if (!args.contactId || !cfg.operand) return false + // Scope to the account so the condition can't be turned into a + // cross-tenant read oracle via the service-role client. const { data } = await db .from('contacts') .select(cfg.operand) .eq('id', args.contactId) + .eq('account_id', args.automation.account_id) .maybeSingle() const v = (data as Record | null)?.[cfg.operand] return v != null && String(v) === String(cfg.value ?? '')