From 2935c293e15fe5d57fd2b7ea360fd8a1ad80c31d Mon Sep 17 00:00:00 2001 From: Codex Date: Thu, 20 Aug 2026 21:48:10 +0300 Subject: [PATCH] fix: reject absolute urls in the content resolver, register the mutants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review CODE-REVIEW-225-r1. M1: urlsplit(url).path was trusted even when the url carried a scheme or an authority, so "https://evil.example/houseplan_files/files/m1/doc.pdf" resolved onto a local file while _looks_internal kept calling it external — the mirror image of the inconsistency this resolver exists to prevent. Only a same-document reference is resolved by its path now. M2: the three mutants the spec described are registered in scripts/mutation-gate.mjs instead of living as a one-off manual run. The traversal entry drops both structural checks at once on purpose: taken one at a time the defence is layered (sanitize_marker_id turns ".." into "misc") and the mutant would be equivalent — established by running it. Issue: #225 User-Visible: no --- custom_components/houseplan/import_export.py | 11 +++++- scripts/mutation-gate.mjs | 41 ++++++++++++++++++++ tests_backend/test_ha_import_export.py | 20 ++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/custom_components/houseplan/import_export.py b/custom_components/houseplan/import_export.py index 9984f2a2..36e8b409 100644 --- a/custom_components/houseplan/import_export.py +++ b/custom_components/houseplan/import_export.py @@ -357,7 +357,16 @@ def _internal_path(root: Path, url: str) -> tuple[str, Path] | None: # 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. - url = urlsplit(url).path + # + # 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 + "/" diff --git a/scripts/mutation-gate.mjs b/scripts/mutation-gate.mjs index 53404361..d00d1a54 100644 --- a/scripts/mutation-gate.mjs +++ b/scripts/mutation-gate.mjs @@ -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:]', + }], + }, ]; // --- механика --------------------------------------------------------------- diff --git a/tests_backend/test_ha_import_export.py b/tests_backend/test_ha_import_export.py index 59f07e60..3d4fcf5f 100644 --- a/tests_backend/test_ha_import_export.py +++ b/tests_backend/test_ha_import_export.py @@ -164,6 +164,26 @@ def test_issue_225_external_url_is_still_external(tmp_path: Path) -> 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),