From 61c7141c6bcac0adb4084a955d1d207e061786a9 Mon Sep 17 00:00:00 2001 From: iroiro147 Date: Fri, 7 Aug 2026 07:22:22 +0530 Subject: [PATCH 1/2] fix(relay): descriptive SEC-006 denial + structured skip log for workflow triggers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a manual workflow trigger's owner-authority check fails, the relay previously returned the same generic 'not authorized to trigger this workflow' for every failure mode — membership lapse, disabled workflow, SEC-006 elevated-role denial — leaving callers unable to act (issue #5122's 'silent, no error, no run record'). Split the rejections by cause: * 'only the workflow owner may trigger' (wrong caller) * 'disabled or inactive' (lifecycle) * 'no channel scope' (cannot verify owner authority) * SEC-006 — 'contains exfiltration-capable actions (call_webhook) that require the owner to hold the owner or admin role in this channel' * generic fallback includes the underlying WorkflowError Also enrich the WARN emitted on event-trigger SEC-006 skip with workflow_id, owner_pubkey, and requires_elevated_authority so the same silent failure mode in the on_event path is diagnosable from relay logs alone. Regression tests cover both branches of the new denial helper — SEC-006 hint must name call_webhook + the required roles; the ordinary-denial branch must not leak SEC-006 vocabulary. Signed-off-by: iroiro147 --- .../src/handlers/command_executor.rs | 82 +++++++++++++++++-- crates/buzz-workflow/src/lib.rs | 9 +- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/crates/buzz-relay/src/handlers/command_executor.rs b/crates/buzz-relay/src/handlers/command_executor.rs index 2d82736807..4e11930e58 100644 --- a/crates/buzz-relay/src/handlers/command_executor.rs +++ b/crates/buzz-relay/src/handlers/command_executor.rs @@ -857,7 +857,7 @@ async fn handle_workflow_trigger( // member could otherwise invoke another user's webhook or message actions. if workflow.owner_pubkey != self_bytes { return Err(IngestError::Rejected( - "forbidden: not authorized to trigger this workflow".into(), + "forbidden: only the workflow owner may trigger this workflow".into(), )); } @@ -866,25 +866,34 @@ async fn handle_workflow_trigger( // Without this, a disabled workflow — including one disabled because its // owner was removed from the channel — could still be fired by the owner. if !workflow.enabled || workflow.status != buzz_db::workflow::WorkflowStatus::Active { - return Err(IngestError::Rejected( - "forbidden: workflow is disabled or inactive".into(), - )); + return Err(IngestError::Rejected(format!( + "forbidden: workflow is {} (enabled={}); \ + active runs require enabled=true and status=Active", + workflow.status, workflow.enabled + ))); } let def: buzz_workflow::WorkflowDef = serde_json::from_value(workflow.definition.clone()) .map_err(|e| IngestError::Internal(format!("error: corrupt workflow definition: {e}")))?; let Some(wf_channel_id) = workflow.channel_id else { // No channel scope means no channel authority to verify — fail closed. return Err(IngestError::Rejected( - "forbidden: workflow has no channel scope".into(), + "forbidden: workflow has no channel scope; cannot verify owner authority".into(), )); }; - state + if let Err(e) = state .workflow_engine .check_owner_authority(community_id, wf_channel_id, &workflow.owner_pubkey, &def) .await - .map_err(|_| { - IngestError::Rejected("forbidden: not authorized to trigger this workflow".into()) - })?; + { + // Surface *why* the authority check failed so the caller can act — + // previously this returned a generic "not authorized" that left the + // caller unable to distinguish a SEC-006 elevated-role denial from a + // disabled workflow or a membership lapse (issue #5122). + return Err(IngestError::Rejected(workflow_owner_authority_denial( + def.requires_elevated_authority(), + &e, + ))); + } // Persist the command event under the workflow channel even though the // trigger event itself only carries the workflow UUID. Storing channel @@ -1368,3 +1377,58 @@ async fn resume_workflow_after_approval( .finalize_run(community_id, run_id, result, existing_trace) .await; } + +/// Build the user-facing rejection text when a manual workflow trigger fails +/// its SEC-006 owner-authority check. +/// +/// Exfiltration-capable definitions (those containing a `call_webhook` step) +/// require the owner to currently hold an elevated role (`owner` or `admin`) — +/// plain membership is insufficient. Ordinary definitions only require channel +/// membership. +/// +/// Previously both branches returned the same generic "not authorized" text, +/// which left the owner of a `call_webhook` workflow unable to tell SEC-006 +/// (role missing) apart from a disabled workflow or a membership lapse (issue +/// #5122: "workflow appears to succeed, no run record"). Naming the cause in +/// the rejection gives the caller an actionable next step. +fn workflow_owner_authority_denial( + requires_elevated_authority: bool, + error: &buzz_workflow::WorkflowError, +) -> String { + if requires_elevated_authority { + format!( + "forbidden: SEC-006 — workflow contains exfiltration-capable \ + actions (call_webhook) that require the owner to hold the \ + 'owner' or 'admin' role in this channel; {error}" + ) + } else { + format!("forbidden: workflow owner's channel authority check failed; {error}") + } +} + +#[cfg(test)] +mod tests { + use super::workflow_owner_authority_denial; + use buzz_workflow::WorkflowError; + + #[test] + fn denial_for_elevated_definition_names_sec006_and_call_webhook() { + let err = WorkflowError::Unauthorized("not a member".into()); + let msg = workflow_owner_authority_denial(true, &err); + assert!(msg.starts_with("forbidden:")); + assert!(msg.contains("SEC-006")); + assert!(msg.contains("call_webhook")); + assert!(msg.contains("owner' or 'admin' role")); + assert!(msg.contains("not a member")); + } + + #[test] + fn denial_for_ordinary_definition_has_no_sec006_hint() { + let err = WorkflowError::Unauthorized("unknown".into()); + let msg = workflow_owner_authority_denial(false, &err); + assert!(msg.starts_with("forbidden:")); + assert!(!msg.contains("SEC-006")); + assert!(!msg.contains("call_webhook")); + assert!(msg.contains("unknown")); + } +} diff --git a/crates/buzz-workflow/src/lib.rs b/crates/buzz-workflow/src/lib.rs index e142221169..1108cddcf7 100644 --- a/crates/buzz-workflow/src/lib.rs +++ b/crates/buzz-workflow/src/lib.rs @@ -388,9 +388,16 @@ impl WorkflowEngine { .check_owner_authority(community_id, channel_id, &workflow.owner_pubkey, &def) .await { + // SEC-006 (exfiltration-capable definitions like call_webhook) + // denies runs without creating a run row or emitting an event — + // the only trace is this log. Include enough fields to diagnose + // "workflow appears to succeed, no run record" reports (issue + // #5122) from relay logs alone. tracing::warn!( workflow_id = %workflow.id, - "Skipping workflow — owner authority check failed: {e}" + owner_pubkey = %hex::encode(&workflow.owner_pubkey), + requires_elevated_authority = def.requires_elevated_authority(), + "Skipping workflow — SEC-006 owner authority check failed: {e}" ); continue; } From 95a872840e8d9583378ebfe88be08f3f5aeb180f Mon Sep 17 00:00:00 2001 From: iroiro147 Date: Mon, 10 Aug 2026 08:56:33 +0530 Subject: [PATCH 2/2] fix(relay): keep store errors out of the SEC-006 denial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check_owner_authority wraps store failures verbatim ("owner authority lookup failed (fail-closed): {e}"), and the denial interpolated that error into the caller-facing rejection. An unavailable membership store therefore turned an authorization denial into an internal-error disclosure, which is a worse outcome than the generic message this PR set out to improve on. The denial now carries a stable public cause only — SEC-006 role_required, or SEC-006 authority_lookup_failed — with no interpolated error, so callers can still branch on why they were denied. The detail moves to a structured warn! carrying the workflow id, hex-encoded owner pubkey and the error, which is where it is actually useful. Adds a regression test that asserts a sentinel store error appears in neither denial, plus a test that the two causes stay distinguishable. Raised in review by @Silentpartnercoding. --- .../src/handlers/command_executor.rs | 86 +++++++++++++------ 1 file changed, 60 insertions(+), 26 deletions(-) diff --git a/crates/buzz-relay/src/handlers/command_executor.rs b/crates/buzz-relay/src/handlers/command_executor.rs index 4e11930e58..86d6522632 100644 --- a/crates/buzz-relay/src/handlers/command_executor.rs +++ b/crates/buzz-relay/src/handlers/command_executor.rs @@ -885,13 +885,21 @@ async fn handle_workflow_trigger( .check_owner_authority(community_id, wf_channel_id, &workflow.owner_pubkey, &def) .await { - // Surface *why* the authority check failed so the caller can act — - // previously this returned a generic "not authorized" that left the - // caller unable to distinguish a SEC-006 elevated-role denial from a - // disabled workflow or a membership lapse (issue #5122). + // Give the caller a stable cause it can branch on (issue #5122) while + // keeping the underlying error server-side: `check_owner_authority` + // wraps store failures verbatim, so returning it would disclose + // internal database state on an authorization path. + // `owner_pubkey` is raw bytes with no Display impl; hex::encode is the + // convention used elsewhere in this file. + tracing::warn!( + %workflow_id, + owner_pubkey = %hex::encode(&workflow.owner_pubkey), + requires_elevated_authority = def.requires_elevated_authority(), + error = %e, + "workflow owner authority check failed" + ); return Err(IngestError::Rejected(workflow_owner_authority_denial( def.requires_elevated_authority(), - &e, ))); } @@ -1391,44 +1399,70 @@ async fn resume_workflow_after_approval( /// (role missing) apart from a disabled workflow or a membership lapse (issue /// #5122: "workflow appears to succeed, no run record"). Naming the cause in /// the rejection gives the caller an actionable next step. -fn workflow_owner_authority_denial( - requires_elevated_authority: bool, - error: &buzz_workflow::WorkflowError, -) -> String { +/// Caller-facing denial for a failed SEC-006 owner-authority check. +/// +/// Carries a stable public cause and never the underlying error. +/// `check_owner_authority` wraps store failures verbatim (`owner authority +/// lookup failed (fail-closed): {e}`), so interpolating it here turned an +/// authorization denial into an internal-error disclosure whenever the +/// membership store was unavailable. The detail is logged server-side. +fn workflow_owner_authority_denial(requires_elevated_authority: bool) -> String { if requires_elevated_authority { - format!( - "forbidden: SEC-006 — workflow contains exfiltration-capable \ - actions (call_webhook) that require the owner to hold the \ - 'owner' or 'admin' role in this channel; {error}" - ) + "forbidden: SEC-006 role_required — workflow contains \ + exfiltration-capable actions (call_webhook) that require the owner \ + to hold the 'owner' or 'admin' role in this channel" + .to_string() } else { - format!("forbidden: workflow owner's channel authority check failed; {error}") + "forbidden: SEC-006 authority_lookup_failed — workflow owner's \ + channel authority check failed" + .to_string() } } #[cfg(test)] mod tests { use super::workflow_owner_authority_denial; - use buzz_workflow::WorkflowError; #[test] - fn denial_for_elevated_definition_names_sec006_and_call_webhook() { - let err = WorkflowError::Unauthorized("not a member".into()); - let msg = workflow_owner_authority_denial(true, &err); + fn denial_for_elevated_definition_names_sec006_role_required() { + let msg = workflow_owner_authority_denial(true); assert!(msg.starts_with("forbidden:")); - assert!(msg.contains("SEC-006")); + assert!(msg.contains("SEC-006 role_required")); assert!(msg.contains("call_webhook")); assert!(msg.contains("owner' or 'admin' role")); - assert!(msg.contains("not a member")); } #[test] - fn denial_for_ordinary_definition_has_no_sec006_hint() { - let err = WorkflowError::Unauthorized("unknown".into()); - let msg = workflow_owner_authority_denial(false, &err); + fn denial_for_ordinary_definition_names_authority_lookup_failed() { + let msg = workflow_owner_authority_denial(false); assert!(msg.starts_with("forbidden:")); - assert!(!msg.contains("SEC-006")); + assert!(msg.contains("SEC-006 authority_lookup_failed")); assert!(!msg.contains("call_webhook")); - assert!(msg.contains("unknown")); + } + + #[test] + fn denial_never_discloses_the_underlying_store_error() { + // Regression for the review on #5135: `check_owner_authority` wraps + // store failures verbatim, so interpolating the error into the + // caller-facing denial leaks internal database state on an + // authorization path. + const SENTINEL: &str = "connection refused to members.db at 10.0.0.7:5432"; + for requires_elevated in [true, false] { + let msg = workflow_owner_authority_denial(requires_elevated); + assert!(!msg.contains(SENTINEL), "denial disclosed the store error: {msg}"); + assert!(!msg.contains("members.db")); + assert!(!msg.contains("10.0.0.7")); + assert!(!msg.contains("lookup failed (fail-closed)")); + } + } + + #[test] + fn denial_causes_are_stable_and_distinguishable() { + let elevated = workflow_owner_authority_denial(true); + let ordinary = workflow_owner_authority_denial(false); + assert_ne!(elevated, ordinary); + assert!(elevated.contains("role_required")); + assert!(ordinary.contains("authority_lookup_failed")); + assert!(!ordinary.contains("role_required")); } }