Skip to content

Commit 4c7872d

Browse files
authored
Add pre-emptive guard entries for 3 CLI write operations missing from WRITE_OPERATIONS (#3609)
Three GitHub CLI write operations had no guard coverage — meaning if a corresponding MCP tool lands in github-mcp-server, it would pass through unclassified. All three are high/medium risk operations involving repo settings, PR reversion, and SSH key grants. ### `tools.rs` — WRITE_OPERATIONS additions - `edit_repository` — `PATCH /repos/{owner}/{repo}`; can flip visibility private→public - `revert_pull_request` — GraphQL `revertPullRequest`; creates branch + PR - `add_deploy_key` / `delete_deploy_key` — `POST/DELETE /repos/{owner}/{repo}/keys`; grants persistent SSH access ### `tool_rules.rs` — DIFC label rules ```rust "edit_repository" => { secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx); integrity = writer_integrity(repo_id, ctx); } "revert_pull_request" => { secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx); integrity = writer_integrity(repo_id, ctx); } "add_deploy_key" | "delete_deploy_key" => { // Always private regardless of repo visibility — deploy key secrets are sensitive secrecy = policy_private_scope_label(&owner, &repo, repo_id, ctx); integrity = writer_integrity(repo_id, ctx); } ``` Deploy key operations use `policy_private_scope_label` unconditionally (not `apply_repo_visibility_secrecy`) since the key material itself is always sensitive regardless of whether the repo is public. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build3092088994/b514/launcher.test /tmp/go-build3092088994/b514/launcher.test -test.testlogfile=/tmp/go-build3092088994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg Bmen/MKrSXkkW-_Q-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet 9006�� .cfg olang.org/grpc@v1.80.0/internal/transport/client_stream.go x_amd64/vet --gdwarf-5 g/grpc/attribute-qE -o x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3092088994/b496/config.test /tmp/go-build3092088994/b496/config.test -test.testlogfile=/tmp/go-build3092088994/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3092088994/b323/vet.cfg 5.0/deviceauth.g-errorsas 5.0/oauth2.go x_amd64/vet --gdwarf-5 nal/protolazy -o x_amd64/vet -I g_.a -I x_amd64/vet --gdwarf-5 go-sdk/auth -o x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3092088994/b514/launcher.test /tmp/go-build3092088994/b514/launcher.test -test.testlogfile=/tmp/go-build3092088994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg Bmen/MKrSXkkW-_Q-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet 9006�� .cfg olang.org/grpc@v1.80.0/internal/transport/client_stream.go x_amd64/vet --gdwarf-5 g/grpc/attribute-qE -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3092088994/b514/launcher.test /tmp/go-build3092088994/b514/launcher.test -test.testlogfile=/tmp/go-build3092088994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg Bmen/MKrSXkkW-_Q-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet 9006�� .cfg olang.org/grpc@v1.80.0/internal/transport/client_stream.go x_amd64/vet --gdwarf-5 g/grpc/attribute-qE -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3092088994/b523/mcp.test /tmp/go-build3092088994/b523/mcp.test -test.testlogfile=/tmp/go-build3092088994/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3092088994/b517/_pkg_.a elemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp@v1.43.0/internal/otlpconfig/envconfig.go/usr/bin/runc.original elemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp@v1.43.0/internal/otlpconfig/options.go x_amd64/vet --gdwarf-5 g/grpc/internal -o x_amd64/vet -obj�� .cfg -importpath x_amd64/vet -ldflags=&#34;-O2&#34; &#34;/usr/libexec/docker/docker-init ecosystem/grpc-g--version -I x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents 987b125 + 6c00147 commit 4c7872d

File tree

3 files changed

+128
-0
lines changed

3 files changed

+128
-0
lines changed

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

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4712,4 +4712,84 @@ mod tests {
47124712

47134713
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "fork_repository should have writer integrity");
47144714
}
4715+
4716+
#[test]
4717+
fn test_apply_tool_labels_edit_repository_writer_integrity() {
4718+
let ctx = default_ctx();
4719+
let repo_id = "github/copilot";
4720+
let tool_args = json!({
4721+
"owner": "github",
4722+
"repo": "copilot",
4723+
});
4724+
4725+
let (_secrecy, integrity, _desc) = apply_tool_labels(
4726+
"edit_repository",
4727+
&tool_args,
4728+
repo_id,
4729+
vec![],
4730+
vec![],
4731+
String::new(),
4732+
&ctx,
4733+
);
4734+
4735+
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "edit_repository should have writer integrity");
4736+
}
4737+
4738+
#[test]
4739+
fn test_apply_tool_labels_revert_pull_request_writer_integrity() {
4740+
let ctx = default_ctx();
4741+
let repo_id = "github/copilot";
4742+
let tool_args = json!({
4743+
"owner": "github",
4744+
"repo": "copilot",
4745+
"pullNumber": 42,
4746+
});
4747+
4748+
let (_secrecy, integrity, _desc) = apply_tool_labels(
4749+
"revert_pull_request",
4750+
&tool_args,
4751+
repo_id,
4752+
vec![],
4753+
vec![],
4754+
String::new(),
4755+
&ctx,
4756+
);
4757+
4758+
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "revert_pull_request should have writer integrity");
4759+
}
4760+
4761+
#[test]
4762+
fn test_apply_tool_labels_deploy_key_operations_private_secrecy() {
4763+
let ctx = default_ctx();
4764+
let repo_id = "github/copilot";
4765+
let tool_args = json!({
4766+
"owner": "github",
4767+
"repo": "copilot",
4768+
});
4769+
4770+
for tool_name in &["add_deploy_key", "delete_deploy_key"] {
4771+
let (secrecy, integrity, _desc) = apply_tool_labels(
4772+
tool_name,
4773+
&tool_args,
4774+
repo_id,
4775+
vec![],
4776+
vec![],
4777+
String::new(),
4778+
&ctx,
4779+
);
4780+
4781+
assert_eq!(
4782+
secrecy,
4783+
super::helpers::policy_private_scope_label("github", "copilot", repo_id, &ctx),
4784+
"{} should have private-scoped secrecy",
4785+
tool_name
4786+
);
4787+
assert_eq!(
4788+
integrity,
4789+
writer_integrity(repo_id, &ctx),
4790+
"{} should have writer integrity",
4791+
tool_name
4792+
);
4793+
}
4794+
}
47154795
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,31 @@ pub fn apply_tool_labels(
644644
integrity = writer_integrity(repo_id, ctx);
645645
}
646646

