Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions guards/github-guard/rust-guard/src/labels/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5907,7 +5907,7 @@ mod tests {
}

#[test]
fn test_apply_tool_labels_notification_management_public_project_github() {
fn test_apply_tool_labels_notification_management_user_private_write() {
let ctx = default_ctx();
let tool_args = json!({ "threadId": "123" });

Expand All @@ -5920,22 +5920,23 @@ mod tests {
let (secrecy, integrity, _desc) =
apply_tool_labels(tool, &tool_args, "", vec![], vec![], String::new(), &ctx);

assert!(
secrecy.is_empty(),
"{} should have empty (public) secrecy",
assert_eq!(
secrecy,
private_user_label(),
"{} should have private:user secrecy",
tool
);
assert_eq!(
integrity,
project_github_label(&ctx),
"{} should have project:github integrity",
writer_integrity(scope_names::USER, &ctx),
"{} should have writer:user integrity",
tool
);
}
}

#[test]
fn test_apply_tool_labels_star_repository_public() {
fn test_apply_tool_labels_star_repository_user_private_write() {
let ctx = default_ctx();
let tool_args = json!({
"owner": "github",
Expand All @@ -5952,19 +5953,20 @@ mod tests {
&ctx,
);

assert!(
secrecy.is_empty(),
"star_repository should have empty (public) secrecy"
assert_eq!(
secrecy,
private_user_label(),
"star_repository should have private:user secrecy"
);
assert_eq!(
integrity,
project_github_label(&ctx),
"star_repository should have project:github integrity"
writer_integrity(scope_names::USER, &ctx),
"star_repository should have writer:user integrity"
);
}

#[test]
fn test_apply_tool_labels_unstar_repository_public_secrecy_github_integrity() {
fn test_apply_tool_labels_unstar_repository_user_private_write() {
let ctx = default_ctx();
let tool_args = json!({
"owner": "github",
Expand All @@ -5981,14 +5983,15 @@ mod tests {
&ctx,
);

assert!(
secrecy.is_empty(),
"unstar_repository should have empty (public) secrecy — starring is a public action"
assert_eq!(
secrecy,
private_user_label(),
"unstar_repository should have private:user secrecy"
);
assert_eq!(
integrity,
project_github_label(&ctx),
"unstar_repository should have project:github integrity"
writer_integrity(scope_names::USER, &ctx),
"unstar_repository should have writer:user integrity"
);
}

Expand Down
22 changes: 11 additions & 11 deletions guards/github-guard/rust-guard/src/labels/tool_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -610,11 +610,11 @@ pub fn apply_tool_labels(
| "mark_all_notifications_read"
| "manage_notification_subscription"
| "manage_repository_notification_subscription" => {
// These operations change notification/subscription state and return minimal metadata.
// S = public (empty); I = project:github
secrecy = vec![];
baseline_scope = Cow::Borrowed(scope_names::GITHUB);
integrity = project_github_label(ctx);
// These operations change notification/subscription state for the authenticated user.
// S = private:user; I = writer(user)
secrecy = private_user_label();
baseline_scope = Cow::Borrowed(scope_names::USER);
integrity = writer_integrity(scope_names::USER, ctx);
Comment on lines +615 to +617
}

// === Private GitHub-controlled metadata (user-associated): PII/org-structure sensitive ===
Expand Down Expand Up @@ -881,13 +881,13 @@ pub fn apply_tool_labels(
integrity = writer_integrity(scope_names::GITHUB, ctx);
}

// === Star/unstar operations (public metadata) ===
// === Star/unstar operations (account-scoped writes) ===
"star_repository" | "unstar_repository" => {
// Starring is a public action; response is minimal metadata.
// S = public (empty); I = project:github
secrecy = vec![];
baseline_scope = Cow::Borrowed(scope_names::GITHUB);
integrity = project_github_label(ctx);
// Starring changes authenticated-user affinity state.
// S = private:user; I = writer(user)
secrecy = private_user_label();
baseline_scope = Cow::Borrowed(scope_names::USER);
integrity = writer_integrity(scope_names::USER, ctx);
}

// === Gist deletion (pre-emptive) ===
Expand Down
120 changes: 108 additions & 12 deletions guards/github-guard/rust-guard/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,15 @@ fn apply_singleton_fallback_if_needed(
} else {
&baseline_scope
};
let (secrecy, _, _) = labels::apply_tool_labels(
&input.tool_name,
&input.tool_args,
&repo_id,
vec![],
vec![],
String::new(),
ctx,
);
// Use writer_integrity which goes through normalize_scope to match
// the policy scope token (e.g., "github" for owner-scoped policies).
let integrity = labels::writer_integrity(scope, ctx);
Expand All @@ -386,7 +395,7 @@ fn apply_singleton_fallback_if_needed(
data: input.tool_result.clone(),
labels: ResourceLabels {
description: desc,
secrecy: vec![].into(),
secrecy: secrecy.into(),
integrity: integrity.into(),
},
});
Expand Down Expand Up @@ -436,21 +445,21 @@ fn infer_scope_for_baseline<'a>(
tool_args: &Value,
repo_id: &'a str,
) -> Cow<'a, str> {
if !repo_id.is_empty() {
return Cow::Borrowed(repo_id);
}

match tool_name {
"dismiss_notification"
| "mark_all_notifications_read"
| "manage_notification_subscription"
| "manage_repository_notification_subscription"
| "create_repository"
| "fork_repository" => Cow::Borrowed(scope_names::GITHUB),
| "star_repository"
| "unstar_repository" => Cow::Borrowed(scope_names::USER),
"create_repository" | "fork_repository" => Cow::Borrowed(scope_names::GITHUB),
"create_codespace" | "update_codespace" | "delete_codespace" | "stop_codespace" => {
Cow::Borrowed(scope_names::USER)
}
"set_secret" | "delete_secret" | "set_variable" | "delete_variable" => {
if !repo_id.is_empty() {
return Cow::Borrowed(repo_id);
}
let org = tool_args
.get("org")
.and_then(Value::as_str)
Expand Down Expand Up @@ -494,13 +503,17 @@ fn infer_scope_for_baseline<'a>(
| "search_pull_requests"
| "search_pull_requests_ff_fields_param"
| "search_commits" => {
if !repo_id.is_empty() {
return Cow::Borrowed(repo_id);
}
let query = tool_args
.get("query")
.and_then(|v| v.as_str())
.unwrap_or("");
let (_, _, repo_from_query) = extract_repo_info_from_search_query(query);
Cow::Owned(repo_from_query)
}
_ if !repo_id.is_empty() => Cow::Borrowed(repo_id),
_ => Cow::Borrowed(""),
}
}
Expand Down Expand Up @@ -1158,6 +1171,49 @@ mod tests {
assert!(labeled_items.is_empty());
}

#[test]
fn plaintext_notification_mutations_keep_user_secrecy() {
let ctx = PolicyContext::default();
for (tool_name, tool_args) in [
("dismiss_notification", json!({"thread_id": "123"})),
("mark_all_notifications_read", json!({})),
(
"manage_notification_subscription",
json!({"thread_id": "123", "action": "delete"}),
),
(
"manage_repository_notification_subscription",
json!({"owner": "github", "repo": "gh-aw-mcpg", "action": "delete"}),
),
] {
let input = LabelResponseInput {
tool_name: tool_name.to_string(),
tool_args,
tool_result: json!({
"content": [{"type": "text", "text": "Notification updated"}]
}),
};
let mut labeled_items = Vec::new();

let action = apply_singleton_fallback_if_needed(&input, &ctx, &mut labeled_items);

assert!(matches!(action, FallbackAction::ContinueProcessing));
assert_eq!(labeled_items.len(), 1);
assert_eq!(
labeled_items[0].labels.secrecy,
labels::private_user_label(),
"{} plaintext response should retain user secrecy",
tool_name
);
assert_eq!(
labeled_items[0].labels.integrity,
labels::writer_integrity(scope_names::USER, &ctx),
"{} plaintext response should retain user integrity",
tool_name
);
}
}

#[test]
fn parse_scope_accepts_owner_wildcard_array_entry() {
let parsed = parse_scope(ReposValue::ScopedList(vec!["octocat/*".to_string()]))
Expand Down Expand Up @@ -1352,7 +1408,7 @@ mod tests {
}

#[test]
fn infer_scope_for_baseline_uses_github_scope_for_notification_management_tools() {
fn infer_scope_for_baseline_uses_user_scope_for_notification_management_tools() {
let tool_args = json!({ "threadId": "123" });
for tool in &[
"dismiss_notification",
Expand All @@ -1363,8 +1419,14 @@ mod tests {
let inferred = infer_scope_for_baseline(tool, &tool_args, "");
assert_eq!(
inferred,
scope_names::GITHUB,
"{} should infer github baseline scope",
scope_names::USER,
"{} should infer user baseline scope",
tool
);
assert_eq!(
infer_scope_for_baseline(tool, &tool_args, "github/gh-aw-mcpg"),
scope_names::USER,
"{} should keep user baseline scope even with repo context",
tool
);
}
Expand Down Expand Up @@ -1409,8 +1471,37 @@ mod tests {

assert_eq!(
after_baseline,
labels::project_github_label(&ctx),
"{} integrity should remain github-scoped after baseline enforcement",
labels::writer_integrity(scope_names::USER, &ctx),
"{} integrity should remain user-scoped after baseline enforcement",
tool
);
}
}

#[test]
fn star_repository_integrity_remains_user_scoped_with_repo_args() {
let ctx = PolicyContext::default();
let tool_args = json!({ "owner": "github", "repo": "gh-aw-mcpg" });
let repo_id = "github/gh-aw-mcpg";

for tool in &["star_repository", "unstar_repository"] {
let (_, integrity, _) = labels::apply_tool_labels(
tool,
&tool_args,
repo_id,
vec![],
vec![],
String::new(),
&ctx,
);
let baseline_scope = infer_scope_for_baseline(tool, &tool_args, repo_id);
let after_baseline =
labels::ensure_integrity_baseline(&baseline_scope, integrity, &ctx);

assert_eq!(
after_baseline,
labels::writer_integrity(scope_names::USER, &ctx),
"{} integrity should remain user-scoped after baseline enforcement",
tool
);
}
Expand Down Expand Up @@ -1484,6 +1575,11 @@ mod tests {
"github",
"{tool} should infer org baseline scope from owner-only synthetic CLI args"
);
assert_eq!(
infer_scope_for_baseline(tool, &json!({}), "github/gh-aw-mcpg"),
"github/gh-aw-mcpg",
"{tool} should preserve repo baseline scope when repo context is present"
);
}

for tool in &["set_secret", "delete_secret"] {
Expand Down
48 changes: 46 additions & 2 deletions guards/github-guard/rust-guard/src/tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ pub const WRITE_OPERATIONS: &[&str] = &[
"label_write",
"lock_issue", // gh issue lock
"lock_pull_request", // gh pr lock
"manage_notification_subscription",
"manage_repository_notification_subscription",
"mark_all_notifications_read",
"mark_project_template", // gh project mark-template — GraphQL markProjectV2AsTemplate
"projects_write",
Expand Down Expand Up @@ -140,6 +138,8 @@ pub const READ_WRITE_OPERATIONS: &[&str] = &[
"issue_dependency_write", // GraphQL addBlockedBy/removeBlockedBy after resolving issue IDs
"issue_write",
"issue_write_ff_remote_mcp_issue_fields", // feature-flag variant of issue_write
"manage_notification_subscription",
"manage_repository_notification_subscription",
"merge_pull_request",
"pull_request_review_write",
"remove_sub_issue", // DELETE/POST — remove sub-issue link
Expand Down Expand Up @@ -730,6 +730,50 @@ mod tests {
}
}

#[test]
fn test_notification_and_star_tools_match_upstream_write_classification() {
for op in &[
"dismiss_notification",
"mark_all_notifications_read",
"star_repository",
"unstar_repository",
] {
assert!(
WRITE_OPERATIONS.binary_search(op).is_ok(),
"{op} must be explicitly listed in WRITE_OPERATIONS"
);
assert!(
is_write_operation(op),
"{op} must be classified as a write operation"
);
assert!(
!is_read_write_operation(op),
"{op} should not be in READ_WRITE_OPERATIONS"
);
}
}

#[test]
fn test_notification_subscription_tools_match_upstream_read_write_classification() {
for op in &[
"manage_notification_subscription",
"manage_repository_notification_subscription",
] {
assert!(
READ_WRITE_OPERATIONS.binary_search(op).is_ok(),
"{op} must be explicitly listed in READ_WRITE_OPERATIONS"
);
assert!(
is_read_write_operation(op),
"{op} must be classified as a read-write operation"
);
assert!(
!is_write_operation(op),
"{op} should not be in WRITE_OPERATIONS"
);
}
}

#[test]
fn test_is_merge_operation() {
assert!(is_merge_operation("merge_pull_request"));
Expand Down
Loading