Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add formal GitHub MCP access-control conformance model and predicate-mapped test suite #44790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Add formal GitHub MCP access-control conformance model and predicate-mapped test suite #44790
Changes from 2 commits
3c229e92b1960285baa4d4bb273bacfb1e0a25bf88efd3d8e4ad7b7912cdbe947b34cfFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
TestFormal_ToolNameFilterhas an implicit assumption that an emptyToolNameis always allowed — but this is untested and undocumented. Line 206 (req.ToolName != "") silently permits any tool whenToolNameis the zero value, which could be a security gap if callers accidentally omit the field.💡 Suggested test
If the intent is to allow empty tool names (e.g., for non-tool requests), document that in a comment on line 206.
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. Thereq.ToolName != ""short-circuit was removed. The tool check is nowlen(cfg.AllowedTools) > 0 && !containsExact(cfg.AllowedTools, req.ToolName), so an empty tool name against a non-empty allowlist correctly denies.TestFormal_ToolNameFiltercovers this case explicitly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formal test suite is intentionally scoped to the decision model documented in
specs/github-mcp-access-control-compliance/README.md: it tests the guard predicates (P1–P6), first-failing-guard error-code semantics, and safety invariants that the spec defines. It is not an integration test of the production gateway — that would require loading compliance fixtures and the actual compiler/gateway, which is a separate concern beyond the scope of this predicate-mapping PR. The PR title and README section "Formal Model" make this boundary explicit. Integration coverage of the real implementation remains a future work item in the spec's §11.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard evaluation order contradicts the formal model
The README formal model defines the conjunction as:
P1_RepoMatch ∧ P2_RoleAllow ∧ P3_PrivateRepoAllow ∧ P4_ToolAllowed ∧ P5_NotBlocked ∧ P6_IntegrityMetBut this evaluator checks
BlockedUsersfirst, before the repo check:This means if a blocked user requests an unallowed repo, the code returns
formalErrorBlockedUser(-32004), but the spec's first-failing-guard rule says P1_RepoMatch fails first, so it should returnformalErrorRepoNotAllowed(-32001). Either:BlockedUsers-firstsemantics (which is a reasonable safety-first design choice).@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. The evaluator now checks guards in the documented order: tool → repo → role → private-repo → blocked-user → integrity. The README formal model (P1_ToolAllowedthroughP6_IntegrityMet) and the evaluator order are now consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] Guard evaluation order in
formalEvaluateAccesscontradicts the README formal model:BlockedUsersis checked first (line 196), but the spec listsP5_NotBlockedas the fifth guard. A request that fails both repo and blocked-user checks returns-32004(blocked user) rather than-32001(repo not allowed), breaking the documented first-failing-guard semantics.💡 Fix suggestion
Either reorder
formalEvaluateAccessso the blocked-user check is at position 5 (matching P1..P6 in the spec), or update the README to declare blocked-user checking as P1.Add a regression test that explicitly documents the intended order:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. Guard evaluation order is now: tool → repo → role → private-repo → blocked-user → integrity, matching both the README formal model (P1–P6) and the spec §4.5.3 order.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocked-user guard order contradicts the formal spec: the implementation checks
BlockedUsersfirst (before repo match), but the README definesP5_NotBlockedas the 5th predicate afterP1_RepoMatch–P4_ToolAllowed.💡 Why this matters
The README states:
ALLOW(r,c) ≜ P1_RepoMatch ∧ P2_RoleAllow ∧ P3_PrivateRepoAllow ∧ P4_ToolAllowed ∧ P5_NotBlocked ∧ P6_IntegrityMet.It also says: _The
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. Blocked-user check now occurs after tool, repo, role, and private-repo guards, matching the spec's integrity-management phase (P5_NotBlocked).TestFormal_ErrorCodeFirstFailingGuardhas a table case for each guard as the first failure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty
Rolesslice vs nil semantics undocumented and potentially dangerous:len(cfg.Roles) > 0treats bothniland[]string{}as \noThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tool allowlist silently bypassed when
ToolNameis empty: a request with an empty tool name skips theAllowedToolsrestriction entirely and is allowed.💡 Detail and suggested fix
The current guard is:
When
req.ToolName == ""the whole condition short-circuits and the allowlist is never consulted. Any caller that forgets to setToolName(or deliberately sends an empty name) bypasses an explicitly configured allowlist — a fail-open security regression.Correct behavior: if
AllowedToolsis configured, an emptyToolNameshould be denied:There is also no test that verifies this case — add one that asserts an empty
ToolNameis denied whenAllowedToolsis non-empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. Thereq.ToolName != ""guard was removed, so the tool allowlist check islen(cfg.AllowedTools) > 0 && !containsExact(cfg.AllowedTools, req.ToolName). An emptyToolNamewith a non-empty allowlist correctly denies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
formalIntegrityRankreturns-1for unknown integrity levels (line 246), but no test covers this case. A request with an unrecognisedContentIntegrityvalue (e.g.,"pending") against a config withMinIntegrity: "approved"would returnformalErrorIntegrityTooLowsilently — but whether that is the intended invariant is undocumented.💡 Suggested test
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
TestFormal_UnknownIntegrityLevelDeniedin commitfix: make unknown MinIntegrity fail-safe and add unknown integrity level tests. The test covers: (1) an unknownContentIntegrityvalue ("pending") against a known threshold denies withformalErrorIntegrityTooLow, and (2) an unrecognizedMinIntegrityconfiguration ("invalid") is treated as fail-safe and denies all requests. TheformalIntegrityRankrank-based comparison is now documented via the evaluator's split conditions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
formalRepositoryAllowedonly handles glob patterns where the wildcard is in the repo name segment (e.g.owner/prefix-*). A pattern like*/gh-aw(wildcard owner) is not tested and the switch statement has no case for it — the function would fall through and returnfalsefor*/gh-awmatchinggithub/gh-aw.💡 Suggested test to document the gap
If owner wildcards should be supported, add a case to the switch in
formalRepositoryAllowed.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrecognized
MinIntegrityvalue silently disables the integrity guard (fail-open):formalIntegrityRankreturns-1for any unknown string, so a misspelledMinIntegrity(e.g."Approved"instead of"approved") causes all requests to pass the integrity check.💡 Detail and suggested fix
If
cfg.MinIntegrity = "Approvd"(typo),formalIntegrityRank("Approvd")returns-1.Any content integrity value's rank will be
>= -1, so the guardrank(content) < rank(minIntegrity)becomesrank(content) < -1, which is never true.Every request passes — the integrity gate is silently gone.
The fix is to distinguish \unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: split integrity failure modes, clarify fail-safe semantics.formalEvaluateAccessnow explicitly checkscfgRank < 0first (fail-safe deny for unrecognizedMinIntegrity) beforereqRank < cfgRank, with separate comments for each mode. The testTestFormal_UnknownIntegrityLevelDeniedcovers both cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Predicate numbering inconsistency between formal model and coverage table
The formal model above defines six predicates P1–P6:
But the behavioral coverage table below uses eight predicates P1–P8 with different names:
This numbering mismatch makes it impossible to trace a predicate from the formal model to its test. The coverage table should use the same P1–P6 identifiers from the formal model, or the formal model should be updated to use P1–P8.
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. The README formal model and the behavioral coverage map now use consistent P1–P6 numbering: P1=ToolAllowed, P2=RepoMatch, P3=RoleAllow, P4=PrivateRepoAllow, P5=NotBlocked, P6=IntegrityMet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The formal model lists guard predicates as
P1–P6in one order, but the behavioral coverage map below uses different numbering (P1–P8plus named predicates). For example, the model hasP5_NotBlockedbut the coverage table labels itP6_NotBlocked. The mismatch makes it hard to trace from the formal spec to the test table without manual reconciliation.💡 Fix suggestion
Align the predicate numbering between the formal definition block and the coverage map, or use names only (drop the
Pnordinals in one place) to avoid the numbering drift.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. The README now defines P1–P6 in evaluation order (P1=ToolAllowed first) and the behavioral coverage table uses the same numbering. No more offset between the formal model and the test coverage map.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
fix: align formal MCP access-control tests with canonical error codes and spec guard order. The README now includes a dedicatedTestFormal_*run command at line 137–141.Uh oh!
There was an error while loading. Please reload this page.