647+
// === Repository settings edit (can change visibility) ===
648+
"edit_repository" => {
649+
// Can change repo visibility, security settings, default branch.
650+
// S = S(repo); I = writer (requires admin access)
651+
secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx);
652+
integrity = writer_integrity(repo_id, ctx);
653+
}
654+
655+
// === PR revert (creates revert branch + PR) ===
656+
"revert_pull_request" => {
657+
// Creates a new branch + PR reverting a merged PR.
658+
// S = S(repo); I = writer
659+
secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx);
660+
integrity = writer_integrity(repo_id, ctx);
661+
}
662+
663+
// === Deploy key management (SSH key with optional write access) ===
664+
"add_deploy_key" | "delete_deploy_key" => {
665+
// Manages SSH deploy keys — `add_deploy_key` may grant persistent write access.
666+
// S = at least private; scope is policy-dependent (may be unscoped, owner-scoped, or repo-scoped)
667+
// I = writer (requires admin access)
668+
secrecy = policy_private_scope_label(&owner, &repo, repo_id, ctx);
669+
integrity = writer_integrity(repo_id, ctx);
670+
}
671+
647672
// === Star/unstar operations (public metadata) ===
648673
"star_repository" | "unstar_repository" => {
649674
// Starring is a public action; response is minimal metadata.

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,13 @@ pub const WRITE_OPERATIONS: &[&str] = &[
5151
"rerun_workflow_run", // gh run rerun — reruns a completed workflow run
5252
"rerun_failed_jobs", // gh run rerun --failed — reruns only failed jobs
5353
"rerun_workflow_job", // gh run rerun --job — reruns a specific job
54+
// Pre-emptive: gh repo edit (PATCH /repos/{owner}/{repo}) — can change visibility, security settings
55+
"edit_repository",
56+
// Pre-emptive: gh pr revert (GraphQL revertPullRequest) — creates revert branch + PR
57+
"revert_pull_request",
58+
// Pre-emptive: gh repo deploy-key add/delete — SSH key with optional write access
59+
"add_deploy_key",
60+
"delete_deploy_key",
5461
];
5562

5663
/// Read-write operations that both read and modify data
@@ -210,6 +217,22 @@ mod tests {
210217
}
211218
}
212219

220+
#[test]
221+
fn test_cli_gap_operations_are_write_operations() {
222+
for op in &[
223+
"edit_repository",
224+
"revert_pull_request",
225+
"add_deploy_key",
226+
"delete_deploy_key",
227+
] {
228+
assert!(
229+
is_write_operation(op),
230+
"{} must be classified as a write operation",
231+
op
232+
);
233+
}
234+
}
235+
213236
#[test]
214237
fn test_create_agent_task_is_read_write_and_blocked() {
215238
assert!(

0 commit comments

Comments
 (0)