Modernize codebase to November 2025 standards #8
Conversation
This commit updates the project to meet modern Python and security best practices as of November 2025. ## Dependency Updates - Update Python requirement from 3.10 to 3.11+ - Update FastAPI from 0.104 to 0.115+ - Update Pydantic from 2.5 to 2.9+ - Update uvicorn from 0.24 to 0.32+ - Update all other dependencies to latest stable versions - Remove unused asyncio-mqtt dependency ## Security Improvements - Add path sanitization to prevent directory traversal attacks - Add validation to ensure paths stay within project root - Fix CORS default from "*" to localhost-only - Add comprehensive logging for security-sensitive operations ## License Fixes - Fix license confusion between README (GPL-3.0) and package metadata - Update pyproject.toml and setup.py to correctly declare GPL-3.0-or-later ## Code Quality - Remove dead mock code from claude_manager.py (25 lines) - Update Black target version from py310 to py311 - Update mypy python_version from 3.10 to 3.11 - Add Python 3.13 classifier ## Testing Improvements - Add test coverage reporting with pytest-cov - Add 'make test' target with HTML coverage reports - Add 'make coverage' to view coverage reports - Add 'make test-no-cov' for quick testing without coverage ## Configuration - Improve CORS configuration with explicit method list - Better environment variable parsing - Add security-focused defaults suitable for firewalled environments All changes maintain backward compatibility while improving security, code quality, and developer experience.
WalkthroughThe PR introduces test coverage reporting in the build system, adds path sanitization and binary validation for enhanced security in process management, restricts CORS defaults to localhost variants, changes the project license to GPL-3.0-or-later, and updates Python minimum version requirement from 3.10 to 3.11 with corresponding dependency version bumps across build and test tools. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ArbenP, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the codebase by upgrading Python and key dependencies, enhancing security measures through path validation and stricter CORS defaults, and improving code quality by removing dead code and updating development tool configurations. It also introduces robust test coverage reporting, ensuring the project adheres to contemporary best practices and provides a more secure and maintainable foundation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant step forward in modernizing the codebase, with valuable dependency updates and security enhancements. The move to stricter CORS policies and the introduction of path sanitization are excellent improvements. My review focuses on further hardening these new security features. I've identified two critical vulnerabilities related to symlink handling where os.path.realpath should be used instead of os.path.abspath. Additionally, I've found a high-severity bug in the new CORS configuration validator and provided suggestions to improve the path sanitization logic and the portability of a new Makefile target. Addressing these points will make the application significantly more secure and robust.
| # Verify the resulting path is still within project_root (defense in depth) | ||
| project_path = os.path.abspath(project_path) | ||
| root_path = os.path.abspath(settings.project_root) |
There was a problem hiding this comment.
There is a potential security vulnerability here. os.path.abspath does not resolve symbolic links, which could allow a specially crafted project_id (containing a symlink) to bypass the directory traversal check. The subsequent os.makedirs could then write files outside the intended project root. You should use os.path.realpath to resolve all symlinks in the path before validation.
| # Verify the resulting path is still within project_root (defense in depth) | |
| project_path = os.path.abspath(project_path) | |
| root_path = os.path.abspath(settings.project_root) | |
| # Verify the resulting path is still within project_root (defense in depth) | |
| project_path = os.path.realpath(project_path) | |
| root_path = os.path.realpath(settings.project_root) |
| # Verify the path is within project_root before deletion | ||
| abs_project_path = os.path.abspath(project_path) | ||
| root_path = os.path.abspath(settings.project_root) |
There was a problem hiding this comment.
Similar to the create_project_directory function, using os.path.abspath here presents a security risk. It does not resolve symlinks, which could lead to a Time-of-check to time-of-use (TOCTOU) vulnerability. An attacker could replace a directory with a symlink after validation but before deletion, causing shutil.rmtree to delete files outside the project root. Please use os.path.realpath to ensure the canonical, symlink-resolved path is checked and used for deletion.
| # Verify the path is within project_root before deletion | |
| abs_project_path = os.path.abspath(project_path) | |
| root_path = os.path.abspath(settings.project_root) | |
| # Verify the path is within project_root before deletion | |
| abs_project_path = os.path.realpath(project_path) | |
| root_path = os.path.realpath(settings.project_root) |
| def sanitize_path_component(component: str) -> str: | ||
| """Sanitize a path component to prevent directory traversal attacks.""" | ||
| import re | ||
| # Remove any path separators and special characters | ||
| sanitized = re.sub(r'[^\w\-.]', '_', component) | ||
| # Remove leading dots to prevent hidden files | ||
| sanitized = sanitized.lstrip('.') | ||
| # Limit length | ||
| sanitized = sanitized[:255] | ||
| if not sanitized: | ||
| raise ValueError("Invalid path component: results in empty string after sanitization") | ||
| return sanitized |
There was a problem hiding this comment.
This sanitization function is a good security measure. However, it could be made more robust and aligned with best practices:
- The
import reshould be at the top of the file (as per PEP 8), along withimport shutilused incleanup_project_directory. - The function doesn't explicitly reject
.or..as path components. While the downstreamcreate_project_directoryfunction'sabspathcheck provides defense-in-depth, a sanitizer's primary role is to reject or clean invalid input. Allowing.or..to pass through is risky.
Here is a suggested improvement that addresses the second point. Please also move the import to the top of the file.
| def sanitize_path_component(component: str) -> str: | |
| """Sanitize a path component to prevent directory traversal attacks.""" | |
| import re | |
| # Remove any path separators and special characters | |
| sanitized = re.sub(r'[^\w\-.]', '_', component) | |
| # Remove leading dots to prevent hidden files | |
| sanitized = sanitized.lstrip('.') | |
| # Limit length | |
| sanitized = sanitized[:255] | |
| if not sanitized: | |
| raise ValueError("Invalid path component: results in empty string after sanitization") | |
| return sanitized | |
| def sanitize_path_component(component: str) -> str: | |
| """Sanitize a path component to prevent directory traversal attacks.""" | |
| import re | |
| # Prohibit special path components that could be used for traversal. | |
| if component in (".", ".."): | |
| raise ValueError(f"Invalid path component: '{component}' is not allowed.") | |
| # Remove any path separators and special characters | |
| sanitized = re.sub(r'[^\w\-.]', '_', component) | |
| # Remove leading dots to prevent hidden files | |
| sanitized = sanitized.lstrip('.') | |
| # Limit length | |
| sanitized = sanitized[:255] | |
| if not sanitized: | |
| raise ValueError("Invalid path component: results in empty string after sanitization") | |
| return sanitized |
| @field_validator('allowed_origins', 'allowed_methods', 'allowed_headers', mode='before') | ||
| def parse_cors_lists(cls, v): | ||
| if isinstance(v, str): | ||
| return [x.strip() for x in v.split(',') if x.strip()] | ||
| return v or ["*"] | ||
| # Support comma-separated values or "*" for all | ||
| parsed = [x.strip() for x in v.split(',') if x.strip()] | ||
| return parsed if parsed else ["http://localhost:*"] | ||
| return v if v else ["http://localhost:*"] |
There was a problem hiding this comment.
This shared validator for allowed_origins, allowed_methods, and allowed_headers has two issues:
- It incorrectly applies the same fallback logic to all three fields. If
ALLOWED_METHODSis set to an empty string, it will be assigned['http://localhost:*'], which is incorrect. - For
allowed_origins, when no value is provided, it falls back to['http://localhost:*'], which is inconsistent with the field's default value that contains three patterns (for localhost, 127.0.0.1, and ::1).
These fields should have separate validators to handle their distinct logic and defaults correctly.
| @field_validator('allowed_origins', 'allowed_methods', 'allowed_headers', mode='before') | |
| def parse_cors_lists(cls, v): | |
| if isinstance(v, str): | |
| return [x.strip() for x in v.split(',') if x.strip()] | |
| return v or ["*"] | |
| # Support comma-separated values or "*" for all | |
| parsed = [x.strip() for x in v.split(',') if x.strip()] | |
| return parsed if parsed else ["http://localhost:*"] | |
| return v if v else ["http://localhost:*"] | |
| @field_validator('allowed_origins', mode='before') | |
| def parse_allowed_origins(cls, v): | |
| default = [ | |
| "http://localhost:*", | |
| "http://127.0.0.1:*", | |
| "http://[::1]:*", | |
| ] | |
| if isinstance(v, str): | |
| parsed = [x.strip() for x in v.split(',') if x.strip()] | |
| return parsed if parsed else default | |
| return v if v else default | |
| @field_validator('allowed_methods', 'allowed_headers', mode='before') | |
| def parse_other_cors(cls, v): | |
| if isinstance(v, str): | |
| parsed = [x.strip() for x in v.split(',') if x.strip()] | |
| # Return None if parsing an empty string, so Pydantic uses the field's default. | |
| return parsed if parsed else None | |
| return v |
| coverage: | ||
| @if [ -f htmlcov/index.html ]; then \ | ||
| echo "Opening coverage report..."; \ | ||
| if command -v xdg-open > /dev/null 2>&1; then \ | ||
| xdg-open htmlcov/index.html; \ | ||
| elif command -v open > /dev/null 2>&1; then \ | ||
| open htmlcov/index.html; \ | ||
| else \ | ||
| echo "Coverage report available at: htmlcov/index.html"; \ | ||
| fi \ | ||
| else \ | ||
| echo "No coverage report found. Run 'make test' first."; \ | ||
| fi |
There was a problem hiding this comment.
The shell logic to find a command to open the browser is clever, but it's not fully portable (e.g., it might not work on all Linux distributions or in environments like WSL without xdg-open). A more portable and simpler approach is to use Python's built-in webbrowser module, which handles the cross-platform differences internally.
coverage:
@if [ -f htmlcov/index.html ]; then \
echo "Opening coverage report in browser..."; \
python -m webbrowser -t "htmlcov/index.html"; \
else \
echo "No coverage report found. Run 'make test' first."; \
fi
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
claude_code_api/core/claude_manager.py (2)
313-324: Good path sanitization, but move import to module level.The sanitization logic effectively prevents directory traversal attacks by removing path separators and limiting to safe characters. The length limit and empty string validation are appropriate.
Consider moving the
import reto the top of the file with other imports:import asyncio import json import os +import re import subprocessThen remove line 315:
def sanitize_path_component(component: str) -> str: """Sanitize a path component to prevent directory traversal attacks.""" - import re # Remove any path separators and special characters
347-368: Strong security validation before deletion, but move import.The path validation before deletion is critical and well-implemented, preventing deletion of files outside the project root. The logging provides good security audit trails.
Move the
import shutilto the module level with other imports:import asyncio import json import os import re +import shutil import subprocessThen remove line 350:
def cleanup_project_directory(project_path: str): """Clean up project directory with path validation.""" try: - import shutil - # Verify the path is within project_root before deletion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(2 hunks)claude_code_api/core/claude_manager.py(2 hunks)claude_code_api/core/config.py(1 hunks)pyproject.toml(3 hunks)setup.py(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 17-17: Target body for "coverage" exceeds allowed length of 5 (12).
(maxbodylength)
🪛 Ruff (0.14.3)
claude_code_api/core/claude_manager.py
323-323: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
362-362: Abstract raise to an inner function
(TRY301)
362-362: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
pyproject.toml (4)
10-10: License change properly reflected in both fields.The change from MIT to GPL-3.0-or-later is consistently applied in both the license text field and the classifier. This is a significant licensing change that affects how the code can be used and distributed.
Also applies to: 18-18
107-107: LGTM! Tooling configuration properly updated.Black and mypy configurations have been correctly updated to target Python 3.11, aligning with the new minimum Python version requirement.
Also applies to: 130-130
28-43: I'll now verify the changelogs for potential breaking changes in the key dependency updates.No breaking changes detected in the specified dependency versions.
All 13 dependency versions have been verified on PyPI. FastAPI 0.115.0 contains no major nor breaking changes. The other dependencies (uvicorn 0.32.0, Pydantic 2.9.0, etc.) are incremental updates with no documented critical breaking changes. The version bumps are safe to deploy.
22-22: Dependency compatibility verified for Python 3.11-3.13.All dependencies specified in pyproject.toml support the updated Python version range:
- Pydantic 2.9.0 meets the 2.8+ requirement for Python 3.13 support
- Uvicorn 0.32.0 includes official Python 3.13 support
- FastAPI, uvicorn, and pytest versions are compatible with 3.11-3.13
No compatibility issues detected.
Makefile (3)
8-10: LGTM! Good coverage reporting setup.The test target now includes comprehensive coverage reporting with both HTML and terminal output, which aligns well with the addition of pytest-cov in the dependencies.
11-12: LGTM! Well-implemented coverage workflow.The new test-no-cov and coverage targets provide good developer ergonomics:
- test-no-cov allows faster test iterations without coverage overhead
- coverage target handles cross-platform HTML report opening with appropriate fallbacks
The shell script complexity is justified for proper cross-platform support.
Also applies to: 17-29
64-67: LGTM! Clear documentation of new test targets.The help text accurately describes the new test and coverage targets, making it easy for developers to understand when to use each option.
claude_code_api/core/claude_manager.py (2)
327-344: Excellent defense-in-depth path validation.The function properly implements multiple layers of security:
- Input sanitization via
sanitize_path_component- Path resolution with
os.path.abspathto handle symbolic links and relative paths- Explicit validation that the result stays within
project_rootusing theos.sepsuffix to prevent prefix matching vulnerabilities- Security-focused logging
371-382: LGTM! Simple and robust binary validation.The validation function properly uses subprocess with a timeout and returns a clear boolean result. The broad exception handling is appropriate since this is a validation check that should gracefully return False on any failure.
setup.py (1)
16-16: LGTM! Consistent with pyproject.toml updates.All changes in setup.py properly mirror the updates made in pyproject.toml:
- Python version requirement: 3.11+
- License: GPL-3.0-or-later
- Python 3.13 classifier added
- Dependency versions match exactly
This consistency between both packaging files is important for proper package installation.
Also applies to: 18-32, 36-47, 58-58, 62-62
claude_code_api/core/config.py (2)
97-104: Excellent security improvement to CORS defaults.Restricting CORS origins from "*" to localhost-only variants significantly improves the default security posture. The configuration:
- Covers all localhost representations (localhost, 127.0.0.1, [::1])
- Supports any port with the
:*wildcard for development flexibility- Provides explicit HTTP methods instead of allowing all
- Can be overridden via environment variables for production needs
This is a breaking change for deployments relying on the "*" default, but the security benefit justifies it.
106-112: Good parsing logic with secure defaults.The validator properly handles comma-separated environment variables and defaults to a secure localhost-only configuration when empty values are provided. This applies to all CORS configuration fields (origins, methods, headers), ensuring consistent behavior.
2 participants
User description
This commit updates the project to meet modern Python and security best practices as of November 2025.
Dependency Updates
Security Improvements
License Fixes
Code Quality
Testing Improvements
Configuration
All changes maintain backward compatibility while improving security, code quality, and developer experience.
PR Type
Enhancement, Bug fix
Description
Update Python requirement from 3.10 to 3.11+
Add path sanitization to prevent directory traversal attacks
Fix CORS default from "*" to localhost-only
Update all dependencies to November 2025 stable versions
Remove unused asyncio-mqtt dependency and dead mock code
Fix license declaration from MIT to GPL-3.0-or-later
Add test coverage reporting with pytest-cov integration
Update Black and mypy target versions to py311
Diagram Walkthrough
File Walkthrough
claude_manager.py
Add path sanitization and remove dead mock codeclaude_code_api/core/claude_manager.py
_start_mock_processmethod)sanitize_path_component()function to prevent directory traversalattacks
create_project_directory()with path sanitization andvalidation
cleanup_project_directory()with path validation and securitychecks
config.py
Restrict CORS to localhost for securityclaude_code_api/core/config.py
allowed_originsdefault from["*"]to localhost-onlypatterns
allowed_methodsfrom["*"]to explicit list: GET, POST,PUT, DELETE, OPTIONS, PATCH
values
setup.py
Update dependencies and Python version requirementssetup.py
>=3.10to>=3.11>=0.104.0to>=0.115.0>=2.5.0to>=2.9.0>=0.24.0to>=0.32.0asyncio-mqtt>=0.16.1dependencyopenai>=1.54.0to dependenciesMakefile
Add test coverage reporting targetsMakefile
testtarget to include coverage reporting with--covflagstest-no-covtarget for quick testing without coveragecoveragetarget to open HTML coverage reportspyproject.toml
Update dependencies and tool configurationspyproject.toml
>=3.10to>=3.11>=0.104.0to>=0.115.0>=2.5.0to>=2.9.0>=0.24.0to>=0.32.0asyncio-mqtt>=0.16.1dependency>=1.0.0to>=1.54.0py310topy3113.10to3.11Summary by CodeRabbit
Release Notes
Bug Fixes
Chores