From b6736bdc6efb7cdacc7d5c71735147c29ddf635c Mon Sep 17 00:00:00 2001 From: nmcitra Date: Mon, 3 Aug 2026 20:41:35 -0600 Subject: [PATCH] fix(workflow): validate approver spec at definition time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RequestApproval::from` documented a `"@release-manager"` role syntax that `check_approver_spec` has never accepted. A definition written to the documented form saved without complaint and failed only when a human tried to approve, while the forms that do work — `"any"` and a bare pubkey — went undocumented. Following the docs produced a gate that fails closed; reaching for the form that works produced a gate any member of the community can close. Add `ApproverSpec` and `parse_approver_spec` to buzz-workflow as the single definition of a valid spec. `WorkflowDef::validate` now parses every `request_approval` step through it, so a spec the relay cannot enforce is rejected at save rather than at approval time. The relay's `check_approver_spec` delegates to the same function instead of re-implementing the rule, so the two layers cannot drift apart again. The relay keeps its rejection path: rows written before this validation existed may still hold a spec that no longer parses, and those must continue to fail closed. Two existing fixtures used the role syntax and now use an enforceable spec. Fixes #2878 Signed-off-by: nmcitra --- .../src/handlers/command_executor.rs | 93 ++++++++--- crates/buzz-workflow/src/schema.rs | 144 +++++++++++++++++- 2 files changed, 210 insertions(+), 27 deletions(-) diff --git a/crates/buzz-relay/src/handlers/command_executor.rs b/crates/buzz-relay/src/handlers/command_executor.rs index 2d82736807..3791b9be54 100644 --- a/crates/buzz-relay/src/handlers/command_executor.rs +++ b/crates/buzz-relay/src/handlers/command_executor.rs @@ -22,6 +22,7 @@ use buzz_core::tenant::{CommunityId, TenantContext}; use buzz_db::workflow::{ApprovalStatus, RunStatus}; use buzz_db::DbError; use buzz_workflow::executor::TriggerContext; +use buzz_workflow::schema::{parse_approver_spec, ApproverSpec}; use crate::state::AppState; use crate::webhook_secret; @@ -996,34 +997,26 @@ async fn handle_workflow_trigger( /// Enforce the approver_spec field against the requesting pubkey. /// -/// Accepted specs: -/// - `""` or `"any"` — any authenticated user may approve. -/// - 64-char lowercase hex string — only that exact pubkey may approve. +/// The accepted forms are defined by [`parse_approver_spec`] in `buzz-workflow`, +/// which also runs at definition-validation time. Both paths share one rule, so +/// a stored definition cannot carry a spec this function would reject. /// -/// All other formats are rejected (fail-closed). +/// Unparseable specs fail closed. Rows written before validation existed may +/// still hold one, so this arm stays reachable. fn check_approver_spec(approver_spec: &str, requester_hex: &str) -> Result<(), IngestError> { - let spec = approver_spec.trim(); - - // Empty or "any" — anyone may approve - if spec.is_empty() || spec == "any" { - return Ok(()); - } - - // Exact pubkey match (64-char hex, case-insensitive) - if spec.len() == 64 && spec.chars().all(|c| c.is_ascii_hexdigit()) { - if requester_hex.to_lowercase() == spec.to_lowercase() { - return Ok(()); + match parse_approver_spec(approver_spec) { + Ok(ApproverSpec::Anyone) => Ok(()), + Ok(ApproverSpec::Pubkey(pubkey)) => { + if requester_hex.to_lowercase() == pubkey { + Ok(()) + } else { + Err(IngestError::Rejected( + "forbidden: not the designated approver for this request".into(), + )) + } } - return Err(IngestError::Rejected( - "forbidden: not the designated approver for this request".into(), - )); + Err(e) => Err(IngestError::Rejected(format!("forbidden: {e}"))), } - - // Role-based or unrecognised — fail closed - Err(IngestError::Rejected(format!( - "forbidden: approver spec '{}' is not yet supported", - spec - ))) } async fn handle_approval_grant( @@ -1368,3 +1361,55 @@ async fn resume_workflow_after_approval( .finalize_run(community_id, run_id, result, existing_trace) .await; } + +#[cfg(test)] +mod tests { + use super::*; + + const APPROVER: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const OTHER: &str = "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210"; + + #[test] + fn any_spec_admits_any_requester() { + assert!(check_approver_spec("any", OTHER).is_ok()); + assert!(check_approver_spec("", OTHER).is_ok()); + assert!(check_approver_spec(" ", OTHER).is_ok()); + } + + #[test] + fn pubkey_spec_admits_only_that_pubkey() { + assert!(check_approver_spec(APPROVER, APPROVER).is_ok()); + assert!(check_approver_spec(&APPROVER.to_uppercase(), APPROVER).is_ok()); + assert!(check_approver_spec(APPROVER, OTHER).is_err()); + } + + #[test] + fn unenforceable_spec_fails_closed() { + // Rows predating definition-time validation may still hold these. + for spec in ["@release-manager", "release-manager", &APPROVER[..63]] { + assert!( + check_approver_spec(spec, APPROVER).is_err(), + "spec {spec:?} must not authorize anyone" + ); + } + } + + #[test] + fn enforcement_agrees_with_definition_validation() { + // The cross-boundary invariant: any spec that survives definition + // validation is a spec this handler can enforce. Before the shared + // parser the two disagreed, and "@release-manager" saved clean then + // failed at approval time — block/buzz#2878. + for spec in ["any", "", APPROVER] { + assert!( + parse_approver_spec(spec).is_ok(), + "spec {spec:?} should validate at definition time" + ); + assert!( + check_approver_spec(spec, APPROVER).is_ok() + || check_approver_spec(spec, OTHER).is_ok(), + "spec {spec:?} validates, so some requester must be able to approve it" + ); + } + } +} diff --git a/crates/buzz-workflow/src/schema.rs b/crates/buzz-workflow/src/schema.rs index 9bc79aa48b..c30804e069 100644 --- a/crates/buzz-workflow/src/schema.rs +++ b/crates/buzz-workflow/src/schema.rs @@ -131,7 +131,12 @@ pub enum ActionDef { }, /// Suspend execution and request approval. RequestApproval { - /// User mention or role (e.g. `"@release-manager"`). + /// Who may approve. Either `"any"` (any authenticated member of the + /// community) or a 64-character hex pubkey designating a single + /// approver. An empty string is treated as `"any"`. + /// + /// Role mentions such as `"@release-manager"` are not supported and are + /// rejected by [`WorkflowDef::validate`]. See [`parse_approver_spec`]. from: String, /// Message shown to the approver. message: String, @@ -203,6 +208,18 @@ impl WorkflowDef { step.id ))); } + + // An approver spec the relay cannot enforce must not be storable. + // Without this check the definition saves clean and fails only when + // a human tries to approve, which is the worst place to find out. + if let ActionDef::RequestApproval { from, .. } = &step.action { + parse_approver_spec(from).map_err(|e| match e { + WorkflowError::InvalidDefinition(msg) => WorkflowError::InvalidDefinition( + format!("step '{}': {}", step.id, msg), + ), + other => other, + })?; + } } if let TriggerDef::Schedule { cron, interval } = &self.trigger { @@ -242,6 +259,60 @@ impl WorkflowDef { } } +/// Who may approve a `request_approval` step. +/// +/// This type and [`parse_approver_spec`] are the single definition of an +/// approver specification. Definition-time validation and the relay's +/// approve/deny path both parse through them, so a definition that saves is a +/// definition the relay can enforce. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ApproverSpec { + /// Any authenticated member of the community may approve. + /// + /// Produced by `"any"` and by the empty string. The empty form is accepted + /// because definitions already stored may omit `from`. + Anyone, + /// Only the named pubkey may approve. Always lowercase hex. + Pubkey(String), +} + +/// Parse an approver specification. +/// +/// Accepted forms: +/// +/// - `""` or whitespace — anyone may approve +/// - `"any"` — anyone may approve +/// - a 64-character hex pubkey — only that key may approve +/// +/// Everything else is rejected, including the `"@role"` mention syntax. Role +/// approval has no membership lookup behind it; accepting the syntax here would +/// let a definition save carrying a gate that cannot be enforced. +pub fn parse_approver_spec(spec: &str) -> Result { + let trimmed = spec.trim(); + + if trimmed.is_empty() || trimmed == "any" { + return Ok(ApproverSpec::Anyone); + } + + if trimmed.len() == 64 && trimmed.chars().all(|c| c.is_ascii_hexdigit()) { + return Ok(ApproverSpec::Pubkey(trimmed.to_lowercase())); + } + + if let Some(role) = trimmed.strip_prefix('@') { + return Err(WorkflowError::InvalidDefinition(format!( + "approver '@{role}' uses role-mention syntax, which is not supported. \ + Use a 64-character hex pubkey to designate a single approver, or 'any' \ + to let any member of the community approve" + ))); + } + + Err(WorkflowError::InvalidDefinition(format!( + "approver '{trimmed}' is not a recognised approver spec. \ + Use a 64-character hex pubkey to designate a single approver, or 'any' \ + to let any member of the community approve" + ))) +} + /// Validate a cron expression using the `cron` crate. /// /// The `cron` crate requires 7 fields: `sec min hour dom month dow year`. @@ -357,7 +428,7 @@ mod tests { " - id: topic\n action: set_channel_topic\n topic: Status active\n", " - id: react\n action: add_reaction\n emoji: white_check_mark\n", " - id: hook\n action: call_webhook\n url: https://hooks.example.com/notify\n method: POST\n", - " - id: approve\n action: request_approval\n from: '@manager'\n message: Approve?\n timeout: 4h\n", + " - id: approve\n action: request_approval\n from: 'any'\n message: Approve?\n timeout: 4h\n", " - id: wait\n action: delay\n duration: 5m\n", ); let (def, _) = parse_yaml(yaml).expect("parse failed"); @@ -393,7 +464,10 @@ mod tests { "name: Deploy Approval\n", "trigger:\n on: webhook\n", "steps:\n", - " - id: request\n action: request_approval\n from: '@engineering-lead'\n", + // Was '@engineering-lead' — role mentions never reached an + // enforceable spec, so the example now designates a pubkey. + " - id: request\n action: request_approval\n", + " from: '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'\n", " message: Approve deploy?\n timeout: 4h\n", " - id: notify_approved\n if: 'steps_request_output_approved == true'\n", " action: send_message\n text: Deploy approved\n", @@ -870,6 +944,70 @@ mod tests { assert!(parse_yaml(yaml).is_ok(), "30m interval should be valid"); } + /// Build a workflow whose single step requests approval from `from`. + fn approval_yaml(from: &str) -> String { + format!( + "name: Release\ntrigger:\n on: message_posted\nsteps:\n - id: gate\n action: request_approval\n from: '{from}'\n message: 'Ship it?'\n" + ) + } + + const PUBKEY: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + + #[test] + fn validate_rejects_role_mention_approver() { + // The syntax the schema used to document. It saved clean and failed + // only when a human tried to approve — see block/buzz#2878. + let err = parse_yaml(&approval_yaml("@release-manager")).unwrap_err(); + let WorkflowError::InvalidDefinition(msg) = err else { + panic!("expected InvalidDefinition"); + }; + assert!(msg.contains("gate"), "message should name the step: {msg}"); + assert!( + msg.contains("role-mention"), + "message should explain the rejection: {msg}" + ); + } + + #[test] + fn validate_rejects_unrecognised_approver() { + let err = parse_yaml(&approval_yaml("release-manager")).unwrap_err(); + assert!(matches!(err, WorkflowError::InvalidDefinition(_))); + } + + #[test] + fn validate_rejects_short_hex_approver() { + // 63 characters — one shy of a pubkey, and previously accepted by + // neither path while saving without complaint. + let err = parse_yaml(&approval_yaml(&PUBKEY[..63])).unwrap_err(); + assert!(matches!(err, WorkflowError::InvalidDefinition(_))); + } + + #[test] + fn validate_accepts_any_and_pubkey_approvers() { + assert!(parse_yaml(&approval_yaml("any")).is_ok(), "'any' is valid"); + assert!( + parse_yaml(&approval_yaml(PUBKEY)).is_ok(), + "a 64-char hex pubkey is valid" + ); + } + + #[test] + fn parse_approver_spec_maps_each_accepted_form() { + assert_eq!(parse_approver_spec("").unwrap(), ApproverSpec::Anyone); + assert_eq!(parse_approver_spec(" ").unwrap(), ApproverSpec::Anyone); + assert_eq!(parse_approver_spec("any").unwrap(), ApproverSpec::Anyone); + assert_eq!( + parse_approver_spec(&PUBKEY.to_uppercase()).unwrap(), + ApproverSpec::Pubkey(PUBKEY.to_owned()), + "hex is normalised to lowercase so comparison is case-insensitive" + ); + assert_eq!( + parse_approver_spec(&format!(" {PUBKEY} ")).unwrap(), + ApproverSpec::Pubkey(PUBKEY.to_owned()), + "surrounding whitespace is trimmed" + ); + } + #[test] fn diff_posted_trigger_roundtrips_yaml() { let yaml = "on: diff_posted\n";