fix(nspd-geo): replace f-string bulk INSERT with parametrized execute (SQL injection fix) #123

Merged

lekss361 merged 1 commit from fix/nspd-geo-sql-injection into main

2026-05-14 20:29:12 +00:00

lekss361 commented

2026-05-14 20:22:33 +00:00

(Migrated from github.com)

Summary

Real SQL injection vector в backend/app/workers/tasks/nspd_geo.py:298-310 — per code review audit (May 14).

Bulk INSERT в enqueue_geo_job строил VALUES через f-string + chr(39)*2 quote escape:

# BEFORE (unsafe):
values = ",".join(
    f"({job_id}, '{c.replace(chr(39), chr(39)*2)}', {t}, 'pending')"
    for c, t in cad_nums_with_thematic
)
db.execute(text(f"INSERT ... VALUES {values} ..."))

Manual quote escape НЕ защищает от backslash escape / Unicode quote tricks. Exploitable если cad_num придёт из user input (e.g. через admin POST endpoint).

Fix

# AFTER (parametrized):
db.execute(
    text("INSERT INTO nspd_geo_targets (job_id, cad_num, thematic_id, status) "
         "VALUES (:job_id, :cad_num, :thematic_id, 'pending') "
         "ON CONFLICT (job_id, cad_num, thematic_id) DO NOTHING"),
    [{"job_id": job_id, "cad_num": c, "thematic_id": t}
     for c, t in cad_nums_with_thematic]
)

SQLAlchemy text + list of dicts → psycopg v3 handles batch executemany. Pattern consistent с другими INSERT'ами в этом файле.

Preserved

ON CONFLICT clause — verbatim
Return value, transaction/commit/close logic, function signature, caller interface — unchanged
DB schema — нет migration needed

Test plan

ruff check passed
CI зелёный
Manual smoke test: POST /admin/scrape/geo/jobs с любым cad_num — должен работать без SQL errors

Vault

fixes/Bug_Nspd_Geo_Sql_Injection_May14.md — created.

Closes audit critical #3.

## Summary **Real SQL injection vector** в `backend/app/workers/tasks/nspd_geo.py:298-310` — per code review audit (May 14). Bulk INSERT в `enqueue_geo_job` строил VALUES через f-string + `chr(39)*2` quote escape: ```python # BEFORE (unsafe): values = ",".join( f"({job_id}, '{c.replace(chr(39), chr(39)*2)}', {t}, 'pending')" for c, t in cad_nums_with_thematic ) db.execute(text(f"INSERT ... VALUES {values} ...")) ``` Manual quote escape НЕ защищает от backslash escape / Unicode quote tricks. Exploitable если `cad_num` придёт из user input (e.g. через admin POST endpoint). ## Fix ```python # AFTER (parametrized): db.execute( text("INSERT INTO nspd_geo_targets (job_id, cad_num, thematic_id, status) " "VALUES (:job_id, :cad_num, :thematic_id, 'pending') " "ON CONFLICT (job_id, cad_num, thematic_id) DO NOTHING"), [{"job_id": job_id, "cad_num": c, "thematic_id": t} for c, t in cad_nums_with_thematic] ) ``` SQLAlchemy `text` + list of dicts → psycopg v3 handles batch executemany. Pattern consistent с другими INSERT'ами в этом файле. ## Preserved - ON CONFLICT clause — verbatim - Return value, transaction/commit/close logic, function signature, caller interface — unchanged - DB schema — нет migration needed ## Test plan - [x] `ruff check` passed - [ ] CI зелёный - [ ] Manual smoke test: `POST /admin/scrape/geo/jobs` с любым cad_num — должен работать без SQL errors ## Vault `fixes/Bug_Nspd_Geo_Sql_Injection_May14.md` — created. Closes audit critical #3.

lekss361 commented

2026-05-14 20:25:08 +00:00

(Migrated from github.com)

Bot review — SECURITY FIX 🔒

Textbook-исправление SQL injection:

✅ f-string interpolation → SQLAlchemy text с bind params (:job_id, :cad_num, :thematic_id)
✅ Manual chr(39)*2 quote-escape removed — psycopg v3 handles escaping correctly
✅ List-of-dicts batch → driver executemany (perf parity с bulk INSERT VALUES)
✅ ON CONFLICT (job_id, cad_num, thematic_id) DO NOTHING сохранён verbatim
✅ Function signature, return value, transaction/commit/close logic — нетронуты
✅ Pattern consistent с другими INSERT'ами в этом же файле

Минор

chr(39) escape был неполной защитой — backslash-escape и Unicode-quote tricks могли бы обойти. Этот fix полностью решает класс уязвимости в этом codepath. Стоит для consistency проверить, нет ли других f-string INSERT'ов в backend/app/workers/tasks/ — отдельным PR.

CI

backend: 🔄
frontend: 🔄

Мерджу когда оба зелёные. High priority — security fix.

## Bot review — SECURITY FIX 🔒 Textbook-исправление SQL injection: ✅ f-string interpolation → SQLAlchemy `text` с bind params (`:job_id`, `:cad_num`, `:thematic_id`) ✅ Manual `chr(39)*2` quote-escape removed — psycopg v3 handles escaping correctly ✅ List-of-dicts batch → driver executemany (perf parity с bulk INSERT VALUES) ✅ `ON CONFLICT (job_id, cad_num, thematic_id) DO NOTHING` сохранён verbatim ✅ Function signature, return value, transaction/commit/close logic — нетронуты ✅ Pattern consistent с другими INSERT'ами в этом же файле ### Минор `chr(39)` escape был неполной защитой — backslash-escape и Unicode-quote tricks могли бы обойти. Этот fix полностью решает класс уязвимости в этом codepath. Стоит для consistency проверить, нет ли других f-string INSERT'ов в `backend/app/workers/tasks/` — отдельным PR. ### CI - backend: 🔄 - frontend: 🔄 Мерджу когда оба зелёные. **High priority** — security fix.

lekss361 commented

2026-05-14 20:27:50 +00:00

(Migrated from github.com)

Tracking: part of audit batch #127. Security: SQL injection fix. Vault: fixes/Bug_Nspd_Geo_Sql_Injection_May14.md.

Tracking: part of audit batch #127. Security: SQL injection fix. Vault: `fixes/Bug_Nspd_Geo_Sql_Injection_May14.md`.

lekss361 commented

2026-05-14 20:28:48 +00:00

(Migrated from github.com)