diff --git a/custom_components/houseplan/plans.py b/custom_components/houseplan/plans.py index 5ca5da9..57d7c9a 100644 --- a/custom_components/houseplan/plans.py +++ b/custom_components/houseplan/plans.py @@ -35,7 +35,11 @@ def unique_filename(directory: Path, name: str) -> str: stem, suffix = safe, "" i = 2 while True: - candidate = f"{stem} ({i}){'.' + suffix if suffix else ''}" + # "-2", not " (2)": the content view sanitizes the name in the REQUEST + # too, and a space or a bracket there turns into "_", so a file called + # "manual (2).pdf" was written and then never served. Only characters + # that survive sanitize_filename may be used to build a name. + candidate = f"{stem}-{i}{'.' + suffix if suffix else ''}" if not (directory / candidate).exists(): return candidate i += 1 diff --git a/custom_components/houseplan/websocket_api.py b/custom_components/houseplan/websocket_api.py index 5f86191..f7f0f74 100755 --- a/custom_components/houseplan/websocket_api.py +++ b/custom_components/houseplan/websocket_api.py @@ -20,7 +20,7 @@ from .const import ( CONTENT_URL, FILES_DIR, MAX_SIGN_PATHS, PLANS_DIR, PLANS_URL, ) from .auth import may_write -from .plans import collect_attachments, collect_plans +from .plans import collect_attachments, collect_plans, unique_filename from .store import HouseplanData, get_data, get_entry from .validation import ( CONFIG_SCHEMA, LAYOUT_SCHEMA, MAX_CONFIG_BYTES, MAX_PLAN_BYTES, @@ -193,15 +193,10 @@ async def ws_files_migrate(hass: HomeAssistant, connection, msg: dict[str, Any]) for f in sorted(src.iterdir()): if not f.is_file(): continue - target = dst / f.name - if target.exists(): - # a different file already owns this name — do NOT silently - # point the url at it; give the copy a unique name instead - stem, suffix = f.stem, f.suffix - i = 2 - while (dst / f"{stem} ({i}){suffix}").exists(): - i += 1 - target = dst / f"{stem} ({i}){suffix}" + # a different file may already own this name — do NOT silently point + # the url at it; the shared helper picks a free one, using only + # characters the content view will accept back in a request + target = dst / unique_filename(dst, f.name) shutil.copy2(str(f), str(target)) mapping[f.name] = target.name return mapping diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 78a38e1..8a3ed21 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -25,7 +25,11 @@ shared folder, so two of them attaching `manual.pdf` ended up pointing at the same physical file. Uploads now take a free name and never overwrite, a new icon gets its own staging folder whose files move to the real icon when the - save is accepted, and an upload nobody saved is collected an hour later. + save is accepted, and an upload nobody saved is collected an hour later. The + name a collision falls back to changed from `manual (2).pdf` to `manual-2.pdf` + — the old one was sanitised on the way back in, so a renamed attachment was + written and then never served (found by the new test, and it applied to + rebind collisions before this release too). - **Two quick edits can no longer lose the second one (HP-1454-03).** The debounce spaced out the starts of a save, not the saves themselves. If one took longer than half a second — a busy instance, a slow link — the next edit diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 3d8e2c8..7d1ccd9 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -33,7 +33,11 @@ один физический файл. Теперь загрузка занимает свободное имя и никогда не перезаписывает, новая иконка получает собственную промежуточную папку, файлы из которой переезжают к настоящей иконке при принятом сохранении, а загрузка, - которую никто не сохранил, убирается через час. + которую никто не сохранил, убирается через час. Имя, на которое уходит + коллизия, сменилось с `manual (2).pdf` на `manual-2.pdf`: старое санитайзилось + на обратном пути, поэтому переименованное вложение записывалось и больше не + отдавалось (нашёл новый тест; до этого релиза так же ломались коллизии при + перепривязке). - **Две быстрые правки больше не теряют вторую (HP-1454-03).** Debounce разносил старты сохранения, а не сами сохранения. Если одно длилось дольше полусекунды — занятый сервер, медленная связь, — следующая правка уходила с diff --git a/test/logic.test.mjs b/test/logic.test.mjs index 7d15154..256de3f 100644 --- a/test/logic.test.mjs +++ b/test/logic.test.mjs @@ -798,8 +798,10 @@ test('migratePdfUrls: only confirmed copies are rewritten (review CR-3)', () => { name: 'b.pdf', url: '/houseplan_files/files/v_old1/b.pdf?v=2' }, ]; // сервер скопировал только a.pdf, причём переименовал из-за коллизии - const out = migratePdfUrls(pdfs, 'v_old1', 'dev99', { 'a.pdf': 'a (2).pdf' }); - assert.equal(out[0].url, '/houseplan_files/files/dev99/a%20(2).pdf?v=1'); + // collision names use only characters the content view accepts back in a + // request: ' (2)' was sanitised to '_2_' server-side and 404'd (v1.46.0) + const out = migratePdfUrls(pdfs, 'v_old1', 'dev99', { 'a.pdf': 'a-2.pdf' }); + assert.equal(out[0].url, '/houseplan_files/files/dev99/a-2.pdf?v=1'); assert.equal(out[1].url, pdfs[1].url, 'нескопированный файл ссылается на старую папку'); // пустой маппинг = ничего не переносим assert.deepEqual(migratePdfUrls(pdfs, 'v_old1', 'dev99', {}).map((p) => p.url), diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index bc25272..d130117 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -690,7 +690,7 @@ async def test_upload_never_overwrites_an_existing_attachment( names = sorted( n for n in await hass.async_add_executor_job(os.listdir, folder) if n.startswith("manual") ) - assert names == ["manual (2).pdf", "manual.pdf"] + assert names == ["manual-2.pdf", "manual.pdf"] got = await http.get(first.replace(CONTENT_URL, CONTENT_URL)) assert await got.read() == b"ONE", "the first file is untouched" diff --git a/tests_backend/test_validation.py b/tests_backend/test_validation.py index c6bdfd6..81207f8 100644 --- a/tests_backend/test_validation.py +++ b/tests_backend/test_validation.py @@ -392,14 +392,20 @@ def test_unique_filename_never_returns_a_taken_name(tmp_path): d.mkdir() assert unique_filename(d, "manual.pdf") == "manual.pdf" (d / "manual.pdf").write_bytes(b"x") - assert unique_filename(d, "manual.pdf") == "manual (2).pdf" - (d / "manual (2).pdf").write_bytes(b"x") - assert unique_filename(d, "manual.pdf") == "manual (3).pdf" + assert unique_filename(d, "manual.pdf") == "manual-2.pdf" + (d / "manual-2.pdf").write_bytes(b"x") + assert unique_filename(d, "manual.pdf") == "manual-3.pdf" # no extension, and a name that needs sanitising (d / "readme").write_bytes(b"x") - assert unique_filename(d, "readme") == "readme (2)" + assert unique_filename(d, "readme") == "readme-2" assert unique_filename(d, "../../etc/passwd") == "passwd" + # A generated name has to survive the sanitiser the CONTENT VIEW applies to + # the request, or the file is written and then never served: " (2)" became + # "_2_" and every collision-renamed attachment 404'd. + for candidate in ("manual-2.pdf", "manual-3.pdf", "readme-2"): + assert v.sanitize_filename(candidate) == candidate + def _acfg(*pairs): return {"markers": [{"id": f"m{i}", "pdfs": [{"url": f"/api/houseplan/content/files/{p}"}]}