From 600496048f835dbb4bce76c912f62b830f7e267c Mon Sep 17 00:00:00 2001 From: Dmitry Date: Sat, 19 Sep 2026 21:55:23 +0300 Subject: [PATCH] =?UTF-8?q?fix(api):=20=D0=B2=D1=81=D0=B5=20=D1=80=D0=BE?= =?UTF-8?q?=D1=83=D1=82=D0=B5=D1=80=D1=8B=20=D0=B7=D0=B0=20=D0=B0=D0=B2?= =?UTF-8?q?=D1=82=D0=BE=D1=80=D0=B8=D0=B7=D0=B0=D1=86=D0=B8=D0=B5=D0=B9,?= =?UTF-8?q?=20=D0=BE=D1=82=D0=BA=D0=B0=D0=B7=20=D0=BE=D1=82=20=D1=81=D0=BB?= =?UTF-8?q?=D0=B0=D0=B1=D0=BE=D0=B3=D0=BE=20JWT=5FSECRET,=20=D0=BB=D0=B8?= =?UTF-8?q?=D0=BC=D0=B8=D1=82=D0=B5=D1=80=20=D0=B8=20X-Forwarded-For?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_current_user навешивается на роутер целиком, публичными остаются только health и auth (список закреплён tests/api/test_route_auth.py). API не стартует с плейсхолдером или коротким JWT_SECRET без ALLOW_DEV_SECRET=1. Лимитер логинов ограничен по числу ключей. Клиентский IP берётся из последнего хопа X-Forwarded-For. --- .env.example | 1 + backend/src/fintracker/api/app.py | 54 +++++++++++++++---------- backend/src/fintracker/api/deps.py | 4 +- backend/src/fintracker/api/ratelimit.py | 20 +++++++-- backend/src/fintracker/config.py | 9 ++++- backend/tests/api/test_route_auth.py | 38 +++++++++++++++++ backend/tests/test_auth.py | 41 +++++++++++++++++++ justfile | 4 +- 8 files changed, 140 insertions(+), 31 deletions(-) create mode 100644 backend/tests/api/test_route_auth.py diff --git a/.env.example b/.env.example index 93acb79..496b19e 100644 --- a/.env.example +++ b/.env.example @@ -10,6 +10,7 @@ DATABASE_URL=postgresql+asyncpg://fintracker:change-me@db:5432/fintracker # --- auth --- # 32+ random bytes: `openssl rand -hex 32` JWT_SECRET=change-me +# the API refuses to start with a placeholder/short secret unless ALLOW_DEV_SECRET=1 (dev only) ACCESS_TOKEN_TTL_SECONDS=3600 REFRESH_TOKEN_TTL_SECONDS=2592000 diff --git a/backend/src/fintracker/api/app.py b/backend/src/fintracker/api/app.py index d28731f..a11dcc0 100644 --- a/backend/src/fintracker/api/app.py +++ b/backend/src/fintracker/api/app.py @@ -7,12 +7,13 @@ from collections.abc import AsyncIterator from contextlib import asynccontextmanager from typing import Any -from fastapi import FastAPI +from fastapi import Depends, FastAPI from fastapi.middleware.cors import CORSMiddleware from fastapi.openapi.utils import get_openapi from fastapi.routing import APIRoute from fintracker import __version__ +from fintracker.api.deps import get_current_user from fintracker.api.errors import install_error_handlers from fintracker.api.routers import ( accounts, @@ -44,6 +45,10 @@ log = logging.getLogger(__name__) API_PREFIX = "/api/v1" +_AUTHENTICATED = [Depends(get_current_user)] +"""Applied per router: a handler that forgets `CurrentUser` is still closed. Only `health` and +`auth` are mounted without it; `tests/api/test_route_auth.py` pins that list.""" + def _operation_id(route: APIRoute) -> str: # "auth_login" instead of "login_api_v1_auth_login_post": readable Dart method names @@ -55,7 +60,12 @@ def _operation_id(route: APIRoute) -> str: async def _lifespan(app: FastAPI) -> AsyncIterator[None]: settings = get_settings() if settings.is_dev_secret: - log.warning("JWT_SECRET is the insecure development default") + if not settings.allow_dev_secret: + raise RuntimeError( + "JWT_SECRET is missing, a placeholder or shorter than 32 characters; " + "set a real one (`openssl rand -hex 32`) or ALLOW_DEV_SECRET=1 for local dev" + ) + log.warning("JWT_SECRET is weak; allowed by ALLOW_DEV_SECRET") yield await reset_engine() @@ -83,29 +93,29 @@ def create_app() -> FastAPI: _install_openapi(app) app.include_router(health.router, prefix=API_PREFIX) app.include_router(auth.router, prefix=API_PREFIX) - app.include_router(sync.router, prefix=API_PREFIX) - app.include_router(accounts.router, prefix=API_PREFIX) - app.include_router(categories.router, prefix=API_PREFIX) - app.include_router(transactions.router, prefix=API_PREFIX) - app.include_router(rules.router, prefix=API_PREFIX) - app.include_router(networth.router, prefix=API_PREFIX) - app.include_router(cashflow.router, prefix=API_PREFIX) - app.include_router(analytics.router, prefix=API_PREFIX) - app.include_router(events.router, prefix=API_PREFIX) - app.include_router(imports.router, prefix=API_PREFIX) + app.include_router(sync.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(accounts.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(categories.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(transactions.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(rules.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(networth.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(cashflow.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(analytics.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(events.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(imports.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) # Before `instruments`: FastAPI matches in registration order, and `/instruments/{id}` # typed `int` does not fall through on a non-numeric segment — it answers 422. So # `/instruments/pending` has to be declared first or it becomes unreachable. - app.include_router(imports.pending_router, prefix=API_PREFIX) - app.include_router(instruments.router, prefix=API_PREFIX) - app.include_router(links.router, prefix=API_PREFIX) - app.include_router(metrics.router, prefix=API_PREFIX) - app.include_router(goals.router, prefix=API_PREFIX) - app.include_router(income.router, prefix=API_PREFIX) - app.include_router(rebalance.router, prefix=API_PREFIX) - app.include_router(tax.router, prefix=API_PREFIX) - app.include_router(benchmarks.router, prefix=API_PREFIX) - app.include_router(benchmarks.analytics_router, prefix=API_PREFIX) + app.include_router(imports.pending_router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(instruments.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(links.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(metrics.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(goals.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(income.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(rebalance.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(tax.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(benchmarks.router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) + app.include_router(benchmarks.analytics_router, prefix=API_PREFIX, dependencies=_AUTHENTICATED) if settings.web_dir is not None: mount_web(app, settings.web_dir, API_PREFIX) return app diff --git a/backend/src/fintracker/api/deps.py b/backend/src/fintracker/api/deps.py index 2568b0d..a318f55 100644 --- a/backend/src/fintracker/api/deps.py +++ b/backend/src/fintracker/api/deps.py @@ -45,7 +45,9 @@ CurrentUser = Annotated[AppUser, Depends(get_current_user)] def client_ip(request: Request) -> str: + # Caddy overwrites a client-supplied X-Forwarded-For with the real peer address; if a + # proxy ever appends instead, the last hop is the one our own proxy vouches for. fwd = request.headers.get("x-forwarded-for") if fwd: - return fwd.split(",")[0].strip() + return fwd.rsplit(",", 1)[-1].strip() return request.client.host if request.client else "unknown" diff --git a/backend/src/fintracker/api/ratelimit.py b/backend/src/fintracker/api/ratelimit.py index 0cd832e..87698a1 100644 --- a/backend/src/fintracker/api/ratelimit.py +++ b/backend/src/fintracker/api/ratelimit.py @@ -7,19 +7,24 @@ default, so this is the only brute-force protection — keep it. from __future__ import annotations import time -from collections import defaultdict, deque +from collections import deque class SlidingWindowLimiter: - def __init__(self, limit: int, window_seconds: float = 60.0) -> None: + def __init__(self, limit: int, window_seconds: float = 60.0, max_keys: int = 10_000) -> None: self.limit = limit self.window = window_seconds - self._hits: dict[str, deque[float]] = defaultdict(deque) + self.max_keys = max_keys + self._hits: dict[str, deque[float]] = {} def hit(self, key: str) -> float | None: """Register an attempt. Returns seconds to wait if over the limit, else None.""" now = time.monotonic() - q = self._hits[key] + q = self._hits.get(key) + if q is None: + if len(self._hits) >= self.max_keys: + self._evict(now) + q = self._hits[key] = deque() while q and now - q[0] > self.window: q.popleft() if len(q) >= self.limit: @@ -29,3 +34,10 @@ class SlidingWindowLimiter: def reset(self, key: str) -> None: self._hits.pop(key, None) + + def _evict(self, now: float) -> None: + """Make room for one more key: drop the expired ones, then the oldest if still full.""" + for k in [k for k, q in self._hits.items() if not q or now - q[-1] > self.window]: + del self._hits[k] + if len(self._hits) >= self.max_keys: + del self._hits[next(iter(self._hits))] # dicts keep insertion order diff --git a/backend/src/fintracker/config.py b/backend/src/fintracker/config.py index 25729d5..cb35f7e 100644 --- a/backend/src/fintracker/config.py +++ b/backend/src/fintracker/config.py @@ -27,14 +27,17 @@ class Settings(BaseSettings): jwt_secret: str = "dev-insecure-secret-change-me" access_token_ttl_seconds: int = 3600 refresh_token_ttl_seconds: int = 30 * 86400 + allow_dev_secret: bool = False + """Let the API start with a weak `JWT_SECRET` (local development only).""" login_rate_limit_per_minute: int = 5 + max_upload_bytes: int = 25 * 1024 * 1024 + """Ceiling for one broker report; Caddy enforces a slightly larger one in front.""" cors_origins: str = "" """Comma-separated origins for the Flutter web build when it is NOT served by Caddy from the same origin (e.g. `flutter run -d chrome` during development).""" timezone: str = "Europe/Moscow" - uploads_dir: Path = Path("/data/uploads") web_dir: Path | None = None """Optional Flutter web build to serve at `/` (dev convenience; Caddy does this in prod).""" log_level: str = "INFO" @@ -71,7 +74,9 @@ class Settings(BaseSettings): @property def is_dev_secret(self) -> bool: - return self.jwt_secret.startswith("dev-insecure") + """The built-in default, the `.env.example` placeholder, or a secret too short to sign.""" + s = self.jwt_secret + return s.startswith("dev-insecure") or "change-me" in s or len(s) < 32 @lru_cache diff --git a/backend/tests/api/test_route_auth.py b/backend/tests/api/test_route_auth.py new file mode 100644 index 0000000..29666d6 --- /dev/null +++ b/backend/tests/api/test_route_auth.py @@ -0,0 +1,38 @@ +"""Every route answers 401 without a token, except a short, explicit list of public ones.""" + +from __future__ import annotations + +import re + +from fintracker.api.app import API_PREFIX + +PUBLIC = { + ("GET", f"{API_PREFIX}/health"), + ("POST", f"{API_PREFIX}/auth/login"), + ("POST", f"{API_PREFIX}/auth/refresh"), + ("POST", f"{API_PREFIX}/auth/logout"), +} + + +def _routes(app) -> list[tuple[str, str]]: + """(METHOD, path) for every documented operation. Walks the OpenAPI schema rather than + `app.routes`: FastAPI nests included routers there in a private wrapper type.""" + return sorted( + (method.upper(), path) for path, item in app.openapi()["paths"].items() for method in item + ) + + +async def test_every_route_but_the_public_ones_requires_a_token(app, client): + routes = _routes(app) + assert len(routes) > 50 # guards against the walk silently matching nothing + assert set(routes) >= PUBLIC # a renamed public route must update this list, not vanish + + open_routes = [] + for method, path in routes: + if (method, path) in PUBLIC: + continue + url = re.sub(r"\{[^}]+\}", "1", path) + r = await client.request(method, url) + if r.status_code != 401: + open_routes.append(f"{method} {path} -> {r.status_code}") + assert not open_routes, "reachable without a token:\n" + "\n".join(open_routes) diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index ab02f77..3931bc5 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -1,3 +1,6 @@ +import pytest + + async def test_login_refresh_logout_cycle(client, user): r = await client.post("/api/v1/auth/login", json=user) assert r.status_code == 200, r.text @@ -41,3 +44,41 @@ async def test_protected_routes_need_token(client): assert r.status_code == 401 r = await client.get("/api/v1/auth/me", headers={"Authorization": "Bearer garbage"}) assert r.status_code == 401 + + +def test_client_ip_trusts_only_the_last_forwarded_hop(): + from starlette.requests import Request + + from fintracker.api.deps import client_ip + + def req(xff: str) -> Request: + return Request({"type": "http", "headers": [(b"x-forwarded-for", xff.encode())]}) + + assert client_ip(req("1.2.3.4")) == "1.2.3.4" + assert client_ip(req("6.6.6.6, 1.2.3.4")) == "1.2.3.4" + + +def test_limiter_evicts_instead_of_growing_without_bound(): + from fintracker.api.ratelimit import SlidingWindowLimiter + + limiter = SlidingWindowLimiter(limit=1, max_keys=3) + for i in range(10): + assert limiter.hit(f"ip-{i}") is None + assert len(limiter._hits) <= 3 + assert limiter.hit("ip-9") is not None # the freshest key keeps its state + + +async def test_weak_jwt_secret_refuses_to_start(monkeypatch): + from fintracker.api.app import _lifespan + from fintracker.config import get_settings + + monkeypatch.setenv("JWT_SECRET", "change-me") + monkeypatch.delenv("ALLOW_DEV_SECRET", raising=False) + get_settings.cache_clear() + try: + with pytest.raises(RuntimeError, match="JWT_SECRET"): + async with _lifespan(None): # type: ignore[arg-type] + pass + finally: + monkeypatch.undo() + get_settings.cache_clear() diff --git a/justfile b/justfile index 6e4a91d..186e8f3 100644 --- a/justfile +++ b/justfile @@ -14,11 +14,11 @@ sync: # run API with reload against the local dev DB api: - cd {{backend}} && DATABASE_URL={{db_url}} uv run fintracker serve --reload + cd {{backend}} && DATABASE_URL={{db_url}} ALLOW_DEV_SECRET=1 uv run fintracker serve --reload # API + Flutter web build from one origin (run `just build-web` first) web: - cd {{backend}} && DATABASE_URL={{db_url}} WEB_DIR=../app/build/web uv run fintracker serve --port 8000 + cd {{backend}} && DATABASE_URL={{db_url}} ALLOW_DEV_SECRET=1 WEB_DIR=../app/build/web uv run fintracker serve --port 8000 # --- flutter ---