From 4ee4d4b8e2bd0757d45ac5786b7386c9947768c4 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Thu, 6 Aug 2026 19:59:34 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein/privacy):=20=D0=BD=D0=BE=D1=80?= =?UTF-8?q?=D0=BC=D0=B0=D0=BB=D0=B8=D0=B7=D0=B0=D1=86=D0=B8=D1=8F=20=D1=82?= =?UTF-8?q?=D0=B5=D0=BB=D0=B5=D1=84=D0=BE=D0=BD=D0=B0=20=D0=BA=20=D0=BA?= =?UTF-8?q?=D0=B0=D0=BD=D0=BE=D0=BD=D0=B8=D1=87=D0=B5=D1=81=D0=BA=D0=BE?= =?UTF-8?q?=D0=BC=D1=83=20=D0=A0=D0=A4-=D0=B2=D0=B8=D0=B4=D1=83=20=D0=BF?= =?UTF-8?q?=D1=80=D0=B8=20erasure=20(#2547)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up к прошлому фиксу (regexp_replace \D): чистое удаление форматирования не закрывало разрыв, который сам ревьюер привёл в примере -- "+7 999 123-45-67" и "89991234567" после digit-stripping дают РАЗНЫЕ строки (79991234567 vs 89991234567, différent на первой цифре) -- классическая для РФ путаница 8/+7 trunk-префикса. _ru_phone_norm_sql(expr) добавляет второй шаг: если после digit-stripping получилось РОВНО 11 цифр с ведущей '8' -- заменить её на '7'. Точное тождество для российской нумерации, не эвристика (обсуждали: усечение до "последних 10 цифр" риск-скориальнее -- склеивает номера разных стран, удаление чужих данных хуже неудаления своих). Оба вызова (_PHONE_COLUMN_NORM_SQL / _PHONE_PARAM_NORM_SQL) строят SQL-структуру из статичных фрагментов (имя колонки / CAST(:phone AS text)) -- ни один телефон не попадает в текст запроса напрямую. Живая проверка (throwaway Postgres 16 в docker): лид "89991234567" находится и удаляется по запросу "+7 999 123-45-67" -- ровно кейс из ревью. Встроенный counterfactual в самом тесте доказывает, что чистый digit-strip (прошлая версия фикса) для этой пары находит 0 строк. Negative control: номер, отличающийся одной значащей цифрой, НЕ удаляется (защита от ложного совпадения = удаления чужих данных). --- .../backend/app/services/data_erasure.py | 75 +++++++-- .../backend/tests/test_data_erasure.py | 146 ++++++++++++++++-- 2 files changed, 195 insertions(+), 26 deletions(-) diff --git a/tradein-mvp/backend/app/services/data_erasure.py b/tradein-mvp/backend/app/services/data_erasure.py index 28f2b098..05a84bba 100644 --- a/tradein-mvp/backend/app/services/data_erasure.py +++ b/tradein-mvp/backend/app/services/data_erasure.py @@ -16,11 +16,13 @@ WHO CAN BE IDENTIFIED, HONESTLY: * `estimate_ids` -- if they still have the link/PDF from their estimate (the UUID in the URL/QR-code IS their proof of "this is mine"). * `phone` -- if they left a contact-request lead with that phone. - Matched by NORMALIZED DIGITS ONLY (regexp_replace strips everything - but 0-9 on both sides), not an exact string: lead.py stores + Matched by CANONICAL RU DIGITS on both sides (see + `_ru_phone_norm_sql` below), not an exact string: lead.py stores `payload.phone` exactly as typed (no E.164 normalization, by - design), so the caller's "+7 999 123-45-67" must still find a row - saved as "89991234567" or any other formatting of the same digits. + design), so "+7 999 123-45-67", "8 (999) 123-45-67" and + "89991234567" must all find the same row. Covers ONLY the + RU 8-vs-7 trunk-prefix case (exact digit-count identity, no + heuristic truncation) -- see the helper's docstring for why. * `tg_chat_id` -- if they messaged @MERAsupport_bot directly (their own Telegram chat id -- not guessable/spoofable by a third party the way a name or IP would be). @@ -68,6 +70,40 @@ from sqlalchemy.orm import Session logger = logging.getLogger(__name__) +def _ru_phone_norm_sql(expr: str) -> str: + """SQL-фрагмент: нормализация телефона к каноническому РФ-виду (11 цифр, + ведущая '7'), для сравнения "разного форматирования одного и того же номера" + (deep-review 2026-08-06, MEDIUM + follow-up). + + Два шага: 1) убрать всё, кроме цифр; 2) если получилось РОВНО 11 цифр с + ведущей '8' -- заменить её на '7'. Это ТОЧНОЕ тождество для российской + нумерации (8 и +7 -- один и тот же trunk-префикс), не эвристика: длина + проверяется явно (=11), заменяется РОВНО одна ведущая цифра. Специально + НЕ "последние 10 цифр" -- усечение убрало бы риск ложных совпадений + неточно: оно склеивает номера РАЗНЫХ стран с теми же 10 хвостовыми + цифрами, а удаление ЧУЖИХ данных по erasure-запросу хуже, чем + неудаление своих. Номера другой длины/страны просто не совпадут ни на + этом шаге, ни дальше -- безопасный отказ, не false positive. + + `expr` -- ВСЕГДА статичный SQL-фрагмент (имя колонки или + `CAST(:bind AS type)`), НИКОГДА значение параметра: эта функция строит + структуру запроса из литералов, вызывающих её мест ровно два (см. + _PHONE_COLUMN_NORM_SQL / _PHONE_PARAM_NORM_SQL ниже) -- ни один телефон + не попадает в текст SQL напрямую, только через bind-параметр `:phone`. + """ + stripped = f"regexp_replace({expr}, '\\D', '', 'g')" + return ( + f"(CASE WHEN length({stripped}) = 11 AND left({stripped}, 1) = '8' " + f"THEN '7' || substring({stripped} FROM 2) ELSE {stripped} END)" + ) + + +# Предвычисленные один раз -- обе стороны сравнения телефона в erase_person_data +# (колонка trade_in_leads.phone / входной CAST(:phone AS text)). +_PHONE_COLUMN_NORM_SQL = _ru_phone_norm_sql("phone") +_PHONE_PARAM_NORM_SQL = _ru_phone_norm_sql("CAST(:phone AS text)") + + def erase_person_data( db: Session, *, @@ -125,30 +161,35 @@ def erase_person_data( # 2. Лиды -- пока estimate_id ещё живой FK (см. п.1), плюс отдельно по # телефону (лид мог быть оставлен без attach к оценке вовсе). # - # ⚠️ Телефон сравнивается по НОРМАЛИЗОВАННЫМ цифрам, не литералом - # (deep-review 2026-08-06, MEDIUM). app/api/v1/lead.py сохраняет - # payload.phone КАК ПРИСЛАЛИ (намеренно -- полная E.164-нормализация - # вне scope MVP, см. lead.py::_PHONE_PATTERN), т.е. одна и та же - # строка может лежать в БД как "+7 999 123-45-67" ИЛИ "89991234567" - # ИЛИ любой другой форматировкой той же маски. Точное `phone = :phone` + # ⚠️ Телефон сравнивается по КАНОНИЧЕСКОМУ РФ-виду, не литералом + # (deep-review 2026-08-06, MEDIUM + follow-up). app/api/v1/lead.py + # сохраняет payload.phone КАК ПРИСЛАЛИ (намеренно -- полная + # E.164-нормализация вне scope MVP, см. lead.py::_PHONE_PATTERN), + # т.е. одна и та же строка может лежать в БД как "+7 999 123-45-67" + # ИЛИ "89991234567" ИЛИ "8 (999) 123-45-67". Точное `phone = :phone` # находит строку только если запрашивающий пришлёт БУКВАЛЬНО ТОТ ЖЕ # формат, каким когда-то ввёл номер -- почти никогда так. Раньше это # молча удаляло 0 строк и всё равно возвращало 200 "данные удалены": # для 152-ФЗ ложное подтверждение удаления хуже честной ошибки. - # `regexp_replace(x, '\\D', '', 'g')` с ОБЕИХ сторон сравнения снимает - # форматирование (пробелы/скобки/дефисы/+) и сравнивает голые цифры. - # Параметр -- CAST(:phone AS text), НЕ конкатенация (psycopg v3 / SQL - # injection convention, .claude/rules/backend.md). + # _PHONE_COLUMN_NORM_SQL / _PHONE_PARAM_NORM_SQL (см. _ru_phone_norm_sql + # выше) снимают форматирование С ОБЕИХ сторон И схлопывают ведущую + # '8' в '7' при 11 цифрах -- покрывает РОВНО RU 8-vs-7 trunk-префикс, + # без усечения до "последних 10 цифр" (риск ложного совпадения с + # номером другой страны -- см. докстринг helper'а). Номера иных + # форматов/длин сравниваются как есть (просто не совпадут). Параметр -- + # CAST(:phone AS text), НЕ конкатенация значения (psycopg v3 / SQL + # injection convention, .claude/rules/backend.md); сам SQL-текст + # собран из СТАТИЧНЫХ фрагментов (_PHONE_*_NORM_SQL), в которых нет + # ни одного значения параметра. ids_param = [str(i) for i in all_estimate_ids] result = db.execute( text( - """ + f""" DELETE FROM trade_in_leads WHERE estimate_id = ANY(CAST(:ids AS uuid[])) OR ( CAST(:phone AS text) IS NOT NULL - AND regexp_replace(phone, '\\D', '', 'g') - = regexp_replace(CAST(:phone AS text), '\\D', '', 'g') + AND {_PHONE_COLUMN_NORM_SQL} = {_PHONE_PARAM_NORM_SQL} ) """ ), diff --git a/tradein-mvp/backend/tests/test_data_erasure.py b/tradein-mvp/backend/tests/test_data_erasure.py index 9ca1471d..9d0e039c 100644 --- a/tradein-mvp/backend/tests/test_data_erasure.py +++ b/tradein-mvp/backend/tests/test_data_erasure.py @@ -139,25 +139,51 @@ def test_erase_by_phone_only_touches_only_leads() -> None: def test_phone_delete_normalizes_digits_on_both_sides() -> None: - """Regression guard for the deep-review MEDIUM finding (2026-08-06): - lead.py stores phone exactly as typed (no E.164 normalization, by - design), so a differently-formatted-but-same-number erasure request - ('+7 999 123-45-67' vs a stored '89991234567') must still match. The old - exact `phone = :phone` comparison silently deleted 0 rows and still + """Regression guard for the deep-review MEDIUM finding (2026-08-06) + + follow-up (RU 8-vs-7 trunk prefix): lead.py stores phone exactly as typed + (no E.164 normalization, by design), so a differently-formatted-but- + same-number erasure request must still match, AND the RU '8...' vs + '+7...' trunk-prefix pair must collapse to the same canonical value. The + old exact `phone = :phone` comparison silently deleted 0 rows and still returned HTTP 200 'erased' -- worse than an honest error under 152-ФЗ. - Both sides of the comparison must go through regexp_replace, and the - literal-equality path must be gone.""" + Both sides must go through the SAME normalization (_PHONE_COLUMN_NORM_SQL + / _PHONE_PARAM_NORM_SQL, see _ru_phone_norm_sql), and the literal-equality + path must be gone.""" db = MagicMock() db.execute.side_effect = [_Result(rowcount=1)] data_erasure.erase_person_data(db, phone="+7 999 123-45-67") sql = _sql_of(db.execute.call_args_list[0]) - assert sql.count("regexp_replace") == 2 - assert "phone = :phone" not in sql + # The comparison uses EXACTLY the two module-level normalized fragments + # (not a hand-rolled inline duplicate) -- pins that both sides go through + # the SAME normalization function, not two independently-drifting copies. + col_norm = data_erasure._PHONE_COLUMN_NORM_SQL + param_norm = data_erasure._PHONE_PARAM_NORM_SQL + assert f"{col_norm} = {param_norm}" in sql + # RU trunk-prefix collapse present on BOTH sides (11 digits, leading '8' -> '7'). + assert sql.count("length(regexp_replace") == 2 + assert sql.count("= '8'") == 2 + assert sql.count("'7' ||") == 2 + assert "phone = :phone" not in sql # old literal-equality path must be GONE assert not re.search(r":\w+::", sql) # psycopg v3 CAST trap +def test_ru_phone_norm_sql_only_ever_takes_static_expressions() -> None: + """`_ru_phone_norm_sql` is a query-STRUCTURE builder, not a data path -- + the two module-level constants are the ONLY call sites, and both pass a + column name / CAST(:bind AS type), never an actual phone value. This + pins that contract so a future call site can't accidentally splice a + real phone string into the SQL text.""" + assert data_erasure._PHONE_COLUMN_NORM_SQL == data_erasure._ru_phone_norm_sql("phone") + assert data_erasure._PHONE_PARAM_NORM_SQL == data_erasure._ru_phone_norm_sql( + "CAST(:phone AS text)" + ) + # column side references the column, never the bind param; param side is the reverse. + assert ":phone" not in data_erasure._PHONE_COLUMN_NORM_SQL + assert "CAST(:phone AS text)" in data_erasure._PHONE_PARAM_NORM_SQL + + def test_erase_by_tg_chat_id_only_touches_only_tg_support() -> None: """Anonymous person with NO username, NO estimate link, NO lead phone -- but they DID message @MERAsupport_bot -- can still be identified by their own @@ -248,3 +274,105 @@ def test_real_erase_by_phone_finds_differently_formatted_number() -> None: ) db.commit() db.close() + + +@pytest.mark.skipif(_live_session() is None, reason="no reachable Postgres test DB") +def test_real_erase_by_phone_finds_ru_trunk_prefix_variant() -> None: + """End-to-end on a real DB: the coordinator's exact follow-up gap + (2026-08-06) -- a lead stored as '89991234567' (leading '8') must be + found and deleted when the erasure requester supplies '+7 999 123-45-67' + (leading '+7'). Pure digit-stripping does NOT close this: stripped, the + two are '89991234567' vs '79991234567' -- different at digit 1. Only the + explicit 11-digit '8'->'7' collapse in _ru_phone_norm_sql makes them + equal. Counterfactual proven manually against this same DB (raw SQL, + see PR discussion): WITHOUT the collapse, `regexp_replace` alone finds 0 + rows for this exact pair.""" + from sqlalchemy import text as _t + + db = _live_session() + assert db is not None + lead_id: Any = None + try: + row = db.execute( + _t( + "INSERT INTO trade_in_leads (phone, consent, expires_at) " + "VALUES (:phone, TRUE, NOW() + interval '180 days') " + "RETURNING id" + ), + {"phone": "89991234567"}, + ).fetchone() + assert row is not None + lead_id = row[0] + db.commit() + + # Counterfactual: plain digit-stripping (the PRE-follow-up fix) does NOT + # match this pair -- proves the 8-vs-7 gap was real, not a strawman. + digits_only_match = db.execute( + _t( + "SELECT count(*) FROM trade_in_leads WHERE id = CAST(:id AS uuid) " + "AND regexp_replace(phone, '\\D', '', 'g') " + "= regexp_replace(CAST(:phone AS text), '\\D', '', 'g')" + ), + {"id": str(lead_id), "phone": "+7 999 123-45-67"}, + ).scalar() + assert digits_only_match == 0, "digit-stripping alone must NOT match 8- vs 7-prefix" + + out = data_erasure.erase_person_data(db, phone="+7 999 123-45-67") + + assert out["trade_in_leads_deleted"] == 1 + remaining = db.execute( + _t("SELECT count(*) FROM trade_in_leads WHERE id = CAST(:id AS uuid)"), + {"id": str(lead_id)}, + ).scalar() + assert remaining == 0 + finally: + if lead_id is not None: + db.execute( + _t("DELETE FROM trade_in_leads WHERE id = CAST(:id AS uuid)"), + {"id": str(lead_id)}, + ) + db.commit() + db.close() + + +@pytest.mark.skipif(_live_session() is None, reason="no reachable Postgres test DB") +def test_real_erase_by_phone_does_not_match_different_number() -> None: + """Negative control: a number differing in even ONE significant digit + must NOT be found -- proves the normalization is an exact-identity + check, not a fuzzy/truncated match that could delete a STRANGER's data. + Stored '89991234567' vs requested '+7 999 123-45-68' (last digit 7->8) + -- same length, same RU-looking shape, one digit off -- zero rows.""" + from sqlalchemy import text as _t + + db = _live_session() + assert db is not None + lead_id: Any = None + try: + row = db.execute( + _t( + "INSERT INTO trade_in_leads (phone, consent, expires_at) " + "VALUES (:phone, TRUE, NOW() + interval '180 days') " + "RETURNING id" + ), + {"phone": "89991234567"}, + ).fetchone() + assert row is not None + lead_id = row[0] + db.commit() + + out = data_erasure.erase_person_data(db, phone="+7 999 123-45-68") + + assert out["trade_in_leads_deleted"] == 0, "one differing digit must NOT match" + remaining = db.execute( + _t("SELECT count(*) FROM trade_in_leads WHERE id = CAST(:id AS uuid)"), + {"id": str(lead_id)}, + ).scalar() + assert remaining == 1, "row must survive an erasure request for a DIFFERENT number" + finally: + if lead_id is not None: + db.execute( + _t("DELETE FROM trade_in_leads WHERE id = CAST(:id AS uuid)"), + {"id": str(lead_id)}, + ) + db.commit() + db.close()