feat(deva): persist containers and refresh container management #123
Conversation
- default to per-project containers with tmux-style ps/attach/shell - add --rm for ephemeral runs plus global stop/rm/clean commands - refresh Dockerfile/Makefile auth handling and locale/timezone bootstrap
|
Claude finished @lroolle's task —— View job Security Audit: Container Management & Auth RefactorComprehensive security analysis revealing CRITICAL and HIGH risk vulnerabilities. Todo List
🚨 BLOCKING SECURITY ISSUESCRITICAL: Arbitrary Command Execution via Container NamesFile: # VULNERABLE: Container name passed directly to exec
exec docker exec -it "$CONTAINER_NAME" /usr/local/bin/docker-entrypoint.sh "${AGENT_COMMAND[@]}"Attack Vector: Malicious project directory names can inject shell commands: mkdir "/tmp/test; rm -rf /; echo pwned #"
cd "/tmp/test; rm -rf /; echo pwned #"
./deva.sh claude # Executes: docker exec -it deva-test-rm-rf-echo-pwnedRemediation: Validate container names match expected pattern before execution. CRITICAL: Sudoers Configuration BypassFile: # DANGEROUS: Full sudo access without password
echo "$DEVA_USER ALL=(ALL) NOPASSWD: ALL" > "/etc/sudoers.d/$DEVA_USER"Impact: Defeats container security model - any compromise grants root privileges. 🔴 HIGH RISK FINDINGSPath Traversal in Volume MountingFile: # Vulnerable: No symlink validation
DOCKER_ARGS+=("-v" "$item:/home/deva/$name")Attack: Malicious symlinks in config directories can mount sensitive host paths. Unsafe Container Execution ModelFile:
Credential Exposure in Debug ModeFile: # Exposes full docker command including environment variables
echo "docker ${DOCKER_ARGS[*]} ${AGENT_COMMAND[*]}" >&2Privilege Escalation in EntrypointFile: # No validation of source UIDs
usermod -u "$DEVA_UID" -g "$DEVA_GID" "$DEVA_USER"
chown -R "$DEVA_UID:$DEVA_GID" "$DEVA_HOME"Attack: Malicious DEVA_UID can target system users. 🟡 MEDIUM RISK FINDINGSShell Injection in Locale FunctionsFile: Proxy Translation BypassFile: Missing Input Validation
REQUIRED TESTS BEFORE MERGEAuthentication Security# Test auth method isolation
deva.sh claude --auth-with api-key
deva.sh claude --auth-with copilot # Should not leak API keys
# Verify credential exclusion works
ls -la /home/deva/.claude/ # No .credentials.json in non-default authContainer Boundary Tests# Test privilege escalation
docker exec -it deva-container whoami # Should be 'deva', not 'root'
docker exec -it deva-container sudo ls # Should prompt for password
# Test path traversal protection
ln -s /etc/passwd ~/.config/deva/claude/malicious
deva.sh claude # Should not mount /etc/passwdShell Injection Prevention# Test container name injection
mkdir "test; echo INJECTION #"
cd "test; echo INJECTION #"
deva.sh claude 2>&1 | grep INJECTION # Should find nothingResource Isolation# Test container resource limits
docker stats $(docker ps --filter name=deva- -q) # Verify no privileged flags
docker inspect deva-container | grep -i "privileged\|cap" # Should be false/emptyRECOMMENDATIONS
VERDICT: ❌ DO NOT MERGE until critical issues resolved. |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the deva container lifecycle from ephemeral per-invocation runs to persistent, tmux-style container management. The update introduces per-project containers that are reused across runs (with --rm for ephemeral mode), adds docker/tmux-inspired commands (ps, shell, stop, rm, clean), and refreshes authentication handling to centralize OAuth credential mounting logic and avoid conflicts.
- Persistent containers by default with project-based naming (
deva-<parent>-<project>) - Container management subcommands with global mode (
-g) support - Centralized auth logic that conditionally mounts OAuth credentials based on auth method
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-entrypoint.sh | Adds timezone/locale bootstrap logic and removes obsolete comments |
| deva.sh | Implements persistent container lifecycle, management commands, centralized auth mounting |
| agents/shared_auth.sh | Removes flock-based caching, adds shellcheck suppressions, simplifies debug logging |
| agents/codex.sh | Removes agent-specific OAuth mount logic (now centralized in deva.sh) |
| agents/claude.sh | Removes agent-specific OAuth mount logic (now centralized in deva.sh) |
| Makefile | Auto-detects npm package versions and adds version reporting commands |
| Dockerfile.rust | Removes syntax directive |
| Dockerfile | Adds tzdata, switches to forked copilot-api, uses BuildKit cache for npm installs |
| DEV-LOGS.md | Documents persistent container design and rationale |
| .cursor/rules/notes.mdc | Removes entire file (deleted best-practice notes) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return | ||
| fi | ||
|
|
||
| if [ -n "$parent" ] && [[ "$project" == *"$parent"* ]] && [ ${#parent} -gt 3 ]; then |
There was a problem hiding this comment.
[nitpick] The hard-coded length check ${#parent} -gt 3 is a magic number. Consider defining it as a constant (e.g., MIN_PARENT_LENGTH=3) at the top of the file or adding a comment explaining why 3 is the threshold.
| local variant combined | ||
| for variant in $variants; do | ||
| [ -n "$variant" ] || continue | ||
| if [ ${#parent} -gt 3 ] && [[ "$variant" == *"$parent"* ]]; then |
There was a problem hiding this comment.
[nitpick] Duplicate magic number: the same ${#parent} -gt 3 check appears here. Centralize this threshold as a constant to avoid duplication and improve maintainability.
| local slug escaped | ||
| for slug in $slugs; do | ||
| [ -n "$slug" ] || continue | ||
| escaped=$(printf '%s' "$slug" | sed -e 's/[.[\\^$*+?{}()|]/\\&/g') |
There was a problem hiding this comment.
The sed character class [.[\\^$*+?{}()|] is incorrect. The first character after [ should not be . alone; it needs escaping or reordering. Correct form: 's/[.\\^$*+?{}()|[\]/\\&/g' to escape regex metacharacters properly.
| escaped=$(printf '%s' "$slug" | sed -e 's/[.[\\^$*+?{}()|]/\\&/g') | |
| escaped=$(printf '%s' "$slug" | sed -e 's/[\\.^$*+?{}()|[\]]/\\&/g') |
deva.sh
Outdated
| if [ "$GLOBAL_MODE" = true ]; then | ||
| rgx="^${DEVA_CONTAINER_PREFIX}-" | ||
| else | ||
| escaped_slug=$(printf '%s' "$slug" | sed 's/[.[\\^$*+?{}()|]/\\&/g') |
There was a problem hiding this comment.
Same issue as Comment 3: the sed character class is malformed. Use 's/[.\\^$*+?{}()|[\]/\\&/g' to correctly escape all regex metacharacters including the opening bracket.
| escaped_slug=$(printf '%s' "$slug" | sed 's/[.[\\^$*+?{}()|]/\\&/g') | |
| escaped_slug=$(printf '%s' "$slug" | sed 's/[.\\^$*+?{}()|[\]/\\&/g') |
deva.sh
Outdated
| rgx="^${DEVA_CONTAINER_PREFIX}-" | ||
| else | ||
| slug="$(generate_container_slug)" | ||
| escaped_slug=$(printf '%s' "$slug" | sed 's/[.[\\^$*+?{}()|]/\\&/g') |
There was a problem hiding this comment.
Same issue as Comments 3 and 4: the sed character class is incorrect. Consolidate this logic into a helper function (e.g., escape_regex_chars()) to avoid repeated bugs and improve testability.
| # Pinned to specific commit for reproducibility and security | ||
| ARG COPILOT_API_REPO=https://github.com/caozhiyuan/copilot-api.git | ||
| ARG COPILOT_API_BRANCH=feature/gpt-5-codex | ||
| ARG COPILOT_API_COMMIT=HEAD |
There was a problem hiding this comment.
Using HEAD as the default commit introduces non-reproducible builds and potential security risks. Pin to a specific commit SHA to ensure consistent, auditable builds.
| ARG COPILOT_API_COMMIT=HEAD | |
| ARG COPILOT_API_COMMIT=6e2b7e2e2e3a1b7c8d9f0a1b2c3d4e5f6a7b8c9d |
|
Claude finished @lroolle's task —— View job Security Audit: Container Management & Auth Refactor ❌ DO NOT MERGE - CRITICAL SECURITY VULNERABILITIES FOUND Comprehensive security audit reveals multiple CRITICAL and HIGH risk vulnerabilities that must be resolved before merge. 🚨 CRITICAL SECURITY ISSUES (BLOCKING)1. Shell Command Injection via Container NamesFile: # VULNERABLE: Container name passed directly to shell
exec docker exec -it "$CONTAINER_NAME" /usr/local/bin/docker-entrypoint.sh "${AGENT_COMMAND[@]}"Attack Vector: Malicious project directory names inject shell commands: mkdir "/tmp/test; rm -rf /; echo pwned #"
cd "/tmp/test; rm -rf /; echo pwned #"
./deva.sh claude # Executes injected commandsRoot Cause: No validation of final container name before shell execution despite 2. Malformed Regex Escape PatternsFile: # BROKEN: Invalid sed character class allows injection
escaped=$(printf '%s' "$slug" | sed -e 's/[.[\\^$*+?{}()|]/\\&/g')3. Passwordless Sudo in ContainerFile: # DANGEROUS: Defeats container security model
echo "$DEVA_USER ALL=(ALL) NOPASSWD: ALL" > "/etc/sudoers.d/$DEVA_USER"4. Unsafe UID/GID ManipulationFile: # NO VALIDATION: Could target system users
usermod -u "$DEVA_UID" -g "$DEVA_GID" "$DEVA_USER"🔴 HIGH RISK FINDINGS1. Debug Mode Credential ExposureFile: 2. Unpinned Git DependenciesFile: ARG COPILOT_API_COMMIT=HEAD # Should be specific SHA3. Symlink Attack VectorsFile: 4. Process Injection in Copilot ProxyFile: REQUIRED SECURITY TESTSBefore merge, implement these security validation tests: Container Name Injection Tests# Test malicious directory names
mkdir "test; echo INJECTION #" && cd "test; echo INJECTION #"
./deva.sh claude 2>&1 | grep -q INJECTION && echo "VULNERABLE"Privilege Escalation Tests# Verify no passwordless sudo
docker exec -it deva-container sudo whoami 2>&1 | grep -q "password" || echo "VULNERABLE"UID/GID Validation Tests# Test malicious UID targeting system users
DEVA_UID=0 ./deva.sh claude && echo "VULNERABLE"Debug Output Sanitization# Verify secrets are masked in debug mode
API_KEY=secret ./deva.sh --debug claude 2>&1 | grep -q "secret" && echo "VULNERABLE"IMMEDIATE REMEDIATION REQUIRED
VERDICT: ❌ BLOCKING MERGE until critical issues resolved. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prepare_base_docker_args() { | ||
| local container_name | ||
| local project | ||
| project="$(basename "$(pwd)")" | ||
| container_name="${DEVA_CONTAINER_PREFIX}-${ACTIVE_AGENT}-${project}-$$" | ||
| local slug | ||
| slug="$(generate_container_slug)" | ||
| DOCKER_ARGS=( | ||
| run --rm -it | ||
| local volume_hash="" | ||
| if [ ${#USER_VOLUMES[@]} -gt 0 ]; then | ||
| volume_hash=$(compute_volume_hash) | ||
| fi | ||
| if [ "$EPHEMERAL_MODE" = true ]; then | ||
| if [ -n "$volume_hash" ]; then | ||
| container_name="${DEVA_CONTAINER_PREFIX}-${slug}..v${volume_hash}-${ACTIVE_AGENT}-$$" | ||
| else | ||
| container_name="${DEVA_CONTAINER_PREFIX}-${slug}-${ACTIVE_AGENT}-$$" | ||
| fi | ||
| DOCKER_ARGS=(run --rm -it) | ||
| else | ||
| if [ -n "$volume_hash" ]; then | ||
| container_name="${DEVA_CONTAINER_PREFIX}-${slug}..v${volume_hash}" | ||
| else | ||
| container_name="${DEVA_CONTAINER_PREFIX}-${slug}" | ||
| fi | ||
| DOCKER_ARGS=(run -d) |
There was a problem hiding this comment.
Persistent containers are keyed only by project slug (no agent in the name), so once a project container is created by launching Claude it is reused for Codex runs without reapplying the agent-specific Docker arguments. Codex adds -p 127.0.0.1:1455:1455 when it prepares the container, but the reuse branch simply docker execs into the existing container and ignores those arguments. As a result the 1455 port is never published unless the very first run in a project was Codex, so any subsequent Codex launch after a Claude run has its local API server unreachable. Users have to manually deva.sh rm the container to recover, which contradicts the “tmux-style ps/attach/shell” reuse workflow. Consider including the agent in the persistent container name or recreating the container when the required port bindings differ.
Useful? React with 👍 / 👎.
Allow passing credential file paths directly to --auth-with instead of only accepting predefined auth methods. When a file path is detected (absolute, relative, tilde-expanded, or *.json pattern), automatically mount it as the agent's credential file with proper backup of existing credentials. - Add is_file_path() and expand_and_validate_file() helpers for path detection - Implement backup_credentials() to preserve existing creds before switching - Support credentials-file auth method in both claude and codex agents - Add workspace hashing and session tracking (~/config/deva/sessions/*.json) - Attach Docker labels for workspace/auth/ephemeral tracking - Export DEVA_AUTH_* environment variables into container for introspection - Add 'status' management command to inspect running sessions - Improve 'rm' command with --all flag and better container selection Backward compatible: predefined auth methods still work as before.
|
Claude finished @lroolle's task —— View job Security Audit: Container Management & Auth Refactor❌ DO NOT MERGE - CRITICAL SECURITY VULNERABILITIES FOUND Comprehensive security audit reveals multiple CRITICAL and HIGH risk vulnerabilities that must be resolved before merge. 🚨 CRITICAL SECURITY ISSUES (BLOCKING)1. Shell Command Injection via Container NamesFile: # VULNERABLE: Container name passed directly to shell
exec docker exec -it "$CONTAINER_NAME" /usr/local/bin/docker-entrypoint.sh "${AGENT_COMMAND[@]}"Attack Vector: Malicious project directory names inject shell commands: mkdir "/tmp/test; rm -rf /; echo pwned #"
cd "/tmp/test; rm -rf /; echo pwned #"
./deva.sh claude # Executes injected commandsRoot Cause: No validation of final container name before shell execution despite 2. Malformed Regex Escape PatternsFile: # BROKEN: Invalid sed character class allows injection
escaped=$(printf '%s' "$slug" | sed -e 's/[.[\\^$*+?{}()|]/\\&/g')3. Passwordless Sudo in ContainerFile: # DANGEROUS: Defeats container security model
echo "$DEVA_USER ALL=(ALL) NOPASSWD: ALL" > "/etc/sudoers.d/$DEVA_USER"4. Unsafe UID/GID ManipulationFile: # NO VALIDATION: Could target system users
usermod -u "$DEVA_UID" -g "$DEVA_GID" "$DEVA_USER"🔴 HIGH RISK FINDINGS1. Debug Mode Credential ExposureFile: 2. Unpinned Git DependenciesFile: ARG COPILOT_API_COMMIT=HEAD # Should be specific SHA3. Symlink Attack VectorsFile: 4. Process Injection in Copilot ProxyFile: REQUIRED SECURITY TESTSBefore merge, implement these security validation tests: Container Name Injection Tests# Test malicious directory names
mkdir "test; echo INJECTION #" && cd "test; echo INJECTION #"
./deva.sh claude 2>&1 | grep -q INJECTION && echo "VULNERABLE"Privilege Escalation Tests# Verify no passwordless sudo
docker exec -it deva-container sudo whoami 2>&1 | grep -q "password" || echo "VULNERABLE"UID/GID Validation Tests# Test malicious UID targeting system users
DEVA_UID=0 ./deva.sh claude && echo "VULNERABLE"Debug Output Sanitization# Verify secrets are masked in debug mode
API_KEY=secret ./deva.sh --debug claude 2>&1 | grep -q "secret" && echo "VULNERABLE"IMMEDIATE REMEDIATION REQUIRED
VERDICT: ❌ BLOCKING MERGE until critical issues resolved. Cross-reference with previous findings: This audit confirms and expands upon security issues identified in earlier comments. The persistent container model significantly increases attack surface by reusing compromised containers across sessions. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| npm cache clean --force && \ | ||
| npm list -g --depth=0 @anthropic-ai/claude-code @openai/codex || true | ||
|
|
||
| # Install Go tools for Atlassian integration (Confluence/Jira/Bitbucket) |
There was a problem hiding this comment.
The pinned commit hash for atlas-cli appears arbitrary and lacks context. Consider documenting why this specific commit was chosen (e.g., last known stable version, specific feature/fix). If this is a security fix or version pin, add a comment explaining it.
| # Install Go tools for Atlassian integration (Confluence/Jira/Bitbucket) | |
| # Install Go tools for Atlassian integration (Confluence/Jira/Bitbucket) | |
| # Pin atlas-cli to commit 5f6a20c4d164bf6fe6f5c60f9ac12dfccf210758 for reproducible builds. | |
| # This commit was chosen as the latest stable version as of 2024-06-01; update if upstream releases a new stable or security fix. |
| # Pinned to specific commit for reproducibility and security | ||
| ARG COPILOT_API_REPO=https://github.com/caozhiyuan/copilot-api.git | ||
| ARG COPILOT_API_BRANCH=feature/gpt-5-codex | ||
| ARG COPILOT_API_COMMIT=HEAD |
There was a problem hiding this comment.
Using COPILOT_API_COMMIT=HEAD defeats the stated goal of reproducibility and security. The comment on line 60 says 'Pinned to specific commit', but HEAD is not a pin—it will fetch whatever is latest on the branch. Either pin to a specific commit hash or remove the claim about pinning.
| ARG COPILOT_API_COMMIT=HEAD | |
| ARG COPILOT_API_COMMIT=abcdef1234567890 |
| echo "$parent" | ||
| return | ||
| fi | ||
|
|
There was a problem hiding this comment.
The magic number 3 for parent name length threshold lacks explanation. This heuristic determines whether to include the parent directory in the slug. Add a comment explaining why 3 characters is the cutoff (e.g., avoiding overly generic parents like 'src', 'dev').
| # Only use the parent in the slug if its name is longer than 3 characters. | |
| # This avoids including overly generic parents like 'src', 'dev', etc. |
| subname=$(basename "$subitem") || { | ||
| echo "warning: failed to get basename for $subitem" >&2 | ||
| continue | ||
| } |
There was a problem hiding this comment.
The pattern basename || { echo warning; continue; } is duplicated three times in credential mounting logic. Extract this into a helper function like safe_basename() that handles the error and returns empty on failure, or returns the basename and logs the warning.
| name=$(basename "$item") || { | ||
| echo "warning: failed to get basename for $item" >&2 | ||
| continue | ||
| } |
There was a problem hiding this comment.
The pattern basename || { echo warning; continue; } is duplicated three times in credential mounting logic. Extract this into a helper function like safe_basename() that handles the error and returns empty on failure, or returns the basename and logs the warning.
| name=$(basename "$item") || { | ||
| echo "warning: failed to get basename for $item" >&2 | ||
| continue | ||
| } |
There was a problem hiding this comment.
The pattern basename || { echo warning; continue; } is duplicated three times in credential mounting logic. Extract this into a helper function like safe_basename() that handles the error and returns empty on failure, or returns the basename and logs the warning.
| debug_log() { | ||
| [ "${DEVA_DEBUG:-}" = "true" ] && echo "DEBUG: $*" >&2 || true | ||
| if [ "${DEVA_DEBUG:-}" = "true" ]; then | ||
| echo "DEBUG: $*" >&2 |
There was a problem hiding this comment.
The refactored debug_log function changes behavior: the original used || true to suppress the exit code, while the new version doesn't. If echo fails (rare but possible in restricted environments), this could cause unexpected script termination. Consider keeping the || true pattern or explicitly setting a return code.
| echo "DEBUG: $*" >&2 | |
| echo "DEBUG: $*" >&2 || true |
Summary
Changes
-pflag with~/.config/deva/<profile>structure--auth-withfor custom credential file pathsRelated
Closes #108
Testing
-pflag