fix: resolve 4 critical security vulnerabilities (P0)#336
Merged
Conversation
- Fix #32: Command injection via in commit_cmd.rs Replace sh -c shell execution with direct Command::new(program).args(args) to prevent shell injection through EDITOR env variable. - Fix #61: Path traversal in JS/TS import resolver Add safe_join() helper that canonicalizes paths and verifies containment within project root. Apply to JS/TS import resolution and all other project_root.join() calls involving user-controlled paths. - Fix #62: Path traversal in symbol resolver Use safe_join() in resolve_symbols, test file resolution, and read_entry_content to prevent reading files outside project root. - Fix #91: API key file written before chmod (TOCTOU race) Create file with 0o600 permissions from the start using OpenOptionsExt on Unix, eliminating the window between write and chmod. Closes #332
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
1 participant
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
Fixes 4 critical security vulnerabilities (P0) identified in the codebase.
Fix #32 — Command injection via `` (commit_cmd.rs)
The commit command was constructing a shell string with and interpolating the environment variable directly, allowing shell injection.
Fix: Parse into program + args using whitespace splitting, then invoke via — no shell involved. Handles editors with flags like or .
Fix #61 — Path traversal in JS/TS import resolver (resolver.rs)
The JS/TS import resolver stripped and prefixes manually then joined with project root, which could be bypassed with path traversal sequences.
Fix: Added helper that canonicalizes paths and verifies containment within the canonicalized project root. Applied to the JS/TS import resolution path.
Fix #62 — Path traversal in symbol resolver (resolver.rs)
accepted and read files via without validation, allowing reads outside the project directory.
Fix: Used in , test file resolution, and to prevent reading files outside the project root. Paths that escape the project directory are logged and skipped.
Fix #91 — API key file written before chmod (TOCTOU race) (loader.rs)
The API key was written to disk with default permissions, then chmod'd to 0o600 — creating a window where the file was readable by other users.
Fix: On Unix, create the file with from the start, eliminating the TOCTOU race. Non-Unix platforms get a best-effort fallback with a comment.
Testing
cargo fmtappliedReferences
Closes #332