mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 19:58:50 +00:00
Merge issue #225 into dev
Import of a backup holding PDF attachments: the content resolver parses a url
as a url, and the three mutants guarding it are registered. The user-visible
change is documented in 4a84734, which carries both changelog entries — this
merge adds no behaviour of its own.
Code review r2 green (docs/reviews/CODE-REVIEW-225-r2.md). The third pass was
a rebase over #226, not a fix — owner arbitration on the review-4 the cycle
counter raised for it (PROCESS.md §4; counter defect filed as #227).
Issue: #225
User-Visible: no
This commit is contained in:
@@ -17,6 +17,7 @@ import unicodedata
|
||||
from datetime import UTC, datetime
|
||||
from pathlib import Path
|
||||
from typing import Any, Callable
|
||||
from urllib.parse import urlsplit
|
||||
|
||||
import voluptuous as vol
|
||||
|
||||
@@ -348,6 +349,24 @@ def placement_manifest(config: dict[str, Any], layout: dict[str, Any]) -> list[d
|
||||
|
||||
|
||||
def _internal_path(root: Path, url: str) -> tuple[str, Path] | None:
|
||||
# A url is parsed as a url, not as a string: everything after "?" or "#"
|
||||
# addresses the transfer, never the file. Legacy attachments carry a
|
||||
# cache-buster (".../files/m1/doc.pdf?v=1783170649"), and while the string
|
||||
# form fed "doc.pdf?v=1783170649" to sanitize_filename the name never
|
||||
# matched itself — the reference read as internal-but-non-canonical and
|
||||
# every backup holding one refused to import (issue #225). Path segments
|
||||
# keep doing the guarding: dropping the query cannot widen what a segment
|
||||
# is allowed to be.
|
||||
#
|
||||
# Only a same-document reference may be trusted this way: with a scheme or
|
||||
# an authority the path belongs to another host, and taking it would let
|
||||
# "https://evil.example/houseplan_files/files/m1/doc.pdf" resolve onto a
|
||||
# local file (review CODE-REVIEW-225-r1, M1). Such a url stays external,
|
||||
# which is also what _looks_internal says about it.
|
||||
parsed = urlsplit(url)
|
||||
if parsed.scheme or parsed.netloc:
|
||||
return None
|
||||
url = parsed.path
|
||||
content_plan = CONTENT_URL + "/plans/_/"
|
||||
if url.startswith(content_plan) or url.startswith(PLANS_URL + "/"):
|
||||
prefix = content_plan if url.startswith(content_plan) else PLANS_URL + "/"
|
||||
|
||||
@@ -7,6 +7,11 @@
|
||||
contains only the remaining active visible entities and disappears when
|
||||
none remain; two explicitly placed entity/device markers still coexist
|
||||
([#226](https://github.com/Matysh/houseplan-card/issues/226)).
|
||||
- Fixed a backup with PDF attachments refusing to import back with "The backup
|
||||
contains invalid or inconsistent content references". Attachment links that
|
||||
carry a cache-buster (`…/files/marker/doc.pdf?v=1783170649`) are now resolved
|
||||
by their path, as a URL rather than as a string
|
||||
([#225](https://github.com/Matysh/houseplan-card/issues/225)).
|
||||
|
||||
## v1.66.0 — 2026-08-20
|
||||
|
||||
|
||||
@@ -14,6 +14,11 @@
|
||||
пустом остатке он исчезает; две явно размещённые привязки entity/device
|
||||
по-прежнему могут сосуществовать
|
||||
([#226](https://github.com/Matysh/houseplan-card/issues/226)).
|
||||
- Исправлено: резервная копия с прикреплёнными PDF отказывалась импортироваться
|
||||
обратно с сообщением «Резервная копия содержит некорректные или
|
||||
несогласованные ссылки на файлы». Ссылки на вложения с кэш-бастером
|
||||
(`…/files/маркер/doc.pdf?v=1783170649`) теперь разбираются по пути — как URL,
|
||||
а не как строка ([#225](https://github.com/Matysh/houseplan-card/issues/225)).
|
||||
|
||||
## v1.66.0 — 2026-08-20
|
||||
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
# Code review #225 — r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/225
|
||||
- **Spec:** тело issue (лёгкий трек `small`), зелёное `SPEC-REVIEW-225-r2.md`
|
||||
- **Reviewed branch:** `issue/225-import-attachment-url-query`
|
||||
- **Reviewed range:** `origin/dev..HEAD` = `e59962f` (единственный продуктовый коммит)
|
||||
- **Base:** `origin/dev` at `5f000cb`
|
||||
- **Reviewer:** Claude, независимая сессия без контекста реализации
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Жёлтый · цикл r1/2 · High: 0 · Medium: 2 → в задаче.**
|
||||
|
||||
Диагноз и контракт («парсить URL как URL», `urlsplit(url).path`, query/fragment
|
||||
не участвуют в определении файла) реализованы и покрыты честными тестами:
|
||||
семь параметризованных тестов на `_internal_path`/`_content_state` плюс
|
||||
полный roundtrip-тест, все зелёные в CI на точном SHA (см. «Как проверялось»).
|
||||
Я независимо воспроизвёл логику `_internal_path` вне HA-харнеса и подтвердил,
|
||||
что каждый из новых тестов **умеет падать** на добеговом резолвере.
|
||||
|
||||
Но контракт реализован не полностью: `urlsplit(url).path` доверяет пути даже
|
||||
тогда, когда у URL есть `scheme`/`netloc` — то есть строка вида
|
||||
`https://evil.example/houseplan_files/files/m1/doc.pdf` после фикса резолвится
|
||||
как **внутренний** файл, хотя `_looks_internal` (строковая проверка префикса,
|
||||
не изменена) по‑прежнему говорит «не внутренний». Это то самое расхождение,
|
||||
от которого защищает `_content_state` (docstring прямо называет угрозу:
|
||||
«a crafted file could ... bypass the mandatory detach decision»), только в
|
||||
обратную сторону от исходного бага. Ни один AC (в частности AC5) эту форму
|
||||
входа не проверяет (M1).
|
||||
|
||||
Отдельно: ТЗ само выписало три записи мутационного гейта с готовыми
|
||||
find/replace-патчами в формате `scripts/mutation-gate.mjs` — ровно так, как
|
||||
это уже делалось для прежних бэкенд-фиксов в этом же файле (#167:
|
||||
`plan-only-*`, тем же коммитом). В этой задаче патчи не легли в реестр —
|
||||
проверка осталась разовой, только в тексте хендоффа (M2).
|
||||
|
||||
Оба High отсутствуют, обе находки в скоупе issue (правка того же файла и
|
||||
того же гейта, который эта задача уже трогает) — по §2.7/§7.2 PROCESS.md
|
||||
это жёлтый вердикт с возвратом автору, а не отдельный issue.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Единственный продуктовый коммит `e59962f` (`Issue: #225`, `User-Visible: yes`,
|
||||
трейлеры на месте):
|
||||
|
||||
- `custom_components/houseplan/import_export.py` (+10) — `_internal_path`
|
||||
теперь режет `urlsplit(url).path` до сравнения сегментов; остальная логика
|
||||
(префиксы `plans/_/`/`FILES_URL`, `sanitize_marker_id`/`sanitize_filename`,
|
||||
требование ровно двух сегментов, посегментное сравнение) не тронута;
|
||||
- `tests_backend/test_ha_import_export.py` (+128, 7 тестов) — AC1/AC1-plan,
|
||||
AC4, AC4a, AC5, AC2 (обе ветки `same_source`), AC3 (полный roundtrip);
|
||||
- `docs/CHANGELOG.md` + `docs/CHANGELOG.ru.md` (+6/+6) — пользовательская
|
||||
формулировка совпадает с USER-GUIDE-стилем сообщения об ошибке, ссылка на
|
||||
#225 есть в обоих.
|
||||
|
||||
Никаких изменений в `src/**`, i18n, миграции, конфиге совместимости — как и
|
||||
заявлено в ТЗ. Ветка `issue/225-import-attachment-url-query` соответствует
|
||||
правилу именования.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
| Гейт | Результат |
|
||||
|---|---|
|
||||
| `npx tsc --noEmit` | pass |
|
||||
| `npm test` | **962/962 pass** |
|
||||
| `npm run build` + сверка трёх копий бандла | pass, `dist`/`custom_components/houseplan/frontend`/`demo/srv/assets` побайтово совпадают, `git status` после сборки чист |
|
||||
| CI `Validate` на точном SHA `e59962f` (`gh run view 32403605914`) | `completed success`; job `backend` — `completed success`, лог: «Backend unit tests (pure + HA harness) — 335 passed in 3.78s» (полный HA-харнес, включая `test_ha_*.py`, не только «чистое» подмножество) |
|
||||
| Независимая проверка «тест умеет падать» — извлёк `_internal_path` (плюс `sanitize_marker_id`/`sanitize_filename`, у функции нет собственной HA-зависимости, она приходит из модуля целиком) и прогнал старую/новую версию на всех кейсах AC1/AC1-plan/AC4/AC4a/AC5 вне HA-харнеса | новая версия проходит все кейсы AC1/AC1-plan/AC4/AC4a/AC5; **старая версия (`origin/dev`) красная** на 4/5 кейсов AC1, обоих кейсах AC1-plan-с-query и обоих кейсах AC4a — регрессия доказуема, а не заявлена на слово |
|
||||
| `python -m pytest tests_backend -q` локально | не прогонял — окружение ревью на py3.12 без `homeassistant`, `test_ha_*.py` тем же механизмом молча пропускается (см. `AGENTS.md`); заменено CI-прогоном выше и независимой проверкой резолвера |
|
||||
| browser `demo/smoke_*.mjs` (127 шт.) | не прогонял — задача не касается `src/**`, ни один AC их не называет |
|
||||
| `npm run golden:verify` | не прогонял — визуала нет, бэкенд-only фикс |
|
||||
| performance-профили | не прогонял — не названы в AC, путь не производительный |
|
||||
|
||||
## Проверка AC1–AC7
|
||||
|
||||
| AC | Метод по ТЗ | Статус | Как закрыт |
|
||||
|---|---|---|---|
|
||||
| AC1 | backend unit | ✅ | `test_issue_225_attachment_url_resolves_regardless_of_query` (5 кейсов) + `..._plan_url_resolves_regardless_of_query` (3 кейса); я пересчитал `urlsplit(url).path` для каждого кейса вручную и вне HA — совпадает |
|
||||
| AC2 | backend unit/`_content_state` | ✅ | `test_issue_225_content_state_accepts_a_cache_busted_attachment`, обе ветки `same_source`; зелёный в CI на точном SHA (см. выше) |
|
||||
| AC3 | backend HA-харнес, roundtrip | ✅ | `test_issue_225_backup_with_an_attachment_survives_a_full_round_trip`; зелёный в CI |
|
||||
| AC4 | backend unit, traversal | ✅ | `test_issue_225_traversal_stays_closed_with_a_query` (4 кейса); подтверждено чтением — `sanitize_marker_id`/`sanitize_filename` и посегментное сравнение работают над `urlsplit(url).path`, то есть после отделения query, ровно как и до фикса |
|
||||
| AC4a | backend unit | ✅ | `test_issue_225_hostile_looking_query_does_not_reject_a_valid_path` (2 кейса); я подтвердил разбором, что мусор в query/fragment действительно не участвует в резолвинге |
|
||||
| AC5 | backend unit | ⚠️ формально ✅, но неполно | `test_issue_225_external_url_is_still_external` зелёный для `https://example.invalid/floor.svg?v=1` — путь этого URL (`/floor.svg`) не совпадает с внутренним префиксом. AC **не покрывает** случай, когда путь абсолютного/protocol‑relative внешнего URL *совпадает* с внутренним префиксом — см. M1: в этом случае `_internal_path` **резолвит его как внутренний**, хотя `_looks_internal` говорит «нет» |
|
||||
| AC6 | regression, существующие тесты без правок | ✅ | diff — только добавления в конец файла (проверено `git show`), ни одна существующая строка теста не изменена; CI backend green |
|
||||
| AC7 | plan-only idempotency (`:704`) | ✅ (чтением) | тесты `test_plan_only_export_projects_geometry_and_round_trips_room_labels` и соседние в диапазоне 489–736 не задеты диффом; `_internal_path` используется в plan-only пути так же, как раньше, отличие только в разборе query/fragment |
|
||||
|
||||
## Находки
|
||||
|
||||
### M1 (Medium, в скоупе) — `_internal_path` доверяет `path` даже при непустых `scheme`/`netloc`
|
||||
|
||||
`urlsplit(url).path` отбрасывает не только query/fragment, но и `scheme` с
|
||||
`netloc`. Для строки с полной схемой или protocol‑relative префиксом это
|
||||
означает, что путь резолвится как внутренний, даже когда URL целиком указывает
|
||||
на другой хост.
|
||||
|
||||
Воспроизведено извлечением и прогоном литеральной логики `_internal_path` вне
|
||||
HA (сравнение с `_looks_internal`, определённой в том же файле построчно):
|
||||
|
||||
```
|
||||
url = "https://evil.example/houseplan_files/files/m1/doc.pdf"
|
||||
_looks_internal(url) -> False (не меняется фиксом)
|
||||
_internal_path(root, url) (НОВЫЙ) -> ('attachment', <root>/files/m1/doc.pdf)
|
||||
_internal_path(root, url) (СТАРЫЙ, origin/dev) -> None
|
||||
```
|
||||
|
||||
То же самое для protocol-relative `//evil.example/houseplan_files/files/m1/doc.pdf`
|
||||
и для варианта с `?v=1`. `urlsplit` реально разбирает такие строки так, что
|
||||
`netloc="evil.example"`, `path="/houseplan_files/files/m1/doc.pdf"` — я
|
||||
проверил это отдельно интерпретатором, это не домысел.
|
||||
|
||||
Практический эффект ограничен (traversal по‑прежнему закрыт AC4 —
|
||||
посегментные проверки не меняются): `_looks_internal` в `_content_state` не
|
||||
кидает `ImportFailure`, потому что она сама смотрит на исходную строку и
|
||||
по‑прежнему говорит «внешний». Но `internal is not None` (новое поведение)
|
||||
уводит такую запись в ветку `available`/`detach_required` вместо `external`:
|
||||
`content_manifest` на экспорте и `_content_state` на импорте начинают
|
||||
описывать заведомо внешнюю ссылку так, будто она указывает на настоящий
|
||||
локальный файл, включая проверку `is_file()` по пути, который в
|
||||
действительности к этому URL не относится. Это ровно тот класс
|
||||
несогласованности, который докстринг `_content_state` называет угрозой
|
||||
(«a crafted file could ... bypass the mandatory detach decision»), только не
|
||||
исходный баг issue, а новый, привнесённый самим фиксом.
|
||||
|
||||
Ни один AC (в частности AC5, единственный про «внешний» URL) не проверяет
|
||||
такую форму — тест `https://example.invalid/floor.svg?v=1` не совпадает по
|
||||
path с внутренним префиксом и потому не может поймать эту ветку.
|
||||
|
||||
**Фикс:** перед тем как доверять `parsed.path`, требовать
|
||||
`not parsed.scheme and not parsed.netloc`, иначе — как и сегодня для любой
|
||||
внешней ссылки — возвращать `None`. Пара строк плюс тест-кейс(ы) с `https://`
|
||||
и `//`-префиксом на пути, совпадающем с внутренним namespace.
|
||||
|
||||
**Вердикт:** в скоупе issue (тот же файл, тот же контракт «разобрать URL как
|
||||
URL»), чинится в этом же цикле.
|
||||
|
||||
### M2 (Medium, в скоупе) — заявленный мутационный гейт не зарегистрирован
|
||||
|
||||
ТЗ issue содержит раздел «Мутационный гейт» с тремя id и готовыми
|
||||
find/replace-патчами (`internal-path-ignores-query`,
|
||||
`internal-path-allows-traversal`, `roundtrip-import-with-attachment`) —
|
||||
буквально в формате записей `scripts/mutation-gate.mjs`. Это не абстрактная
|
||||
формулировка риска: патчи прямо адресуют строки `import_export.py`.
|
||||
|
||||
Прецедент в этом же файле — issue #167 (`feat: add plan-only space export`,
|
||||
коммит `7f397a6`): пять аналогичных backend-мутантов
|
||||
(`plan-only-room-area-restored` и соседние, гвард
|
||||
`node scripts/backend-test-guard.mjs <pattern>`) были добавлены в реестр
|
||||
**тем же коммитом**, что и сама правка. `scripts/backend-test-guard.mjs`
|
||||
уже поддерживает произвольный `-k`-паттерн по `test_ha_import_export.py`, то
|
||||
есть механизм для #225 был готов без доработок — `node
|
||||
scripts/backend-test-guard.mjs issue_225_attachment_url_resolves` и подобные
|
||||
сразу работали бы как guard.
|
||||
|
||||
В `e59962f` `scripts/mutation-gate.mjs` не менялся (проверено `git show
|
||||
e59962f --stat` и `git log --follow -- scripts/mutation-gate.mjs`). Хендофф
|
||||
описывает, что автор вручную прогонял мутации через отдельные патч-версии
|
||||
файла и даже сделал полезное наблюдение (первая редакция
|
||||
`internal-path-allows-traversal` оказалась «эквивалентной» — traversal
|
||||
защищён двумя независимыми проверками одновременно) — но эта работа нигде не
|
||||
осела постоянной проверкой. Без записи в реестре периодический
|
||||
предрелизный `mutation-gate` (`.github/workflows/mutation-gate.yml`) никогда
|
||||
не перепроверит, что будущий рефакторинг `_internal_path` не вернёт баг
|
||||
#225 бесшумно — то есть именно тот сценарий, ради которого механизм
|
||||
существует (см. комментарий в начале `scripts/mutation-gate.mjs`: «зелёный
|
||||
тест в этом проекте несколько раз означал ничего не проверено»).
|
||||
|
||||
**Вердикт:** в скоупе issue (`scripts/mutation-gate.mjs` — класс B, «может
|
||||
использовать issue того изменения, которое покрывает»), чинится добавлением
|
||||
двух работающих записей в этом же цикле (третью, `internal-path-allows-
|
||||
traversal`, — с патчем, который автор уже подобрал как небезрезультатный,
|
||||
согласно находке из хендоффа).
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Диагноз бага (несовпадение `_looks_internal`/`_internal_path` из-за
|
||||
`sanitize_filename` над сырым хвостом с `?v=...`) и контракт исправления
|
||||
(`urlsplit`, query/fragment не участвуют в определении файла) — совпадают
|
||||
с фактическим кодом `_internal_path` до и после правки.
|
||||
- Данные пользователя не переписываются: URL в конфиге сохраняется как есть
|
||||
(проверено чтением — фикс меняет только внутреннюю логику резолвинга,
|
||||
возвращаемое значение из `create_export`/`_content_state` не трогает
|
||||
`item["url"]`; тест `test_issue_225_content_state_accepts_a_cache_busted_attachment`
|
||||
отдельно утверждает `rows[0]["url"] == url`).
|
||||
- `identity()` (`:1034-1039`) не включает `storage`/`state` в ключ сравнения
|
||||
— смена классификации `external → internal` для канонических cache-busted
|
||||
ссылок (сама цель фикса) не ломает сопоставление manifest-строк; тем же
|
||||
свойством объясняется, почему M1 не валит существующие проверки
|
||||
идентичности, только их семантику для одной специфичной формы входа.
|
||||
- Traversal-защита (AC4) не ослаблена: `sanitize_marker_id`/
|
||||
`sanitize_filename` и требование `len(tail) == 2` работают над результатом
|
||||
`urlsplit(url).path`, то есть над тем же материалом, что и раньше для
|
||||
URL без scheme/netloc — я прогнал старую и новую версию резолвера на всех
|
||||
четырёх кейсах AC4 и получил идентичный `None` в обеих.
|
||||
- AC6/AC7 не нарушены: diff — чистое добавление в конец файла тестов, ни
|
||||
одна существующая строка не тронута; plan-only regression-диапазон вне
|
||||
диффа.
|
||||
- Trailers `Issue: #225`/`User-Visible: yes`, оба changelog в одном коммите,
|
||||
ветка `issue/225-import-attachment-url-query` — по правилам.
|
||||
- Три копии бандла побайтово совпадают после локальной пересборки — класс D
|
||||
не разошёлся, хотя эта задача его не трогает (бэкенд-only).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- `python -m pytest tests_backend -q` в собственном окружении — ревью
|
||||
выполняется на py3.12 без установленного `homeassistant`; `conftest.py`
|
||||
тем же механизмом, что и у автора, молча пропустил бы `test_ha_*.py`, то
|
||||
есть локальный зелёный прогон здесь ничего не доказывает (см. `AGENTS.md`,
|
||||
раздел «Backend»). Использован CI `Validate` на точном SHA (`backend`:
|
||||
335 passed, полный HA-харнес) плюс независимое исполнение чистой логики
|
||||
`_internal_path` вне HA — сильнее, чем «поверил хендоффу».
|
||||
- Полный набор из 127 browser-smoke и `performance_smoke` — задача не
|
||||
касается `src/**`, ни один AC их не называет, поверхность чисто бэкендовая.
|
||||
- `npm run golden:verify` — визуальных изменений нет.
|
||||
- Мутационный гейт (`node scripts/mutation-gate.mjs`, дорогой прогон с
|
||||
пересборкой бандла в отдельном worktree) целиком не запускал — сам факт
|
||||
отсутствия трёх заявленных записей в реестре зафиксирован находкой M2 без
|
||||
необходимости гонять существующие 40+ мутантов, которых этот диф не
|
||||
касается.
|
||||
@@ -0,0 +1,149 @@
|
||||
# Code review #225 — r2
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/225
|
||||
- **Spec:** тело issue (лёгкий трек `small`), зелёное `SPEC-REVIEW-225-r2.md`
|
||||
- **Reviewed branch:** `issue/225-import-attachment-url-query`
|
||||
- **r1 code review:** жёлтый · SHA `e59962f` · документ `docs/reviews/CODE-REVIEW-225-r1.md`
|
||||
- **Дельта этого раунда:** `git diff e59962f..HEAD` = коммит `04945e1`
|
||||
(«fix: reject absolute urls in the content resolver, register the mutants»)
|
||||
- **Base:** `origin/dev` at `5f000cb` (ветка — линейное продолжение `5f000cb`,
|
||||
не ребейз: `git merge-base HEAD origin/dev` = `5f000cb`, `origin/dev` ушёл
|
||||
вперёд по несвязанному issue #226 своей веткой)
|
||||
- **Reviewer:** Claude, независимая сессия без контекста реализации
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Зелёный · цикл r2/2 · High: 0 · Medium: 0.**
|
||||
|
||||
Разбор — по дельте (§2.10 PROCESS.md, issue #214): r1 закончился жёлтым с
|
||||
двумя находками в скоупе (M1 — резолвер доверял `path` даже при `scheme`/
|
||||
`netloc`; M2 — заявленные мутанты не осели в реестре). Дельта `e59962f..HEAD`
|
||||
— один коммит, касающийся только того же файла/теста/гейта, которые уже были
|
||||
в скоупе задачи; не ребейз, контракт поведения не менялся, новая подсистема не
|
||||
затронута — сокращённый разбор оправдан.
|
||||
|
||||
Обе находки закрыты правильно и это подтверждено не заявлением автора, а
|
||||
независимой проверкой: извлёк литеральную логику `_internal_path` (плюс
|
||||
`sanitize_marker_id`/`sanitize_filename`, `_looks_internal`) вне HA-харнеса и
|
||||
прогнал её на всех AC-кейсах и на всех трёх мутантах из реестра — старая
|
||||
(до фикса) версия резолвит `https://evil.example/houseplan_files/files/m1/doc.pdf`
|
||||
как внутренний файл, новая — корректно возвращает `None`; каждый из трёх
|
||||
зарегистрированных мутантов при применении к текущему коду переворачивает
|
||||
хотя бы один из своих AC-кейсов (детали — «Закрытие раунда r1» ниже). CI
|
||||
`Validate` зелёный на точном SHA `04945e1`, job `backend` — 339 passed (было
|
||||
335 на `e59962f`, +4 — ровно новый параметризованный тест на 4 варианта
|
||||
абсолютного/protocol-relative URL). Дешёвые гейты (`tsc`, `npm test`
|
||||
962/962, `build` + сверка трёх копий бандла, `mutation-gate --check` по
|
||||
всему реестру, включая три новых id) прогнаны локально и зелёные.
|
||||
|
||||
Новых находок в этом раунде нет.
|
||||
|
||||
## Скоуп дельты
|
||||
|
||||
Один продуктовый коммит `04945e1` (`Issue: #225`, `User-Visible: no` —
|
||||
верно: это закрытие M1/M2, а не пользовательское поведение; пользовательский
|
||||
эффект уже описан в changelog коммитом `e59962f`, changelog в `04945e1` не
|
||||
трогается и не должен был):
|
||||
|
||||
- `custom_components/houseplan/import_export.py` (+10/−1) — в `_internal_path`
|
||||
перед тем как доверять `parsed.path`, добавлена проверка
|
||||
`if parsed.scheme or parsed.netloc: return None`; комментарий указывает на
|
||||
ревью r1/M1. Остальная функция (префиксы, `sanitize_marker_id`/
|
||||
`sanitize_filename`, посегментное сравнение) не тронута — совпадает с
|
||||
диффом `e59962f..HEAD` построчно;
|
||||
- `tests_backend/test_ha_import_export.py` (+20, 1 тест, 4 кейса) —
|
||||
`test_issue_225_absolute_url_never_resolves_onto_a_local_file`: https+FILES_URL,
|
||||
https+CONTENT_URL с query, protocol-relative `//`, https+PLANS_URL;
|
||||
- `scripts/mutation-gate.mjs` (+41, 3 записи) — `internal-path-ignores-query`,
|
||||
`internal-path-trusts-foreign-host`, `internal-path-allows-traversal`, все
|
||||
с `guard: node scripts/backend-test-guard.mjs issue_225`;
|
||||
- никаких изменений в `docs/CHANGELOG*`, `src/**`, i18n, конфиге
|
||||
совместимости — соответствует `User-Visible: no` и тому, что коммит чинит
|
||||
находки код-ревью, а не меняет заявленное поведение.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
| Гейт | Результат |
|
||||
|---|---|
|
||||
| `npx tsc --noEmit` | pass |
|
||||
| `npm test` | **962/962 pass** (включает дешёвую половину мутационного гейта — `test/mutation-gate.test.mjs`: уникальность якорей и существование guard-файлов для всех мутантов, включая три новых) |
|
||||
| `npm run build` + сверка трёх копий бандла | pass, `dist`/`custom_components/houseplan/frontend`/`demo/srv/assets` побайтово совпадают, `git status` после сборки чист |
|
||||
| `node scripts/mutation-gate.mjs --check` (весь реестр, не только новые id) | **ok** на всех записях — новые три не столкнулись якорями со старыми 20+ |
|
||||
| CI `Validate` на точном SHA `04945e1` (`gh api .../commits/04945e1/check-runs`, job `backend` id `96542792712`) | `completed success`; лог: «Backend unit tests (pure + HA harness) — **339 passed** in 5.26s» (было 335 на `e59962f`, делта +4 совпадает с числом новых параметризованных кейсов) |
|
||||
| Независимая проверка «мутант умеет ловиться» — вынес `_internal_path`/`_looks_internal`/`sanitize_marker_id`/`sanitize_filename` в автономный скрипт (без HA) и прогнал: (а) новую версию на AC1/AC1-plan/AC4/AC4a/AC5-M1; (б) буквальные патчи всех трёх мутантов реестра против неё | новая версия проходит все кейсы; **все три мутанта переворачивают** результат хотя бы одного своего AC-кейса — не эквивалентные патчи, не «зелёный тест ничего не проверяет» (детали ниже) |
|
||||
| `python -m pytest tests_backend -q` локально | не прогонял — окружение ревью на py3.12 без `homeassistant`; `test_ha_*.py` тем же механизмом молча пропускается, что и у автора и у меня же в r1 (см. `AGENTS.md`, «Backend»). Заменено CI-прогоном на точном SHA плюс независимым исполнением чистой логики вне HA — то же покрытие, что использовалось в r1 |
|
||||
| browser `demo/smoke_*.mjs` (127 шт.), `npm run golden:verify`, performance-профили | не прогонял — дельта не касается `src/**`/визуала/производительности, ни один AC их не называет; то же основание, что в r1 |
|
||||
|
||||
## Закрытие раунда r1
|
||||
|
||||
| Находка r1 | Чем закрыта | Где это видно |
|
||||
|---|---|---|
|
||||
| **M1** — `urlsplit(url).path` доверяет пути даже при `scheme`/`netloc`; `https://evil.example/houseplan_files/files/m1/doc.pdf` резолвится как локальный файл, хотя `_looks_internal` говорит «внешний» | Добавлена проверка `if parsed.scheme or parsed.netloc: return None` перед использованием `parsed.path` | `custom_components/houseplan/import_export.py:364-366`; тест `test_issue_225_absolute_url_never_resolves_onto_a_local_file` (4 кейса: https+FILES_URL, https+CONTENT_URL+query, `//`+FILES_URL, https+PLANS_URL) — все требуют `_internal_path(...) is None` и `_looks_internal(...) is False`. Независимо прогнал ту же логику вне HA: без проверки все 4 URL резолвятся в валидный локальный путь, с проверкой — все 4 дают `None` |
|
||||
| **M2** — три мутанта из ТЗ (`internal-path-ignores-query`, `internal-path-allows-traversal`, плюс подразумеваемый мутант на M1) не зарегистрированы в `scripts/mutation-gate.mjs`, разовый ручной прогон не оставляет постоянной защиты | Три записи добавлены в `MUTANTS` (`scripts/mutation-gate.mjs:676-716`): `internal-path-ignores-query`, `internal-path-trusts-foreign-host` (регрессия M1), `internal-path-allows-traversal`, все с `guard: node scripts/backend-test-guard.mjs issue_225` | `node scripts/mutation-gate.mjs --check` — все три анкера уникальны в текущем коде; `test/mutation-gate.test.mjs` (часть `npm test`, 962/962) проверяет то же плюс существование guard-файла. Прогнал патчи буквально: `internal-path-ignores-query` переворачивает 2/6 кейсов AC1 (варианты с `#fragment`) и оба кейса AC4a; `internal-path-trusts-foreign-host` переворачивает все 4 кейса нового теста M1; `internal-path-allows-traversal` переворачивает кейс `plans/_/../x.svg?v=1` из AC4 (для `files`-ветки этот же мутант оказывается эквивалентным — `sanitize_marker_id("..") == "misc"` ловит traversal независимо от структурной проверки, поэтому реестр справедливо не утверждает, что он ловит все AC4-кейсы, только тот один, который у файловой ветки не защищён вторым эшелоном) |
|
||||
|
||||
## Унаследовано из r1
|
||||
|
||||
Без повторной проверки в этом раунде — дельта их не касается — приняты выводы
|
||||
из `docs/reviews/CODE-REVIEW-225-r1.md` (SHA `e59962f`):
|
||||
|
||||
- диагноз бага и контракт исправления (`urlsplit`, query/fragment вне
|
||||
определения файла) совпадают с кодом;
|
||||
- данные пользователя не переписываются (`item["url"]` не трогается);
|
||||
- `identity()` (`:1034-1039`) не включает `storage` в ключ сравнения —
|
||||
смена классификации `external → internal` для канонических cache-busted
|
||||
ссылок не ломает сопоставление manifest-строк (AC6);
|
||||
- traversal-защита для случаев без `scheme`/`netloc` (три из четырёх кейсов
|
||||
AC4) не ослаблена дропом query — сегментные проверки работают над тем же
|
||||
материалом, что и раньше;
|
||||
- AC6/AC7 (regression, plan-only idempotency) — diff `e59962f` только
|
||||
добавляет тесты в конец файла, plan-only диапазон (`:489-736`) вне диффа;
|
||||
- trailers, ветка, оба changelog в коммите `e59962f` — по правилам.
|
||||
|
||||
Дельта r1→r2 не касается этих участков кода (только резолвер и мутационный
|
||||
реестр), поэтому пересчёт не требовался — см. «Скоуп дельты» выше, где
|
||||
подтверждено, что diff `e59962f..HEAD` ограничен ровно тем, что было
|
||||
находками r1.
|
||||
|
||||
## Что проверено и корректно (в этом раунде)
|
||||
|
||||
- M1-фикс не задевает уже покрытые ветки: прогнал старую/новую версию
|
||||
резолвера на AC1 (5+3 кейса), AC4 (4 кейса), AC4a (2 кейса) — результат
|
||||
идентичен до и после добавления проверки `scheme`/`netloc`, потому что ни
|
||||
один из этих URL её не содержит.
|
||||
- `_looks_internal` не менялась (проверено `git diff` — файл затронут только
|
||||
в `_internal_path`), поэтому согласованность двух функций, из-за
|
||||
расхождения которых родился исходный баг #225, теперь восстановлена в обе
|
||||
стороны: внутренний путь ⇒ обе функции согласны; внешний хост ⇒ обе
|
||||
согласны «внешний».
|
||||
- Мутант `internal-path-allows-traversal` умышленно снимает два патча сразу;
|
||||
комментарий `because` в реестре объясняет, что снятие одного из них
|
||||
(`len(tail) != 2`) для файловой ветки было бы эквивалентным мутантом —
|
||||
проверил это утверждение напрямую (см. таблицу выше), оно верно.
|
||||
- Trailers коммита `04945e1` (`Issue: #225`, `User-Visible: no`) корректны:
|
||||
фикс не меняет наблюдаемое пользователем поведение (только закрывает
|
||||
внутреннюю рассогласованность двух приватных функций), поэтому отсутствие
|
||||
правки changelog — не пропуск.
|
||||
- `process-gate`/pre-push пройден (CI job `process-gate`: success на
|
||||
`04945e1`).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- `python -m pytest tests_backend -q` в собственном окружении — та же
|
||||
причина, что в r1 (py3.12, без `homeassistant`, `test_ha_*.py` молча
|
||||
пропускается `conftest.py`). Компенсировано CI-прогоном на точном SHA
|
||||
(339 passed) и независимым исполнением чистой логики резолвера вне HA.
|
||||
- Полный (дорогой) прогон `node scripts/mutation-gate.mjs` с пересборкой
|
||||
бандла в worktree для каждого из 20+ мутантов реестра — не требовался:
|
||||
задача не трогает остальные мутанты, а три новых проверены анкерным
|
||||
`--check` и независимым буквальным прогоном патчей против чистой логики
|
||||
(эквивалент того, что делает дорогой прогон, без пересборки бандла на
|
||||
каждый). Дорогой прогон — предрелизный гейт (`.github/workflows/
|
||||
mutation-gate.yml`), не гейт ревью (PROCESS.md §8 vs §2.7).
|
||||
- Browser-смоки (127 шт.), `npm run golden:verify`, performance-профили —
|
||||
дельта не касается `src/**`, визуала или производительности; то же
|
||||
основание, что в r1.
|
||||
|
||||
## Лимит циклов
|
||||
|
||||
Код-ревью лёгкого трека — 2 цикла (§4). Это второй и последний цикл; вердикт
|
||||
зелёный, лимит не понадобился.
|
||||
@@ -703,6 +703,47 @@ export const MUTANTS = [
|
||||
replace: ' .dev:not(.unavail):hover {',
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'internal-path-ignores-query',
|
||||
guard: 'node scripts/backend-test-guard.mjs issue_225',
|
||||
because: 'разбор url строкой вместо urlsplit возвращает баг #225: кэш-бастер '
|
||||
+ '?v=… делает имя файла не равным самому себе, ссылка читается как '
|
||||
+ 'внутренняя-но-неканоническая, и бэкап с вложением снова не импортируется',
|
||||
patches: [{
|
||||
file: 'custom_components/houseplan/import_export.py',
|
||||
find: ' parsed = urlsplit(url)\n if parsed.scheme or parsed.netloc:\n return None\n url = parsed.path',
|
||||
replace: ' url = url.split("?", 1)[0]',
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'internal-path-trusts-foreign-host',
|
||||
guard: 'node scripts/backend-test-guard.mjs issue_225',
|
||||
because: 'доверие к path при наличии scheme/netloc позволяет '
|
||||
+ '"https://evil.example/houseplan_files/files/m1/doc.pdf" разрешиться в локальный '
|
||||
+ 'файл, хотя _looks_internal считает такую ссылку внешней (ревью r1, M1)',
|
||||
patches: [{
|
||||
file: 'custom_components/houseplan/import_export.py',
|
||||
find: ' if parsed.scheme or parsed.netloc:\n return None',
|
||||
replace: ' if False:\n return None',
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'internal-path-allows-traversal',
|
||||
guard: 'node scripts/backend-test-guard.mjs issue_225',
|
||||
because: 'структурные проверки пути — единственное, что режет traversal после '
|
||||
+ 'отделения query; снимать их нельзя. Мутант снимает обе сразу: по одной '
|
||||
+ 'защита эшелонирована (sanitize-сравнение ловит "..") и мутант был бы '
|
||||
+ 'эквивалентным — выяснено прогоном при реализации',
|
||||
patches: [{
|
||||
file: 'custom_components/houseplan/import_export.py',
|
||||
find: ' if not raw_name or "/" in raw_name:\n return None',
|
||||
replace: ' raw_name = raw_name.split("/")[-1]\n if not raw_name:\n return None',
|
||||
}, {
|
||||
file: 'custom_components/houseplan/import_export.py',
|
||||
find: ' tail = url[len(prefix):].split("/")\n if len(tail) != 2:\n return None',
|
||||
replace: ' tail = url[len(prefix):].split("/")[-2:]',
|
||||
}],
|
||||
},
|
||||
];
|
||||
|
||||
// --- механика ---------------------------------------------------------------
|
||||
|
||||
@@ -37,6 +37,7 @@ from custom_components.houseplan.const import (
|
||||
MAX_IMPORT_PREVIEWS_TOTAL,
|
||||
PLAN_MODEL_VERSION,
|
||||
FILES_DIR, PLANS_DIR,
|
||||
CONTENT_URL, FILES_URL, PLANS_URL,
|
||||
)
|
||||
from custom_components.houseplan.store import (
|
||||
async_save_layout_state,
|
||||
@@ -87,6 +88,153 @@ def _document(tmp_path: Path, kind: str = "full") -> dict:
|
||||
return document
|
||||
|
||||
|
||||
# --- issue #225: an attachment url carries a cache-buster ---------------------
|
||||
#
|
||||
# Legacy references look like "/houseplan_files/files/m1/doc.pdf?v=1783170649".
|
||||
# The resolver used to compare the raw tail with its sanitized form, so the
|
||||
# query made the name differ from itself: the reference read as internal (by
|
||||
# prefix) yet non-canonical (by name), and _content_state refused the whole
|
||||
# document. Every backup holding one attachment was impossible to import back.
|
||||
|
||||
|
||||
@pytest.mark.parametrize("url, expected_tail", [
|
||||
(f"{FILES_URL}/m1/doc.pdf?v=1783170649", ("m1", "doc.pdf")),
|
||||
(f"{CONTENT_URL}/files/m1/doc.pdf?v=1783170649", ("m1", "doc.pdf")),
|
||||
(f"{FILES_URL}/m1/doc.pdf#page=2", ("m1", "doc.pdf")),
|
||||
(f"{FILES_URL}/m1/doc.pdf?v=1#page=2", ("m1", "doc.pdf")),
|
||||
(f"{FILES_URL}/m1/doc.pdf", ("m1", "doc.pdf")),
|
||||
])
|
||||
def test_issue_225_attachment_url_resolves_regardless_of_query(
|
||||
tmp_path: Path, url: str, expected_tail: tuple[str, str],
|
||||
) -> None:
|
||||
"""AC1: query and fragment address the transfer, never the file."""
|
||||
resolved = import_export_api._internal_path(tmp_path, url)
|
||||
assert resolved is not None, url
|
||||
kind, path = resolved
|
||||
assert kind == "attachment"
|
||||
assert path == tmp_path / FILES_DIR / expected_tail[0] / expected_tail[1]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("url", [
|
||||
f"{PLANS_URL}/f1.svg?v=1",
|
||||
f"{CONTENT_URL}/plans/_/f1.svg?v=1#page=2",
|
||||
f"{PLANS_URL}/f1.svg",
|
||||
])
|
||||
def test_issue_225_plan_url_resolves_regardless_of_query(tmp_path: Path, url: str) -> None:
|
||||
"""AC1: the plan branch of the same resolver behaves identically."""
|
||||
resolved = import_export_api._internal_path(tmp_path, url)
|
||||
assert resolved == ("plan", tmp_path / PLANS_DIR / "f1.svg")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("url", [
|
||||
f"{FILES_URL}/../../secret.pdf?v=1",
|
||||
f"{FILES_URL}/m1/../../secret.pdf",
|
||||
f"{CONTENT_URL}/plans/_/../x.svg?v=1",
|
||||
f"{PLANS_URL}/../x.svg",
|
||||
])
|
||||
def test_issue_225_traversal_stays_closed_with_a_query(tmp_path: Path, url: str) -> None:
|
||||
"""AC4: dropping the query must not widen what a path segment may be."""
|
||||
assert import_export_api._internal_path(tmp_path, url) is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize("url", [
|
||||
f"{FILES_URL}/m1/doc.pdf?x=/../../etc",
|
||||
f"{FILES_URL}/m1/doc.pdf#/../..",
|
||||
])
|
||||
def test_issue_225_hostile_looking_query_does_not_reject_a_valid_path(
|
||||
tmp_path: Path, url: str,
|
||||
) -> None:
|
||||
"""AC4a: the guard is the path split, not string filtering.
|
||||
|
||||
A query may contain anything at all — slashes and dot-dots included — and
|
||||
still address the very same file. Rejecting on the sight of ".." would fail
|
||||
a legitimate reference while adding no protection: the path segments are
|
||||
what the resolver validates.
|
||||
"""
|
||||
assert import_export_api._internal_path(tmp_path, url) == (
|
||||
"attachment", tmp_path / FILES_DIR / "m1" / "doc.pdf",
|
||||
)
|
||||
|
||||
|
||||
def test_issue_225_external_url_is_still_external(tmp_path: Path) -> None:
|
||||
"""AC5: nothing outside the internal namespaces became internal."""
|
||||
assert import_export_api._internal_path(
|
||||
tmp_path, "https://example.invalid/floor.svg?v=1",
|
||||
) is None
|
||||
assert import_export_api._looks_internal("https://example.invalid/floor.svg?v=1") is False
|
||||
|
||||
|
||||
@pytest.mark.parametrize("url", [
|
||||
f"https://evil.example{FILES_URL}/m1/doc.pdf",
|
||||
f"https://evil.example{CONTENT_URL}/files/m1/doc.pdf?v=1",
|
||||
f"//evil.example{FILES_URL}/m1/doc.pdf",
|
||||
f"https://evil.example{PLANS_URL}/f1.svg",
|
||||
])
|
||||
def test_issue_225_absolute_url_never_resolves_onto_a_local_file(
|
||||
tmp_path: Path, url: str,
|
||||
) -> None:
|
||||
"""AC5: only a same-document reference may be resolved by its path.
|
||||
|
||||
A scheme or an authority means the path belongs to another host. Taking it
|
||||
would let a crafted document describe an outside link as a local file —
|
||||
the same inconsistency the resolver is meant to prevent, mirrored
|
||||
(review CODE-REVIEW-225-r1, M1).
|
||||
"""
|
||||
assert import_export_api._internal_path(tmp_path, url) is None
|
||||
assert import_export_api._looks_internal(url) is False
|
||||
|
||||
|
||||
@pytest.mark.parametrize("same_source, expected_state, expected_confirmation", [
|
||||
(True, "available", False),
|
||||
(False, "detach_required", True),
|
||||
])
|
||||
def test_issue_225_content_state_accepts_a_cache_busted_attachment(
|
||||
tmp_path: Path, same_source: bool, expected_state: str, expected_confirmation: bool,
|
||||
) -> None:
|
||||
"""AC2: both branches of the ownership question, neither an outright refusal."""
|
||||
url = f"{FILES_URL}/lamp/manual.pdf?v=1783170649"
|
||||
attachment = tmp_path / FILES_DIR / "lamp" / "manual.pdf"
|
||||
attachment.parent.mkdir(parents=True, exist_ok=True)
|
||||
attachment.write_bytes(b"%PDF-1.4\n")
|
||||
config = _config()
|
||||
config["markers"][0]["pdfs"] = [{"name": "Manual", "url": url}]
|
||||
runtime = SimpleNamespace(instance_id="instance-a")
|
||||
document, _ = create_export(
|
||||
runtime, {"config": config}, {"layout": {}}, kind="full", space_id=None,
|
||||
card_version="1.61.0", config_root=tmp_path,
|
||||
)
|
||||
rows, confirmation = import_export_api._content_state(document, same_source, tmp_path)
|
||||
assert [row["state"] for row in rows] == [expected_state]
|
||||
assert confirmation is expected_confirmation
|
||||
assert rows[0]["url"] == url, "the stored reference is preserved, cache-buster included"
|
||||
|
||||
|
||||
def test_issue_225_backup_with_an_attachment_survives_a_full_round_trip(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""AC3: export then import the same document back, no manual edits."""
|
||||
url = f"{FILES_URL}/lamp/manual.pdf?v=1783170649"
|
||||
attachment = tmp_path / FILES_DIR / "lamp" / "manual.pdf"
|
||||
attachment.parent.mkdir(parents=True, exist_ok=True)
|
||||
attachment.write_bytes(b"%PDF-1.4\n")
|
||||
config = _config()
|
||||
config["markers"][0]["pdfs"] = [{"name": "Manual", "url": url}]
|
||||
runtime = SimpleNamespace(instance_id="instance-a")
|
||||
document, _ = create_export(
|
||||
runtime, {"config": config}, {"layout": {}}, kind="full", space_id=None,
|
||||
card_version="1.61.0", config_root=tmp_path,
|
||||
)
|
||||
response = create_preview(
|
||||
SimpleNamespace(instance_id="instance-a", import_previews={}),
|
||||
json.dumps(document).encode(), owner_id="alice", duplicate_policy="skip",
|
||||
current_config_data={"config": _config(), "rev": 1},
|
||||
current_layout_data={"layout": {}, "rev": 1}, config_root=tmp_path,
|
||||
)
|
||||
content = response["preview"]["content"]
|
||||
assert [row["state"] for row in content] == ["available"]
|
||||
assert response["preview"]["confirmation_required"] is False
|
||||
|
||||
|
||||
def test_background_defaults_and_store_migration_preserve_legacy_view() -> None:
|
||||
assert DEFAULT_CONFIG["settings"]["bg_mode"] == "daynight"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user