Skip to content

Commit 1aeb8fb

Browse files
authored
Rust guard: remove list_commits secrecy clone and add project-item path labeling regressions (#6941)
This tightens two remaining weak spots in `response_paths.rs`: `list_commits` still deep-cloned secrecy labels on every call, and the `list_project_items` / `projects_list` path-labeling branch lacked direct regression coverage for its security-relevant edge cases. - **`list_commits`: eliminate the last `Vec<String>` deep clone** - Convert repo secrecy to `SharedLabels` immediately. - Reuse it via cheap `Arc` clones for per-item labels. - Move the same shared value into `default_labels` on its final use. ```rust let default_secrecy: crate::SharedLabels = repo_visibility_secrecy(&arg_owner, &arg_repo, &default_repo, ctx).into(); // per-item secrecy: default_secrecy.clone(), // default labels secrecy: default_secrecy, ``` - **Project-item response-path regressions** - Add coverage for the fail-secure branch when an `ISSUE` item has no repo context. - Assert `DRAFT_ISSUE` stays owner-writer integrity with empty secrecy. - Assert `projects_list` remains behaviorally identical to `list_project_items`. - **`items_path` contract** - Add an explicit assertion that `{ "items": [...] }` responses propagate `items_path == Some("/items")` and produce `/items/{n}` entry paths. These changes keep the response-labeling behavior unchanged where already correct, while removing unnecessary allocation/copying in commit labeling and documenting the intended contract around heterogeneous project items.
2 parents 71ebfed + 3804236 commit 1aeb8fb

1 file changed

Lines changed: 98 additions & 6 deletions

File tree

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

Lines changed: 98 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -338,8 +338,8 @@ pub fn label_response_paths(
338338
} else {
339339
String::new()
340340
};
341-
let default_secrecy =
342-
repo_visibility_secrecy(&arg_owner, &arg_repo, &default_repo, ctx);
341+
let default_secrecy: crate::SharedLabels =
342+
repo_visibility_secrecy(&arg_owner, &arg_repo, &default_repo, ctx).into();
343343
let repo_private = if !arg_owner.is_empty() && !arg_repo.is_empty() {
344344
match super::backend::is_repo_private(&arg_owner, &arg_repo) {
345345
Some(value) => value,
@@ -351,8 +351,6 @@ pub fn label_response_paths(
351351

352352
// Commits on default branch (main/master) get merged-level integrity
353353
let is_default_branch = is_default_branch_ref(sha);
354-
let default_secrecy_shared: crate::SharedLabels = default_secrecy.clone().into();
355-
356354
let limited_items = limit_items_with_log(items, "list_commits");
357355
let mut labeled_paths = Vec::with_capacity(limited_items.len());
358356

@@ -380,7 +378,7 @@ pub fn label_response_paths(
380378
path: format!("/{}", i),
381379
labels: crate::ResourceLabels {
382380
description: format!("commit:{}@{}", repo_for_labels, short_sha),
383-
secrecy: default_secrecy_shared.clone(),
381+
secrecy: default_secrecy.clone(),
384382
integrity: integrity.into(),
385383
},
386384
});
@@ -390,7 +388,7 @@ pub fn label_response_paths(
390388
labeled_paths,
391389
default_labels: Some(crate::ResourceLabels {
392390
description: "commit".to_string(),
393-
secrecy: default_secrecy.into(),
391+
secrecy: default_secrecy,
394392
integrity: if is_default_branch {
395393
merged_integrity(&default_repo, ctx)
396394
} else if repo_private {
@@ -905,4 +903,98 @@ mod tests {
905903
default_integrity
906904
);
907905
}
906+
907+
#[test]
908+
fn list_project_items_missing_repo_fails_secure_and_forwards_items_path() {
909+
let tool_args = json!({"owner": "octocat"});
910+
let response = json!({
911+
"items": [{
912+
"type": "ISSUE",
913+
"content": {
914+
"author_association": "CONTRIBUTOR"
915+
}
916+
}]
917+
});
918+
919+
let result = label_response_paths("list_project_items", &tool_args, &response, &ctx())
920+
.expect("list_project_items should produce path labels");
921+
922+
assert_eq!(result.items_path, Some("/items"));
923+
assert_eq!(result.labeled_paths.len(), 1);
924+
925+
let entry = &result.labeled_paths[0];
926+
assert_eq!(entry.path, "/items/0");
927+
assert_eq!(entry.labels.description, "project-item:issue");
928+
assert_eq!(
929+
entry.labels.secrecy,
930+
policy_private_scope_label("octocat", "", "", &ctx()),
931+
"missing repo context should fail secure"
932+
);
933+
assert_eq!(
934+
entry.labels.integrity,
935+
author_association_floor_from_str("octocat", Some("CONTRIBUTOR"), &ctx()),
936+
"missing repo context should fall back to owner-scoped association integrity"
937+
);
938+
}
939+
940+
#[test]
941+
fn list_project_items_draft_issue_gets_writer_integrity_and_empty_secrecy() {
942+
let tool_args = json!({"owner": "octocat"});
943+
let response = json!({
944+
"items": [{
945+
"type": "DRAFT_ISSUE",
946+
"title": "todo item"
947+
}]
948+
});
949+
950+
let result = label_response_paths("list_project_items", &tool_args, &response, &ctx())
951+
.expect("list_project_items should produce path labels");
952+
953+
assert_eq!(result.labeled_paths.len(), 1);
954+
let entry = &result.labeled_paths[0];
955+
assert_eq!(entry.labels.description, "project-item:draft_issue");
956+
assert!(entry.labels.secrecy.is_empty(), "draft issue should have empty secrecy");
957+
assert_eq!(
958+
entry.labels.integrity,
959+
writer_integrity("octocat", &ctx()),
960+
"draft issue should use owner-scoped writer integrity"
961+
);
962+
}
963+
964+
#[test]
965+
fn projects_list_alias_matches_list_project_items() {
966+
let tool_args = json!({"owner": "octocat"});
967+
let response = json!({
968+
"items": [{
969+
"type": "PULL_REQUEST",
970+
"content": {
971+
"repository_url": "https://api.github.com/repos/octocat/hello-world",
972+
"author_association": "MEMBER"
973+
}
974+
}]
975+
});
976+
977+
let list_project_items =
978+
label_response_paths("list_project_items", &tool_args, &response, &ctx())
979+
.expect("list_project_items should produce path labels");
980+
let projects_list = label_response_paths("projects_list", &tool_args, &response, &ctx())
981+
.expect("projects_list should produce path labels");
982+
983+
assert_eq!(
984+
list_project_items.items_path, projects_list.items_path,
985+
"alias should preserve items_path"
986+
);
987+
assert_eq!(
988+
list_project_items.labeled_paths.len(),
989+
projects_list.labeled_paths.len(),
990+
"alias should produce the same number of labeled paths"
991+
);
992+
993+
let left = &list_project_items.labeled_paths[0];
994+
let right = &projects_list.labeled_paths[0];
995+
assert_eq!(left.path, right.path);
996+
assert_eq!(left.labels.description, right.labels.description);
997+
assert_eq!(left.labels.secrecy, right.labels.secrecy);
998+
assert_eq!(left.labels.integrity, right.labels.integrity);
999+
}
9081000
}

0 commit comments

Comments
 (0)