fix: harden devcontainer policy validation and add test coverage#7
Merged
Conversation
Extract remote_user_home into a shared function. Add tests for resolve_workspace_config_path, sanitize_volume_name, and is_docker_socket_path.
Reserve DISABLE_AUTOUPDATER as a protected container env key. Replace --cap-add blocklist with an explicit allowlist (NET_ADMIN, NET_RAW). Use shared remote_user_home from config.rs. Add tests for remote_user validation, container path normalization, and run_args edge cases.
3f7575c to
9cd720c
Compare
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
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
DISABLE_AUTOUPDATERas a protected container env key. Previously a user-provided[container_env] DISABLE_AUTOUPDATER = "0"silently overrode the Bulkhead-set value. Now rejected at validation time, matching the pattern forCLAUDE_CONFIG_DIRandBULKHEAD_SELECTED_AGENTS.--cap-addblocklist with an explicit capability allowlist. The old check blockedSYS_ADMIN(substring match) andALL(exact match) but allowed other dangerous capabilities likeSYS_PTRACE,SYS_MODULE, andSYS_RAWIO. Now onlyNET_ADMINandNET_RAWare permitted; all other capabilities are rejected. Both--cap-add=VALUEand--cap-add VALUE(split) forms are validated.remote_user_homeintoconfig.rs. The "if root then /root else /home/{user}" pattern was duplicated indevcontainer.rs,config.rs::gitconfig_target, andconfig.rs::PreinstalledAgent::config_target. Consolidated into a singlepub(crate) fn remote_user_home.render_commandby reusingsystem::render_command.commands/clone.rshad its own 5-line copy. Made the generic version insystem.rspub(crate)and deleted the duplicate.bash -nsyntax check forbulkhead-post-create.shtoscripts/verify.sh. Shell syntax errors in the embedded template were previously silent until container startup.resolve_workspace_config_path: empty input, absolute paths, home expansion, variable expansion, directory traversal, valid paths (8 tests)sanitize_volume_name: valid names, special char replacement, dash trimming, all-special-char inputs (4 tests)is_docker_socket_path: standard locations, non-socket path rejection (2 tests)remote_uservalidation: empty, whitespace-only, special characters, valid usernames (4 tests)normalize_container_path: relative rejection, root escape, dot/slash normalization, root handling, trailing slash (5 tests)validate_run_argsedge cases:--privileged=true, split-form namespace flags,--volumes-from, case-insensitiveALL, safe arg acceptance (5 tests)--cap-addallowlist: dangerous capabilities rejected, allowed capabilities accepted, split form rejection (3 tests)DISABLE_AUTOUPDATERreservation (1 test)Changes by commit
e77ea7dci: add bash syntax check for post-create scriptscripts/verify.sh19ac283refactor: deduplicate render_commandsrc/system.rs,src/commands/clone.rs8562b81refactor: deduplicate remote_user_home and add config.rs test coveragesrc/config.rs28ca576fix: harden devcontainer security policy and add test coveragesrc/devcontainer.rsTest plan
cargo testpasses (34 -> 66 tests, all green)cargo clippy --all-targets -- -D warningscleancargo fmt --all -- --checkcleanbash -n templates/bulkhead-post-create.shpassescargo deny checkpasses./scripts/verify.shpasses at every commit--cap-add=NET_ADMINand--cap-add=NET_RAWacceptedagents = ["claude"]with noDISABLE_AUTOUPDATERoverride works as before--cap-add=SYS_PTRACEis now rejected (was previously accepted)