fix: security-validated inputs and expanded unit-test coverage - #356
Open
awdemos wants to merge 8 commits into
Open
fix: security-validated inputs and expanded unit-test coverage#356awdemos wants to merge 8 commits into
awdemos wants to merge 8 commits into
Conversation
Explicitly close and check the temporary state file after writing but before passing its path to git notes. The previous deferred close happened after the git command ran, which can fail on Windows (open handle prevents git from reading the file) and masks write/close errors. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Check errors from scanner.Err() and os.Stdout.Write(), and close pipe ends cleanly after reading. Previously these errors were silently discarded, which could truncate output or leave pipe handles open on Windows without reporting failure to the caller. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add validateGitRefComponent to reject ref/branch names that start with '-' or contain unsafe characters before passing them to git commands. Apply validation in Checkout, Merge, Apply, and initializeWorktree to prevent argument injection. Also reject absolute paths and '..' traversal in exportEnvironmentFile so that agent-provided target_file cannot escape the environment worktree on the host filesystem. Regression tests included for ref validation and path traversal classification.
Reject unsupported shell strings before building container exec args. This prevents a caller from passing an arbitrary executable path as the shell interpreter (e.g. '/bin/echo') in environment_run_cmd. Regression test included.
Change the default debug stderr log file mode from 0644 to 0600 so that other users on the same host cannot read potentially sensitive command output or environment metadata.
Cover KVList CRUD, ServiceConfigs lookup, EnvironmentConfig copy/save, State marshal/unmarshal/legacy migration, and Notes formatting. Raises environment short-test coverage from 4.7% to 22.1%.
…se marshaling Cover newRepositoryTool/newEnvironmentTool schema shapes, createTools, wrapTool/wrapToolWithClient behavior, and Environment response marshaling. Raises mcpserver short-test coverage from 5.7% to 18.9%.
Cover version extraction, runtime info string, build info, title truncation, completion helpers, git URL normalization, safe path creation, binary file detection, and repository path helpers. Raises cmd/container-use coverage from 20.1% to 22.7% and repository from 25.4% to 27.9%.
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.
What this PR does
A combined bug-fix sweep and focused unit-test coverage improvement, since the tests validate the security-related fixes.
Bug fixes
git notesreads it (repository/git.go)watchcommand (cmd/container-use/watch_windows.go)repository/git.go,repository/repository.go)Environment.Run(environment/environment.go)cmd/container-use/logger.go)Test coverage improvements
Added pure-unit tests that do not require a Dagger engine or live Git repo:
environment/config_extra_test.go,environment/state_test.go,environment/note_test.gomcpserver/args_test.go,mcpserver/tools_test.gocmd/container-use/main_test.go, updates tocmd/container-use/version_test.gorepository/git_test.go,repository/repository_test.goCoverage delta
environmentmcpservercmd/container-userepositoryTests
go test -short -count=1 ./...passes for all packages.go vet ./...is clean.go fmt ./...is clean.Note:
dagger call lintand fullgo test ./...could not be run locally because the Dagger/Podman engine on this host is in a bad rootless user-namespace state (cannot re-exec process to join the existing user namespace), which appears to be a Toolbx/host-level issue unrelated to these code changes.Checklist
go test -short ./...passesgo vet ./...passesgo fmt ./...passes