Skip to content

Commit bc4e02f

Browse files
authored
Cover notification and star mutations in GitHub guard (#12105)
The GitHub guard was missing or misclassifying several mutating upstream MCP tools, leaving notification and star operations without the intended write/read-write inventory coverage and DIFC labels. - **Tool classification** - Classifies notification dismiss/read-all and star/unstar as write operations. - Classifies notification subscription management as read-write operations. ```rust pub const WRITE_OPERATIONS: &[&str] = &[ "dismiss_notification", "mark_all_notifications_read", "star_repository", "unstar_repository", ]; pub const READ_WRITE_OPERATIONS: &[&str] = &[ "manage_notification_subscription", "manage_repository_notification_subscription", ]; ``` - **DIFC labeling** - Labels notification management and star/unstar as authenticated user state: - secrecy: `private:user` - integrity: `approved:user` - Keeps account-scoped baseline inference ahead of generic repo fallback so repo arguments do not accidentally relabel user-scoped mutations. - **Regression coverage** - Adds focused tests for the six upstream operations’ inventory buckets. - Adds baseline-scope tests for notification/star tools and preserves existing account-scoped behavior for codespace and secret/variable operations. <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #12081
2 parents 34ed844 + 6753034 commit bc4e02f

4 files changed

Lines changed: 186 additions & 43 deletions

File tree

guards/github-guard/rust-guard/src/labels/mod.rs

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5907,7 +5907,7 @@ mod tests {
59075907
}
59085908

59095909
#[test]
5910-
fn test_apply_tool_labels_notification_management_public_project_github() {
5910+
fn test_apply_tool_labels_notification_management_user_private_write() {
59115911
let ctx = default_ctx();
59125912
let tool_args = json!({ "threadId": "123" });
59135913

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

5923-
assert!(
5924-
secrecy.is_empty(),
5925-
"{} should have empty (public) secrecy",
5923+
assert_eq!(
5924+
secrecy,
5925+
private_user_label(),
5926+
"{} should have private:user secrecy",
59265927
tool
59275928
);
59285929
assert_eq!(
59295930
integrity,
5930-
project_github_label(&ctx),
5931-
"{} should have project:github integrity",
5931+
writer_integrity(scope_names::USER, &ctx),
5932+
"{} should have writer:user integrity",
59325933
tool
59335934
);
59345935
}
59355936
}
59365937

59375938
#[test]
5938-
fn test_apply_tool_labels_star_repository_public() {
5939+
fn test_apply_tool_labels_star_repository_user_private_write() {
59395940
let ctx = default_ctx();
59405941
let tool_args = json!({
59415942
"owner": "github",
@@ -5952,19 +5953,20 @@ mod tests {
59525953
&ctx,
59535954
);
59545955

5955-
assert!(
5956-
secrecy.is_empty(),
5957-
"star_repository should have empty (public) secrecy"
5956+
assert_eq!(
5957+
secrecy,
5958+
private_user_label(),
5959+
"star_repository should have private:user secrecy"
59585960
);
59595961
assert_eq!(
59605962
integrity,
5961-
project_github_label(&ctx),
5962-
"star_repository should have project:github integrity"
5963+
writer_integrity(scope_names::USER, &ctx),
5964+
"star_repository should have writer:user integrity"
59635965
);
59645966
}
59655967

59665968
#[test]
5967-
fn test_apply_tool_labels_unstar_repository_public_secrecy_github_integrity() {
5969+
fn test_apply_tool_labels_unstar_repository_user_private_write() {
59685970
let ctx = default_ctx();
59695971
let tool_args = json!({
59705972
"owner": "github",
@@ -5981,14 +5983,15 @@ mod tests {
59815983
&ctx,
59825984
);
59835985

5984-
assert!(
5985-
secrecy.is_empty(),
5986-
"unstar_repository should have empty (public) secrecy — starring is a public action"
5986+
assert_eq!(
5987+
secrecy,
5988+
private_user_label(),
5989+
"unstar_repository should have private:user secrecy"
59875990
);
59885991
assert_eq!(
59895992
integrity,
5990-
project_github_label(&ctx),
5991-
"unstar_repository should have project:github integrity"
5993+
writer_integrity(scope_names::USER, &ctx),
5994+
"unstar_repository should have writer:user integrity"
59925995
);
59935996
}
59945997

guards/github-guard/rust-guard/src/labels/tool_rules.rs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -610,11 +610,11 @@ pub fn apply_tool_labels(
610610
| "mark_all_notifications_read"
611611
| "manage_notification_subscription"
612612
| "manage_repository_notification_subscription" => {
613-
// These operations change notification/subscription state and return minimal metadata.
614-
// S = public (empty); I = project:github
615-
secrecy = vec![];
616-
baseline_scope = Cow::Borrowed(scope_names::GITHUB);
617-
integrity = project_github_label(ctx);
613+
// These operations change notification/subscription state for the authenticated user.
614+
// S = private:user; I = writer(user)
615+
secrecy = private_user_label();
616+
baseline_scope = Cow::Borrowed(scope_names::USER);
617+
integrity = writer_integrity(scope_names::USER, ctx);
618618
}
619619

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

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

893893
// === Gist deletion (pre-emptive) ===

guards/github-guard/rust-guard/src/lib.rs

Lines changed: 108 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,15 @@ fn apply_singleton_fallback_if_needed(
372372
} else {
373373
&baseline_scope
374374
};
375+
let (secrecy, _, _) = labels::apply_tool_labels(
376+
&input.tool_name,
377+
&input.tool_args,
378+
&repo_id,
379+
vec![],
380+
vec![],
381+
String::new(),
382+
ctx,
383+
);
375384
// Use writer_integrity which goes through normalize_scope to match
376385
// the policy scope token (e.g., "github" for owner-scoped policies).
377386
let integrity = labels::writer_integrity(scope, ctx);
@@ -386,7 +395,7 @@ fn apply_singleton_fallback_if_needed(
386395
data: input.tool_result.clone(),
387396
labels: ResourceLabels {
388397
description: desc,
389-
secrecy: vec![].into(),
398+
secrecy: secrecy.into(),
390399
integrity: integrity.into(),
391400
},
392401
});
@@ -436,21 +445,21 @@ fn infer_scope_for_baseline<'a>(
436445
tool_args: &Value,
437446
repo_id: &'a str,
438447
) -> Cow<'a, str> {
439-
if !repo_id.is_empty() {
440-
return Cow::Borrowed(repo_id);
441-
}
442-
443448
match tool_name {
444449
"dismiss_notification"
445450
| "mark_all_notifications_read"
446451
| "manage_notification_subscription"
447452
| "manage_repository_notification_subscription"
448-
| "create_repository"
449-
| "fork_repository" => Cow::Borrowed(scope_names::GITHUB),
453+
| "star_repository"
454+
| "unstar_repository" => Cow::Borrowed(scope_names::USER),
455+
"create_repository" | "fork_repository" => Cow::Borrowed(scope_names::GITHUB),
450456
"create_codespace" | "update_codespace" | "delete_codespace" | "stop_codespace" => {
451457
Cow::Borrowed(scope_names::USER)
452458
}
453459
"set_secret" | "delete_secret" | "set_variable" | "delete_variable" => {
460+
if !repo_id.is_empty() {
461+
return Cow::Borrowed(repo_id);
462+
}
454463
let org = tool_args
455464
.get("org")
456465
.and_then(Value::as_str)
@@ -494,13 +503,17 @@ fn infer_scope_for_baseline<'a>(
494503
| "search_pull_requests"
495504
| "search_pull_requests_ff_fields_param"
496505
| "search_commits" => {
506+
if !repo_id.is_empty() {
507+
return Cow::Borrowed(repo_id);
508+
}
497509
let query = tool_args
498510
.get("query")
499511
.and_then(|v| v.as_str())
500512
.unwrap_or("");
501513
let (_, _, repo_from_query) = extract_repo_info_from_search_query(query);
502514
Cow::Owned(repo_from_query)
503515
}
516+
_ if !repo_id.is_empty() => Cow::Borrowed(repo_id),
504517
_ => Cow::Borrowed(""),
505518
}
506519
}
@@ -1158,6 +1171,49 @@ mod tests {
11581171
assert!(labeled_items.is_empty());
11591172
}
11601173

1174+
#[test]
1175+
fn plaintext_notification_mutations_keep_user_secrecy() {
1176+
let ctx = PolicyContext::default();
1177+
for (tool_name, tool_args) in [
1178+
("dismiss_notification", json!({"thread_id": "123"})),
1179+
("mark_all_notifications_read", json!({})),
1180+
(
1181+
"manage_notification_subscription",
1182+
json!({"thread_id": "123", "action": "delete"}),
1183+
),
1184+
(
1185+
"manage_repository_notification_subscription",
1186+
json!({"owner": "github", "repo": "gh-aw-mcpg", "action": "delete"}),
1187+
),
1188+
] {
1189+
let input = LabelResponseInput {
1190+
tool_name: tool_name.to_string(),
1191+
tool_args,
1192+
tool_result: json!({
1193+
"content": [{"type": "text", "text": "Notification updated"}]
1194+
}),
1195+
};
1196+
let mut labeled_items = Vec::new();
1197+
1198+
let action = apply_singleton_fallback_if_needed(&input, &ctx, &mut labeled_items);
1199+
1200+
assert!(matches!(action, FallbackAction::ContinueProcessing));
1201+
assert_eq!(labeled_items.len(), 1);
1202+
assert_eq!(
1203+
labeled_items[0].labels.secrecy,
1204+
labels::private_user_label(),
1205+
"{} plaintext response should retain user secrecy",
1206+
tool_name
1207+
);
1208+
assert_eq!(
1209+
labeled_items[0].labels.integrity,
1210+
labels::writer_integrity(scope_names::USER, &ctx),
1211+
"{} plaintext response should retain user integrity",
1212+
tool_name
1213+
);
1214+
}
1215+
}
1216+
11611217
#[test]
11621218
fn parse_scope_accepts_owner_wildcard_array_entry() {
11631219
let parsed = parse_scope(ReposValue::ScopedList(vec!["octocat/*".to_string()]))
@@ -1352,7 +1408,7 @@ mod tests {
13521408
}
13531409

13541410
#[test]
1355-
fn infer_scope_for_baseline_uses_github_scope_for_notification_management_tools() {
1411+
fn infer_scope_for_baseline_uses_user_scope_for_notification_management_tools() {
13561412
let tool_args = json!({ "threadId": "123" });
13571413
for tool in &[
13581414
"dismiss_notification",
@@ -1363,8 +1419,14 @@ mod tests {
13631419
let inferred = infer_scope_for_baseline(tool, &tool_args, "");
13641420
assert_eq!(
13651421
inferred,
1366-
scope_names::GITHUB,
1367-
"{} should infer github baseline scope",
1422+
scope_names::USER,
1423+
"{} should infer user baseline scope",
1424+
tool
1425+
);
1426+
assert_eq!(
1427+
infer_scope_for_baseline(tool, &tool_args, "github/gh-aw-mcpg"),
1428+
scope_names::USER,
1429+
"{} should keep user baseline scope even with repo context",
13681430
tool
13691431
);
13701432
}
@@ -1409,8 +1471,37 @@ mod tests {
14091471

14101472
assert_eq!(
14111473
after_baseline,
1412-
labels::project_github_label(&ctx),
1413-
"{} integrity should remain github-scoped after baseline enforcement",
1474+
labels::writer_integrity(scope_names::USER, &ctx),
1475+
"{} integrity should remain user-scoped after baseline enforcement",
1476+
tool
1477+
);
1478+
}
1479+
}
1480+
1481+
#[test]
1482+
fn star_repository_integrity_remains_user_scoped_with_repo_args() {
1483+
let ctx = PolicyContext::default();
1484+
let tool_args = json!({ "owner": "github", "repo": "gh-aw-mcpg" });
1485+
let repo_id = "github/gh-aw-mcpg";
1486+
1487+
for tool in &["star_repository", "unstar_repository"] {
1488+
let (_, integrity, _) = labels::apply_tool_labels(
1489+
tool,
1490+
&tool_args,
1491+
repo_id,
1492+
vec![],
1493+
vec![],
1494+
String::new(),
1495+
&ctx,
1496+
);
1497+
let baseline_scope = infer_scope_for_baseline(tool, &tool_args, repo_id);
1498+
let after_baseline =
1499+
labels::ensure_integrity_baseline(&baseline_scope, integrity, &ctx);
1500+
1501+
assert_eq!(
1502+
after_baseline,
1503+
labels::writer_integrity(scope_names::USER, &ctx),
1504+
"{} integrity should remain user-scoped after baseline enforcement",
14141505
tool
14151506
);
14161507
}
@@ -1484,6 +1575,11 @@ mod tests {
14841575
"github",
14851576
"{tool} should infer org baseline scope from owner-only synthetic CLI args"
14861577
);
1578+
assert_eq!(
1579+
infer_scope_for_baseline(tool, &json!({}), "github/gh-aw-mcpg"),
1580+
"github/gh-aw-mcpg",
1581+
"{tool} should preserve repo baseline scope when repo context is present"
1582+
);
14871583
}
14881584

14891585
for tool in &["set_secret", "delete_secret"] {

guards/github-guard/rust-guard/src/tools.rs

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,6 @@ pub const WRITE_OPERATIONS: &[&str] = &[
5757
"label_write",
5858
"lock_issue", // gh issue lock
5959
"lock_pull_request", // gh pr lock
60-
"manage_notification_subscription",
61-
"manage_repository_notification_subscription",
6260
"mark_all_notifications_read",
6361
"mark_project_template", // gh project mark-template — GraphQL markProjectV2AsTemplate
6462
"projects_write",
@@ -140,6 +138,8 @@ pub const READ_WRITE_OPERATIONS: &[&str] = &[
140138
"issue_dependency_write", // GraphQL addBlockedBy/removeBlockedBy after resolving issue IDs
141139
"issue_write",
142140
"issue_write_ff_remote_mcp_issue_fields", // feature-flag variant of issue_write
141+
"manage_notification_subscription",
142+
"manage_repository_notification_subscription",
143143
"merge_pull_request",
144144
"pull_request_review_write",
145145
"remove_sub_issue", // DELETE/POST — remove sub-issue link
@@ -730,6 +730,50 @@ mod tests {
730730
}
731731
}
732732

733+
#[test]
734+
fn test_notification_and_star_tools_match_upstream_write_classification() {
735+
for op in &[
736+
"dismiss_notification",
737+
"mark_all_notifications_read",
738+
"star_repository",
739+
"unstar_repository",
740+
] {
741+
assert!(
742+
WRITE_OPERATIONS.binary_search(op).is_ok(),
743+
"{op} must be explicitly listed in WRITE_OPERATIONS"
744+
);
745+
assert!(
746+
is_write_operation(op),
747+
"{op} must be classified as a write operation"
748+
);
749+
assert!(
750+
!is_read_write_operation(op),
751+
"{op} should not be in READ_WRITE_OPERATIONS"
752+
);
753+
}
754+
}
755+
756+
#[test]
757+
fn test_notification_subscription_tools_match_upstream_read_write_classification() {
758+
for op in &[
759+
"manage_notification_subscription",
760+
"manage_repository_notification_subscription",
761+
] {
762+
assert!(
763+
READ_WRITE_OPERATIONS.binary_search(op).is_ok(),
764+
"{op} must be explicitly listed in READ_WRITE_OPERATIONS"
765+
);
766+
assert!(
767+
is_read_write_operation(op),
768+
"{op} must be classified as a read-write operation"
769+
);
770+
assert!(
771+
!is_write_operation(op),
772+
"{op} should not be in WRITE_OPERATIONS"
773+
);
774+
}
775+
}
776+
733777
#[test]
734778
fn test_is_merge_operation() {
735779
assert!(is_merge_operation("merge_pull_request"));

0 commit comments

Comments
 (0)