-
Notifications
You must be signed in to change notification settings - Fork 6
Addressing PR comments #700
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: lpcox <[email protected]>
Co-authored-by: lpcox <[email protected]>
Co-authored-by: lpcox <[email protected]>
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.34% | 82.33% | 📉 -0.01% |
| Statements | 82.38% | 82.38% | ➡️ +0.00% |
| Functions | 82.14% | 82.14% | ➡️ +0.00% |
| Branches | 74.70% | 74.64% | 📉 -0.06% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
83.9% → 83.8% (-0.04%) | 83.3% → 83.3% (-0.04%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Status: PASS
|
Go Build Test Results
Overall: PASS ✅ All Go projects built and tested successfully.
|
Bun Build Test Results
Overall: PASS ✅ All Bun projects built and tested successfully.
|
|
Smoke Test Results Last 2 merged PRs:
✅ GitHub MCP - PASS Overall: PASS 🎉
|
Node.js Build Test Results ✅All Node.js projects built and tested successfully!
Overall: ✅ PASS All dependencies installed without vulnerabilities and all test suites passed.
|
.NET Build Test Results
Overall: PASS ✅ All .NET projects successfully restored dependencies, built, and ran without errors.
|
Build Test: Rust ✅
Overall: PASS ✅ All Rust projects built and tested successfully.
|
C++ Build Test Results
Overall: PASS ✅ All C++ projects configured and built successfully.
|
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.
Pull request overview
Closes a credential-exfiltration bypass in chroot mode by ensuring sensitive files are hidden via /dev/null overlays at both home mount locations ($HOME and /host$HOME), and adds integration coverage + documentation to prevent regression.
Changes:
- Apply selective credential-hiding mounts in all non-full-filesystem modes (including chroot direct-home path).
- Add chroot-mode integration tests targeting the direct-home bypass scenario and renumber subsequent tests.
- Update selective-mounting docs and add a security-fix summary with analysis/verification steps.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/docker-manager.ts |
Ensures credential overlays are applied for the direct $HOME mount even in chroot mode, and keeps /host$HOME overlays. |
tests/integration/credential-hiding.test.ts |
Adds new chroot-mode regression tests for the direct-home bypass and renumbers later tests. |
docs/selective-mounting.md |
Documents the dual home mounts in chroot mode and the need to hide credentials at both paths. |
SECURITY_FIX_SUMMARY.md |
Adds a written vulnerability analysis, fix details, and verification guidance. |
Comments suppressed due to low confidence (2)
tests/integration/credential-hiding.test.ts:228
- This test claims the hosts.yml output “should be empty”, but the assertions only check that it doesn’t contain specific token strings. This can produce false positives (e.g., file missing, different token format, or other sensitive content still leaking). Consider asserting the output is exactly empty (as in Test 1/Test 8) or otherwise validating that a /dev/null overlay is in effect (e.g., size 0).
expect(result).toSucceed();
const output = result.stdout.trim();
// Should be empty (no oauth_token visible via direct home mount)
expect(output).not.toContain('oauth_token');
expect(output).not.toContain('gho_');
src/docker-manager.ts:692
- In chroot mode, effectiveHome is already set to getRealUserHome(). This block calls getRealUserHome() again; consider reusing effectiveHome (or passing userHome through) to avoid duplicated logic and any chance of inconsistency if getRealUserHome() behavior changes.
const userHome = getRealUserHome();
const chrootCredentialFiles = [
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SECURITY_FIX_SUMMARY.md
Outdated
| 1. **Direct mount at `$HOME`** (line 437 in `src/docker-manager.ts`) | ||
| - Purpose: Allow container environment to access workspace files | ||
| - Example: `/home/runner:/home/runner:rw` | ||
|
|
||
| 2. **Chroot mount at `/host$HOME`** (line 479 in `src/docker-manager.ts`) |
Copilot
AI
Feb 12, 2026
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.
This document references specific line numbers in src/docker-manager.ts for mounts and the fix. Those line numbers are likely to drift as the file changes, making the analysis harder to trust over time. Consider replacing exact line references with function names + a short code identifier (e.g., the relevant volume strings) or links to a specific commit/tag.
| 1. **Direct mount at `$HOME`** (line 437 in `src/docker-manager.ts`) | |
| - Purpose: Allow container environment to access workspace files | |
| - Example: `/home/runner:/home/runner:rw` | |
| 2. **Chroot mount at `/host$HOME`** (line 479 in `src/docker-manager.ts`) | |
| 1. **Direct mount at `$HOME`** (in `src/docker-manager.ts`, volume spec `/home/runner:/home/runner:rw`) | |
| - Purpose: Allow container environment to access workspace files | |
| - Example: `/home/runner:/home/runner:rw` | |
| 2. **Chroot mount at `/host$HOME`** (in `src/docker-manager.ts`, volume spec `/home/runner:/host/home/runner:rw`) |
| // This test verifies that credentials are hidden at BOTH locations: | ||
| // 1. At /host${HOME} (chroot path) | ||
| // 2. At ${HOME} (direct mount) |
Copilot
AI
Feb 12, 2026
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 comment says this test verifies credentials are hidden at BOTH /host$HOME and $HOME, but this test only checks the direct $HOME path. Either update the comment to reflect what’s actually asserted here, or add a second assertion for the /host path (or explicitly point to Test 6 as covering it).
This issue also appears on line 224 of the same file.
| // This test verifies that credentials are hidden at BOTH locations: | |
| // 1. At /host${HOME} (chroot path) | |
| // 2. At ${HOME} (direct mount) | |
| // This test specifically verifies that credentials are hidden at the direct ${HOME} mount | |
| // (the bypass path). The /host${HOME} chroot path is covered by | |
| // "Test 6: Chroot mode hides credentials at /host paths". |
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.
Updated test comments to clarify that Test 8 only verifies the direct home path (the bypass path), with explicit reference to Test 6 for /host path coverage. Same fix applied to Test 9. Commit c873bbc
src/docker-manager.ts
Outdated
| // - At ${effectiveHome} (direct mount for container environment) | ||
| // - At /host${userHome} (for chroot operations) |
Copilot
AI
Feb 12, 2026
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 comment uses “${userHome}” even though userHome isn’t defined until later, and the “${…}” notation can be confused with template-string interpolation. Consider rephrasing to plain text (e.g., “effective home path” and “/host + real user home”) to avoid confusion while keeping the security rationale.
This issue also appears on line 691 of the same file.
| // - At ${effectiveHome} (direct mount for container environment) | |
| // - At /host${userHome} (for chroot operations) | |
| // - At the effective home path used inside the container environment | |
| // - At /host plus the real user home path on the host (for chroot operations) |
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.
Replaced template string notation with plain text descriptions to avoid confusion with actual interpolation. Changed to "effective home path" and "/host plus the real user home path". Commit c873bbc
docs/selective-mounting.md
Outdated
| // IMPORTANT: Home directory is mounted at TWO locations in chroot mode | ||
| // Credentials MUST be hidden at BOTH paths to prevent bypass attacks | ||
|
|
||
| // 1. Direct home mount (for container environment) | ||
| const directHomeCredentials = [ | ||
| '/dev/null:/home/runner/.docker/config.json:ro', | ||
| '/dev/null:/home/runner/.npmrc:ro', | ||
| '/dev/null:/home/runner/.cargo/credentials:ro', | ||
| '/dev/null:/home/runner/.composer/auth.json:ro', | ||
| '/dev/null:/home/runner/.config/gh/hosts.yml:ro', |
Copilot
AI
Feb 12, 2026
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.
This documentation example hardcodes /home/runner paths for the hidden credentials. Since the code uses the real user home at runtime, consider using ${HOME} (or a placeholder like ) consistently here to avoid implying chroot-mode protection only applies to GitHub Actions runners.
See below for a potential fix:
`/dev/null:${HOME}/.docker/config.json:ro`,
`/dev/null:${HOME}/.npmrc:ro`,
`/dev/null:${HOME}/.cargo/credentials:ro`,
`/dev/null:${HOME}/.composer/auth.json:ro`,
`/dev/null:${HOME}/.config/gh/hosts.yml:ro`,
`/dev/null:${HOME}/.ssh/id_rsa:ro`,
`/dev/null:${HOME}/.ssh/id_ed25519:ro`,
`/dev/null:${HOME}/.ssh/id_ecdsa:ro`,
`/dev/null:${HOME}/.ssh/id_dsa:ro`,
`/dev/null:${HOME}/.aws/credentials:ro`,
`/dev/null:${HOME}/.aws/config:ro`,
`/dev/null:${HOME}/.kube/config:ro`,
`/dev/null:${HOME}/.azure/credentials:ro`,
`/dev/null:${HOME}/.config/gcloud/credentials.db:ro`,
];
// 2. Chroot /host mount (for chroot operations)
const chrootHiddenCredentials = [
`/dev/null:/host${HOME}/.docker/config.json:ro`,
`/dev/null:/host${HOME}/.npmrc:ro`,
`/dev/null:/host${HOME}/.cargo/credentials:ro`,
`/dev/null:/host${HOME}/.composer/auth.json:ro`,
`/dev/null:/host${HOME}/.config/gh/hosts.yml:ro`,
`/dev/null:/host${HOME}/.ssh/id_rsa:ro`,
`/dev/null:/host${HOME}/.ssh/id_ed25519:ro`,
`/dev/null:/host${HOME}/.ssh/id_ecdsa:ro`,
`/dev/null:/host${HOME}/.ssh/id_dsa:ro`,
`/dev/null:/host${HOME}/.aws/credentials:ro`,
`/dev/null:/host${HOME}/.aws/config:ro`,
`/dev/null:/host${HOME}/.kube/config:ro`,
`/dev/null:/host${HOME}/.azure/credentials:ro`,
`/dev/null:/host${HOME}/.config/gcloud/credentials.db:ro`,
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.
Changed hardcoded /home/runner paths to ${HOME} placeholders to be environment-agnostic and match runtime behavior. Commit c873bbc
Deno Build Test Results
Overall: ✅ PASS All Deno tests completed successfully.
|
Java Build Test Results
Overall: PASS ✅ All Java projects compiled successfully and all tests passed.
|
Chroot Version Comparison Test Results
Status: Some version mismatches detected. Chroot mode successfully uses host binaries, but the versions differ from what's installed in the container.
|
SECURITY_FIX_SUMMARY.md
Outdated
| @@ -0,0 +1,228 @@ | |||
| # Security Fix: Chroot Mode Credential Bypass Vulnerability | |||
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.
@claude[agent] please remove this file
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.
Removed in commit c873bbc
Co-authored-by: Mossaka <[email protected]>
No description provided.