-
Notifications
You must be signed in to change notification settings - Fork 1
Increase overall security #51
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
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.
Pull request overview
This PR implements comprehensive input validation and sanitization to enhance overall security across the application. The changes introduce a new sanitization helper module, add validation to all major routes (backtest, loanertech, and LAF), and include test coverage for the new functionality.
Key Changes:
- Added
bleachlibrary for HTML sanitization andpytestfor testing infrastructure - Implemented sanitization helpers with validation for MongoDB injection, ObjectIds, and text input
- Applied validation to route parameters and model fields across backtest, loanertech, and LAF endpoints
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Added bleach and pytest dependencies, updated certifi version |
pyproject.toml |
Added bleach and pytest to dependencies, configured pytest options |
tests/test_sanitize.py |
New test file for sanitization functions with basic coverage |
server/helpers/sanitize.py |
New helper module implementing text sanitization, MongoDB operator validation, and ObjectId validation |
server/models/common.py |
New file consolidating common response models and validation functions |
server/models/__init__.py |
Removed (content moved to common.py) |
server/models/backtest.py |
Added ObjectId and CourseCode validation with custom validators |
server/models/auth.py |
Added UUID v4 validation for authentication codes |
server/models/laf.py |
Added comprehensive validation for descriptions, types, locations, and names |
server/models/loanertech.py |
Added phone number validation and description sanitization |
server/routes/backtest.py |
Applied CourseCode and ObjectId validation to route parameters |
server/routes/laf.py |
Added validation to query parameters and sanitized location inputs |
server/routes/loanertech.py |
Added integer validation with minimum value constraints |
server/database/laf.py |
Added MongoDB operator rejection to prevent injection attacks |
server/database/loanertech.py |
Added MongoDB operator rejection to prevent injection attacks |
.github/workflows/pytest.yml |
New workflow for running pytest on Python file changes |
.github/workflows/ruff.yml |
Updated action references with commit hashes |
.github/workflows/docker-test.yml |
Updated action references with commit hashes |
.github/workflows/docker-deploy.yml |
Updated action references with commit hashes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v = v.strip() if isinstance(v, str) else str(v) | ||
| try: | ||
| # Validate it's a valid UUID v4 | ||
| uuid_obj = UUID(v, version=4) |
Copilot
AI
Jan 5, 2026
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.
The UUID validation logic has a flaw: UUID(v, version=4) doesn't actually validate that the UUID is version 4, it only attempts to parse v as a UUID and raises an error if the version parameter doesn't match. A non-v4 UUID string will still parse successfully. Use uuid_obj.version to check if it equals 4 after parsing, or use a regex pattern to validate the v4 format specifically.
| uuid_obj = UUID(v, version=4) | |
| uuid_obj = UUID(v) | |
| if uuid_obj.version != 4: | |
| raise ValueError("code must be a valid UUID v4") |
| auth: bool = Depends(required_auth), | ||
| ) -> LostReportItemsResponse: | ||
| sanitized_locations = ( | ||
| [sanitize_text(x, max_len=60) for x in location] |
Copilot
AI
Jan 5, 2026
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.
Inconsistent max_len values: location list items are sanitized with max_len=60 here, but the Location type in models/laf.py uses max_len=340. This inconsistency could lead to unexpected behavior where individual location items in a list are truncated differently than a single location string. Ensure these limits are consistent or document why they differ.
Add validation and sanitization of input data