refactor: remove --allow-full-filesystem-access flag#1056
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
🧪 Build Test: Bun Results
Overall: ✅ PASS
|
🦕 Deno Build Test Results
Overall: ✅ PASS Test output detailsoak:
|
|
Smoke test results:
|
Go Build Test Results
Overall: ✅ PASS
|
C++ Build Test Results
Overall: PASS
|
🦀 Rust Build Test Results
Overall: ✅ PASS
|
There was a problem hiding this comment.
Pull request overview
This PR removes the --allow-full-filesystem-access CLI flag that allowed disabling AWF's selective mounting security feature. The flag was an escape hatch that mounted the entire host filesystem with read-write access (/:/host:rw), defeating credential exfiltration protection. With this removal, credential hiding via selective mounting becomes the only supported mode, strengthening the security posture.
Changes:
- Removed the
--allow-full-filesystem-accessCLI option and type definitions - Made credential hiding unconditional by removing if/else branches in docker-manager
- Deleted 4 tests (2 unit, 2 integration) that exercised the removed flag
- Cleaned up documentation references across usage guides, selective-mounting docs, CLI reference, and test analysis docs
- Added 92 lines of new block-domains integration tests and test fixture support for envAll/cliEnv (not mentioned in PR description)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli.ts | Removed --allow-full-filesystem-access option definition and removed allowFullFilesystemAccess from config object |
| src/types.ts | Removed allowFullFilesystemAccess field and its extensive documentation block from WrapperConfig interface |
| src/docker-manager.ts | Removed if/else branching - credential hiding via /dev/null mounts is now unconditional; updated comments |
| src/docker-manager.test.ts | Deleted 2 duplicate unit tests that verified blanket mount behavior with the flag |
| tests/fixtures/awf-runner.ts | Removed allowFullFilesystemAccess from interface; added blockDomains, envAll, and cliEnv support |
| tests/integration/credential-hiding.test.ts | Deleted Test 10 (security warnings) and Test 11 (Docker config not hidden) that tested the flag |
| tests/integration/blocked-domains.test.ts | Added 92 lines of new block-domains integration tests (6 test cases) |
| docs/usage.md | Removed 6 lines describing the flag and its security warnings |
| docs/selective-mounting.md | Removed "Full Filesystem Access" section (14 lines) and updated comments |
| docs-site/.../cli-reference.md | Removed CLI table row and 26-line detail section for the flag |
| docs/test-analysis/test-infra.md | Removed allowFullFilesystemAccess from interface documentation |
| docs/test-analysis/protocol-security.md | Removed 3 references to full filesystem access tests |
| docs/test-analysis/chroot.md | Removed 2 gap notes about missing flag tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('Block Domains Deny-List (--block-domains)', () => { | ||
| let runner: AwfRunner; | ||
|
|
||
| beforeAll(async () => { | ||
| await cleanup(false); | ||
| runner = createRunner(); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await cleanup(false); | ||
| }); | ||
|
|
||
| test('should block specific subdomain while allowing parent domain', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 10 https://api.github.com/zen', | ||
| { | ||
| allowDomains: ['github.com'], | ||
| blockDomains: ['api.github.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result).toFail(); | ||
| }, 120000); | ||
|
|
||
| test('should still allow non-blocked subdomains when parent is allowed', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 10 https://github.com', | ||
| { | ||
| allowDomains: ['github.com'], | ||
| blockDomains: ['api.github.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result).toSucceed(); | ||
| }, 120000); | ||
|
|
||
| test('should block domain that is also in the allow list (block takes precedence)', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 5 https://example.com', | ||
| { | ||
| allowDomains: ['example.com'], | ||
| blockDomains: ['example.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result).toFail(); | ||
| }, 120000); | ||
|
|
||
| test('should block wildcard pattern while allowing parent domain', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 10 https://api.github.com/zen', | ||
| { | ||
| allowDomains: ['github.com'], | ||
| blockDomains: ['*.github.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result).toFail(); | ||
| }, 120000); | ||
|
|
||
| test('should handle multiple blocked domains', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'bash -c "curl -f --max-time 10 https://api.github.com/zen 2>&1; echo exit=$?"', | ||
| { | ||
| allowDomains: ['github.com'], | ||
| blockDomains: ['api.github.com', 'raw.githubusercontent.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result.stdout).toContain('exit='); | ||
| expect(result.stdout).not.toContain('exit=0'); | ||
| }, 120000); | ||
|
|
||
| test('should show blocked domains in debug output', async () => { | ||
| const result = await runner.runWithSudo( | ||
| 'echo "test"', | ||
| { | ||
| allowDomains: ['github.com'], | ||
| blockDomains: ['api.github.com'], | ||
| logLevel: 'debug', | ||
| timeout: 60000, | ||
| } | ||
| ); | ||
| expect(result).toSucceed(); | ||
| expect(result.stderr).toMatch(/[Bb]locked domains:/i); | ||
| }, 120000); | ||
| }); |
There was a problem hiding this comment.
The PR description does not mention adding these 92 lines of new block-domains integration tests. This appears to be a separate feature enhancement bundled with the flag removal. Consider splitting this into a separate PR or updating the PR description to document this addition.
These tests add coverage for the existing --block-domains feature:
- Blocking specific subdomains while allowing parent
- Block precedence over allow
- Wildcard patterns in block lists
- Multiple blocked domains
- Debug output verification
tests/fixtures/awf-runner.ts
Outdated
| @@ -52,6 +54,11 @@ export class AwfRunner { | |||
| args.push('--allow-domains', options.allowDomains.join(',')); | |||
| } | |||
|
|
|||
| // Add block-domains | |||
| if (options.blockDomains && options.blockDomains.length > 0) { | |||
| args.push('--block-domains', options.blockDomains.join(',')); | |||
| } | |||
|
|
|||
| // Add other flags | |||
| if (options.keepContainers) { | |||
| args.push('--keep-containers'); | |||
| @@ -100,16 +107,23 @@ export class AwfRunner { | |||
| args.push('--allow-host-ports', options.allowHostPorts); | |||
| } | |||
|
|
|||
| // Add allow-full-filesystem-access flag | |||
| if (options.allowFullFilesystemAccess) { | |||
| args.push('--allow-full-filesystem-access'); | |||
| } | |||
|
|
|||
| // Add enable-api-proxy flag | |||
| if (options.enableApiProxy) { | |||
| args.push('--enable-api-proxy'); | |||
| } | |||
|
|
|||
| // Add --env-all flag | |||
| if (options.envAll) { | |||
| args.push('--env-all'); | |||
| } | |||
|
|
|||
| // Add explicit -e KEY=VALUE flags | |||
| if (options.cliEnv) { | |||
| for (const [key, value] of Object.entries(options.cliEnv)) { | |||
| args.push('-e', `${key}=${value}`); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
The PR description does not mention adding support for envAll and cliEnv fields to the AwfOptions interface and both run methods. These appear to be separate enhancements for environment variable handling in tests, bundled with the flag removal. Consider updating the PR description to document these additions or splitting them into a separate PR.
These additions enable tests to:
- Pass --env-all flag via envAll boolean
- Pass explicit -e KEY=VALUE flags via cliEnv object
|
🤖 Smoke test results for Copilot engine — run 22415315431 ✅ GitHub MCP — Last 2 merged PRs:
✅ Playwright — github.com title contains "GitHub" Overall: PASS | Author:
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
Smoke Test Results
Overall: PASS
|
Java Build Test Results
Overall: PASS ✅ Both projects compiled and all tests passed successfully using Maven with proxy configuration.
|
🟢 Node.js Build Test Results
Overall: PASS ✅
|
Chroot Version Comparison Results
Overall: ❌ Not all versions match — Python minor version differs (3.12.12 vs 3.12.3) and Node.js major version differs (v24 vs v20).
|
The flag was an escape hatch that disabled selective mounting security by adding a blanket /:/host:rw mount, exposing all credential files. It contradicts the security model and is no longer needed: - gh-aw never passes this flag (not in awf_helpers.go) - No CI workflows or smoke tests use it - It was already hidden from --help in PR #1035 - The --mount flag covers mounting specific directories Credential hiding is now unconditional — the if/else guards around selective mounting are removed, making the secure path the only path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c95d447 to
24e648e
Compare
|
Smoke Test Results
Overall: PASS
|
C++ Build Test Results
Overall: PASS
|
🤖 Smoke Test Results
Overall: PASS | PR author:
|
🧪 Bun Build Test Results
Overall: ✅ PASS Bun version:
|
🦕 Deno Build Test Results
Overall: ✅ PASS
|
Rust Build Test Results
Overall: ✅ PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world: json-parse:
|
Go Build Test Results
Overall: ✅ PASS
|
Node.js Build Test Results ✅
Overall: PASS
|
Chroot Version Comparison Results
Overall: ❌ Not all versions matched — Python and Node.js versions differ between host and chroot environment.
|
|
GitHub MCP merged PRs: ✅
|
Java Build Test Results
Overall: ✅ PASS All Java projects compiled and tested successfully against Maven Central via the AWF proxy.
|
Summary
--allow-full-filesystem-accessCLI flag that disabled selective mounting securityMotivation
The flag was an escape hatch that mounted the entire host filesystem with read-write access (
/:/host:rw), defeating AWF's credential exfiltration protection. It is no longer needed because:awf_helpers.go)--helpin PR feat: group --help flags by category, hide dev-only options #1035--mountflag covers the use case of mounting specific directoriesFiles changed (12)
src/cli.tssrc/types.tsallowFullFilesystemAccessfield fromWrapperConfigsrc/docker-manager.tssrc/docker-manager.test.tsallowFullFilesystemAccesstest casestests/fixtures/awf-runner.tsAwfOptionsinterface and bothifblockstests/integration/credential-hiding.test.tsdocs/usage.mddocs/selective-mounting.mddocs-site/.../cli-reference.mddocs/test-analysis/*.mdTest plan
npx tsc --noEmit— compiles with no errorsnpm test— all 798 unit tests passnode dist/cli.js --help— flag absent from outputnode dist/cli.js --allow-full-filesystem-access -- echo test— errors as unknown option🤖 Generated with Claude Code