diff --git a/cloud/.env.example b/cloud/.env.example index 555caf93..7333e26e 100644 --- a/cloud/.env.example +++ b/cloud/.env.example @@ -126,6 +126,10 @@ GHOST_STEP_TIMEOUT_MS="30000" # browser cannot wedge the pod forever. # GHOST_RUN_TIMEOUT_MS="1800000" +# Cap for `GET /api/audit/verify?mode=full`. Over this many events the route +# returns 413 and asks the caller to use `mode=head` instead. Default 50000. +# GHOST_AUDIT_VERIFY_MAX_EVENTS="50000" + # How many runs one worker process executes at once. Each holds a browser, so # this is memory-bound. Default 2. WORKER_CONCURRENCY="2" diff --git a/cloud/apps/web/src/app/(app)/audit/page.tsx b/cloud/apps/web/src/app/(app)/audit/page.tsx index b276fa6e..259ef05b 100644 --- a/cloud/apps/web/src/app/(app)/audit/page.tsx +++ b/cloud/apps/web/src/app/(app)/audit/page.tsx @@ -1,47 +1,39 @@ import { auth } from "@/auth"; import { prisma } from "@/lib/db"; import { Card, CardBody } from "@/components/ui/card"; -import { verifyAuditChain } from "@ghost/core/audit"; -import { auditPayloadFromRow } from "@ghost/core/audit-log"; export const dynamic = "force-dynamic"; /** - * The organization's tamper-evident ledger, and whether it still verifies. + * The organization's tamper-evident ledger, and whether its expected head + * still matches. * - * The hash chain existed from Phase 1 but nothing ever checked it — a log - * nobody can verify is just a log. This page answers the question a customer's - * auditor actually asks: has anything in this history been altered since it was - * written? + * Full mid-chain re-hashing of unbounded history is an API concern + * (`GET /api/audit/verify?mode=full`) and is capped. This page used to load + * the entire chain on every render (P1-1) — the integrity badge now uses the + * cheap expected-head check, which still catches suffix deletion. */ export default async function AuditPage() { const session = await auth(); const orgId = session?.user.orgId; - // Verification must walk the chain from its real first event. Verifying a - // truncated window would report every healthy chain past the page size as - // "broken at #0", because the oldest row in that window has a non-null - // `prevHash` while the walk expects the first link's predecessor to be null — - // a false alarm in the one feature whose entire job is trustworthy - // verification. - const [events, allForChain] = orgId + const [events, orgRow, count, tail] = orgId ? await Promise.all([ prisma.auditEvent.findMany({ where: { orgId }, orderBy: { seq: "desc" }, take: 200 }), - prisma.auditEvent.findMany({ + prisma.organization.findUniqueOrThrow({ + where: { id: orgId }, + select: { auditChainHead: true }, + }), + prisma.auditEvent.count({ where: { orgId } }), + prisma.auditEvent.findFirst({ where: { orgId }, - orderBy: { seq: "asc" }, - select: { action: true, entityType: true, entityId: true, metadata: true, prevHash: true, hash: true }, + orderBy: { seq: "desc" }, + select: { hash: true }, }), ]) - : [[], []]; + : [[], null, 0, null]; - const chain = verifyAuditChain( - allForChain.map((e) => ({ - prevHash: e.prevHash, - hash: e.hash, - payload: auditPayloadFromRow(e), - })), - ); + const headMatches = (tail?.hash ?? null) === (orgRow?.auditChainHead ?? null); return (
@@ -57,23 +49,19 @@ export default async function AuditPage() {
Chain integrity

- {allForChain.length === 0 + {count === 0 ? "No events recorded yet." - : `Full chain verified — all ${allForChain.length} events${ - allForChain.length > events.length ? `, showing the most recent ${events.length}` : "" - }.`} + : `Expected head check across ${count} events${ + count > events.length ? `, showing the most recent ${events.length}` : "" + }. Full mid-chain verify: GET /api/audit/verify?mode=full.`}

- {allForChain.length === 0 - ? "—" - : chain.intact - ? "Intact" - : `Broken at #${(chain.firstBreakIndex ?? 0) + 1}`} + {count === 0 ? "—" : headMatches ? "Intact" : "Head mismatch"} diff --git a/cloud/apps/web/src/app/api/audit/verify/route.ts b/cloud/apps/web/src/app/api/audit/verify/route.ts index ddfb8de8..66c3032b 100644 --- a/cloud/apps/web/src/app/api/audit/verify/route.ts +++ b/cloud/apps/web/src/app/api/audit/verify/route.ts @@ -8,36 +8,92 @@ import { auditPayloadFromRow, runEventPayloadFromRow } from "@ghost/core/audit-l /** * Verify the organization's audit chain, and optionally one run's journal. * - * `verifyAuditChain` has existed and been unit-tested since Phase 1 but was - * never reachable from anywhere — an audit log nobody can check is a log, not - * a proof. This is the endpoint that lets a customer (or their auditor) ask the - * question directly. + * Two modes (query `mode=`): * - * A broken chain is reported, not hidden. If an org's history was forked by the - * old non-transactional appender, this will say so, which is the right outcome: - * an audit log that quietly conceals a break is worse than one that admits it. + * - `head` (default for the audit page): cheap — count + last hash vs + * `Organization.auditChainHead`. Detects suffix deletion / head drift. + * Does **not** re-hash every event; mid-chain payload edits need `full`. + * - `full`: walks every event through `verifyAuditChain`. Caped by + * `GHOST_AUDIT_VERIFY_MAX_EVENTS` (default 50_000) so an unbounded + * tenant history cannot take the web tier down (P1-1). Over the cap → 413. + * + * A broken chain is reported, not hidden. */ + +const DEFAULT_MAX_EVENTS = 50_000; + +function maxEvents(): number { + const fromEnv = Number(process.env.GHOST_AUDIT_VERIFY_MAX_EVENTS); + return Number.isFinite(fromEnv) && fromEnv > 0 ? fromEnv : DEFAULT_MAX_EVENTS; +} + export async function GET(req: Request) { const session = await auth(); if (!session?.user?.orgId) { return NextResponse.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; - const runId = new URL(req.url).searchParams.get("runId"); - - // Verification is inherently whole-chain — a hash chain cannot be checked in - // pages — so this is the most expensive read in the product and it grows - // with the tenant's entire history. Rate limited per organization because a - // loop over it is a one-request-each denial of service against the web tier, - // and because it is a deliberate audit action rather than something a UI - // polls. Six a minute is generous for a human and useless as an attack. - // - // This bounds the blast radius; it does not fix the underlying cost. The - // unbounded findMany below is P1-1 in docs/ARCHITECTURE_DECISIONS.md and - // needs incremental verification with a checkpoint, not a limiter. + const url = new URL(req.url); + const runId = url.searchParams.get("runId"); + const mode = url.searchParams.get("mode") === "full" ? "full" : "head"; + + // Verification of a large chain is the most expensive read in the product. + // Rate limited per organization so a loop over it is not a one-request-each + // denial of service. Six a minute is generous for a human and useless as an + // attack. const limited = await rateLimit(`audit-verify:${orgId}`, { limit: 6, windowSeconds: 60 }); if (!limited.ok) return tooManyRequests(limited); + const orgRow = await prisma.organization.findUniqueOrThrow({ + where: { id: orgId }, + select: { auditChainHead: true }, + }); + + const count = await prisma.auditEvent.count({ where: { orgId } }); + const tail = await prisma.auditEvent.findFirst({ + where: { orgId }, + orderBy: { seq: "desc" }, + select: { hash: true, seq: true }, + }); + const orgHead = tail?.hash ?? null; + const orgExpectedHeadMatches = orgHead === orgRow.auditChainHead; + + if (mode === "head") { + const body: Record = { + mode: "head", + org: { + // Head mode cannot claim mid-chain integrity — only that the expected + // tail matches what is stored on Organization. + intact: orgExpectedHeadMatches, + count, + headHash: orgHead, + expectedHeadHash: orgRow.auditChainHead, + expectedHeadMatches: orgExpectedHeadMatches, + fullVerifyAvailable: count <= maxEvents(), + }, + }; + if (runId) { + const runHead = await verifyRunHead(orgId, runId); + if (!runHead) return NextResponse.json({ error: "not found" }, { status: 404 }); + body.run = runHead; + } + return NextResponse.json(body); + } + + // Full verify — capped. + const cap = maxEvents(); + if (count > cap) { + return NextResponse.json( + { + error: "audit chain too large for a single full verify", + count, + maxEvents: cap, + hint: "use mode=head for the expected-tail check, or raise GHOST_AUDIT_VERIFY_MAX_EVENTS", + }, + { status: 413 }, + ); + } + const events = await prisma.auditEvent.findMany({ where: { orgId }, orderBy: { seq: "asc" }, @@ -46,20 +102,8 @@ export async function GET(req: Request) { events.map((e) => ({ prevHash: e.prevHash, hash: e.hash, payload: auditPayloadFromRow(e) })), ); - // Internal hash links alone cannot detect deletion of a valid suffix — the - // surviving prefix still verifies as intact, just shorter. auditChainHead is - // the expected tail persisted on Organization outside AuditEvent itself (see - // appendAuditEvent), so a suffix deleted without also rewriting that column - // is caught here instead of silently accepted as "the chain that happens to - // exist now". - const orgRow = await prisma.organization.findUniqueOrThrow({ - where: { id: orgId }, - select: { auditChainHead: true }, - }); - const orgHead = events.at(-1)?.hash ?? null; - const orgExpectedHeadMatches = orgHead === orgRow.auditChainHead; - const body: Record = { + mode: "full", org: { intact: chain.intact && orgExpectedHeadMatches, firstBreakIndex: chain.firstBreakIndex, @@ -81,7 +125,17 @@ export async function GET(req: Request) { where: { runId }, orderBy: { seq: "asc" }, }); - const chain = verifyAuditChain( + if (runEvents.length > cap) { + return NextResponse.json( + { + error: "run journal too large for a single full verify", + count: runEvents.length, + maxEvents: cap, + }, + { status: 413 }, + ); + } + const runChain = verifyAuditChain( runEvents.map((e) => ({ prevHash: e.prevHash, hash: e.hash, @@ -89,8 +143,6 @@ export async function GET(req: Request) { })), ); - // The org chain records each finished run's journal head. If the two - // disagree, the journal was altered after the run sealed it. const head = runEvents.at(-1)?.hash ?? null; const expectedHeadMatches = head === owned.journalHead; const anchors = events @@ -99,8 +151,8 @@ export async function GET(req: Request) { .filter((h): h is string => typeof h === "string"); body.run = { - intact: chain.intact && expectedHeadMatches, - firstBreakIndex: chain.firstBreakIndex, + intact: runChain.intact && expectedHeadMatches, + firstBreakIndex: runChain.firstBreakIndex, count: runEvents.length, headHash: head, expectedHeadHash: owned.journalHead, @@ -112,3 +164,26 @@ export async function GET(req: Request) { return NextResponse.json(body); } + +async function verifyRunHead(orgId: string, runId: string) { + const owned = await prisma.run.findFirst({ + where: { id: runId, orgId }, + select: { id: true, journalHead: true }, + }); + if (!owned) return null; + const count = await prisma.runEvent.count({ where: { runId } }); + const tail = await prisma.runEvent.findFirst({ + where: { runId }, + orderBy: { seq: "desc" }, + select: { hash: true }, + }); + const head = tail?.hash ?? null; + const expectedHeadMatches = head === owned.journalHead; + return { + intact: expectedHeadMatches, + count, + headHash: head, + expectedHeadHash: owned.journalHead, + expectedHeadMatches, + }; +} diff --git a/cloud/apps/web/src/app/api/runs/[id]/approvals/[stepIndex]/route.ts b/cloud/apps/web/src/app/api/runs/[id]/approvals/[stepIndex]/route.ts index 00388c59..53aeeb42 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/approvals/[stepIndex]/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/approvals/[stepIndex]/route.ts @@ -1,8 +1,10 @@ import { NextResponse } from "next/server"; import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { enqueueCompensateRun, enqueueRunWorkflow } from "@/lib/queue"; import { appendAuditEvent, appendRunEvent } from "@ghost/core/audit-log"; +import { canApproveRun } from "@ghost/core/roles"; import { RUN_EVENT_TYPES } from "@ghost/core/run-events"; /** @@ -21,11 +23,11 @@ export async function POST( { params }: { params: Promise<{ id: string; stepIndex: string }> }, ) { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return NextResponse.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; - const userId = session.user.id ?? null; + const userId = session.user.id; const { id, stepIndex } = await params; const index = Number(stepIndex); if (!Number.isInteger(index) || index < 0) { @@ -46,6 +48,14 @@ export async function POST( if (!run) return NextResponse.json({ error: "not found" }, { status: 404 }); const approve = body.decision === "approve"; + // Approving authorizes a sensitive mutation — OWNER/ADMIN only. Rejecting + // stops the action and stays open to every member (see canApproveRun). + if (approve) { + const actor = await loadActor(orgId, userId); + if (!actor || !canApproveRun(actor.role)) { + return NextResponse.json({ error: "forbidden" }, { status: 403 }); + } + } const now = new Date(); // Which direction of the run is waiting. A gate opened while reversing the diff --git a/cloud/apps/web/src/app/api/runs/route.ts b/cloud/apps/web/src/app/api/runs/route.ts index c11007a9..72fbd0ec 100644 --- a/cloud/apps/web/src/app/api/runs/route.ts +++ b/cloud/apps/web/src/app/api/runs/route.ts @@ -1,15 +1,25 @@ import { NextResponse } from "next/server"; +import { canStartRun } from "@ghost/core/roles"; import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { startRun } from "@/lib/start-run"; /** Trigger a run of a workflow's latest version. */ export async function POST(req: Request) { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return NextResponse.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; + const userId = session.user.id; + // Re-read role from the DB; JWT says nothing about a demotion since sign-in. + // Today every Role may start a run; the check is here so a future VIEWER + // cannot slip through when canStartRun tightens. + const actor = await loadActor(orgId, userId); + if (!actor || !canStartRun(actor.role)) { + return NextResponse.json({ error: "forbidden" }, { status: 403 }); + } const body = (await req.json().catch(() => ({}))) as { workflowId?: string }; if (!body.workflowId) { @@ -28,7 +38,7 @@ export async function POST(req: Request) { const started = await startRun({ orgId, workflowVersionId: version.id, - triggeredById: session.user.id, + triggeredById: userId, }); if (!started.ok) { // 503, not 500: the request was fine and retrying is the right move once diff --git a/cloud/apps/web/src/app/api/runs/separation-of-duties.test.ts b/cloud/apps/web/src/app/api/runs/separation-of-duties.test.ts index 4f892117..0587e5aa 100644 --- a/cloud/apps/web/src/app/api/runs/separation-of-duties.test.ts +++ b/cloud/apps/web/src/app/api/runs/separation-of-duties.test.ts @@ -3,12 +3,12 @@ import { prisma } from "@ghost/core/db"; import type { WorkflowSteps } from "@ghost/core/schema/step"; /** - * Separation of duties on the approval gate. + * Separation of duties + admin-only approval on the approval gate. * * Ghost's central claim is human approval on sensitive actions. Without this, * "human approval" means "somebody with a login clicked yes" — including the - * same person who started the run. These tests pin the rule and, just as - * importantly, pin what it deliberately does *not* restrict. + * same person who started the run, or any MEMBER. Approving is now + * OWNER/ADMIN only; rejecting stays open to every member. * * Requires DATABASE_URL; skips cleanly without one. */ @@ -32,30 +32,35 @@ const steps: WorkflowSteps = [ describe.skipIf(!hasDb)("approval separation of duties (Postgres)", () => { let orgId: string; - let triggerer: string; - let colleague: string; + let owner: string; + let admin: string; + let member: string; const slug = `sod-${Date.now()}`; beforeAll(async () => { const org = await prisma.organization.create({ data: { name: "SoD", slug } }); orgId = org.id; const a = await prisma.user.create({ - data: { email: `${slug}-a@example.com`, memberships: { create: { orgId, role: "OWNER" } } }, + data: { email: `${slug}-owner@example.com`, memberships: { create: { orgId, role: "OWNER" } } }, }); const b = await prisma.user.create({ - data: { email: `${slug}-b@example.com`, memberships: { create: { orgId, role: "MEMBER" } } }, + data: { email: `${slug}-admin@example.com`, memberships: { create: { orgId, role: "ADMIN" } } }, }); - triggerer = a.id; - colleague = b.id; + const c = await prisma.user.create({ + data: { email: `${slug}-member@example.com`, memberships: { create: { orgId, role: "MEMBER" } } }, + }); + owner = a.id; + admin = b.id; + member = c.id; }); afterAll(async () => { await prisma.organization.delete({ where: { id: orgId } }).catch(() => undefined); - await prisma.user.deleteMany({ where: { id: { in: [triggerer, colleague] } } }); + await prisma.user.deleteMany({ where: { id: { in: [owner, admin, member] } } }); }); beforeEach(() => { - session.current = { user: { id: triggerer, orgId } }; + session.current = { user: { id: owner, orgId } }; }); /** A workflow + a run halted at a gate, with the policy set as given. */ @@ -96,22 +101,17 @@ describe.skipIf(!hasDb)("approval separation of duties (Postgres)", () => { } it("refuses the triggerer's own approval, and records the refusal", async () => { - const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: triggerer }); + const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: owner }); const res = await decide(runId, "approve"); expect(res.status).toBe(403); - // The gate must still be closed — a refused approval that resolved anything - // would be worse than no control at all. const approval = await prisma.approval.findFirstOrThrow({ where: { runId } }); expect(approval.status).toBe("PENDING"); const run = await prisma.run.findUniqueOrThrow({ where: { id: runId } }); expect(run.status).toBe("AWAITING_APPROVAL"); - // Nothing entered the run journal: no decision was made. expect(await prisma.runEvent.count({ where: { runId } })).toBe(0); - // …but the attempt is on the record. A silent 403 would leave no evidence - // the control did its job. const audit = await prisma.auditEvent.findFirst({ where: { orgId, action: "approval.self_approval_refused" }, orderBy: { seq: "desc" }, @@ -119,23 +119,43 @@ describe.skipIf(!hasDb)("approval separation of duties (Postgres)", () => { expect(audit?.metadata).toMatchObject({ runId, stepIndex: 1 }); }); - it("lets a different member approve the same gate", async () => { - const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: triggerer }); + it("lets a different ADMIN approve the same gate", async () => { + const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: owner }); - session.current = { user: { id: colleague, orgId } }; + session.current = { user: { id: admin, orgId } }; const res = await decide(runId, "approve"); expect(res.status).toBe(200); const approval = await prisma.approval.findFirstOrThrow({ where: { runId } }); expect(approval.status).toBe("APPROVED"); - expect(approval.resolvedById).toBe(colleague); + expect(approval.resolvedById).toBe(admin); + }); + + it("refuses a MEMBER who tries to approve", async () => { + const runId = await gatedRun({ requireSeparateApprover: false, triggeredById: owner }); + + session.current = { user: { id: member, orgId } }; + const res = await decide(runId, "approve"); + + expect(res.status).toBe(403); + const approval = await prisma.approval.findFirstOrThrow({ where: { runId } }); + expect(approval.status).toBe("PENDING"); }); it("still lets the triggerer REJECT their own run", async () => { - // Rejecting stops the action rather than authorizing it. Blocking it would - // leave whoever started a runaway run unable to halt it. - const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: triggerer }); + const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: owner }); + + const res = await decide(runId, "reject"); + + expect(res.status).toBe(200); + const approval = await prisma.approval.findFirstOrThrow({ where: { runId } }); + expect(approval.status).toBe("REJECTED"); + }); + + it("still lets a MEMBER reject (halting is not authorizing)", async () => { + const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: owner }); + session.current = { user: { id: member, orgId } }; const res = await decide(runId, "reject"); expect(res.status).toBe(200); @@ -143,9 +163,8 @@ describe.skipIf(!hasDb)("approval separation of duties (Postgres)", () => { expect(approval.status).toBe("REJECTED"); }); - it("does not restrict anything when the policy is off", async () => { - // The default, and what every existing workflow has: behaviour unchanged. - const runId = await gatedRun({ requireSeparateApprover: false, triggeredById: triggerer }); + it("does not restrict self-approval when the SoD policy is off", async () => { + const runId = await gatedRun({ requireSeparateApprover: false, triggeredById: owner }); const res = await decide(runId, "approve"); @@ -155,8 +174,6 @@ describe.skipIf(!hasDb)("approval separation of duties (Postgres)", () => { }); it("does not lock out an agent-started run whose triggerer is null", async () => { - // `triggeredById` is null for agent-started runs. Comparing null to a null - // user id would match and make the run unapprovable by anyone. const runId = await gatedRun({ requireSeparateApprover: true, triggeredById: null }); const res = await decide(runId, "approve"); diff --git a/cloud/apps/web/src/app/api/workflows/[id]/route.ts b/cloud/apps/web/src/app/api/workflows/[id]/route.ts index 40e18c5f..ec1d7043 100644 --- a/cloud/apps/web/src/app/api/workflows/[id]/route.ts +++ b/cloud/apps/web/src/app/api/workflows/[id]/route.ts @@ -1,22 +1,29 @@ import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { appendAuditEvent } from "@ghost/core/audit-log"; import { parseMaxActiveRuns } from "@ghost/core/concurrency"; +import { canPublishWorkflow } from "@ghost/core/roles"; import { enqueueCompensateRun, enqueueRunWorkflow } from "@/lib/queue"; /** - * Update a workflow's settings. Currently only its concurrency cap. + * Update a workflow's settings. Currently its concurrency cap and four-eyes + * toggle. * * The cap is a governance control — it decides how much load Ghost is allowed * to put on a customer's system — so changing it is audited like any other - * configuration change, with the old and new values recorded. + * configuration change, with the old and new values recorded. OWNER/ADMIN only. */ export async function PATCH(req: Request, context: { params: Promise<{ id: string }> }) { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return Response.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; + const actor = await loadActor(orgId, session.user.id); + if (!actor || !canPublishWorkflow(actor.role)) { + return Response.json({ error: "forbidden" }, { status: 403 }); + } const { id } = await context.params; const body = (await req.json().catch(() => ({}))) as { diff --git a/cloud/apps/web/src/app/api/workflows/[id]/versions/route.ts b/cloud/apps/web/src/app/api/workflows/[id]/versions/route.ts index 5ce2b426..d8455c3e 100644 --- a/cloud/apps/web/src/app/api/workflows/[id]/versions/route.ts +++ b/cloud/apps/web/src/app/api/workflows/[id]/versions/route.ts @@ -1,7 +1,9 @@ import { Prisma } from "@ghost/core/db"; import { appendAuditEvent } from "@ghost/core/audit-log"; +import { canPublishWorkflow } from "@ghost/core/roles"; import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { formatIssues, publishVersionInput } from "@/lib/workflow-input"; import type { WorkflowSteps } from "@ghost/core/schema/step"; @@ -13,14 +15,21 @@ import type { WorkflowSteps } from "@ghost/core/schema/step"; * definition it started with while an edit lands — the alternative is changing * the steps out from under a run that is halfway through a customer's system, * possibly while it waits at an approval gate a human is still reading. + * + * OWNER/ADMIN only. A MEMBER must not rewrite what Ghost will do to a + * customer's system — that is the publish side of P1-5. */ export async function POST(req: Request, context: { params: Promise<{ id: string }> }) { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return Response.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; - const userId = session.user.id ?? null; + const userId = session.user.id; + const actor = await loadActor(orgId, userId); + if (!actor || !canPublishWorkflow(actor.role)) { + return Response.json({ error: "forbidden" }, { status: 403 }); + } const { id } = await context.params; const parsed = publishVersionInput.safeParse(await req.json().catch(() => null)); diff --git a/cloud/apps/web/src/app/api/workflows/demo/route.ts b/cloud/apps/web/src/app/api/workflows/demo/route.ts index 400d79a8..e7f59121 100644 --- a/cloud/apps/web/src/app/api/workflows/demo/route.ts +++ b/cloud/apps/web/src/app/api/workflows/demo/route.ts @@ -1,7 +1,9 @@ import { NextResponse } from "next/server"; import { Prisma } from "@ghost/core/db"; +import { canPublishWorkflow } from "@ghost/core/roles"; import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { demoWorkflowSteps, DEMO_WORKFLOW_NAME, @@ -11,9 +13,13 @@ import { /** Create the seed demo workflow (with its first version) for the current org. */ export async function POST() { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return NextResponse.json({ error: "unauthorized" }, { status: 401 }); } + const actor = await loadActor(session.user.orgId, session.user.id); + if (!actor || !canPublishWorkflow(actor.role)) { + return NextResponse.json({ error: "forbidden" }, { status: 403 }); + } const workflow = await prisma.workflow.create({ data: { diff --git a/cloud/apps/web/src/app/api/workflows/route.ts b/cloud/apps/web/src/app/api/workflows/route.ts index a9e07a30..58274cba 100644 --- a/cloud/apps/web/src/app/api/workflows/route.ts +++ b/cloud/apps/web/src/app/api/workflows/route.ts @@ -1,7 +1,9 @@ import { Prisma } from "@ghost/core/db"; import { appendAuditEvent } from "@ghost/core/audit-log"; +import { canPublishWorkflow } from "@ghost/core/roles"; import { auth } from "@/auth"; import { prisma } from "@/lib/db"; +import { loadActor } from "@/lib/members"; import { createWorkflowInput, formatIssues } from "@/lib/workflow-input"; /** Thrown inside the creation transaction to abort it without committing — @@ -18,11 +20,18 @@ class RecordingNotClaimableError extends Error {} */ export async function POST(req: Request) { const session = await auth(); - if (!session?.user?.orgId) { + if (!session?.user?.orgId || !session.user.id) { return Response.json({ error: "unauthorized" }, { status: 401 }); } const orgId = session.user.orgId; - const userId = session.user.id ?? null; + const userId = session.user.id; + // Creating a workflow publishes its first version — same privilege as + // `POST /versions`. Re-read the role from the DB; the JWT is minted at + // sign-in and says nothing about a demotion since. + const actor = await loadActor(orgId, userId); + if (!actor || !canPublishWorkflow(actor.role)) { + return Response.json({ error: "forbidden" }, { status: 403 }); + } const parsed = createWorkflowInput.safeParse(await req.json().catch(() => null)); if (!parsed.success) { diff --git a/cloud/apps/web/src/app/api/workflows/workflows.test.ts b/cloud/apps/web/src/app/api/workflows/workflows.test.ts index e538882d..8356ce36 100644 --- a/cloud/apps/web/src/app/api/workflows/workflows.test.ts +++ b/cloud/apps/web/src/app/api/workflows/workflows.test.ts @@ -42,6 +42,8 @@ describe.skipIf(!hasDb)("workflow authoring routes (Postgres)", () => { let userA: string; const slug = `authoring-${Date.now()}`; + let memberA: string; + beforeAll(async () => { const a = await prisma.organization.create({ data: { name: "A", slug: `${slug}-a` } }); const b = await prisma.organization.create({ data: { name: "B", slug: `${slug}-b` } }); @@ -50,16 +52,30 @@ describe.skipIf(!hasDb)("workflow authoring routes (Postgres)", () => { const u = await prisma.user.create({ data: { email: `${slug}@example.com`, - memberships: { create: { orgId: orgA, role: "OWNER" } }, + memberships: { + create: [ + { orgId: orgA, role: "OWNER" }, + // Membership in B so the cross-tenant publish attempt is a real + // authz miss (404) rather than "not a member of the session org". + { orgId: orgB, role: "OWNER" }, + ], + }, }, }); userA = u.id; + const m = await prisma.user.create({ + data: { + email: `${slug}-member@example.com`, + memberships: { create: { orgId: orgA, role: "MEMBER" } }, + }, + }); + memberA = m.id; session.current = { user: { id: userA, orgId: orgA } }; }); afterAll(async () => { await prisma.organization.deleteMany({ where: { id: { in: [orgA, orgB] } } }); - await prisma.user.deleteMany({ where: { id: userA } }); + await prisma.user.deleteMany({ where: { id: { in: [userA, memberA] } } }); }); async function post(body: unknown) { @@ -154,4 +170,22 @@ describe.skipIf(!hasDb)("workflow authoring routes (Postgres)", () => { expect(res.status).toBe(401); session.current = { user: { id: userA, orgId: orgA } }; }); + + it("refuses a MEMBER who tries to create or publish", async () => { + session.current = { user: { id: memberA, orgId: orgA } }; + const create = await post({ name: "Member draft", steps }); + expect(create.status).toBe(403); + + // Seed a workflow as OWNER, then switch back to MEMBER for publish. + session.current = { user: { id: userA, orgId: orgA } }; + const seeded = await post({ name: "Seed", steps }); + const { workflowId } = (await seeded.json()) as { workflowId: string }; + + session.current = { user: { id: memberA, orgId: orgA } }; + const pub = await publish(workflowId, { steps }); + expect(pub.status).toBe(403); + expect(await prisma.workflowVersion.count({ where: { workflowId } })).toBe(1); + + session.current = { user: { id: userA, orgId: orgA } }; + }); }); diff --git a/cloud/apps/web/src/components/workflow-editor.tsx b/cloud/apps/web/src/components/workflow-editor.tsx index 1959bf36..44eab0fb 100644 --- a/cloud/apps/web/src/components/workflow-editor.tsx +++ b/cloud/apps/web/src/components/workflow-editor.tsx @@ -460,9 +460,11 @@ function StepFields({ Value update(index, { value: e.target.value } as Partial)} placeholder={step.type === "fill" ? "Ada Lovelace" : "Option label"} + autoComplete={step.type === "fill" && step.sensitive ? "off" : undefined} />
)} diff --git a/cloud/apps/web/src/lib/run-view.ts b/cloud/apps/web/src/lib/run-view.ts index e9e4fc35..590e0d60 100644 --- a/cloud/apps/web/src/lib/run-view.ts +++ b/cloud/apps/web/src/lib/run-view.ts @@ -1,5 +1,7 @@ import { prisma } from "@/lib/db"; +import { canApproveRun } from "@ghost/core/roles"; import { throttleReason } from "@ghost/core/concurrency"; +import { loadActor } from "@/lib/members"; /** * Build the run detail view: status, ordered steps, pending approvals, and @@ -62,6 +64,11 @@ export async function buildRunView(orgId: string, viewerId: string, runId: strin } } + // Role is re-read from the DB (not the JWT) so a demotion takes effect on + // the next poll rather than lasting until the session expires. + const actor = await loadActor(orgId, viewerId); + const roleAllowsApprove = actor !== null && canApproveRun(actor.role); + // "Queued" alone does not say whether Ghost is busy, broken, or deliberately // holding this run back behind its workflow's concurrency cap. // @@ -109,11 +116,12 @@ export async function buildRunView(orgId: string, viewerId: string, runId: strin // Whether *this viewer* may approve. Computed here rather than shipping the // policy and the triggerer's id to the client and asking it to decide: the // route is the authority either way, and this keeps who-started-what out of - // a polling payload. + // a polling payload. OWNER/ADMIN only, plus SoD when the workflow opts in. canApprove: - !run.workflowVersion.workflow.requireSeparateApprover || - run.triggeredById === null || - run.triggeredById !== viewerId, + roleAllowsApprove && + (!run.workflowVersion.workflow.requireSeparateApprover || + run.triggeredById === null || + run.triggeredById !== viewerId), startedAt: run.startedAt, endedAt: run.endedAt, // The step the run is stopped on, so an incident can offer retry/skip. @@ -129,7 +137,10 @@ export async function buildRunView(orgId: string, viewerId: string, runId: strin verification: s.verification, error: s.error, attempt: s.attempt, - output: s.output, + // Extract values stay in the run journal (needed for {{ }} refs) but are + // not shipped to the browser — cleartext PII/extracted secrets must not + // sit in every poll of the timeline (P1-4 mitigation). + output: null, })), approvals: run.approvals.map((a) => ({ stepIndex: a.stepIndex, diff --git a/cloud/apps/worker/src/browser/driver.ts b/cloud/apps/worker/src/browser/driver.ts index d38bf253..5ce751b2 100644 --- a/cloud/apps/worker/src/browser/driver.ts +++ b/cloud/apps/worker/src/browser/driver.ts @@ -1,5 +1,6 @@ import { chromium, type Browser, type BrowserContext, type Page } from "playwright"; import type { WorkflowStep } from "@ghost/core/schema/step"; +import { classifyStep } from "@ghost/core/classifier"; import { assertPublicHttpUrl } from "@ghost/core/net/public-url"; import { discoverChromium } from "./chromium.js"; import { resolveLocator } from "./selector.js"; @@ -228,9 +229,17 @@ export async function verifyStep(page: Page, step: WorkflowStep): Promise { + it("captures ordinary fills", () => { + const step = { + id: "1", + type: "fill", + selector: { role: "textbox", name: "Full name" }, + value: "Ada", + sensitive: false, + } satisfies WorkflowStep; + expect(shouldCaptureScreenshot(step)).toBe(true); + }); + + it("skips when the author marked the fill sensitive", () => { + const step = { + id: "1", + type: "fill", + selector: { role: "textbox", name: "Full name" }, + value: "secret", + sensitive: true, + } satisfies WorkflowStep; + expect(shouldCaptureScreenshot(step)).toBe(false); + }); + + it("skips when the classifier gates a password-shaped field even without the flag", () => { + const step = { + id: "1", + type: "fill", + selector: { role: "textbox", name: "Password" }, + value: "hunter2", + sensitive: false, + } satisfies WorkflowStep; + expect(shouldCaptureScreenshot(step)).toBe(false); + }); + + it("still captures non-fill steps", () => { + expect( + shouldCaptureScreenshot({ + id: "1", + type: "click", + selector: { role: "button", name: "Submit" }, + }), + ).toBe(true); + }); +}); diff --git a/cloud/docs/ARCHITECTURE_DECISIONS.md b/cloud/docs/ARCHITECTURE_DECISIONS.md index bfeedce6..bd33dd53 100644 --- a/cloud/docs/ARCHITECTURE_DECISIONS.md +++ b/cloud/docs/ARCHITECTURE_DECISIONS.md @@ -338,11 +338,11 @@ plus a small number of real defects. Ordered by business impact. | # | Finding | Why it matters | Status | |---|---|---|---| -| P1-1 | `/api/audit/verify` loads the entire org chain with no pagination. | The "prove your audit log is intact" feature is the first thing that breaks at scale, and it takes the web tier with it. | Open — rate-limited; needs checkpointed verify, not just pagination. | -| P1-2 | Screenshots are captured after every step, including `fill` steps marked `sensitive`, and `step-*.png` is on the servable allow-list. OTP and card fields are `type="text"`, so nothing masks them. | Cardholder data at rest in the blob store. | **Partial.** Worker skips screenshot capture for `fill`+`sensitive` (`shouldCaptureScreenshot` in `driver.ts`); editor password-input and secret-reference design still open. | +| P1-1 | `/api/audit/verify` loads the entire org chain with no pagination. | The "prove your audit log is intact" feature is the first thing that breaks at scale, and it takes the web tier with it. | **Partial.** Default `mode=head` (expected-tail check); `mode=full` capped by `GHOST_AUDIT_VERIFY_MAX_EVENTS` (413 over cap). Audit page no longer loads the full chain. Durable checkpointed walk still open. | +| P1-2 | Screenshots are captured after every step, including `fill` steps marked `sensitive`, and `step-*.png` is on the servable allow-list. OTP and card fields are `type="text"`, so nothing masks them. | Cardholder data at rest in the blob store. | **Partial.** Worker skips screenshots when `fill.sensitive` **or** `classifyStep` gates the fill; editor uses `type="password"` for sensitive fills. Next-step bleed / secret references still open. | | P1-3 | Secret values are stored in plaintext in the workflow definition; `sensitive` is a label. There is no secret-reference mechanism. | A database dump is every customer credential. The values also leave via the agent API, which returns `latestVersion.steps` verbatim. | Open — blocked on connector credentials (§5 step 6). | -| P1-4 | `extract` outputs are written into the hash-chained journal in cleartext. | A GDPR erasure request against an intentionally immutable chain is unresolvable. Fix the shape before there is data in it. | Open — needs journal payload allow-list / redaction design. | -| P1-5 | **No RBAC on any business operation.** A `MEMBER` can publish workflows, start runs, approve sensitive steps, and mint agent keys. Separation of duties is enforced (approver ≠ triggerer) but any two colleagues satisfy it. | "Human approval" currently means "anyone with a login." | **Partial.** Minting agent credentials is now OWNER/ADMIN only. Publish / start-run / approve still any member; VIEWER/APPROVER roles not added yet. | +| P1-4 | `extract` outputs are written into the hash-chained journal in cleartext. | A GDPR erasure request against an intentionally immutable chain is unresolvable. Fix the shape before there is data in it. | **Partial.** Org audit never carried extract text; run timeline no longer ships `RunStep.output` to the browser. Journal payload allow-list / erasable side store still open. | +| P1-5 | **No RBAC on any business operation.** A `MEMBER` can publish workflows, start runs, approve sensitive steps, and mint agent keys. Separation of duties is enforced (approver ≠ triggerer) but any two colleagues satisfy it. | "Human approval" currently means "anyone with a login." | **Partial.** Mint / publish / create / approve are OWNER/ADMIN only; start + reject stay open to MEMBER. VIEWER/APPROVER roles not added yet. | | P1-6 | SSRF: `navigate` accepts any URL, the worker runs `--no-sandbox`, and there is no private-IP denylist. Cloud metadata endpoints are reachable, and `extract` reads the result back out. | Credential theft by any authenticated user. | **Partial.** `checkPublicHttpUrl` blocks cloud metadata + RFC1918 (loopback allowed for fixtures; other private hosts only if they match `APP_URL`). Enforced at schema author time and again in `applyStep`. `--no-sandbox` and DNS-rebinding still open. | | P1-7 | Three BullMQ workers share one Redis connection, which BullMQ uses for blocking reads. | Intermittent stalls that will look like engine bugs. | **Closed.** Each Worker gets its own Redis connection (`apps/worker/src/index.ts`). | | P1-8 | No wall-clock run timeout. The lease heartbeat renews unconditionally for as long as the process lives. | Two pathological runs wedge a worker pod for every tenant. | **Closed.** `GHOST_RUN_TIMEOUT_MS` (default 30m); heartbeat stops renewing past the deadline and the loop raises `RUN_TIMEOUT` incident. | @@ -371,13 +371,12 @@ as P1-11 — green output that covers less than it appears to: 1. ~~P0-1 — reject session auth on the agent surface.~~ Done. 2. ~~P0-3 — rate limits on the four named endpoints.~~ Done (incl. Auth.js). -3. P1-5 — finish RBAC on approve / publish / start-run. `isOrgAdmin` already - exists; minting is gated; the role vocabulary still needs `VIEWER` and - `APPROVER`. -4. P1-2, P1-3, P1-4 — the secret-handling triad. Screenshot skip for sensitive - fills is in; secret references, editor masking, journal payload allow-list - remain. -5. P1-6 remainder (DNS-rebinding / sandbox), P1-1 checkpointed verify. +3. ~~P1-5 — RBAC on approve / publish / mint.~~ Done for the three-role matrix; + VIEWER/APPROVER vocabulary still open. +4. P1-2, P1-3, P1-4 — the secret-handling triad. Screenshot skip + editor + password mask + UI output redaction are in; secret references and journal + payload allow-list remain. +5. P1-6 remainder (DNS-rebinding / sandbox); P1-1 durable checkpointed verify. 6. ~~P1-10 — reject unimplemented step types at the API boundary.~~ Done. 7. ~~P1-11 — make local `pnpm test` fail loudly when `DATABASE_URL` is unset.~~ Done. diff --git a/cloud/packages/core/src/roles.test.ts b/cloud/packages/core/src/roles.test.ts index 9a8b580a..82952b8a 100644 --- a/cloud/packages/core/src/roles.test.ts +++ b/cloud/packages/core/src/roles.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { isOrgAdmin } from "./roles.js"; +import { canApproveRun, canPublishWorkflow, canStartRun, isOrgAdmin } from "./roles.js"; describe("isOrgAdmin", () => { it("treats OWNER and ADMIN as admin roles", () => { @@ -11,3 +11,16 @@ describe("isOrgAdmin", () => { expect(isOrgAdmin("MEMBER")).toBe(false); }); }); + +describe("capability matrix", () => { + it("lets only admins publish and approve", () => { + for (const role of ["OWNER", "ADMIN"] as const) { + expect(canPublishWorkflow(role)).toBe(true); + expect(canApproveRun(role)).toBe(true); + expect(canStartRun(role)).toBe(true); + } + expect(canPublishWorkflow("MEMBER")).toBe(false); + expect(canApproveRun("MEMBER")).toBe(false); + expect(canStartRun("MEMBER")).toBe(true); + }); +}); diff --git a/cloud/packages/core/src/roles.ts b/cloud/packages/core/src/roles.ts index dc951c2b..5ff13e46 100644 --- a/cloud/packages/core/src/roles.ts +++ b/cloud/packages/core/src/roles.ts @@ -1,16 +1,46 @@ import type { Role } from "@prisma/client"; /** - * Roles that may act on behalf of the organization rather than only - * themselves — inventory and revoke a colleague's agent credential, for - * instance. `Role` has been stored on `Membership` since Phase 0 but nothing - * has read it until now; every existing member is `OWNER` (orgs are - * auto-created single-member on first sign-in, and there is no invite flow - * yet), so this does not change behavior for any org that exists today. It - * only matters once an org has more than one member. + * Capability checks over `Membership.role`. + * + * `Role` has been stored since Phase 0; until recently almost nothing read it. + * Every existing single-member org is `OWNER`, so gating publish/approve to + * admins changes nothing for those orgs — it only matters once an org has + * MEMBERs who must not expand or authorize mutating work. + * + * VIEWER / APPROVER are deliberately not in the enum yet. Until they are, the + * three-role matrix is: + * + * | Capability | OWNER/ADMIN | MEMBER | + * | publish / create | yes | no | + * | approve (not reject) | yes | no | + * | start a run | yes | yes | + * | reject a gate | yes | yes | + * + * Reject stays open to every member: stopping an action is not authorizing it, + * and whoever started a runaway run has to be able to halt it. */ + const ADMIN_ROLES: ReadonlySet = new Set(["OWNER", "ADMIN"]); export function isOrgAdmin(role: Role): boolean { return ADMIN_ROLES.has(role); } + +/** Create a workflow or publish a new version of one. */ +export function canPublishWorkflow(role: Role): boolean { + return isOrgAdmin(role); +} + +/** + * Approve a pending sensitive-step gate. Reject is deliberately not gated — + * see the module comment. + */ +export function canApproveRun(role: Role): boolean { + return isOrgAdmin(role); +} + +/** Enqueue a run of a published workflow. Any member may trigger. */ +export function canStartRun(_role: Role): boolean { + return true; +} diff --git a/cloud/turbo.json b/cloud/turbo.json index e5ab1eb9..83c2ed31 100644 --- a/cloud/turbo.json +++ b/cloud/turbo.json @@ -18,6 +18,7 @@ "HR_API_KEY", "GHOST_STEP_TIMEOUT_MS", "GHOST_RUN_TIMEOUT_MS", + "GHOST_AUDIT_VERIFY_MAX_EVENTS", "GHOST_ALLOW_SKIP_DB_TESTS", "WORKER_CONCURRENCY", "APP_URL",