audit: `Run` and `SignalWithStart` refuse a manual start under the workflow's `manual:` block after the admission ALLOW is already written, and record no DENY — the trail for a refused `manual: denied` or `allowed_principals` start reads as allowed
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Observed behavior
At 7530b29 (origin/main is b015dc6; the one commit between them touches flowtest only), Run writes its audit record at admission, pkg/flowstate/v1/server/server.go:1317, and the comment above it says why it is written there: "this RPC's authorization question is 'may this caller start work in their own namespace' and nothing between here and there can change the answer: what follows is the specification being checked, which is a question about the file rather than about the caller."
That stopped being true when manual: landed. server.go:1428 reaches a second question about the caller, v1.CheckManualStart(workflow, manualStartPrincipal(ctx), req.Msg.GetReason()), which refuses under three rules the workflow declares (trigger.proto:112-135): denied, require_reason, and allowed_principals matched against the caller's issuer-qualified principal. A refusal is return nil, connect.NewError(connect.CodePermissionDenied, err) with no auditDeny. The request leaves one record: rpc: Run, decision: ALLOW, resource kind NAMESPACE.
SignalWithStart has the same shape at lifecycle.go:834: the admission ALLOW is written at :721, CheckManualStart refuses at :834, and nothing records it. That is a third unaudited refusal on that handler beside the two authorizeSignal refusals #1883 reports.
Two things keep this out of the existing guard. TestTheAuditSeamIsNotBypassed (auditseam_test.go:88) tracks callers of authorizeRunDecision and authorizeScheduleDecision only; CheckManualStart is a function in pkg/flowstate/v1, not a server decision function, so the seam test does not see it as a decision at all. And the tests that prove the refusal (pkg/flowstate/v1/trigger_manual_test.go, flowfile/manualtrigger_test.go) assert the refusal and nothing about records.
The assessing pass drove both manual: denied and an unsatisfied allowed_principals through the handler with a recording sink and observed exactly one record each, rpc: Run / decision: ALLOW. I confirmed the control flow from source and did not re-run the probe.
Enforcement holds. The defect is evidence: an operator asking "who tried to start the break-glass rotation and was refused" finds the refused caller recorded as allowed, the same failure #1883 describes for signal delivery, on the RPC that flow run and every MCP agent reach.
Desired outcome
A refused manual start records a DENY under the same RPC name as the admission record, before the refusal is returned, coded POLICY_DENIED (the code authorizeSignal's name-level refusal already uses, audit.proto:542) with the rule field naming which of denied, require_reason, or allowed_principals refused. The admission ALLOW stays: it is a true record of the admission decision, and this is a second decision, which is the same one-record-per-decision reading #1883 takes.
The seam test should cover it. Either CheckManualStart is called only through an audited server wrapper (authorizeManualStart) that the seam test tracks the way it tracks authorizeRunDecision, or analyzeServerSource learns that v1.CheckManualStart is a decision needing a named-caller exemption. The first is smaller and matches the pattern the file already has.
The comment at server.go:1310-1316 is rewritten so it no longer asserts that nothing after the admission record can change the answer.
Acceptance criteria
- A test in the shape of
TestSignalWithStartAuthorizesCreateAndDeliverySeparatelyrunsRunwith a recording sink under each of the three rules and asserts: one ALLOW at admission, one DENY codedPOLICY_DENIEDnaming the rule, and nothing else. The same forSignalWithStartunderdeniedandallowed_principals(it has no reason field, sorequire_reasonrefuses unconditionally there and that case is asserted too). - Under
--audit-requiredwith a failing sink, the refused caller sees the sink's failure surfaced, matchingauditDeny's existing contract. TestTheAuditSeamIsNotBypassedfails if a future caller reachesCheckManualStartwithout the audited form.docs/DEPLOYMENT.md"What the trail records" lists manual-start refusals among the decisions it records.
Constraints and dependencies
- Sibling of #1883, same fix shape, different decision. Land them together if the record shape #1733's per-action design needs is decided in one place; otherwise this one is independent and smaller.
- The webhook bridge does not call
CheckManualStart(a webhook is not a manual start), so it is out of scope here. - Adjacent: #1883, #1733, #1774, #1797,
audit.go's one-record rule.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with pkg/flowstate/v1/server/server.go at Run and SignalWithStart, then read CheckManualStart and the existing auditDeny path. Run the manual-start tests, auditseam_test.go, and the recording-sink cases to understand the expected decision records. Done means the listed refusal rules produce the required DENY evidence, seam coverage passes, and docs/DEPLOYMENT.md describes manual-start refusals.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100