diff --git a/docs/reviews/SPEC-REVIEW-43-r3.md b/docs/reviews/SPEC-REVIEW-43-r3.md new file mode 100644 index 00000000..24a25a5c --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-43-r3.md @@ -0,0 +1,205 @@ +# SPEC-REVIEW-43-r3 + +- Issue: #43 «Диалог помощи и обратной связи с обезличенным support report» +- Этап: spec (PROCESS.md §2.4) +- Заход: r3 · блокирующих циклов израсходовано 2 из 4 (после этого раунда) +- Материал: `docs/specs/043-private-support-report.md` на HEAD `c43bf847` + (ветка `issue/43-help-feedback`), плюс код `scripts/support-relay/**` как + доказательство исполнимости заявленных в ТЗ §9 контрактов. +- Предыдущий раунд: r2, вердикт **зелёный**, SHA `e25aa302` (High 0, Medium 0); + документ `docs/reviews/SPEC-REVIEW-43-r2.md`, опубликован коммитом `cf7f47db`. + +## Почему разбор не сведён к «только дельта» + +Между r2 и r3 в issue появилась реальная production-инфраструктура: relay +поднят на боевом узле, владелец лично проверил rate limiting и попытку подмены +источника через `X-Forwarded-For`, обнаружил, что прямой Telegram недоступен с +хостинга проекта, и по итогу сменил канал доставки «последней мили» на приватный +вебхук собственного Home Assistant. Это смена контракта в подсистеме, которую +r1/r2 уже проверяли (§9 relay), а не локальная правка формулировки — поэтому +разбор §9 и связанных разделов (§14.3/§14.4/§16/§17) сделан заново целиком, а не +только «что изменилось в тексте». + +## Дельта r2→r3 + +`git diff e25aa302..HEAD`: + +- `cf7f47db` — публикация `docs/reviews/SPEC-REVIEW-43-r2.md` (только review-артефакт, не часть ТЗ); +- `00102819 feat(relay): receive support reports on the project stand` — первая реализация `scripts/support-relay/**` (класс B), 1593 строки; +- `6b10cd52 docs: switch the support sink to a maintainer channel` — правка §5.1/§9.1/§9.3/§12/AC12/§14.3/§16/§17/§18/§19/§20 ТЗ: mailbox → приватный канал мейнтейнера; +- `c7efcf16 fix(relay): pin the rate-limit source to the proxy-supplied address` — код и тест против подмены источника лимита через `X-Forwarded-For`, найденной живым пентестом на стенде; **ТЗ этим коммитом не тронуто**; +- `c43bf847 feat(relay): deliver through the maintainer's Home Assistant webhook` — второй канал доставки `ha_webhook`, правка §9.1/§9.3 ТЗ. + +Все четыре продуктовых коммита несут `Issue: #43` и `User-Visible: no` — +консистентно с тем, что видимое поведение карточки (`src/**`) ещё не начато, +только class-B relay. + +## Закрытие раунда r2 + +r2 закрылся с 0 находок (High 0, Medium 0) — закрывать в r3 нечего, таблица +пустая. Единственное, что произошло после r2, — новая работа поверх зелёного +вердикта, а не исправление старых замечаний. + +## Унаследовано из r2 (и из r1 через r2) + +Без повторной проверки приняты — как есть в `docs/reviews/SPEC-REVIEW-43-r2.md` +на SHA `e25aa302` — потому что дельта r2→r3 не касается доказательной базы +этих пунктов: + +- §1–§6 (сценарий, UX-контракт кнопки/диалога/формы/preview/submit) — текст не + менялся в дельте; +- §7 (allowlist/pseudonymization/privacy invariant support package v1) — не + менялся; +- §8 (backend API preview/discard/submit) — не менялся; +- §10 (state/compatibility), §11 (touch/a11y — включая каноническую фразу + `Touch editor: supported`, закрытую в r1→r2), §13 AC1–AC11, AC13–AC17 + (кроме евиденс-колонки AC12, см. находку ниже); +- классификация `scripts/support-relay/**` как class B по `AGENTS.md`/ + `PROCESS.md` (закрытая в r1→r2 находка) — подтверждена и в r3 живым прогоном + `process-gate.mjs` на новом HEAD (см. «Гейты»), т.к. дерево `scripts/` + выросло, но regex-классификация в `process-gate.mjs` не менялась. + +## Находки + +### Medium (в скоупе #43, чинится этим же ТЗ, без нового цикла к владельцу — вопрос чисто технический) + +**Требование к источнику rate-limit не зафиксировано в ТЗ, хотя код и тесты +уже реализуют его правильно.** + +§9.2 ТЗ (не тронут дельтой) по-прежнему говорит только: + +> 5 attempts/hour and 20/day per source address plus a global circuit breaker; +> source IP is used only through a daily-keyed rate-limit hash with ≤24 h TTL + +Это не говорит, ОТКУДА берётся «source address», хотя корректность всей +анти-abuse истории публичного эндпоинта (§16, риск 3 «Public relay attracts +spam») зависит именно от этого. На реальном стенде это оказалось не +теоретическим риском: согласно комментарию владельца от 2026-09-01 21:25, +подмена `X-Forwarded-For` почти позволила обойти лимит одной строкой запроса — +спасло только совпадение с дефолтом чужого Caddy-конфига. Исправление внесено +кодом (`c7efcf16`) и покрыто тестом, но **в ТЗ это исправление не попало**: + +- `docs/specs/043-private-support-report.md` §9.2 не требует, что источник + берётся из доверенного hop-а прокси, а не из клиентского заголовка; +- AC12 (§13) по-прежнему говорит только «Relay enforces schema, size, hash, + idempotency and rate limits» — не называет спуфинг-стойкость как часть + контракта; +- §14.3 (test plan) и особенно §14.4 (mutation requirements) — единственный + раздел ТЗ, который явно перечисляет КАЖДУЮ обязательную мутацию для security- + инвариантов (checkbox default, redirect/SSRF, forbidden field leak и т.д.) — + не содержит пункта «клиент подделывает X-Forwarded-For и выбирает себе новую + корзину лимита → тест красный», хотя именно такой тест уже существует в коде + (`test_client_cannot_pick_its_own_rate_bucket`, + `scripts/support-relay/tests/test_relay.py:454-470`) и хотя именно этот + сценарий уже был проверен на боевом трафике. + +Реализация де-факто правильная (проверено чтением и исполнением, см. «Гейты»): +`hp_relay/app.py:147-158` берёт последний элемент `X-Forwarded-For` только при +`cfg.trusted_proxy` (default `HP_RELAY_TRUSTED_PROXY=1`, +`hp_relay/config.py:41,82`), а `deploy/Caddyfile.fragment:12-14` полностью +перезаписывает заголовок на обоих сайтах (`header_up X-Forwarded-For +{remote_host}`), а не дополняет его. Но эта перепись — рассыпанные по трём +файлам факты (`app.py`, `Caddyfile.fragment`, `env.example`), а не пункт +контракта ТЗ. Проблема не «оно не работает сейчас», а «ничто в ТЗ не обязывает +это работать после следующего рефакторинга»: спека — это то, с чем сверяется +будущий код-ревью и будущий переписчик `hp_relay/app.py`, а не README +деплоя. + +**Чем закрыть, не открывая вопрос владельцу** (технический пункт, решается +автором ТЗ по §7.1, не владельцем): + +1. в §9.2 добавить явное предложение: источник для rate-limit — последний + элемент `X-Forwarded-For`, только если relay сконфигурирован работать за + доверенным прокси (`trusted_proxy`), который обязан перезаписывать этот + заголовок целиком, а не дополнять; без этой конфигурации — адрес + TCP-соединения; +2. AC12 — добавить пункт: «источник для лимита не может быть выбран клиентом + через заголовки»; +3. §14.4 — добавить строку: «клиент посылает поддельный `X-Forwarded-For` → + получает собственную корзину лимита → тест красный». + +### Low (не блокирует, можно снять с записью) + +Продакшн-канал `ha_webhook` целиком зависит от одной автоматизации на личном +Home Assistant мейнтейнера («House Plan: приёмщик обратной связи → личка», +`scripts/support-relay/README.md:151-152`). Ротация вебхука прямо требует +правки этой автоматизации, но её конфигурация не задокументирована и не +сохранена нигде в репозитории — только упоминание, что она существует. Отказ +этого узла не теряет данные и не обманывает пользователя (отчёт остаётся в +спуле, клиент честно получает `support_unavailable` — проверено чтением +`hp_relay/app.py:110-116`), поэтому это не приватность/безопасность и не +блокирует зелёный; но при потере доступа к личному HA мейнтейнера +восстановление доставки требует пересоздания автоматизации «с нуля» без +письменной инструкции. Стоит одной строкой в `scripts/support-relay/README.md` +зафиксировать минимальную конфигурацию вебхука (принимает POST, поле `text`, +пересылает в Telegram), не дожидаясь следующего цикла. + +## Что проверено и признано корректным + +- **Два канала доставки (`telegram`/`ha_webhook`) в §9.1** описаны в ТЗ + консистентно с кодом: `hp_relay/delivery.py:141-203` (`TelegramDelivery`, + `HaWebhookDelivery`, `build()`) реализует ровно то, что описано — + вложение остаётся в спуле и не покидает узел на канале `ha_webhook` + (`hp_relay/delivery.py:168-188`, тест + `test_webhook_sends_text_and_keeps_the_package_on_the_node`). +- **«Запись до попытки доставки» (§9.1, «a failed delivery costs a promise, not + the user's request»)** — подтверждено чтением: `app.py:107-116` вызывает + `store.save()` до `delivery.send()`, а при неуспехе возвращает + `support_unavailable`, не удаляя отчёт. +- **Privacy-текст §9.3** («exact geometry ... transit the project relay and the + maintainer messenger») намеренно описывает худший случай для обоих каналов, + а не конкретно активный канал: фронтенд не знает, какой канал выбран на + деплое (это решение эксплуатации, §9.1 «chosen by deployment»), поэтому + общая (не заниженная) формулировка — осознанный выбор, а не забытая правка. + Проверено сопоставлением с `hp_relay/config.py:66-68` (канал — переменная + окружения, недоступная фронтенду). +- **§17 DoR** обновлён согласованно с новым каналом (пункт 2 — путь к секрету + вебхука как учётные данные), пункты 1/3/4/5 отражены в отдельных комментариях + владельца как реально выполненные на стенде — вне объёма самого текста ТЗ, + но не противоречат ему. +- **Лимиты §7.5/§9.5** (8 MiB / 8.5 MiB) совпадают с кодом: + `hp_relay/config.py:15-16` (`MAX_REQUEST_BYTES`, `MAX_ATTACHMENT_BYTES`). +- **Классификация class B** (`scripts/support-relay/**`) остаётся верной после + роста дерева — `process-gate.mjs` не меняла regex, живой прогон на HEAD это + подтверждает. +- **SCOPE.md** проверен на предмет открытых вопросов дельты: раздел «Where + users are» фиксирует уже существующий *публичный* support-чат + `t.me/ha_houseplan` как канал общей обратной связи — пивот к *приватному* + каналу мейнтейнера в r3 ему не противоречит и не подменяет его, они решают + разные задачи (публичный сигнал vs. приватный geometry-репорт). Собственно + вопрос «попадает ли фича в Core user jobs» дельтой r2→r3 не поднимается + (§1–§6 не менялись) и наследуется как принятый в r1/r2. + +## Чего не проверял и почему + +- `npm test` / `npm run build` (сверка трёх копий бандла) / `node + scripts/check-docs.mjs` — дельта не касается `src/**`, предмета проверки нет. +- `python -m pytest tests_backend -q` — дельта не касается + `custom_components/**` (relay — отдельный сервис вне HA-интеграции). +- `npm run invariants` / инварианты модели — дельта не касается геометрии, + `layout`, `marker.space`, `open_spans` или толщины стен. +- Браузерные смоки, `npm run golden:verify` — фронтенд-часть фичи (§1–§8) ещё + не реализована, `src/**` не тронут. +- Полный аудит `scripts/support-relay/hp_relay/{multipart,validate}.py` — эти + файлы не входят в дельту r2→r3 (не менялись коммитами `6b10cd52`/`c7efcf16`/ + `c43bf847` за пределами уже описанного), их корректность — предмет будущего + code-review по §13 AC5–AC12, а не этого spec-раунда. + +## Гейты, которые прогнал сам (зелёного Validate на `c43bf847` нет) + +- `node scripts/process-gate.mjs` → «гейт пройден, предупреждений 0» (диапазон + `origin/dev..HEAD`, 8 коммитов, офлайн). +- `npx tsc --noEmit` → 0 ошибок (дельта не трогает `src/**`, прогнан как + дешёвая проверка базовой линии). +- `python3 -m unittest discover -s scripts/support-relay/tests` → **35 + проверок, все зелёные**, включая `test_client_cannot_pick_its_own_rate_bucket` + и `test_webhook_sends_text_and_keeps_the_package_on_the_node`. +- Дисциплина «тест должен уметь падать» проверена для обоих названных выше + тестов лично, мутациями (изменения отменены после проверки, `git status` + чист): + - `forwarded.split(",")[-1]` → `[0]` (доверие первому, клиентскому элементу + XFF вместо последнего, проксёй-контролируемого) — тест + `test_client_cannot_pick_its_own_rate_bucket` упал (3 корзины вместо 1); + - `HaWebhookDelivery.send` начинает прикладывать байты вложения в JSON + вебхука — тест `test_webhook_sends_text_and_keeps_the_package_on_the_node` + упал (лишнее поле `attachment` в теле).