Merge pull request #194 from ArnasDon/fix/automation-engine-cross-tenant-contact

fix(automations): enforce tenant isolation on caller-supplied contactId

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<string, unknown>[],
+    steps: [] as Record<string, unknown>[],
+    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<string, unknown> = {
+      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" },
+  };
+}

src/lib/automations/engine.ts

@@ -57,6 +57,31 @@ export interface DispatchInput {
 export async function runAutomationsForTrigger(input: DispatchInput): Promise<void> {
   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<string>
     }
 
     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<string>
     }
 
     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<string>
       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<string, unknown> | null)?.[cfg.operand]
       return v != null && String(v) === String(cfg.value ?? '')