-
Notifications
You must be signed in to change notification settings - Fork 6
fix: create .copilot/logs mountpoint before docker mount #722
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -502,8 +502,9 @@ export function generateDockerCompose( | |||||
| } | ||||||
|
|
||||||
| // Mount ~/.cargo for Rust binaries (read-only) if it exists | ||||||
| // SKIP if allowFullFilesystemAccess is false (credentials will be hidden via tmpfs) | ||||||
| const hostCargoDir = path.join(userHome, '.cargo'); | ||||||
| if (fs.existsSync(hostCargoDir)) { | ||||||
| if (fs.existsSync(hostCargoDir) && config.allowFullFilesystemAccess) { | ||||||
| agentVolumes.push(`${hostCargoDir}:/host${hostCargoDir}:ro`); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -672,6 +673,9 @@ export function generateDockerCompose( | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| // Store credential tmpfs mounts to add later | ||||||
| const credentialTmpfsMounts: string[] = []; | ||||||
|
|
||||||
| // Apply security policy: selective mounting vs full filesystem access | ||||||
| if (config.allowFullFilesystemAccess) { | ||||||
| // User explicitly opted into full filesystem access - log security warning | ||||||
|
|
@@ -687,64 +691,66 @@ export function generateDockerCompose( | |||||
| // This provides security against credential exfiltration via prompt injection | ||||||
| logger.debug('Using selective mounting for security (credential files hidden)'); | ||||||
|
|
||||||
| // SECURITY: Hide credential files by mounting /dev/null over them | ||||||
| // SECURITY: Hide credential directories using tmpfs (empty in-memory filesystem) | ||||||
| // This prevents prompt-injected commands from reading sensitive tokens | ||||||
| // even if the attacker knows the file paths | ||||||
| const credentialFiles = [ | ||||||
| `${effectiveHome}/.docker/config.json`, // Docker Hub tokens | ||||||
| `${effectiveHome}/.npmrc`, // NPM registry tokens | ||||||
| `${effectiveHome}/.cargo/credentials`, // Rust crates.io tokens | ||||||
| `${effectiveHome}/.composer/auth.json`, // PHP Composer tokens | ||||||
| `${effectiveHome}/.config/gh/hosts.yml`, // GitHub CLI OAuth tokens | ||||||
| // SSH private keys (CRITICAL - server access, git operations) | ||||||
| `${effectiveHome}/.ssh/id_rsa`, | ||||||
| `${effectiveHome}/.ssh/id_ed25519`, | ||||||
| `${effectiveHome}/.ssh/id_ecdsa`, | ||||||
| `${effectiveHome}/.ssh/id_dsa`, | ||||||
| // Cloud provider credentials (CRITICAL - infrastructure access) | ||||||
| `${effectiveHome}/.aws/credentials`, | ||||||
| `${effectiveHome}/.aws/config`, | ||||||
| `${effectiveHome}/.kube/config`, | ||||||
| `${effectiveHome}/.azure/credentials`, | ||||||
| `${effectiveHome}/.config/gcloud/credentials.db`, | ||||||
| // even if the attacker knows the file paths. | ||||||
| // Using tmpfs instead of /dev/null mounts avoids Docker errors when parent directories | ||||||
| // don't exist in the container filesystem. | ||||||
| const credentialDirs = [ | ||||||
| `${effectiveHome}/.docker`, // Docker Hub tokens (config.json) | ||||||
| `${effectiveHome}/.ssh`, // SSH private keys (CRITICAL - server access, git operations) | ||||||
| `${effectiveHome}/.aws`, // AWS credentials (CRITICAL - infrastructure access) | ||||||
| `${effectiveHome}/.kube`, // Kubernetes config (CRITICAL - cluster access) | ||||||
| `${effectiveHome}/.azure`, // Azure credentials | ||||||
| `${effectiveHome}/.config/gcloud`, // Google Cloud credentials | ||||||
| `${effectiveHome}/.config/gh`, // GitHub CLI OAuth tokens | ||||||
| `${effectiveHome}/.cargo`, // Rust crates.io tokens (credentials file) | ||||||
| `${effectiveHome}/.composer`, // PHP Composer tokens (auth.json) | ||||||
| ]; | ||||||
|
|
||||||
| credentialFiles.forEach(credFile => { | ||||||
| agentVolumes.push(`/dev/null:${credFile}:ro`); | ||||||
| // Add tmpfs mounts for credential directories | ||||||
| credentialDirs.forEach(credDir => { | ||||||
| credentialTmpfsMounts.push(`${credDir}:rw,noexec,nosuid,size=1m`); | ||||||
| }); | ||||||
|
|
||||||
| logger.debug(`Hidden ${credentialFiles.length} credential file(s) via /dev/null mounts`); | ||||||
| // Also hide ~/.npmrc file (NPM registry tokens) - needs special handling as it's a file | ||||||
| // Mount its parent directory as tmpfs to hide it | ||||||
|
||||||
| // Mount its parent directory as tmpfs to hide it | |
| // Mount ~/.npmrc as a tmpfs directory to hide the NPM registry token file |
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 new functionality to create ~/.copilot/logs mountpoint lacks test coverage, while similar directory creation functionality (agent-logs, squid-logs) has dedicated tests in docker-manager.test.ts. Consider adding a test that verifies the .copilot/logs directory is created in both chroot and non-chroot modes, similar to the existing "should create agent-logs directory" test at line 1511-1528.
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 PR title and description focus on fixing the .copilot/logs mountpoint issue, but this PR also includes a significant architectural change: migrating all credential hiding from /dev/null volume mounts to tmpfs mounts. While this change appears to be intentional and correct (tmpfs avoids errors when target paths don't exist), it represents a substantial scope expansion that should be documented in the PR description. This makes it difficult for reviewers to understand the full scope of changes and their implications.
This issue also appears on line 1304 of the same file.