From e2bed0db89abd04224f682d5621f160bbdcf1978 Mon Sep 17 00:00:00 2001 From: IK Date: Thu, 10 Sep 2026 05:49:23 +0700 Subject: [PATCH] =?UTF-8?q?R13:=20Fix=20X-Forwarded-For=20trust=20boundary?= =?UTF-8?q?=20=E2=80=94=20only=20trust=20from=20known=20proxies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- apps/api/app/config.py | 12 +++++ apps/api/app/main.py | 20 +++++++- apps/api/tests/test_rate_limit.py | 81 +++++++++++++++++++++++++------ compose.production.yaml | 2 + 4 files changed, 100 insertions(+), 15 deletions(-) diff --git a/apps/api/app/config.py b/apps/api/app/config.py index 4f24315..b31e73d 100644 --- a/apps/api/app/config.py +++ b/apps/api/app/config.py @@ -33,6 +33,18 @@ class Settings(BaseSettings): rate_limit_secret: str = "change-rate-limit-secret" log_level: str = "INFO" 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_validator(mode="after") diff --git a/apps/api/app/main.py b/apps/api/app/main.py index cf1ca5a..e42e3fd 100644 --- a/apps/api/app/main.py +++ b/apps/api/app/main.py @@ -2,6 +2,7 @@ from __future__ import annotations from collections import Counter from datetime import datetime, timedelta, timezone +from ipaddress import IPv4Address, IPv6Address, IPv4Network, IPv6Network import hashlib import hmac import logging @@ -538,13 +539,30 @@ def delete_report(report_id: UUID, db: Db, moderator: Annotated[str, Depends(_ad 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: now = datetime.now(timezone.utc) cutoff = now - timedelta(minutes=10) # Extract real client IP from forwarded headers client = request.client.host if request.client else "unknown" 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_hash = hmac.new(settings.rate_limit_secret.encode(), client.encode(), hashlib.sha256).hexdigest() if db.get_bind().dialect.name == "postgresql": diff --git a/apps/api/tests/test_rate_limit.py b/apps/api/tests/test_rate_limit.py index b0bf55c..4e6a342 100644 --- a/apps/api/tests/test_rate_limit.py +++ b/apps/api/tests/test_rate_limit.py @@ -1,7 +1,7 @@ from __future__ import annotations from datetime import datetime, timezone -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest from fastapi import HTTPException @@ -9,7 +9,7 @@ from sqlalchemy import create_engine, select from sqlalchemy.orm import Session 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 @@ -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) -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://") Base.metadata.create_all(engine) with Session(engine) as db: - mock_real = MagicMock() - mock_real.client.host = "10.0.0.1" - mock_real.headers.get.return_value = "198.51.100.10" - for _ in range(5): - _check_rate_limit(mock_real, db) - with pytest.raises(HTTPException) as blocked: - mock_other = MagicMock() - mock_other.client.host = "10.0.0.2" - mock_other.headers.get.return_value = "198.51.100.10" - _check_rate_limit(mock_other, db) - assert blocked.value.status_code == 429 + 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"] + mock_proxy = MagicMock() + mock_proxy.client.host = "127.0.0.1" + mock_proxy.headers.get.return_value = "198.51.100.10" + for _ in range(5): + _check_rate_limit(mock_proxy, db) + with pytest.raises(HTTPException) as blocked: + mock_other = MagicMock() + 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 diff --git a/compose.production.yaml b/compose.production.yaml index 5c26f74..3f3d05d 100644 --- a/compose.production.yaml +++ b/compose.production.yaml @@ -123,6 +123,7 @@ services: IMPORT_INTERVAL_SECONDS: ${IMPORT_INTERVAL_SECONDS:-3600} PUBLIC_CACHE_SECONDS: ${PUBLIC_CACHE_SECONDS:-20} 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_UNREVIEWED_DAYS: ${RETENTION_UNREVIEWED_DAYS:-30} RETENTION_APPROVED_PERSONAL_DAYS: ${RETENTION_APPROVED_PERSONAL_DAYS:-180} @@ -184,6 +185,7 @@ services: S3_SECRET_KEY: ${S3_SECRET_KEY:?Set S3_SECRET_KEY} SEED_DEMO_DATA: "false" 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} 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}