houseplan-card/docs/reviews/SPEC-REVIEW-225-r1.md

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=<timestamp> в 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 действует со второго цикла).