fix(cloud): close P0/P1 audit findings (agent auth, SSRF, reliability) - #424
Conversation
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 <mohabbis@users.noreply.github.com>
Co-authored-by: Muhammad Rafiq <mohabbis@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 <mohabbis@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
ghost/cloud/apps/worker/src/browser/driver.ts
Lines 106 to 107 in 52d6f43
When an allowed public URL responds with a redirect to 169.254.169.254 or an RFC1918 host, assertPublicHttpUrl approves only the initial URL and page.goto follows the redirect without another policy check; a subsequent extract step can therefore still read the private response. Enforce the policy on every browser request/redirect, including restore navigation, rather than only before the initial goto.
AGENTS.md reference: AGENTS.md:L203-L206
ghost/cloud/apps/worker/src/browser/driver.ts
Lines 232 to 233 in 52d6f43
When a sensitive fill is followed by any ordinary step while the value remains visible, only the fill's screenshot is skipped; the next click, waitFor, or verify calls runStep again and captures the same page, persisting the secret. The updated driver test itself fills a card number and then immediately takes a screenshot during the click, so suppression must remain active until the sensitive field is cleared, obscured, or the page changes.
AGENTS.md reference: AGENTS.md:L201-L201
ghost/cloud/apps/worker/src/jobs/runWorkflow.ts
Lines 166 to 168 in 52d6f43
When a step is still awaited at the deadline—for example, an unbounded waitFor.ms or a browser action whose timeout extends past the remaining run budget—the heartbeat stops here but the browser operation is not canceled, and the loop cannot raise RUN_TIMEOUT until that operation returns. This still allows a run to wedge the worker indefinitely and eventually lets the expired lease be reclaimed while the original executor remains active; the deadline needs to interrupt and close the in-flight browser work before lease renewal stops.
ghost/cloud/scripts/require-test-env.mjs
Lines 15 to 16 in 52d6f43
When a developer follows the emitted instruction and copies .env.example to cloud/.env (including via pnpm demo) without exporting those variables into the shell, this guard still reports all three as missing because Node does not load .env automatically and the check runs before Turbo or package-level environment loaders. Consequently the documented local pnpm test path always exits 1; load the cloud env file before checking or invoke the guard with an explicit env-file loader.
When the supported Node deployment is exposed directly or sits behind a proxy that preserves or appends a caller-supplied X-Forwarded-For, clientKey(req) trusts the first entry, so an attacker can rotate that header on every /api/auth/* POST and receive a fresh 20-attempt bucket each time. This leaves the sign-in brute-force path effectively unbounded outside proxies such as Vercel that overwrite the header; derive the key from trusted platform metadata or normalize forwarding headers at a trusted proxy boundary.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* fix(cloud): tighten RBAC, redact extract UI, and cap audit verify Close remaining safe P1 leftovers after #424: - OWNER/ADMIN-only publish/create/approve; MEMBER may start and reject - Password inputs + classifyStep-aligned screenshot skip for sensitive fills - Run timeline never ships extract cleartext to the browser - Audit verify defaults to head mode with a hard full-chain cap Co-authored-by: Muhammad Rafiq <mohabbis@users.noreply.github.com> * fix(cloud): gate run start through canStartRun Wire the role helper into POST /api/runs so future VIEWER restrictions do not need a second pass over the start path. Co-authored-by: Muhammad Rafiq <mohabbis@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Muhammad Rafiq <mohabbis@users.noreply.github.com>
Summary
Closes the actionable P0/P1 findings from
cloud/docs/ARCHITECTURE_DECISIONS.mdthat do not need a product redesign.Trust & safety
Touches files / filesystem mutation: none
Touches OS input: none
Touches screenshots / screen contents: yes — skips screenshot capture for
fillsteps markedsensitiveso OTP/card pixels never hit the artifact storeTouches network: yes —
navigateURLs are checked against a public-URL policy (blocks cloud metadata + RFC1918; loopback allowed for fixtures; other private hosts only if they matchAPP_URL)Touches authentication / secrets: yes —
/api/agent/*is bearer-only (session cookies refused); Auth.js routes are rate-limited; minting agent credentials requires OWNER/ADMINTouches app / window state: none
Risky actions remain deny-by-default
No silent delete or silent overwrite
Experimental features stay gated / out of scope
Changes
resolveAgentPrincipalrefuses session cookies — agents must use a minted API keycheckPublicHttpUrl+ author-time (authoredSteps) and runtime (applyStep) enforcementGHOST_RUN_TIMEOUT_MSwall-clock budget (default 30m); heartbeat stops renewing past deadlinescripts/require-test-env.mjsgatespnpm test;@ghost/coretestdepends onbuildARCHITECTURE_DECISIONS.mdupdated with statusStill open (need design): P1-1 checkpointed audit verify, P1-3/P1-4 secret refs + journal redaction, full P1-5 VIEWER/APPROVER roles, P1-9 per-org fairness, DNS-rebinding /
--no-sandbox.Validation
cd cloud && pnpm typecheckcd cloud && pnpm test— core 241, web 115, worker 120, mcp 1 (all passed; local Postgres+Redis)cd cloud && pnpm buildRisks / follow-up
pnpm testwithout DB now fails unlessGHOST_ALLOW_SKIP_DB_TESTS=1.APP_URL.