fixup(tradein): address deep-review BLOCK findings on FDW PR
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.
This commit is contained in:
lekss361
parent
698ef4f003
commit
6507a5af64
6 changed files with 140 additions and 35 deletions
|
|
@ -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 если файлы не менялись.
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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$$;
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue