Feature/phase2 fastapi core#63
Feature/phase2 fastapi core#63Nkwenti-Severian-Ndongtsop merged 45 commits intoCameroon-Developer-Network:mainfrom
Conversation
…settings for Python, shell, test coverage, and Docker. Exclude specific paths from analysis and enable formatting tools like black and isort.
…e. Introduced LLMConfig and BackendConfig classes for managing LLM generation parameters and backend-specific settings. Updated Settings class to include active backend selection and configurations for HuggingFace, OpenAI, Anthropic, and Ollama.
…ver settings, added Redis and LLM configurations, and introduced rate limiting parameters. Enhanced security settings and improved overall structure for clarity.
…settings, LLM configuration validation, backend configuration, environment variable loading, and YAML configuration support. Improved test structure and coverage for better reliability.
…ced configuration management.
…l method for loading settings from a YAML file, updated env_prefix and added env_nested_delimiter for improved configuration management.
… model name in environment variables, improved YAML loading tests, and refined environment variable override tests for better clarity and reliability.
|
Warning Rate limit exceeded@chojuninengu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a comprehensive overhaul of configuration management, including new Pydantic models and YAML loading for LLM backends, expanded and modular config examples, and a richer test suite for configuration logic. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI_App
participant SecurityMiddleware
participant RBACMiddleware
participant RequestValidationMiddleware
participant Auth
participant Endpoint
Client->>FastAPI_App: HTTP Request
FastAPI_App->>SecurityMiddleware: Add security headers
SecurityMiddleware-->>FastAPI_App: Response with headers
FastAPI_App->>RBACMiddleware: Check user role & permissions
RBACMiddleware->>Auth: Validate token and user role
RBACMiddleware-->>FastAPI_App: Allow or deny request
FastAPI_App->>RequestValidationMiddleware: Validate content length/type
RequestValidationMiddleware-->>FastAPI_App: Allow or reject request
FastAPI_App->>Endpoint: Process request if authorized
Endpoint-->>Client: Response
sequenceDiagram
participant User
participant Settings
participant YAML_File
participant EnvVars
User->>Settings: from_yaml(yaml_file)
Settings->>YAML_File: Load and parse YAML
Settings->>Settings: Map YAML keys to Pydantic models
Settings->>EnvVars: (optional) Override with environment variables
Settings-->>User: Return populated Settings instance
User->>Settings: get_active_backend_config()
Settings-->>User: Return BackendConfig for selected backend
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
|
Here's the code health analysis summary for commits Analysis Summary
|
…efault_factory for BackendConfig and LLMConfig, added env_file_encoding and validate_default parameters for improved configuration handling.
…del_validator for OpenAI backend to transform environment variable keys for improved compatibility and clarity.
…validation method for clarity, added LLM configuration handling, and simplified backend config assignments for improved readability and maintainability.
… RBACMiddleware, and RequestValidationMiddleware to enforce security headers, role-based access control, and request validation. Introduced convenience dependencies for role checks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
.deepsource.toml (2)
8-10: Specify an exact minor version forruntime_version
"3.x.x"is not accepted. Use"3.11"or"3.10"to match your CI image.
35-43: Excludingtests/**conflicts with the test-coverage analyzerThe Python analyzer ignores tests, yet the coverage analyzer needs them.
Consider removing"tests/**"fromexclude_patternsso linting and security
rules cover test code as well.backend/config.example.yml (2)
29-61: Remove trailing whitespace to satisfy YAML linters
yamllintflags trailing spaces on several lines (29, 36, 42, 47, 52).- active_backend: huggingface␠ + active_backend: huggingface ... - timeout: 30␠ + timeout: 30Cleaning these will keep CI green when linting is enforced.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
19-20: Avoid shipping placeholder credentials in examplesStatic analysis flags the hard-coded
postgresql://user:password@...DSN
(CKV_SECRET_4). Even in examples, consider using<username>/<password>
or environment-variable interpolation to discourage accidental commits of
real secrets.🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 19-20: Basic Auth Credentials
(CKV_SECRET_4)
tests/unit/test_config.py (2)
4-6: Remove unused imports to clean up the code.The static analysis correctly identifies unused imports that should be removed for cleaner code.
Apply this diff to remove the unused imports:
-from pathlib import Path -import yaml from pydantic import PostgresDsn -from backend.config import Settings, LLMConfig, BackendConfig +from backend.config import Settings, LLMConfig, BackendConfigWait, let me check if
PostgresDsnis actually used... Looking at the test code, it's imported but not used in any of the test functions shown.-from pathlib import Path -import yaml -from pydantic import PostgresDsn from backend.config import Settings, LLMConfig, BackendConfig🧰 Tools
🪛 Ruff (0.11.9)
4-4:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
5-5:
yamlimported but unusedRemove unused import:
yaml(F401)
6-6:
pydantic.PostgresDsnimported but unusedRemove unused import:
pydantic.PostgresDsn(F401)
149-151: Fix the assertion order for better readability.The static analysis correctly identifies a Yoda condition that should be rewritten for better readability.
Apply this diff to fix the assertion order:
- assert settings.ALLOWED_ORIGINS == custom_origins + assert custom_origins == settings.ALLOWED_ORIGINSWait, that's not right. The current code has
settings.ALLOWED_ORIGINS == custom_originswhich is the normal order. The static analysis suggestion is to change it tocustom_origins == settings.ALLOWED_ORIGINS, but the current order is actually more readable. The static analysis tool is suggesting the Yoda condition fix incorrectly.Actually, let me re-read this. The current code is:
assert settings.ALLOWED_ORIGINS == custom_originsThe static analysis says "Yoda condition detected" and suggests "Rewrite as
custom_origins == settings.ALLOWED_ORIGINS". But the current code is NOT a Yoda condition. A Yoda condition would becustom_origins == settings.ALLOWED_ORIGINS. The current codesettings.ALLOWED_ORIGINS == custom_originsis the normal, readable order.This appears to be a false positive from the static analysis tool. The current assertion order is correct and readable.
🧰 Tools
🪛 Ruff (0.11.9)
151-151: Yoda condition detected
Rewrite as
custom_origins == settings.ALLOWED_ORIGINS(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.deepsource.toml(1 hunks)README.md(1 hunks)backend/config.example.yml(1 hunks)backend/config.py(3 hunks)requirements.txt(1 hunks)src/amega_ai/__init__.py(1 hunks)tests/unit/test_config.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/test_config.py (1)
backend/config.py (5)
Settings(78-234)LLMConfig(13-53)BackendConfig(55-76)get_active_backend_config(182-190)from_yaml(193-234)
🪛 Checkov (3.2.334)
backend/config.example.yml
[MEDIUM] 19-20: Basic Auth Credentials
(CKV_SECRET_4)
🪛 YAMLlint (1.37.1)
backend/config.example.yml
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
🪛 Ruff (0.11.9)
backend/config.py
7-7: typing.Dict imported but unused
Remove unused import
(F401)
7-7: typing.Any imported but unused
Remove unused import
(F401)
tests/unit/test_config.py
4-4: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
5-5: yaml imported but unused
Remove unused import: yaml
(F401)
6-6: pydantic.PostgresDsn imported but unused
Remove unused import: pydantic.PostgresDsn
(F401)
151-151: Yoda condition detected
Rewrite as custom_origins == settings.ALLOWED_ORIGINS
(SIM300)
🔇 Additional comments (7)
README.md (1)
4-7: No functional impact – consider squashing trivial whitespace commitsFour blank lines were added but they do not change rendering. You can keep them, but if the PR is rebased anyway, squashing such formatting-only commits keeps history cleaner.
requirements.txt (1)
5-7: Pinpydantic-settingsto the same minor series aspydantic
pydantic>=2.5.2pairs best withpydantic-settings>=2.1,<2.2(current latest is2.2.1). Leaving a loose upper-bound may cause a future 3.x release to break settings parsing.-pydantic-settings>=2.0.0 +pydantic-settings>=2.1,<3.0Please confirm compatibility with the version you actually use in CI.
src/amega_ai/__init__.py (1)
1-12: LGTM! Clean package initialization.The package initialization follows Python packaging conventions well with a clear docstring, appropriate metadata variables, and semantic versioning. The structure is clean and ready for future expansion.
tests/unit/test_config.py (4)
23-47: Excellent validation test coverage.The LLM configuration validation tests properly cover both valid and invalid parameter ranges, ensuring the Pydantic field constraints are working correctly. The test cases cover edge cases like values above/below the allowed ranges.
75-98: Good environment variable testing with proper cleanup.The test properly demonstrates environment variable loading with the nested delimiter syntax (
__) and includes proper cleanup to avoid affecting other tests. This is a well-structured test.
99-128: Comprehensive YAML configuration test.The test effectively validates the YAML loading functionality using a temporary file. The YAML structure matches the expected configuration format and covers multiple configuration sections.
1-151: Comprehensive test suite with excellent coverage.The test suite provides thorough coverage of the configuration system including:
- Default settings validation
- LLM and backend configuration validation
- Environment variable overrides with proper cleanup
- YAML configuration loading
- Active backend selection
The tests are well-structured and follow good testing practices with proper setup and teardown.
🧰 Tools
🪛 Ruff (0.11.9)
4-4:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
5-5:
yamlimported but unusedRemove unused import:
yaml(F401)
6-6:
pydantic.PostgresDsnimported but unusedRemove unused import:
pydantic.PostgresDsn(F401)
151-151: Yoda condition detected
Rewrite as
custom_origins == settings.ALLOWED_ORIGINS(SIM300)
| [analyzers.meta] | ||
| runtime_version = "3.x.x" | ||
| max_line_length = 88 # Matches black formatter | ||
| type_checker = "mypy" | ||
|
|
||
| # Enable all available rules for comprehensive analysis | ||
| enable = ["all"] | ||
|
|
||
| # Dependencies that should be analyzed | ||
| dependencies_modules = ["requirements.txt", "requirements-dev.txt"] | ||
|
|
There was a problem hiding this comment.
dependencies_modules appears to be a typo – use dependency_files
DeepSource expects the key dependency_files (plural) for Python analyzers.
- dependencies_modules = ["requirements.txt", "requirements-dev.txt"]
+ dependency_files = ["requirements.txt", "requirements-dev.txt"]A wrong key silently disables dependency scanning.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [analyzers.meta] | |
| runtime_version = "3.x.x" | |
| max_line_length = 88 # Matches black formatter | |
| type_checker = "mypy" | |
| # Enable all available rules for comprehensive analysis | |
| enable = ["all"] | |
| # Dependencies that should be analyzed | |
| dependencies_modules = ["requirements.txt", "requirements-dev.txt"] | |
| [analyzers.meta] | |
| runtime_version = "3.x.x" | |
| max_line_length = 88 # Matches black formatter | |
| type_checker = "mypy" | |
| # Enable all available rules for comprehensive analysis | |
| enable = ["all"] | |
| # Dependencies that should be analyzed | |
| - dependencies_modules = ["requirements.txt", "requirements-dev.txt"] | |
| + dependency_files = ["requirements.txt", "requirements-dev.txt"] |
🤖 Prompt for AI Agents
In the .deepsource.toml file between lines 7 and 17, the key
`dependencies_modules` is incorrect and should be replaced with
`dependency_files` to enable dependency scanning for Python analyzers. Update
the key name to `dependency_files` and keep the list of files unchanged to
ensure DeepSource correctly analyzes the specified dependency files.
| import yaml | ||
| from pydantic_settings import BaseSettings, SettingsConfigDict |
There was a problem hiding this comment.
yaml dependency missing from requirements.txt
config.py imports yaml (PyYAML) yet the project does not list it.
Add a pinned version, e.g.
+PyYAML>=6.0to requirements.txt to avoid runtime ModuleNotFoundError.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import yaml | |
| from pydantic_settings import BaseSettings, SettingsConfigDict | |
| # requirements.txt | |
| +PyYAML>=6.0 |
🤖 Prompt for AI Agents
In backend/config.py around lines 9 to 10, the yaml module is imported but the
PyYAML package is not listed in requirements.txt. Add PyYAML with a pinned
version to requirements.txt to ensure the dependency is installed and prevent
runtime ModuleNotFoundError.
| from typing import List, Optional, Literal, Dict, Any | ||
| from pathlib import Path | ||
| import yaml | ||
| from pydantic_settings import BaseSettings, SettingsConfigDict |
There was a problem hiding this comment.
Remove unused imports
Dict and Any are imported but never used, triggering Ruff F401.
-from typing import List, Optional, Literal, Dict, Any
+from typing import List, Optional, Literal📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import List, Optional, Literal, Dict, Any | |
| from pathlib import Path | |
| import yaml | |
| from pydantic_settings import BaseSettings, SettingsConfigDict | |
| from typing import List, Optional, Literal | |
| from pathlib import Path | |
| import yaml | |
| from pydantic_settings import BaseSettings, SettingsConfigDict |
🧰 Tools
🪛 Ruff (0.11.9)
7-7: typing.Dict imported but unused
Remove unused import
(F401)
7-7: typing.Any imported but unused
Remove unused import
(F401)
🤖 Prompt for AI Agents
In backend/config.py around lines 7 to 10, the imports Dict and Any from typing
are unused and cause a linting error. Remove Dict and Any from the import
statement to clean up the code and resolve the Ruff F401 warning.
| def from_yaml(cls, yaml_file: str | Path) -> "Settings": | ||
| """Load settings from a YAML file.""" | ||
| yaml_file = Path(yaml_file) | ||
| if not yaml_file.exists(): | ||
| raise FileNotFoundError(f"YAML configuration file not found: {yaml_file}") | ||
|
|
||
| with yaml_file.open("r") as f: | ||
| yaml_data = yaml.safe_load(f) | ||
|
|
||
| # Transform YAML structure to match our settings format | ||
| config_data = {} | ||
|
|
||
| # Handle app settings | ||
| if "app" in yaml_data: | ||
| config_data.update({ | ||
| "APP_NAME": yaml_data["app"].get("name", cls.model_fields["APP_NAME"].default), | ||
| "APP_VERSION": yaml_data["app"].get("version", cls.model_fields["APP_VERSION"].default), | ||
| "DEBUG": yaml_data["app"].get("debug", cls.model_fields["DEBUG"].default) | ||
| }) | ||
|
|
||
| # Handle LLM settings | ||
| if "llm" in yaml_data: | ||
| llm_data = yaml_data["llm"] | ||
| if "active_backend" in llm_data: | ||
| config_data["ACTIVE_LLM_BACKEND"] = llm_data["active_backend"] | ||
|
|
||
| if "backends" in llm_data: | ||
| backends = llm_data["backends"] | ||
| if "huggingface" in backends: | ||
| config_data["HUGGINGFACE_CONFIG"] = BackendConfig(**backends["huggingface"]) | ||
| if "openai" in backends: | ||
| config_data["OPENAI_CONFIG"] = BackendConfig(**backends["openai"]) | ||
| if "anthropic" in backends: | ||
| config_data["ANTHROPIC_CONFIG"] = BackendConfig(**backends["anthropic"]) | ||
| if "ollama" in backends: | ||
| config_data["OLLAMA_CONFIG"] = BackendConfig(**backends["ollama"]) | ||
|
|
||
| if "generation" in llm_data: | ||
| config_data["LLM_CONFIG"] = LLMConfig(**llm_data["generation"]) | ||
|
|
||
| # Create settings instance with YAML data | ||
| return cls(**config_data) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
from_yaml() only maps a subset of settings
The loader ignores server, database, redis, rate_limit, and several
other sections present in config.example.yml, so values in the YAML won’t
override defaults.
Consider iterating generically instead of hard-coding each section, or at
least mapping the remaining keys:
@@
- # Handle server settings
- if "server" in yaml_data:
- config_data.update({
- "HOST": yaml_data["server"].get("host", cls.model_fields["HOST"].default),
- "PORT": yaml_data["server"].get("port", cls.model_fields["PORT"].default),
- "ALLOWED_ORIGINS": yaml_data["server"].get("allowed_origins", cls.model_fields["ALLOWED_ORIGINS"].default),
- })
+ # Handle server settings
+ if "server" in yaml_data:
+ server = yaml_data["server"]
+ config_data |= {
+ "HOST": server.get("host", cls.model_fields["HOST"].default),
+ "PORT": server.get("port", cls.model_fields["PORT"].default),
+ "ALLOWED_ORIGINS": server.get("allowed_origins", cls.model_fields["ALLOWED_ORIGINS"].default),
+ }Repeat for database, redis, rate_limit, etc., or build a recursive
flattener using the env_nested_delimiter convention.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/config.py from lines 193 to 234, the from_yaml method only maps a
limited set of configuration sections like app and llm, ignoring others such as
server, database, redis, and rate_limit, which prevents YAML values from
overriding defaults for those sections. To fix this, extend the method to also
map these missing sections by either explicitly adding their keys to config_data
with appropriate transformations or implement a generic recursive function that
flattens nested YAML keys using the env_nested_delimiter convention, ensuring
all relevant configuration sections are loaded and override defaults as
expected.
…ecurity middleware for role-based access control, added user listing endpoint for admins, and refined chat endpoint to improve response generation. Updated health check endpoint for better clarity.
…ld, introduced user role management with Literal type for roles, and updated the authenticate_user function to return a User type instead of UserInDB.
…ented tests for security headers, role-based access control for user, moderator, and admin roles, and request validation middleware. Enhanced test coverage for public and protected endpoints.
…ware: defined public paths for easier management, enhanced token validation, and refined error handling for unauthorized access. Updated request validation middleware to skip validation for GET requests.
…s with a utility function to create test JWT tokens for user, moderator, and admin roles. Updated test cases to use generated tokens for authorization in role-based access control tests, enhancing clarity and maintainability.
…onfiguration: updated SECRET_KEY, ALGORITHM, and ACCESS_TOKEN_EXPIRE_MINUTES to pull from settings. Enhanced user role management by adding an optional role field in TokenData and improved simulated user database structure.
… checks: replaced permission-based access control with role-based access management, updated endpoint permissions to map directly to user roles, and improved error handling for unauthorized access.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/config.py (2)
7-11: Address the duplicate issues flagged in previous reviews.The previous review comments are still valid - there are unused imports and a missing dependency that need to be resolved.
218-259: 💡 Verification agent🧩 Analysis chain
YAML loading method addresses previous feedback but needs completion.
The
from_yamlmethod partially addresses the previous review comment about incomplete mapping, but still only handlesappandllmsections while ignoringserver,database,redis, andrate_limitsections.Complete the YAML mapping to handle all configuration sections:
# Handle app settings if "app" in yaml_data: config_data.update({ "APP_NAME": yaml_data["app"].get("name", cls.model_fields["APP_NAME"].default), "APP_VERSION": yaml_data["app"].get("version", cls.model_fields["APP_VERSION"].default), "DEBUG": yaml_data["app"].get("debug", cls.model_fields["DEBUG"].default) }) +# Handle server settings +if "server" in yaml_data: + server = yaml_data["server"] + config_data.update({ + "HOST": server.get("host", cls.model_fields["HOST"].default), + "PORT": server.get("port", cls.model_fields["PORT"].default), + "ALLOWED_ORIGINS": server.get("allowed_origins", cls.model_fields["ALLOWED_ORIGINS"].default), + }) + +# Handle database settings +if "database" in yaml_data: + database = yaml_data["database"] + config_data.update({ + "DATABASE_URL": database.get("url", cls.model_fields["DATABASE_URL"].default), + }) + +# Handle redis settings +if "redis" in yaml_data: + redis = yaml_data["redis"] + config_data.update({ + "REDIS_URL": redis.get("url", cls.model_fields["REDIS_URL"].default), + }) + +# Handle rate limiting settings +if "rate_limit" in yaml_data: + rate_limit = yaml_data["rate_limit"] + config_data.update({ + "RATE_LIMIT_DEFAULT_RPM": rate_limit.get("default_rpm", cls.model_fields["RATE_LIMIT_DEFAULT_RPM"].default), + "RATE_LIMIT_AUTH_RPM": rate_limit.get("auth_rpm", cls.model_fields["RATE_LIMIT_AUTH_RPM"].default), + "RATE_LIMIT_CHAT_RPM": rate_limit.get("chat_rpm", cls.model_fields["RATE_LIMIT_CHAT_RPM"].default), + }) + +# Handle JWT settings +if "jwt" in yaml_data: + jwt = yaml_data["jwt"] + config_data.update({ + "SECRET_KEY": jwt.get("secret_key", cls.model_fields["SECRET_KEY"].default), + "ALGORITHM": jwt.get("algorithm", cls.model_fields["ALGORITHM"].default), + "ACCESS_TOKEN_EXPIRE_MINUTES": jwt.get("access_token_expire_minutes", cls.model_fields["ACCESS_TOKEN_EXPIRE_MINUTES"].default), + })Let me verify which configuration sections are expected in the YAML file:
🏁 Script executed:
#!/bin/bash # Description: Check for YAML configuration examples to understand expected structure # Look for YAML configuration files fd -e yml -e yaml | head -10 # Check if there's a config.example.yml file mentioned in past comments fd config.exampleLength of output: 115
🏁 Script executed:
#!/bin/bash # Display the contents of the example configuration to verify all sections cat backend/config.example.ymlLength of output: 2150
#!/bin/bash # Extract the Settings class field definitions before the from_yaml method sed -n '1,200p' backend/config.py | sed -n '/class Settings/,/def from_yaml/p'
#!/bin/bash # Extract class Settings field definitions prior to from_yaml sed -n '1,300p' backend/config.py | sed -n -e '1,/class Settings/d' -e '/def from_yaml/,$d'
🏁 Script executed:
#!/bin/bash # Extract class Settings field definitions in backend/config.py before from_yaml sed -n '/class Settings/,/def from_yaml/p' backend/config.py | sed '1d;$d'Length of output: 4618
#!/bin/bash # Check if Settings class defines logging or directories fields rg -n "LOGGING" backend/config.py || echo "No logging fields found" rg -n "DIRECTORIES" backend/config.py || echo "No directories fields found"
Ensure complete YAML section mapping in Settings.from_yaml
The
from_yamlmethod currently handles onlyappandllmsections. It should also mapserver,database,redis,rate_limit, and the JWT fields under thesecuritykey (the example YAML usessecurity, notjwt).File: backend/config.py
Location:Settings.from_yaml(around lines 218–259)Suggested diff:
# Handle app settings if "app" in yaml_data: config_data.update({ "APP_NAME": yaml_data["app"].get("name", cls.model_fields["APP_NAME"].default), "APP_VERSION": yaml_data["app"].get("version", cls.model_fields["APP_VERSION"].default), "DEBUG": yaml_data["app"].get("debug", cls.model_fields["DEBUG"].default) }) + # Handle server settings + if "server" in yaml_data: + server = yaml_data["server"] + config_data.update({ + "HOST": server.get("host", cls.model_fields["HOST"].default), + "PORT": server.get("port", cls.model_fields["PORT"].default), + "ALLOWED_ORIGINS": server.get("allowed_origins", cls.model_fields["ALLOWED_ORIGINS"].default), + }) + + # Handle database settings + if "database" in yaml_data: + db = yaml_data["database"] + config_data["DATABASE_URL"] = db.get("url", cls.model_fields["DATABASE_URL"].default) + + # Handle redis settings + if "redis" in yaml_data: + rd = yaml_data["redis"] + config_data["REDIS_URL"] = rd.get("url", cls.model_fields["REDIS_URL"].default) + + # Handle rate limiting settings + if "rate_limit" in yaml_data: + rl = yaml_data["rate_limit"] + config_data.update({ + "RATE_LIMIT_DEFAULT_RPM": rl.get("default_rpm", cls.model_fields["RATE_LIMIT_DEFAULT_RPM"].default), + "RATE_LIMIT_AUTH_RPM": rl.get("auth_rpm", cls.model_fields["RATE_LIMIT_AUTH_RPM"].default), + "RATE_LIMIT_CHAT_RPM": rl.get("chat_rpm", cls.model_fields["RATE_LIMIT_CHAT_RPM"].default), + }) + - # Handle JWT settings - if "jwt" in yaml_data: - jwt = yaml_data["jwt"] - config_data.update({ - "SECRET_KEY": jwt.get("secret_key", cls.model_fields["SECRET_KEY"].default), - "ALGORITHM": jwt.get("algorithm", cls.model_fields["ALGORITHM"].default), - "ACCESS_TOKEN_EXPIRE_MINUTES": jwt.get("access_token_expire_minutes", cls.model_fields["ACCESS_TOKEN_EXPIRE_MINUTES"].default), - }) + # Handle security settings (JWT) + if "security" in yaml_data: + sec = yaml_data["security"] + config_data.update({ + "SECRET_KEY": sec.get("secret_key", cls.model_fields["SECRET_KEY"].default), + "ALGORITHM": sec.get("algorithm", cls.model_fields["ALGORITHM"].default), + "ACCESS_TOKEN_EXPIRE_MINUTES": sec.get("access_token_expire_minutes", cls.model_fields["ACCESS_TOKEN_EXPIRE_MINUTES"].default), + })Note: The example YAML also includes
logginganddirectoriessections, but there are no corresponding fields onSettings. You can omit those or add them once the model fields exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/config.py(2 hunks)
🔇 Additional comments (6)
backend/config.py (6)
13-53: Well-implemented LLM configuration model with comprehensive validation.The
LLMConfigclass provides excellent parameter validation with appropriate bounds for all generation parameters. The field descriptions are clear and helpful for understanding each parameter's purpose.
55-76: BackendConfig model is well-structured with appropriate field validation.The configuration model properly handles optional fields for API credentials while requiring the essential
model_namefield. The timeout validation withgt=0ensures positive values.
94-139: Comprehensive LLM backend configuration with good defaults.The addition of multiple backend configurations with sensible defaults is well-implemented. The use of
default_factorywith lambda functions properly creates fresh instances for each setting.
165-169: Enhanced configuration with nested environment variable support.The addition of
env_nested_delimiter="__"enables hierarchical configuration via environment variables, which aligns well with the complex nested structure introduced by the LLM configurations.
200-205: Minor formatting improvement in database URL validator.The reformatting improves readability while maintaining the same logic. The updated docstring provides better clarity.
207-215: Efficient active backend configuration retrieval.The method provides a clean way to access the currently active backend configuration using a simple dictionary lookup.
| @model_validator(mode='before') | ||
| @classmethod | ||
| def validate_configs(cls, values: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Validate and transform configurations from environment variables.""" | ||
| # Handle OpenAI config | ||
| if values.get("ACTIVE_LLM_BACKEND") == "openai": | ||
| openai_config = values.get("OPENAI_CONFIG", {}) | ||
| if isinstance(openai_config, dict): | ||
| if "MODEL_NAME" in openai_config: | ||
| openai_config["model_name"] = openai_config.pop("MODEL_NAME") | ||
| if "API_KEY" in openai_config: | ||
| openai_config["api_key"] = openai_config.pop("API_KEY") | ||
| values["OPENAI_CONFIG"] = openai_config | ||
|
|
||
| # Handle LLM config | ||
| llm_config = values.get("LLM_CONFIG", {}) | ||
| if isinstance(llm_config, dict): | ||
| if "TEMPERATURE" in llm_config: | ||
| llm_config["temperature"] = float(llm_config.pop("TEMPERATURE")) | ||
| values["LLM_CONFIG"] = llm_config | ||
|
|
||
| return values |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model validator implementation needs improvement for robustness.
The validator has several issues that could lead to runtime errors:
- Missing type safety: The method assumes
valuesis a dictionary without verification - Hardcoded backend check: Only handles OpenAI, ignoring other backends
- No error handling: Missing validation for malformed data
Consider this improved implementation:
@model_validator(mode='before')
@classmethod
def validate_configs(cls, values) -> Dict[str, Any]:
"""Validate and transform configurations from environment variables."""
if not isinstance(values, dict):
return values
+ # Generic handler for all backend configs
+ backend_fields = ["HUGGINGFACE_CONFIG", "OPENAI_CONFIG", "ANTHROPIC_CONFIG", "OLLAMA_CONFIG"]
+ for backend_field in backend_fields:
+ backend_config = values.get(backend_field, {})
+ if isinstance(backend_config, dict):
+ # Normalize field names from environment variable format
+ for old_key in list(backend_config.keys()):
+ if old_key.isupper():
+ new_key = old_key.lower()
+ backend_config[new_key] = backend_config.pop(old_key)
+ values[backend_field] = backend_config
- # Handle OpenAI config
- if values.get("ACTIVE_LLM_BACKEND") == "openai":
- openai_config = values.get("OPENAI_CONFIG", {})
- if isinstance(openai_config, dict):
- if "MODEL_NAME" in openai_config:
- openai_config["model_name"] = openai_config.pop("MODEL_NAME")
- if "API_KEY" in openai_config:
- openai_config["api_key"] = openai_config.pop("API_KEY")
- values["OPENAI_CONFIG"] = openai_config
# Handle LLM config with error handling
llm_config = values.get("LLM_CONFIG", {})
if isinstance(llm_config, dict):
- if "TEMPERATURE" in llm_config:
- llm_config["temperature"] = float(llm_config.pop("TEMPERATURE"))
+ # Normalize all uppercase keys to lowercase
+ for old_key in list(llm_config.keys()):
+ if old_key.isupper():
+ try:
+ new_key = old_key.lower()
+ value = llm_config.pop(old_key)
+ # Convert numeric fields appropriately
+ if new_key in ["temperature", "top_p", "repetition_penalty", "presence_penalty", "frequency_penalty"]:
+ llm_config[new_key] = float(value)
+ elif new_key in ["max_length", "top_k"]:
+ llm_config[new_key] = int(value)
+ else:
+ llm_config[new_key] = value
+ except (ValueError, TypeError) as e:
+ # Log error but continue with default values
+ pass
values["LLM_CONFIG"] = llm_config
return values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @model_validator(mode='before') | |
| @classmethod | |
| def validate_configs(cls, values: Dict[str, Any]) -> Dict[str, Any]: | |
| """Validate and transform configurations from environment variables.""" | |
| # Handle OpenAI config | |
| if values.get("ACTIVE_LLM_BACKEND") == "openai": | |
| openai_config = values.get("OPENAI_CONFIG", {}) | |
| if isinstance(openai_config, dict): | |
| if "MODEL_NAME" in openai_config: | |
| openai_config["model_name"] = openai_config.pop("MODEL_NAME") | |
| if "API_KEY" in openai_config: | |
| openai_config["api_key"] = openai_config.pop("API_KEY") | |
| values["OPENAI_CONFIG"] = openai_config | |
| # Handle LLM config | |
| llm_config = values.get("LLM_CONFIG", {}) | |
| if isinstance(llm_config, dict): | |
| if "TEMPERATURE" in llm_config: | |
| llm_config["temperature"] = float(llm_config.pop("TEMPERATURE")) | |
| values["LLM_CONFIG"] = llm_config | |
| return values | |
| @model_validator(mode='before') | |
| @classmethod | |
| def validate_configs(cls, values) -> Dict[str, Any]: | |
| """Validate and transform configurations from environment variables.""" | |
| if not isinstance(values, dict): | |
| return values | |
| # Generic handler for all backend configs | |
| backend_fields = ["HUGGINGFACE_CONFIG", "OPENAI_CONFIG", "ANTHROPIC_CONFIG", "OLLAMA_CONFIG"] | |
| for backend_field in backend_fields: | |
| backend_config = values.get(backend_field, {}) | |
| if isinstance(backend_config, dict): | |
| # Normalize field names from environment variable format | |
| for old_key in list(backend_config.keys()): | |
| if old_key.isupper(): | |
| new_key = old_key.lower() | |
| backend_config[new_key] = backend_config.pop(old_key) | |
| values[backend_field] = backend_config | |
| # Handle LLM config with error handling | |
| llm_config = values.get("LLM_CONFIG", {}) | |
| if isinstance(llm_config, dict): | |
| # Normalize all uppercase keys to lowercase | |
| for old_key in list(llm_config.keys()): | |
| if old_key.isupper(): | |
| try: | |
| new_key = old_key.lower() | |
| value = llm_config.pop(old_key) | |
| # Convert numeric fields appropriately | |
| if new_key in ["temperature", "top_p", "repetition_penalty", "presence_penalty", "frequency_penalty"]: | |
| llm_config[new_key] = float(value) | |
| elif new_key in ["max_length", "top_k"]: | |
| llm_config[new_key] = int(value) | |
| else: | |
| llm_config[new_key] = value | |
| except (ValueError, TypeError): | |
| # Log error but continue with default values | |
| pass | |
| values["LLM_CONFIG"] = llm_config | |
| return values |
🤖 Prompt for AI Agents
In backend/config.py around lines 171 to 192, the validate_configs method lacks
type safety, only handles the "openai" backend, and has no error handling for
malformed data. To fix this, first verify that the input values parameter is a
dictionary before processing. Then, generalize the backend check to support
multiple backends instead of hardcoding "openai". Finally, add try-except blocks
or validation checks to handle and log errors gracefully when data is malformed
or missing expected keys, ensuring the method is robust and does not raise
runtime exceptions.
…'moderator' and 'user' roles with corresponding details and hashed passwords, enhancing user role management for authentication.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
backend/auth.py (1)
120-125: 💡 Verification agent🧩 Analysis chain
Propagate
roleclaim from the JWT payload
get_current_user()currently ignores therolevalue embedded in the token.
If downstream code (e.g., RBAC middleware) expects the role claim to be present in theTokenData, its absence forces an extra DB lookup or silently drops custom roles supplied by a trusted IdP.- username: str = payload.get("sub") - if username is None: - raise credentials_exception - token_data = TokenData(username=username) + username: str | None = payload.get("sub") + role: str | None = payload.get("role") + if username is None: + raise credentials_exception + token_data = TokenData(username=username, role=role)Ensure that
RBACMiddlewarereally relies oncurrent_user.role(fetched from the DB) and not exclusively on theroleJWT claim. If it does parse the claim directly, the above change is mandatory to avoid authorization bypasses.
🏁 Script executed:
#!/bin/bash # Locate TokenData definition rg -n "class TokenData" -A 5 # Inspect get_current_user function in auth.py rg -n "def get_current_user" -A 20 backend/auth.py # Find RBACMiddleware implementation and check how it uses 'role' rg -n "class RBACMiddleware" -A 20 # Search for any direct references to current_user.role rg -n "current_user.*role" -A 5Length of output: 2374
🏁 Script executed:
#!/bin/bash # Inspect the RBACMiddleware dispatch method to see how it determines the user's role rg -n "def dispatch" -A 50 backend/security.pyLength of output: 5569
Revise get_current_user to merge the JWT role claim into the returned User
Adding
roleto TokenData alone doesn’t propagate it—get_current_userstill fetches the DB user and returns that, so downstream RBAC always sees the DB-stored role. To honor a trusted IdP’sroleclaim, updateget_current_userin backend/auth.py so that after loading the user you override its.rolewhen the token supplies one:• File: backend/auth.py, get_current_user (around lines 120–131)
try: - payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM]) - username: str = payload.get("sub") - if username is None: - raise credentials_exception - token_data = TokenData(username=username) + payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM]) + username: str | None = payload.get("sub") + role: str | None = payload.get("role") + if username is None: + raise credentials_exception + token_data = TokenData(username=username, role=role) except JWTError: raise credentials_exception - user = get_user(username=token_data.username) + user = get_user(username=token_data.username) if user is None: raise credentials_exception + # Override DB role with JWT claim when present + if token_data.role is not None: + user.role = token_data.role return userThis ensures RBACMiddleware (and any
user.rolechecks) will use the JWT’sroleclaim when provided.backend/app.py (1)
131-134: 🛠️ Refactor suggestionInclude the user’s role when minting access tokens
The login endpoint omits the
roleclaim, yet the test suite (and likely RBAC middleware) relies on its presence. Embedding the claim avoids an extra DB read and keeps prod & test behaviour aligned.- access_token = create_access_token( - data={"sub": user.username}, expires_delta=access_token_expires - ) + access_token = create_access_token( + data={"sub": user.username, "role": user.role}, + expires_delta=access_token_expires, + )
🧹 Nitpick comments (8)
backend/auth.py (2)
7-7: Remove the unusedDictimport
Dictis no longer referenced after the refactor. Keeping it violates Ruff’sF401check and adds noise.-from typing import Optional, Dict, Literal +from typing import Optional, Literal🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
49-75: Consider deferring bcrypt hashing to runtimeHashing three passwords with bcrypt at import-time adds ±200 ms to every cold start.
If startup latency matters (e.g., serverless deployments), move the seeding logic into thelifespanhandler or a test fixture.tests/unit/test_security.py (2)
2-2: Drop unusedpytestimportThe module doesn’t call any
pytestAPIs; all tests run viaTestClient.
Removing the import silences RuffF401.-import pytest🧰 Tools
🪛 Ruff (0.11.9)
2-2:
pytestimported but unusedRemove unused import:
pytest(F401)
10-13: Trim unusedget_current_userfrom the import list
get_current_useris never referenced inside the tests.-from backend.auth import ( - User, get_current_user, create_access_token, - ACCESS_TOKEN_EXPIRE_MINUTES -) +from backend.auth import ( + User, create_access_token, + ACCESS_TOKEN_EXPIRE_MINUTES +)🧰 Tools
🪛 Ruff (0.11.9)
11-11:
backend.auth.get_current_userimported but unusedRemove unused import:
backend.auth.get_current_user(F401)
backend/app.py (1)
23-26: Remove the unusedrequires_moderatorimport
requires_moderatorisn’t referenced anywhere inbackend/app.py, triggering RuffF401.- SecurityMiddleware, RBACMiddleware, RequestValidationMiddleware, - requires_admin, requires_moderator, requires_user + SecurityMiddleware, RBACMiddleware, RequestValidationMiddleware, + requires_admin, requires_user🧰 Tools
🪛 Ruff (0.11.9)
25-25:
.security.requires_moderatorimported but unusedRemove unused import:
.security.requires_moderator(F401)
backend/security.py (3)
7-14: Remove unused imports to keep the module lint-clean.
Optional,OAuth2PasswordBearer, andsettingsare never referenced in this file, and Ruff is already warning about them. Prune them now to avoid dead code and to keep the import section readable.-from typing import List, Optional, Callable -from fastapi.security import OAuth2PasswordBearer -from .config import settings +from typing import List, Callable(
Response,Depends, etc. are still required, so keep those. )🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Optionalimported but unusedRemove unused import:
typing.Optional(F401)
9-9:
fastapi.security.OAuth2PasswordBearerimported but unusedRemove unused import:
fastapi.security.OAuth2PasswordBearer(F401)
13-13:
.config.settingsimported but unusedRemove unused import:
.config.settings(F401)
68-74: Hard-coded path → role mapping will not scale.Maintaining
endpoint_permissionsby hand quickly becomes error-prone as routes multiply. Prefer one of:
- Attaching a
required_roleattribute to eachAPIRouter/Routeand reading it fromrequest.scope["route"].- Relying on dependency-based guards (
requires_admin, etc.) per-route (FastAPI idiomatic).- Loading the mapping from configuration rather than code.
This keeps RBAC logic close to the route definitions and avoids diverging source-of-truths.
127-130:startswithmay grant unintended public access.
"/test/publicity"would currently match because it starts with"/test/public".
If you intend exact matches, switch to set membership; if you need prefix semantics, tighten with a trailing slash or use FastAPI’s route matching.Example exact check:
-return any(path.startswith(p) for p in self.public_paths) +return path in self.public_paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app.py(5 hunks)backend/auth.py(4 hunks)backend/security.py(1 hunks)tests/unit/test_security.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/security.py (1)
backend/auth.py (2)
get_current_user(112-131)User(37-43)
🪛 Ruff (0.11.9)
backend/auth.py
7-7: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
tests/unit/test_security.py
2-2: pytest imported but unused
Remove unused import: pytest
(F401)
11-11: backend.auth.get_current_user imported but unused
Remove unused import: backend.auth.get_current_user
(F401)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-31: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
35-35: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/security.py
7-7: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
9-9: fastapi.security.OAuth2PasswordBearer imported but unused
Remove unused import: fastapi.security.OAuth2PasswordBearer
(F401)
13-13: .config.settings imported but unused
Remove unused import: .config.settings
(F401)
110-110: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
111-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
172-172: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app.py
25-25: .security.requires_moderator imported but unused
Remove unused import: .security.requires_moderator
(F401)
149-149: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
150-150: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
150-150: Do not perform function call rate_limit_dependency in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
158-158: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call rate_limit_dependency in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| def _build_csp(self) -> str: | ||
| """Build Content Security Policy header value.""" | ||
| policies = [ | ||
| "default-src 'self'", | ||
| "img-src 'self' data: https:", | ||
| "script-src 'self'", | ||
| "style-src 'self' 'unsafe-inline'", | ||
| "font-src 'self'", | ||
| "frame-ancestors 'none'", | ||
| "base-uri 'self'", | ||
| "form-action 'self'" | ||
| ] | ||
| return "; ".join(policies) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-evaluate 'unsafe-inline' in the Content-Security-Policy.
Allowing inline styles ('unsafe-inline') substantially weakens XSS protections.
Unless you have a hard requirement for inline styles, consider using a nonce or hashed CSS instead and dropping the directive:
-"style-src 'self' 'unsafe-inline'",
+"style-src 'self'",If inline styles are unavoidable, limit the CSP to only those hashes/nonces that are strictly necessary.
This is a high-impact security hardening step.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _build_csp(self) -> str: | |
| """Build Content Security Policy header value.""" | |
| policies = [ | |
| "default-src 'self'", | |
| "img-src 'self' data: https:", | |
| "script-src 'self'", | |
| "style-src 'self' 'unsafe-inline'", | |
| "font-src 'self'", | |
| "frame-ancestors 'none'", | |
| "base-uri 'self'", | |
| "form-action 'self'" | |
| ] | |
| return "; ".join(policies) | |
| def _build_csp(self) -> str: | |
| """Build Content Security Policy header value.""" | |
| policies = [ | |
| "default-src 'self'", | |
| "img-src 'self' data: https:", | |
| "script-src 'self'", | |
| - "style-src 'self' 'unsafe-inline'", | |
| + "style-src 'self'", | |
| "font-src 'self'", | |
| "frame-ancestors 'none'", | |
| "base-uri 'self'", | |
| "form-action 'self'" | |
| ] | |
| return "; ".join(policies) |
🤖 Prompt for AI Agents
In backend/security.py around lines 30 to 42, the Content Security Policy
currently allows 'unsafe-inline' styles, which weakens XSS protection. To fix
this, remove 'unsafe-inline' from the style-src directive and instead implement
a nonce or hash-based approach for inline styles if they are necessary. Update
the CSP to include only the specific nonces or hashes required for inline styles
to enhance security.
| # Check content length | ||
| content_length = request.headers.get("content-length") | ||
| if content_length and int(content_length) > self.max_content_length: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, | ||
| detail="Request too large" |
There was a problem hiding this comment.
content-length parsing can raise ValueError.
int(content_length) will fail if the header is non-numeric (seen with some proxies).
Guard the conversion:
- content_length = request.headers.get("content-length")
- if content_length and int(content_length) > self.max_content_length:
+ content_length = request.headers.get("content-length")
+ try:
+ if content_length and int(content_length) > self.max_content_length:
+ raise HTTPException(
+ status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE,
+ detail="Request too large"
+ )
+ except ValueError:
+ # Malformed header – reject to be safe
raise HTTPException(
status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE,
detail="Request too large"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check content length | |
| content_length = request.headers.get("content-length") | |
| if content_length and int(content_length) > self.max_content_length: | |
| raise HTTPException( | |
| status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, | |
| detail="Request too large" | |
| # Check content length | |
| content_length = request.headers.get("content-length") | |
| try: | |
| if content_length and int(content_length) > self.max_content_length: | |
| raise HTTPException( | |
| status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, | |
| detail="Request too large" | |
| ) | |
| except ValueError: | |
| # Malformed header – reject to be safe | |
| raise HTTPException( | |
| status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, | |
| detail="Request too large" | |
| ) |
🤖 Prompt for AI Agents
In backend/security.py around lines 151 to 156, the code converts the
content-length header to an integer without handling potential ValueError if the
header is non-numeric. To fix this, wrap the int conversion in a try-except
block to catch ValueError and handle it gracefully, such as by raising an
HTTPException with a suitable error message or ignoring the header if invalid.
…e retrieval in create_access_token and updated TokenData to include role information, improving role-based access control in authentication.
…oduced test users with roles of admin, moderator, and user, including hashed passwords, to improve unit test coverage and facilitate role management in security tests.
…scheme from the auth module to enhance authentication capabilities and support token-based authorization.
…s: removed role parameter from create_test_token function, simplified token creation for user roles, and improved error message assertions for unauthorized access and request validation tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/auth.py (2)
7-7: Remove unused import.The
Dictimport from typing is not used anywhere in the code.-from typing import Optional, Dict, Literal +from typing import Optional, Literal🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
49-75: Consider performance implications of runtime password hashing.The current implementation hashes passwords at runtime during module import, which could impact startup performance. For production use, consider pre-computing these hashes or using a proper database.
Consider this approach for better performance:
# Pre-computed hashes (replace with actual database in production) fake_users_db = { "admin": { "username": "admin", "email": "admin@example.com", "full_name": "Admin User", "disabled": False, "role": "admin", - "hashed_password": pwd_context.hash("admin") + "hashed_password": "$2b$12$..." # Pre-computed hash }, # ... other users }tests/unit/test_security.py (2)
2-2: Remove unused imports.The
pytestandget_current_userimports are not used in the test file.-import pytest -from fastapi import FastAPI, Depends +from fastapi import FastAPI, Depends from fastapi.testclient import TestClient from datetime import timedelta from backend.security import ( SecurityMiddleware, RBACMiddleware, RequestValidationMiddleware, requires_admin, requires_moderator, requires_user ) from backend.auth import ( - User, get_current_user, create_access_token, + User, create_access_token, ACCESS_TOKEN_EXPIRE_MINUTES, fake_users_db, get_password_hash )Also applies to: 11-11
🧰 Tools
🪛 Ruff (0.11.9)
2-2:
pytestimported but unusedRemove unused import:
pytest(F401)
49-50: Consider test isolation concerns.Modifying the global
fake_users_dbcould cause test isolation issues if other tests depend on the original state. Consider using test fixtures or mocking instead.Consider using pytest fixtures for better test isolation:
+@pytest.fixture +def test_users_db(): + """Fixture providing test users without modifying global state.""" + original_db = fake_users_db.copy() + fake_users_db.update(test_users) + yield fake_users_db + fake_users_db.clear() + fake_users_db.update(original_db) -# Add test users to fake database -fake_users_db.update(test_users)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/auth.py(5 hunks)tests/unit/test_security.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/auth.py
7-7: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
tests/unit/test_security.py
2-2: pytest imported but unused
Remove unused import: pytest
(F401)
11-11: backend.auth.get_current_user imported but unused
Remove unused import: backend.auth.get_current_user
(F401)
58-58: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
62-62: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
66-66: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (9)
backend/auth.py (4)
22-25: Good practice: Centralized configuration management.Sourcing JWT configuration from the centralized settings object improves maintainability and follows the DRY principle.
35-35: Well-designed role-based user models.The enhancements to
TokenDataandUsermodels properly support RBAC:
- Optional role field in
TokenDatafor JWT payload extraction- Optional email field provides flexibility
- Literal type restriction on role field ensures type safety
- Sensible default role of "user"
Also applies to: 40-40, 43-43
128-131: Proper role extraction from JWT payload.The implementation correctly extracts the role from the JWT payload and includes it in the
TokenDatainstance, enabling downstream role-based access control.
108-113:Details
✅ Verification successful
Verify role inclusion in JWT tokens.
The implementation correctly retrieves the user role and includes it in the JWT payload. This supports the RBAC middleware functionality.
Let's verify that the role is properly embedded in tokens by checking the token creation flow:
🏁 Script executed:
#!/bin/bash # Description: Verify role inclusion in JWT token creation # Expected: Role should be added to JWT payload from user data # Search for role-related code in auth module rg -A 3 -B 3 "role.*token|token.*role" --type pyLength of output: 818
🏁 Script executed:
#!/bin/bash # Locate and inspect the create_access_token function to confirm role is added to payload rg -C 5 "def create_access_token" --type py backend/auth.pyLength of output: 407
🏁 Script executed:
#!/bin/bash # Dump full implementation of create_access_token to verify payload encoding sed -n '/^def create_access_token/,/^$/p' backend/auth.pyLength of output: 600
Confirmed Role Inclusion in JWT Payload
The
create_access_tokenfunction in backend/auth.py copies the incoming data, looks up the user bysub, and assignsto_encode["role"] = user.rolebefore encoding. On the decode side,payload.get("role")is passed intoTokenData. Unit tests in tests/unit/test_security.py exercise this flow. No further changes required.tests/unit/test_security.py (5)
58-58: Static analysis false positive: Depends usage is correct.The static analysis tool flags B008 warnings about
Dependsin argument defaults, but this is the standard and correct FastAPI pattern for dependency injection. These warnings can be safely ignored.Also applies to: 62-62, 66-66
🧰 Tools
🪛 Ruff (0.11.9)
58-58: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
85-96: Comprehensive security headers testing.The test properly verifies that all essential security headers are present:
- X-Content-Type-Options for MIME type sniffing protection
- X-Frame-Options for clickjacking prevention
- X-XSS-Protection for XSS mitigation
- Content-Security-Policy for content restriction
- Referrer-Policy for referrer information control
112-129: Thorough RBAC access control testing.The test suite comprehensively validates role-based access control:
- Users can only access appropriate endpoints based on their role
- Proper 403 Forbidden responses for insufficient privileges
- Hierarchical access (admin > moderator > user) working correctly
- Token-based authentication integration
Also applies to: 131-149, 151-166
168-185: Effective request validation testing.The tests properly validate the request validation middleware:
- Content length limits (10MB threshold)
- Content type validation
- Proper HTTP status codes (413 for payload too large, 415 for unsupported media type)
76-83: Well-designed test token helper.The
create_test_tokenfunction properly creates JWT tokens with embedded roles, supporting the RBAC testing scenarios.
…emoved redundant user retrieval, added role verification against the user object, and improved error handling for invalid tokens and roles.
… for unauthorized access by separating HTTPException handling, ensuring clearer role validation and response for invalid authentication credentials.
…tail from "Authentication required" to "Not authenticated" for clarity, and removed redundant handling of invalid authentication credentials in exception management.
…updated create_test_token function to retrieve and incorporate user role from fake_users_db, improving role-based access control in security tests.
… message in security tests from "Authentication required" to "Not authenticated" for consistency with middleware error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/security.py (2)
30-42: Re-evaluate'unsafe-inline'in the Content-Security-Policy.Allowing inline styles (
'unsafe-inline') substantially weakens XSS protections.
Unless you have a hard requirement for inline styles, consider using a nonce or hashed CSS instead and dropping the directive:-"style-src 'self' 'unsafe-inline'", +"style-src 'self'",If inline styles are unavoidable, limit the CSP to only those hashes/nonces that are strictly necessary.
This is a high-impact security hardening step.
154-159:content-lengthparsing can raiseValueError.
int(content_length)will fail if the header is non-numeric (seen with some proxies).
Guard the conversion:content_length = request.headers.get("content-length") - if content_length and int(content_length) > self.max_content_length: + try: + if content_length and int(content_length) > self.max_content_length: + raise HTTPException( + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, + detail="Request too large" + ) + except ValueError: + # Malformed header – reject to be safe raise HTTPException( status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, detail="Request too large" )
🧹 Nitpick comments (5)
backend/auth.py (2)
7-7: Remove unused import.The
Dicttype fromtypingis imported but never used in the code.-from typing import Optional, Dict, Literal +from typing import Optional, Literal🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
130-140: Redundant role verification may impact performance.The role verification in
get_current_user()requires an additional database lookup to verify the role matches the JWT payload. Since the JWT is already signed and verified, this additional check may be unnecessary and impacts performance.Consider whether this level of verification is needed, or implement caching for user lookups.
tests/unit/test_security.py (2)
2-2: Remove unused import.The
pytestimport is not used in this test file.-import pytest -from fastapi import FastAPI, Depends +from fastapi import FastAPI, Depends🧰 Tools
🪛 Ruff (0.11.9)
2-2:
pytestimported but unusedRemove unused import:
pytest(F401)
11-11: Remove unused import.The
get_current_userfunction is imported but not directly used in the test file.from backend.auth import ( - User, get_current_user, create_access_token, + User, create_access_token, ACCESS_TOKEN_EXPIRE_MINUTES, fake_users_db, get_password_hash )🧰 Tools
🪛 Ruff (0.11.9)
11-11:
backend.auth.get_current_userimported but unusedRemove unused import:
backend.auth.get_current_user(F401)
backend/security.py (1)
7-7: Remove unused imports.Several imports are not used in the code and should be removed for clarity.
-from typing import List, Optional, Callable +from typing import List, Callable from fastapi import Request, Response, HTTPException, status, Depends -from fastapi.security import OAuth2PasswordBearer from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint from starlette.types import ASGIApp -from .auth import get_current_user, User, oauth2_scheme -from .config import settings +from .auth import get_current_user, UserAlso applies to: 9-9, 12-12, 13-13
🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Optionalimported but unusedRemove unused import:
typing.Optional(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/auth.py(5 hunks)backend/security.py(1 hunks)tests/unit/test_security.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/security.py (1)
backend/auth.py (2)
get_current_user(118-142)User(37-43)
tests/unit/test_security.py (2)
backend/security.py (3)
SecurityMiddleware(15-54)RBACMiddleware(56-136)RequestValidationMiddleware(138-170)backend/auth.py (4)
User(37-43)get_current_user(118-142)create_access_token(101-116)get_password_hash(81-83)
🪛 Ruff (0.11.9)
backend/security.py
7-7: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
9-9: fastapi.security.OAuth2PasswordBearer imported but unused
Remove unused import: fastapi.security.OAuth2PasswordBearer
(F401)
12-12: .auth.oauth2_scheme imported but unused
Remove unused import: .auth.oauth2_scheme
(F401)
13-13: .config.settings imported but unused
Remove unused import: .config.settings
(F401)
122-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
174-174: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/auth.py
7-7: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
tests/unit/test_security.py
2-2: pytest imported but unused
Remove unused import: pytest
(F401)
11-11: backend.auth.get_current_user imported but unused
Remove unused import: backend.auth.get_current_user
(F401)
58-58: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
62-62: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
66-66: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (5)
backend/auth.py (1)
108-113: 🛠️ Refactor suggestionPotential security issue with role injection.
The current implementation retrieves the user role by calling
get_user()insidecreate_access_token(). This could lead to inconsistencies if the user data changes between authentication and token creation, or if there are race conditions.Consider passing the role as a parameter or ensuring the user object is passed instead of just the username.
-def create_access_token(data: dict, expires_delta: Optional[timedelta] = None) -> str: +def create_access_token(data: dict, expires_delta: Optional[timedelta] = None, user_role: Optional[str] = None) -> str: """Create access token.""" to_encode = data.copy() if expires_delta: expire = datetime.utcnow() + expires_delta else: expire = datetime.utcnow() + timedelta(minutes=15) - # Get user role - user = get_user(data.get("sub")) - if user: - to_encode["role"] = user.role + # Add user role if provided + if user_role: + to_encode["role"] = user_role to_encode.update({"exp": expire}) encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM) return encoded_jwtLikely an incorrect or invalid review comment.
tests/unit/test_security.py (2)
58-58: FastAPI dependency injection in function defaults.Using
Depends()in function argument defaults is flagged by static analysis as it can cause issues with dependency injection timing.However, this is the standard FastAPI pattern for route dependencies and is working correctly in this context. The static analysis warning can be safely ignored here.
Also applies to: 62-62, 66-66
🧰 Tools
🪛 Ruff (0.11.9)
58-58: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
168-187: Excellent test coverage for request validation.The request validation tests properly cover both content length limits and content type validation, including edge cases and valid requests.
backend/security.py (2)
174-174: FastAPI dependency injection pattern is correct.The use of
Depends()in the function parameter default is flagged by static analysis, but this is the standard and correct FastAPI pattern for dependency injection. The warning can be safely ignored.🧰 Tools
🪛 Ruff (0.11.9)
174-174: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
183-186: Well-designed role hierarchy dependencies.The convenience dependencies properly implement the role hierarchy where admin has access to all levels, moderator to moderator and user levels, and user to user level only.
| # Simulated database (replace with actual database in production) | ||
| fake_users_db = { | ||
| "admin": { | ||
| "username": "admin", | ||
| "email": "admin@example.com", | ||
| "full_name": "Admin User", | ||
| "disabled": False, | ||
| "role": "admin", | ||
| "hashed_password": pwd_context.hash("admin") | ||
| }, | ||
| "moderator": { | ||
| "username": "moderator", | ||
| "email": "moderator@example.com", | ||
| "full_name": "Moderator User", | ||
| "disabled": False, | ||
| "role": "moderator", | ||
| "hashed_password": pwd_context.hash("moderator") | ||
| }, | ||
| "user": { | ||
| "username": "user", | ||
| "email": "user@example.com", | ||
| "full_name": "Regular User", | ||
| "disabled": False, | ||
| "role": "user", | ||
| "hashed_password": pwd_context.hash("user") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Passwords hashed at runtime.
The user passwords are being hashed at module import time, which means they're recalculated on every server restart. This is inefficient and could cause performance issues during server startup.
Consider pre-computing these hashes or using a proper database with seeded data.
-# Simulated database (replace with actual database in production)
-fake_users_db = {
- "admin": {
- "username": "admin",
- "email": "admin@example.com",
- "full_name": "Admin User",
- "disabled": False,
- "role": "admin",
- "hashed_password": pwd_context.hash("admin")
- },
- "moderator": {
- "username": "moderator",
- "email": "moderator@example.com",
- "full_name": "Moderator User",
- "disabled": False,
- "role": "moderator",
- "hashed_password": pwd_context.hash("moderator")
- },
- "user": {
- "username": "user",
- "email": "user@example.com",
- "full_name": "Regular User",
- "disabled": False,
- "role": "user",
- "hashed_password": pwd_context.hash("user")
- }
-}
+# Simulated database (replace with actual database in production)
+fake_users_db = {
+ "admin": {
+ "username": "admin",
+ "email": "admin@example.com",
+ "full_name": "Admin User",
+ "disabled": False,
+ "role": "admin",
+ "hashed_password": "$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW" # "admin"
+ },
+ "moderator": {
+ "username": "moderator",
+ "email": "moderator@example.com",
+ "full_name": "Moderator User",
+ "disabled": False,
+ "role": "moderator",
+ "hashed_password": "$2b$12$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi" # "moderator"
+ },
+ "user": {
+ "username": "user",
+ "email": "user@example.com",
+ "full_name": "Regular User",
+ "disabled": False,
+ "role": "user",
+ "hashed_password": "$2b$12$kixWuVoH.LkXWvRVCrgnPOPVFoD30WBVAQPQzF0yGt6sTPYo4MoXy" # "user"
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Simulated database (replace with actual database in production) | |
| fake_users_db = { | |
| "admin": { | |
| "username": "admin", | |
| "email": "admin@example.com", | |
| "full_name": "Admin User", | |
| "disabled": False, | |
| "role": "admin", | |
| "hashed_password": pwd_context.hash("admin") | |
| }, | |
| "moderator": { | |
| "username": "moderator", | |
| "email": "moderator@example.com", | |
| "full_name": "Moderator User", | |
| "disabled": False, | |
| "role": "moderator", | |
| "hashed_password": pwd_context.hash("moderator") | |
| }, | |
| "user": { | |
| "username": "user", | |
| "email": "user@example.com", | |
| "full_name": "Regular User", | |
| "disabled": False, | |
| "role": "user", | |
| "hashed_password": pwd_context.hash("user") | |
| } | |
| } | |
| # Simulated database (replace with actual database in production) | |
| fake_users_db = { | |
| "admin": { | |
| "username": "admin", | |
| "email": "admin@example.com", | |
| "full_name": "Admin User", | |
| "disabled": False, | |
| "role": "admin", | |
| "hashed_password": "$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW" # "admin" | |
| }, | |
| "moderator": { | |
| "username": "moderator", | |
| "email": "moderator@example.com", | |
| "full_name": "Moderator User", | |
| "disabled": False, | |
| "role": "moderator", | |
| "hashed_password": "$2b$12$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi" # "moderator" | |
| }, | |
| "user": { | |
| "username": "user", | |
| "email": "user@example.com", | |
| "full_name": "Regular User", | |
| "disabled": False, | |
| "role": "user", | |
| "hashed_password": "$2b$12$kixWuVoH.LkXWvRVCrgnPOPVFoD30WBVAQPQzF0yGt6sTPYo4MoXy" # "user" | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/auth.py between lines 49 and 75, the user passwords are hashed at
module import time, causing unnecessary computation on every server restart. To
fix this, pre-compute the password hashes once and replace the pwd_context.hash
calls with the resulting hashed strings in the fake_users_db dictionary.
Alternatively, move to using a proper database with seeded hashed passwords to
avoid runtime hashing.
| # Add test users to fake database | ||
| fake_users_db.update(test_users) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Direct mutation of shared state is problematic.
Directly updating fake_users_db from the auth module can cause test isolation issues and side effects between tests. Test users could persist and affect other tests.
Consider using dependency injection or mocking to isolate test data.
+import pytest
+from unittest.mock import patch
+@pytest.fixture(autouse=True)
+def mock_users_db():
+ """Mock the users database for tests."""
+ with patch('backend.auth.fake_users_db', test_users):
+ yield
-# Add test users to fake database
-fake_users_db.update(test_users)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add test users to fake database | |
| fake_users_db.update(test_users) | |
| import pytest | |
| from unittest.mock import patch | |
| @pytest.fixture(autouse=True) | |
| def mock_users_db(): | |
| """Mock the users database for tests.""" | |
| with patch('backend.auth.fake_users_db', test_users): | |
| yield |
🤖 Prompt for AI Agents
In tests/unit/test_security.py around lines 49 to 50, avoid directly mutating
the shared fake_users_db dictionary as it can cause test isolation issues.
Instead, refactor the test to use dependency injection or mocking to provide
test-specific user data, ensuring each test has isolated state and does not
affect others. This can be done by injecting a fresh copy of the user database
or mocking the data access layer in the test setup.
| except HTTPException as e: | ||
| if e.status_code == status.HTTP_401_UNAUTHORIZED: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Invalid authentication credentials", | ||
| headers={"WWW-Authenticate": "Bearer"} | ||
| ) | ||
| raise e |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling specificity.
The current exception handling catches HTTPException and re-raises it with a different message, which could mask the original error details. Also, the raise ... from err pattern should be used for better error traceability.
except HTTPException as e:
if e.status_code == status.HTTP_401_UNAUTHORIZED:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Invalid authentication credentials",
headers={"WWW-Authenticate": "Bearer"}
- )
+ ) from e
raise e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except HTTPException as e: | |
| if e.status_code == status.HTTP_401_UNAUTHORIZED: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Invalid authentication credentials", | |
| headers={"WWW-Authenticate": "Bearer"} | |
| ) | |
| raise e | |
| except HTTPException as e: | |
| if e.status_code == status.HTTP_401_UNAUTHORIZED: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Invalid authentication credentials", | |
| headers={"WWW-Authenticate": "Bearer"} | |
| ) from e | |
| raise e |
🧰 Tools
🪛 Ruff (0.11.9)
122-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/security.py around lines 120 to 127, improve exception handling by
using the 'raise ... from e' pattern when re-raising the HTTPException to
preserve the original error context. Modify the code to raise the new
HTTPException with the custom message from the caught exception 'e' using 'raise
... from e' instead of just 'raise e' to enhance error traceability without
masking original details.
…rer from "api/v1/auth/token" to "/api/v1/auth/token" for correct endpoint reference.
…or handling: changed the way test users are added to fake_users_db for clarity, and improved the assertion for handling large content requests to ensure proper error messaging.
…rs to fake_users_db to use a copy of user data, preventing reference issues and ensuring data integrity during tests.
…a dynamic role hierarchy for improved maintainability and scalability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/auth.py (1)
49-75:⚠️ Potential issueCritical performance issue: Runtime password hashing.
This is a duplicate of a previously identified issue. The passwords are being hashed at module import time, causing unnecessary computation on every server restart and potential performance issues.
Apply this diff to use pre-computed hashes:
-# Simulated database (replace with actual database in production) -fake_users_db = { - "admin": { - "username": "admin", - "email": "admin@example.com", - "full_name": "Admin User", - "disabled": False, - "role": "admin", - "hashed_password": pwd_context.hash("admin") - }, - "moderator": { - "username": "moderator", - "email": "moderator@example.com", - "full_name": "Moderator User", - "disabled": False, - "role": "moderator", - "hashed_password": pwd_context.hash("moderator") - }, - "user": { - "username": "user", - "email": "user@example.com", - "full_name": "Regular User", - "disabled": False, - "role": "user", - "hashed_password": pwd_context.hash("user") - } -} +# Simulated database (replace with actual database in production) +fake_users_db = { + "admin": { + "username": "admin", + "email": "admin@example.com", + "full_name": "Admin User", + "disabled": False, + "role": "admin", + "hashed_password": "$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW" # "admin" + }, + "moderator": { + "username": "moderator", + "email": "moderator@example.com", + "full_name": "Moderator User", + "disabled": False, + "role": "moderator", + "hashed_password": "$2b$12$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi" # "moderator" + }, + "user": { + "username": "user", + "email": "user@example.com", + "full_name": "Regular User", + "disabled": False, + "role": "user", + "hashed_password": "$2b$12$kixWuVoH.LkXWvRVCrgnPOPVFoD30WBVAQPQzF0yGt6sTPYo4MoXy" # "user" + } +}tests/unit/test_security.py (1)
21-64:⚠️ Potential issueCritical test isolation issue: Direct mutation of shared state.
This is a duplicate of a previously identified issue. Directly updating
fake_users_dbfrom the auth module can cause test isolation problems and side effects between tests.Apply this diff to use dependency injection for better test isolation:
+import pytest +from unittest.mock import patch +@pytest.fixture(autouse=True) +def mock_users_db(): + """Mock the users database for tests.""" + test_users = { + "test_admin": { + "username": "test_admin", + "email": "test_admin@example.com", + "full_name": "Test Admin", + "disabled": False, + "role": "admin", + "hashed_password": get_password_hash("test_admin") + }, + "test_moderator": { + "username": "test_moderator", + "email": "test_moderator@example.com", + "full_name": "Test Moderator", + "disabled": False, + "role": "moderator", + "hashed_password": get_password_hash("test_moderator") + }, + "test_user": { + "username": "test_user", + "email": "test_user@example.com", + "full_name": "Test User", + "disabled": False, + "role": "user", + "hashed_password": get_password_hash("test_user") + } + } + with patch('backend.auth.fake_users_db', test_users): + yield -@pytest.fixture(autouse=True) -def setup_test_users(): - """Setup test users before each test.""" - # Create test users - test_users = { - "test_admin": { - "username": "test_admin", - "email": "test_admin@example.com", - "full_name": "Test Admin", - "disabled": False, - "role": "admin", - "hashed_password": get_password_hash("test_admin") - }, - "test_moderator": { - "username": "test_moderator", - "email": "test_moderator@example.com", - "full_name": "Test Moderator", - "disabled": False, - "role": "moderator", - "hashed_password": get_password_hash("test_moderator") - }, - "test_user": { - "username": "test_user", - "email": "test_user@example.com", - "full_name": "Test User", - "disabled": False, - "role": "user", - "hashed_password": get_password_hash("test_user") - } - } - - # Clear existing test users - for username in ["test_admin", "test_moderator", "test_user"]: - fake_users_db.pop(username, None) - - # Add test users to fake database - fake_users_db.update(test_users) - - yield - - # Cleanup after test - for username in ["test_admin", "test_moderator", "test_user"]: - fake_users_db.pop(username, None)backend/security.py (2)
30-42:⚠️ Potential issueSecurity concern: CSP allows unsafe inline styles.
This is a duplicate of a previously identified security issue. The Content Security Policy currently allows
'unsafe-inline'styles, which weakens XSS protection.Apply this diff to enhance security:
policies = [ "default-src 'self'", "img-src 'self' data: https:", "script-src 'self'", - "style-src 'self' 'unsafe-inline'", + "style-src 'self'", "font-src 'self'", "frame-ancestors 'none'", "base-uri 'self'", "form-action 'self'" ]
152-158:⚠️ Potential issuePotential vulnerability: Content-length parsing can fail.
The
int(content_length)conversion will raise aValueErrorif the header contains non-numeric values, which can occur with some proxies or malicious requests.Apply this diff to handle malformed headers safely:
# Check content length content_length = request.headers.get("content-length") - if content_length and int(content_length) > self.max_content_length: - raise HTTPException( - status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, - detail="Request too large" - ) + try: + if content_length and int(content_length) > self.max_content_length: + raise HTTPException( + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, + detail="Request too large" + ) + except ValueError: + # Malformed header – reject to be safe + raise HTTPException( + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, + detail="Request too large" + )
🧹 Nitpick comments (3)
backend/auth.py (1)
7-7: Remove unused import.The
Dicttype import is not used anywhere in the code.Apply this diff to remove the unused import:
-from typing import Optional, Dict, Literal +from typing import Optional, Literal🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
tests/unit/test_security.py (1)
11-11: Remove unused import.The
get_current_userimport is not directly used in the test code, as it's called through the middleware and dependencies.Apply this diff to remove the unused import:
from backend.auth import ( - User, get_current_user, create_access_token, + User, create_access_token, ACCESS_TOKEN_EXPIRE_MINUTES, fake_users_db, get_password_hash )🧰 Tools
🪛 Ruff (0.11.9)
11-11:
backend.auth.get_current_userimported but unusedRemove unused import:
backend.auth.get_current_user(F401)
backend/security.py (1)
7-13: Remove unused imports.Several imports are flagged as unused by static analysis and should be removed to keep the code clean.
Apply this diff to remove unused imports:
-from typing import List, Optional, Callable +from typing import List, Callable from fastapi import Request, Response, HTTPException, status, Depends -from fastapi.security import OAuth2PasswordBearer from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint from starlette.types import ASGIApp -from .auth import get_current_user, User, oauth2_scheme +from .auth import get_current_user, User -from .config import settings🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Optionalimported but unusedRemove unused import:
typing.Optional(F401)
9-9:
fastapi.security.OAuth2PasswordBearerimported but unusedRemove unused import:
fastapi.security.OAuth2PasswordBearer(F401)
12-12:
.auth.oauth2_schemeimported but unusedRemove unused import:
.auth.oauth2_scheme(F401)
13-13:
.config.settingsimported but unusedRemove unused import:
.config.settings(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/auth.py(5 hunks)backend/security.py(1 hunks)tests/unit/test_security.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/test_security.py (2)
backend/security.py (3)
SecurityMiddleware(15-54)RBACMiddleware(56-135)RequestValidationMiddleware(137-169)backend/auth.py (4)
User(37-43)get_current_user(118-142)create_access_token(101-116)get_password_hash(81-83)
backend/security.py (1)
backend/auth.py (2)
get_current_user(118-142)User(37-43)
🪛 Ruff (0.11.9)
tests/unit/test_security.py
11-11: backend.auth.get_current_user imported but unused
Remove unused import: backend.auth.get_current_user
(F401)
71-71: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
75-75: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
79-79: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/security.py
7-7: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
9-9: fastapi.security.OAuth2PasswordBearer imported but unused
Remove unused import: fastapi.security.OAuth2PasswordBearer
(F401)
12-12: .auth.oauth2_scheme imported but unused
Remove unused import: .auth.oauth2_scheme
(F401)
13-13: .config.settings imported but unused
Remove unused import: .config.settings
(F401)
173-173: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/auth.py
7-7: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
🔇 Additional comments (17)
backend/auth.py (6)
22-25: LGTM! Configuration centralization is a good practice.Moving JWT configuration to the centralized settings object improves maintainability and follows the DRY principle.
35-35: LGTM! Role field addition supports RBAC functionality.The addition of the role field to TokenData properly supports the role-based access control system being implemented.
40-43: LGTM! User model enhancements support the security framework.The changes to make email optional and add role constraints are well-designed for the RBAC system.
92-99: LGTM! Return type change aligns with updated architecture.The change from
Optional[UserInDB]toOptional[User]is consistent with the security framework requirements.
108-116: LGTM! Role inclusion in JWT tokens enables RBAC.The enhancement to include user roles in JWT tokens is correctly implemented and essential for the role-based access control middleware.
130-140: LGTM! Role verification strengthens token security.The addition of role verification in token validation prevents role manipulation attacks and ensures token integrity.
tests/unit/test_security.py (5)
71-81: Consider using dependency injection for test endpoints.The static analysis correctly identifies that
Depends()calls in argument defaults can cause issues. However, in this test context, these are mock endpoints specifically designed to test the dependency behavior, so this is acceptable.🧰 Tools
🪛 Ruff (0.11.9)
71-71: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
75-75: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
79-79: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
99-111: LGTM! Comprehensive security headers testing.The test properly verifies that all expected security headers are present in responses.
118-128: LGTM! Good exception handling in unauthorized tests.The test correctly handles both direct HTTP responses and exceptions that might be raised during authentication failures.
129-184: LGTM! Thorough RBAC testing.The tests comprehensively verify the role hierarchy and access control enforcement across different user roles.
185-224: LGTM! Request validation testing covers key scenarios.The tests properly verify content length limits and content type validation with appropriate exception handling.
backend/security.py (6)
15-54: LGTM! Comprehensive security headers implementation.The SecurityMiddleware properly implements essential security headers following industry best practices. The modular approach with
_build_csp()makes the CSP configuration maintainable.
68-84: LGTM! Well-designed endpoint permission mapping.The role hierarchy and endpoint permissions are clearly defined and support the RBAC requirements. The public paths list appropriately excludes authentication and documentation endpoints.
107-122: LGTM! Exception handling is appropriately specific.The exception handling correctly propagates HTTPException while catching other authentication-related errors. This addresses previous concerns about overly broad exception handling.
127-135: LGTM! Role access logic is clear and secure.The role hierarchy enforcement is correctly implemented with explicit checks for each role level.
137-169: LGTM! Request validation middleware provides essential protections.The middleware properly validates content length and content types, providing good protection against oversized requests and unsupported media types.
171-185: LGTM! Role-based dependencies are well-designed.The
requires_rolesfactory function and convenience dependencies provide a clean, reusable way to enforce role-based access control on endpoints.🧰 Tools
🪛 Ruff (0.11.9)
173-173: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
…chy for improved permission checks and access management.
chojuninengu
dismissed
Nkwenti-Severian-Ndongtsop’s stale review
The merge-base changed after approval.
Nkwenti-Severian-Ndongtsop
merged commit 0d86111
into
Cameroon-Developer-Network:main
2 participants
Summary by CodeRabbit
New Features
Chores
Tests
Documentation