Skip to content

[test] Add tests for launcher.canonicalizeRoots and launcher.isUnderRoot - #11564

Merged
lpcox merged 2 commits into
mainfrom
add-tests-mount-policy-canonicalize-roots-a84e9451f7eba3d4
Aug 20, 2026
Merged

lpcox merged 2 commits into
mainfrom
add-tests-mount-policy-canonicalize-roots-a84e9451f7eba3d4

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Test Coverage Improvement: canonicalizeRoots / isUnderRoot

Function Analyzed

  • Package: internal/launcher
  • File: mount_policy.go
  • Functions: canonicalizeRoots, isUnderRoot
  • Previous Coverage: canonicalizeRoots 82.4%, isUnderRoot 83.3%
  • New Coverage: both 100.0%
  • Complexity: Medium-High — security-sensitive path canonicalization/containment logic used to enforce the mount allowlist that prevents container-backed MCP servers from bind-mounting arbitrary host paths.

Why This Function?

mount_policy.go implements the host-mount security boundary described in the MCP Gateway containerization spec. canonicalizeRoots and isUnderRoot are the core primitives that decide which host directories are trusted and whether a requested mount path is actually contained within an allowed root. Both had real, untested branches (not just debug-log gates):

  • canonicalizeRoots silently drops any root whose path fails canonicalizePath (e.g. a non-absolute path) — untested.
  • canonicalizeRoots deduplicates roots that canonicalize to the same path (e.g. a root and a symlink alias pointing at it) — untested.
  • isUnderRoot's filepath.Rel error branch, triggered when mixing an absolute path with a relative root (or vice versa) — untested.

These are exactly the kind of edge cases where a bug would silently weaken the mount-escape protection, so they're high-value to lock down with tests.

Tests Added

  • ✅ TestCanonicalizeRootsDropsUncanonicalizableRoots — a non-absolute root is silently dropped while a valid root is kept.
  • ✅ TestCanonicalizeRootsDeduplicatesEquivalentPaths — a real directory and a symlink pointing at it canonicalize to the same path and collapse to one root, keeping the first-seen entry's writability.
  • ✅ TestCanonicalizeRootsEmptyInput — empty input yields an empty (non-nil-panicking) result.
  • ✅ TestIsUnderRoot (7 sub-cases) — exact match, nested path, sibling path sharing a string prefix (must not be treated as "under" via naive prefix matching), parent-traversal escape, root pointing above path, and both directions of the filepath.Rel error branch (absolute path + relative root, and relative path + absolute root).

Coverage Report

Before: canonicalizeRoots 82.4%, isUnderRoot 83.3%
After:  canonicalizeRoots 100.0%, isUnderRoot 100.0%
Improvement: +17.6% / +16.7%

Test Execution

All new and existing tests in internal/launcher pass:

--- PASS: TestCanonicalizeRootsDropsUncanonicalizableRoots (0.00s)
--- PASS: TestCanonicalizeRootsDeduplicatesEquivalentPaths (0.00s)
--- PASS: TestCanonicalizeRootsEmptyInput (0.00s)
--- PASS: TestIsUnderRoot (0.00s)
    --- PASS: TestIsUnderRoot/path_equals_root (0.00s)
    --- PASS: TestIsUnderRoot/path_nested_under_root (0.00s)
    --- PASS: TestIsUnderRoot/sibling_path_sharing_string_prefix_is_not_under_root (0.00s)
    --- PASS: TestIsUnderRoot/path_escapes_root_via_parent_traversal (0.00s)
    --- PASS: TestIsUnderRoot/root_is_parent_of_path_root_itself (0.00s)
    --- PASS: TestIsUnderRoot/filepath.Rel_error_from_mixing_absolute_path_and_relative_root (0.00s)
    --- PASS: TestIsUnderRoot/filepath.Rel_error_from_mixing_relative_path_and_absolute_root (0.00s)
ok  	github.com/github/gh-aw-mcpg/internal/launcher	46.834s	coverage: 97.4% of statements

go vet ./internal/launcher/... and gofmt -l are clean.


Generated by Test Coverage Improver
Next run will target the next most complex under-tested function

Generated by Test Coverage Improver · auto · 205.7 AIC · ⊞ 10.5K · ◷

- canonicalizeRoots: cover dropping uncanonicalizable roots, deduping
  equivalent roots reached via symlink aliases, and empty input.
- isUnderRoot: cover exact match, nested path, sibling prefix
  collision, parent traversal escape, and the filepath.Rel error
  branch from mixing absolute/relative path+root arguments.

Coverage: canonicalizeRoots 82.4% -> 100.0%, isUnderRoot 83.3% -> 100.0%

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
lpcox marked this pull request as ready for review August 20, 2026 17:08
Copilot AI balanced review requested due to automatic review settings August 20, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds targeted tests for security-sensitive mount path canonicalization and containment.

Changes:

  • Tests invalid, duplicate, and empty mount roots.
  • Adds table-driven containment edge-case tests.
