feat: Simple volume mount validation for Linux#115
Merged
Conversation
Simple, maintainable approach focused on preventing common mistakes: - Named volumes: Always allowed (managed by Docker, safe) - Default blocklist: /etc, /root, /var/run/docker.sock, credential dirs - YOLO mode: SAFETY_YOLO_MODE=true bypasses all validation - Configurable: SAFETY_VOLUME_MOUNT_BLOCKLIST, SAFETY_VOLUME_MOUNT_ALLOWLIST Philosophy: Prevent accidents, trust users for advanced cases. No complex Windows path normalization or edge case handling. About 85 lines of code vs 1000+ in previous over-engineered approach. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Simple, maintainable approach (~85 lines): - Named volume detection (always safe) - Basic path normalization (collapse slashes, handle Windows separators) - Default Linux blocklist (/etc, /root, docker.sock, credential dirs) - Optional allowlist for strict mode - YOLO mode to bypass all checks Philosophy: Prevent common Linux mistakes, trust users for advanced cases. Not a security fortress - just accident prevention. Configuration: - SAFETY_YOLO_MODE: Bypass all checks (user responsibility) - SAFETY_VOLUME_MOUNT_BLOCKLIST: Custom blocklist - SAFETY_VOLUME_MOUNT_ALLOWLIST: Restrict to specific paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Removed 4 Gemini workflow files that were accidentally added: - gemini-dispatch.yml - gemini-invoke.yml - gemini-triage.yml - gemini-scheduled-triage.yml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![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".
Removed security review markdown files from version control: - security_review_gpt-5.md - security_review_claude.md - security_review_tasks.md - security_review_gemini.md Files are preserved locally and added to .gitignore. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed volume_mount_validation_proposal.md from version control. File is preserved locally and added to .gitignore. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added 20 new unit tests for the simple volume mount validation: Named Volume Detection (4 tests): - Simple alphanumeric volume names - Paths with separators (not named volumes) - Volumes starting with dot - Special characters Mount Path Validation (16 tests): - YOLO mode bypasses all checks - Named volumes always allowed - Safe paths allowed - Default blocklist (etc, root, docker socket, SSH, credentials) - Custom blocklist - Empty blocklist - Path normalization (duplicate slashes, Windows separators) - Allowlist restricts to specific paths - Allowlist + blocklist interaction - Error messages include path and suggest YOLO mode All 78 safety unit tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
🧪 Test Results for
|
| Test Suite | Status | Passed | Failed | Skipped | Duration |
|---|---|---|---|---|---|
| Unit Tests - Python 3.11 | ✅ | 850 | 0 | 0 | 57s |
| Unit Tests - Python 3.13 | ✅ | 850 | 0 | 0 | 1.0m |
| Unit Tests - Python 3.12 | ✅ | 850 | 0 | 0 | 1.0m |
| Unit Tests - Python 3.14 | ✅ | 850 | 0 | 0 | 59s |
| E2E | ✅ | 62 | 0 | 0 | 1.3m |
| Integration | ✅ | 83 | 0 | 1 | 1.9m |
| Total | ✅ | 3545 | 0 | 1 | - |
📊 Coverage
Coverage reports are available in the workflow artifacts.
📦 Download Reports
Improved volume mount validation to catch credential directories anywhere in the path, not just at root level. Changes: - Credential dirs (.ssh, .aws, .kube, .docker) now use substring matching - Blocks /home/user/.ssh, /opt/app/.docker, etc. - System paths (/etc, /root, docker.sock) use prefix matching - Credential protection is always active (even with empty blocklist) Tests: - Added 3 new tests for credential directory substring matching - Updated existing tests to verify new behavior - All 79 safety unit tests passing This closes the security gap where /home/user/.ssh was previously allowed despite comments saying credential dirs were blocked. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The SECURITY.md documentation claimed the startup scripts enable "TLS, OAuth, and security features" but both scripts explicitly set SECURITY_OAUTH_ENABLED=false and display "Authentication: DISABLED". Changes: - Updated SECURITY.md to accurately describe what's enabled by default - Added clear checkmarks showing OAuth is disabled by default - Added note that OAuth must be manually enabled by editing scripts - Scripts themselves were already honest (comments and output show disabled) What the scripts actually enable: ✅ TLS/HTTPS (requires certs) ✅ Rate limiting (60 req/min) ✅ Audit logging ❌ OAuth/OIDC (disabled, must manually enable) This fixes the discrepancy between docs and actual script behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
CRITICAL SECURITY FIX: Environment variables in docker exec were not
validated, allowing command injection attacks.
Attack Vector:
- ExecCommandTool accepted arbitrary environment variables
- Combined with shell commands like ["sh", "-c", "$VAR"]
- Attacker payload: {"environment": {"X": "$(cat /etc/passwd)"}}
- Shell would execute the command substitution
Fix:
- Enhanced validate_environment_variable() to reject dangerous chars
- Blocks: $(, `, ;, &, |, \n, \r (command injection characters)
- Added validation loop in ExecCommandTool.execute()
- Safe values (paths, URLs, flags) still allowed
Tests:
- Added 8 new unit tests for command injection scenarios
- All 87 safety unit tests passing
- Verified attacks are blocked while legitimate values work
Security Impact: CVSS 8.1 → Mitigated
Status: H1 HIGH PRIORITY issue resolved
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
CRITICAL SECURITY FIX: Environment variables were exposed in
generate_compose prompt, leaking secrets to remote LLM APIs.
Attack Vector:
- generate_compose prompt included full env var values
- Example: DATABASE_URL=postgresql://user:PASSWORD@db/app
- Secrets sent to Claude API, OpenAI API, etc.
- Potentially logged, used for training, or exposed
Fix:
- Redact all environment variable values in prompts
- Show only keys: DATABASE_URL=<REDACTED>
- Added warning in prompt description
- Documented protection in SECURITY.md
Implementation:
- Simple redaction: var.split("=", 1)[0] + "=<REDACTED>"
- Applied in GenerateComposePrompt.generate()
- LLM still knows which env vars exist (enough for compose generation)
- No secrets sent to remote APIs
Tests:
- Added comprehensive test with real secret examples
- Verifies passwords, API keys, AWS secrets NOT in output
- Verifies keys shown with <REDACTED>
- All 7 GenerateComposePrompt tests passing
Documentation:
- Added "Secret Redaction in Prompts" section to SECURITY.md
- Explains the risk, protection mechanism, and scope
- Updated overview to list this as security feature #10
Security Impact: HIGH severity credential disclosure → Mitigated
Status: H2 HIGH PRIORITY issue resolved
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
SECURITY FIX: Rate limiter created unbounded semaphores for each unique client_id, allowing attackers to exhaust memory with many fake client IDs. Changes: - Add max_clients limit to RateLimiter (default: 10, max: 100) - Reject new clients when limit reached with clear error message - Add SECURITY_RATE_LIMIT_MAX_CLIENTS config option (env var) - Existing clients unaffected - can still acquire slots Implementation: - src/mcp_docker/security/rate_limiter.py: Add max_clients check - src/mcp_docker/config.py: Add rate_limit_max_clients field - src/mcp_docker/server.py: Pass config to RateLimiter - tests/unit/test_security/test_rate_limiter.py: 3 new tests Tests: - test_init_max_clients: Verify max_clients is set - test_max_clients_limit_enforced: Verify limit blocks new clients - test_existing_client_can_acquire_at_max_clients: Existing clients OK All 19 rate limiter tests passing. Security Impact: HIGH severity DoS → Mitigated Default: 10 max clients (configurable 1-100) Status: H5 HIGH PRIORITY issue resolved 🤖 Generated with Claude Code
williajm
force-pushed
the
fix/simple-volume-validation
branch
from
680bccd to
a1ff724
Compare
Add test coverage to verify that dangerous patterns are properly detected in list-form commands (e.g., ['rm', '-rf', '/']). While the implementation already correctly validates list commands by joining them to strings before pattern matching, there were no tests explicitly verifying this behavior. Tests Added (6 new, all fast unit tests): - test_sanitize_command_list_rm_rf_root: Verify rm -rf / blocked - test_sanitize_command_list_shutdown: Verify shutdown blocked - test_sanitize_command_list_curl_pipe_bash: Verify curl|bash blocked - test_sanitize_command_list_chmod_777_root: Verify chmod 777 / blocked - test_sanitize_command_list_dd_disk_wipe: Verify dd disk wipe blocked - test_sanitize_command_list_safe_commands: Verify safe commands allowed All 93 safety tests passing in 0.12s. Addresses M5 concern from security review (no code changes needed, implementation was already correct, just missing test coverage). 🤖 Generated with Claude Code
SECURITY FIX: Audit log directory and files were created with world-readable permissions (0o755/0o644), allowing any user on the system to read sensitive operation logs. Changes: - Set directory permissions to 0o700 (owner-only access) - Set file permissions to 0o600 (owner-only read/write) - Fix permissions on existing directories (chmod after mkdir) Implementation: - src/mcp_docker/security/audit.py: Add chmod calls after mkdir/logger.add - tests/unit/test_security/test_audit.py: 3 new permission tests Tests Added (3 new, all fast unit tests): - test_audit_log_directory_permissions: Verify directory is 0o700 - test_audit_log_file_permissions: Verify file is 0o600 - test_audit_log_permissions_existing_directory: Verify existing dirs fixed All 11 audit logger tests passing in 0.21s. Security Impact: LOW severity information disclosure → Mitigated Permissions: Directory 0o700, File 0o600 (owner-only) Status: L2 LOW PRIORITY issue resolved 🤖 Generated with Claude Code
Version bump and changelog update for v1.1.1 release. This release includes critical security fixes and new safety features: - Command injection prevention (H1) - Secret redaction in prompts (H2) - Rate limiter DoS protection (H5) - Simple volume mount validation - Audit log permissions hardening (L2) Changes: - pyproject.toml: version 1.1.1.dev0 → 1.1.1 - CHANGELOG.md: Move Unreleased to [1.1.1] - 2025-11-15 - Added: Volume mount validation, rate limiter max clients, audit log permissions - Security: 4 security fixes (H1, H2, H5, L2) - Fixed: Documentation accuracy for OAuth - Tests: 41 new unit tests, all passing Ready for release. 🤖 Generated with Claude Code
Extract container context building into separate helper method to reduce cognitive complexity from 16 to 15. Changes: - Add _build_container_context() helper method - Move all container data extraction and formatting logic to helper - Simplify generate() method by calling helper instead of inline logic Benefits: - Reduced cognitive complexity (16 → 15) - Better separation of concerns - More testable code structure - No functional changes All 7 GenerateComposePrompt tests passing. 🤖 Generated with Claude Code
SECURITY FIX: Volume mount validation was vulnerable to path traversal bypass attacks using .. segments (e.g., ../../etc). Attack Vector: - Attacker provides: "../../etc" or "../../../../var/run/docker.sock" - Previous validation: "/../../etc" doesn't start with "/etc" ✗ BYPASSED - Docker resolves the path, potentially mounting blocked directories Fix: - Reject any mount path containing ".." segments - Simple check: if ".." in normalized → raise UnsafeOperationError - Clear error message directs users to use absolute paths - YOLO mode bypass available for advanced users Changes: - src/mcp_docker/utils/safety.py: Add .. check after path normalization - tests/unit/test_safety.py: 5 new tests for path traversal Tests Added (5 new, all passing in 0.10s): - test_validate_mount_path_blocks_path_traversal_leading: ../../etc - test_validate_mount_path_blocks_path_traversal_middle: /home/user/../../etc - test_validate_mount_path_blocks_path_traversal_multiple: ../../../../var/run/docker.sock - test_validate_mount_path_blocks_path_traversal_docker_socket: ../../var/run/docker.sock - test_validate_mount_path_yolo_mode_allows_path_traversal: YOLO mode bypass works All 98 safety tests passing in 0.12s. Security Impact: MEDIUM-HIGH severity path traversal → Mitigated Status: P1 CRITICAL issue resolved 🤖 Generated with Claude Code
Owner
Author
|
@codex review |
Owner
Author
|
@claude review |
|
Claude finished @williajm's task —— View job Code ReviewI've reviewed the PR implementing simple volume mount validation for Linux. Here's my comprehensive analysis: SummaryThis PR implements a pragmatic approach to volume mount validation that focuses on preventing common mistakes while remaining maintainable. The implementation is clean and well-tested, with 94% coverage on new code. Strengths
Critical Security Issue (P1)The Codex P1 review comment is valid and needs to be addressed. Issue: Credential directories are only blocked via substring matching, but the current blocklist entries like Evidence from code ( # Default credential directories (substring matching to catch /home/user/.ssh etc.)
credential_dirs = ["/.ssh", "/.aws", "/.kube", "/.docker"]
# Check system paths (prefix matching)
for blocked in blocked_paths:
if normalized.startswith(blocked): # Prefix matching
raise UnsafeOperationError(...)
# Check credential directories (substring matching to catch any user)
for cred_dir in credential_dirs:
if cred_dir in normalized: # Substring matching - THIS WORKS CORRECTLY
raise UnsafeOperationError(...)Actually, the implementation is CORRECT! The credential directories use Verification from tests ( def test_validate_mount_path_credential_dirs_under_home(self) -> None:
"""Test credential directories are blocked under /home (substring matching)."""
# This is the key test - credential dirs anywhere in path are blocked
with pytest.raises(UnsafeOperationError, match="credential directory"):
validate_mount_path("/home/user/.ssh") # ✅ BLOCKED
with pytest.raises(UnsafeOperationError, match="credential directory"):
validate_mount_path("/home/jmw/.aws") # ✅ BLOCKEDConclusion: The Codex P1 concern is incorrect. The implementation correctly blocks credential directories anywhere in the path via substring matching. The tests confirm this works as intended. Minor Issues1. Missing Coverage (4 lines in
|
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".
|
Code Review - PR #115 Summary: This PR implements simple volume mount validation and addresses multiple security vulnerabilities. Excellent security work with strong test coverage. Strengths:
Potential Issues:
Recommendations: Final Verdict: APPROVE with minor recommendations This PR significantly improves security posture. The volume mount validation strikes the right balance between safety and usability. Critical security fixes are solid. Test coverage is outstanding, code quality is high. Great work! |
CRITICAL BUG FIX: Rate limiter never removed client semaphores when concurrent requests reached zero, causing permanent DoS after max_clients unique clients had connected once. The Problem: - acquire_concurrent_slot() checks: len(self._semaphores) >= max_clients - release_concurrent_slot() decremented counter but never removed semaphore - After 10 clients connected once (max_clients=10), semaphores dict had 10 entries - All subsequent clients rejected forever, even with zero active load - Turned memory-exhaustion protection into permanent denial of service The Fix: - release_concurrent_slot() now removes semaphore when concurrent_requests == 0 - Cleanup happens automatically when client becomes idle - max_clients now applies to ACTIVE clients, not "all clients ever seen" Changes: - src/mcp_docker/security/rate_limiter.py: Add cleanup logic in release_concurrent_slot() - tests/unit/test_security/test_rate_limiter.py: 3 new tests Tests Added (3 new, all passing in 0.27s): - test_client_cleanup_when_idle: Verify cleanup when concurrent_requests reaches 0 - test_new_clients_allowed_after_cleanup: Verify no permanent DoS after max_clients - test_partial_cleanup_with_multiple_slots: Verify cleanup only when ALL slots released All 22 rate limiter tests passing in 1.49s. Bug Impact: CRITICAL permanent DoS after N clients → Fixed Status: P1 CRITICAL issue resolved 🤖 Generated with Claude Code
Owner
Author
|
@codex review |
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".
Fixed race condition in rate limiter cleanup logic where concurrent releases would delete semaphore entries while other threads were still accessing them, causing KeyError in integration tests. Solution: Instead of deleting idle client entries, count only ACTIVE clients (concurrent_requests > 0) toward max_clients limit. This prevents both race conditions and permanent DoS attacks. Changes: - Modified acquire_concurrent_slot() to count only active clients - Simplified release_concurrent_slot() to remove cleanup logic - Updated 3 unit tests to reflect new behavior (no cleanup) Tests: All 22 unit tests and 83 integration tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
Added race condition fix to v1.1.1 Fixed section documenting the KeyError bug fix in concurrent operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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".
Fixed memory leak where attackers could cycle through unbounded unique client IDs, each leaving an idle semaphore entry in memory. Previous fix counted only active clients (count > 0), allowing attackers to bypass the limit by using sequential unique IDs. Each would connect, disconnect, and leave an idle entry that didn't count toward max_clients. Solution: Check total dictionary size (len(_semaphores)) instead of counting only active entries. This prevents unbounded memory growth. Tradeoff: After max_clients unique clients connect, no new clients can connect until server restart. But this is acceptable for DoS protection that prioritizes server stability over perfect UX. Changes: - Modified acquire_concurrent_slot() to check len(_semaphores) - Updated test to verify new clients blocked at max tracked clients - Updated CHANGELOG to document the fix Tests: All 22 unit tests and 2 integration tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
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".
Fixes permanent client banning bug where new clients were permanently rejected after max_clients capacity was reached, even when previous clients were idle. Solution: When at max_clients capacity, evict the first idle client (count==0) to make room for new client. Only reject when all tracked clients have active requests. Changes: - Rate limiter now evicts idle clients when at capacity (LRU policy) - Only rejects new clients when all tracked clients are active - Balances memory protection with normal multi-user operation - Prevents permanent DoS from idle client accumulation Tests: - test_idle_client_eviction_allows_new_clients: Verifies eviction - test_active_clients_block_new_clients: Verifies rejection logic - All 23 rate limiter tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
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".
Comment on lines
+512
to
+515
| "`", # Backtick command substitution | ||
| ";", # Command separator | ||
| "&", # Background execution / command chaining | ||
| "|", # Pipe to another command |
There was a problem hiding this comment.
The new validation treats & and | as "dangerous characters" and raises ValidationError whenever they appear in an environment variable value. However, environment variables passed to ExecCommandTool are sent to the Docker API as structured data, not through a shell, so characters such as & or | are perfectly safe and quite common in connection strings or URLs (e.g. postgres://…?sslmode=require&pool=10). After this change such legitimate values are rejected and the exec command cannot run. Consider limiting the validation to cases where the value is actually interpolated into a shell command, or removing these characters from the block list to avoid breaking normal usage.
Useful? React with 👍 / 👎.
…nerTool Code review identified that environment variable validation for command injection was only applied in ExecCommandTool, not in CreateContainerTool. This left the H1 command injection vulnerability exploitable when creating containers with malicious environment variables. Changes: - Import validate_environment_variable in container_lifecycle_tools.py - Add validation loop in CreateContainerTool._validate_inputs() - Validates all environment variables for dangerous characters before creation - Prevents command injection via $(cmd), backticks, semicolons, pipes, etc. Impact: - Closes H1 security gap in docker_create_container tool - Validation now applied consistently across both container creation and exec Testing: - Existing unit tests validate the function works (12 tests) - Integration test confirms validation is applied in container creation - All tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed overly strict validation that blocked & and | characters in environment variable values. These are safe because Docker passes env vars as structured data to the API, not through a shell. Why this is safe: - Docker SDK passes environment variables as dict to API - Not interpolated into shell commands - Only dangerous if value is later used in a shell command context Why this matters: - Connection strings commonly use & for parameters Example: postgres://localhost?sslmode=require&pool=10 - Filter strings may use | for OR logic Example: status=active|ready Still blocking truly dangerous characters: - $( - command substitution - ` - backtick substitution - ; - command separator - \n - newline injection - \r - carriage return injection Changes: - Removed & and | from dangerous_chars list in validate_environment_variable() - Updated tests to verify ampersands and pipes are allowed - Added realistic test cases with database connection strings Testing: - All 12 environment variable validation tests passing - Tests verify legitimate connection strings now work 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
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".
williajm
added a commit
that referenced
this pull request
Nov 20, 2025
…urity (#68) ## Problem OpenSSF Scorecard flagged 6 critical security alerts for unpinned dependencies in the ClusterFuzzLite fuzzing infrastructure, creating potential supply chain vulnerabilities. ## Changes ### Dockerfile (.clusterfuzzlite/Dockerfile) 1. **Pin Docker base image by SHA256 hash** - Before: `gcr.io/oss-fuzz-base/base-builder-python` - After: `gcr.io/oss-fuzz-base/base-builder-python@sha256:122afa...` - Ensures reproducible builds and prevents malicious image substitution 2. **Pin pip to specific version** - Pinned to pip==24.3.1 - Ensures consistent package resolution across builds 3. **Pin all Python dependencies** - atheris==2.3.0 (fuzzing engine) - pydantic==2.10.4 (validation library) - pydantic-settings==2.6.1 (settings management) - loguru==0.7.3 (logging) - cryptography==44.0.0 (SSH authentication) ### Build Script (.clusterfuzzlite/build.sh) 1. **Add --no-deps flag to local install** - Prevents pip from installing unpinned transitive dependencies - All dependencies are already installed with pinned versions in Dockerfile 2. **Add documentation comments** - Explains why local project install doesn't need version pinning - Clarifies that external dependencies are pinned elsewhere ## Security Impact - ✅ Resolves 6 Pinned-Dependencies security alerts - ✅ Improves OpenSSF Scorecard rating - ✅ Prevents supply chain attacks via dependency confusion - ✅ Ensures reproducible fuzzing builds - ✅ Maintains compatibility with existing fuzz tests ## Testing - All pinned versions meet minimum requirements in pyproject.toml - Dependencies tested in main CI pipeline - No functional changes to fuzzing behavior ## Related - Addresses: https://github.com/williajm/mcp_docker/security/code-scanning - Alerts: #115, #116, #117, #118, #119, #120 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
williajm
added a commit
that referenced
this pull request
Nov 20, 2025
* feat: Add simple volume mount validation for Linux Simple, maintainable approach focused on preventing common mistakes: - Named volumes: Always allowed (managed by Docker, safe) - Default blocklist: /etc, /root, /var/run/docker.sock, credential dirs - YOLO mode: SAFETY_YOLO_MODE=true bypasses all validation - Configurable: SAFETY_VOLUME_MOUNT_BLOCKLIST, SAFETY_VOLUME_MOUNT_ALLOWLIST Philosophy: Prevent accidents, trust users for advanced cases. No complex Windows path normalization or edge case handling. About 85 lines of code vs 1000+ in previous over-engineered approach. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add simple volume mount validation for Linux Simple, maintainable approach (~85 lines): - Named volume detection (always safe) - Basic path normalization (collapse slashes, handle Windows separators) - Default Linux blocklist (/etc, /root, docker.sock, credential dirs) - Optional allowlist for strict mode - YOLO mode to bypass all checks Philosophy: Prevent common Linux mistakes, trust users for advanced cases. Not a security fortress - just accident prevention. Configuration: - SAFETY_YOLO_MODE: Bypass all checks (user responsibility) - SAFETY_VOLUME_MOUNT_BLOCKLIST: Custom blocklist - SAFETY_VOLUME_MOUNT_ALLOWLIST: Restrict to specific paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Remove accidental Gemini workflows Removed 4 Gemini workflow files that were accidentally added: - gemini-dispatch.yml - gemini-invoke.yml - gemini-triage.yml - gemini-scheduled-triage.yml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Remove security review files from git, keep locally Removed security review markdown files from version control: - security_review_gpt-5.md - security_review_claude.md - security_review_tasks.md - security_review_gemini.md Files are preserved locally and added to .gitignore. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Remove validation proposal from git, keep locally Removed volume_mount_validation_proposal.md from version control. File is preserved locally and added to .gitignore. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test: Add comprehensive unit tests for volume mount validation Added 20 new unit tests for the simple volume mount validation: Named Volume Detection (4 tests): - Simple alphanumeric volume names - Paths with separators (not named volumes) - Volumes starting with dot - Special characters Mount Path Validation (16 tests): - YOLO mode bypasses all checks - Named volumes always allowed - Safe paths allowed - Default blocklist (etc, root, docker socket, SSH, credentials) - Custom blocklist - Empty blocklist - Path normalization (duplicate slashes, Windows separators) - Allowlist restricts to specific paths - Allowlist + blocklist interaction - Error messages include path and suggest YOLO mode All 78 safety unit tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Enhance credential directory protection with substring matching Improved volume mount validation to catch credential directories anywhere in the path, not just at root level. Changes: - Credential dirs (.ssh, .aws, .kube, .docker) now use substring matching - Blocks /home/user/.ssh, /opt/app/.docker, etc. - System paths (/etc, /root, docker.sock) use prefix matching - Credential protection is always active (even with empty blocklist) Tests: - Added 3 new tests for credential directory substring matching - Updated existing tests to verify new behavior - All 79 safety unit tests passing This closes the security gap where /home/user/.ssh was previously allowed despite comments saying credential dirs were blocked. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Fix misleading claims about OAuth in startup scripts The SECURITY.md documentation claimed the startup scripts enable "TLS, OAuth, and security features" but both scripts explicitly set SECURITY_OAUTH_ENABLED=false and display "Authentication: DISABLED". Changes: - Updated SECURITY.md to accurately describe what's enabled by default - Added clear checkmarks showing OAuth is disabled by default - Added note that OAuth must be manually enabled by editing scripts - Scripts themselves were already honest (comments and output show disabled) What the scripts actually enable: ✅ TLS/HTTPS (requires certs) ✅ Rate limiting (60 req/min) ✅ Audit logging ❌ OAuth/OIDC (disabled, must manually enable) This fixes the discrepancy between docs and actual script behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * security: Fix command injection via environment variables (H1) CRITICAL SECURITY FIX: Environment variables in docker exec were not validated, allowing command injection attacks. Attack Vector: - ExecCommandTool accepted arbitrary environment variables - Combined with shell commands like ["sh", "-c", "$VAR"] - Attacker payload: {"environment": {"X": "$(cat /etc/passwd)"}} - Shell would execute the command substitution Fix: - Enhanced validate_environment_variable() to reject dangerous chars - Blocks: $(, `, ;, &, |, \n, \r (command injection characters) - Added validation loop in ExecCommandTool.execute() - Safe values (paths, URLs, flags) still allowed Tests: - Added 8 new unit tests for command injection scenarios - All 87 safety unit tests passing - Verified attacks are blocked while legitimate values work Security Impact: CVSS 8.1 → Mitigated Status: H1 HIGH PRIORITY issue resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * security: Redact environment variable values in prompts (H2) CRITICAL SECURITY FIX: Environment variables were exposed in generate_compose prompt, leaking secrets to remote LLM APIs. Attack Vector: - generate_compose prompt included full env var values - Example: DATABASE_URL=postgresql://user:PASSWORD@db/app - Secrets sent to Claude API, OpenAI API, etc. - Potentially logged, used for training, or exposed Fix: - Redact all environment variable values in prompts - Show only keys: DATABASE_URL=<REDACTED> - Added warning in prompt description - Documented protection in SECURITY.md Implementation: - Simple redaction: var.split("=", 1)[0] + "=<REDACTED>" - Applied in GenerateComposePrompt.generate() - LLM still knows which env vars exist (enough for compose generation) - No secrets sent to remote APIs Tests: - Added comprehensive test with real secret examples - Verifies passwords, API keys, AWS secrets NOT in output - Verifies keys shown with <REDACTED> - All 7 GenerateComposePrompt tests passing Documentation: - Added "Secret Redaction in Prompts" section to SECURITY.md - Explains the risk, protection mechanism, and scope - Updated overview to list this as security feature #10 Security Impact: HIGH severity credential disclosure → Mitigated Status: H2 HIGH PRIORITY issue resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * security: Prevent rate limiter memory exhaustion DoS (H5) SECURITY FIX: Rate limiter created unbounded semaphores for each unique client_id, allowing attackers to exhaust memory with many fake client IDs. Changes: - Add max_clients limit to RateLimiter (default: 10, max: 100) - Reject new clients when limit reached with clear error message - Add SECURITY_RATE_LIMIT_MAX_CLIENTS config option (env var) - Existing clients unaffected - can still acquire slots Implementation: - src/mcp_docker/security/rate_limiter.py: Add max_clients check - src/mcp_docker/config.py: Add rate_limit_max_clients field - src/mcp_docker/server.py: Pass config to RateLimiter - tests/unit/test_security/test_rate_limiter.py: 3 new tests Tests: - test_init_max_clients: Verify max_clients is set - test_max_clients_limit_enforced: Verify limit blocks new clients - test_existing_client_can_acquire_at_max_clients: Existing clients OK All 19 rate limiter tests passing. Security Impact: HIGH severity DoS → Mitigated Default: 10 max clients (configurable 1-100) Status: H5 HIGH PRIORITY issue resolved 🤖 Generated with Claude Code * test: Add list-based command validation tests Add test coverage to verify that dangerous patterns are properly detected in list-form commands (e.g., ['rm', '-rf', '/']). While the implementation already correctly validates list commands by joining them to strings before pattern matching, there were no tests explicitly verifying this behavior. Tests Added (6 new, all fast unit tests): - test_sanitize_command_list_rm_rf_root: Verify rm -rf / blocked - test_sanitize_command_list_shutdown: Verify shutdown blocked - test_sanitize_command_list_curl_pipe_bash: Verify curl|bash blocked - test_sanitize_command_list_chmod_777_root: Verify chmod 777 / blocked - test_sanitize_command_list_dd_disk_wipe: Verify dd disk wipe blocked - test_sanitize_command_list_safe_commands: Verify safe commands allowed All 93 safety tests passing in 0.12s. Addresses M5 concern from security review (no code changes needed, implementation was already correct, just missing test coverage). 🤖 Generated with Claude Code * security: Set restrictive permissions on audit log files (L2) SECURITY FIX: Audit log directory and files were created with world-readable permissions (0o755/0o644), allowing any user on the system to read sensitive operation logs. Changes: - Set directory permissions to 0o700 (owner-only access) - Set file permissions to 0o600 (owner-only read/write) - Fix permissions on existing directories (chmod after mkdir) Implementation: - src/mcp_docker/security/audit.py: Add chmod calls after mkdir/logger.add - tests/unit/test_security/test_audit.py: 3 new permission tests Tests Added (3 new, all fast unit tests): - test_audit_log_directory_permissions: Verify directory is 0o700 - test_audit_log_file_permissions: Verify file is 0o600 - test_audit_log_permissions_existing_directory: Verify existing dirs fixed All 11 audit logger tests passing in 0.21s. Security Impact: LOW severity information disclosure → Mitigated Permissions: Directory 0o700, File 0o600 (owner-only) Status: L2 LOW PRIORITY issue resolved 🤖 Generated with Claude Code * chore: Release v1.1.1 Version bump and changelog update for v1.1.1 release. This release includes critical security fixes and new safety features: - Command injection prevention (H1) - Secret redaction in prompts (H2) - Rate limiter DoS protection (H5) - Simple volume mount validation - Audit log permissions hardening (L2) Changes: - pyproject.toml: version 1.1.1.dev0 → 1.1.1 - CHANGELOG.md: Move Unreleased to [1.1.1] - 2025-11-15 - Added: Volume mount validation, rate limiter max clients, audit log permissions - Security: 4 security fixes (H1, H2, H5, L2) - Fixed: Documentation accuracy for OAuth - Tests: 41 new unit tests, all passing Ready for release. 🤖 Generated with Claude Code * refactor: Reduce cognitive complexity in generate_compose prompt Extract container context building into separate helper method to reduce cognitive complexity from 16 to 15. Changes: - Add _build_container_context() helper method - Move all container data extraction and formatting logic to helper - Simplify generate() method by calling helper instead of inline logic Benefits: - Reduced cognitive complexity (16 → 15) - Better separation of concerns - More testable code structure - No functional changes All 7 GenerateComposePrompt tests passing. 🤖 Generated with Claude Code * security: Block path traversal in volume mount validation (P1) SECURITY FIX: Volume mount validation was vulnerable to path traversal bypass attacks using .. segments (e.g., ../../etc). Attack Vector: - Attacker provides: "../../etc" or "../../../../var/run/docker.sock" - Previous validation: "/../../etc" doesn't start with "/etc" ✗ BYPASSED - Docker resolves the path, potentially mounting blocked directories Fix: - Reject any mount path containing ".." segments - Simple check: if ".." in normalized → raise UnsafeOperationError - Clear error message directs users to use absolute paths - YOLO mode bypass available for advanced users Changes: - src/mcp_docker/utils/safety.py: Add .. check after path normalization - tests/unit/test_safety.py: 5 new tests for path traversal Tests Added (5 new, all passing in 0.10s): - test_validate_mount_path_blocks_path_traversal_leading: ../../etc - test_validate_mount_path_blocks_path_traversal_middle: /home/user/../../etc - test_validate_mount_path_blocks_path_traversal_multiple: ../../../../var/run/docker.sock - test_validate_mount_path_blocks_path_traversal_docker_socket: ../../var/run/docker.sock - test_validate_mount_path_yolo_mode_allows_path_traversal: YOLO mode bypass works All 98 safety tests passing in 0.12s. Security Impact: MEDIUM-HIGH severity path traversal → Mitigated Status: P1 CRITICAL issue resolved 🤖 Generated with Claude Code * fix: Cleanup idle clients in rate limiter to prevent permanent DoS CRITICAL BUG FIX: Rate limiter never removed client semaphores when concurrent requests reached zero, causing permanent DoS after max_clients unique clients had connected once. The Problem: - acquire_concurrent_slot() checks: len(self._semaphores) >= max_clients - release_concurrent_slot() decremented counter but never removed semaphore - After 10 clients connected once (max_clients=10), semaphores dict had 10 entries - All subsequent clients rejected forever, even with zero active load - Turned memory-exhaustion protection into permanent denial of service The Fix: - release_concurrent_slot() now removes semaphore when concurrent_requests == 0 - Cleanup happens automatically when client becomes idle - max_clients now applies to ACTIVE clients, not "all clients ever seen" Changes: - src/mcp_docker/security/rate_limiter.py: Add cleanup logic in release_concurrent_slot() - tests/unit/test_security/test_rate_limiter.py: 3 new tests Tests Added (3 new, all passing in 0.27s): - test_client_cleanup_when_idle: Verify cleanup when concurrent_requests reaches 0 - test_new_clients_allowed_after_cleanup: Verify no permanent DoS after max_clients - test_partial_cleanup_with_multiple_slots: Verify cleanup only when ALL slots released All 22 rate limiter tests passing in 1.49s. Bug Impact: CRITICAL permanent DoS after N clients → Fixed Status: P1 CRITICAL issue resolved 🤖 Generated with Claude Code * fix: Rate limiter race condition causing CI test failures Fixed race condition in rate limiter cleanup logic where concurrent releases would delete semaphore entries while other threads were still accessing them, causing KeyError in integration tests. Solution: Instead of deleting idle client entries, count only ACTIVE clients (concurrent_requests > 0) toward max_clients limit. This prevents both race conditions and permanent DoS attacks. Changes: - Modified acquire_concurrent_slot() to count only active clients - Simplified release_concurrent_slot() to remove cleanup logic - Updated 3 unit tests to reflect new behavior (no cleanup) Tests: All 22 unit tests and 83 integration tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Add rate limiter race condition fix to CHANGELOG Added race condition fix to v1.1.1 Fixed section documenting the KeyError bug fix in concurrent operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: P1 - Rate limiter memory exhaustion from unique client IDs Fixed memory leak where attackers could cycle through unbounded unique client IDs, each leaving an idle semaphore entry in memory. Previous fix counted only active clients (count > 0), allowing attackers to bypass the limit by using sequential unique IDs. Each would connect, disconnect, and leave an idle entry that didn't count toward max_clients. Solution: Check total dictionary size (len(_semaphores)) instead of counting only active entries. This prevents unbounded memory growth. Tradeoff: After max_clients unique clients connect, no new clients can connect until server restart. But this is acceptable for DoS protection that prioritizes server stability over perfect UX. Changes: - Modified acquire_concurrent_slot() to check len(_semaphores) - Updated test to verify new clients blocked at max tracked clients - Updated CHANGELOG to document the fix Tests: All 22 unit tests and 2 integration tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: P1 - Implement LRU eviction for idle clients in rate limiter Fixes permanent client banning bug where new clients were permanently rejected after max_clients capacity was reached, even when previous clients were idle. Solution: When at max_clients capacity, evict the first idle client (count==0) to make room for new client. Only reject when all tracked clients have active requests. Changes: - Rate limiter now evicts idle clients when at capacity (LRU policy) - Only rejects new clients when all tracked clients are active - Balances memory protection with normal multi-user operation - Prevents permanent DoS from idle client accumulation Tests: - test_idle_client_eviction_allows_new_clients: Verifies eviction - test_active_clients_block_new_clients: Verifies rejection logic - All 23 rate limiter tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Clean up CHANGELOG - remove internal debugging details * fix: CRITICAL - Apply environment variable validation in CreateContainerTool Code review identified that environment variable validation for command injection was only applied in ExecCommandTool, not in CreateContainerTool. This left the H1 command injection vulnerability exploitable when creating containers with malicious environment variables. Changes: - Import validate_environment_variable in container_lifecycle_tools.py - Add validation loop in CreateContainerTool._validate_inputs() - Validates all environment variables for dangerous characters before creation - Prevents command injection via $(cmd), backticks, semicolons, pipes, etc. Impact: - Closes H1 security gap in docker_create_container tool - Validation now applied consistently across both container creation and exec Testing: - Existing unit tests validate the function works (12 tests) - Integration test confirms validation is applied in container creation - All tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Clean up CHANGELOG - remove internal implementation details * fix: Allow ampersands and pipes in environment variables Removed overly strict validation that blocked & and | characters in environment variable values. These are safe because Docker passes env vars as structured data to the API, not through a shell. Why this is safe: - Docker SDK passes environment variables as dict to API - Not interpolated into shell commands - Only dangerous if value is later used in a shell command context Why this matters: - Connection strings commonly use & for parameters Example: postgres://localhost?sslmode=require&pool=10 - Filter strings may use | for OR logic Example: status=active|ready Still blocking truly dangerous characters: - $( - command substitution - ` - backtick substitution - ; - command separator - \n - newline injection - \r - carriage return injection Changes: - Removed & and | from dangerous_chars list in validate_environment_variable() - Updated tests to verify ampersands and pipes are allowed - Added realistic test cases with database connection strings Testing: - All 12 environment variable validation tests passing - Tests verify legitimate connection strings now work 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Simple, maintainable volume mount validation (~85 lines vs previous 1000+ line approach).
Philosophy: Prevent common Linux mistakes, trust users for advanced cases. Not a security fortress - just accident prevention.
What's New
/etc,/root,/var/run/docker.sock, credential dirsSAFETY_YOLO_MODE=truebypasses all checks (user responsibility)Configuration
Examples
Testing
Manual smoke test confirms:
Why Simple?
Previous approach grew to 1000+ lines with Unix duplicate slash normalization, Windows root drive logic, UNC share conversion, path traversal resolution, symlink resolution, etc.
This approach: Just prevent obvious Linux mistakes. Let users handle edge cases via YOLO mode.
🤖 Generated with Claude Code