13dec6b4b8
Seven parallel agents and an adversarial verification pass. The three things worth knowing before reading the diff: RBAC WAS ALREADY BUILT. docs/build-plan.md marks F2 and F3 outstanding and is stale — packages/core/src/permissions.ts and lib/mutation.ts shipped long ago. So this does not rebuild them; it closes the gaps an audit found. The big one is that reads were entirely ungoverned: every GET was "any authenticated member", so a junior demand rep and a research contractor could both pull per-block supplier cost and break-even prices from /api/capacity/margin, and every contract's negotiated terms. For a company whose margin is the business, that was the hole that mattered. Adds book:read / economics:read / team:read, a readGuard middleware, and a `viewer` role below member. THE BUTTON AND THE 403 DISAGREED — the exact thing F3 said must never happen. Contracts.tsx never called can() at all, so its save button was always enabled against a server requiring contract:sign; Capacity.tsx gated commitment creation on deal:write/demand while the server wanted commitment:write/supply. POST /api/activities was the one write bypassing executeMutation: no capability check, and any member could mutate accounts.lastActivityAt as a side effect. It is now a proper mutation() behind activity:write. The shell becomes three panes — a collapsible shadcn sidebar with an account switcher on the Piggy accent, a header with real search, and Piggy docked to the right, page-aware and persistent across navigation. The phone keeps its bottom tab bar, which is the thing this product already beat trycompai/crm on, and gains the sidebar as a sheet. Calendar is a projection over thirteen dated sources rather than a new table, because a table would duplicate dates that already live on contracts, deals and commitments and would drift — and one ledger answering the question is the whole argument. It surfaces export_authorizations and compliance_artifacts, which had indexed expires_at columns, schema comments saying they must be alerted on, and no read endpoint or UI anywhere. Learn carries two tracks. Concepts are members-only; the platform track can be opened with a share code by someone with no account. The code mints a scoped learn-only token and never a Principal — every route here resolves a principal and then checks capabilities, so a principal-minting code would be one missing check away from leaking the book. "Only platform-track rows may be code-visible" is a database CHECK constraint as well as a write-path rule, and a test asserts a valid learn token still gets 401 on /api/dashboard, /api/accounts and /api/contracts — the same invariant scripts/deploy.sh refuses to ship without. CD becomes tag-to-ship. CI publishes an image to the Gitea registry on a release-* tag and cloud-2 pulls it, so no credential on the shared runner can execute anything on production — by construction rather than by policy. Both halves of deploy.sh's original rule survive: nothing on the runner reaches the host, and a human still decides when it ships. deploy.sh gains a rollback and a public-origin check, and PIG_IMAGE now reaches compose through `sudo env`, without which sudo's env_reset silently resolved every release to pig:local. Tests 141 -> 261. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
198 lines
8.8 KiB
TypeScript
198 lines
8.8 KiB
TypeScript
/**
|
|
* That reads are governed, and that the guard actually runs.
|
|
*
|
|
* Two separate risks. The policy could be wrong — a research contractor let
|
|
* near supplier cost — and that is what the first suite checks. Or the policy
|
|
* could be right and never execute, because Hono runs matched handlers in
|
|
* registration order and a guard mounted after its handler is inert. That
|
|
* second failure produces no error, no warning and a 200, which is exactly the
|
|
* shape of the bug being fixed, so it is checked separately and explicitly.
|
|
*/
|
|
import { strict as assert } from 'node:assert';
|
|
import { readdirSync, readFileSync } from 'node:fs';
|
|
import { join } from 'node:path';
|
|
import { describe, it } from 'node:test';
|
|
import type { Team, TeamRole } from '@pig/core';
|
|
import { Hono } from 'hono';
|
|
import { AuthError, type Principal } from '../src/lib/auth';
|
|
import { apiError, type ApiEnv } from '../src/lib/mutation';
|
|
import { createReadGuardRoutes, READ_RULES } from '../src/routes/read-guards';
|
|
import { principal as makePrincipal } from './helpers/principal';
|
|
|
|
/** The app's own error mapping, reproduced so a 403 here means a 403 there. */
|
|
function guardedApp(principal: Principal, mountGuardsFirst = true) {
|
|
const app = new Hono<ApiEnv>();
|
|
app.use('*', async (context, next) => {
|
|
context.set('principal', principal);
|
|
await next();
|
|
});
|
|
const handlers = new Hono<ApiEnv>();
|
|
for (const rule of READ_RULES) handlers.on(rule.method, rule.path, (c) => c.json({ ok: true }));
|
|
|
|
if (mountGuardsFirst) {
|
|
app.route('/', createReadGuardRoutes());
|
|
app.route('/', handlers);
|
|
} else {
|
|
app.route('/', handlers);
|
|
app.route('/', createReadGuardRoutes());
|
|
}
|
|
|
|
app.onError((error, c) =>
|
|
error instanceof AuthError
|
|
? c.json(apiError(error.code, error.message), error.status)
|
|
: c.json({ error: 'Internal error' }, 500),
|
|
);
|
|
return app;
|
|
}
|
|
|
|
function on(team: Team, role: TeamRole): Principal {
|
|
return makePrincipal({ teams: [{ team, role }] });
|
|
}
|
|
|
|
async function statusFor(principal: Principal, rule: (typeof READ_RULES)[number]) {
|
|
const path = rule.path.replace(':id', '00000000-0000-4000-8000-000000000001');
|
|
const response = await guardedApp(principal).request(path, {
|
|
method: rule.method,
|
|
...(rule.method === 'POST'
|
|
? { headers: { 'content-type': 'application/json' }, body: '{}' }
|
|
: {}),
|
|
});
|
|
return response.status;
|
|
}
|
|
|
|
describe('read policy', () => {
|
|
it('denies every governed read to someone on no team', async () => {
|
|
const stranger = makePrincipal({ teams: [] });
|
|
for (const rule of READ_RULES) {
|
|
assert.equal(await statusFor(stranger, rule), 403, `${rule.method} ${rule.path}`);
|
|
}
|
|
});
|
|
|
|
it('admits every governed read to a platform admin', async () => {
|
|
const admin = makePrincipal({ isPlatformAdmin: true, teams: [] });
|
|
for (const rule of READ_RULES) {
|
|
assert.equal(await statusFor(admin, rule), 200, `${rule.method} ${rule.path}`);
|
|
}
|
|
});
|
|
|
|
/**
|
|
* The case the audit named: a research contractor and a demand rep seeing
|
|
* supplier cost economics identically. They must now differ, and only on the
|
|
* economics rules — research still reads the book.
|
|
*/
|
|
it('splits research off the economics rules and nothing else', async () => {
|
|
const researcher = on('research', 'lead');
|
|
for (const rule of READ_RULES) {
|
|
const expected = rule.capability === 'economics:read' ? 403 : 200;
|
|
assert.equal(await statusFor(researcher, rule), expected, `${rule.method} ${rule.path}`);
|
|
}
|
|
});
|
|
|
|
it('gives a viewer the book and the roster but not the cost side', async () => {
|
|
const viewer = on('demand', 'viewer');
|
|
for (const rule of READ_RULES) {
|
|
const expected = rule.capability === 'economics:read' ? 403 : 200;
|
|
assert.equal(await statusFor(viewer, rule), expected, `${rule.method} ${rule.path}`);
|
|
}
|
|
});
|
|
|
|
it('admits a commercial member to everything, cost included', async () => {
|
|
const seller = on('demand', 'member');
|
|
for (const rule of READ_RULES) {
|
|
assert.equal(await statusFor(seller, rule), 200, `${rule.method} ${rule.path}`);
|
|
}
|
|
});
|
|
|
|
it('refuses a write-only credential even where the person qualifies', async () => {
|
|
const writeOnly = makePrincipal({ via: 'api_key', scopes: ['write'] });
|
|
const response = await guardedApp(writeOnly).request('/api/capacity/margin');
|
|
|
|
assert.equal(response.status, 403);
|
|
assert.equal(((await response.json()) as { code: string }).code, 'insufficient_scope');
|
|
});
|
|
});
|
|
|
|
describe('the guard has to be mounted before the handler', () => {
|
|
it('runs when registered first', async () => {
|
|
const response = await guardedApp(on('research', 'lead'), true).request('/api/capacity/margin');
|
|
assert.equal(response.status, 403);
|
|
});
|
|
|
|
/**
|
|
* Not a test of desired behaviour — a test of the trap. If this ever starts
|
|
* returning 403, Hono's dispatch order changed and the warning comment in
|
|
* read-guards.ts can be deleted. Until then, the mount position in
|
|
* `createApp` is load-bearing and this records why.
|
|
*/
|
|
it('is silently inert when registered after', async () => {
|
|
const response = await guardedApp(on('research', 'lead'), false).request('/api/capacity/margin');
|
|
assert.equal(response.status, 200);
|
|
});
|
|
});
|
|
|
|
/**
|
|
* Nothing stops a future GET being added without a row in READ_RULES, so this
|
|
* reads the routing source and insists that every `/api` GET is either
|
|
* governed or listed below with a reason. It is a coarse regex over source
|
|
* text and that is deliberate: a cleverer check would need the app running,
|
|
* and a check that is hard to run is a check that gets deleted.
|
|
*/
|
|
describe('no read escapes the table', () => {
|
|
/** Reads whose own handler authorises them, or which must stay open. */
|
|
const DELIBERATELY_UNGOVERNED: Readonly<Record<string, string>> = {
|
|
'/api/health': 'Liveness, for load balancers. Unauthenticated by design.',
|
|
'/api/config': 'Public front-end configuration; contains no secret.',
|
|
'/api/me': 'Your own identity. Gating it would hide the reason you are gated.',
|
|
'/api/me/profile': 'Your own profile row.',
|
|
'/api/api-keys': 'Guarded by requireApiKeyManagement, which also bars API keys.',
|
|
'/api/admin/settings': 'settings:admin, enforced in admin-settings.ts.',
|
|
'/api/admin/invites': 'settings:admin, enforced in admin-settings.ts.',
|
|
'/api/admin/members': 'settings:admin, enforced in admin-settings.ts.',
|
|
'/api/admin/integrations': 'settings:admin, enforced in integration-settings.ts.',
|
|
'/api/piggy/status': 'Whether the assistant is switched on; carries no book data.',
|
|
'/api/imports/config': 'data:import, enforced by the router middleware.',
|
|
'/api/imports/google/status': 'integration:connect, enforced by the router middleware.',
|
|
'/api/imports/google/files': 'data:import, enforced by the router middleware.',
|
|
'/api/imports/google/spreadsheets/:id/sheets': 'data:import, enforced by the router middleware.',
|
|
'/api/imports/notion/status': 'integration:connect, enforced by the router middleware.',
|
|
'/api/imports/notion/connections/:id/data-sources': 'integration:connect, ditto.',
|
|
'/api/integrations/hubspot/oauth/callback': 'OAuth redirect; verifies its own state.',
|
|
'/api/integrations/hubspot/connections': 'settings:admin, enforced in hubspot.ts.',
|
|
'/api/integrations/slack/channel-links': 'Channel wiring, not book data.',
|
|
'/api/integrations/buzz/channel-links': 'Channel wiring, not book data.',
|
|
'/api/calendar': 'Owned by the calendar track; gated in calendar.ts.',
|
|
'/api/calendar/entries': 'Owned by the calendar track; gated in calendar.ts.',
|
|
// Landed while this table was being written and carries its own access
|
|
// code rather than a capability. Listed so the check stays green, not
|
|
// because the arrangement has been reviewed — the learn track owns it.
|
|
'/api/learn': 'Owned by the learn track; gated by its own access code.',
|
|
'/api/learn/access-code': 'Owned by the learn track; gated by its own access code.',
|
|
};
|
|
|
|
it('has a row, or a stated reason, for every GET', () => {
|
|
const root = join(import.meta.dirname, '..', 'src');
|
|
const files = [
|
|
join(root, 'app.ts'),
|
|
...readdirSync(join(root, 'routes'))
|
|
.filter((name) => name.endsWith('.ts'))
|
|
.map((name) => join(root, 'routes', name)),
|
|
];
|
|
|
|
const governed = new Set(READ_RULES.filter((rule) => rule.method === 'GET').map((r) => r.path));
|
|
const found = new Set<string>();
|
|
for (const file of files) {
|
|
const source = readFileSync(file, 'utf8');
|
|
for (const match of source.matchAll(/\.get\(\s*'(\/api\/[^']*)'/g)) found.add(match[1]!);
|
|
}
|
|
|
|
const ungoverned = [...found].filter(
|
|
(path) => !governed.has(path) && !(path in DELIBERATELY_UNGOVERNED),
|
|
);
|
|
assert.deepEqual(
|
|
ungoverned,
|
|
[],
|
|
`these reads are ungoverned — add a READ_RULES row or a stated reason:\n${ungoverned.join('\n')}`,
|
|
);
|
|
});
|
|
});
|