test: expand credential hiding tests to all 14 protected paths#1163
test: expand credential hiding tests to all 14 protected paths#1163
Conversation
Add integration tests for the 11 previously untested credential paths: SSH keys (id_rsa, id_ed25519, id_ecdsa, id_dsa), AWS credentials/config, Kube config, Azure credentials, GCloud credentials.db, Cargo credentials, and Composer auth.json. Tests verify each path returns 0 bytes (hidden via /dev/null mount) at both the direct home path and /host chroot path. Fixes #761 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Expands the integration test suite to verify credential-hiding behavior across all 14 protected credential paths (home and /host chroot), ensuring the files are effectively empty when mounted from /dev/null.
Changes:
- Updates the existing “Test 4” to more explicitly target the original 3 credential paths and adjusts its shell check.
- Adds a new test group that covers the 11 previously untested credential file paths.
- Verifies 0-byte size for direct home paths and
/hostpaths, plus acat-content check.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const homeDir = os.homedir(); | ||
| const paths = untestedPaths.map(p => `${homeDir}/${p.path}`).join(' '); |
There was a problem hiding this comment.
Building a shell command by interpolating paths directly into sh -c '...' is unsafe and can break when homeDir contains spaces or shell-special characters (and is also an injection risk since this runs with sudo). Prefer passing file paths as positional args to sh -c and iterating over "$@" (or otherwise robustly quoting/escaping each path) so the loop receives the exact paths regardless of characters.
| const homeDir = os.homedir(); | |
| const paths = untestedPaths.map(p => `${homeDir}/${p.path}`).join(' '); | |
| const paths = untestedPaths.map(p => `"$HOME/${p.path}"`).join(' '); |
| // Check all credential files in a single container run for efficiency. | ||
| // wc -c reports byte count; /dev/null-mounted files should be 0 bytes. | ||
| const result = await runner.runWithSudo( | ||
| `sh -c 'for f in ${paths}; do wc -c "$f" 2>/dev/null; done' 2>&1 | grep -v "^\\["`, |
There was a problem hiding this comment.
Building a shell command by interpolating paths directly into sh -c '...' is unsafe and can break when homeDir contains spaces or shell-special characters (and is also an injection risk since this runs with sudo). Prefer passing file paths as positional args to sh -c and iterating over "$@" (or otherwise robustly quoting/escaping each path) so the loop receives the exact paths regardless of characters.
|
|
||
| // cat all files and concatenate output - should be empty | ||
| const result = await runner.runWithSudo( | ||
| `sh -c 'for f in ${paths}; do cat "$f" 2>/dev/null; done' 2>&1 | grep -v "^\\["`, |
There was a problem hiding this comment.
This test can produce a false pass if one or more files don’t exist or aren’t mounted: cat errors are suppressed (2>/dev/null), so the combined stdout can be empty even when paths are missing. To make the assertion meaningful, ensure each file is actually present/checked (e.g., fail the loop if a path is missing, or emit/validate per-file markers), or remove this test and rely on the wc -c tests that already validate counts.
| `sh -c 'for f in ${paths}; do cat "$f" 2>/dev/null; done' 2>&1 | grep -v "^\\["`, | |
| `sh -c 'for f in ${paths}; do cat "$f"; done' 2>&1 | grep -v "^\\["`, |
| const lines = result.stdout.split('\n').filter(l => l.match(/^\s*\d+/)); | ||
| // Each file should be 0 bytes (hidden via /dev/null) | ||
| lines.forEach(line => { | ||
| const size = parseInt(line.trim().split(/\s+/)[0]); |
There was a problem hiding this comment.
parseInt should be called with an explicit radix to avoid edge-case parsing issues. Use parseInt(value, 10) here (and in the similar parsing block in the /host test) to make the intent unambiguous.
| test('Test 4: Original 3 credential files are mounted from /dev/null', async () => { | ||
| const homeDir = os.homedir(); | ||
|
|
||
| // Check multiple credential files in one command | ||
| // Check the originally-tested credential files in one command | ||
| const result = await runner.runWithSudo( | ||
| `sh -c 'for f in ${homeDir}/.docker/config.json ${homeDir}/.npmrc ${homeDir}/.config/gh/hosts.yml; do if [ -f "$f" ]; then wc -c "$f"; fi; done' 2>&1 | grep -v "^\\["`, | ||
| `sh -c 'for f in ${homeDir}/.docker/config.json ${homeDir}/.npmrc ${homeDir}/.config/gh/hosts.yml; do wc -c "$f" 2>/dev/null; done' 2>&1 | grep -v "^\\["`, |
There was a problem hiding this comment.
With 2>/dev/null added and no follow-up assertion that all three files produced output, this test can now silently skip missing/unmounted files (no wc output) and potentially still pass depending on downstream expectations. To preserve the “mounted from /dev/null” guarantee, parse the wc output and assert you got exactly 3 size lines and that each is 0 (or make the shell loop fail if any file is missing).
🧪 Build Test: Bun
Overall: ✅ PASS
|
Build Test: Node.js Results ✅
Overall: PASS
|
Go Build Test Results
Overall: PASS ✅
|
C++ Build Test Results
Overall: PASS
|
|
Smoke test results for ✅ GitHub MCP: #1159 fix(security): eliminate TOCTOU race conditions in ssl-bump.ts, #1158 fix(security): stop logging partial token values Overall: PASS
|
Build Test: Deno ✅
Overall: PASS
|
Java Build Test Results ✅
Overall: PASS
|
|
Test results:
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
🦀 Rust Build Test Results
Overall: ✅ PASS
|
Smoke Test Results
Overall: PASS
|
Summary
Test plan
npm run buildpassesnpm testpasses (831 tests)npm run lintpasses (0 errors)Fixes #761
🤖 Generated with Claude Code