sec: lock down 5 cross-tenant IDORs uncovered in second-pass review
1. HIGH — /api/v1/admin/ports/[id] PATCH+GET let any port-admin (manage_settings) mutate any other tenant's port row by passing the foreign id in the path. Now non-super-admins must target their own ctx.portId; listPorts and createPort are super-admin only. 2. HIGH — Invoice create/update accepted arbitrary expenseIds and linked them into invoice_expenses with no port check; the GET response then re-emitted those foreign expense rows via the linkedExpenses join. assertExpensesInPort now validates each id belongs to the caller's portId before insert; getInvoiceById's join filters by expenses.portId as defense-in-depth. 3. HIGH — Document creation paths (createDocument, createFromWizard, createFromUpload) persisted user-supplied clientId/interestId/ companyId/yachtId/reservationId without verifying those FKs were in-port. sendForSigning then loaded the foreign client/interest by id alone and pushed their PII into the Documenso payload. New assertSubjectFksInPort helper rejects out-of-port FKs at create time; sendForSigning's interest+client lookups now also filter by portId. 4. MEDIUM — calculateInterestScore read its redis cache before verifying portId, and the cache key was interestId-only — a foreign-port caller could observe a cached score breakdown. Cache key now includes portId, and the port-scope DB lookup runs before any cache.get. 5. MEDIUM — AI email-draft job results were retrievable by anyone who could guess the BullMQ jobId (default sequential integers). Job ids are now random UUIDs, requestEmailDraft validates interestId/ clientId belong to ctx.portId before enqueueing, the worker's client lookup is port-scoped, and getEmailDraftResult requires the caller to match the original requester's userId+portId before returning the drafted subject/body. The interest-scoring unit test that asserted "DB is bypassed on cache hit" is updated to reflect the new (security-correct) ordering. Two new regression test files cover the email-draft binding (5 tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
146
tests/integration/email-draft-job-isolation.test.ts
Normal file
146
tests/integration/email-draft-job-isolation.test.ts
Normal file
@@ -0,0 +1,146 @@
|
||||
/**
|
||||
* Security regression: AI email-draft jobs are bound to the requesting
|
||||
* user + port. A foreign caller who knows the jobId must NOT receive the
|
||||
* drafted subject/body.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
|
||||
import { ForbiddenError, ValidationError } from '@/lib/errors';
|
||||
|
||||
// Mock the queue. Each test sets up a fresh per-test job map.
|
||||
const fakeJobs = new Map<string, { data: unknown; returnvalue: unknown; state: string }>();
|
||||
|
||||
vi.mock('@/lib/queue', () => ({
|
||||
getQueue: () => ({
|
||||
add: vi.fn(async (_name: string, data: unknown, opts: { jobId: string }) => {
|
||||
fakeJobs.set(opts.jobId, { data, returnvalue: null, state: 'completed' });
|
||||
return { id: opts.jobId };
|
||||
}),
|
||||
getJob: vi.fn(async (id: string) => {
|
||||
const j = fakeJobs.get(id);
|
||||
if (!j) return null;
|
||||
return {
|
||||
data: j.data,
|
||||
returnvalue: j.returnvalue,
|
||||
getState: async () => j.state,
|
||||
};
|
||||
}),
|
||||
}),
|
||||
}));
|
||||
|
||||
// Mock interest/client lookups so requestEmailDraft doesn't hit the DB.
|
||||
vi.mock('@/lib/db', () => ({
|
||||
db: {
|
||||
query: {
|
||||
interests: {
|
||||
findFirst: vi.fn(async ({ where: _w }) => ({ id: 'iA', portId: 'pA' })),
|
||||
},
|
||||
clients: {
|
||||
findFirst: vi.fn(async ({ where: _w }) => ({ id: 'cA', portId: 'pA' })),
|
||||
},
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
beforeEach(() => {
|
||||
fakeJobs.clear();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('email-draft job binding', () => {
|
||||
it('rejects readers with a different userId', async () => {
|
||||
const { requestEmailDraft, getEmailDraftResult } =
|
||||
await import('@/lib/services/email-draft.service');
|
||||
|
||||
const { jobId } = await requestEmailDraft('user-A', {
|
||||
interestId: 'iA',
|
||||
clientId: 'cA',
|
||||
portId: 'pA',
|
||||
context: 'follow_up',
|
||||
});
|
||||
|
||||
// Wire in a completed return value so a successful path would otherwise
|
||||
// produce a result.
|
||||
fakeJobs.get(jobId)!.returnvalue = {
|
||||
subject: 'leak',
|
||||
body: 'leak',
|
||||
generatedAt: new Date().toISOString(),
|
||||
};
|
||||
|
||||
await expect(getEmailDraftResult(jobId, { userId: 'user-B', portId: 'pA' })).rejects.toThrow(
|
||||
ForbiddenError,
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects readers with a different portId', async () => {
|
||||
const { requestEmailDraft, getEmailDraftResult } =
|
||||
await import('@/lib/services/email-draft.service');
|
||||
|
||||
const { jobId } = await requestEmailDraft('user-A', {
|
||||
interestId: 'iA',
|
||||
clientId: 'cA',
|
||||
portId: 'pA',
|
||||
context: 'follow_up',
|
||||
});
|
||||
fakeJobs.get(jobId)!.returnvalue = {
|
||||
subject: 'leak',
|
||||
body: 'leak',
|
||||
generatedAt: new Date().toISOString(),
|
||||
};
|
||||
|
||||
await expect(getEmailDraftResult(jobId, { userId: 'user-A', portId: 'pB' })).rejects.toThrow(
|
||||
ForbiddenError,
|
||||
);
|
||||
});
|
||||
|
||||
it('returns drafted content to the original requester', async () => {
|
||||
const { requestEmailDraft, getEmailDraftResult } =
|
||||
await import('@/lib/services/email-draft.service');
|
||||
|
||||
const { jobId } = await requestEmailDraft('user-A', {
|
||||
interestId: 'iA',
|
||||
clientId: 'cA',
|
||||
portId: 'pA',
|
||||
context: 'follow_up',
|
||||
});
|
||||
fakeJobs.get(jobId)!.returnvalue = {
|
||||
subject: 'subject-A',
|
||||
body: 'body-A',
|
||||
generatedAt: new Date().toISOString(),
|
||||
};
|
||||
|
||||
const result = await getEmailDraftResult(jobId, { userId: 'user-A', portId: 'pA' });
|
||||
expect(result?.subject).toBe('subject-A');
|
||||
expect(result?.body).toBe('body-A');
|
||||
});
|
||||
|
||||
it('jobId is a UUID, not a sequential integer', async () => {
|
||||
const { requestEmailDraft } = await import('@/lib/services/email-draft.service');
|
||||
|
||||
const { jobId } = await requestEmailDraft('user-A', {
|
||||
interestId: 'iA',
|
||||
clientId: 'cA',
|
||||
portId: 'pA',
|
||||
context: 'follow_up',
|
||||
});
|
||||
// Crude UUID-shape check: 8-4-4-4-12 hex.
|
||||
expect(jobId).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i);
|
||||
});
|
||||
|
||||
it('rejects requests whose interest is not in the supplied port', async () => {
|
||||
const { db } = await import('@/lib/db');
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValueOnce(null);
|
||||
|
||||
const { requestEmailDraft } = await import('@/lib/services/email-draft.service');
|
||||
|
||||
await expect(
|
||||
requestEmailDraft('user-A', {
|
||||
interestId: 'foreign-interest',
|
||||
clientId: 'cA',
|
||||
portId: 'pA',
|
||||
context: 'follow_up',
|
||||
}),
|
||||
).rejects.toThrow(ValidationError);
|
||||
});
|
||||
});
|
||||
@@ -155,9 +155,10 @@ describe('calculateInterestScore', () => {
|
||||
// High engagement: 5 notes, 3 emails, 2 reminders
|
||||
const selectChain = {
|
||||
from: vi.fn().mockReturnThis(),
|
||||
where: vi.fn()
|
||||
.mockResolvedValueOnce([{ value: 5 }]) // notes
|
||||
.mockResolvedValueOnce([{ value: 2 }]) // reminders
|
||||
where: vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce([{ value: 5 }]) // notes
|
||||
.mockResolvedValueOnce([{ value: 2 }]) // reminders
|
||||
.mockResolvedValueOnce([{ value: 3 }]), // emails
|
||||
};
|
||||
(db.select as ReturnType<typeof vi.fn>).mockReturnValue(selectChain);
|
||||
@@ -254,12 +255,20 @@ describe('calculateInterestScore', () => {
|
||||
const selectChain = makeSelectChain(0);
|
||||
(db.select as ReturnType<typeof vi.fn>).mockReturnValue(selectChain);
|
||||
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValue({ ...base, id: 'i6', berthId: 'b1' });
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||
...base,
|
||||
id: 'i6',
|
||||
berthId: 'b1',
|
||||
});
|
||||
const withBerth = await calculateInterestScore('i6', 'p1');
|
||||
expect(withBerth.breakdown.berthLinked).toBe(25);
|
||||
|
||||
(redis.get as ReturnType<typeof vi.fn>).mockResolvedValue(null);
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValue({ ...base, id: 'i7', berthId: null });
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||
...base,
|
||||
id: 'i7',
|
||||
berthId: null,
|
||||
});
|
||||
const withoutBerth = await calculateInterestScore('i7', 'p1');
|
||||
expect(withoutBerth.breakdown.berthLinked).toBe(0);
|
||||
});
|
||||
@@ -269,7 +278,11 @@ describe('calculateInterestScore', () => {
|
||||
await expect(calculateInterestScore('missing', 'p1')).rejects.toThrow('Interest not found');
|
||||
});
|
||||
|
||||
it('returns cached result when redis has a hit', async () => {
|
||||
it('returns cached result when redis has a hit (after port-scope DB check)', async () => {
|
||||
// Security fix: the DB lookup runs FIRST to confirm the interest is
|
||||
// in the caller's port. Only then is the (port-scoped) cache key read.
|
||||
// A test that asserts the DB is bypassed would be asserting the
|
||||
// pre-fix bug; this test asserts the new ordering.
|
||||
const cachedScore = {
|
||||
totalScore: 42,
|
||||
breakdown: {
|
||||
@@ -281,11 +294,26 @@ describe('calculateInterestScore', () => {
|
||||
},
|
||||
calculatedAt: new Date().toISOString(),
|
||||
};
|
||||
(db.query.interests.findFirst as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||
id: 'cached-id',
|
||||
portId: 'p1',
|
||||
clientId: 'c1',
|
||||
createdAt: daysAgo(10),
|
||||
pipelineStage: 'open',
|
||||
eoiStatus: null,
|
||||
contractStatus: null,
|
||||
depositStatus: null,
|
||||
dateEoiSigned: null,
|
||||
dateContractSigned: null,
|
||||
dateDepositReceived: null,
|
||||
berthId: null,
|
||||
});
|
||||
(redis.get as ReturnType<typeof vi.fn>).mockResolvedValue(JSON.stringify(cachedScore));
|
||||
|
||||
const result = await calculateInterestScore('cached-id', 'p1');
|
||||
expect(result.totalScore).toBe(42);
|
||||
// Should NOT hit the database
|
||||
expect(db.query.interests.findFirst).not.toHaveBeenCalled();
|
||||
// Port-scope check: the DB IS hit, but no other queries (notes/threads)
|
||||
// are needed since the cache served the score body.
|
||||
expect(db.query.interests.findFirst).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user