fix(cloud): RBAC matrix, sensitive-fill hygiene, audit verify cap - #425
Conversation
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>
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed77f13f1
ℹ️ 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".
| if (approve) { | ||
| const actor = await loadActor(orgId, userId); |
There was a problem hiding this comment.
When an administrator removes a user from the organization, that user's JWT can remain valid for up to 12 hours, but loadActor runs only for approve. The removed user can therefore still submit decision: "reject" for a known pending gate, marking a forward run FAILED or rejecting compensation even though they are no longer the “member” this policy intends to authorize. Load and require a current actor for both decisions, then apply the OWNER/ADMIN capability check only to approval.
AGENTS.md reference: AGENTS.md:L101-L112
Useful? React with 👍 / 👎.
| where: { runId }, | ||
| orderBy: { seq: "asc" }, | ||
| }); |
There was a problem hiding this comment.
When runId refers to a journal larger than GHOST_AUDIT_VERIFY_MAX_EVENTS, this query materializes the entire journal before checking runEvents.length and returning 413. A run can have far more journal events than the organization audit-event count used by the earlier guard, so an authenticated full-verify request can still consume the web tier's memory and CPU—the failure this cap is meant to prevent. Count first or query at most cap + 1 rows before attempting verification.
Useful? React with 👍 / 👎.
| const orgRow = await prisma.organization.findUniqueOrThrow({ | ||
| where: { id: orgId }, | ||
| select: { auditChainHead: true }, | ||
| }); |
There was a problem hiding this comment.
When an audit event is appended between this organization-head read and the later tail query, the statements observe different snapshots: orgRow.auditChainHead can be the old head while tail.hash is the newly committed head, causing an intact chain to be returned as intact: false. The audit page has the same race because its head and tail queries run separately. Read both values in a repeatable-read transaction or otherwise from one database snapshot so ordinary concurrent activity cannot produce a false tampering alarm.
AGENTS.md reference: AGENTS.md:L19-L25
Useful? React with 👍 / 👎.
| if (runId) { | ||
| const runHead = await verifyRunHead(orgId, runId); | ||
| if (!runHead) return NextResponse.json({ error: "not found" }, { status: 404 }); | ||
| body.run = runHead; |
There was a problem hiding this comment.
When the terminal run timeline calls /api/audit/verify?runId=... without an explicit mode, this assignment returns verifyRunHead, which omits the previous anchored and anchorMatches checks. The UI therefore labels the journal “intact” solely because its tail matches the mutable Run.journalHead and silently suppresses the anchor status; rewriting both the journal and that column now passes even when the independently recorded organization-ledger anchor disagrees. Retain the anchor comparison in head mode or have this integrity UI request full verification.
AGENTS.md reference: AGENTS.md:L19-L25
Useful? React with 👍 / 👎.
| const actor = await loadActor(orgId, userId); | ||
| if (!actor || !canPublishWorkflow(actor.role)) { | ||
| return Response.json({ error: "forbidden" }, { status: 403 }); |
There was a problem hiding this comment.
When a MEMBER uses the existing workflow UI, the unchanged /workflows, /workflows/new, and workflow-detail pages still expose Create demo, New workflow, Publish, concurrency, and separate-approver controls even though these new checks reject every corresponding request with 403. In particular, CreateDemoButton swallows the failure and appears to do nothing, while the other controls let members edit before failing at save. Read the membership role in these server pages and hide or disable the admin-only controls while leaving Run available.
Useful? React with 👍 / 👎.
| | 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. | |
There was a problem hiding this comment.
This change alters cloud API and trust behavior—the publish/approval role matrix, audit verification modes and limits, and sensitive-screenshot handling—but only updates cloud/docs/ARCHITECTURE_DECISIONS.md. The matching canonical docs/trust-pipeline.md still describes approval and verification without these authorization and integrity boundaries, even though repository instructions require cloud API/job/trust behavior changes to update both cloud docs and the trust-pipeline document in the same change.
AGENTS.md reference: AGENTS.md:L223-L228
Useful? React with 👍 / 👎.
2 participants
Summary
Follow-up to #424. Closes the remaining safe P1 leftovers from
cloud/docs/ARCHITECTURE_DECISIONS.mdwithout waiting on connectors (P1-3) or VIEWER/APPROVER schema work.Trust & safety
Touches files / filesystem mutation: none
Touches OS input: none
Touches screenshots / screen contents: worker skips screenshots for sensitive fills (
fill.sensitiveorclassifyStepgate); editor masks sensitive fill values withtype="password"Touches network: none new
Touches authentication / secrets: role gates on publish/create/approve/start; run timeline no longer ships extract cleartext (
output: null)Touches app / window state: none
Risky actions remain deny-by-default (approve/publish admin-only; reject stays open)
No silent delete or silent overwrite
New Tauri commands have a module and a risk class — N/A (cloud only)
Experimental features stay gated — N/A
Changes
canPublishWorkflow/canApproveRun→ OWNER/ADMIN;canStartRun→ any member; wired into create/publish/demo/settings PATCH, approve route,POST /api/runs, andbuildRunView.canApproveshouldCaptureScreenshotalso consultsclassifyStep; unit tests addedoutput: null(journal still holds values for{{ }}templating)/api/audit/verifydefaults tomode=head;mode=fullcapped byGHOST_AUDIT_VERIFY_MAX_EVENTS(default 50k → 413); audit page uses expected-head check.env.example/turbo.jsonupdatedValidation
Tip SHA:
5ed77f1pnpm typecheck(cloud) — passpnpm test(cloud) — pass (worker 124; full suite green under Postgres/Redis)pnpm build(cloud) — passLegacy desktop
cargo/makechecks intentionally not run (cloud-only change).Risks / follow-up
Still open (need design / larger PR):
--no-sandbox