diff --git a/docs/reviews/SPEC-REVIEW-225-r1.md b/docs/reviews/SPEC-REVIEW-225-r1.md new file mode 100644 index 00000000..362eb6ba --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-225-r1.md @@ -0,0 +1,253 @@ +# SPEC-REVIEW — issue #225, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/225 +- **ТЗ:** тело issue #225 (лёгкий трек `small`, файл в `docs/specs/` не + создаётся согласно §5 PROCESS.md) +- **Трек:** `small` — сложность 2/10, одна поверхность + (`custom_components/houseplan/import_export.py`), лимит циклов ревью ТЗ — 2 +- **Ревьюер:** Claude (роль «Ревьюер ТЗ», отдельная сессия от автора) +- **Вердикт:** жёлтый · цикл r1/2 · High: 0 · Medium: 1 → в задаче + +## Скоуп проверки + +Issue #225 — баг: собственный экспорт House Plan (PDF-вложения с +кэш-бастером `?v=` в URL) не проходит собственный импорт, +падая на `invalid_content`. ТЗ в теле issue (лёгкий трек) описывает фикс +`_internal_path` (`import_export.py:350`): разбирать URL через +`urlsplit`, игнорируя `query`/`fragment` при резолвинге пути к файлу. + +Проверялись: + +1. критерии лёгкого трека (§5 PROCESS.md) — действительно ли применимы; +2. соответствие описания бага фактическому состоянию кода + (`_internal_path`, `_looks_internal`, `_content_state`, + `content_manifest`, `sanitize_filename`, `_SAFE_NAME_RE`); +3. однозначность и доказуемость каждого AC1…AC7, включая перечисленные + в AC4 конкретные traversal-полезные нагрузки; +4. отсутствие догадок, выданных за факт, без пометки «предположение»; +5. корректность решения не эскалировать вопрос об error-message + владельцу (уже решено на этапе аналитики тем же лицом). + +Это первый цикл, раздел «объём по дельте» (§2.10 PROCESS.md) не +применяется — разбор выполнен полностью. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md` (J6 «Keep the plan true as the home + evolves» — перенос/восстановление конфигурации), `AGENTS.md`, + `PROCESS.md` целиком (§2.3–2.4, §5, §7.1, §7.2, §12). +- Прочитано тело issue #225 полностью и оба комментария (аналитика с + оценками и меткой `small`, публикация ТЗ в теле issue). Метка на + момент ревью — `S4-spec-review`. +- Прочитан `custom_components/houseplan/import_export.py`: + - `_internal_path` (350-370) — подтверждено: ветка `files/` режет + `url[len(prefix):]` по `/` **без** отделения query, `tail` включает + `?v=...` как часть последнего сегмента; + - `_looks_internal` (373-377) — подтверждено: смотрит только на + префикс, не видит query; + - `content_manifest` (380-405) — подтверждено: `storage: "internal" + if internal else "external"`, ровно как описано в ТЗ; + - `_content_state` (1021-1071) — подтверждено: строка 1060 + `if _looks_internal(item["url"]) and internal is None: raise + ImportFailure("invalid_content", ...)` — воспроизводит механизм + отказа дословно; `identity()` (1034-1039) не включает `storage` в + ключ сравнения — подтверждает риск ТЗ про сохранение совместимости + по AC6; + - `_validate_plan_only_document` (685-712), в частности строка 704 + (`placement_manifest`) — подтверждено, что классификация + `storage` не участвует в её проверках, только `owner == "space"` — + AC7 обоснован. +- Прочитан `custom_components/houseplan/validation.py`: + `_SAFE_NAME_RE = [^A-Za-z0-9._-]+`, `sanitize_filename`, + `sanitize_marker_id` — подтверждено, что `?`/`=` заменяются на `_`, + из-за чего сравнение `name == tail[-1]` не проходит на суффиксе + `?v=...`. +- Прочитан `custom_components/houseplan/http_api.py:290-316` — подтверждено, + что текущая ручка загрузки возвращает URL без query + (`f"{CONTENT_URL}/files/{marker_id}/{name}"`), то есть суффикс — + исторические данные, а не то, что генерирует текущий код. +- Прочитан `src/logic.ts:1841-1861` (`migratePdfUrls`) — подтверждено, + что фронт уже явно отделяет `query` от `name` при переносе вложений; + довод ТЗ «контракт на фронте уже есть, бэкенд не в курсе» подтверждён + чтением, а не принят на слово. +- Проверено существование прецедента для AC2/AC3 — + `tests_backend/test_ha_import_export.py:1439-1456` + (`test_apply_rechecks_attachment_under_the_write_lock`) использует + канонический URL без query; ТЗ верно называет его образцом для новой + фикстуры. +- Проверено существование теста на внешний URL для AC5 — + `tests_backend/test_ha_import_export.py:366,459` использует + `https://example.invalid/floor.svg` — существующий тест, изменения не + требует. +- Пройдена по шагам вычислением (без исполнения кода) каждая из четырёх + строк AC4 против описанного контракта (`urlsplit(url).path`, query и + fragment исключены из резолвинга) — см. находку M1 ниже. +- Гейты кода не запускались: предмет ревью — ТЗ, реализации ещё нет + (`S4-spec-review`), код не менялся. + +## Находки + +### Medium — в скоупе задачи + +**M1. Третий пример в AC4 требует поведения, которое прямо противоречит +описанному в этом же ТЗ контракту резолвинга — критерий недоказуем без +нарушения собственного контракта.** + +AC4 перечисляет четыре traversal-нагрузки и требует, чтобы все четыре +резолвились в `None`: + +> `…/files/../../secret.pdf?v=1`, `…/files/m1/../../secret.pdf`, +> `…/files/m1/doc.pdf?x=/../../etc`, `…/plans/_/../x.svg?v=1` + +Контракт поведения этого же ТЗ формулирует фикс так: «путь для +резолвинга берётся из `urlsplit(url).path`, `query` и `fragment` в +определении файла не участвуют». + +Проверка построчно: + +- пример 1 (`…/files/../../secret.pdf?v=1`): `path` после отсечения + query = `.../files/../../secret.pdf`; хвост после префикса — + `../../secret.pdf`, при разбиении по `/` даёт 3 сегмента → уже сейчас, + до всякого фикса, возвращает `None` (проверка `len(tail) != 2`, + `import_export.py:364`). Легитимный regression-тест, поведение верно + предсказано. +- пример 2 (`…/files/m1/../../secret.pdf`): хвост `m1/../../secret.pdf` + → 4 сегмента → `None`. Тоже легитимно и уже работает без фикса. +- пример 4 (`…/plans/_/../x.svg?v=1`): `path` без query = `.../plans/_/../x.svg`; + `raw_name` после префикса — `../x.svg`, содержит `/` → ветка `plans` + (`import_export.py:355`) уже сегодня возвращает `None` по проверке + `"/" in raw_name`. Легитимно. +- **пример 3 (`…/files/m1/doc.pdf?x=/../../etc`): по заявленному + контракту `query` не участвует в резолвинге пути. `urlsplit(url).path` + для этой строки — `.../files/m1/doc.pdf`, то есть чистый, без единой + точки `..`. Хвост после префикса — `m1/doc.pdf`, ровно 2 сегмента, + `marker="m1"`, `name="doc.pdf"` — оба проходят `sanitize_*` без + изменений. Корректная по контракту реализация обязана вернуть + `("attachment", root/FILES_DIR/"m1"/"doc.pdf")`, а не `None`.** + +Иначе говоря: содержимое `query` в этом примере физически не может +повлиять на путь к файлу — «`../../etc`» лежит внутри значения +параметра `x`, а не в пути. Требование «резолвится в `None`» либо +недостижимо без дополнительной, нигде в ТЗ не описанной проверки +(«отклонять `..` при обнаружении где угодно в исходной строке URL, а не +только в пути») — тогда это новый пункт контракта, а не следствие уже +заявленного; либо AC написан по ошибке и пример перепутан местами: +скорее всего он должен был доказывать *обратное* — что «мусорный» +traversal-паттерн в `query` безопасен именно потому, что резолвинг его +игнорирует, и годился бы как положительный кейс в AC1, а не как +негативный в AC4. + +**Почему это Medium, а не Low.** Это не стилистическая неточность: +разработчик, который напишет тест по этому пункту буквально, либо +получит красный тест на корректной реализации (совпадающей с +контрактом ТЗ), либо втихую ослабит заявленный контракт («`query` не +участвует»), добавив непрописанную проверку — оба исхода нежелательны, +и оба возникают из-за противоречия внутри одного документа. Именно +такую непроверяемость обязано ловить ревью ТЗ (§7.1 PROCESS.md: +«Ambiguity is asked, not guessed»). + +**Фикс, ожидаемый в этом же цикле:** одно из двух — либо убрать третий +пример из AC4 и перенести его (в виде «резолвится корректно, `query` с +`../../` внутри не создаёт traversal и не бьёт по AC1») в AC1 как ещё +один параметр, подтверждающий, что мусор в `query` безопасен именно +потому что игнорируется; либо, если автор действительно имел в виду +доп. defense-in-depth проверку по всей исходной строке — явно дописать +её в «Контракт поведения» и объяснить, зачем она нужна при том, что +`query` и так не участвует в резолвинге. Само по себе это правка на +две строки текста ТЗ, не код. + +### Low + +Не найдено находок, которые стоило бы фиксировать отдельно и не +чинить. (См. отдельно нижеследующее наблюдение по AC1 — оно снимается +ревьюером с записью, не заводится как Low-находка.) + +## Что проверено и корректно + +- Обязательные для лёгкого трека элементы шаблона (§5 PROCESS.md) + присутствуют и по существу: проблема (с воспроизведением и точными + номерами строк), контракт поведения, AC1…AC7 каждый с указанием + доказательства (`backend`), план автотестов, риски, откат. ТЗ также + добавляет сверх минимума сценарий/персону, «что человек увидит», + «не в скоупе», модель данных/миграция/i18n и блок явных технических + предположений — то есть избыточно полно для лёгкого трека, а не + урезано. +- Критерии лёгкого трека (§5) выполнены одновременно: сложность 2/10, + одна поверхность (`import_export.py`), нет миграции конфига и + compatibility-полей, нет нового UX-контракта (импорт собственного + бэкапа — уже описанное поведение, меняется только исход), нет влияния + на perf/touch. Трек выбран верно. +- Диагноз бага воспроизведён чтением кода, а не принят на слово автора + — см. «Как проверялось»: `_internal_path`/`_looks_internal` реально + расходятся именно так, как описано, `_SAFE_NAME_RE` реально ломает + сравнение на `?`/`=`. +- AC1, AC2, AC5, AC6, AC7 однозначны и доказуемы: у каждого понятный + вход/выход и существующая или явно описанная новая точка проверки. + AC3 корректно указывает на существующий прецедент фикстуры + (`test_apply_rechecks_attachment_under_the_write_lock`) как образец, + без которого регрессия дожила до боевого конфига. +- Риск «резолвер стоит на пути защиты от traversal» назван явно и + вынесен в отдельный AC (AC4), а не спрятан в примечание — правильная + структура даже с учётом находки M1 (ошибка в одном из четырёх + примеров, не в самом принципе выделения traversal в отдельный + критерий). +- Риск смены классификации `content_manifest` (`external` → `internal`) + проверен по коду: `identity()` в `_content_state` (`:1034-1039`) + действительно не использует `storage` в ключе сравнения — совместимость + со старыми экспортами реально сохраняется, а не заявлена без + проверки. +- Решение не выносить текст ошибки `invalid_content` пользователю в + скоуп — уже принято владельцем на этапе аналитики (тот же аккаунт, + комментарий S2-analysis) как сужение с обоснованием «вторая + поверхность, потеря лёгкого трека»; ревью с этим решением не спорит, + так как это ровно тот класс продуктового вопроса, который решает + владелец, а не автор ТЗ задним числом. +- Блок «Принято предположительно» — реальные технические допущения + (выбор `urlsplit`, отказ от нормализации URL в сторону сохранения + как есть, расположение юнитов в `test_validation.py`), не подмена + продуктового решения; продуктовое решение «бэкап должен + импортироваться, вложения ведут себя как раньше» дано самим текстом + issue. +- Открытых продуктовых вопросов владельцу нет и не должно быть: + ожидаемое поведение (импорт проходит, `detach_required` на чужом + инстансе штатно) уже зафиксировано существующим механизмом импорта, + решать в ТЗ было нечего кроме технического разбора URL. +- Откат осмыслен и минимален: одна функция, откат — возврат прежнего + тела, данные не мигрируют. +- Release-артефакты корректно требуют `User-Visible: yes` и оба + changelog — пользователь видит другой исход операции импорта, это не + внутренняя правка. + +### Наблюдение по AC1 (снято ревьюером, не Low-находка) + +AC1 явно тестирует резолвинг только для ветки вложений (`files/...` с +`marker_id`+`name`); ветка планов (`plans/_/...`) с `query` отдельным +кейсом не покрыта, хотя это та же функция `_internal_path` и правка +(отделение query через `urlsplit` в начале функции) по построению +одинаково затрагивает обе ветки. Не поднимаю до Low: реальных +источников `query` у `plan_url` в кодовой базе не найдено (ни +`contentUrl()` в `src/logic.ts`, ни ручка загрузки не добавляют его), и +баг-репорт исходно и единственно про PDF-вложения. Разработчик волен +добавить такой кейс без правки ТЗ — тестовое покрытие, а не контракт. + +## Чего не проверял + +- Гейты кода (`npx tsc --noEmit`, `npm test`, `npm run build`, + `python -m pytest tests_backend -q`) не запускались: на этапе ревью + ТЗ реализации ещё нет, продуктовый код не менялся (только тело + issue — не файл в репозитории). +- Полный HA-харнесс и реальный сценарий экспорт→импорт с настоящим PDF + не прогонялся — на этом этапе прогонять нечего, это предмет AC3 на + код-ревью. +- Не проверялась историческая полнота предыдущих ревью по #50 и #167 + (упомянуты автором как соседние, не дубликаты) на предмет их + собственных незакрытых находок — вне предмета этого ревью. +- Percent-encoding в именах файлов и нормализация существующих + конфигов — намеренно вне скоупа этого issue (см. «Не в скоупе» ТЗ), + не проверялись как отдельная тема. + +## Раздел «Унаследовано» — не применяется + +Это первый цикл ревью ТЗ (r1); раздел «Унаследовано из r» и +таблица закрытия предыдущего раунда не ведутся (§2.10 PROCESS.md +действует со второго цикла).