From eea1ecc55d66fa8a878f615f4a7a762e250ebeb9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 06:45:43 +0000 Subject: [PATCH 1/3] fix(cloud): close P0/P1 audit findings for agent auth and reliability Reject session cookies on /api/agent (bearer only), rate-limit Auth.js, block navigate SSRF to metadata/RFC1918, give each BullMQ worker its own Redis connection, add a run wall-clock timeout, skip screenshots on sensitive fills, require admin to mint agent keys, and fail pnpm test loudly when DATABASE_URL/REDIS_URL/GHOST_SESSION_KEY are unset. Co-authored-by: Muhammad Rafiq --- cloud/.env.example | 5 + .../web/src/app/api/agent/recordings/route.ts | 4 +- cloud/apps/web/src/app/api/agent/route.ts | 2 +- .../src/app/api/auth/[...nextauth]/route.ts | 36 ++++- .../agent-credentials.test.ts | 63 ++++---- .../api/settings/agent-credentials/route.ts | 15 ++ cloud/apps/web/src/lib/agent-auth.test.ts | 95 ++++-------- cloud/apps/web/src/lib/agent-auth.ts | 132 +++++++---------- cloud/apps/web/src/middleware.ts | 11 +- cloud/apps/worker/src/browser/driver.ts | 23 ++- cloud/apps/worker/src/index.ts | 45 ++++-- cloud/apps/worker/src/jobs/runWorkflow.ts | 37 ++++- cloud/docs/ARCHITECTURE_DECISIONS.md | 65 ++++----- cloud/package.json | 2 +- cloud/packages/core/package.json | 3 +- .../packages/core/src/net/public-url.test.ts | 49 +++++++ cloud/packages/core/src/net/public-url.ts | 137 ++++++++++++++++++ cloud/packages/core/src/schema/step.ts | 23 ++- cloud/packages/core/turbo.json | 3 + cloud/scripts/require-test-env.mjs | 36 +++++ cloud/turbo.json | 2 + 21 files changed, 554 insertions(+), 234 deletions(-) create mode 100644 cloud/packages/core/src/net/public-url.test.ts create mode 100644 cloud/packages/core/src/net/public-url.ts create mode 100644 cloud/scripts/require-test-env.mjs diff --git a/cloud/.env.example b/cloud/.env.example index 6c86ac37..555caf93 100644 --- a/cloud/.env.example +++ b/cloud/.env.example @@ -121,6 +121,11 @@ GHOST_SESSION_KEY="Z2hvc3QtZGV2LW9ubHkta2V5LW5vdC1hLXNlY3JldCE=" # Default per-step wall-clock budget in ms (a step may override it). Default 30s. GHOST_STEP_TIMEOUT_MS="30000" +# Wall-clock budget for an entire run in ms. Default 30 minutes. When exceeded +# the worker stops renewing the run lease and raises an INCIDENT so a stuck +# browser cannot wedge the pod forever. +# GHOST_RUN_TIMEOUT_MS="1800000" + # 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/api/agent/recordings/route.ts b/cloud/apps/web/src/app/api/agent/recordings/route.ts index b45c7def..e82ed34c 100644 --- a/cloud/apps/web/src/app/api/agent/recordings/route.ts +++ b/cloud/apps/web/src/app/api/agent/recordings/route.ts @@ -15,8 +15,8 @@ import { ingestTrace, MAX_TRACE_BYTES } from "@/lib/recording-ingest"; * human reviews the steps in the editor and publishes through * `POST /api/workflows`, which revalidates them. * - * `resolveAgentPrincipal` accepts a session too, and enforces the second - * factor on that path — see `lib/agent-auth.ts`. + * `resolveAgentPrincipal` requires a bearer credential (session cookies are + * refused) — see `lib/agent-auth.ts`. * * Body is JSON rather than multipart: the extension builds the trace in * memory and has no file to attach. diff --git a/cloud/apps/web/src/app/api/agent/route.ts b/cloud/apps/web/src/app/api/agent/route.ts index bf317daf..18d6d7f5 100644 --- a/cloud/apps/web/src/app/api/agent/route.ts +++ b/cloud/apps/web/src/app/api/agent/route.ts @@ -4,7 +4,7 @@ import { resolveAgentPrincipal } from "@/lib/agent-auth"; /** * Agent surface catalog. - * GET /api/agent — tool list + human-approval contract (session or Ghost credential). + * GET /api/agent — tool list + human-approval contract (Ghost credential). */ export async function GET(req: Request) { const authz = await resolveAgentPrincipal(req); diff --git a/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts b/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts index 86c9f3da..44dc1a30 100644 --- a/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts +++ b/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts @@ -1,3 +1,37 @@ import { handlers } from "@/auth"; +import { clientKey, rateLimit, tooManyRequests } from "@/lib/rate-limit"; -export const { GET, POST } = handlers; +/** + * Auth.js route handlers, wrapped with a fixed-window rate limit. + * + * MFA verify / invite accept / audit verify are already throttled. Sign-in was + * the remaining open door from the audit: an unthrottled `/api/auth/*` lets an + * attacker grind credentials, magic-link tokens, and OAuth callbacks without + * bound. + * + * POST (credentials / magic-link / callback) is the attack surface and is + * keyed tightly. GET (session / csrf / providers) is hit by ordinary + * `useSession` traffic, so it gets a looser budget that still bounds a + * scripted sweep of token endpoints. + * + * Fail-closed via `rateLimit` itself: a Redis outage refuses the request + * rather than letting auth through unmetered. + */ + +async function limited( + req: Request, + handle: (req: Request) => Promise, + { limit, windowSeconds, key }: { limit: number; windowSeconds: number; key: string }, +): Promise { + const result = await rateLimit(`${key}:${clientKey(req)}`, { limit, windowSeconds }); + if (!result.ok) return tooManyRequests(result); + return handle(req); +} + +export async function GET(req: Request): Promise { + return limited(req, handlers.GET, { limit: 120, windowSeconds: 60, key: "auth:get" }); +} + +export async function POST(req: Request): Promise { + return limited(req, handlers.POST, { limit: 20, windowSeconds: 60, key: "auth:post" }); +} diff --git a/cloud/apps/web/src/app/api/settings/agent-credentials/agent-credentials.test.ts b/cloud/apps/web/src/app/api/settings/agent-credentials/agent-credentials.test.ts index 7286a60e..d9f5e4cb 100644 --- a/cloud/apps/web/src/app/api/settings/agent-credentials/agent-credentials.test.ts +++ b/cloud/apps/web/src/app/api/settings/agent-credentials/agent-credentials.test.ts @@ -1,15 +1,15 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { createAgentToken } from "@ghost/core/agent-credentials"; import { prisma } from "@ghost/core/db"; /** - * Agent credential hardening: optional expiry at creation, and org-admin - * inventory/revocation. + * Agent credential hardening: optional expiry at creation, org-admin + * inventory/revocation, and admin-only minting. * * Revocation and expiry *enforcement* already existed (`resolveAgentPrincipal` - * in `@/lib/agent-auth` already rejected a revoked or expired credential) — - * what was missing was any way to actually set an expiry, and any way for an - * org admin to see or revoke a credential that isn't their own. `Role` has - * been stored on `Membership` since Phase 0 but nothing read it until now. + * rejects a revoked or expired credential). Minting is now OWNER/ADMIN only — + * a MEMBER must not expand the agent attack surface by creating keys that can + * start runs. * * Requires DATABASE_URL; skips cleanly without one. */ @@ -46,7 +46,8 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { }); beforeEach(() => { - session.current = { user: { id: member, orgId } }; + // Minting is admin-only; most create tests run as OWNER. + session.current = { user: { id: owner, orgId } }; }); async function create(body: Record) { @@ -72,6 +73,21 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { return GET(); } + /** Seed a credential owned by `userId` without going through the HTTP mint gate. */ + async function seedCredential(userId: string, name: string) { + const generated = createAgentToken(); + return prisma.agentCredential.create({ + data: { + orgId, + userId, + name, + tokenHash: generated.tokenHash, + tokenHint: generated.tokenHint, + }, + select: { id: true }, + }); + } + it("creates a credential with no expiry by default", async () => { const res = await create({ name: "no-expiry" }); expect(res.status).toBe(201); @@ -85,7 +101,6 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { expect(res.status).toBe(201); const body = (await res.json()) as { credential: { expiresAt: string } }; const expiresAt = new Date(body.credential.expiresAt).getTime(); - // ~30 days out, generous window for test execution time. expect(expiresAt).toBeGreaterThan(before + 29 * 86_400_000); expect(expiresAt).toBeLessThan(before + 31 * 86_400_000); }); @@ -100,23 +115,25 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { }, ); + it("refuses minting to a non-admin MEMBER", async () => { + session.current = { user: { id: member, orgId } }; + const res = await create({ name: "member-mint" }); + expect(res.status).toBe(403); + const created = await prisma.agentCredential.findFirst({ where: { orgId, name: "member-mint" } }); + expect(created).toBeNull(); + }); + it("lets a member revoke their own credential but not a colleague's", async () => { - session.current = { user: { id: owner, orgId } }; - const ownerCred = ( - (await (await create({ name: "owner-cred" })).json()) as { credential: { id: string } } - ).credential; + const ownerCred = (await (await create({ name: "owner-cred" })).json() as { + credential: { id: string }; + }).credential; - session.current = { user: { id: member, orgId } }; - const memberCred = ( - (await (await create({ name: "member-cred" })).json()) as { credential: { id: string } } - ).credential; + const memberCred = await seedCredential(member, "member-cred"); - // Member revoking their own credential succeeds. + session.current = { user: { id: member, orgId } }; const ownRevoke = await revoke(memberCred.id); expect(ownRevoke.status).toBe(200); - // Member revoking the owner's credential does not — same 404 shape as a - // nonexistent id, so a non-admin cannot even confirm it exists. const otherRevoke = await revoke(ownerCred.id); expect(otherRevoke.status).toBe(404); const stillActive = await prisma.agentCredential.findUniqueOrThrow({ @@ -126,10 +143,7 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { }); it("lets an OWNER revoke a member's credential", async () => { - session.current = { user: { id: member, orgId } }; - const memberCred = ( - (await (await create({ name: "revoke-me" })).json()) as { credential: { id: string } } - ).credential; + const memberCred = await seedCredential(member, "revoke-me"); session.current = { user: { id: owner, orgId } }; const res = await revoke(memberCred.id); @@ -145,8 +159,7 @@ describe.skipIf(!hasDb)("agent credential hardening (Postgres)", () => { }); it("shows an OWNER every active credential in the org, across members", async () => { - session.current = { user: { id: member, orgId } }; - await create({ name: "member-visible-to-owner" }); + await seedCredential(member, "member-visible-to-owner"); session.current = { user: { id: owner, orgId } }; const res = await orgList(); diff --git a/cloud/apps/web/src/app/api/settings/agent-credentials/route.ts b/cloud/apps/web/src/app/api/settings/agent-credentials/route.ts index f8b3dac2..1586d596 100644 --- a/cloud/apps/web/src/app/api/settings/agent-credentials/route.ts +++ b/cloud/apps/web/src/app/api/settings/agent-credentials/route.ts @@ -1,5 +1,6 @@ import { auth } from "@/auth"; import { createAgentToken } from "@ghost/core/agent-credentials"; +import { isOrgAdmin } from "@ghost/core/roles"; import { prisma } from "@/lib/db"; export async function GET() { @@ -34,6 +35,20 @@ export async function POST(req: Request) { if (!session?.user.id || !session.user.orgId) { return Response.json({ error: "unauthorized" }, { status: 401 }); } + + // Minting a credential that can start runs is an org-admin action. A MEMBER + // can still *use* a key an admin handed them (or revoke their own), but they + // cannot expand the attack surface by creating more. + const membership = await prisma.membership.findUnique({ + where: { + userId_orgId: { userId: session.user.id, orgId: session.user.orgId }, + }, + select: { role: true }, + }); + if (!membership || !isOrgAdmin(membership.role)) { + return Response.json({ error: "forbidden" }, { status: 403 }); + } + const body = (await req.json().catch(() => null)) as { name?: unknown; expiresInDays?: unknown; diff --git a/cloud/apps/web/src/lib/agent-auth.test.ts b/cloud/apps/web/src/lib/agent-auth.test.ts index d412169e..c47501f1 100644 --- a/cloud/apps/web/src/lib/agent-auth.test.ts +++ b/cloud/apps/web/src/lib/agent-auth.test.ts @@ -1,84 +1,53 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; /** - * The agent surface is excluded from the middleware matcher, so the - * second-factor gate that covers every other session-authenticated route has - * to be enforced inside `resolveAgentPrincipal`. + * The agent surface is excluded from the middleware matcher (bearer clients + * have no session), so it must refuse a browser session cookie entirely — + * not merely MFA-gate it. Otherwise a stolen session can `POST /api/agent/runs` + * while `/api/runs` is correctly challenged. * - * Without it, a stolen session cookie for an MFA-enabled user could start a - * run through `POST /api/agent/runs` — against the customer's real systems — - * while the identical action through `POST /api/runs` was correctly - * challenged. That is the second factor covering everything except the - * endpoint that moves money. - * - * These tests pin the three cases that matter. They are hermetic: no database, - * no session, just the branch logic. + * These tests pin the refusal. They are hermetic: no database. */ -const state = vi.hoisted(() => ({ - session: null as null | { - user: { id?: string; orgId?: string; mfaEnabled?: boolean; sid?: string }; +vi.mock("@/lib/db", () => ({ + prisma: { + agentCredential: { + findUnique: async () => null, + update: async () => undefined, + }, + membership: { findUnique: async () => null }, }, - mfaCookie: undefined as string | undefined, - mfaVerifies: false, })); -vi.mock("@/auth", () => ({ auth: async () => state.session })); - -vi.mock("next/headers", () => ({ - cookies: async () => ({ get: () => (state.mfaCookie ? { value: state.mfaCookie } : undefined) }), -})); - -vi.mock("@/lib/mfa-cookie", () => ({ - MFA_COOKIE_NAME: "ghost_mfa", - verifyMfaCookie: async () => state.mfaVerifies, -})); - -vi.mock("@/lib/db", () => ({ prisma: {} })); - const { resolveAgentPrincipal } = await import("@/lib/agent-auth"); -/** No Authorization header, so resolution falls to the session path. */ -function sessionRequest() { +function bareRequest() { return new Request("https://ghost.test/api/agent/runs", { method: "POST" }); } -beforeEach(() => { - state.session = { user: { id: "u1", orgId: "o1", mfaEnabled: false, sid: "s1" } }; - state.mfaCookie = undefined; - state.mfaVerifies = false; -}); - -describe("resolveAgentPrincipal — session path", () => { - it("admits a user who has not enabled two-factor", async () => { - const result = await resolveAgentPrincipal(sessionRequest()); - expect(result).toMatchObject({ ok: true, principal: { orgId: "o1", via: "session" } }); - }); - - it("refuses an MFA-enabled user whose challenge has not been answered", async () => { - state.session = { user: { id: "u1", orgId: "o1", mfaEnabled: true, sid: "s1" } }; - state.mfaVerifies = false; - - const result = await resolveAgentPrincipal(sessionRequest()); - - // 403, matching what the middleware returns for an API path — a distinct - // status from 401, because the caller *is* authenticated and needs to do - // something specific about it. - expect(result).toMatchObject({ ok: false, status: 403 }); +function bearerRequest(token: string) { + return new Request("https://ghost.test/api/agent/runs", { + method: "POST", + headers: { Authorization: `Bearer ${token}` }, }); +} - it("admits an MFA-enabled user who has answered the challenge", async () => { - state.session = { user: { id: "u1", orgId: "o1", mfaEnabled: true, sid: "s1" } }; - state.mfaCookie = "signed-cookie"; - state.mfaVerifies = true; +beforeEach(() => { + vi.clearAllMocks(); +}); - const result = await resolveAgentPrincipal(sessionRequest()); - expect(result).toMatchObject({ ok: true, principal: { userId: "u1", via: "session" } }); +describe("resolveAgentPrincipal — bearer only", () => { + it("refuses a caller with no Authorization header", async () => { + const result = await resolveAgentPrincipal(bareRequest()); + expect(result).toMatchObject({ + ok: false, + status: 401, + error: expect.stringMatching(/agent api key required/i), + }); }); - it("still refuses an unauthenticated caller with 401", async () => { - state.session = null; - const result = await resolveAgentPrincipal(sessionRequest()); - expect(result).toMatchObject({ ok: false, status: 401 }); + it("refuses an invalid bearer token", async () => { + const result = await resolveAgentPrincipal(bearerRequest("not-a-real-key")); + expect(result).toMatchObject({ ok: false, status: 401, error: "invalid agent api key" }); }); }); diff --git a/cloud/apps/web/src/lib/agent-auth.ts b/cloud/apps/web/src/lib/agent-auth.ts index 77d17835..f29aae4e 100644 --- a/cloud/apps/web/src/lib/agent-auth.ts +++ b/cloud/apps/web/src/lib/agent-auth.ts @@ -1,35 +1,27 @@ -import { cookies } from "next/headers"; -import { auth } from "@/auth"; import { prisma } from "@/lib/db"; -import { MFA_COOKIE_NAME, verifyMfaCookie } from "@/lib/mfa-cookie"; import { hashAgentToken } from "@ghost/core/agent-credentials"; /** * Principal for agent HTTP / MCP calls. * - * Two paths: - * 1. Browser session (NextAuth) — same as the dashboard. - * 2. A revocable bearer credential created inside the authenticated Ghost app. + * **Bearer credential only.** A browser session cookie is deliberately + * refused here. `/api/agent` is excluded from the middleware matcher because a + * bearer-credential caller has no session at all — and that same exclusion + * used to mean a stolen session cookie could `POST /api/agent/runs` and start + * a real run against the customer's systems while the identical action through + * `/api/runs` was MFA-gated. Closing that hole by re-checking MFA on the + * session path was a bandage; the honest boundary is that agents authenticate + * with a revocable, expirable credential minted in Settings, and humans use + * the dashboard routes. * - * **The second factor is enforced here, not in middleware.** `/api/agent` is - * excluded from the middleware matcher because a bearer-credential caller has - * no session at all and a session gate would reject every legitimate MCP - * client. But the session path below is real: a browser cookie authenticates - * these routes too, and `POST /api/agent/runs` starts a run against the - * customer's systems. Without the check below, a stolen session cookie for an - * MFA-enabled user could execute a workflow through `/api/agent` while the - * identical action through `/api/runs` was correctly challenged — the second - * factor covering everything except the endpoint that moves money. - * - * Bearer credentials are deliberately not MFA-gated: they are non-interactive, - * already revocable and expirable, and there is nobody present to answer a - * challenge. Their security story is issuance and revocation, not a second - * factor. + * Bearer credentials are not MFA-gated: they are non-interactive, already + * revocable and expirable, and there is nobody present to answer a challenge. + * Their security story is issuance and revocation, not a second factor. */ export type AgentPrincipal = { userId: string; orgId: string; - via: "session" | "api_key"; + via: "api_key"; }; function bearerToken(req: Request): string | null { @@ -50,71 +42,55 @@ export async function resolveAgentPrincipal( > { const token = bearerToken(req); - if (token) { - const credential = await prisma.agentCredential.findUnique({ - where: { tokenHash: hashAgentToken(token) }, - select: { - id: true, - orgId: true, - userId: true, - revokedAt: true, - expiresAt: true, - }, - }); - if ( - !credential || - credential.revokedAt || - (credential.expiresAt && credential.expiresAt <= new Date()) - ) { - return { ok: false, status: 401, error: "invalid agent api key" }; - } - const membership = await prisma.membership.findUnique({ - where: { - userId_orgId: { userId: credential.userId, orgId: credential.orgId }, - }, - select: { id: true }, - }); - if (!membership) - return { ok: false, status: 401, error: "invalid agent api key" }; - await prisma.agentCredential.update({ - where: { id: credential.id }, - data: { lastUsedAt: new Date() }, - }); + if (!token) { + // A session cookie is not enough. Even with MFA answered, the agent surface + // is for non-interactive clients that hold a minted credential — not for + // the browser. Dashboard actions go through `/api/runs` et al., which the + // middleware MFA-gates. Distinguishing "has a session" from "has nothing" + // is unnecessary: both get 401, and the message tells a browser caller how + // to proceed. return { - ok: true, - principal: { - orgId: credential.orgId, - userId: credential.userId, - via: "api_key", - }, + ok: false, + status: 401, + error: "agent api key required — create one in Settings", }; } - const session = await auth(); - if (!session?.user?.orgId || !session.user.id) { - return { ok: false, status: 401, error: "unauthorized" }; - } - - // Same verdict the middleware would reach for any other session-authenticated - // route, reproduced here because the middleware cannot run on this path. - if (session.user.mfaEnabled) { - const jar = await cookies(); - const verified = await verifyMfaCookie( - jar.get(MFA_COOKIE_NAME)?.value, - session.user.id, - session.user.sid, - ); - if (!verified) { - return { ok: false, status: 403, error: "two-factor verification required" }; - } + const credential = await prisma.agentCredential.findUnique({ + where: { tokenHash: hashAgentToken(token) }, + select: { + id: true, + orgId: true, + userId: true, + revokedAt: true, + expiresAt: true, + }, + }); + if ( + !credential || + credential.revokedAt || + (credential.expiresAt && credential.expiresAt <= new Date()) + ) { + return { ok: false, status: 401, error: "invalid agent api key" }; } - + const membership = await prisma.membership.findUnique({ + where: { + userId_orgId: { userId: credential.userId, orgId: credential.orgId }, + }, + select: { id: true }, + }); + if (!membership) + return { ok: false, status: 401, error: "invalid agent api key" }; + await prisma.agentCredential.update({ + where: { id: credential.id }, + data: { lastUsedAt: new Date() }, + }); return { ok: true, principal: { - orgId: session.user.orgId, - userId: session.user.id, - via: "session", + orgId: credential.orgId, + userId: credential.userId, + via: "api_key", }, }; } diff --git a/cloud/apps/web/src/middleware.ts b/cloud/apps/web/src/middleware.ts index d3d6def8..291edb20 100644 --- a/cloud/apps/web/src/middleware.ts +++ b/cloud/apps/web/src/middleware.ts @@ -78,13 +78,10 @@ export const config = { // Session-authenticated API surface, so the second-factor gate above // applies to it too. Three exclusions, each load-bearing: // auth — Auth.js's own endpoints; gating them prevents signing in. - // agent — may be authenticated by a bearer agent credential, which has - // no session at all, so a session gate here would reject every - // legitimate MCP/agent client. That surface also accepts a - // session, and `resolveAgentPrincipal` runs the same - // second-factor check this middleware would have — see - // lib/agent-auth.ts. Do not rely on this exclusion meaning - // "unauthenticated": it means "authenticated elsewhere". + // agent — bearer-credential authenticated only (no session). A session + // gate here would reject every legitimate MCP/agent client; + // `resolveAgentPrincipal` refuses cookies and requires a minted + // API key — see lib/agent-auth.ts. // mfa — how the challenge is answered; gating it deadlocks the user. "/api/((?!auth|agent|mfa).*)", ], diff --git a/cloud/apps/worker/src/browser/driver.ts b/cloud/apps/worker/src/browser/driver.ts index c1204664..d38bf253 100644 --- a/cloud/apps/worker/src/browser/driver.ts +++ b/cloud/apps/worker/src/browser/driver.ts @@ -1,12 +1,18 @@ import { chromium, type Browser, type BrowserContext, type Page } from "playwright"; import type { WorkflowStep } from "@ghost/core/schema/step"; +import { assertPublicHttpUrl } from "@ghost/core/net/public-url"; import { discoverChromium } from "./chromium.js"; import { resolveLocator } from "./selector.js"; import { runVerification, type VerifyResult } from "./verify.js"; import type { RestorePlan } from "../runtime/restore.js"; export interface StepResult { - screenshot: Buffer; + /** + * PNG bytes, or `null` when the step must not be captured (sensitive fills). + * The caller skips the artifact store when this is null so OTP/card pixels + * never land in the blob store. + */ + screenshot: Buffer | null; verification: VerifyResult | null; /** Values this step captured, by name. Only `extract` produces any. */ outputs: Record; @@ -94,6 +100,10 @@ export async function applyStep( switch (step.type) { case "navigate": + // Defense in depth: schema already rejects private hosts at author time, + // but a version written before that check — or a hand-edited row — must + // still not reach metadata endpoints from the worker. + assertPublicHttpUrl(step.url); await page.goto(step.url, { waitUntil: "domcontentloaded", timeout }); return {}; case "click": @@ -218,6 +228,11 @@ export async function verifyStep(page: Page, step: WorkflowStep): Promise { const outputs = await applyStep(page, step, opts); const verification = await verifyStep(page, step); - const screenshot = await page.screenshot(); + // Sensitive fills (OTP, card, password-shaped) leave secret pixels on the + // page. Capturing them would put cardholder data into the artifact store + // behind a positive allow-list of `step-*.png` keys. Skip the capture + // entirely rather than store-and-redact — there is nothing safe to show. + const screenshot = shouldCaptureScreenshot(step) ? await page.screenshot() : null; return { screenshot, verification, outputs, url: page.url() }; } diff --git a/cloud/apps/worker/src/index.ts b/cloud/apps/worker/src/index.ts index 8048fb42..8316c10e 100644 --- a/cloud/apps/worker/src/index.ts +++ b/cloud/apps/worker/src/index.ts @@ -23,15 +23,26 @@ import { startCaptureServer } from "./capture/server.js"; * * `noop` proves the web ↔ Redis ↔ worker wiring (Phase 0). `run-workflow` * executes a workflow run via Playwright, halting at approval gates (Phase 1). - * Each queue gets its own Worker so failures stay isolated. + * Each queue gets its own Worker so failures stay isolated — and each Worker + * gets its own Redis connection. BullMQ uses blocking reads on the connection + * it is given; sharing one across three workers produced intermittent stalls + * that looked like engine bugs. */ initSentry("worker"); const log = createLogger("worker"); -const connection = createRedisConnection(); + +const runConnection = createRedisConnection(); +const compensateConnection = createRedisConnection(); +const noopConnection = createRedisConnection(); +const purgeConnection = createRedisConnection(); +// Queues that only enqueue (schedulers / reclaim) can share — they do not +// block. Keep them separate from the Worker connections anyway so a Worker +// close cannot take the scheduler's connection with it. +const schedulerConnection = createRedisConnection(); const runWorker = new Worker(QUEUE_NAMES.runWorkflow, runWorkflowJob, { - connection, + connection: runConnection, // A run holds a browser, so concurrency is memory-bound rather than // CPU-bound; keep it small and explicit rather than relying on the default. concurrency: Number(process.env.WORKER_CONCURRENCY ?? 2), @@ -51,7 +62,13 @@ runWorker.on("failed", (job, err) => { const compensateWorker = new Worker( QUEUE_NAMES.compensateRun, compensateRunJob, - { connection, concurrency: 1, lockDuration: 60_000, stalledInterval: 30_000, maxStalledCount: 1 }, + { + connection: compensateConnection, + concurrency: 1, + lockDuration: 60_000, + stalledInterval: 30_000, + maxStalledCount: 1, + }, ); compensateWorker.on("failed", (job, err) => { log.error("compensate-run job failed", { runId: job?.data.runId, ...serializeError(err) }); @@ -64,7 +81,7 @@ const noopWorker = new Worker( log.info("noop job received", { jobId: job.id, message: job.data.message, requestedAt: job.data.requestedAt }); return { ok: true, handledAt: new Date().toISOString() }; }, - { connection }, + { connection: noopConnection }, ); noopWorker.on("completed", (job) => { @@ -77,7 +94,7 @@ noopWorker.on("failed", (job, err) => { const purgeWorker = new Worker( QUEUE_NAMES.purgeArtifacts, purgeArtifactsJob, - { connection, concurrency: 1 }, + { connection: purgeConnection, concurrency: 1 }, ); purgeWorker.on("failed", (job, err) => { log.error("purge-artifacts job failed", { jobId: job?.id, ...serializeError(err) }); @@ -89,7 +106,9 @@ purgeWorker.on("failed", (job, err) => { // so every boot (including a redeploy) re-asserts the same daily schedule // instead of accumulating a duplicate one — this line runs on every worker // start, not just the first. -const purgeQueue = new Queue(QUEUE_NAMES.purgeArtifacts, { connection }); +const purgeQueue = new Queue(QUEUE_NAMES.purgeArtifacts, { + connection: schedulerConnection, +}); await purgeQueue.upsertJobScheduler( "purge-artifacts-daily", { every: 24 * 60 * 60 * 1000 }, @@ -102,7 +121,9 @@ await purgeQueue.upsertJobScheduler( // this process is up. Safe to run in every replica: the job id is derived from // the expired lease, so concurrent sweeps collapse into one job, and the run // lease still admits exactly one executor. -const reclaimQueue = new Queue(QUEUE_NAMES.runWorkflow, { connection }); +const reclaimQueue = new Queue(QUEUE_NAMES.runWorkflow, { + connection: schedulerConnection, +}); const sweepStalledRuns = async (): Promise => { try { await reclaimStalledRuns(reclaimQueue); @@ -146,7 +167,13 @@ async function shutdown(signal: string): Promise { purgeWorker.close(), purgeQueue.close(), ]); - await connection.quit(); + await Promise.all([ + runConnection.quit(), + compensateConnection.quit(), + noopConnection.quit(), + purgeConnection.quit(), + schedulerConnection.quit(), + ]); process.exit(0); } diff --git a/cloud/apps/worker/src/jobs/runWorkflow.ts b/cloud/apps/worker/src/jobs/runWorkflow.ts index c0417fa5..293bff1f 100644 --- a/cloud/apps/worker/src/jobs/runWorkflow.ts +++ b/cloud/apps/worker/src/jobs/runWorkflow.ts @@ -64,6 +64,12 @@ import { const LEASE_MS = 60_000; const TERMINAL = new Set(["SUCCEEDED", "FAILED", "CANCELED"]); +/** Wall-clock budget for one run. Default 30 minutes; override with GHOST_RUN_TIMEOUT_MS. */ +function runTimeoutMs(): number { + const fromEnv = Number(process.env.GHOST_RUN_TIMEOUT_MS); + return Number.isFinite(fromEnv) && fromEnv > 0 ? fromEnv : 30 * 60_000; +} + export async function runWorkflowJob(job: Job): Promise { const { runId } = job.data; @@ -150,7 +156,15 @@ export async function runWorkflowJob(job: Job): Promise { }); if (leased.count !== 1) return; + // Wall-clock deadline for this attempt. The lease heartbeat below renews + // unconditionally for as long as the process lives — without a separate + // deadline, two pathological runs can wedge a worker pod forever. Checked + // at the top of every loop iteration; once exceeded we stop renewing and + // raise an incident rather than leaving the run RUNNING with no owner. + const runDeadline = Date.now() + runTimeoutMs(); + const heartbeat = setInterval(() => { + if (Date.now() >= runDeadline) return; void prisma.run .updateMany({ where: { id: runId, leaseOwner: owner }, @@ -273,6 +287,17 @@ export async function runWorkflowJob(job: Job): Promise { const mutableOutputs = scope.steps as Record>; for (;;) { + if (Date.now() >= runDeadline) { + await raiseIncident( + runId, + run.orgId, + run.triggeredById, + run.cursor, + `RUN_TIMEOUT: exceeded wall-clock budget of ${runTimeoutMs()}ms`, + ); + return; + } + const fresh = await prisma.run.findUnique({ where: { id: runId }, select: { status: true }, @@ -552,11 +577,13 @@ async function executeStep(args: ExecuteArgs): Promise { } const result = await runStep(session.page, step, { timeoutMs }); - screenshotRef = await artifactStore().put( - screenshotKey(runId, index), - result.screenshot, - "image/png", - ); + if (result.screenshot) { + screenshotRef = await artifactStore().put( + screenshotKey(runId, index), + result.screenshot, + "image/png", + ); + } verification = result.verification; outputs = result.outputs; url = result.url; diff --git a/cloud/docs/ARCHITECTURE_DECISIONS.md b/cloud/docs/ARCHITECTURE_DECISIONS.md index cc93a5ae..f1a4a024 100644 --- a/cloud/docs/ARCHITECTURE_DECISIONS.md +++ b/cloud/docs/ARCHITECTURE_DECISIONS.md @@ -328,27 +328,27 @@ plus a small number of real defects. Ordered by business impact. ### P0 — before this is pointed at anything real -| # | Finding | Why it matters | -|---|---|---| -| P0-1 | **MFA is bypassable on `/api/agent/*`.** The middleware matcher excludes `/api/agent`, and `resolveAgentPrincipal` accepts a session cookie in addition to a bearer token. `POST /api/agent/runs` starts a real run. | The second factor does not cover the action that moves money. A stolen session cookie executes workflows; the same action through `/api/runs` is correctly blocked. | -| P0-2 | **Raw traces shipped to a third party.** | Closed by §1. | -| P0-3 | **No rate limiting anywhere.** `/api/mfa/verify` accepts unlimited attempts against a six-digit code with a skew window. Also sign-in, `/api/invitations/accept`, `/api/audit/verify`. | An unthrottled TOTP endpoint is not a second factor. | +| # | Finding | Why it matters | Status | +|---|---|---|---| +| P0-1 | **MFA is bypassable on `/api/agent/*`.** The middleware matcher excludes `/api/agent`, and `resolveAgentPrincipal` accepted a session cookie in addition to a bearer token. `POST /api/agent/runs` starts a real run. | The second factor did not cover the action that moves money. | **Closed.** `resolveAgentPrincipal` now refuses session cookies entirely — bearer credential only (`apps/web/src/lib/agent-auth.ts`). Humans use `/api/runs`; agents mint a key in Settings. | +| P0-2 | **Raw traces shipped to a third party.** | Closed by §1. | Closed | +| P0-3 | **No rate limiting anywhere.** `/api/mfa/verify` accepts unlimited attempts against a six-digit code with a skew window. Also sign-in, `/api/invitations/accept`, `/api/audit/verify`. | An unthrottled TOTP endpoint is not a second factor. | **Closed.** Redis fixed-window limiter (`apps/web/src/lib/rate-limit.ts`) on MFA verify, invite accept, audit verify, and Auth.js GET/POST (`/api/auth/[...nextauth]`). Fail-closed on Redis outage. | ### P1 — before real customers -| # | Finding | Why it matters | -|---|---|---| -| 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. | -| 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. | -| 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. | -| 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. | -| 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." This is the first control an auditor asks about. | -| 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. | -| P1-7 | Three BullMQ workers share one Redis connection, which BullMQ uses for blocking reads. | Intermittent stalls that will look like engine bugs. | -| 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. | -| P1-9 | Concurrency is per-workflow only; the queue is a single global FIFO with no per-org fairness. | One tenant starves all others. No per-tenant SLA is possible. | -| P1-10 | `apiCall` and `sendEmail` parse, classify, gate — and execute nothing. The editor refuses them; the API does not. | A human approves "send this invoice," nothing is sent, and the run reports SUCCEEDED. The one unfinished feature that produces a wrong business outcome. | -| P1-11 | Local `pnpm test` exits 0 while skipping ~100 DB-gated tests, including every test of the run/approval/verify loop. | The safety-critical half of the product has no pre-push signal on a developer machine. | +| # | 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-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-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` denylist (private/link-local/metadata) 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. | +| P1-9 | Concurrency is per-workflow only; the queue is a single global FIFO with no per-org fairness. | One tenant starves all others. No per-tenant SLA is possible. | Open — product/capacity model. | +| P1-10 | `apiCall` and `sendEmail` parse, classify, gate — and execute nothing. The editor refuses them; the API does not. | A human approves "send this invoice," nothing is sent, and the run reports SUCCEEDED. | **Closed.** `authoredSteps` rejects non-`EDITABLE_STEP_TYPES` at the API boundary. | +| P1-11 | Local `pnpm test` exits 0 while skipping ~100 DB-gated tests, including every test of the run/approval/verify loop. | The safety-critical half of the product has no pre-push signal on a developer machine. | **Closed.** `scripts/require-test-env.mjs` refuses `pnpm test` without `DATABASE_URL`/`REDIS_URL`/`GHOST_SESSION_KEY` (escape hatch: `GHOST_ALLOW_SKIP_DB_TESTS=1`). `@ghost/core`'s `test` now depends on its own `build` so prisma generate races are gone. | ### P2 — real, not urgent @@ -363,24 +363,23 @@ given 256-bit random input, but unpeppered); Chromium runs `--no-sandbox`. Two build-system defects found while running the suite, both in the same family as P1-11 — green output that covers less than it appears to: -- `turbo.json` omits `GHOST_MFA_KEY` from `globalEnv`, so Turbo strips it from - `pnpm test` and `pnpm build`. -- `test` depends on `^build` (dependencies' builds) but not on a package's own - `build`, so `@ghost/core`'s tests race its own `prisma generate`. This fails - intermittently with `Cannot find module '.prisma/client/index.js'`. +- ~~`turbo.json` omits `GHOST_MFA_KEY` from `globalEnv`~~ — closed; present in root `turbo.json`. +- ~~`test` depends on `^build` but not on a package's own `build`~~ — closed for + `@ghost/core` (`packages/core/turbo.json` makes `test` depend on `build`). ### Recommended order -1. P0-1 — reject session auth on the agent surface. Hours. -2. P0-3 — rate limits on the four named endpoints. A day. -3. P1-5 — RBAC on approve / publish / start-run / mint-key. `isOrgAdmin` already - exists; the role vocabulary needs `VIEWER` and `APPROVER` first. -4. P1-2, P1-3, P1-4 — the secret-handling triad. One design (secret references, - screenshot suppression, journal payload allow-list), three call sites. -5. P1-6 URL allow-list, P1-8 run timeout, P1-1 pagination. -6. P1-10 — reject the unimplemented step types at the API boundary, not only in - the editor. -7. P1-11 — make local `pnpm test` fail loudly when `DATABASE_URL` is unset. +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. +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/package.json b/cloud/package.json index 6b06a85e..2217fceb 100644 --- a/cloud/package.json +++ b/cloud/package.json @@ -15,7 +15,7 @@ "build": "turbo run build", "lint": "turbo run lint", "typecheck": "turbo run typecheck", - "test": "turbo run test", + "test": "node scripts/require-test-env.mjs && turbo run test", "format": "prettier --write \"**/*.{ts,tsx,md,json}\"", "format:check": "prettier --check \"**/*.{ts,tsx,md,json}\"", "db:generate": "prisma generate --schema packages/core/prisma/schema.prisma", diff --git a/cloud/packages/core/package.json b/cloud/packages/core/package.json index 5d642587..dd2f7bed 100644 --- a/cloud/packages/core/package.json +++ b/cloud/packages/core/package.json @@ -36,7 +36,8 @@ "./retention": "./src/retention.ts", "./logger": "./src/logger.ts", "./sentry": "./src/sentry.ts", - "./env": "./src/env.ts" + "./env": "./src/env.ts", + "./net/public-url": "./src/net/public-url.ts" }, "scripts": { "build": "prisma generate && tsc --noEmit", diff --git a/cloud/packages/core/src/net/public-url.test.ts b/cloud/packages/core/src/net/public-url.test.ts new file mode 100644 index 00000000..aac17213 --- /dev/null +++ b/cloud/packages/core/src/net/public-url.test.ts @@ -0,0 +1,49 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { checkPublicHttpUrl } from "./public-url.js"; + +describe("checkPublicHttpUrl", () => { + const prevAppUrl = process.env.APP_URL; + + afterEach(() => { + if (prevAppUrl === undefined) delete process.env.APP_URL; + else process.env.APP_URL = prevAppUrl; + }); + + it.each([ + "https://example.com/path", + "http://example.com", + "https://shop.customer.com/orders", + "https://8.8.8.8/health", + "http://localhost:3000/fixtures/order", + "http://127.0.0.1:4123/", + ])("allows %s", (raw) => { + delete process.env.APP_URL; + expect(checkPublicHttpUrl(raw).ok).toBe(true); + }); + + it.each([ + "http://169.254.169.254/latest/meta-data/", + "http://metadata.google.internal/", + "ftp://example.com/", + "file:///etc/passwd", + "not a url", + "http://10.0.0.5/secret", + "http://192.168.1.1/", + "http://172.16.0.1/", + "http://printer.local/", + ])("rejects %s", (raw) => { + delete process.env.APP_URL; + expect(checkPublicHttpUrl(raw).ok).toBe(false); + }); + + it("allows a private APP_URL host for self-hosted Ghost", () => { + process.env.APP_URL = "http://10.0.0.5:3000"; + expect(checkPublicHttpUrl("http://10.0.0.5:3000/fixtures/order").ok).toBe(true); + expect(checkPublicHttpUrl("http://10.0.0.9/").ok).toBe(false); + }); + + it("never allows cloud metadata even if APP_URL were somehow set to it", () => { + process.env.APP_URL = "http://169.254.169.254"; + expect(checkPublicHttpUrl("http://169.254.169.254/latest/meta-data/").ok).toBe(false); + }); +}); diff --git a/cloud/packages/core/src/net/public-url.ts b/cloud/packages/core/src/net/public-url.ts new file mode 100644 index 00000000..ede3a19b --- /dev/null +++ b/cloud/packages/core/src/net/public-url.ts @@ -0,0 +1,137 @@ +/** + * Reject URLs that would let a workflow reach private network targets from + * the worker's browser. + * + * `navigate` previously accepted any `z.string().url()`. The worker launches + * Chromium with `--no-sandbox`, so a crafted step could hit cloud metadata + * endpoints (or RFC1918 hosts on the worker's network) and `extract` the + * response back out — credential theft by any authenticated author. + * + * Policy: + * 1. Only `http:` / `https:`. + * 2. Always refuse cloud-metadata hostnames and `169.254.169.254`. + * 3. Loopback (`localhost`, `127.0.0.0/8`, `::1`) is allowed — the local + * demo fixture and the worker's hermetic test servers live there. + * 4. Other private / link-local / ULA hosts are refused, **except** when + * the host matches `APP_URL` (a self-hosted Ghost on a private IP). + * + * DNS rebinding is outside this check — that needs a resolve-then-connect + * gate in the browser launcher. + */ + +const METADATA_HOSTNAMES = new Set([ + "metadata.google.internal", + "metadata.google", + "metadata.azure.com", + "metadata", +]); + +/** IPv4 dotted-quad → 32-bit int, or null if not a literal IPv4. */ +function parseIpv4(host: string): number | null { + const m = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/.exec(host); + if (!m) return null; + const octets = m.slice(1).map(Number); + if (octets.some((n) => n > 255)) return null; + return ((octets[0]! << 24) | (octets[1]! << 16) | (octets[2]! << 8) | octets[3]!) >>> 0; +} + +function isLoopbackHost(host: string): boolean { + if (host === "localhost" || host.endsWith(".localhost")) return true; + if (host === "::1") return true; + const ipv4 = parseIpv4(host); + // 127.0.0.0/8 + if (ipv4 !== null && (ipv4 & 0xff000000) === 0x7f000000) return true; + return false; +} + +function isPrivateNonLoopback(host: string): boolean { + if (host.endsWith(".local")) return true; + const ipv4 = parseIpv4(host); + if (ipv4 !== null) { + // 0.0.0.0/8, 10.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16 + if ((ipv4 & 0xff000000) === 0x00000000) return true; + if ((ipv4 & 0xff000000) === 0x0a000000) return true; + if ((ipv4 & 0xffff0000) === 0xa9fe0000) return true; + if ((ipv4 & 0xfff00000) === 0xac100000) return true; + if ((ipv4 & 0xffff0000) === 0xc0a80000) return true; + } + if (host.includes(":")) { + const h = host.toLowerCase(); + if (h === "::") return true; + // Unique local (fc00::/7) and link-local (fe80::/10). + if ( + h.startsWith("fc") || + h.startsWith("fd") || + h.startsWith("fe8") || + h.startsWith("fe9") || + h.startsWith("fea") || + h.startsWith("feb") + ) { + return true; + } + } + return false; +} + +/** Hostname of APP_URL, if set and parseable — the one private origin we allow. */ +function appUrlHost(): string | null { + const raw = process.env.APP_URL?.trim(); + if (!raw) return null; + try { + return new URL(raw).hostname.replace(/^\[|\]$/g, "").toLowerCase(); + } catch { + return null; + } +} + +export type PublicUrlCheck = + | { ok: true; url: URL } + | { ok: false; reason: string }; + +/** + * Parse `raw` and report whether a worker may navigate to it. + */ +export function checkPublicHttpUrl(raw: string): PublicUrlCheck { + let url: URL; + try { + url = new URL(raw); + } catch { + return { ok: false, reason: "not a valid URL" }; + } + + if (url.protocol !== "http:" && url.protocol !== "https:") { + return { ok: false, reason: `scheme "${url.protocol}" is not allowed` }; + } + + const host = url.hostname.replace(/^\[|\]$/g, "").toLowerCase(); + if (!host) return { ok: false, reason: "URL has no hostname" }; + + // Cloud instance metadata — never reachable, even via APP_URL. + if (METADATA_HOSTNAMES.has(host) || host === "169.254.169.254") { + return { ok: false, reason: `host "${host}" is not reachable from Ghost` }; + } + + if (isLoopbackHost(host)) { + return { ok: true, url }; + } + + if (isPrivateNonLoopback(host)) { + const allowed = appUrlHost(); + if (allowed && host === allowed) { + return { ok: true, url }; + } + return { + ok: false, + reason: `host "${host}" is a private or link-local address`, + }; + } + + return { ok: true, url }; +} + +/** Throws with a short message when `raw` is not an allowed http(s) URL. */ +export function assertPublicHttpUrl(raw: string): URL { + const result = checkPublicHttpUrl(raw); + if (!result.ok) throw new Error(`refusing to navigate: ${result.reason}`); + return result.url; +} diff --git a/cloud/packages/core/src/schema/step.ts b/cloud/packages/core/src/schema/step.ts index ed8b403e..458b448f 100644 --- a/cloud/packages/core/src/schema/step.ts +++ b/cloud/packages/core/src/schema/step.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { checkPublicHttpUrl } from "../net/public-url.js"; /** * The workflow step schema. @@ -121,12 +122,22 @@ const base = { }; -export const navigateStep = z.object({ - ...base, - type: z.literal("navigate"), - url: z.string().url(), - verify: verificationSchema.optional(), -}); +export const navigateStep = z + .object({ + ...base, + type: z.literal("navigate"), + url: z.string().url(), + verify: verificationSchema.optional(), + }) + .superRefine((step, ctx) => { + const result = checkPublicHttpUrl(step.url); + if (result.ok) return; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["url"], + message: result.reason, + }); + }); export const clickStep = z.object({ ...base, diff --git a/cloud/packages/core/turbo.json b/cloud/packages/core/turbo.json index 790b3c4d..b8b9630a 100644 --- a/cloud/packages/core/turbo.json +++ b/cloud/packages/core/turbo.json @@ -4,6 +4,9 @@ "build": { "dependsOn": ["^build"], "outputs": [] + }, + "test": { + "dependsOn": ["build"] } } } diff --git a/cloud/scripts/require-test-env.mjs b/cloud/scripts/require-test-env.mjs new file mode 100644 index 00000000..8e5e2167 --- /dev/null +++ b/cloud/scripts/require-test-env.mjs @@ -0,0 +1,36 @@ +#!/usr/bin/env node +/** + * Fail loudly when the env that DB-gated tests need is missing. + * + * Without this, `pnpm test` exits 0 while ~100 tests — every test of the + * run / approval / verify loop — silently skip via `describe.skipIf(!hasDb)`. + * That is a green signal covering nothing of the safety-critical half of the + * product. CI always sets these; a local developer who has not started + * Postgres/Redis should see a clear refusal rather than a false pass. + * + * Escape hatch: `GHOST_ALLOW_SKIP_DB_TESTS=1` restores the old skip behaviour + * for hermetic unit-only iteration. Prefer `pnpm demo` / docker compose. + */ + +const required = ["DATABASE_URL", "REDIS_URL", "GHOST_SESSION_KEY"]; +const missing = required.filter((k) => !process.env[k]?.trim()); + +if (missing.length === 0) process.exit(0); + +if (process.env.GHOST_ALLOW_SKIP_DB_TESTS === "1") { + console.warn( + `[ghost] GHOST_ALLOW_SKIP_DB_TESTS=1 — continuing without ${missing.join(", ")}; DB-gated tests will skip.`, + ); + process.exit(0); +} + +console.error( + `[ghost] pnpm test requires ${missing.join(", ")} so the run/approval/verify suite actually executes.`, +); +console.error( + " Start Postgres + Redis (`pnpm demo` or `docker compose up -d`), then copy cloud/.env.example → cloud/.env.", +); +console.error( + " To run unit-only tests and knowingly skip the DB suite: GHOST_ALLOW_SKIP_DB_TESTS=1 pnpm test", +); +process.exit(1); diff --git a/cloud/turbo.json b/cloud/turbo.json index c84ac097..e5ab1eb9 100644 --- a/cloud/turbo.json +++ b/cloud/turbo.json @@ -17,6 +17,8 @@ "GHOST_MFA_KEY", "HR_API_KEY", "GHOST_STEP_TIMEOUT_MS", + "GHOST_RUN_TIMEOUT_MS", + "GHOST_ALLOW_SKIP_DB_TESTS", "WORKER_CONCURRENCY", "APP_URL", "NODE_ENV", From 28f0c39c54a701338675960e767546d5943f8c57 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 06:45:48 +0000 Subject: [PATCH 2/3] docs(cloud): clarify P1-6 loopback exception in audit status Co-authored-by: Muhammad Rafiq --- cloud/docs/ARCHITECTURE_DECISIONS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloud/docs/ARCHITECTURE_DECISIONS.md b/cloud/docs/ARCHITECTURE_DECISIONS.md index f1a4a024..bfeedce6 100644 --- a/cloud/docs/ARCHITECTURE_DECISIONS.md +++ b/cloud/docs/ARCHITECTURE_DECISIONS.md @@ -343,7 +343,7 @@ plus a small number of real defects. Ordered by business impact. | 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-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` denylist (private/link-local/metadata) enforced at schema author time and again in `applyStep`. `--no-sandbox` and DNS-rebinding still open. | +| 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. | | P1-9 | Concurrency is per-workflow only; the queue is a single global FIFO with no per-org fairness. | One tenant starves all others. No per-tenant SLA is possible. | Open — product/capacity model. | From 52d6f4317e39e7a2bc8c4b9cce99801e3718fedf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 06:49:20 +0000 Subject: [PATCH 3/3] fix(cloud): repair navigate URL policy typecheck and fixture tests Keep navigateStep as a ZodObject so the discriminated union stays sound; enforce public-URL checks in authoredSteps + applyStep. Fix unsigned IPv4 bitmask compares, serve driver fixtures over loopback HTTP, and type the Auth.js rate-limit wrapper with NextRequest. Co-authored-by: Muhammad Rafiq --- .../src/app/api/auth/[...nextauth]/route.ts | 9 ++-- cloud/apps/web/src/lib/workflow-input.ts | 40 ++++++++++++++--- cloud/apps/worker/src/browser/driver.test.ts | 43 ++++++++++++++++--- cloud/packages/core/src/net/public-url.ts | 13 +++--- cloud/packages/core/src/schema/step.ts | 23 +++------- 5 files changed, 90 insertions(+), 38 deletions(-) diff --git a/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts b/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts index 44dc1a30..f17d85c2 100644 --- a/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts +++ b/cloud/apps/web/src/app/api/auth/[...nextauth]/route.ts @@ -1,3 +1,4 @@ +import type { NextRequest } from "next/server"; import { handlers } from "@/auth"; import { clientKey, rateLimit, tooManyRequests } from "@/lib/rate-limit"; @@ -19,8 +20,8 @@ import { clientKey, rateLimit, tooManyRequests } from "@/lib/rate-limit"; */ async function limited( - req: Request, - handle: (req: Request) => Promise, + req: NextRequest, + handle: (req: NextRequest) => Promise, { limit, windowSeconds, key }: { limit: number; windowSeconds: number; key: string }, ): Promise { const result = await rateLimit(`${key}:${clientKey(req)}`, { limit, windowSeconds }); @@ -28,10 +29,10 @@ async function limited( return handle(req); } -export async function GET(req: Request): Promise { +export async function GET(req: NextRequest): Promise { return limited(req, handlers.GET, { limit: 120, windowSeconds: 60, key: "auth:get" }); } -export async function POST(req: Request): Promise { +export async function POST(req: NextRequest): Promise { return limited(req, handlers.POST, { limit: 20, windowSeconds: 60, key: "auth:post" }); } diff --git a/cloud/apps/web/src/lib/workflow-input.ts b/cloud/apps/web/src/lib/workflow-input.ts index bf935554..af78a31c 100644 --- a/cloud/apps/web/src/lib/workflow-input.ts +++ b/cloud/apps/web/src/lib/workflow-input.ts @@ -1,5 +1,6 @@ import { z } from "zod"; import { EDITABLE_STEP_TYPES, workflowSteps } from "@ghost/core/schema/step"; +import { checkPublicHttpUrl } from "@ghost/core/net/public-url"; /** * Validation for authored workflow definitions arriving over HTTP. @@ -17,6 +18,9 @@ import { EDITABLE_STEP_TYPES, workflowSteps } from "@ghost/core/schema/step"; * gate but perform nothing — a run containing one sails past it and reports * SUCCEEDED having skipped the step. The editor does not offer them; this stops * a hand-rolled POST from introducing one anyway. + * + * Navigate URLs are also checked here (and again in the worker) so a private + * or metadata host never lands in a version — see `@ghost/core/net/public-url`. */ const editableTypes = new Set(EDITABLE_STEP_TYPES); @@ -24,12 +28,36 @@ export const authoredSteps = workflowSteps .min(1, "a workflow needs at least one step") .superRefine((steps, ctx) => { steps.forEach((step, i) => { - if (editableTypes.has(step.type)) return; - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: [i, "type"], - message: `step type "${step.type}" has no executor yet and cannot be saved`, - }); + if (!editableTypes.has(step.type)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [i, "type"], + message: `step type "${step.type}" has no executor yet and cannot be saved`, + }); + } + if (step.type === "navigate") { + const result = checkPublicHttpUrl(step.url); + if (!result.ok) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [i, "url"], + message: result.reason, + }); + } + } + if (step.compensate) { + step.compensate.actions.forEach((action, j) => { + if (action.type !== "navigate") return; + const result = checkPublicHttpUrl(action.url); + if (!result.ok) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [i, "compensate", "actions", j, "url"], + message: result.reason, + }); + } + }); + } }); }); diff --git a/cloud/apps/worker/src/browser/driver.test.ts b/cloud/apps/worker/src/browser/driver.test.ts index fe488405..c8d62e4f 100644 --- a/cloud/apps/worker/src/browser/driver.test.ts +++ b/cloud/apps/worker/src/browser/driver.test.ts @@ -1,6 +1,6 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { readFileSync } from "node:fs"; -import { createServer } from "node:http"; +import { createServer, type Server } from "node:http"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import type { WorkflowStep } from "@ghost/core/schema/step"; @@ -14,18 +14,32 @@ import { } from "./driver.js"; /** - * Hermetic integration test: drives real Chromium against a local file fixture. - * No database, no network — exercises selector resolution, actions, screenshots, - * and verification end to end. + * Hermetic integration test: drives real Chromium against a local HTTP fixture. + * No database — exercises selector resolution, actions, screenshots, and + * verification end to end. Served over loopback http (not `file:`) so the + * navigate URL policy matches production. */ const here = dirname(fileURLToPath(import.meta.url)); -const fixtureUrl = "file://" + join(here, "__fixtures__", "form.html"); +const fixtureHtml = readFileSync(join(here, "__fixtures__", "form.html"), "utf8"); const OPTS = { timeoutMs: 15_000 }; let session: BrowserSession; +let server: Server; +let fixtureUrl: string; beforeAll(async () => { + server = createServer((_req, res) => { + res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); + res.end(fixtureHtml); + }); + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => resolve()); + }); + const addr = server.address(); + if (!addr || typeof addr === "string") throw new Error("no listen address"); + fixtureUrl = `http://127.0.0.1:${addr.port}/`; + // No browser-path setup: `launchOptions` resolves a preinstalled Chromium // itself. That used to live here, which is exactly why the worker shipped // without it. @@ -34,6 +48,9 @@ beforeAll(async () => { afterAll(async () => { await session?.close(); + await new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); }); async function submitCount(page: BrowserSession["page"]): Promise { @@ -57,7 +74,21 @@ describe("Playwright driver", () => { } satisfies WorkflowStep, OPTS, ); - expect(fill.screenshot.length).toBeGreaterThan(0); + expect(fill.screenshot).not.toBeNull(); + expect(fill.screenshot!.length).toBeGreaterThan(0); + + const sensitive = await runStep( + page, + { + id: "2b", + type: "fill", + selector: { role: "textbox", name: "Full name" }, + value: "4111111111111111", + sensitive: true, + } satisfies WorkflowStep, + OPTS, + ); + expect(sensitive.screenshot).toBeNull(); await runStep( page, diff --git a/cloud/packages/core/src/net/public-url.ts b/cloud/packages/core/src/net/public-url.ts index ede3a19b..63f78dff 100644 --- a/cloud/packages/core/src/net/public-url.ts +++ b/cloud/packages/core/src/net/public-url.ts @@ -48,12 +48,15 @@ function isPrivateNonLoopback(host: string): boolean { if (host.endsWith(".local")) return true; const ipv4 = parseIpv4(host); if (ipv4 !== null) { + // Bitwise ops in JS are signed int32; `>>> 0` keeps comparisons unsigned + // so 192.168/16 and 172.16/12 (high bit set) match correctly. + const u = (mask: number, expect: number) => ((ipv4 & mask) >>> 0) === expect; // 0.0.0.0/8, 10.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16 - if ((ipv4 & 0xff000000) === 0x00000000) return true; - if ((ipv4 & 0xff000000) === 0x0a000000) return true; - if ((ipv4 & 0xffff0000) === 0xa9fe0000) return true; - if ((ipv4 & 0xfff00000) === 0xac100000) return true; - if ((ipv4 & 0xffff0000) === 0xc0a80000) return true; + if (u(0xff000000, 0x00000000)) return true; + if (u(0xff000000, 0x0a000000)) return true; + if (u(0xffff0000, 0xa9fe0000)) return true; + if (u(0xfff00000, 0xac100000)) return true; + if (u(0xffff0000, 0xc0a80000)) return true; } if (host.includes(":")) { const h = host.toLowerCase(); diff --git a/cloud/packages/core/src/schema/step.ts b/cloud/packages/core/src/schema/step.ts index 458b448f..ed8b403e 100644 --- a/cloud/packages/core/src/schema/step.ts +++ b/cloud/packages/core/src/schema/step.ts @@ -1,5 +1,4 @@ import { z } from "zod"; -import { checkPublicHttpUrl } from "../net/public-url.js"; /** * The workflow step schema. @@ -122,22 +121,12 @@ const base = { }; -export const navigateStep = z - .object({ - ...base, - type: z.literal("navigate"), - url: z.string().url(), - verify: verificationSchema.optional(), - }) - .superRefine((step, ctx) => { - const result = checkPublicHttpUrl(step.url); - if (result.ok) return; - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: ["url"], - message: result.reason, - }); - }); +export const navigateStep = z.object({ + ...base, + type: z.literal("navigate"), + url: z.string().url(), + verify: verificationSchema.optional(), +}); export const clickStep = z.object({ ...base,