Skip to content

fix(relay): descriptive SEC-006 denial + structured skip log for workflow triggers - #5135

Open
iroiro147 wants to merge 2 commits into
block:mainfrom
iroiro147:fix/5122-descriptive-workflow-auth-errors
Open

fix(relay): descriptive SEC-006 denial + structured skip log for workflow triggers#5135
iroiro147 wants to merge 2 commits into
block:mainfrom
iroiro147:fix/5122-descriptive-workflow-auth-errors

Conversation

@iroiro147

Copy link
Copy Markdown
Contributor

Resolves #5122 (partial).

Root cause analysis

Issue #5122 reports that call_webhook workflow steps never deliver HTTP requests, with no error surfaced and no run record visible. The investigation identified three distinct, interlocking causes:

  1. Silent SEC-006 denial (actionable, fixed here): When a manual trigger's owner-authority check fails, the relay returned the same generic "forbidden: not authorized to trigger this workflow" for every failure mode — membership lapse, disabled workflow, SEC-006 elevated-role denial — leaving callers unable to distinguish "you lack the admin role for call_webhook" from "workflow disabled." On the event-trigger path, the skip was only a WARN log with the workflow_id and error, no owner_pubkey or requires_elevated flag.

  2. Run history unreadable (known, separately filed as Workflow run history is unreadable: always returns [] #2980 and workflow run history is write-never: 'buzz workflows runs' queries kinds 46001-46003 but relay never publishes them #4478): buzz workflows runs queries Nostr kinds 46001–46003, but the relay never emits those events. Run state lives only in Postgres. This means even when call_webhook does fail (e.g. outbound HTTP blocked by infrastructure), the failure is invisible to the CLI. Not addressed here — it is a larger feature (event emission) tracked in its own issues.

  3. Outbound HTTP blocked by deployment (infrastructure, not code): If the relay is hosted where egress HTTP is restricted, call_webhook_impl returns Err(WorkflowError::WebhookError(...)), the run is marked Failed in Postgres, but cause Initial release — Sprout Nostr relay with enterprise extensions #2 hides the failure. Not fixable in-repo.

What this PR fixes

Split the rejection text by cause in handle_workflow_trigger so callers can act:

  • "forbidden: only the workflow owner may trigger this workflow" (wrong caller)
  • "forbidden: workflow is {status} (enabled={bool}); active runs require enabled=true and status=Active" (lifecycle)
  • "forbidden: workflow has no channel scope; cannot verify owner authority" (fail-closed)
  • SEC-006 branch: "forbidden: SEC-006 — workflow contains exfiltration-capable actions (call_webhook) that require the owner to hold the 'owner' or 'admin' role in this channel; {underlying_error}" — names the cause and the fix
  • Ordinary-denial fallback: "forbidden: workflow owner's channel authority check failed; {error}"

Also enrich the WARN log on the event-trigger path (on_eventcheck_owner_authority) with owner_pubkey and requires_elevated_authority, so the same silent skip is diagnosable from relay logs.

What this PR does NOT fix

Tests

  • cargo check -p buzz-relay -p buzz-workflow: clean
  • cargo fmt --check -p buzz-relay -p buzz-workflow: clean
  • cargo clippy -p buzz-relay -p buzz-workflow --all-targets -- -D warnings: clean
  • cargo test -p buzz-relay --lib: 854 passed, 8 pre-existing failures (api::admin / api::media, verified identical on base, unrelated)
  • cargo test -p buzz-workflow --lib: 154 passed, 0 failed
  • New unit tests: denial_for_elevated_definition_names_sec006_and_call_webhook, denial_for_ordinary_definition_has_no_sec006_hint

Files changed

  • crates/buzz-relay/src/handlers/command_executor.rs (+52/−7): descriptive rejections + extracted workflow_owner_authority_denial helper + unit tests
  • crates/buzz-workflow/src/lib.rs (+9/−2): structured WARN log with owner_pubkey + requires_elevated_authority

…flow triggers

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
block#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 <sarthak.singh@mastersunion.org>
@iroiro147
iroiro147 requested a review from a team as a code owner August 7, 2026 01:54
@Silentpartnercoding

Copy link
Copy Markdown

workflow_owner_authority_denial() now includes {error} in the caller-facing rejection. For a lookup failure, check_owner_authority() wraps the database error verbatim as owner authority lookup failed (fail-closed): {e}, so a valid workflow owner can receive internal database or connection details when the membership store is unavailable.

Could this return a stable public cause such as SEC-006 role_required or authority_lookup_failed, while logging the detailed error server-side with the workflow and owner fields? A regression with a sentinel lookup error should prove the sentinel is absent from the rejection but present in the structured log. That keeps the new diagnosis useful without turning an authorization failure into an internal-error disclosure.

@iroiro147

Copy link
Copy Markdown
Contributor Author

Good catch — you're right, and the fix you describe is the right shape.

Confirmed in the diff: workflow_owner_authority_denial() interpolates the WorkflowError directly into the caller-facing string, and check_owner_authority() wraps the underlying store error verbatim (owner authority lookup failed (fail-closed): {e}). So an unavailable membership store turns an authorization denial into an internal-error disclosure. That's a real regression in what was meant to be a diagnosability improvement, and it isn't a tradeoff worth making.

I'll change it to what you proposed:

  • caller-facing rejection carries a stable public cause only — SEC-006 role_required when elevated authority is required, SEC-006 authority_lookup_failed when the check itself failed — with no interpolated error;
  • the detailed error goes to the structured log alongside the workflow and owner fields, which is where it's actually useful;
  • a regression test injects a sentinel lookup error and asserts the sentinel is absent from the rejection and present in the structured log, so this can't silently come back.

That keeps the diagnosis the PR was after while leaving the denial opaque to the caller. Pushing shortly.

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.
@iroiro147

Copy link
Copy Markdown
Contributor Author

Pushed as 95a87284.

  • workflow_owner_authority_denial() no longer takes the error at all — it returns a stable public cause: SEC-006 role_required when elevated authority is required, SEC-006 authority_lookup_failed otherwise.
  • The detail moved to a structured tracing::warn! carrying workflow_id, hex-encoded owner_pubkey, requires_elevated_authority and the error.
  • denial_never_discloses_the_underlying_store_error injects a sentinel (connection refused to members.db at 10.0.0.7:5432) and asserts it appears in neither denial, along with members.db, the host, and the lookup failed (fail-closed) wrapper text.
  • Added denial_causes_are_stable_and_distinguishable, since callers branch on the cause and the two must not collide.

cargo check -p buzz-relay is clean and all four denial tests pass locally. Thanks for catching it — the original change made the diagnosis better and the security worse, which wasn't a trade worth making.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Workflow call_webhook action never delivers — silent, no error, no run record

2 participants