Fix #34: Replace setup redirect allowlist prefix matching with explicit allowlist
- Replace fragile startswith() matching with explicit path matching - Split allowlist into _EXACT_ALLOWED (exact paths) and _PREFIX_ALLOWED (prefixes) - Prefix paths MUST end with '/' to prevent matching unintended paths like /api/setup-debug - Paths correctly matched: /api/setup, /api/health, /api/docs, /api/redoc, /api/openapi.json, /api/setup/timezone - Paths correctly blocked: /api/setup-debug, /api/setup123, /api/jails - Add comprehensive Setup Guard Route Policy documentation to Backend-Development.md - Update line numbers in documentation to reflect current implementation This prevents future route additions from accidentally bypassing the setup guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1726,6 +1726,57 @@ Cookie: bangui_session=...
|
|||||||
(no X-BanGUI-Request header needed)
|
(no X-BanGUI-Request header needed)
|
||||||
```
|
```
|
||||||
|
|
||||||
|
### Setup Guard Route Policy
|
||||||
|
|
||||||
|
BanGUI requires a one-time setup wizard to be completed before the application is usable. The `SetupRedirectMiddleware` enforces this by redirecting unauthenticated API requests to `/api/setup` until setup is complete.
|
||||||
|
|
||||||
|
**How It Works:**
|
||||||
|
|
||||||
|
1. **Explicit Allowlist:** The middleware maintains two allowlists:
|
||||||
|
- `_EXACT_ALLOWED`: Exact paths that bypass the guard (e.g., `/api/setup`, `/api/health`, `/api/docs`)
|
||||||
|
- `_PREFIX_ALLOWED`: Route prefixes that bypass the guard (e.g., `/api/setup/` for nested routes like `/api/setup/timezone`)
|
||||||
|
|
||||||
|
2. **Path Matching Strategy:** The middleware uses **exact matching for exact paths** and **prefix matching with trailing slashes for nested routes**. This prevents fragile prefix-based allowlists (e.g., using `startswith("/api/setup")` would accidentally allow `/api/setup-debug`).
|
||||||
|
|
||||||
|
3. **When Setup is Complete:** Once setup completes, the middleware becomes a no-op and all routes are accessible normally.
|
||||||
|
|
||||||
|
**Allowlisted Paths:**
|
||||||
|
- `/api/setup` — Setup status check and initialization endpoint
|
||||||
|
- `/api/setup/timezone` — Timezone configuration (reaches via `/api/setup/` prefix)
|
||||||
|
- `/api/health` — Health check endpoint (used by monitoring and load balancers)
|
||||||
|
- `/api/docs` — Swagger UI documentation
|
||||||
|
- `/api/redoc` — ReDoc documentation
|
||||||
|
- `/api/openapi.json` — OpenAPI schema (required by docs frontends)
|
||||||
|
|
||||||
|
**Adding New Setup Routes:**
|
||||||
|
|
||||||
|
When adding new routes to the setup flow:
|
||||||
|
1. If the route is an exact path (e.g., `/api/setup/validate`), add it to `_EXACT_ALLOWED`
|
||||||
|
2. If the route is nested under `/api/setup/` (e.g., `/api/setup/validate/config`), ensure `/api/setup/` is in `_PREFIX_ALLOWED` (it already is)
|
||||||
|
3. Never use prefix matching without a trailing slash — it leads to security issues with future route additions
|
||||||
|
|
||||||
|
**Implementation Location:**
|
||||||
|
- Middleware: `backend/app/main.py` — `SetupRedirectMiddleware` class
|
||||||
|
- Configuration: Lines 584–601 in `backend/app/main.py` — `_EXACT_ALLOWED` and `_PREFIX_ALLOWED` constants
|
||||||
|
- Guard logic: Lines 638–648 in `backend/app/main.py` — `dispatch()` method
|
||||||
|
|
||||||
|
**Example:**
|
||||||
|
```python
|
||||||
|
# If setup is incomplete:
|
||||||
|
GET /api/jails
|
||||||
|
→ 307 Temporary Redirect to /api/setup
|
||||||
|
|
||||||
|
# Allowlisted paths are always accessible:
|
||||||
|
GET /api/setup → 200 OK (setup status)
|
||||||
|
POST /api/setup → 201 Created (run setup)
|
||||||
|
GET /api/setup/timezone → 200 OK (get timezone)
|
||||||
|
GET /api/health → 200 OK (health check)
|
||||||
|
GET /api/docs → 200 OK (documentation)
|
||||||
|
|
||||||
|
# If setup is complete, all routes are accessible:
|
||||||
|
GET /api/jails → 200 OK (jail list)
|
||||||
|
```
|
||||||
|
|
||||||
### fail2ban_start_command Configuration
|
### fail2ban_start_command Configuration
|
||||||
|
|
||||||
The `fail2ban_start_command` setting specifies the shell command used to start the fail2ban daemon during recovery operations (e.g., after a rollback).
|
The `fail2ban_start_command` setting specifies the shell command used to start the fail2ban daemon during recovery operations (e.g., after a rollback).
|
||||||
|
|||||||
@@ -1,23 +1,3 @@
|
|||||||
## 33) Trusted proxy configuration is hardcoded in auth router
|
|
||||||
- Where found:
|
|
||||||
- [backend/app/routers/auth.py](backend/app/routers/auth.py#L46)
|
|
||||||
- [backend/app/utils/client_ip.py](backend/app/utils/client_ip.py)
|
|
||||||
- Why this is needed:
|
|
||||||
- Incorrect client IP extraction can break per-IP rate limiting behind proxies.
|
|
||||||
- Goal:
|
|
||||||
- Move trusted proxies to validated runtime config.
|
|
||||||
- What to do:
|
|
||||||
- Add settings for trusted proxy IPs/CIDRs.
|
|
||||||
- Validate and use these in client IP extraction.
|
|
||||||
- Possible traps and issues:
|
|
||||||
- Over-trusting headers can enable spoofing.
|
|
||||||
- Docs changes needed:
|
|
||||||
- Add reverse-proxy deployment configuration section.
|
|
||||||
- Doc references:
|
|
||||||
- [Docs/Instructions.md](Docs/Instructions.md)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 34) Setup redirect allowlist uses broad prefix matching
|
## 34) Setup redirect allowlist uses broad prefix matching
|
||||||
- Where found:
|
- Where found:
|
||||||
- [backend/app/main.py](backend/app/main.py#L434)
|
- [backend/app/main.py](backend/app/main.py#L434)
|
||||||
|
|||||||
@@ -41,6 +41,7 @@ from app.exceptions import (
|
|||||||
ServiceUnavailableError,
|
ServiceUnavailableError,
|
||||||
)
|
)
|
||||||
from app.middleware.csrf import CsrfMiddleware
|
from app.middleware.csrf import CsrfMiddleware
|
||||||
|
from app.models.response import ErrorResponse
|
||||||
from app.routers import (
|
from app.routers import (
|
||||||
auth,
|
auth,
|
||||||
bans,
|
bans,
|
||||||
@@ -56,7 +57,6 @@ from app.routers import (
|
|||||||
setup,
|
setup,
|
||||||
)
|
)
|
||||||
from app.startup import startup_shared_resources
|
from app.startup import startup_shared_resources
|
||||||
from app.models.response import ErrorResponse
|
|
||||||
from app.utils.rate_limiter import RateLimiter
|
from app.utils.rate_limiter import RateLimiter
|
||||||
from app.utils.runtime_state import ApplicationState, RuntimeState
|
from app.utils.runtime_state import ApplicationState, RuntimeState
|
||||||
from app.utils.session_cache import InMemorySessionCache, NoOpSessionCache
|
from app.utils.session_cache import InMemorySessionCache, NoOpSessionCache
|
||||||
@@ -180,7 +180,7 @@ def _get_error_code(exc: Exception) -> str:
|
|||||||
"""
|
"""
|
||||||
if hasattr(exc, "error_code"):
|
if hasattr(exc, "error_code"):
|
||||||
return exc.error_code
|
return exc.error_code
|
||||||
|
|
||||||
exc_name = exc.__class__.__name__
|
exc_name = exc.__class__.__name__
|
||||||
import re
|
import re
|
||||||
snake_case = re.sub(r"(?<!^)(?=[A-Z])", "_", exc_name).lower()
|
snake_case = re.sub(r"(?<!^)(?=[A-Z])", "_", exc_name).lower()
|
||||||
@@ -546,7 +546,7 @@ async def _http_exception_handler(
|
|||||||
status_code=exc.status_code,
|
status_code=exc.status_code,
|
||||||
error=exc.detail,
|
error=exc.detail,
|
||||||
)
|
)
|
||||||
|
|
||||||
error_code_map = {
|
error_code_map = {
|
||||||
status.HTTP_400_BAD_REQUEST: "invalid_input",
|
status.HTTP_400_BAD_REQUEST: "invalid_input",
|
||||||
status.HTTP_401_UNAUTHORIZED: "authentication_required",
|
status.HTTP_401_UNAUTHORIZED: "authentication_required",
|
||||||
@@ -558,14 +558,14 @@ async def _http_exception_handler(
|
|||||||
status.HTTP_500_INTERNAL_SERVER_ERROR: "internal_error",
|
status.HTTP_500_INTERNAL_SERVER_ERROR: "internal_error",
|
||||||
status.HTTP_503_SERVICE_UNAVAILABLE: "service_unavailable",
|
status.HTTP_503_SERVICE_UNAVAILABLE: "service_unavailable",
|
||||||
}
|
}
|
||||||
|
|
||||||
error_code = error_code_map.get(exc.status_code, "internal_error")
|
error_code = error_code_map.get(exc.status_code, "internal_error")
|
||||||
error_response = ErrorResponse(
|
error_response = ErrorResponse(
|
||||||
code=error_code,
|
code=error_code,
|
||||||
detail=exc.detail,
|
detail=exc.detail,
|
||||||
metadata={},
|
metadata={},
|
||||||
)
|
)
|
||||||
|
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
status_code=exc.status_code,
|
status_code=exc.status_code,
|
||||||
content=error_response.model_dump(),
|
content=error_response.model_dump(),
|
||||||
@@ -577,18 +577,42 @@ async def _http_exception_handler(
|
|||||||
# Setup-redirect middleware
|
# Setup-redirect middleware
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
# Paths that are always reachable, even before setup is complete.
|
# Exact paths that are always reachable, even before setup is complete.
|
||||||
_ALWAYS_ALLOWED: frozenset[str] = frozenset(
|
# Using exact matching prevents fragile prefix-based allowlists. For example,
|
||||||
{"/api/setup", "/api/health", "/api/docs", "/api/redoc", "/api/openapi.json"},
|
# if we used startswith(), a future route like /api/setup-debug would bypass
|
||||||
|
# the guard without being explicitly allowed.
|
||||||
|
_EXACT_ALLOWED: frozenset[str] = frozenset(
|
||||||
|
{
|
||||||
|
"/api/setup", # GET/POST /api/setup
|
||||||
|
"/api/health", # Health check endpoint
|
||||||
|
"/api/docs", # Swagger UI
|
||||||
|
"/api/redoc", # ReDoc
|
||||||
|
"/api/openapi.json", # OpenAPI schema
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
# Prefix paths that are always reachable. These MUST end with "/" to prevent
|
||||||
|
# matching paths like "/api/setup-debug" while still matching nested routes
|
||||||
|
# like "/api/setup/timezone".
|
||||||
|
_PREFIX_ALLOWED: frozenset[str] = frozenset(
|
||||||
|
{
|
||||||
|
"/api/setup/", # Nested setup routes (e.g., /api/setup/timezone)
|
||||||
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
class SetupRedirectMiddleware(BaseHTTPMiddleware):
|
class SetupRedirectMiddleware(BaseHTTPMiddleware):
|
||||||
"""Redirect all API requests to ``/api/setup`` until setup is done.
|
"""Redirect all API requests to ``/api/setup`` until setup is done.
|
||||||
|
|
||||||
Once setup is complete this middleware is a no-op. Paths listed in
|
Once setup is complete this middleware is a no-op. Paths listed in
|
||||||
:data:`_ALWAYS_ALLOWED` are exempt so the setup endpoint itself is
|
:data:`_EXACT_ALLOWED` and :data:`_PREFIX_ALLOWED` are exempt so the
|
||||||
always reachable.
|
setup endpoint and dependencies (health, docs, openapi schema) are always
|
||||||
|
reachable.
|
||||||
|
|
||||||
|
This middleware uses explicit path matching rather than prefix-based rules
|
||||||
|
to prevent fragile allowlists. For example, using startswith() could
|
||||||
|
accidentally allow paths like /api/setup-debug that shouldn't bypass
|
||||||
|
the setup guard.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
async def dispatch(
|
async def dispatch(
|
||||||
@@ -606,12 +630,23 @@ class SetupRedirectMiddleware(BaseHTTPMiddleware):
|
|||||||
Either a ``307 Temporary Redirect`` to ``/api/setup`` or the
|
Either a ``307 Temporary Redirect`` to ``/api/setup`` or the
|
||||||
normal router response.
|
normal router response.
|
||||||
"""
|
"""
|
||||||
|
# Remove trailing slash for consistent path comparison.
|
||||||
|
# Note: request.url.path does not include query parameters, so those
|
||||||
|
# don't need special handling.
|
||||||
path: str = request.url.path.rstrip("/") or "/"
|
path: str = request.url.path.rstrip("/") or "/"
|
||||||
|
|
||||||
# Allow requests that don't need setup guard.
|
# Check if path is in the explicit allowlist (exact match).
|
||||||
if any(path.startswith(allowed) for allowed in _ALWAYS_ALLOWED):
|
if path in _EXACT_ALLOWED:
|
||||||
return await call_next(request)
|
return await call_next(request)
|
||||||
|
|
||||||
|
# Check if path matches any allowed prefix. Prefixes in _PREFIX_ALLOWED
|
||||||
|
# end with "/" to prevent accidental matches. For example:
|
||||||
|
# - "/api/setup/" matches "/api/setup/timezone" (prefix match)
|
||||||
|
# - "/api/setup/" does NOT match "/api/setup-debug" (exact prefix without /)
|
||||||
|
for prefix in _PREFIX_ALLOWED:
|
||||||
|
if path == prefix.rstrip("/") or path.startswith(prefix):
|
||||||
|
return await call_next(request)
|
||||||
|
|
||||||
# If setup is not complete, block all other API requests.
|
# If setup is not complete, block all other API requests.
|
||||||
# The setup completion state is resolved at startup and stored in
|
# The setup completion state is resolved at startup and stored in
|
||||||
# ``app.state.setup_complete_cached`` so this middleware does not
|
# ``app.state.setup_complete_cached`` so this middleware does not
|
||||||
|
|||||||
Reference in New Issue
Block a user