Conversation
…and repo creation writes Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/52471578-c908-4c54-a0a3-4ed6d7167541 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
TestBinaryInvocation_RoutedMode and TestBinaryInvocation_UnifiedMode were using 'docker run alpine:latest echo' which fails in CI environments where Docker image pulls are blocked by firewall rules. Replaced with in-process mock MCP HTTP backends (using the existing createMinimalMockMCPBackend helper) and JSON stdin config, matching the pattern used by other integration tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the GitHub guard’s DIFC labeling by adding explicit secrecy/integrity semantics for previously ambiguous write tools (notification management and repo create/fork), and updates unit tests to lock in the intended behavior.
Changes:
- Split notification read tools from notification management write tools, giving management writes explicit
S=[]and github-scoped integrity. - Split
create_repository/fork_repositoryfrom repo-content writes and assign explicit account-scoped write labels. - Update/expand label-rule unit tests for notification management, fork, and create-repo expectations.
Show a summary per file
| File | Description |
|---|---|
guards/github-guard/rust-guard/src/labels/tool_rules.rs |
Adds new match arms to enforce explicit DIFC labels for notification management and repo create/fork. |
guards/github-guard/rust-guard/src/labels/mod.rs |
Updates tests to reflect the new explicit tool-label semantics and expands coverage for the affected tools. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
guards/github-guard/rust-guard/src/labels/tool_rules.rs:589
- Same baseline-scope issue as above: this arm sets github-scoped writer integrity (
writer_integrity("github", ctx)), butlabel_resourcere-appliesensure_integrity_baseline()usinginfer_scope_for_baseline()which returns""whenrepo_idis empty. That second pass will downgrade the github-scoped integrity labels to unscopednone, making these explicit DIFC semantics ineffective at runtime. Updateinfer_scope_for_baseline()to return"github"forcreate_repository/fork_repository(or otherwise prevent the second baseline pass from clobbering tool-specific baseline_scope).
// === Repository creation/fork (user/org-scoped writes) ===
"create_repository" | "fork_repository" => {
// Creating/forking repositories is account-scoped and does not return repo content.
// S = public (empty); I = writer(github)
secrecy = vec![];
baseline_scope = "github".to_string();
integrity = writer_integrity("github", ctx);
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
| // === Notification management (account-scoped writes) === | ||
| "dismiss_notification" | ||
| | "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 = "github".to_string(); | ||
| integrity = project_github_label(ctx); | ||
| } |
There was a problem hiding this comment.
apply_tool_labels sets baseline_scope = "github" and returns github-scoped writer integrity for these notification management tools, but label_resource later re-applies ensure_integrity_baseline() using infer_scope_for_baseline(). For these tools repo_id is typically empty and infer_scope_for_baseline() returns "", which causes max_integrity("", ...) to downgrade the returned github-scoped integrity tags to unscoped none. Fix by teaching infer_scope_for_baseline() to return "github" for these tool names (or by removing/avoiding the second baseline enforcement so it can’t clobber tool-specific baseline_scope).
This issue also appears on line 582 of the same file.
| fn test_apply_tool_labels_notification_management_public_project_github() { | ||
| let ctx = default_ctx(); | ||
| let tool_args = json!({ "threadId": "123" }); | ||
|
|
||
| let (secrecy, integrity, _desc) = apply_tool_labels( | ||
| for tool in &[ | ||
| "dismiss_notification", | ||
| &tool_args, | ||
| "", | ||
| vec![], | ||
| vec![], | ||
| String::new(), | ||
| &ctx, | ||
| ); | ||
| "mark_all_notifications_read", | ||
| "manage_notification_subscription", | ||
| "manage_repository_notification_subscription", | ||
| ] { | ||
| let (secrecy, integrity, _desc) = | ||
| apply_tool_labels(tool, &tool_args, "", vec![], vec![], String::new(), &ctx); | ||
|
|
||
| assert_eq!(secrecy, private_user_label(), "dismiss_notification must carry private:user secrecy"); | ||
| assert_eq!(integrity, none_integrity("", &ctx), "dismiss_notification should have none-level integrity (references unknown content)"); | ||
| assert!( | ||
| secrecy.is_empty(), | ||
| "{} should have empty (public) secrecy", | ||
| tool | ||
| ); | ||
| assert_eq!( | ||
| integrity, | ||
| project_github_label(&ctx), | ||
| "{} should have project:github integrity", | ||
| tool | ||
| ); | ||
| } |
There was a problem hiding this comment.
These tests assert apply_tool_labels() output directly, but the production path in label_resource re-applies ensure_integrity_baseline() using infer_scope_for_baseline(). For tools with empty repo_id, that second pass can change the final integrity labels (including potentially downgrading github-scoped integrity to unscoped none). Add coverage that exercises label_resource (or at least infer_scope_for_baseline for these tool names) so the intended end-to-end DIFC behavior is locked in.
|
@copilot address this review feedback #4300 (review) |
…creation writes Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/0114d8b5-ccf7-4dee-b9d7-630f4545b568 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
The guard’s write classification was complete, but several mutating GitHub MCP tools could still inherit caller-provided DIFC labels via fallback behavior. This change removes that ambiguity for high-impact write paths by defining explicit secrecy/integrity outcomes.
Notification write operations now have explicit DIFC semantics
apply_tool_labelshandling for:dismiss_notificationmark_all_notifications_readmanage_notification_subscriptionmanage_repository_notification_subscriptionS = public([])I = project:githubgithubRepository creation/fork operations split from repo-content writes
create_repositoryandfork_repositoryout of repo-content mutation grouping.S = public([])I = writer(github)githubCoverage updates in label-rule tests
fork_repositoryexpectations to github-scoped writer integrity/public secrecy.create_repositorytest to lock intended DIFC behavior.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build1964592199/b509/launcher.test /tmp/go-build1964592199/b509/launcher.test -test.testlogfile=/tmp/go-build1964592199/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.9/x-p go x_amd64/vet(dns block)/tmp/go-build2556502756/b513/launcher.test /tmp/go-build2556502756/b513/launcher.test -test.testlogfile=/tmp/go-build2556502756/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� .rlib 6b2b26a06.rlib 327.rlib 653.rlib z8tylr32cgwnziux/tmp/go-build197148868/b453/vet.cfg ruczucboywd9kbz6.0ufrg49.rcgu.o aiea5rg1toc6oekg.0ufrg49.rcgu.o fnr5�� p1agk9if6se1ud54.0ufrg49.rcgu.o fc88gtczrpthgg1u.0ufrg49.rcgu.o x_amd64/vet 5vmlrtxer9j2lnp9/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/link zucrirrh7hnf4z1l-o o4qlefxtp7qruodt/tmp/go-build2556502756/b507/guard.test x_amd64/vet(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build1964592199/b491/config.test /tmp/go-build1964592199/b491/config.test -test.testlogfile=/tmp/go-build1964592199/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-p go x_amd64/compile(dns block)/tmp/go-build2556502756/b495/config.test /tmp/go-build2556502756/b495/config.test -test.testlogfile=/tmp/go-build2556502756/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o ithub-guard/rust-guard/target/debug/build/serde-0c0d3ac83fe3437f/rustcMFMlVN/symbols.o ithub-guard/rust-guard/target/debug/build/serde-0c0d3ac83fe3437f/build_script_build-0c0d3ac83fe3/tmp/go-build197148868/b384/vet.cfg ithub-guard/rust-guard/target/debug/build/serde-0c0d3ac83fe3437f/build_script_build-0c0d3ac83fe3437f.build_scrip/tmp/go-build1020409741/b001/exe/a.out ithub-guard/rust/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet ild-e2b702800175/tmp/go-build197148868/b292/vet.cfg ild-e2b702800175437e.f3itfh70g07-m known-linux-gnu/lib/rustlib/x86_fix(guard): enforce explicit DIFC labels for notification management and repo cr-buildid=dcc-Xk-BVFiljIesDi3n/H9wQikqwjRQnx90KP6dH/hKNTvz9caF-1eL6QKuz0/dcc-Xk-B-unsafeptr=false know�� known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libobject-926daa94a00ee327.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libmemchr-48d5b0db80402653.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libaddr2line-3367f26bd486b29d.rlib known-linux-gnu//opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet known-linux-gnu//tmp/go-build197148868/b425/vet.cfg known-linux-gnu/lib/rustlib/x86_/tmp/go-build197148868/b039/vet.cfg known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libstd_detect-b16e5cb5eba3e0fd.rlib(dns block)nonexistent.local/tmp/go-build1964592199/b509/launcher.test /tmp/go-build1964592199/b509/launcher.test -test.testlogfile=/tmp/go-build1964592199/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.9/x-p go x_amd64/vet(dns block)/tmp/go-build2556502756/b513/launcher.test /tmp/go-build2556502756/b513/launcher.test -test.testlogfile=/tmp/go-build2556502756/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� .rlib 6b2b26a06.rlib 327.rlib 653.rlib z8tylr32cgwnziux/tmp/go-build197148868/b453/vet.cfg ruczucboywd9kbz6.0ufrg49.rcgu.o aiea5rg1toc6oekg.0ufrg49.rcgu.o fnr5�� p1agk9if6se1ud54.0ufrg49.rcgu.o fc88gtczrpthgg1u.0ufrg49.rcgu.o x_amd64/vet 5vmlrtxer9j2lnp9/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/link zucrirrh7hnf4z1l-o o4qlefxtp7qruodt/tmp/go-build2556502756/b507/guard.test x_amd64/vet(dns block)slow.example.com/tmp/go-build1964592199/b509/launcher.test /tmp/go-build1964592199/b509/launcher.test -test.testlogfile=/tmp/go-build1964592199/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.9/x-p go x_amd64/vet(dns block)/tmp/go-build2556502756/b513/launcher.test /tmp/go-build2556502756/b513/launcher.test -test.testlogfile=/tmp/go-build2556502756/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� .rlib 6b2b26a06.rlib 327.rlib 653.rlib z8tylr32cgwnziux/tmp/go-build197148868/b453/vet.cfg ruczucboywd9kbz6.0ufrg49.rcgu.o aiea5rg1toc6oekg.0ufrg49.rcgu.o fnr5�� p1agk9if6se1ud54.0ufrg49.rcgu.o fc88gtczrpthgg1u.0ufrg49.rcgu.o x_amd64/vet 5vmlrtxer9j2lnp9/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/link zucrirrh7hnf4z1l-o o4qlefxtp7qruodt/tmp/go-build2556502756/b507/guard.test x_amd64/vet(dns block)this-host-does-not-exist-12345.com/tmp/go-build1964592199/b518/mcp.test /tmp/go-build1964592199/b518/mcp.test -test.testlogfile=/tmp/go-build1964592199/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true proto/proto.go s/alert.go x_amd64/compile -o lts /tmp/cc9cHcxs.s x_amd64/compile -I g_.a 4592199/b165/ x_amd64/vet --gdwarf-5 ternal/sys -o x_amd64/vet(dns block)/tmp/go-build2556502756/b522/mcp.test /tmp/go-build2556502756/b522/mcp.test -test.testlogfile=/tmp/go-build2556502756/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s ithu�� ithub-guard/rust-guard/target/debug/deps/github_guard-57d41235e07a5585.3r7b32ab9hw1mmy6a1aekmmsh/usr/libexec/docker/docker-init ithub-guard/rust-guard/target/debug/deps/github_guard-57d41235e07a5585.485oswk5h4p2kq4dabhq5b2ke--version x_amd64/vet 6e1dc71b.rlib m/github/gh-aw-m--version -nilfunc x_amd64/vet n-me�� aw-mcpg/guards/github-guard/rust-guard/target/debug/deps/rustcq8hDXS/symbols.o aw-mcpg/guards/github-guard/rust-guard/target/debug/deps/github_guard-57d41235e07a5585.0r6f2y9pm/usr/bin/runc x_amd64/vet aw-mcpg/guards/gbash aw-mcpg/guards/g/usr/bin/runc aw-mcpg/guards/g--version x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either: