fix: Address 11 bugs in security implementation #2
Conversation
High Priority Fixes: - Fixed timezone calculation to properly handle DST (CST/CDT) - Fixed resource leaks in StreamingDownload with context manager - Fixed resource leak in streaming_download with proper cleanup Medium Priority Fixes: - Added reset functions for global state (testing support) - Enhanced input validation for environment variables - Added TLS level validation in PacerConfig - Documented response consumption requirements Low Priority Fixes: - Improved cipher suite (removed DHE, ECDHE only) - Added validation for rate_limit_rpm > 0 - Fixed unused return value in show_peak_hours_warning() Co-authored-by: johnzfitch <10013448+johnzfitch@users.noreply.github.com>
- More specific exception handling in timezone fallback - Catch only specific exceptions in resource cleanup - Add runtime check for response consumption Co-authored-by: johnzfitch <10013448+johnzfitch@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses 11 security bugs ranging from resource leaks to timezone handling issues. The main focus is on fixing DST-related timezone calculations (affecting 8 months of the year), preventing resource leaks through proper context managers, and improving input validation with better error messages.
- Replaces fixed UTC-6 offset with zoneinfo-based timezone handling that properly accounts for CDT/CST transitions
- Adds context manager support to StreamingDownload class and updates streaming_download() function to ensure HTTP responses are properly closed
- Introduces helper functions for resetting global state (rate limiter and audit logger) to improve test isolation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/indepacer/security.py | Core fixes: DST-aware timezone handling, resource leak prevention via context managers, global state cleanup functions, enhanced input validation in SecurityConfig.from_env(), updated cipher suite to remove DHE, response consumption detection, and removal of unused return value |
| src/indepacer/config.py | Added Literal type hint for tls_level and field_validator to prevent division by zero in rate_limit_rpm |
| src/indepacer/cli.py | Changed show_peak_hours_warning() return type from bool to None since return value was never used |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| # Ensure response is closed on error | ||
| response.close() |
There was a problem hiding this comment.
The response.close() call in the except block may execute after the context manager has already closed the response. While the close() method handles this gracefully by catching OSError, this creates a redundant close operation. Consider removing the explicit response.close() call in the except block since the context manager's exit will handle cleanup even when exceptions occur.
| # Check if response was already consumed | ||
| if hasattr(response, '_content') and response._content is not None: | ||
| raise ValueError( | ||
| "Response has already been consumed. " | ||
| "safe_response_content() must be called before accessing response.content or response.text" | ||
| ) | ||
|
|
There was a problem hiding this comment.
This check accesses the private attribute '_content' of the requests.Response object, which is an implementation detail that may change in future versions of the requests library. While this works with current versions, consider using a more stable approach such as checking response.headers.get('content-length') against 0, or documenting the requests version dependency if this private attribute access is intentional.
| # Check if response was already consumed | |
| if hasattr(response, '_content') and response._content is not None: | |
| raise ValueError( | |
| "Response has already been consumed. " | |
| "safe_response_content() must be called before accessing response.content or response.text" | |
| ) | |
| try: | ||
| value = float(value_str) | ||
| if value < min_value: | ||
| raise ValueError( | ||
| f"Invalid value for {env_var}: {value}. " | ||
| f"Must be >= {min_value}" | ||
| ) | ||
| return value | ||
| except ValueError as e: | ||
| if "could not convert" in str(e).lower(): | ||
| raise ValueError( | ||
| f"Invalid float value for {env_var}: '{value_str}'. " | ||
| f"Must be a positive number" | ||
| ) | ||
| raise |
There was a problem hiding this comment.
The error handling logic has a flaw. When a ValueError is raised due to value < min_value (line 844), it will be caught by the except block (line 849). However, the check "could not convert" in str(e).lower() will be False for this case, causing the original ValueError to be re-raised without the helpful formatting. This means errors for values below the minimum won't get the user-friendly error message wrapper. The min_value validation should be moved outside the try-except block, or the except block should only catch ValueError from the float() conversion.
Comprehensive bug fix sweep of the security module identified 11 issues ranging from resource leaks to incorrect timezone handling that affects peak hours detection 8 months of the year.
High Severity
Timezone calculation - Fixed DST handling. Previous implementation used fixed UTC-6 offset; now uses
zoneinfowith fallback:Resource leaks - StreamingDownload and streaming_download() weren't closing responses:
Medium Severity
reset_rate_limiter()andreset_audit_logger()for test isolationSecurityConfig.from_env()now validates types with helpful errors instead of cryptic tracebackstls_levelnow usesLiteral["standard", "strict", "paranoid"]for compile-time validationsafe_response_content()to detect already-consumed responsesLow Severity
"ECDHE+AESGCM:ECDHE+CHACHA20"@field_validatorto prevent division by zeroshow_peak_hours_warning()return type changed frombooltoNone(return value was unused)All changes verified with test suite. CodeQL scan: 0 alerts.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.