fix(projects): stop the cockpit offering email controls the API rejects (stable) (#977)

Closes #969 on stable. Backport of #976.

The cockpit's email feed rendered preview/resend/cancel/retry/send-now for every mail regardless of role or permission, producing 404s (CRM document mail has no event_id; project ownership does not imply event ownership) and 403s (preview needs events.view, the write actions need email.send).

getProjectOverview now stamps each email with an authoritative canAct, mirroring filterOwnedEventIds; created_by is selected only for that check and stripped before the response. A missing canAct reads as false.
This commit is contained in:
Paul Nothaft
2026-08-03 14:49:04 +02:00
committed by GitHub
parent cc49f6997a
commit 2d0e6ab2dc
6 changed files with 379 additions and 9 deletions
@@ -0,0 +1,149 @@
/**
* getProjectOverview stamps each email with `canAct` — whether the queued-mail
* routes (requireOwnedQueuedEmail) would actually accept an action on it.
*
* The cockpit used to derive this client-side from `event_id != null`, which is
* weaker than the backend rule in a way that still produced dead controls:
* requireOwnedQueuedEmail ALSO requires ownership of that event, while
* getProjectOverview lists the project's events by project_id alone. Project
* ownership does not imply event ownership — ownedProjectsSubquery's
* `projects.created_by = admin.id` branch places no constraint on the linked
* events' owners, so a super_admin can attach admin B's event to admin A's
* project. See #969 / codex review round 1.
*/
const path = require('path');
const fs = require('fs');
const os = require('os');
process.env.NODE_ENV = 'test';
process.env.TEST_DATABASE_PATH = path.join(
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-canact-')), 'db.sqlite',
);
process.env.JWT_SECRET = process.env.JWT_SECRET || 'canact-test-secret';
const bcrypt = require('bcrypt');
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
describe('getProjectOverview email canAct (#969)', () => {
let db; let cleanup; let projectService;
let adminA; let adminB; let superAdmin;
let projectId; let ownEventId; let foreignEventId; let ownerlessEventId;
const mkAdmin = async (username, roleName) => {
const role = await db('roles').where({ name: roleName }).first();
const r = await db('admin_users').insert({
username, email: `${username}@example.com`,
password_hash: await bcrypt.hash('Passw0rd!', 4),
role_id: role.id, is_active: 1,
created_at: new Date().toISOString(), updated_at: new Date().toISOString(),
}).returning('id');
return r[0]?.id ?? r[0];
};
const mkEvent = async (slug, createdBy, project) => {
const r = await db('events').insert({
slug, event_type: 'wedding', event_name: slug, event_date: '2026-08-01',
host_email: '[email protected]', admin_email: '[email protected]', password_hash: 'x',
share_token: `t-${slug}`, share_link: `/g/${slug}/t-${slug}`,
created_by: createdBy, project_id: project,
expires_at: new Date(Date.now() + 864e5).toISOString(),
is_active: 1, is_archived: 0, is_draft: 0,
created_at: new Date().toISOString(),
}).returning('id');
return r[0]?.id ?? r[0];
};
const mkMail = async (eventId, type) => {
const r = await db('email_queue').insert({
recipient_email: '[email protected]', email_type: type, status: 'sent',
event_id: eventId,
created_at: new Date().toISOString(), sent_at: new Date().toISOString(),
}).returning('id');
return r[0]?.id ?? r[0];
};
beforeAll(async () => {
({ db, cleanup } = await bootCrmDb());
await seedMinimal(db);
projectService = require('../../src/services/projectService');
adminA = await mkAdmin('canact-a', 'editor');
adminB = await mkAdmin('canact-b', 'editor');
superAdmin = await mkAdmin('canact-root', 'super_admin');
const p = await db('projects').insert({
name: 'Cockpit canAct', status: 'active', created_by: adminA,
created_at: new Date().toISOString(), updated_at: new Date().toISOString(),
}).returning('id');
projectId = p[0]?.id ?? p[0];
// All three hang off adminA's project. Only the first is adminA's; the
// third is an ownerless legacy row, which filterOwnedEventIds treats as
// owned by whoever asks — but only once we know who is asking.
ownEventId = await mkEvent('canact-own', adminA, projectId);
foreignEventId = await mkEvent('canact-foreign', adminB, projectId);
ownerlessEventId = await mkEvent('canact-legacy', null, projectId);
await mkMail(ownEventId, 'gallery_ready');
await mkMail(foreignEventId, 'gallery_ready');
await mkMail(ownerlessEventId, 'gallery_ready');
}, 120000);
afterAll(async () => { if (cleanup) await cleanup(); });
const byEvent = (overview) => {
const m = new Map();
for (const e of overview.emails) m.set(e.eventId, e);
return m;
};
it('clears mail on an event the caller owns', async () => {
const overview = await projectService.getProjectOverview(
projectId, {}, { id: adminA, roleName: 'editor' },
);
expect(byEvent(overview).get(ownEventId).canAct).toBe(true);
});
it('denies mail on a foreign admin\'s event inside the caller\'s own project', async () => {
const overview = await projectService.getProjectOverview(
projectId, {}, { id: adminA, roleName: 'editor' },
);
// event_id is non-null here — the old client-side rule would have offered
// controls, and requireOwnedQueuedEmail would have 404'd them.
const row = byEvent(overview).get(foreignEventId);
expect(row.eventId).not.toBeNull();
expect(row.canAct).toBe(false);
});
it('clears everything for a super_admin', async () => {
const overview = await projectService.getProjectOverview(
projectId, {}, { id: superAdmin, roleName: 'super_admin' },
);
expect(overview.emails.every((e) => e.canAct === true)).toBe(true);
});
it('clears mail on an ownerless legacy event for an identified caller', async () => {
// Parity with filterOwnedEventIds, which allows created_by IS NULL.
const overview = await projectService.getProjectOverview(
projectId, {}, { id: adminA, roleName: 'editor' },
);
expect(byEvent(overview).get(ownerlessEventId).canAct).toBe(true);
});
it('denies everything when no admin context is supplied', async () => {
// Including the ownerless event: `created_by == null` must not read as
// "owned" when we do not know who is asking (codex review round 2).
const overview = await projectService.getProjectOverview(projectId, {});
expect(overview.emails.length).toBe(3);
expect(overview.emails.every((e) => e.canAct === false)).toBe(true);
});
it('does not leak event ownership to the client', async () => {
const overview = await projectService.getProjectOverview(
projectId, {}, { id: adminA, roleName: 'editor' },
);
expect(overview.events.length).toBe(3);
for (const e of overview.events) expect(e).not.toHaveProperty('created_by');
});
});
+1 -1
View File
@@ -183,7 +183,7 @@ router.get('/:id/overview', requirePermission('events.view'), requireProjectOwne
quotes: await userHasAnyPermission(req.admin.id, ['quotes.view']),
contracts: await userHasAnyPermission(req.admin.id, ['contracts.view']),
};
const overview = await projectService.getProjectOverview(parseInt(req.params.id, 10), perms);
const overview = await projectService.getProjectOverview(parseInt(req.params.id, 10), perms, req.admin);
return successResponse(res, overview);
}));
+40 -3
View File
@@ -454,16 +454,48 @@ function computeValuation(invoices = [], quotes = []) {
* Full overview aggregation for the cockpit. Returns the project, its events,
* and the rolled-up emails / quotes / contracts / invoices / hours + a
* timeline of milestones. `perms` gates which doc types are included.
*
* `admin` (optional) is used only to stamp each email with `canAct` — whether
* the queued-mail routes would actually accept an action on it. See below.
*/
async function getProjectOverview(id, perms = {}) {
async function getProjectOverview(id, perms = {}, admin = null) {
const project = await getProjectById(id);
if (!project) throw new AppError('Project not found', 404);
const events = await db('events')
// `created_by` is selected for the ownership check below and stripped again
// before the response — the cockpit has no business learning who owns a
// sibling event.
const eventRows = await db('events')
.where({ project_id: id })
.select('id', 'event_name', 'event_date', 'slug', 'is_active', 'is_draft', 'expires_at', 'is_archived');
.select('id', 'event_name', 'event_date', 'slug', 'is_active', 'is_draft', 'expires_at', 'is_archived', 'created_by');
const events = eventRows.map(({ created_by: _ignored, ...e }) => e);
const eventIds = events.map((e) => e.id);
// Which of this project's events would filterOwnedEventIds() let `admin`
// act on. Mirrors that predicate exactly (ownership.js): super_admin gets
// everything, otherwise created_by IS NULL OR created_by = admin.id.
//
// Project ownership does NOT imply event ownership — ownedProjectsSubquery's
// `projects.created_by = admin.id` branch places no constraint on who owns
// the linked events, so a super_admin can attach admin B's event to admin
// A's project. Deriving actionability from `event_id != null` alone (as the
// UI first did) would then still render controls that requireOwnedQueuedEmail
// rejects with a 404.
// No admin context → nothing is actionable. Without this, an ownerless
// (legacy/system) event would satisfy `created_by == null` and be marked
// actionable for a caller we know nothing about.
const isSuperAdmin = admin?.roleName === 'super_admin';
let actionableEventIds = new Set();
if (isSuperAdmin) {
actionableEventIds = new Set(eventIds);
} else if (admin?.id != null) {
actionableEventIds = new Set(
eventRows
.filter((e) => e.created_by == null || Number(e.created_by) === Number(admin.id))
.map((e) => e.id),
);
}
const out = { project, events, emails: [], quotes: [], contracts: [], invoices: [], hours: { entries: [], totalMinutes: 0 } };
// Invoices (by event) incl. storno.
@@ -518,6 +550,11 @@ async function getProjectOverview(id, perms = {}) {
queuedAt: e.created_at, sentAt: e.sent_at, error: e.error_message, eventId: e.event_id,
// false → the cockpit preview will re-render from the current template.
stored: !!Number(e.has_rendered),
// Would requireOwnedQueuedEmail accept preview/resend/cancel/retry/send-now
// on this row? Authoritative here because the client cannot derive it: CRM
// document mail has no event to own, and event mail additionally requires
// ownership of THAT event, which the response deliberately does not expose.
canAct: isSuperAdmin || (e.event_id != null && actionableEventIds.has(e.event_id)),
});
const emailRows = [];
@@ -27,6 +27,7 @@ import { useLocalizedDate } from '../../../hooks/useLocalizedDate';
import { useMutationWithToast } from '../../../hooks';
import { formatMoneyMinor } from '../../../utils/money';
import { useFeatureFlags, type FeatureKey } from '../../../contexts/FeatureFlagsContext';
import { usePermissions } from '../../../contexts/PermissionsContext';
type FeedKind = 'email' | 'quote' | 'contract' | 'invoice' | 'gallery' | 'hours';
@@ -42,6 +43,9 @@ interface FeedItem {
emailId?: number;
emailStatus?: string;
reRendered?: boolean;
/** Server's verdict on whether the queued-mail routes would accept an action
* on this row. Not derivable client-side see canActOnEmail. */
emailCanAct?: boolean;
}
/** The feature flag that gates each document's detail ROUTE (RequireFeature
@@ -109,8 +113,23 @@ export const ProjectCockpitPage: React.FC = () => {
const navigate = useNavigate();
const qc = useQueryClient();
const { flags } = useFeatureFlags();
const { hasPermission } = usePermissions();
const { format, formatTime } = useLocalizedDate();
// Which email controls are actually reachable for THIS admin, so the feed
// stops offering buttons the API will reject (#969):
// 404 — requireOwnedQueuedEmail (adminProjects.js) scopes queued mail
// through email_queue.event_id AND ownership of that event. CRM
// document mail carries no event_id at all. The server decides this
// for us (`canAct`) because neither half is derivable here: the
// overview deliberately does not expose who owns a sibling event.
// Absent (older backend) → treat as not actionable, so we fail to
// hiding a live control rather than offering a dead one.
// 403 — preview needs `events.view`, but resend/cancel/retry/send-now
// need `email.send`; the feed used to render all four regardless.
const canActOnEmail = (item: FeedItem) => item.emailCanAct === true;
const canSendEmail = hasPermission('email.send');
const [editName, setEditName] = useState<string | null>(null);
const [preview, setPreview] = useState<EmailPreview | null>(null);
const [previewLoading, setPreviewLoading] = useState(false);
@@ -189,6 +208,7 @@ export const ProjectCockpitPage: React.FC = () => {
title: t(`projects.feed.email`, 'Email') + ` · ${e.type}`,
subtitle: e.recipient + (e.error ? `${e.error}` : ''),
status: e.status, emailId: e.id, emailStatus: e.status, reRendered: !e.stored,
emailCanAct: e.canAct,
});
}
for (const q of data.quotes) {
@@ -376,7 +396,9 @@ export const ProjectCockpitPage: React.FC = () => {
// not a dead strip next to them). Hours have neither → static.
const onRowClick = item.href
? () => navigate(item.href as string)
: (item.kind === 'email' && item.emailId != null ? () => openPreview(item.emailId as number) : undefined);
: (item.kind === 'email' && item.emailId != null && canActOnEmail(item)
? () => openPreview(item.emailId as number)
: undefined);
return (
<li
key={item.key}
@@ -397,7 +419,7 @@ export const ProjectCockpitPage: React.FC = () => {
<span className="inline-block rounded-full px-2 py-0.5 text-xs bg-neutral-100 dark:bg-neutral-700 text-neutral-600 dark:text-neutral-300">{item.status}</span>
)}
{item.amount && <span className="text-xs font-medium text-neutral-700 dark:text-neutral-300">{item.amount}</span>}
{item.kind === 'email' && item.emailId != null && (
{item.kind === 'email' && item.emailId != null && canActOnEmail(item) && (
<div className="flex items-center gap-1" onClick={(e) => e.stopPropagation()}>
<button onClick={() => openPreview(item.emailId as number)} className="inline-flex items-center gap-1 text-xs text-primary-600 hover:underline">
<Eye className="w-3 h-3" />{t('projects.email.preview', 'Preview')}
@@ -410,12 +432,12 @@ export const ProjectCockpitPage: React.FC = () => {
{t('projects.email.reRenderedTag', '≈ re-rendered')}
</span>
)}
{item.emailStatus === 'sent' && (
{canSendEmail && item.emailStatus === 'sent' && (
<button onClick={() => emailActionMutation.mutate({ action: 'resend', emailId: item.emailId as number })} className="inline-flex items-center gap-1 text-xs text-neutral-600 dark:text-neutral-300 hover:underline">
<Send className="w-3 h-3" />{t('projects.email.resend', 'Resend')}
</button>
)}
{item.emailStatus === 'pending' && (
{canSendEmail && item.emailStatus === 'pending' && (
<>
<button onClick={() => emailActionMutation.mutate({ action: 'sendNow', emailId: item.emailId as number })} className="inline-flex items-center gap-1 text-xs text-neutral-600 dark:text-neutral-300 hover:underline">
<Send className="w-3 h-3" />{t('projects.email.sendNow', 'Send now')}
@@ -425,7 +447,7 @@ export const ProjectCockpitPage: React.FC = () => {
</button>
</>
)}
{item.emailStatus === 'failed' && (
{canSendEmail && item.emailStatus === 'failed' && (
<button onClick={() => emailActionMutation.mutate({ action: 'retry', emailId: item.emailId as number })} className="inline-flex items-center gap-1 text-xs text-amber-600 hover:underline">
<RotateCw className="w-3 h-3" />{t('projects.email.retry', 'Retry')}
</button>
@@ -0,0 +1,158 @@
/**
* The cockpit's email feed used to render preview/resend/cancel/retry/send-now
* for every mail, consulting neither the caller's role nor their permissions.
* Two ways that produced dead controls (#969):
*
* 404 requireOwnedQueuedEmail scopes queued mail through
* email_queue.event_id. CRM document mail (quote/contract/invoice/
* storno) is queued with event_id = null, so for a non-super_admin it
* has no ownable parent and always 404s. Introduced by the GHSA-93x4
* fix in #960/#966, which added the ownership middleware.
* 403 preview requires `events.view` but the four write actions require
* `email.send`; the feed rendered all of them regardless.
*/
import { render, screen, waitFor } from '@testing-library/react';
import { vi, describe, it, expect, beforeEach } from 'vitest';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { MemoryRouter, Routes, Route } from 'react-router-dom';
vi.mock('react-i18next', async () => {
const actual = await vi.importActual<typeof import('react-i18next')>('react-i18next');
return {
...actual,
useTranslation: () => ({
t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k),
i18n: { language: 'en' },
}),
};
});
vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn() } }));
// An event mail (actionable by anyone who owns the event) and a CRM document
// mail (event_id null → only a super_admin can act on it). Both 'sent', so the
// Resend button is the one under test in each row.
const OVERVIEW = {
project: {
id: 1, name: 'Hochzeit Müller', customerAccountId: 1,
customerEmail: '[email protected]', status: 'active',
createdAt: '2026-07-01T00:00:00Z', updatedAt: '2026-08-01T00:00:00Z',
},
events: [], quotes: [], contracts: [], invoices: [],
hours: { entries: [], totalMinutes: 0 },
milestones: [],
valuation: { byCurrency: [] },
emails: [
// Event mail the caller owns → the server says actionable.
{ id: 11, recipient: '[email protected]', type: 'gallery_ready', status: 'sent',
queuedAt: '2026-08-01T10:00:00Z', sentAt: '2026-08-01T10:00:05Z', error: null,
eventId: 42, stored: true, canAct: true },
// CRM document mail — no event to own, so 404 for a non-super_admin.
{ id: 12, recipient: '[email protected]', type: 'invoice_sent', status: 'sent',
queuedAt: '2026-08-01T11:00:00Z', sentAt: '2026-08-01T11:00:05Z', error: null,
eventId: null, stored: true, canAct: false },
// Event mail for a FOREIGN event: a super_admin can attach admin B's event
// to admin A's project, and project ownership does not imply event
// ownership — so eventId is non-null yet the action still 404s.
{ id: 13, recipient: '[email protected]', type: 'gallery_ready', status: 'sent',
queuedAt: '2026-08-01T12:00:00Z', sentAt: '2026-08-01T12:00:05Z', error: null,
eventId: 99, stored: true, canAct: false },
],
};
/** The same payload as a super_admin sees it — the server stamps every row. */
const OVERVIEW_SUPER = {
...OVERVIEW,
emails: OVERVIEW.emails.map((e) => ({ ...e, canAct: true })),
};
let currentOverview: typeof OVERVIEW = OVERVIEW;
vi.mock('../../../../services/projects.service', () => ({
projectsService: {
overview: vi.fn(async () => currentOverview),
emailPreview: vi.fn(async () => ({ html: '<p>x</p>', subject: 's' })),
resendEmail: vi.fn(), cancelEmail: vi.fn(), retryEmail: vi.fn(), sendEmailNow: vi.fn(),
update: vi.fn(), assignEvent: vi.fn(),
},
}));
vi.mock('../../../../services/events.service', () => ({
eventsService: { getEvents: vi.fn(async () => []) },
}));
vi.mock('../../../../contexts/FeatureFlagsContext', () => ({
useFeatureFlags: () => ({ flags: { projects: true, quotes: true, contracts: true, bills: true } }),
}));
let mockPerms = { isSuperAdmin: false, permissions: ['events.view'] };
vi.mock('../../../../contexts/PermissionsContext', () => ({
usePermissions: () => ({
isSuperAdmin: mockPerms.isSuperAdmin,
hasPermission: (p: string) => mockPerms.isSuperAdmin || mockPerms.permissions.includes(p),
}),
}));
import { ProjectCockpitPage } from '../ProjectCockpitPage';
const renderPage = async () => {
const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={qc}>
<MemoryRouter initialEntries={['/admin/projects/1']}>
<Routes>
<Route path="/admin/projects/:id" element={<ProjectCockpitPage />} />
</Routes>
</MemoryRouter>
</QueryClientProvider>,
);
await waitFor(() => expect(screen.getAllByText(/Email/).length).toBeGreaterThan(0));
};
describe('ProjectCockpitPage email controls (#969)', () => {
beforeEach(() => {
mockPerms = { isSuperAdmin: false, permissions: ['events.view'] };
currentOverview = OVERVIEW;
});
it('super_admin sees controls on every row', async () => {
mockPerms = { isSuperAdmin: true, permissions: [] };
currentOverview = OVERVIEW_SUPER;
await renderPage();
// All three rows, Resend on all (super_admin implies email.send).
expect(screen.getAllByText('Preview')).toHaveLength(3);
expect(screen.getAllByText('Resend')).toHaveLength(3);
});
it('a scoped admin gets controls only on mail the server says it can act on', async () => {
mockPerms = { isSuperAdmin: false, permissions: ['events.view', 'email.send'] };
await renderPage();
// Only the owned event mail. The CRM row (no event) and the foreign-event
// row both 404, so neither offers anything.
expect(screen.getAllByText('Preview')).toHaveLength(1);
expect(screen.getAllByText('Resend')).toHaveLength(1);
});
it('an admin without email.send sees preview but no write actions (would 403)', async () => {
mockPerms = { isSuperAdmin: false, permissions: ['events.view'] };
await renderPage();
expect(screen.getAllByText('Preview')).toHaveLength(1);
expect(screen.queryByText('Resend')).toBeNull();
expect(screen.queryByText('Cancel')).toBeNull();
expect(screen.queryByText('Retry')).toBeNull();
expect(screen.queryByText('Send now')).toBeNull();
});
it('offers nothing when the server omits canAct (older backend)', async () => {
mockPerms = { isSuperAdmin: false, permissions: ['events.view', 'email.send'] };
currentOverview = {
...OVERVIEW,
emails: OVERVIEW.emails.map(({ canAct: _drop, ...e }) => e as typeof OVERVIEW.emails[number]),
};
await renderPage();
// Fail to hiding a live control rather than offering a dead one.
expect(screen.queryByText('Preview')).toBeNull();
expect(screen.queryByText('Resend')).toBeNull();
});
});
@@ -49,6 +49,10 @@ export interface ProjectEmail {
eventId: number | null;
/** true = exact HTML stored at send time; false = preview re-rendered. */
stored: boolean;
/** Authoritative: would the queued-mail routes accept an action on this row?
* False for CRM document mail (no event to own) and for event mail the
* caller does not own. Absent on older backends treated as false. */
canAct?: boolean;
}
export interface ProjectInvoice {