sec: gate super-admin invite minting, OCR settings, and alert mutations
Three findings from the branch security review:
1. HIGH — Privilege escalation via super-admin invite. POST
/api/v1/admin/invitations was gated only by manage_users (held by the
port-scoped director role). The body schema accepted isSuperAdmin
from the request, createCrmInvite persisted it verbatim, and
consumeCrmInvite copied it into userProfiles.isSuperAdmin — granting
the new account cross-tenant access. Now the route rejects
isSuperAdmin=true unless ctx.isSuperAdmin, and createCrmInvite
requires invitedBy.isSuperAdmin as defense-in-depth.
2. HIGH — Receipt-image exfiltration via OCR settings. The route
/api/v1/admin/ocr-settings (and the sibling /test) were wrapped only
in withAuth — any port role including viewer could PUT a swapped
provider apiKey + flip aiEnabled, redirecting every subsequent
receipt scan to attacker infrastructure. Both are now wrapped in
withPermission('admin','manage_settings',…) matching the sibling
admin routes (ai-budget, settings).
3. MEDIUM — Cross-tenant alert IDOR. dismissAlert / acknowledgeAlert
issued UPDATE … WHERE id=? with no portId predicate. Any
authenticated user with a foreign alert UUID could mutate it. Both
service functions now require portId and add it to the WHERE; the
route handlers pass ctx.portId.
The dev-trigger-crm-invite script passes a synthetic super-admin caller
identity since it runs out-of-band.
The two public-form tests randomize their IP prefix per run so a fresh
test process doesn't collide with leftover redis sliding-window entries
from a prior run (publicForm limiter pexpires after 1h).
Two new regression test files cover the fixes (6 tests).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
89
tests/integration/alerts-tenant-isolation.test.ts
Normal file
89
tests/integration/alerts-tenant-isolation.test.ts
Normal file
@@ -0,0 +1,89 @@
|
||||
/**
|
||||
* Security regression: dismissAlert / acknowledgeAlert must filter by
|
||||
* portId so an alert UUID from port A can't be mutated by a session
|
||||
* scoped to port B.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeAll } from 'vitest';
|
||||
import { eq } from 'drizzle-orm';
|
||||
|
||||
import { db } from '@/lib/db';
|
||||
import { alerts } from '@/lib/db/schema/insights';
|
||||
import { user } from '@/lib/db/schema/users';
|
||||
import { dismissAlert, acknowledgeAlert } from '@/lib/services/alerts.service';
|
||||
import { makePort } from '../helpers/factories';
|
||||
|
||||
let TEST_USER_ID = '';
|
||||
|
||||
beforeAll(async () => {
|
||||
// dismissedBy / acknowledgedBy reference user.id; pull any seeded user.
|
||||
const [u] = await db.select({ id: user.id }).from(user).limit(1);
|
||||
if (!u) throw new Error('No user available; run pnpm db:seed first');
|
||||
TEST_USER_ID = u.id;
|
||||
});
|
||||
|
||||
async function makeAlert(portId: string) {
|
||||
const [row] = await db
|
||||
.insert(alerts)
|
||||
.values({
|
||||
portId,
|
||||
ruleId: 'document_overdue',
|
||||
severity: 'medium',
|
||||
title: 'Test alert',
|
||||
link: '/test',
|
||||
fingerprint: `fp-${Math.random().toString(36).slice(2)}`,
|
||||
metadata: {},
|
||||
})
|
||||
.returning({ id: alerts.id });
|
||||
if (!row) throw new Error('failed to insert alert');
|
||||
return row.id;
|
||||
}
|
||||
|
||||
describe('alerts service — tenant isolation', () => {
|
||||
it('dismissAlert is a no-op when called with the wrong portId', async () => {
|
||||
const portA = await makePort();
|
||||
const portB = await makePort();
|
||||
const alertId = await makeAlert(portA.id);
|
||||
|
||||
// Attacker session scoped to port B tries to dismiss port A's alert.
|
||||
await dismissAlert(alertId, portB.id, TEST_USER_ID);
|
||||
|
||||
const after = await db.query.alerts.findFirst({ where: eq(alerts.id, alertId) });
|
||||
expect(after?.dismissedAt).toBeNull();
|
||||
expect(after?.dismissedBy).toBeNull();
|
||||
});
|
||||
|
||||
it('dismissAlert succeeds when portId matches', async () => {
|
||||
const port = await makePort();
|
||||
const alertId = await makeAlert(port.id);
|
||||
|
||||
await dismissAlert(alertId, port.id, TEST_USER_ID);
|
||||
|
||||
const after = await db.query.alerts.findFirst({ where: eq(alerts.id, alertId) });
|
||||
expect(after?.dismissedAt).not.toBeNull();
|
||||
expect(after?.dismissedBy).toBe(TEST_USER_ID);
|
||||
});
|
||||
|
||||
it('acknowledgeAlert is a no-op when called with the wrong portId', async () => {
|
||||
const portA = await makePort();
|
||||
const portB = await makePort();
|
||||
const alertId = await makeAlert(portA.id);
|
||||
|
||||
await acknowledgeAlert(alertId, portB.id, TEST_USER_ID);
|
||||
|
||||
const after = await db.query.alerts.findFirst({ where: eq(alerts.id, alertId) });
|
||||
expect(after?.acknowledgedAt).toBeNull();
|
||||
expect(after?.acknowledgedBy).toBeNull();
|
||||
});
|
||||
|
||||
it('acknowledgeAlert succeeds when portId matches', async () => {
|
||||
const port = await makePort();
|
||||
const alertId = await makeAlert(port.id);
|
||||
|
||||
await acknowledgeAlert(alertId, port.id, TEST_USER_ID);
|
||||
|
||||
const after = await db.query.alerts.findFirst({ where: eq(alerts.id, alertId) });
|
||||
expect(after?.acknowledgedAt).not.toBeNull();
|
||||
expect(after?.acknowledgedBy).toBe(TEST_USER_ID);
|
||||
});
|
||||
});
|
||||
32
tests/integration/crm-invite-super-admin-gate.test.ts
Normal file
32
tests/integration/crm-invite-super-admin-gate.test.ts
Normal file
@@ -0,0 +1,32 @@
|
||||
/**
|
||||
* Security regression: only an existing super-admin caller can mint a
|
||||
* super-admin CRM invitation. A port `director` (or any caller without
|
||||
* `invitedBy.isSuperAdmin === true`) must be rejected at the service layer
|
||||
* even if the route handler somehow lets the body flag through.
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
|
||||
import { createCrmInvite } from '@/lib/services/crm-invite.service';
|
||||
import { ValidationError } from '@/lib/errors';
|
||||
|
||||
describe('createCrmInvite — super-admin gate', () => {
|
||||
it('rejects super-admin invites when caller is not a super-admin', async () => {
|
||||
await expect(
|
||||
createCrmInvite({
|
||||
email: `attacker-${Date.now()}@example.test`,
|
||||
isSuperAdmin: true,
|
||||
invitedBy: { userId: 'director-id', isSuperAdmin: false },
|
||||
}),
|
||||
).rejects.toThrow(ValidationError);
|
||||
});
|
||||
|
||||
it('rejects super-admin invites when invitedBy is omitted entirely', async () => {
|
||||
await expect(
|
||||
createCrmInvite({
|
||||
email: `attacker-${Date.now()}-noctx@example.test`,
|
||||
isSuperAdmin: true,
|
||||
}),
|
||||
).rejects.toThrow(ValidationError);
|
||||
});
|
||||
});
|
||||
@@ -18,12 +18,15 @@ vi.mock('@/lib/services/inquiry-notifications.service', () => ({
|
||||
sendInquiryNotifications: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
// The rate-limiter is keyed by IP header and persists across requests inside a
|
||||
// single process. Use a unique IP per test call to avoid 429s.
|
||||
// The rate-limiter is keyed by IP header and is now redis-backed; entries
|
||||
// pexpire after the publicForm window (1h). Randomize the high octets so a
|
||||
// fresh test run doesn't collide with leftover redis state from a previous
|
||||
// run sharing the same redis instance.
|
||||
const IP_PREFIX = `10.${Math.floor(Math.random() * 200) + 10}`;
|
||||
let ipCounter = 1;
|
||||
function uniqueIp(): string {
|
||||
ipCounter += 1;
|
||||
return `10.0.${Math.floor(ipCounter / 255) % 255}.${ipCounter % 255}`;
|
||||
return `${IP_PREFIX}.${Math.floor(ipCounter / 255) % 255}.${ipCounter % 255}`;
|
||||
}
|
||||
|
||||
describe('POST /api/public/interests — trio creation', () => {
|
||||
|
||||
@@ -17,10 +17,13 @@ import { makeMockRequest } from '../helpers/route-tester';
|
||||
vi.mock('@/lib/socket/server', () => ({ emitToRoom: vi.fn() }));
|
||||
vi.mock('@/lib/email', () => ({ sendEmail: vi.fn().mockResolvedValue(undefined) }));
|
||||
|
||||
// Randomize per-run prefix so leftover redis sliding-window entries from a
|
||||
// previous run don't 429 the new run (publicForm limiter pexpires after 1h).
|
||||
const IP_PREFIX = `10.${Math.floor(Math.random() * 200) + 10}`;
|
||||
let ipCounter = 1;
|
||||
function uniqueIp(): string {
|
||||
ipCounter += 1;
|
||||
return `10.50.${Math.floor(ipCounter / 255) % 255}.${ipCounter % 255}`;
|
||||
return `${IP_PREFIX}.${Math.floor(ipCounter / 255) % 255}.${ipCounter % 255}`;
|
||||
}
|
||||
|
||||
describe('POST /api/public/residential-inquiries', () => {
|
||||
|
||||
Reference in New Issue
Block a user