From 3cb0a87961f59b63a7ff379bae5d7addfdb56811 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Wed, 15 Apr 2026 17:39:06 +1000 Subject: [PATCH 1/2] Add collaborator permission fallback to response-level integrity functions The collaborator permission API fallback (from PR #2863) was only applied in resolve_author_integrity (Phase 1 LabelResource), but not in the per-item response labeling functions issue_integrity, pr_integrity, and commit_integrity (Phase 5 LabelResponse). For reads, Phase 2 skips the coarse-grained block and defers to Phase 5's per-item labels. So for org owners whose author_association is 'NONE' (because admin access is inherited through org ownership rather than direct collaboration), the per-item integrity was too low, causing content to be incorrectly filtered. Fix: Extract a shared elevate_via_collaborator_permission helper and apply it in all four integrity-computing functions: - resolve_author_integrity (refactored to use shared helper) - issue_integrity (new: adds collaborator permission fallback) - pr_integrity (new: adds collaborator permission fallback) - commit_integrity (new: adds collaborator permission fallback) --- .../rust-guard/src/labels/helpers.rs | 88 +++++++++++++++++++ .../rust-guard/src/labels/tool_rules.rs | 36 ++------ 2 files changed, 94 insertions(+), 30 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index bf1a68ce..56141f1c 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1291,6 +1291,63 @@ pub fn collaborator_permission_floor( } } +/// Elevate integrity via collaborator permission fallback for org repos. +/// +/// When `author_association` gives insufficient integrity (fewer than 3 labels, +/// i.e. below approved/writer level), this function checks the user's effective +/// permission via the GitHub collaborator permission API. This correctly handles +/// org owners/admins whose `author_association` is reported as "NONE" because +/// their access is inherited through org ownership rather than direct collaboration. +/// +/// Parameters: +/// - `author_login`: the issue/PR/commit author's GitHub login +/// - `repo_full_name`: "owner/repo" string +/// - `resource_label`: label for logging (e.g. "issue", "pr", "commit") +/// - `resource_id`: number or identifier for logging +/// - `integrity`: current integrity labels to potentially elevate +/// - `ctx`: policy context +/// +/// Returns the (potentially elevated) integrity labels. +pub fn elevate_via_collaborator_permission( + author_login: &str, + repo_full_name: &str, + resource_label: &str, + resource_id: &str, + integrity: Vec, + ctx: &PolicyContext, +) -> Vec { + if integrity.len() >= 3 || author_login.is_empty() { + return integrity; + } + let (owner, repo) = match repo_full_name.split_once('/') { + Some((o, r)) if !o.is_empty() && !r.is_empty() => (o, r), + _ => return integrity, + }; + let is_org = super::backend::is_repo_org_owned(owner, repo).unwrap_or(false); + if !is_org { + return integrity; + } + crate::log_info(&format!( + "[integrity] {}:{}: author_association floor insufficient (len={}), checking collaborator permission for {}", + resource_label, resource_id, integrity.len(), author_login + )); + if let Some(collab) = super::backend::get_collaborator_permission(owner, repo, author_login) { + let perm_floor = collaborator_permission_floor(repo_full_name, collab.permission.as_deref(), ctx); + let merged = max_integrity(repo_full_name, integrity, perm_floor, ctx); + crate::log_info(&format!( + "[integrity] {}:{}: collaborator permission={:?} → merged floor len={}", + resource_label, resource_id, collab.permission, merged.len() + )); + merged + } else { + crate::log_info(&format!( + "[integrity] {}:{}: collaborator permission lookup returned None for {}, keeping author_association floor", + resource_label, resource_id, author_login + )); + integrity + } +} + /// Check if a branch/ref should be treated as default branch context pub fn is_default_branch_ref(branch_ref: &str) -> bool { branch_ref.is_empty() @@ -1417,6 +1474,16 @@ pub fn pr_integrity( } } + // Collaborator permission fallback for org repos (handles org owners/admins + // whose author_association is "NONE" due to inherited org access). + if !repo_private { + let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0); + integrity = elevate_via_collaborator_permission( + author_login, repo_full_name, "pr", &format!("{}#{}", repo_full_name, number), + integrity, ctx, + ); + } + if repo_private { integrity = max_integrity( repo_full_name, @@ -1527,6 +1594,16 @@ pub fn issue_integrity( } } + // Collaborator permission fallback for org repos (handles org owners/admins + // whose author_association is "NONE" due to inherited org access). + if !repo_private { + let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0); + integrity = elevate_via_collaborator_permission( + author_login, repo_full_name, "issue", &format!("{}#{}", repo_full_name, number), + integrity, ctx, + ); + } + if repo_private { integrity = max_integrity( repo_full_name, @@ -1579,6 +1656,17 @@ pub fn commit_integrity( let mut integrity = author_association_floor(item, repo_full_name, ctx); + // Collaborator permission fallback for org repos (handles org owners/admins + // whose author_association is "NONE" due to inherited org access). + if !repo_private { + 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 }; + integrity = elevate_via_collaborator_permission( + author_login, repo_full_name, "commit", &format!("{}@{}", repo_full_name, short_sha), + integrity, ctx, + ); + } + if repo_private { integrity = max_integrity( repo_full_name, diff --git a/guards/github-guard/rust-guard/src/labels/tool_rules.rs b/guards/github-guard/rust-guard/src/labels/tool_rules.rs index a0c756e7..bd89d994 100644 --- a/guards/github-guard/rust-guard/src/labels/tool_rules.rs +++ b/guards/github-guard/rust-guard/src/labels/tool_rules.rs @@ -7,7 +7,8 @@ use serde_json::Value; use super::constants::{field_names, SENSITIVE_FILE_KEYWORDS, SENSITIVE_FILE_PATTERNS}; use super::helpers::{ - author_association_floor_from_str, collaborator_permission_floor, ensure_integrity_baseline, + author_association_floor_from_str, + elevate_via_collaborator_permission, ensure_integrity_baseline, extract_number_as_string, extract_repo_info, extract_repo_info_from_search_query, format_repo_id, is_configured_trusted_bot, is_default_branch_commit_context, is_default_branch_ref, is_trusted_first_party_bot, is_trusted_user, max_integrity, @@ -100,35 +101,10 @@ fn resolve_author_integrity( { floor = max_integrity(repo_id, floor, writer_integrity(repo_id, ctx), ctx); } - if floor.len() < 3 { - let is_org = super::backend::is_repo_org_owned(owner, repo).unwrap_or(false); - if is_org { - crate::log_info(&format!( - "{} {}/{}#{}: author_association floor insufficient (len={}), checking collaborator permission for {}", - resource_label, owner, repo, resource_num, floor.len(), login - )); - if let Some(collab) = - super::backend::get_collaborator_permission(owner, repo, login) - { - let perm_floor = collaborator_permission_floor( - repo_id, - collab.permission.as_deref(), - ctx, - ); - let merged = max_integrity(repo_id, floor, perm_floor, ctx); - crate::log_info(&format!( - "{} {}/{}#{}: collaborator permission={:?} → merged floor len={}", - resource_label, owner, repo, resource_num, collab.permission, merged.len() - )); - floor = merged; - } else { - crate::log_info(&format!( - "{} {}/{}#{}: collaborator permission lookup returned None for {}, keeping author_association floor", - resource_label, owner, repo, resource_num, login - )); - } - } - } + let resource_id = format!("{}/{}#{}", owner, repo, resource_num); + floor = elevate_via_collaborator_permission( + login, repo_id, resource_label, &resource_id, floor, ctx, + ); } max_integrity(repo_id, base_integrity, floor, ctx) From 56359085c08f4ae7ed480348fb3b61a064a96044 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 15 Apr 2026 09:42:14 -0700 Subject: [PATCH 2/2] fix: address review feedback on elevate_via_collaborator_permission 1. Replace brittle integrity.len() >= 3 check with semantic integrity_rank() >= WRITER_RANK (3). The rank-based check is future-proof if label sets ever change. 2. Downgrade all log_info to log_debug in the helper. The function runs per-item in list/search responses, so info-level logging would be extremely noisy in production. 3. Add 7 unit tests for elevate_via_collaborator_permission covering all early-return paths: already writer, already merged, empty login, invalid repo format, empty owner/repo, non-org repo (cache miss), and reader integrity preservation. 4. Add doc comment noting backend calls are cached per-user, so repeated lookups across list items are inexpensive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../rust-guard/src/labels/helpers.rs | 35 ++++--- .../github-guard/rust-guard/src/labels/mod.rs | 93 +++++++++++++++++++ 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 56141f1c..83ddf79d 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1293,11 +1293,20 @@ pub fn collaborator_permission_floor( /// Elevate integrity via collaborator permission fallback for org repos. /// -/// When `author_association` gives insufficient integrity (fewer than 3 labels, -/// i.e. below approved/writer level), this function checks the user's effective -/// permission via the GitHub collaborator permission API. This correctly handles -/// org owners/admins whose `author_association` is reported as "NONE" because -/// their access is inherited through org ownership rather than direct collaboration. +/// Rank threshold for writer-level integrity (none=1, reader=2, writer=3, merged=4). +const WRITER_RANK: u8 = 3; + +/// Attempt to elevate integrity for an author in an org-owned repository +/// by checking their effective collaborator permission. +/// +/// When `author_association` gives insufficient integrity (below writer level), +/// this function checks the user's effective permission via the GitHub +/// collaborator permission API. This correctly handles org owners/admins whose +/// `author_association` is reported as "NONE" because their access is inherited +/// through org ownership rather than direct collaboration. +/// +/// Backend calls are cached per-user, so repeated lookups for the same author +/// across list/search items are inexpensive. /// /// Parameters: /// - `author_login`: the issue/PR/commit author's GitHub login @@ -1316,7 +1325,7 @@ pub fn elevate_via_collaborator_permission( integrity: Vec, ctx: &PolicyContext, ) -> Vec { - if integrity.len() >= 3 || author_login.is_empty() { + if integrity_rank(repo_full_name, &integrity, ctx) >= WRITER_RANK || author_login.is_empty() { return integrity; } let (owner, repo) = match repo_full_name.split_once('/') { @@ -1327,20 +1336,20 @@ pub fn elevate_via_collaborator_permission( if !is_org { return integrity; } - crate::log_info(&format!( - "[integrity] {}:{}: author_association floor insufficient (len={}), checking collaborator permission for {}", - resource_label, resource_id, integrity.len(), author_login + crate::log_debug(&format!( + "[integrity] {}:{}: author_association floor below writer (rank={}), checking collaborator permission for {}", + resource_label, resource_id, integrity_rank(repo_full_name, &integrity, ctx), author_login )); if let Some(collab) = super::backend::get_collaborator_permission(owner, repo, author_login) { let perm_floor = collaborator_permission_floor(repo_full_name, collab.permission.as_deref(), ctx); let merged = max_integrity(repo_full_name, integrity, perm_floor, ctx); - crate::log_info(&format!( - "[integrity] {}:{}: collaborator permission={:?} → merged floor len={}", - resource_label, resource_id, collab.permission, merged.len() + crate::log_debug(&format!( + "[integrity] {}:{}: collaborator permission={:?} → merged rank={}", + resource_label, resource_id, collab.permission, integrity_rank(repo_full_name, &merged, ctx) )); merged } else { - crate::log_info(&format!( + crate::log_debug(&format!( "[integrity] {}:{}: collaborator permission lookup returned None for {}, keeping author_association floor", resource_label, resource_id, author_login )); diff --git a/guards/github-guard/rust-guard/src/labels/mod.rs b/guards/github-guard/rust-guard/src/labels/mod.rs index 04724b49..47f6144a 100644 --- a/guards/github-guard/rust-guard/src/labels/mod.rs +++ b/guards/github-guard/rust-guard/src/labels/mod.rs @@ -4957,4 +4957,97 @@ mod tests { helpers::blocked_integrity(repo, &ctx) ); } + + // === elevate_via_collaborator_permission tests === + + #[test] + fn test_elevate_via_collab_permission_skips_when_already_writer() { + let ctx = default_ctx(); + let repo = "github/copilot"; + let writer = writer_integrity(repo, &ctx); + let result = helpers::elevate_via_collaborator_permission( + "someuser", repo, "issue", "github/copilot#1", + writer.clone(), &ctx, + ); + assert_eq!(result, writer, "should return unchanged when already at writer level"); + } + + #[test] + fn test_elevate_via_collab_permission_skips_when_merged() { + let ctx = default_ctx(); + let repo = "github/copilot"; + let merged = merged_integrity(repo, &ctx); + let result = helpers::elevate_via_collaborator_permission( + "someuser", repo, "issue", "github/copilot#1", + merged.clone(), &ctx, + ); + assert_eq!(result, merged, "should return unchanged when already at merged level"); + } + + #[test] + fn test_elevate_via_collab_permission_skips_empty_login() { + let ctx = default_ctx(); + let repo = "github/copilot"; + let none = none_integrity(repo, &ctx); + let result = helpers::elevate_via_collaborator_permission( + "", repo, "issue", "github/copilot#1", + none.clone(), &ctx, + ); + assert_eq!(result, none, "should return unchanged when author_login is empty"); + } + + #[test] + fn test_elevate_via_collab_permission_skips_invalid_repo() { + let ctx = default_ctx(); + let none = none_integrity("invalid", &ctx); + let result = helpers::elevate_via_collaborator_permission( + "someuser", "invalid", "issue", "invalid#1", + none.clone(), &ctx, + ); + assert_eq!(result, none, "should return unchanged for invalid repo format"); + } + + #[test] + fn test_elevate_via_collab_permission_skips_empty_owner_or_repo() { + let ctx = default_ctx(); + let none = none_integrity("/repo", &ctx); + let result = helpers::elevate_via_collaborator_permission( + "someuser", "/repo", "issue", "/repo#1", + none.clone(), &ctx, + ); + assert_eq!(result, none, "should return unchanged for empty owner"); + + let none2 = none_integrity("owner/", &ctx); + let result2 = helpers::elevate_via_collaborator_permission( + "someuser", "owner/", "issue", "owner/#1", + none2.clone(), &ctx, + ); + assert_eq!(result2, none2, "should return unchanged for empty repo"); + } + + #[test] + fn test_elevate_via_collab_permission_no_elevation_non_org_repo() { + // In test mode, is_repo_org_owned returns None (no cache) → unwrap_or(false) + // so the function should return integrity unchanged + let ctx = default_ctx(); + let repo = "github/copilot"; + let none = none_integrity(repo, &ctx); + let result = helpers::elevate_via_collaborator_permission( + "dsyme", repo, "issue", "github/copilot#42", + none.clone(), &ctx, + ); + assert_eq!(result, none, "should return unchanged when repo is not org-owned (cache miss → false)"); + } + + #[test] + fn test_elevate_via_collab_permission_preserves_reader_integrity() { + let ctx = default_ctx(); + let repo = "github/copilot"; + let reader = reader_integrity(repo, &ctx); + let result = helpers::elevate_via_collaborator_permission( + "contributor", repo, "pr", "github/copilot#10", + reader.clone(), &ctx, + ); + assert_eq!(result, reader, "should preserve reader integrity when no org lookup available"); + } }