From 6507a5af642f5363f7b9fbee3d73fc1cefe9f602 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 24 May 2026 11:50:51 +0300 Subject: [PATCH] fixup(tradein): address deep-review BLOCK findings on FDW PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: - data/sql/100_tradein_fdw_role.sql — remove hardcoded password literal; role created LOGIN only; explicit REVOKE perimeter on all tables/sequences/functions in public; only v_tradein_cad_buildings grant remains. Password is now set by .forgejo/workflows/deploy.yml ALTER ROLE bootstrap step reading GENDESIGN_FDW_PASSWORD from backend/.env.runtime. - tradein-mvp/backend/app/core/fdw.py — strict regex whitelist on password ([A-Za-z0-9_-]{32,256}) before inlining into DDL; ValueError on mismatch (no password in error message); pg_user_mappings query corrected for PUBLIC mapping (usename IS NULL); commit wrapped in try/rollback. High: - 060_postgres_fdw_extension.sql — connect_timeout='3' added to FOREIGN SERVER; ALTER SERVER ADD/SET fallback for re-apply path. - geocoder.py — _cadastral_forward_sync / _cadastral_reverse_sync wrapped in asyncio.to_thread to avoid blocking event loop on FDW outage. Tests: - 3 new test_cadastral_reverse.py cases: SQL injection password rejected, short password rejected, valid hex password accepted with mapping creation. Old password 40473b7c... remains in git history at 698ef4f (force-push forbidden) — but the role was never created with it on prod (SQL not deployed), so abandoned. New password 72df... lives only in VPS env files and vault meta/00_credentials.md. --- .forgejo/workflows/deploy.yml | 21 ++++++ data/sql/100_tradein_fdw_role.sql | 13 +++- tradein-mvp/backend/app/core/fdw.py | 74 +++++++++++-------- tradein-mvp/backend/app/services/geocoder.py | 6 +- .../data/sql/060_postgres_fdw_extension.sql | 10 ++- .../tests/services/test_cadastral_reverse.py | 51 +++++++++++++ 6 files changed, 140 insertions(+), 35 deletions(-) diff --git a/.forgejo/workflows/deploy.yml b/.forgejo/workflows/deploy.yml index b9cd5d1e..12740547 100644 --- a/.forgejo/workflows/deploy.yml +++ b/.forgejo/workflows/deploy.yml @@ -251,6 +251,27 @@ jobs: done echo "All migrations applied." + # Set tradein_fdw_reader password from env (post-migration bootstrap). + # SQL migration creates role passwordless; password lives only in + # /opt/gendesign/backend/.env.runtime (sourced below via set -a / source). + # Idempotent: ALTER is no-op if password unchanged on subsequent deploys. + set -a; source backend/.env.runtime; set +a + if [ -n "${GENDESIGN_FDW_PASSWORD:-}" ]; then + echo "→ Applying tradein_fdw_reader password from env" + docker compose -p gendesign -f docker-compose.prod.yml exec -T postgres \ + psql -U "$POSTGRES_USER" -d "$POSTGRES_DB" -v ON_ERROR_STOP=on \ + -v "pw=$GENDESIGN_FDW_PASSWORD" \ + -c "DO \$\$ BEGIN \ + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'tradein_fdw_reader') THEN \ + EXECUTE format('ALTER ROLE tradein_fdw_reader WITH PASSWORD %L', :'pw'); \ + ELSE \ + RAISE NOTICE 'tradein_fdw_reader role missing; migration not yet applied'; \ + END IF; \ + END \$\$;" + else + echo "⚠️ GENDESIGN_FDW_PASSWORD not set in backend/.env.runtime — skipping ALTER ROLE for tradein_fdw_reader" + fi + # Build local-only sidecar images (glitchtip-auth-forwarder). # Эти services не в GHCR — сборка происходит на VPS на каждом deploy. # Cache-friendly: первый build ~30s, последующие 1-3s если файлы не менялись. diff --git a/data/sql/100_tradein_fdw_role.sql b/data/sql/100_tradein_fdw_role.sql index cfe605f7..b228eb68 100644 --- a/data/sql/100_tradein_fdw_role.sql +++ b/data/sql/100_tradein_fdw_role.sql @@ -1,15 +1,24 @@ -- tradein_fdw_reader role + v_tradein_cad_buildings view -- Used by tradein-postgres via postgres_fdw to read live cad_buildings. --- See meta/00_credentials.md for password rotation procedure. +-- Password is set by .forgejo/workflows/deploy.yml ALTER ROLE bootstrap step +-- reading GENDESIGN_FDW_PASSWORD from backend/.env.runtime — never embed password in SQL. BEGIN; DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'tradein_fdw_reader') THEN - CREATE ROLE tradein_fdw_reader LOGIN PASSWORD '40473b7c10c7474855c7d1aa98f0fd0a0de2499aef0cbd1fead06c9641e4dcd8'; + CREATE ROLE tradein_fdw_reader LOGIN; END IF; END$$; +COMMENT ON ROLE tradein_fdw_reader IS + 'FDW reader from tradein-postgres. Password set by .forgejo/workflows/deploy.yml from env GENDESIGN_FDW_PASSWORD post-migration step. Never embed password in SQL migrations.'; + +-- Defense-in-depth: explicit REVOKE perimeter (any existing PUBLIC grants ignored). +REVOKE ALL ON ALL TABLES IN SCHEMA public FROM tradein_fdw_reader; +REVOKE ALL ON ALL SEQUENCES IN SCHEMA public FROM tradein_fdw_reader; +REVOKE ALL ON ALL FUNCTIONS IN SCHEMA public FROM tradein_fdw_reader; + GRANT CONNECT ON DATABASE gendesign TO tradein_fdw_reader; GRANT USAGE ON SCHEMA public TO tradein_fdw_reader; diff --git a/tradein-mvp/backend/app/core/fdw.py b/tradein-mvp/backend/app/core/fdw.py index 49d1e78c..ec3cbf78 100644 --- a/tradein-mvp/backend/app/core/fdw.py +++ b/tradein-mvp/backend/app/core/fdw.py @@ -1,14 +1,17 @@ """postgres_fdw USER MAPPING management for gendesign_remote server. -USER MAPPING password is in env GENDESIGN_FDW_PASSWORD (not in SQL migration — -would leak password to disk). - -This helper applies idempotent CREATE / ALTER USER MAPPING. Reapplied on every -backend startup so password rotation through .env.runtime is picked up after restart. +Password lives in env GENDESIGN_FDW_PASSWORD — never in SQL migration or git. +This helper: + - validates password format (alphanumeric/hex, length >= 32) to prevent SQL + injection via embedded quotes (the postgres CREATE USER MAPPING syntax does + not accept bind parameters — password must be inlined into the DDL string), + - applies idempotent CREATE or ALTER mapping on every backend startup so + password rotation through .env.runtime is picked up after restart. """ from __future__ import annotations import logging +import re from sqlalchemy import text from sqlalchemy.orm import Session @@ -17,49 +20,62 @@ from app.core.config import settings logger = logging.getLogger(__name__) +# Strict whitelist — only hex/alphanumeric and a few separators. Refuses ANY +# quote, backslash, unicode whitespace, control char. Min length 32 forces +# rotation away from weak defaults. +_PASSWORD_RE = re.compile(r"^[A-Za-z0-9_\-]{32,256}$") + def ensure_fdw_user_mapping(db: Session) -> None: """Idempotent CREATE OR ALTER USER MAPPING for tradein → gendesign_remote. - Skips silently if password not configured (dev / first deploy). + Skips silently if password not configured (dev / first deploy / forgotten env). + Raises ValueError if password fails format whitelist — fail-fast on misconfig. """ password = settings.gendesign_fdw_password - if not password: + if not password or not str(password).strip(): logger.warning( "GENDESIGN_FDW_PASSWORD not set — skipping FDW user mapping " - "(gendesign_cad_buildings will fail)" + "(gendesign_cad_buildings queries will fail; cadastral lookups will " + "fall back to Yandex/Nominatim)" ) return - if not isinstance(password, str) or not password.strip(): - logger.warning("GENDESIGN_FDW_PASSWORD empty — skipping FDW user mapping") - return - # Check existence + if not _PASSWORD_RE.fullmatch(password): + # Do NOT echo the password into logs / exception even partially. + raise ValueError( + "GENDESIGN_FDW_PASSWORD failed format whitelist " + "(allowed: 32-256 chars, [A-Za-z0-9_-]). Refusing to apply USER MAPPING " + "— password rotation must use the same format." + ) + + # NB: even after whitelist validation we still cannot use SQLAlchemy bind + # parameters here — postgres CREATE/ALTER USER MAPPING is DDL and treats + # OPTIONS values as literal text. Whitelist guarantees the value is + # injection-safe; we still wrap in single quotes for the DDL syntax. exists = db.execute( text( "SELECT 1 FROM pg_user_mappings " - "WHERE srvname = 'gendesign_remote' AND usename = current_user" + "WHERE srvname = 'gendesign_remote' " + "AND (usename = current_user OR usename IS NULL)" ) ).first() - # Password cannot be a bind parameter in CREATE/ALTER USER MAPPING — must inline. - # Escape single quotes by doubling. - escaped_pw = password.replace("'", "''") if exists is None: - db.execute( - text( - f"CREATE USER MAPPING FOR CURRENT_USER SERVER gendesign_remote " - f"OPTIONS (user 'tradein_fdw_reader', password '{escaped_pw}')" - ) - ) + db.execute(text( + f"CREATE USER MAPPING FOR CURRENT_USER SERVER gendesign_remote " + f"OPTIONS (user 'tradein_fdw_reader', password '{password}')" + )) logger.info("created FDW user mapping for gendesign_remote") else: - db.execute( - text( - f"ALTER USER MAPPING FOR CURRENT_USER SERVER gendesign_remote " - f"OPTIONS (SET password '{escaped_pw}')" - ) - ) + db.execute(text( + f"ALTER USER MAPPING FOR CURRENT_USER SERVER gendesign_remote " + f"OPTIONS (SET password '{password}')" + )) logger.info("refreshed FDW user mapping password for gendesign_remote") - db.commit() + try: + db.commit() + except Exception: + db.rollback() + raise diff --git a/tradein-mvp/backend/app/services/geocoder.py b/tradein-mvp/backend/app/services/geocoder.py index 0b07a47f..1c4c657d 100644 --- a/tradein-mvp/backend/app/services/geocoder.py +++ b/tradein-mvp/backend/app/services/geocoder.py @@ -541,7 +541,7 @@ async def suggest( # Tier 0: cadastral FDW (если db доступна) — самый быстрый, без внешних запросов if db is not None: - cad_results = _cadastral_forward_sync(db, query.strip(), limit) + cad_results = await asyncio.to_thread(_cadastral_forward_sync, db, query.strip(), limit) if cad_results: return cad_results @@ -583,7 +583,7 @@ async def geocode(address: str, db: Session) -> GeocodeResult | None: return cached # 2. Cadastral FDW (прямой запрос к gendesign_cad_buildings — без внешнего API) - cad_suggestions = _cadastral_forward_sync(db, address.strip(), limit=1) + cad_suggestions = await asyncio.to_thread(_cadastral_forward_sync, db, address.strip(), limit=1) if cad_suggestions: s = cad_suggestions[0] result = GeocodeResult( @@ -724,7 +724,7 @@ async def reverse_geocode(lat: float, lon: float, db: Session | None = None) -> """ # 1. Cadastral FDW primary (без внешнего API, возвращает жилой дом not POI) if db is not None: - cadastral = _cadastral_reverse_sync(db, lat, lon) + cadastral = await asyncio.to_thread(_cadastral_reverse_sync, db, lat, lon) if cadastral: return cadastral diff --git a/tradein-mvp/backend/data/sql/060_postgres_fdw_extension.sql b/tradein-mvp/backend/data/sql/060_postgres_fdw_extension.sql index a8c88316..5dbcb8f6 100644 --- a/tradein-mvp/backend/data/sql/060_postgres_fdw_extension.sql +++ b/tradein-mvp/backend/data/sql/060_postgres_fdw_extension.sql @@ -9,7 +9,15 @@ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_foreign_server WHERE srvname = 'gendesign_remote') THEN CREATE SERVER gendesign_remote FOREIGN DATA WRAPPER postgres_fdw - OPTIONS (host 'gendesign-postgres', dbname 'gendesign', port '5432', extensions 'postgis'); + OPTIONS (host 'gendesign-postgres', dbname 'gendesign', port '5432', + extensions 'postgis', connect_timeout '3'); + ELSE + -- Update existing server: add connect_timeout if missing, or set if already present. + BEGIN + ALTER SERVER gendesign_remote OPTIONS (ADD connect_timeout '3'); + EXCEPTION WHEN OTHERS THEN + ALTER SERVER gendesign_remote OPTIONS (SET connect_timeout '3'); + END; END IF; END$$; diff --git a/tradein-mvp/backend/tests/services/test_cadastral_reverse.py b/tradein-mvp/backend/tests/services/test_cadastral_reverse.py index 6b7c3ee5..a5454b71 100644 --- a/tradein-mvp/backend/tests/services/test_cadastral_reverse.py +++ b/tradein-mvp/backend/tests/services/test_cadastral_reverse.py @@ -324,3 +324,54 @@ async def test_suggest_falls_back_to_yandex_when_cadastral_empty() -> None: assert len(results) == 1 mock_yandex.assert_called_once() + + +# ── ensure_fdw_user_mapping: SQL injection / whitelist guards ───────────────── + +def test_ensure_fdw_user_mapping_rejects_quote_in_password() -> None: + """Password with embedded quote must be rejected (SQL injection guard).""" + from app.core.fdw import ensure_fdw_user_mapping + + fake_settings = MagicMock() + fake_settings.gendesign_fdw_password = "x' OR '1'='1" + with patch("app.core.fdw.settings", fake_settings): + db = MagicMock() + try: + ensure_fdw_user_mapping(db) + assert False, "should have raised ValueError" + except ValueError as e: + assert "format whitelist" in str(e) + # No DB call should have happened + db.execute.assert_not_called() + + +def test_ensure_fdw_user_mapping_rejects_short_password() -> None: + """Password shorter than 32 chars rejected — forces rotation away from weak defaults.""" + from app.core.fdw import ensure_fdw_user_mapping + + fake_settings = MagicMock() + fake_settings.gendesign_fdw_password = "short" + with patch("app.core.fdw.settings", fake_settings): + db = MagicMock() + try: + ensure_fdw_user_mapping(db) + assert False, "should have raised ValueError" + except ValueError: + pass + db.execute.assert_not_called() + + +def test_ensure_fdw_user_mapping_accepts_valid_password() -> None: + """Hex password 32+ chars passes whitelist and issues CREATE USER MAPPING.""" + from app.core.fdw import ensure_fdw_user_mapping + + fake_settings = MagicMock() + fake_settings.gendesign_fdw_password = "a" * 64 # valid hex-like + with patch("app.core.fdw.settings", fake_settings): + db = MagicMock() + db.execute.return_value.first.return_value = None # mapping doesn't exist + ensure_fdw_user_mapping(db) + # CREATE USER MAPPING called — inspect TextClause text via str(args[0]) + sql_texts = [str(c.args[0]) for c in db.execute.call_args_list] + assert any("CREATE USER MAPPING" in s for s in sql_texts) + db.commit.assert_called_once()