Security: Remove shell=True to prevent command injection (Fix #261)#293
Open
GaneshPatil7517 wants to merge 2 commits intoControlCore-Project:devfrom
Open
Security: Remove shell=True to prevent command injection (Fix #261)#293GaneshPatil7517 wants to merge 2 commits intoControlCore-Project:devfrom
GaneshPatil7517 wants to merge 2 commits intoControlCore-Project:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR mitigates a command injection vulnerability in the FRI Flask server by removing shell=True usage and adding input validation for user-supplied values used in subprocess calls for /contribute and /library.
Changes:
- Added a regex-based
validate_input()helper and applied it to request parameters in/contributeand/library. - Switched Windows subprocess execution to
subprocess.run(..., check=True, capture_output=True, text=True)(noshell=True). - Added explicit handling for
ValueErrorandCalledProcessErrorin both endpoints.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add validate_filename() for strict filename validation (no path traversal) - Add validate_text_field() for PR title/body (allow punctuation, check length) - Add get_error_output() to extract error details from CalledProcessError - Use cmd.exe /c to invoke .bat scripts on Windows for shell=False compatibility - Add required parameter to validate_input() to reject None for required fields - Normalize None to empty string for optional fields - Include captured output in error responses for better diagnostics
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.
@pradeeban
Fixes critical command injection vulnerability in FRI server endpoints (/contribute and /library).
Root Cause
User input was executed via subprocess.check_output() with shell=True, allowing attackers to inject arbitrary shell commands via malformed JSON payloads (e.g., {"branch": "main & rm -rf /"}).
Fix
Removed shell=True from subprocess calls on Windows
Switched to subprocess.run() with check=True, capture_output=True, text=True for safe argument handling
Added input validation function (validate_input()) using regex pattern ^[a-zA-Z0-9_-/. ]+$
All user-supplied parameters (study, path, auth, branch, title, desc, filename) are now validated before execution
Added proper error handling for ValueError and CalledProcessError
Changes
/contribute endpoint: Removed shell=True, added input validation for 6 parameters
/library endpoint: Removed shell=True, added input validation for 2 parameters
Impact
Testing
#261