-
Notifications
You must be signed in to change notification settings - Fork 8
Move git binary before starting tests
#88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ect, ls-tree, write-tree, commit-tree, and clone commands
…SHAs and create additional files for specific test repositories
…ly into cloneRepo for improved clarity
…fy Go toolchain version
…ygit implementation for Git functionality
… including init, cat-file, hash-object, ls-tree, write-tree, commit-tree, and clone
…h for dynamic binary selection
…nary selection in your_program.sh
…ormalization in stages_test.go
…and linux-arm64; streamline your_program.sh to directly execute the built binary
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
git binary before starting tests
|
Triggered a Github Actions job to update fixtures. |
…n a temporary directory for testing
…ensure proper execution without elevated privileges
…ctory for testCloneRepository function
…s to ensure proper execution with elevated privileges
rohitpaulk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes added
… for commit-tree and init commands in temporary directories
…direct output to io.Discard, and improve temporary directory cleanup
…it.defaultBranch to main unconditionally
…m.sh to avoid unnecessary warnings
…rement for the git binary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: `restoreSystemGit` Function Security and Error Handling Flaws
The restoreSystemGit function has multiple issues:
- Shell Injection Vulnerability: The
mvcommand is constructed by directly interpolating unescaped paths, creating a shell injection risk. - Inconsistent Error Handling: The function declares an
errorreturn type but always panics on failure and returnsnilon success, never returning an actualerrorvalue. This panicking behavior, especially when used as a teardown function, can mask the true cause of test failures. - Incorrect Error Message: The panic message for
os.RemoveAllincorrectly formats the directory path instead of the actual error.
internal/utils.go#L40-L55
Lines 40 to 55 in 3fa89a6
| // RestoreSystemGit moves the git binary back to its original location and cleans up | |
| func restoreSystemGit(newPath string, originalPath string) error { | |
| command := fmt.Sprintf("mv %s %s", newPath, originalPath) | |
| moveCmd := exec.Command("sh", "-c", command) | |
| moveCmd.Stdout = io.Discard | |
| moveCmd.Stderr = io.Discard | |
| if err := moveCmd.Run(); err != nil { | |
| panic(fmt.Sprintf("CodeCrafters Internal Error: mv restore for git failed: %v", err)) | |
| } | |
| if err := os.RemoveAll(path.Dir(newPath)); err != nil { | |
| panic(fmt.Sprintf("CodeCrafters Internal Error: delete tmp git directory failed: %s", path.Dir(newPath))) | |
| } | |
| return nil | |
| } |
Bug: Script Overwrites Global Git Settings
The script modifies global Git configuration by setting init.defaultBranch to main. This change persists after test execution and can overwrite a developer's existing global Git settings, affecting their workflow outside the test environment. It should use local configuration instead.
internal/test_helpers/pass_all/your_program.sh#L13-L17
git-tester/internal/test_helpers/pass_all/your_program.sh
Lines 13 to 17 in 3fa89a6
| if [ -x "$tmpdir" ]; then | |
| # If defaultBranch config is not set, we set it to main (doesn't work without global config) | |
| if ! "$tmpdir" config --global --get init.defaultBranch >/dev/null 2>&1; then | |
| "$tmpdir" config --global init.defaultBranch main | |
| fi |
Bug: CI Test Privilege Escalation Issue
The CI workflow's change to sudo make test will cause failures in environments where sudo is unavailable (e.g., some Docker containers). This also introduces unnecessary security risks by elevating test execution privileges.
.github/workflows/test.yml#L21-L22
git-tester/.github/workflows/test.yml
Lines 21 to 22 in 3fa89a6
| - run: sudo make test |
Bug: Shell Injection and Error Handling Issues
Shell injection vulnerability exists in RelocateSystemGit and restoreSystemGit where mv commands are built by directly interpolating unescaped paths into sh -c, enabling command injection. Additionally, the restoreSystemGit function's error return value is ignored by the registered teardown function, which could lead to silent cleanup failures.
internal/utils.go#L26-L37
Lines 26 to 37 in 3fa89a6
| command := fmt.Sprintf("mv %s %s", oldGitPath, tmpGitPath) | |
| moveCmd := exec.Command("sh", "-c", command) | |
| moveCmd.Stdout = io.Discard | |
| moveCmd.Stderr = io.Discard | |
| if err := moveCmd.Run(); err != nil { | |
| os.RemoveAll(tmpGitDir) | |
| panic(fmt.Sprintf("CodeCrafters Internal Error: mv git to tmp directory failed: %v", err)) | |
| } | |
| // Register teardown function to automatically restore git | |
| harness.RegisterTeardownFunc(func() { restoreSystemGit(tmpGitPath, oldGitPath) }) |
Was this report helpful? Give feedback by reacting with 👍 or 👎
rohitpaulk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note added
| func testWriteTree(harness *test_case_harness.TestCaseHarness) error { | ||
| logger := harness.Logger | ||
| executable := harness.Executable | ||
| // This stage Requires the git binary for verifying the git object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems weird behaviour to not do this here too. Can't we move the git binary back for verifying and then place it back again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or can we just use the path of the temporary git to execute that?
Introduce a command-line tool for basic Git operations and streamline repository cloning. Enhance testing capabilities with Docker support and manage Git executables in temporary directories for isolated testing. Update dependencies and improve build scripts for cross-platform compatibility. Remove obsolete configurations and binaries.