Show a summary per file
File Description
internal/launcher/mount_policy_test.go Expands coverage for canonicalization and root containment.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread internal/launcher/mount_policy_test.go Outdated
want: false,
},
{
name: "root is parent of path root itself",
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lpcox

lpcox commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot AI commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@copilot address review feedback and fix failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/32394432076/job/96512793943?pr=11564

Addressed in commit f9e78f5.

  • Renamed the misnamed TestIsUnderRoot scenario for accuracy.
  • Fixed the failing CI check by making TestExecuteHTTPRequest_ConnectionError deterministic (uses http://127.0.0.1:0 to avoid dead-server port reuse races).
  • Re-ran verification, including make agent-finished, and all checks passed.

Copilot AI requested a review from lpcox August 20, 2026 17:26
@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — default

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default AWF (normal container isolation)

Part Surface Op Result Expected Status
A MCP list_issues 3 issues returned ALLOWED ✅
A MCP list_pull_requests 3 PRs returned ALLOWED ✅
A MCP get_file_contents (README.md) content returned ALLOWED ✅
A MCP list_commits 3 commits returned ALLOWED ✅
B MCP writes (reaction/star/issue/comment/branch/file/PR) all "unknown tool" BLOCKED ⚠️
C CLI list_issues (github CLI) data returned ALLOWED ✅
C CLI get_file_contents (github CLI) data returned ALLOWED ✅
D CLI REST writes (reaction/star/issue/comment/file) — BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) — BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Part B: All 6 write tools absent from catalog (unknown tool error). Backend runs with GITHUB_READ_ONLY=1 so no write tools are registered — this confirms the gh-aw framework's defense-in-depth, but does not independently confirm mcpg's own DIFC/guard enforcement layer (write calls never reach a write-capable backend).

⚠️ Parts D & E: gh is not authenticated in this environment (no GH_TOKEN). The REST/GraphQL token-scope boundary could not be validated. All Part D/E rows are INCONCLUSIVE rather than PASS.

No writes leaked. No FAIL conditions observed.

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — docker-sbx

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: docker-sbx (KVM-isolated microVM)

Part Surface Op Result Expected Status
A MCP reads (issues/PRs/file/commits) data returned ✓ ALLOWED ✅
B MCP writes (reaction/star/issue/comment/branch/file/PR) all tools absent from catalog BLOCKED ⚠️
C CLI reads (list_issues / get_file_contents) data returned ✓ ALLOWED ✅
D CLI REST writes (reaction/star/issue/comment/file) 401 Bad credentials BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/createIssue) 401 Bad credentials BLOCKED ⚠️

Overall: INCONCLUSIVE

Notes

  • Part A: All 4 MCP reads succeeded — list_issues, list_pull_requests, get_file_contents, list_commits all returned data.
  • Part B (INCONCLUSIVE): The github CLI on PATH exposes exactly 23 read-only tools (no write tools in catalog). All write tool attempts (add_issue_comment, issue_write, star_repository, create_branch, create_or_update_file, create_pull_request) returned Error [-32602]: unknown tool. This is consistent with GITHUB_READ_ONLY=1 suppressing write tools at the backend — absent from catalog proves backend config, not independent gateway enforcement. No write leaked.
  • Part C: CLI reads succeeded via the gateway-backed github CLI.
  • Part D/E (INCONCLUSIVE): gh auth status reports GH_TOKEN is invalid — gh is unauthenticated in this environment. All REST write and GraphQL mutation attempts returned HTTP 401 Bad credentials. Cannot distinguish gateway enforcement from token invalidity. No write leaked.

References: §32397209354

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx runtime

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — gVisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gVisor (runsc)

Part Surface Op Result Expected Status
A MCP list_issues 3 issues returned ALLOWED ✅
A MCP list_pull_requests 3 PRs returned ALLOWED ✅
A MCP get_file_contents (README.md) data returned ALLOWED ✅
A MCP list_commits 3 SHAs returned ALLOWED ✅
B MCP add_issue_comment unknown tool BLOCKED ⚠️
B MCP star_repository unknown tool BLOCKED ⚠️
B MCP issue_write unknown tool BLOCKED ⚠️
B MCP create_branch unknown tool BLOCKED ⚠️
B MCP create_or_update_file unknown tool BLOCKED ⚠️
B MCP create_pull_request unknown tool BLOCKED ⚠️
C CLI github list_issues data returned ALLOWED ✅
C CLI github get_file_contents data returned ALLOWED ✅
D CLI REST writes (all) gh unauthenticated BLOCKED ⚠️
E CLI GraphQL mutations (all) gh unauthenticated BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Notes:

  • Part B: All targeted write tools (add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request) are absent from the MCP tool catalog — backend launched with GITHUB_READ_ONLY=1, so write tools are never registered. Refusals reflect backend configuration, not independently confirmed gateway-level DIFC enforcement. No write leaked.
  • Part D/E: gh is not authenticated in this environment (gh auth status → "not logged into any GitHub hosts"). REST and GraphQL write attempts cannot be made; these rows are INCONCLUSIVE rather than PASS.
  • No writes leaked in any part.

Run: §32397209342

🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime

@lpcox
lpcox merged commit b7a42ac into main Aug 20, 2026
34 of 35 checks passed
@lpcox
lpcox deleted the add-tests-mount-policy-canonicalize-roots-a84e9451f7eba3d4 branch August 20, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants