mirror of
https://github.com/Luchexx/wacrm.git
synced 2026-11-02 09:19:51 +00:00
fix(automations): enforce tenant isolation on caller-supplied contactId
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) <noreply@anthropic.com>
This commit is contained in:
parent
19a1df275a
commit
b4f18537bb
170
src/lib/automations/engine.test.ts
Normal file
170
src/lib/automations/engine.test.ts
Normal file
@ -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" },
|
||||||
|
};
|
||||||
|
}
|
||||||
@ -57,6 +57,31 @@ export interface DispatchInput {
|
|||||||
export async function runAutomationsForTrigger(input: DispatchInput): Promise<void> {
|
export async function runAutomationsForTrigger(input: DispatchInput): Promise<void> {
|
||||||
try {
|
try {
|
||||||
const db = supabaseAdmin()
|
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
|
const { data: automations, error } = await db
|
||||||
.from('automations')
|
.from('automations')
|
||||||
.select('*')
|
.select('*')
|
||||||
@ -368,6 +393,9 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise<string>
|
|||||||
}
|
}
|
||||||
|
|
||||||
case 'add_tag': {
|
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
|
const cfg = step.step_config as TagStepConfig
|
||||||
if (!args.contactId || !cfg.tag_id) throw new Error('add_tag needs contact + tag_id')
|
if (!args.contactId || !cfg.tag_id) throw new Error('add_tag needs contact + tag_id')
|
||||||
await db
|
await db
|
||||||
@ -380,6 +408,8 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise<string>
|
|||||||
}
|
}
|
||||||
|
|
||||||
case 'remove_tag': {
|
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
|
const cfg = step.step_config as TagStepConfig
|
||||||
if (!args.contactId || !cfg.tag_id) throw new Error('remove_tag needs contact + tag_id')
|
if (!args.contactId || !cfg.tag_id) throw new Error('remove_tag needs contact + tag_id')
|
||||||
await db
|
await db
|
||||||
@ -421,10 +451,14 @@ async function runStep(step: AutomationStep, args: ExecuteArgs): Promise<string>
|
|||||||
if (!allowed.has(cfg.field)) {
|
if (!allowed.has(cfg.field)) {
|
||||||
return `field ${cfg.field} not writable from automations`
|
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
|
await db
|
||||||
.from('contacts')
|
.from('contacts')
|
||||||
.update({ [cfg.field]: cfg.value, updated_at: new Date().toISOString() })
|
.update({ [cfg.field]: cfg.value, updated_at: new Date().toISOString() })
|
||||||
.eq('id', args.contactId)
|
.eq('id', args.contactId)
|
||||||
|
.eq('account_id', args.automation.account_id)
|
||||||
return `${cfg.field} updated`
|
return `${cfg.field} updated`
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -517,6 +551,9 @@ async function evaluateCondition(cfg: ConditionStepConfig, args: ExecuteArgs): P
|
|||||||
switch (cfg.subject) {
|
switch (cfg.subject) {
|
||||||
case 'tag_presence': {
|
case 'tag_presence': {
|
||||||
if (!args.contactId || !cfg.operand) return false
|
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
|
const { count } = await db
|
||||||
.from('contact_tags')
|
.from('contact_tags')
|
||||||
.select('id', { count: 'exact', head: true })
|
.select('id', { count: 'exact', head: true })
|
||||||
@ -526,10 +563,13 @@ async function evaluateCondition(cfg: ConditionStepConfig, args: ExecuteArgs): P
|
|||||||
}
|
}
|
||||||
case 'contact_field': {
|
case 'contact_field': {
|
||||||
if (!args.contactId || !cfg.operand) return false
|
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
|
const { data } = await db
|
||||||
.from('contacts')
|
.from('contacts')
|
||||||
.select(cfg.operand)
|
.select(cfg.operand)
|
||||||
.eq('id', args.contactId)
|
.eq('id', args.contactId)
|
||||||
|
.eq('account_id', args.automation.account_id)
|
||||||
.maybeSingle()
|
.maybeSingle()
|
||||||
const v = (data as Record<string, unknown> | null)?.[cfg.operand]
|
const v = (data as Record<string, unknown> | null)?.[cfg.operand]
|
||||||
return v != null && String(v) === String(cfg.value ?? '')
|
return v != null && String(v) === String(cfg.value ?? '')
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user