Add SMP encoders service and environment-driven Django settings with security defaults#913
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR introduces two independent functional changes: Django settings now retrieve configuration and security properties from environment variables with production-safe defaults, and a new Encoders microservice is added to the MAGE gateway with endpoints for health checks and dynamic encoder listing. ChangesDjango Settings Hardening
Encoders Service and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 1 minor |
| Security | 1 high |
🟢 Metrics 5 complexity
Metric Results Complexity 5
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ] | ||
|
|
||
| ALLOWED_HOSTS = [] | ||
| if not DEBUG and SECRET_KEY == "dev-insecure-key-change-me": |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678f259ced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SECURE_REFERRER_POLICY = "same-origin" | ||
| CSRF_COOKIE_SECURE = env_bool("DJANGO_CSRF_COOKIE_SECURE", default=not DEBUG) | ||
| SESSION_COOKIE_SECURE = env_bool("DJANGO_SESSION_COOKIE_SECURE", default=not DEBUG) | ||
| SECURE_SSL_REDIRECT = env_bool("DJANGO_SECURE_SSL_REDIRECT", default=not DEBUG) |
There was a problem hiding this comment.
Turning SECURE_SSL_REDIRECT on by default in non-debug mode can break deployments behind a TLS-terminating reverse proxy, because Django will see backend traffic as HTTP unless SECURE_PROXY_SSL_HEADER is configured. In that setup, every request is treated as insecure and redirected again, causing redirect loops and an effectively unavailable app; this change introduces that risk for any non-debug environment that doesn’t explicitly override the new default.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'encoders' service to list SMP encoders and their TIMM-backed variants, including the necessary API routes and tests. Additionally, it updates 'settings.py' to include a helper for boolean environment variables, improved configuration for 'DEBUG' and 'ALLOWED_HOSTS', and new security hardening settings. The review feedback highlights that the 'encoders' service should pre-calculate metadata to avoid blocking imports in the request path and notes that the 'SECURE_BROWSER_XSS_FILTER' setting is obsolete in modern Django versions and should be removed.
| from importlib import import_module | ||
|
|
||
| from fastapi import APIRouter, FastAPI, HTTPException | ||
|
|
||
| router = APIRouter(tags=["encoders"]) | ||
|
|
||
|
|
||
| @router.get("/health") | ||
| async def healthcheck() -> dict[str, str]: | ||
| """Service-local health-check endpoint.""" | ||
| return {"encoders_service": "UP"} | ||
|
|
||
|
|
||
| @router.get("/encoders") | ||
| async def list_encoders() -> dict[str, list[str] | int]: | ||
| """List SMP encoders and a subset that are TIMM-backed.""" | ||
| try: | ||
| smp = import_module("segmentation_models_pytorch") | ||
| smp_encoders = smp.encoders | ||
| except Exception as exc: | ||
| raise HTTPException(status_code=503, detail="segmentation_models_pytorch is unavailable") from exc | ||
|
|
||
| names = sorted(smp_encoders.get_encoder_names()) | ||
| timm_backed = [name for name in names if name.startswith("tu-")] | ||
| return { | ||
| "encoders": names, | ||
| "timm_backed_encoders": timm_backed, | ||
| "total": len(names), | ||
| "timm_backed_total": len(timm_backed), | ||
| } |
There was a problem hiding this comment.
Importing a heavy library like segmentation_models_pytorch inside an async request handler is inefficient and can block the FastAPI event loop, especially during the first call when the module is actually loaded. Since the list of encoders is static for a given installation, it should be imported and computed once at the module level or cached. Additionally, catching a generic Exception is too broad; it's better to specifically catch ImportError or AttributeError to handle missing optional dependencies.
from fastapi import APIRouter, FastAPI, HTTPException
# Pre-calculate encoder metadata to avoid expensive/blocking imports in the request path.
try:
import segmentation_models_pytorch as smp
_NAMES = sorted(smp.encoders.get_encoder_names())
_TIMM_BACKED = [n for n in _NAMES if n.startswith("tu-")]
_ENCODER_DATA = {
"encoders": _NAMES,
"timm_backed_encoders": _TIMM_BACKED,
"total": len(_NAMES),
"timm_backed_total": len(_TIMM_BACKED),
}
except (ImportError, AttributeError):
_ENCODER_DATA = None
router = APIRouter(tags=["encoders"])
@router.get("/health")
async def healthcheck() -> dict[str, str]:
"""Service-local health-check endpoint."""
return {"encoders_service": "UP"}
@router.get("/encoders")
async def list_encoders() -> dict[str, list[str] | int]:
"""List SMP encoders and a subset that are TIMM-backed."""
if _ENCODER_DATA is None:
raise HTTPException(status_code=503, detail="segmentation_models_pytorch is unavailable")
return _ENCODER_DATA|
|
||
|
|
||
| # Basic production security hardening. These can be tuned via environment. | ||
| SECURE_BROWSER_XSS_FILTER = True |
There was a problem hiding this comment.
The SECURE_BROWSER_XSS_FILTER setting was deprecated in Django 3.0 and removed in Django 4.0. Since this project is using Django 6.0.1 (as indicated in the file header), this setting is obsolete and has no effect. It should be removed to avoid configuration clutter.
2 participants
Motivation
Description
env_boolhelper and switchedDEBUGto read fromDJANGO_DEBUG, populatedALLOWED_HOSTSfromDJANGO_ALLOWED_HOSTS, and enforced that a non-defaultDJANGO_SECRET_KEYis required when not debugging inFARM/settings.py.FARM/settings.py(e.g.SECURE_BROWSER_XSS_FILTER,SECURE_CONTENT_TYPE_NOSNIFF,X_FRAME_OPTIONS,SECURE_REFERRER_POLICY,CSRF_COOKIE_SECURE,SESSION_COOKIE_SECURE,SECURE_SSL_REDIRECT) with defaults controllable via environment variables.MAGE/api/services/encoders.pythat inspectssegmentation_models_pytorchand returns encoder names and a list of TIMM-backed encoders.MAGE/api/main.pyby mounting/services/encodersand adding a backward-compatible/encodersroute.MAGE/tests/test_main.pywith aFakeSMPshim, registered it in the import fixture, and addedtest_encoders_listto validate the/encodersendpoint.Testing
pytest MAGE/tests/test_main.pyand the suite succeeded.test_encoders_listpassed, validating encoder discovery and detection of TIMM-backed names.Codex Task
Summary by CodeRabbit
New Features
/encodersAPI endpoint that lists available encoders and identifies TIMM-backed variants.Configuration