R13: Fix X-Forwarded-For trust boundary — only trust from known proxies
- Add _is_trusted_proxy() to check client IP against trusted CIDRs - Only use X-Forwarded-For if connection came from trusted proxy - Add TRUSTED_PROXY_CIDRS config (default: 127.0.0.1/32, ::1/128) - Add parse_comma_separated_lists for env var parsing - Add 3 unit tests: trusted CIDR check, untrusted ignores forwarded, trusted uses forwarded
This commit is contained in:
@@ -33,6 +33,18 @@ class Settings(BaseSettings):
|
|||||||
rate_limit_secret: str = "change-rate-limit-secret"
|
rate_limit_secret: str = "change-rate-limit-secret"
|
||||||
log_level: str = "INFO"
|
log_level: str = "INFO"
|
||||||
cors_origins: list[str] = Field(default_factory=lambda: ["http://localhost:4321", "http://127.0.0.1:4321"])
|
cors_origins: list[str] = Field(default_factory=lambda: ["http://localhost:4321", "http://127.0.0.1:4321"])
|
||||||
|
trusted_proxy_cidrs: list[str] = Field(default_factory=lambda: ["127.0.0.1/32", "::1/128"])
|
||||||
|
|
||||||
|
@model_validator(mode="before")
|
||||||
|
@classmethod
|
||||||
|
def parse_comma_separated_lists(cls, data: dict) -> dict:
|
||||||
|
"""Parse comma-separated string values into lists."""
|
||||||
|
if isinstance(data, dict):
|
||||||
|
for field_name in ["cors_origins", "trusted_proxy_cidrs"]:
|
||||||
|
value = data.get(field_name)
|
||||||
|
if isinstance(value, str) and value:
|
||||||
|
data[field_name] = [item.strip() for item in value.split(",") if item.strip()]
|
||||||
|
return data
|
||||||
model_config = SettingsConfigDict(env_file=".env", extra="ignore")
|
model_config = SettingsConfigDict(env_file=".env", extra="ignore")
|
||||||
|
|
||||||
@model_validator(mode="after")
|
@model_validator(mode="after")
|
||||||
|
|||||||
+19
-1
@@ -2,6 +2,7 @@ from __future__ import annotations
|
|||||||
|
|
||||||
from collections import Counter
|
from collections import Counter
|
||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
|
from ipaddress import IPv4Address, IPv6Address, IPv4Network, IPv6Network
|
||||||
import hashlib
|
import hashlib
|
||||||
import hmac
|
import hmac
|
||||||
import logging
|
import logging
|
||||||
@@ -538,13 +539,30 @@ def delete_report(report_id: UUID, db: Db, moderator: Annotated[str, Depends(_ad
|
|||||||
return Response(status_code=204)
|
return Response(status_code=204)
|
||||||
|
|
||||||
|
|
||||||
|
def _is_trusted_proxy(address: str, trusted_cidrs: list[str]) -> bool:
|
||||||
|
"""Check if address is in trusted proxy CIDRs."""
|
||||||
|
try:
|
||||||
|
addr = IPv4Address(address) if ":" not in address else IPv6Address(address)
|
||||||
|
except ValueError:
|
||||||
|
return False
|
||||||
|
for cidr in trusted_cidrs:
|
||||||
|
try:
|
||||||
|
network = IPv4Network(cidr) if ":" not in cidr else IPv6Network(cidr)
|
||||||
|
if addr in network:
|
||||||
|
return True
|
||||||
|
except ValueError:
|
||||||
|
continue
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
def _check_rate_limit(request: Request, db: Session) -> None:
|
def _check_rate_limit(request: Request, db: Session) -> None:
|
||||||
now = datetime.now(timezone.utc)
|
now = datetime.now(timezone.utc)
|
||||||
cutoff = now - timedelta(minutes=10)
|
cutoff = now - timedelta(minutes=10)
|
||||||
# Extract real client IP from forwarded headers
|
# Extract real client IP from forwarded headers
|
||||||
client = request.client.host if request.client else "unknown"
|
client = request.client.host if request.client else "unknown"
|
||||||
forwarded = request.headers.get("x-forwarded-for")
|
forwarded = request.headers.get("x-forwarded-for")
|
||||||
if forwarded:
|
# Only trust X-Forwarded-For if connection came from a trusted proxy
|
||||||
|
if forwarded and request.client and _is_trusted_proxy(request.client.host, settings.trusted_proxy_cidrs):
|
||||||
client = forwarded.split(",")[0].strip()
|
client = forwarded.split(",")[0].strip()
|
||||||
client_hash = hmac.new(settings.rate_limit_secret.encode(), client.encode(), hashlib.sha256).hexdigest()
|
client_hash = hmac.new(settings.rate_limit_secret.encode(), client.encode(), hashlib.sha256).hexdigest()
|
||||||
if db.get_bind().dialect.name == "postgresql":
|
if db.get_bind().dialect.name == "postgresql":
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from unittest.mock import MagicMock
|
from unittest.mock import MagicMock, patch
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from fastapi import HTTPException
|
from fastapi import HTTPException
|
||||||
@@ -9,7 +9,7 @@ from sqlalchemy import create_engine, select
|
|||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
from app.database import Base
|
from app.database import Base
|
||||||
from app.main import _check_rate_limit
|
from app.main import _check_rate_limit, _is_trusted_proxy
|
||||||
from app.models import SubmissionAttempt
|
from app.models import SubmissionAttempt
|
||||||
|
|
||||||
|
|
||||||
@@ -34,18 +34,71 @@ def test_rate_limit_is_persistent_and_does_not_store_raw_client() -> None:
|
|||||||
assert all(item.created_at.replace(tzinfo=timezone.utc) <= datetime.now(timezone.utc) for item in attempts)
|
assert all(item.created_at.replace(tzinfo=timezone.utc) <= datetime.now(timezone.utc) for item in attempts)
|
||||||
|
|
||||||
|
|
||||||
def test_rate_limit_uses_forwarded_for_header() -> None:
|
def test_rate_limit_uses_forwarded_for_header_from_trusted_proxy() -> None:
|
||||||
|
"""X-Forwarded-For should be used when client is trusted proxy."""
|
||||||
engine = create_engine("sqlite://")
|
engine = create_engine("sqlite://")
|
||||||
Base.metadata.create_all(engine)
|
Base.metadata.create_all(engine)
|
||||||
with Session(engine) as db:
|
with Session(engine) as db:
|
||||||
mock_real = MagicMock()
|
with patch("app.main.settings") as mock_settings:
|
||||||
mock_real.client.host = "10.0.0.1"
|
mock_settings.rate_limit_secret = "test-secret-for-testing"
|
||||||
mock_real.headers.get.return_value = "198.51.100.10"
|
mock_settings.trusted_proxy_cidrs = ["127.0.0.1/32"]
|
||||||
for _ in range(5):
|
mock_proxy = MagicMock()
|
||||||
_check_rate_limit(mock_real, db)
|
mock_proxy.client.host = "127.0.0.1"
|
||||||
with pytest.raises(HTTPException) as blocked:
|
mock_proxy.headers.get.return_value = "198.51.100.10"
|
||||||
mock_other = MagicMock()
|
for _ in range(5):
|
||||||
mock_other.client.host = "10.0.0.2"
|
_check_rate_limit(mock_proxy, db)
|
||||||
mock_other.headers.get.return_value = "198.51.100.10"
|
with pytest.raises(HTTPException) as blocked:
|
||||||
_check_rate_limit(mock_other, db)
|
mock_other = MagicMock()
|
||||||
assert blocked.value.status_code == 429
|
mock_other.client.host = "127.0.0.1"
|
||||||
|
mock_other.headers.get.return_value = "198.51.100.10"
|
||||||
|
_check_rate_limit(mock_other, db)
|
||||||
|
assert blocked.value.status_code == 429
|
||||||
|
|
||||||
|
|
||||||
|
def test_trusted_proxy_checks_cidrs() -> None:
|
||||||
|
assert _is_trusted_proxy("127.0.0.1", ["127.0.0.1/32"]) is True
|
||||||
|
assert _is_trusted_proxy("10.0.0.1", ["10.0.0.0/8"]) is True
|
||||||
|
assert _is_trusted_proxy("192.168.1.1", ["192.168.1.0/24"]) is True
|
||||||
|
assert _is_trusted_proxy("::1", ["::1/128"]) is True
|
||||||
|
assert _is_trusted_proxy("203.0.113.5", ["127.0.0.1/32"]) is False
|
||||||
|
assert _is_trusted_proxy("invalid", []) is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_rate_limit_ignores_forwarded_for_from_untrusted_client() -> None:
|
||||||
|
"""X-Forwarded-For should be ignored when client is not in trusted CIDRs."""
|
||||||
|
engine = create_engine("sqlite://")
|
||||||
|
Base.metadata.create_all(engine)
|
||||||
|
with Session(engine) as db:
|
||||||
|
# Client 203.0.113.42 is NOT trusted by default
|
||||||
|
mock_untrusted = MagicMock()
|
||||||
|
mock_untrusted.client.host = "203.0.113.42"
|
||||||
|
mock_untrusted.headers.get.return_value = "10.0.0.99"
|
||||||
|
for i in range(3):
|
||||||
|
_check_rate_limit(mock_untrusted, db)
|
||||||
|
# Should use real client 203.0.113.42, not forwarded 10.0.0.99
|
||||||
|
# So 3 attempts from 203.0.113.42 should be allowed (limit is 5)
|
||||||
|
mock_different = MagicMock()
|
||||||
|
mock_different.client.host = "203.0.113.42"
|
||||||
|
mock_different.headers.get.return_value = "10.0.0.88"
|
||||||
|
_check_rate_limit(mock_different, db) # Should succeed, not blocked
|
||||||
|
|
||||||
|
|
||||||
|
def test_rate_limit_uses_forwarded_for_from_trusted_proxy() -> None:
|
||||||
|
"""X-Forwarded-For should be used when client IS in trusted CIDRs."""
|
||||||
|
engine = create_engine("sqlite://")
|
||||||
|
Base.metadata.create_all(engine)
|
||||||
|
with Session(engine) as db:
|
||||||
|
# 127.0.0.1 IS trusted by default
|
||||||
|
with patch("app.main.settings") as mock_settings:
|
||||||
|
mock_settings.rate_limit_secret = "test-secret-for-testing"
|
||||||
|
mock_settings.trusted_proxy_cidrs = ["127.0.0.1/32", "::1/128"]
|
||||||
|
mock_proxy = MagicMock()
|
||||||
|
mock_proxy.client.host = "127.0.0.1"
|
||||||
|
mock_proxy.headers.get.return_value = "203.0.113.100"
|
||||||
|
for _ in range(5):
|
||||||
|
_check_rate_limit(mock_proxy, db)
|
||||||
|
# Should use forwarded IP 203.0.113.100, so a different forwarded IP should be allowed
|
||||||
|
mock_other_forwarded = MagicMock()
|
||||||
|
mock_other_forwarded.client.host = "127.0.0.1"
|
||||||
|
mock_other_forwarded.headers.get.return_value = "198.51.100.50"
|
||||||
|
_check_rate_limit(mock_other_forwarded, db) # Should succeed
|
||||||
|
|||||||
@@ -123,6 +123,7 @@ services:
|
|||||||
IMPORT_INTERVAL_SECONDS: ${IMPORT_INTERVAL_SECONDS:-3600}
|
IMPORT_INTERVAL_SECONDS: ${IMPORT_INTERVAL_SECONDS:-3600}
|
||||||
PUBLIC_CACHE_SECONDS: ${PUBLIC_CACHE_SECONDS:-20}
|
PUBLIC_CACHE_SECONDS: ${PUBLIC_CACHE_SECONDS:-20}
|
||||||
RATE_LIMIT_SECRET: ${RATE_LIMIT_SECRET:?Set RATE_LIMIT_SECRET}
|
RATE_LIMIT_SECRET: ${RATE_LIMIT_SECRET:?Set RATE_LIMIT_SECRET}
|
||||||
|
TRUSTED_PROXY_CIDRS: '${TRUSTED_PROXY_CIDRS:-"127.0.0.1/32,::1/128"}'
|
||||||
RETENTION_SUBMISSION_DAYS: ${RETENTION_SUBMISSION_DAYS:-1}
|
RETENTION_SUBMISSION_DAYS: ${RETENTION_SUBMISSION_DAYS:-1}
|
||||||
RETENTION_UNREVIEWED_DAYS: ${RETENTION_UNREVIEWED_DAYS:-30}
|
RETENTION_UNREVIEWED_DAYS: ${RETENTION_UNREVIEWED_DAYS:-30}
|
||||||
RETENTION_APPROVED_PERSONAL_DAYS: ${RETENTION_APPROVED_PERSONAL_DAYS:-180}
|
RETENTION_APPROVED_PERSONAL_DAYS: ${RETENTION_APPROVED_PERSONAL_DAYS:-180}
|
||||||
@@ -184,6 +185,7 @@ services:
|
|||||||
S3_SECRET_KEY: ${S3_SECRET_KEY:?Set S3_SECRET_KEY}
|
S3_SECRET_KEY: ${S3_SECRET_KEY:?Set S3_SECRET_KEY}
|
||||||
SEED_DEMO_DATA: "false"
|
SEED_DEMO_DATA: "false"
|
||||||
RATE_LIMIT_SECRET: ${RATE_LIMIT_SECRET:?Set RATE_LIMIT_SECRET}
|
RATE_LIMIT_SECRET: ${RATE_LIMIT_SECRET:?Set RATE_LIMIT_SECRET}
|
||||||
|
TRUSTED_PROXY_CIDRS: '${TRUSTED_PROXY_CIDRS:-"127.0.0.1/32,::1/128"}'
|
||||||
COMMUNITY_IMPORT_INTERVAL_SECONDS: ${COMMUNITY_IMPORT_INTERVAL_SECONDS:-1800}
|
COMMUNITY_IMPORT_INTERVAL_SECONDS: ${COMMUNITY_IMPORT_INTERVAL_SECONDS:-1800}
|
||||||
RF4MAP_POINT_URL: ${RF4MAP_POINT_URL:-https://rf4map.ru/points/275}
|
RF4MAP_POINT_URL: ${RF4MAP_POINT_URL:-https://rf4map.ru/points/275}
|
||||||
RF4POSTS_SPOT_URL: ${RF4POSTS_SPOT_URL:-https://rf4-posts.com/ru/spots/d0c6d9c6-4ebf-49a7-98a8-9a562553a8ee}
|
RF4POSTS_SPOT_URL: ${RF4POSTS_SPOT_URL:-https://rf4-posts.com/ru/spots/d0c6d9c6-4ebf-49a7-98a8-9a562553a8ee}
|
||||||
|
|||||||
Reference in New Issue
Block a user