diff --git a/guards/github-guard/docs/AGENTIC_WORKFLOW_POLICY_FRONTMATTER.md b/guards/github-guard/docs/AGENTIC_WORKFLOW_POLICY_FRONTMATTER.md index 2c4b907eb..54010286d 100644 --- a/guards/github-guard/docs/AGENTIC_WORKFLOW_POLICY_FRONTMATTER.md +++ b/guards/github-guard/docs/AGENTIC_WORKFLOW_POLICY_FRONTMATTER.md @@ -8,8 +8,10 @@ Provide a small, explicit policy surface in workflow frontmatter that can expres - **Scope filtering** (what repository visibility/scope is allowed) - **Integrity floor** (minimum trust level for input content) +- **Blocked-user enforcement** (unconditionally block content from specific authors) +- **Label-based promotion** (trust labels applied by reviewers to promote content integrity) -Both controls are used to filter inputs before the agent consumes content. +All controls are used to filter inputs before the agent consumes content. --- @@ -24,6 +26,19 @@ Both controls are used to filter inputs before the agent consumes content. } ``` +With optional integrity-level management fields: + +```json +{ + "AllowOnly": { + "Repos": "Public", + "min-integrity": "approved", + "blocked-users": ["external-bot", "untrusted-fork"], + "approval-labels": ["approved", "human-reviewed"] + } +} +``` + ### Field semantics - `AllowOnly.Repos` (optional) @@ -37,6 +52,36 @@ Both controls are used to filter inputs before the agent consumes content. - `Approved` - `Merged` +- `AllowOnly.blocked-users` (optional, array of strings) + - GitHub usernames whose content items are **unconditionally blocked**, regardless of labels or min-integrity. + - Items from blocked users receive an effective integrity of `blocked` (below `none`), which is always denied by the DIFC filter. + - `approval-labels` cannot override a blocked-user exclusion. + - An empty array is equivalent to omitting the field. + +- `AllowOnly.approval-labels` (optional, array of strings) + - GitHub label names that **promote a content item's effective integrity to `approved`** when present on the item. + - If the item's computed integrity is already higher than `approved` (e.g. `merged`), it remains unchanged. + - Does not override `blocked-users`: blocked authors remain blocked even if they have an approval label. + - An empty array is equivalent to omitting the field. + +### Integrity Level Hierarchy + +``` +blocked (below none) < none < unapproved < approved < merged +``` + +### Effective Integrity Computation + +``` +1. Start with the item's base integrity level (from GitHub metadata). +2. IF the item's author is in blocked-users: + effective_integrity ← blocked (item is always denied) +3. ELSE IF any label on the item is in approval-labels: + effective_integrity ← max(base_integrity, approved) +4. ELSE: + effective_integrity ← base_integrity +``` + Rules: - `Integrity` is required for all policies. diff --git a/guards/github-guard/docs/GATEWAY_ALLOWONLY_INTEGRATION_SPEC.md b/guards/github-guard/docs/GATEWAY_ALLOWONLY_INTEGRATION_SPEC.md index f399e581b..e0617fc52 100644 --- a/guards/github-guard/docs/GATEWAY_ALLOWONLY_INTEGRATION_SPEC.md +++ b/guards/github-guard/docs/GATEWAY_ALLOWONLY_INTEGRATION_SPEC.md @@ -6,8 +6,10 @@ This document defines the gateway changes required to support: - Policy-driven DIFC input filtering via `AllowOnly` - New required policy field `Integrity` -- New integrity hierarchy with baseline `none` +- New integrity hierarchy with baseline `none` and `blocked` level below `none` - Guard-side policy initialization via a new `label_agent` interface +- Blocked-user enforcement via `blocked-users` +- Label-based integrity promotion via `approval-labels` It is written as an implementation checklist for gateway and guard integration. @@ -28,6 +30,19 @@ Gateway must accept and pass this policy shape: } ``` +With optional integrity-level management fields: + +```json +{ + "AllowOnly": { + "Repos": "Public", + "min-integrity": "approved", + "blocked-users": ["external-bot"], + "approval-labels": ["approved", "human-reviewed"] + } +} +``` + `AllowOnly.Repos` supports: - `"Public"` - `{ "owner": "" }` @@ -39,9 +54,21 @@ Gateway must accept and pass this policy shape: - `Approved` - `Merged` +`AllowOnly.blocked-users` (optional, array of strings): +- GitHub usernames unconditionally blocked regardless of labels or min-integrity. +- Items from blocked users have effective integrity `blocked` (below `none`) and are always denied. +- Cannot be overridden by `approval-labels`. + +`AllowOnly.approval-labels` (optional, array of strings): +- GitHub label names that promote an item's effective integrity to at least `approved`. +- Promotion uses `max(base_integrity, approved)` — never lowers integrity. +- Does not override `blocked-users`. + ### Integrity order -`Merged > Approveduted > Unapproveduted > none` +``` +blocked (below none) < none < unapproved < approved < merged +``` For resource/response labels, `none` is always the baseline: - public scope: `none` diff --git a/guards/github-guard/docs/agentic-workflow-policy.schema.json b/guards/github-guard/docs/agentic-workflow-policy.schema.json index c4f6378d4..c39244b8f 100644 --- a/guards/github-guard/docs/agentic-workflow-policy.schema.json +++ b/guards/github-guard/docs/agentic-workflow-policy.schema.json @@ -48,8 +48,25 @@ ] }, "min-integrity": { + "description": "Minimum integrity level required for a content item to be accessible. Items with effective integrity below this level are blocked.", "type": "string", "enum": ["none", "unapproved", "approved", "merged"] + }, + "blocked-users": { + "description": "GitHub usernames whose content items are unconditionally blocked (effective integrity = blocked, below none). Approval labels cannot override this.", + "type": "array", + "items": { + "type": "string", + "minLength": 1 + } + }, + "approval-labels": { + "description": "GitHub label names that promote a content item's effective integrity to 'approved' when present. Does not override blocked-users.", + "type": "array", + "items": { + "type": "string", + "minLength": 1 + } } } } diff --git a/guards/github-guard/rust-guard/src/labels/constants.rs b/guards/github-guard/rust-guard/src/labels/constants.rs index b52d30e0d..15e6c74de 100644 --- a/guards/github-guard/rust-guard/src/labels/constants.rs +++ b/guards/github-guard/rust-guard/src/labels/constants.rs @@ -11,6 +11,8 @@ pub mod label_constants { pub const WRITER_PREFIX: &str = "approved:"; pub const MERGED_PREFIX: &str = "merged:"; pub const NONE_PREFIX: &str = "none:"; + pub const BLOCKED_PREFIX: &str = "blocked:"; + pub const BLOCKED_BASE: &str = "blocked"; pub const READER_BASE: &str = "unapproved"; pub const WRITER_BASE: &str = "approved"; pub const MERGED_BASE: &str = "merged"; diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 69e004b72..2cf088dc0 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -55,6 +55,12 @@ pub struct PolicyContext { /// of their author_association, just like the built-in trusted first-party bots. /// This list is additive and cannot override the built-in trusted bot list. pub trusted_bots: Vec, + /// Usernames whose content items are always blocked (effective integrity = blocked). + /// Blocked items are unconditionally denied regardless of approval labels or min-integrity. + pub blocked_users: Vec, + /// GitHub label names that promote a content item's effective integrity to "approved" + /// when present on the item. Does not override blocked_users. + pub approval_labels: Vec, } fn normalize_scope(scope: &str, ctx: &PolicyContext) -> String { @@ -167,6 +173,62 @@ pub fn none_integrity(scope: &str, ctx: &PolicyContext) -> Vec { )] } +/// Generate blocked-level integrity tags for a scope. +/// +/// Items with blocked integrity are unconditionally denied by the DIFC filter +/// because no agent is ever assigned a "blocked:" tag. This represents the +/// integrity level for items authored by users in the `blocked-users` list. +pub fn blocked_integrity(scope: &str, ctx: &PolicyContext) -> Vec { + let normalized_scope = normalize_scope(scope, ctx); + vec![format_integrity_label( + label_constants::BLOCKED_PREFIX, + &normalized_scope, + label_constants::BLOCKED_BASE, + )] +} + +/// Check if a username appears in the configured blocked-users list (case-insensitive). +pub fn is_blocked_user(username: &str, ctx: &PolicyContext) -> bool { + if ctx.blocked_users.is_empty() { + return false; + } + let lower = username.to_lowercase(); + ctx.blocked_users.iter().any(|u| u.to_lowercase() == lower) +} + +/// Extract GitHub label names from a content item's `labels` array. +/// +/// Returns the `name` field from each element of the item's `labels` array. +fn extract_github_label_names<'a>(item: &'a Value) -> Vec<&'a str> { + item.get("labels") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|label| label.get("name").and_then(|v| v.as_str())) + .collect() + }) + .unwrap_or_default() +} + +/// Check whether a content item carries at least one label from the configured +/// `approval-labels` list (case-insensitive comparison). +pub fn has_approval_label(item: &Value, ctx: &PolicyContext) -> bool { + first_matching_approval_label(item, ctx).is_some() +} + +/// Return the first matching approval label name from an item, if any. +fn first_matching_approval_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> { + if ctx.approval_labels.is_empty() { + return None; + } + let label_names = extract_github_label_names(item); + label_names.into_iter().find(|name| { + ctx.approval_labels + .iter() + .any(|al| al.eq_ignore_ascii_case(name)) + }) +} + pub fn ensure_integrity_baseline( scope: &str, integrity: Vec, @@ -771,10 +833,12 @@ pub fn is_default_branch_commit_context(tool_name: &str, sha_or_ref: &str) -> bo /// Determine integrity level for a pull request /// Rules: +/// - PR authored by a blocked user => blocked-level (unconditional denial) /// - merged PR => merged-level /// - private repo PR => approved /// - public forked PR => unapproved /// - public direct PR => approved +/// - PR with an approval label => at least approved pub fn pr_integrity( item: &Value, repo_full_name: &str, @@ -782,6 +846,17 @@ pub fn pr_integrity( is_forked: Option, ctx: &PolicyContext, ) -> Vec { + // Step 1: Check if author is in blocked_users — takes precedence over all other rules. + let author_login = extract_author_login(item); + if !author_login.is_empty() && is_blocked_user(author_login, ctx) { + let number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + crate::log_info(&format!( + "[integrity] pr:{}#{} → blocked (author '{}' in blocked-users)", + repo_full_name, number, author_login + )); + return blocked_integrity(repo_full_name, ctx); + } + let mut integrity = author_association_floor(item, repo_full_name, ctx); // Check if PR is merged (either merged_at field exists or merged boolean is true) @@ -825,19 +900,49 @@ pub fn pr_integrity( ); } - ensure_integrity_baseline(repo_full_name, integrity, ctx) + let integrity = ensure_integrity_baseline(repo_full_name, integrity, ctx); + + // Step 2: Apply approval-labels promotion — raise to at least approved. + if let Some(label) = first_matching_approval_label(item, ctx) { + let number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + crate::log_info(&format!( + "[integrity] pr:{}#{} promoted to approved (label '{}' in approval-labels)", + repo_full_name, number, label + )); + max_integrity( + repo_full_name, + integrity, + writer_integrity(repo_full_name, ctx), + ctx, + ) + } else { + integrity + } } /// Determine integrity level for an issue /// Rules: +/// - Issue authored by a blocked user => blocked-level (unconditional denial) /// - private repo issues => approved /// - public repo issues => no integrity +/// - Issue with an approval label => at least approved pub fn issue_integrity( item: &Value, repo_full_name: &str, repo_private: bool, ctx: &PolicyContext, ) -> Vec { + // Step 1: Check if author is in blocked_users — takes precedence over all other rules. + let author_login = extract_author_login(item); + if !author_login.is_empty() && is_blocked_user(author_login, ctx) { + let number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + crate::log_info(&format!( + "[integrity] issue:{}#{} → blocked (author '{}' in blocked-users)", + repo_full_name, number, author_login + )); + return blocked_integrity(repo_full_name, ctx); + } + let mut integrity = author_association_floor(item, repo_full_name, ctx); if repo_private { integrity = max_integrity( @@ -847,15 +952,36 @@ pub fn issue_integrity( ctx, ); } - ensure_integrity_baseline(repo_full_name, integrity, ctx) + let integrity = ensure_integrity_baseline(repo_full_name, integrity, ctx); + + // Step 2: Apply approval-labels promotion — raise to at least approved. + if let Some(label) = first_matching_approval_label(item, ctx) { + let number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + crate::log_info(&format!( + "[integrity] issue:{}#{} promoted to approved (label '{}' in approval-labels)", + repo_full_name, number, label + )); + max_integrity( + repo_full_name, + integrity, + writer_integrity(repo_full_name, ctx), + ctx, + ) + } else { + integrity + } } /// Determine integrity level for a commit. /// /// Rules: +/// - Commit authored by a blocked user => blocked-level (unconditional denial) /// - Start from author_association floor /// - Private repo commits elevate to approved /// - Default-branch reachable commits elevate to merged +/// +/// Note: approval-labels promotion does not apply to commits because GitHub +/// commits do not carry issue/PR-style labels. pub fn commit_integrity( item: &Value, repo_full_name: &str, @@ -863,6 +989,18 @@ pub fn commit_integrity( is_default_branch: bool, ctx: &PolicyContext, ) -> Vec { + // Step 1: Check if author is in blocked_users — takes precedence over all other rules. + let author_login = extract_author_login(item); + if !author_login.is_empty() && is_blocked_user(author_login, ctx) { + let sha = item.get("sha").and_then(|v| v.as_str()).unwrap_or("unknown"); + let short_sha = if sha.len() > 8 { &sha[..8] } else { sha }; + crate::log_info(&format!( + "[integrity] commit:{}@{} → blocked (author '{}' in blocked-users)", + repo_full_name, short_sha, author_login + )); + return blocked_integrity(repo_full_name, ctx); + } + let mut integrity = author_association_floor(item, repo_full_name, ctx); if repo_private { diff --git a/guards/github-guard/rust-guard/src/labels/mod.rs b/guards/github-guard/rust-guard/src/labels/mod.rs index 7a9777cce..271ca19ad 100644 --- a/guards/github-guard/rust-guard/src/labels/mod.rs +++ b/guards/github-guard/rust-guard/src/labels/mod.rs @@ -42,12 +42,13 @@ pub use constants::MEDIUM_BUFFER_SIZE; // re-exported for external modules and tests, not used within mod.rs itself #[allow(unused_imports)] pub use helpers::{ - commit_integrity, ensure_integrity_baseline, extract_items_array, extract_number_as_string, - extract_repo_from_item, extract_repo_info, extract_repo_info_from_search_query, get_bool_or, - get_nested_str, get_str_or, is_bot, issue_integrity, limit_items_with_log, make_item_path, - merged_integrity, none_integrity, pr_integrity, private_scope_label, private_user_label, - project_github_label, reader_integrity, secret_label, writer_integrity, MinIntegrity, - PolicyContext, PolicyScopeEntry, ScopeKind, + blocked_integrity, commit_integrity, ensure_integrity_baseline, extract_items_array, + extract_number_as_string, extract_repo_from_item, extract_repo_info, + extract_repo_info_from_search_query, get_bool_or, get_nested_str, get_str_or, has_approval_label, + is_blocked_user, is_bot, issue_integrity, limit_items_with_log, make_item_path, merged_integrity, + none_integrity, pr_integrity, private_scope_label, private_user_label, project_github_label, + reader_integrity, secret_label, writer_integrity, MinIntegrity, PolicyContext, PolicyScopeEntry, + ScopeKind, }; // Re-export response labeling functions (wrappers that pass PolicyContext) @@ -1435,4 +1436,260 @@ mod tests { assert_eq!(result.labeled_paths[0].labels.description, "project-item:issue"); assert_eq!(result.labeled_paths[1].labels.description, "project-item:draft_issue"); } + + // ========================================================================= + // blocked-users tests + // ========================================================================= + + fn ctx_with_blocked_users(blocked: Vec<&str>) -> PolicyContext { + PolicyContext { + blocked_users: blocked.into_iter().map(|s| s.to_string()).collect(), + ..Default::default() + } + } + + fn ctx_with_approval_labels(labels: Vec<&str>) -> PolicyContext { + PolicyContext { + approval_labels: labels.into_iter().map(|s| s.to_string()).collect(), + ..Default::default() + } + } + + #[test] + fn test_is_blocked_user_empty_ctx() { + let ctx = default_ctx(); + assert!(!is_blocked_user("evil-bot", &ctx)); + assert!(!is_blocked_user("", &ctx)); + } + + #[test] + fn test_is_blocked_user_case_insensitive() { + let ctx = ctx_with_blocked_users(vec!["evil-bot", "untrusted-fork"]); + assert!(is_blocked_user("evil-bot", &ctx)); + assert!(is_blocked_user("Evil-Bot", &ctx)); + assert!(is_blocked_user("EVIL-BOT", &ctx)); + assert!(is_blocked_user("untrusted-fork", &ctx)); + assert!(!is_blocked_user("trusted-user", &ctx)); + } + + #[test] + fn test_blocked_integrity_returns_blocked_tag() { + let ctx = default_ctx(); + let scope = "github/copilot"; + let labels = blocked_integrity(scope, &ctx); + assert_eq!(labels.len(), 1); + assert_eq!(labels[0], format!("{}{}", + label_constants::BLOCKED_PREFIX, scope)); + } + + #[test] + fn test_pr_integrity_blocked_user_overrides_everything() { + let repo = "github/copilot"; + let ctx = ctx_with_blocked_users(vec!["evil-bot"]); + + // Blocked user PR — even if it has trusted association, it's blocked + let pr = json!({ + "user": {"login": "evil-bot"}, + "author_association": "OWNER", + "merged_at": "2024-01-15T10:00:00Z" + }); + assert_eq!( + pr_integrity(&pr, repo, false, Some(false), &ctx), + blocked_integrity(repo, &ctx) + ); + + // Non-blocked user PR still works normally + let normal_pr = json!({ + "user": {"login": "trusted-user"}, + "author_association": "OWNER", + "merged": false + }); + assert_eq!( + pr_integrity(&normal_pr, repo, false, Some(false), &ctx), + writer_integrity(repo, &ctx) + ); + } + + #[test] + fn test_issue_integrity_blocked_user() { + let repo = "github/copilot"; + let ctx = ctx_with_blocked_users(vec!["bad-actor"]); + + let issue = json!({ + "user": {"login": "bad-actor"}, + "author_association": "OWNER" + }); + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + blocked_integrity(repo, &ctx) + ); + + // Private repo also blocked if user is in blocked list + assert_eq!( + issue_integrity(&issue, repo, true, &ctx), + blocked_integrity(repo, &ctx) + ); + } + + #[test] + fn test_commit_integrity_blocked_user() { + let repo = "github/copilot"; + let ctx = ctx_with_blocked_users(vec!["bad-actor"]); + + // Commit via author.login + let commit = json!({ + "author": {"login": "bad-actor"}, + "author_association": "OWNER" + }); + assert_eq!( + commit_integrity(&commit, repo, false, false, &ctx), + blocked_integrity(repo, &ctx) + ); + + // Even default branch commits from blocked users are blocked + assert_eq!( + commit_integrity(&commit, repo, false, true, &ctx), + blocked_integrity(repo, &ctx) + ); + } + + #[test] + fn test_blocked_user_not_promoted_by_approval_label() { + let repo = "github/copilot"; + let ctx = PolicyContext { + blocked_users: vec!["evil-bot".to_string()], + approval_labels: vec!["approved".to_string()], + ..Default::default() + }; + + // Even with an approval label, a blocked user's PR remains blocked + let pr = json!({ + "user": {"login": "evil-bot"}, + "author_association": "NONE", + "merged": false, + "labels": [{"name": "approved"}] + }); + assert_eq!( + pr_integrity(&pr, repo, false, Some(false), &ctx), + blocked_integrity(repo, &ctx) + ); + } + + // ========================================================================= + // approval-labels tests + // ========================================================================= + + #[test] + fn test_has_approval_label_empty_ctx() { + let ctx = default_ctx(); + let item = json!({"labels": [{"name": "approved"}]}); + assert!(!has_approval_label(&item, &ctx)); + } + + #[test] + fn test_has_approval_label_no_match() { + let ctx = ctx_with_approval_labels(vec!["human-reviewed"]); + let item = json!({"labels": [{"name": "needs-work"}]}); + assert!(!has_approval_label(&item, &ctx)); + } + + #[test] + fn test_has_approval_label_matching() { + let ctx = ctx_with_approval_labels(vec!["approved", "human-reviewed"]); + + let item_approved = json!({"labels": [{"name": "approved"}]}); + assert!(has_approval_label(&item_approved, &ctx)); + + let item_reviewed = json!({"labels": [{"name": "human-reviewed"}, {"name": "bug"}]}); + assert!(has_approval_label(&item_reviewed, &ctx)); + } + + #[test] + fn test_has_approval_label_case_insensitive() { + let ctx = ctx_with_approval_labels(vec!["Approved"]); + let item = json!({"labels": [{"name": "approved"}]}); + assert!(has_approval_label(&item, &ctx)); + + let item2 = json!({"labels": [{"name": "APPROVED"}]}); + assert!(has_approval_label(&item2, &ctx)); + } + + #[test] + fn test_has_approval_label_missing_labels_field() { + let ctx = ctx_with_approval_labels(vec!["approved"]); + let item = json!({"title": "some issue"}); + assert!(!has_approval_label(&item, &ctx)); + } + + #[test] + fn test_pr_integrity_approval_label_promotes_to_approved() { + let repo = "github/copilot"; + let ctx = ctx_with_approval_labels(vec!["approved"]); + + // Public forked PR normally gets unapproved (reader) integrity + let forked_pr = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "merged": false, + "labels": [{"name": "approved"}] + }); + // With approval label, should be promoted to at least writer (approved) + assert_eq!( + pr_integrity(&forked_pr, repo, false, Some(true), &ctx), + writer_integrity(repo, &ctx) + ); + } + + #[test] + fn test_pr_integrity_approval_label_does_not_downgrade_merged() { + let repo = "github/copilot"; + let ctx = ctx_with_approval_labels(vec!["approved"]); + + // Merged PR already at merged-level — approval label should not downgrade it + let merged_pr = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "merged_at": "2024-01-15T10:00:00Z", + "labels": [{"name": "approved"}] + }); + assert_eq!( + pr_integrity(&merged_pr, repo, false, Some(false), &ctx), + merged_integrity(repo, &ctx) + ); + } + + #[test] + fn test_issue_integrity_approval_label_promotes() { + let repo = "github/copilot"; + let ctx = ctx_with_approval_labels(vec!["safe-to-process"]); + + // Public repo issue normally gets none integrity + let issue = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "labels": [{"name": "safe-to-process"}] + }); + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + writer_integrity(repo, &ctx) + ); + } + + #[test] + fn test_issue_integrity_already_at_writer_not_affected_by_label() { + let repo = "github/copilot"; + let ctx = ctx_with_approval_labels(vec!["approved"]); + + // Private repo issue already gets writer integrity — approval label does not change it + let issue = json!({ + "user": {"login": "member"}, + "author_association": "MEMBER", + "labels": [{"name": "approved"}] + }); + // Private repo → writer_integrity; approval label → max(writer, writer) = writer + assert_eq!( + issue_integrity(&issue, repo, true, &ctx), + writer_integrity(repo, &ctx) + ); + } } diff --git a/guards/github-guard/rust-guard/src/lib.rs b/guards/github-guard/rust-guard/src/lib.rs index 336764566..38e7d6dd2 100644 --- a/guards/github-guard/rust-guard/src/lib.rs +++ b/guards/github-guard/rust-guard/src/lib.rs @@ -277,6 +277,10 @@ struct AllowOnlyPolicy { scope: ReposValue, #[serde(rename = "min-integrity")] min_integrity: String, + #[serde(rename = "blocked-users", default)] + blocked_users: Vec, + #[serde(rename = "approval-labels", default)] + approval_labels: Vec, } #[derive(Debug, Deserialize)] @@ -506,6 +510,8 @@ pub extern "C" fn label_agent( let ctx = PolicyContext { scopes: scopes.clone(), trusted_bots, + blocked_users: policy.blocked_users, + approval_labels: policy.approval_labels, }; set_runtime_policy_context(ctx.clone()); diff --git a/internal/config/guard_policy.go b/internal/config/guard_policy.go index 675ab2ce0..2922ee6cd 100644 --- a/internal/config/guard_policy.go +++ b/internal/config/guard_policy.go @@ -39,15 +39,19 @@ type WriteSinkPolicy struct { // AllowOnlyPolicy configures scope and minimum required integrity. type AllowOnlyPolicy struct { - Repos interface{} `toml:"Repos" json:"repos"` - MinIntegrity string `toml:"MinIntegrity" json:"min-integrity"` + Repos interface{} `toml:"Repos" json:"repos"` + MinIntegrity string `toml:"MinIntegrity" json:"min-integrity"` + BlockedUsers []string `toml:"BlockedUsers" json:"blocked-users,omitempty"` + ApprovalLabels []string `toml:"ApprovalLabels" json:"approval-labels,omitempty"` } // NormalizedGuardPolicy is a canonical policy representation for caching and observability. type NormalizedGuardPolicy struct { - ScopeKind string `json:"scope_kind"` - ScopeValues []string `json:"scope_values,omitempty"` - MinIntegrity string `json:"min-integrity"` + ScopeKind string `json:"scope_kind"` + ScopeValues []string `json:"scope_values,omitempty"` + MinIntegrity string `json:"min-integrity"` + BlockedUsers []string `json:"blocked-users,omitempty"` + ApprovalLabels []string `json:"approval-labels,omitempty"` } func (p *GuardPolicy) UnmarshalJSON(data []byte) error { @@ -120,6 +124,14 @@ func (p *AllowOnlyPolicy) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(value, &p.MinIntegrity); err != nil { return fmt.Errorf("invalid allow-only.min-integrity: %w", err) } + case "blocked-users": + if err := json.Unmarshal(value, &p.BlockedUsers); err != nil { + return fmt.Errorf("invalid allow-only.blocked-users: %w", err) + } + case "approval-labels": + if err := json.Unmarshal(value, &p.ApprovalLabels); err != nil { + return fmt.Errorf("invalid allow-only.approval-labels: %w", err) + } default: return fmt.Errorf("allow-only contains unsupported field %q", key) } @@ -137,8 +149,10 @@ func (p *AllowOnlyPolicy) UnmarshalJSON(data []byte) error { func (p AllowOnlyPolicy) MarshalJSON() ([]byte, error) { type serializedAllowOnly struct { - Repos interface{} `json:"repos"` - MinIntegrity string `json:"min-integrity"` + Repos interface{} `json:"repos"` + MinIntegrity string `json:"min-integrity"` + BlockedUsers []string `json:"blocked-users,omitempty"` + ApprovalLabels []string `json:"approval-labels,omitempty"` } return json.Marshal(serializedAllowOnly(p)) @@ -278,6 +292,40 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) { logGuardPolicy.Printf("Normalizing guard policy: integrity=%s, reposType=%T", integrity, policy.AllowOnly.Repos) + // Validate and normalize blocked-users. + // Dedup uses lowercased keys to match Rust guard's case-insensitive comparison. + if len(policy.AllowOnly.BlockedUsers) > 0 { + seen := make(map[string]struct{}, len(policy.AllowOnly.BlockedUsers)) + for _, u := range policy.AllowOnly.BlockedUsers { + u = strings.TrimSpace(u) + if u == "" { + return nil, fmt.Errorf("allow-only.blocked-users entries must not be empty") + } + key := strings.ToLower(u) + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + normalized.BlockedUsers = append(normalized.BlockedUsers, u) + } + } + } + + // Validate and normalize approval-labels. + // Dedup uses lowercased keys to match Rust guard's case-insensitive comparison. + if len(policy.AllowOnly.ApprovalLabels) > 0 { + seen := make(map[string]struct{}, len(policy.AllowOnly.ApprovalLabels)) + for _, l := range policy.AllowOnly.ApprovalLabels { + l = strings.TrimSpace(l) + if l == "" { + return nil, fmt.Errorf("allow-only.approval-labels entries must not be empty") + } + key := strings.ToLower(l) + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + normalized.ApprovalLabels = append(normalized.ApprovalLabels, l) + } + } + } + switch scope := policy.AllowOnly.Repos.(type) { case string: scopeValue := strings.ToLower(strings.TrimSpace(scope)) diff --git a/internal/config/guard_policy_test.go b/internal/config/guard_policy_test.go index d6c55dd8b..7dfeff2c5 100644 --- a/internal/config/guard_policy_test.go +++ b/internal/config/guard_policy_test.go @@ -167,7 +167,121 @@ func TestNormalizeGuardPolicy(t *testing.T) { } } -// TestIsValidRepoScope tests all branch paths in isValidRepoScope. +// TestNormalizeGuardPolicyBlockedAndApproval tests NormalizeGuardPolicy with blocked-users and approval-labels. +func TestNormalizeGuardPolicyBlockedAndApproval(t *testing.T) { + t.Run("blocked-users propagated to normalized policy", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{"evil-bot", "bad-actor"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, []string{"evil-bot", "bad-actor"}, got.BlockedUsers) + }) + + t.Run("approval-labels propagated to normalized policy", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + ApprovalLabels: []string{"approved", "human-reviewed"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, []string{"approved", "human-reviewed"}, got.ApprovalLabels) + }) + + t.Run("blocked-users deduplication", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{"evil-bot", "evil-bot", "bad-actor"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.BlockedUsers, 2) + assert.Contains(t, got.BlockedUsers, "evil-bot") + assert.Contains(t, got.BlockedUsers, "bad-actor") + }) + + t.Run("blocked-users case-insensitive deduplication", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{"Evil-Bot", "evil-bot", "EVIL-BOT"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.BlockedUsers, 1) + assert.Equal(t, "Evil-Bot", got.BlockedUsers[0]) // keeps first occurrence + }) + + t.Run("approval-labels deduplication", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + ApprovalLabels: []string{"approved", "approved", "human-reviewed"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.ApprovalLabels, 2) + }) + + t.Run("approval-labels case-insensitive deduplication", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + ApprovalLabels: []string{"Approved", "approved", "APPROVED"}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.ApprovalLabels, 1) + assert.Equal(t, "Approved", got.ApprovalLabels[0]) // keeps first occurrence + }) + + t.Run("empty blocked-users string entry returns error", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{"valid-bot", ""}, + }} + + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "blocked-users entries must not be empty") + }) + + t.Run("empty approval-labels string entry returns error", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + ApprovalLabels: []string{"approved", ""}, + }} + + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "approval-labels entries must not be empty") + }) + + t.Run("empty blocked-users slice results in nil normalized list", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{}, + }} + + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Empty(t, got.BlockedUsers) + }) +} + func TestIsValidRepoScope(t *testing.T) { tests := []struct { name string @@ -481,6 +595,43 @@ func TestAllowOnlyPolicyUnmarshalJSON(t *testing.T) { assert.Len(t, repos, 2) }, }, + { + name: "blocked-users parsed correctly", + json: `{"repos":"public","min-integrity":"none","blocked-users":["evil-bot","bad-actor"]}`, + check: func(t *testing.T, p *AllowOnlyPolicy) { + assert.Equal(t, []string{"evil-bot", "bad-actor"}, p.BlockedUsers) + }, + }, + { + name: "approval-labels parsed correctly", + json: `{"repos":"public","min-integrity":"none","approval-labels":["approved","human-reviewed"]}`, + check: func(t *testing.T, p *AllowOnlyPolicy) { + assert.Equal(t, []string{"approved", "human-reviewed"}, p.ApprovalLabels) + }, + }, + { + name: "empty blocked-users array is valid", + json: `{"repos":"public","min-integrity":"none","blocked-users":[]}`, + check: func(t *testing.T, p *AllowOnlyPolicy) { + assert.Empty(t, p.BlockedUsers) + }, + }, + { + name: "empty approval-labels array is valid", + json: `{"repos":"public","min-integrity":"none","approval-labels":[]}`, + check: func(t *testing.T, p *AllowOnlyPolicy) { + assert.Empty(t, p.ApprovalLabels) + }, + }, + { + name: "all fields together parse correctly", + json: `{"repos":"public","min-integrity":"approved","blocked-users":["bad"],"approval-labels":["ok"]}`, + check: func(t *testing.T, p *AllowOnlyPolicy) { + assert.Equal(t, "approved", p.MinIntegrity) + assert.Equal(t, []string{"bad"}, p.BlockedUsers) + assert.Equal(t, []string{"ok"}, p.ApprovalLabels) + }, + }, } for _, tt := range tests { @@ -541,19 +692,54 @@ func TestGuardPolicyMarshalJSON(t *testing.T) { // TestAllowOnlyPolicyMarshalJSON tests AllowOnlyPolicy serializes with canonical keys. func TestAllowOnlyPolicyMarshalJSON(t *testing.T) { - policy := AllowOnlyPolicy{ - Repos: []interface{}{"myorg/*", "myorg/repo"}, - MinIntegrity: "approved", - } + t.Run("basic fields", func(t *testing.T) { + policy := AllowOnlyPolicy{ + Repos: []interface{}{"myorg/*", "myorg/repo"}, + MinIntegrity: "approved", + } - data, err := json.Marshal(policy) - require.NoError(t, err) + data, err := json.Marshal(policy) + require.NoError(t, err) - jsonStr := string(data) - assert.Contains(t, jsonStr, `"min-integrity"`) - assert.Contains(t, jsonStr, `"approved"`) - // Should NOT use legacy "integrity" key - assert.NotContains(t, jsonStr, `"integrity":`) + jsonStr := string(data) + assert.Contains(t, jsonStr, `"min-integrity"`) + assert.Contains(t, jsonStr, `"approved"`) + // Should NOT use legacy "integrity" key + assert.NotContains(t, jsonStr, `"integrity":`) + }) + + t.Run("blocked-users and approval-labels are included when set", func(t *testing.T) { + policy := AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + BlockedUsers: []string{"evil-bot"}, + ApprovalLabels: []string{"approved", "human-reviewed"}, + } + + data, err := json.Marshal(policy) + require.NoError(t, err) + + jsonStr := string(data) + assert.Contains(t, jsonStr, `"blocked-users"`) + assert.Contains(t, jsonStr, `"evil-bot"`) + assert.Contains(t, jsonStr, `"approval-labels"`) + assert.Contains(t, jsonStr, `"approved"`) + assert.Contains(t, jsonStr, `"human-reviewed"`) + }) + + t.Run("nil blocked-users and approval-labels are omitted", func(t *testing.T) { + policy := AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + } + + data, err := json.Marshal(policy) + require.NoError(t, err) + + jsonStr := string(data) + assert.NotContains(t, jsonStr, `"blocked-users"`) + assert.NotContains(t, jsonStr, `"approval-labels"`) + }) } // TestParseGuardPolicyJSONComprehensive tests ParseGuardPolicyJSON edge cases. diff --git a/internal/guard/wasm.go b/internal/guard/wasm.go index 36f6a708d..7446dac38 100644 --- a/internal/guard/wasm.go +++ b/internal/guard/wasm.go @@ -377,8 +377,18 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e if !hasIntegrity { integrityRaw, hasIntegrity = allowOnly["integrity"] } - if !hasRepos || !hasIntegrity || len(allowOnly) != 2 { - return nil, fmt.Errorf("invalid guard policy transport shape: expected {\"allow-only\":{\"repos\":...,\"min-integrity\":...}}") + if !hasRepos || !hasIntegrity { + return nil, fmt.Errorf("invalid guard policy transport shape: missing required fields repos and/or min-integrity in allow-only object") + } + + // Validate that the allow-only object contains only known keys. + for k := range allowOnly { + switch k { + case "repos", "min-integrity", "integrity", "blocked-users", "approval-labels": + // valid allow-only keys + default: + return nil, fmt.Errorf("invalid guard policy transport shape: unexpected allow-only key %q", k) + } } if !isValidAllowOnlyRepos(reposRaw) { @@ -396,6 +406,32 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e return nil, fmt.Errorf("invalid integrity value: expected one of none|unapproved|approved|merged") } + // Validate blocked-users if present: must be a non-empty array of non-empty strings. + if blockedUsersRaw, ok := allowOnly["blocked-users"]; ok { + arr, ok := blockedUsersRaw.([]interface{}) + if !ok { + return nil, fmt.Errorf("invalid blocked-users value: expected array of strings") + } + for _, entry := range arr { + if s, ok := entry.(string); !ok || strings.TrimSpace(s) == "" { + return nil, fmt.Errorf("invalid blocked-users value: each entry must be a non-empty string") + } + } + } + + // Validate approval-labels if present: must be a non-empty array of non-empty strings. + if approvalLabelsRaw, ok := allowOnly["approval-labels"]; ok { + arr, ok := approvalLabelsRaw.([]interface{}) + if !ok { + return nil, fmt.Errorf("invalid approval-labels value: expected array of strings") + } + for _, entry := range arr { + if s, ok := entry.(string); !ok || strings.TrimSpace(s) == "" { + return nil, fmt.Errorf("invalid approval-labels value: each entry must be a non-empty string") + } + } + } + // Validate trusted-bots if present. // Per spec §4.1.3.4: trustedBots MUST be a non-empty array of strings when present. if trustedBotsRaw, hasTrustedBots := payload["trusted-bots"]; hasTrustedBots { diff --git a/internal/guard/wasm_test.go b/internal/guard/wasm_test.go index 0460632a1..4a853a88b 100644 --- a/internal/guard/wasm_test.go +++ b/internal/guard/wasm_test.go @@ -305,7 +305,7 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) { _, err := buildStrictLabelAgentPayload(policy) require.Error(t, err) - assert.Contains(t, err.Error(), "expected {\"allow-only\":{\"repos\"") + assert.Contains(t, err.Error(), "missing required fields repos and/or min-integrity") }) t.Run("allow-only with missing integrity returns error", func(t *testing.T) { @@ -317,7 +317,7 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) { _, err := buildStrictLabelAgentPayload(policy) require.Error(t, err) - assert.Contains(t, err.Error(), "expected {\"allow-only\":{\"repos\"") + assert.Contains(t, err.Error(), "missing required fields repos and/or min-integrity") }) t.Run("allow-only with empty array repos returns error", func(t *testing.T) { @@ -502,6 +502,114 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "non-empty string") }) + + t.Run("valid blocked-users in allow-only succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "blocked-users": []interface{}{"evil-bot", "untrusted-fork"}, + }, + } + + result, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + require.NotNil(t, result) + allowOnly := result["allow-only"].(map[string]interface{}) + assert.Contains(t, allowOnly, "blocked-users") + }) + + t.Run("valid approval-labels in allow-only succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "approval-labels": []interface{}{"approved", "human-reviewed"}, + }, + } + + result, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + require.NotNil(t, result) + allowOnly := result["allow-only"].(map[string]interface{}) + assert.Contains(t, allowOnly, "approval-labels") + }) + + t.Run("blocked-users and approval-labels together with trusted-bots succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "blocked-users": []interface{}{"bad-actor"}, + "approval-labels": []interface{}{"approved"}, + }, + "trusted-bots": []interface{}{"my-org-bot"}, + } + + result, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + require.NotNil(t, result) + assert.Contains(t, result, "allow-only") + assert.Contains(t, result, "trusted-bots") + }) + + t.Run("blocked-users with non-string entry returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "blocked-users": []interface{}{"valid", 42}, + }, + } + + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "blocked-users") + assert.Contains(t, err.Error(), "non-empty string") + }) + + t.Run("blocked-users with empty string entry returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "blocked-users": []interface{}{"valid", ""}, + }, + } + + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "blocked-users") + }) + + t.Run("approval-labels with non-string entry returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "approval-labels": []interface{}{"approved", 99}, + }, + } + + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "approval-labels") + assert.Contains(t, err.Error(), "non-empty string") + }) + + t.Run("unknown allow-only key returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "none", + "unknown-field": "value", + }, + } + + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpected allow-only key") + }) } func TestBuildLabelAgentPayload(t *testing.T) {