v1.7.12: security hardening — CORS fix, security headers, fail-closed rate limiter, OpenAPI docs disabled by default, config auth privacy, webhook validation
This commit is contained in:
@@ -76,6 +76,10 @@ class Settings(BaseSettings):
|
||||
RATE_LIMIT_REQUESTS: int = 120
|
||||
RATE_LIMIT_WINDOW_SECONDS: int = 60
|
||||
|
||||
# Security / docs exposure
|
||||
DOCS_ENABLED: bool = False
|
||||
METRICS_ALLOWED_IPS: str = "127.0.0.1,::1,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
|
||||
|
||||
|
||||
_settings = Settings()
|
||||
|
||||
@@ -127,3 +131,6 @@ WEBHOOK_CLIENT_SECRET = _settings.WEBHOOK_CLIENT_SECRET
|
||||
RATE_LIMIT_ENABLED = _settings.RATE_LIMIT_ENABLED
|
||||
RATE_LIMIT_REQUESTS = _settings.RATE_LIMIT_REQUESTS
|
||||
RATE_LIMIT_WINDOW_SECONDS = _settings.RATE_LIMIT_WINDOW_SECONDS
|
||||
|
||||
DOCS_ENABLED = _settings.DOCS_ENABLED
|
||||
METRICS_ALLOWED_IPS = _settings.METRICS_ALLOWED_IPS
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import asyncio
|
||||
import ipaddress
|
||||
import logging
|
||||
import os
|
||||
import time
|
||||
@@ -7,7 +8,15 @@ from pathlib import Path
|
||||
|
||||
import structlog
|
||||
from audit_trail import log_action
|
||||
from config import AI_FEATURES_ENABLED, AUTH_ENABLED, CORS_ORIGINS, ENABLE_PERIODIC_FETCH, FETCH_INTERVAL_MINUTES
|
||||
from config import (
|
||||
AI_FEATURES_ENABLED,
|
||||
AUTH_ENABLED,
|
||||
CORS_ORIGINS,
|
||||
DOCS_ENABLED,
|
||||
ENABLE_PERIODIC_FETCH,
|
||||
FETCH_INTERVAL_MINUTES,
|
||||
METRICS_ALLOWED_IPS,
|
||||
)
|
||||
from database import setup_indexes
|
||||
from fastapi import FastAPI, HTTPException, Request
|
||||
from fastapi.middleware.cors import CORSMiddleware
|
||||
@@ -51,20 +60,28 @@ def configure_logging():
|
||||
configure_logging()
|
||||
logger = structlog.get_logger("aoc.fetcher")
|
||||
|
||||
app = FastAPI()
|
||||
# Disable OpenAPI docs in production by default
|
||||
app = FastAPI(
|
||||
docs_url="/docs" if DOCS_ENABLED else None,
|
||||
redoc_url="/redoc" if DOCS_ENABLED else None,
|
||||
openapi_url="/openapi.json" if DOCS_ENABLED else None,
|
||||
)
|
||||
|
||||
# CORS: warn if wildcard is used with auth enabled, but do not break deployments
|
||||
# CORS: when auth is enabled, never allow credentials with wildcard origins
|
||||
_effective_cors = CORS_ORIGINS
|
||||
_cors_credentials = True
|
||||
if AUTH_ENABLED and "*" in _effective_cors:
|
||||
logger.warning(
|
||||
"CORS wildcard (*) is insecure when AUTH_ENABLED=true. Set CORS_ORIGINS to your actual origin(s) in production."
|
||||
"CORS wildcard (*) is insecure with AUTH_ENABLED=true and allow_credentials. "
|
||||
"Disabling credentials. Set CORS_ORIGINS to your actual origin(s)."
|
||||
)
|
||||
_cors_credentials = False
|
||||
|
||||
app.add_middleware(CorrelationIdMiddleware)
|
||||
app.add_middleware(
|
||||
CORSMiddleware,
|
||||
allow_origins=_effective_cors,
|
||||
allow_credentials=True,
|
||||
allow_credentials=_cors_credentials,
|
||||
allow_methods=["*"],
|
||||
allow_headers=["*"],
|
||||
)
|
||||
@@ -81,7 +98,7 @@ async def prometheus_middleware(request: Request, call_next):
|
||||
|
||||
|
||||
@app.middleware("http")
|
||||
async def cache_control_middleware(request: Request, call_next):
|
||||
async def security_headers_middleware(request: Request, call_next):
|
||||
response = await call_next(request)
|
||||
# Prevent caching of HTML and API responses by default
|
||||
if request.url.path.startswith("/api/") or request.url.path in ("/", "/index.html"):
|
||||
@@ -100,6 +117,13 @@ async def cache_control_middleware(request: Request, call_next):
|
||||
"img-src 'self' data:; "
|
||||
"font-src 'self' data:;"
|
||||
)
|
||||
# Additional security headers
|
||||
response.headers["X-Content-Type-Options"] = "nosniff"
|
||||
response.headers["X-Frame-Options"] = "DENY"
|
||||
response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin"
|
||||
response.headers["Permissions-Policy"] = (
|
||||
"accelerometer=(), camera=(), geolocation=(), gyroscope=(), magnetometer=(), microphone=(), payment=(), usb=()"
|
||||
)
|
||||
return response
|
||||
|
||||
|
||||
@@ -165,8 +189,39 @@ async def health_check():
|
||||
raise HTTPException(status_code=503, detail="Database unavailable") from exc
|
||||
|
||||
|
||||
def _client_ip(request: Request) -> str:
|
||||
"""Best-effort client IP: X-Forwarded-For first hop, or direct client host."""
|
||||
forwarded = request.headers.get("x-forwarded-for")
|
||||
if forwarded:
|
||||
return forwarded.split(",")[0].strip()
|
||||
return request.client.host if request.client else ""
|
||||
|
||||
|
||||
def _is_metrics_allowed(ip: str) -> bool:
|
||||
"""Check if IP is in the configured metrics allowlist."""
|
||||
if not METRICS_ALLOWED_IPS:
|
||||
return True
|
||||
try:
|
||||
client_addr = ipaddress.ip_address(ip)
|
||||
except ValueError:
|
||||
return False
|
||||
for network in METRICS_ALLOWED_IPS.split(","):
|
||||
network = network.strip()
|
||||
if not network:
|
||||
continue
|
||||
try:
|
||||
if client_addr in ipaddress.ip_network(network, strict=False):
|
||||
return True
|
||||
except ValueError:
|
||||
continue
|
||||
return False
|
||||
|
||||
|
||||
@app.get("/metrics")
|
||||
async def metrics():
|
||||
async def metrics(request: Request):
|
||||
client_ip = _client_ip(request)
|
||||
if not _is_metrics_allowed(client_ip):
|
||||
raise HTTPException(status_code=403, detail="Forbidden")
|
||||
return Response(content=prometheus_metrics(), media_type="text/plain")
|
||||
|
||||
|
||||
|
||||
@@ -79,4 +79,5 @@ async def check_rate_limit(request: Request):
|
||||
except RateLimitExceeded:
|
||||
raise
|
||||
except Exception as exc:
|
||||
logger.warning("Rate limiter Redis error; allowing request", error=str(exc))
|
||||
logger.warning("Rate limiter Redis error; failing closed", error=str(exc))
|
||||
raise RateLimitExceeded(retry_after=60) from None
|
||||
|
||||
@@ -18,8 +18,8 @@ def auth_config():
|
||||
logger.debug("Auth config requested", auth_enabled=AUTH_ENABLED)
|
||||
return {
|
||||
"auth_enabled": AUTH_ENABLED,
|
||||
"tenant_id": AUTH_TENANT_ID,
|
||||
"client_id": AUTH_CLIENT_ID,
|
||||
"tenant_id": AUTH_TENANT_ID if AUTH_ENABLED else "",
|
||||
"client_id": AUTH_CLIENT_ID if AUTH_ENABLED else "",
|
||||
"scope": AUTH_SCOPE,
|
||||
"redirect_uri": None, # frontend uses window.location.origin by default
|
||||
}
|
||||
|
||||
@@ -17,7 +17,15 @@ async def graph_webhook(request: Request):
|
||||
if validation_token:
|
||||
# Microsoft sends validationToken as a query param during subscription creation.
|
||||
# Echo it back as plain text to prove endpoint ownership.
|
||||
return Response(content=validation_token, media_type="text/plain")
|
||||
# Validate to prevent content injection if endpoint is hit directly.
|
||||
if len(validation_token) > 1024 or not validation_token.isascii():
|
||||
logger.warning("Invalid validationToken rejected", length=len(validation_token))
|
||||
return Response(status_code=400)
|
||||
return Response(
|
||||
content=validation_token,
|
||||
media_type="text/plain",
|
||||
headers={"X-Content-Type-Options": "nosniff"},
|
||||
)
|
||||
|
||||
try:
|
||||
body = await request.json()
|
||||
|
||||
@@ -51,18 +51,32 @@ def client(mock_events_collection, mock_watermarks_collection, monkeypatch):
|
||||
|
||||
# Mock Redis so tests don't require a running Redis server
|
||||
class FakeRedis:
|
||||
_store = {}
|
||||
|
||||
async def get(self, key):
|
||||
return None
|
||||
return self._store.get(key)
|
||||
|
||||
async def setex(self, key, ttl, value):
|
||||
self._store[key] = value
|
||||
|
||||
async def incr(self, key):
|
||||
self._store[key] = self._store.get(key, 0) + 1
|
||||
return self._store[key]
|
||||
|
||||
async def expire(self, key, ttl):
|
||||
pass
|
||||
|
||||
async def fake_get_arq_pool():
|
||||
return FakeRedis()
|
||||
|
||||
async def fake_get_redis():
|
||||
return FakeRedis()
|
||||
|
||||
monkeypatch.setattr("redis_client.get_arq_pool", fake_get_arq_pool)
|
||||
monkeypatch.setattr("redis_client.get_redis", fake_get_redis)
|
||||
monkeypatch.setattr("routes.ask.get_arq_pool", fake_get_arq_pool)
|
||||
monkeypatch.setattr("routes.jobs.get_redis", fake_get_arq_pool)
|
||||
monkeypatch.setattr("routes.jobs.get_redis", fake_get_redis)
|
||||
monkeypatch.setattr("rate_limiter.get_redis", fake_get_redis)
|
||||
|
||||
from main import app
|
||||
|
||||
|
||||
@@ -268,7 +268,7 @@ def test_health(client):
|
||||
|
||||
|
||||
def test_metrics(client):
|
||||
response = client.get("/metrics")
|
||||
response = client.get("/metrics", headers={"X-Forwarded-For": "127.0.0.1"})
|
||||
assert response.status_code == 200
|
||||
assert "aoc_request_duration_seconds" in response.text
|
||||
|
||||
|
||||
Reference in New Issue
Block a user