Skip to content

Commit 932b074

Browse files
committed
Harden ZIP extractor against Windows drive-letter and backslash entries
Copilot review on #427 flagged that the path-traversal guard added in #426 was OS-dependent: on Windows, a crafted member like "addons/godot_ai/C:evil.txt" yields a relative pathlib.Path with a drive component, and `target_addon / rel` discards `target_addon`, writing outside the fixture directory. CI runs on Linux so the existing guard passed, but the harness also runs on developer Windows machines. Three-pronged fix: - Parse zip member names as `PurePosixPath` (ZIP spec uses `/`) so Windows host pathlib doesn't reinterpret drive-letter syntax. - Reject any member name containing a literal backslash up front (mirrors update_reload_runner.gd::_is_safe_zip_addon_file()). - Final containment check via `Path.is_relative_to()` on the resolved output path -- catches any OS-specific path-parsing quirk that sneaks past the parts-based check. New `tests/unit/test_self_update_fixture_extract.py` covers the clean-zip happy path, parent-traversal rejection, backslash rejection, zip-directory-entries are skipped, and entries outside the addons/godot_ai prefix are skipped. 912 pytest tests pass (+5). https://claude.ai/code/session_01VgXf3Lqv2ypt36g6EqpRYg
1 parent 4eda186 commit 932b074

2 files changed

Lines changed: 114 additions & 3 deletions

File tree

tests/integration/_self_update_fixture.py

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import subprocess
66
import zipfile
77
from importlib.machinery import SourceFileLoader
8-
from pathlib import Path
8+
from pathlib import Path, PurePosixPath
99
from types import ModuleType
1010

1111
import pytest
@@ -274,17 +274,37 @@ def assert_no_update_parse_errors(log: str) -> None:
274274

275275
def extract_addon_from_zip(zip_path: Path, target_addon: Path) -> None:
276276
target_addon.mkdir(parents=True)
277+
target_resolved = target_addon.resolve()
277278
with zipfile.ZipFile(zip_path) as zf:
278279
for info in zf.infolist():
279280
if not info.filename.startswith("addons/godot_ai/") or info.is_dir():
280281
continue
281-
rel = Path(info.filename).relative_to("addons/godot_ai")
282+
# ZIP spec uses POSIX-style forward-slash separators; parsing the
283+
# member name as PurePosixPath stops a Windows host from
284+
# reinterpreting an entry like "addons/godot_ai/C:evil.txt" as a
285+
# drive component (which would make `target_addon / rel` discard
286+
# the prefix and write outside the fixture).
287+
if "\\" in info.filename:
288+
raise ValueError(
289+
f"Refusing unsafe zip entry {info.filename!r}: contains backslash"
290+
)
291+
rel = PurePosixPath(info.filename).relative_to("addons/godot_ai")
282292
if rel.is_absolute() or any(part in ("", ".", "..") for part in rel.parts):
283293
raise ValueError(
284294
f"Refusing unsafe zip entry {info.filename!r}: relative path "
285295
f"{rel!s} contains absolute or traversal segments"
286296
)
287-
out = target_addon / rel
297+
out = target_addon.joinpath(*rel.parts)
298+
# Belt-and-suspenders containment: if the resolved output path
299+
# would escape target_addon (e.g. an OS-specific path-parsing
300+
# quirk we didn't anticipate), refuse the entry.
301+
candidate = out if out.exists() else target_resolved / Path(*rel.parts)
302+
resolved = candidate.resolve()
303+
if not resolved.is_relative_to(target_resolved):
304+
raise ValueError(
305+
f"Refusing unsafe zip entry {info.filename!r}: resolved "
306+
f"output {resolved!s} escapes target {target_resolved!s}"
307+
)
288308
out.parent.mkdir(parents=True, exist_ok=True)
289309
out.write_bytes(zf.read(info.filename))
290310

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
"""Unit tests for `tests/integration/_self_update_fixture.py::extract_addon_from_zip`.
2+
3+
The helper extracts release-zip entries into a fixture directory before the
4+
integration tests in `tests/integration/test_self_update_*.py` exercise the
5+
update flow. The runtime installer (`update_reload_runner.gd`) rejects unsafe
6+
zip entries via `_is_safe_zip_addon_file()`; this helper mirrors that
7+
defense in Python so a crafted/corrupt release zip can't escape the fixture
8+
sandbox on developer machines or CI runners.
9+
"""
10+
11+
from __future__ import annotations
12+
13+
import zipfile
14+
from pathlib import Path
15+
16+
import pytest
17+
18+
from tests.integration._self_update_fixture import extract_addon_from_zip
19+
20+
21+
def _write_zip(path: Path, members: dict[str, bytes]) -> None:
22+
with zipfile.ZipFile(path, "w") as zf:
23+
for name, data in members.items():
24+
zf.writestr(name, data)
25+
26+
27+
def test_clean_zip_extracts_files_to_target(tmp_path: Path) -> None:
28+
zip_path = tmp_path / "good.zip"
29+
_write_zip(
30+
zip_path,
31+
{
32+
"addons/godot_ai/plugin.cfg": b"[plugin]\nname=ok\n",
33+
"addons/godot_ai/sub/file.gd": b"pass",
34+
},
35+
)
36+
target = tmp_path / "target"
37+
extract_addon_from_zip(zip_path, target)
38+
assert (target / "plugin.cfg").read_bytes().startswith(b"[plugin]")
39+
assert (target / "sub" / "file.gd").read_bytes() == b"pass"
40+
41+
42+
def test_parent_traversal_entry_is_rejected(tmp_path: Path) -> None:
43+
zip_path = tmp_path / "bad.zip"
44+
_write_zip(
45+
zip_path,
46+
{
47+
"addons/godot_ai/plugin.cfg": b"[plugin]\nname=ok\n",
48+
"addons/godot_ai/../../escape.txt": b"escape",
49+
},
50+
)
51+
with pytest.raises(ValueError, match="absolute or traversal segments"):
52+
extract_addon_from_zip(zip_path, tmp_path / "target")
53+
54+
55+
def test_backslash_in_entry_name_is_rejected(tmp_path: Path) -> None:
56+
zip_path = tmp_path / "bad.zip"
57+
_write_zip(
58+
zip_path,
59+
{
60+
"addons/godot_ai/plugin.cfg": b"[plugin]\nname=ok\n",
61+
"addons/godot_ai/sub\\windows-style.gd": b"oops",
62+
},
63+
)
64+
with pytest.raises(ValueError, match="contains backslash"):
65+
extract_addon_from_zip(zip_path, tmp_path / "target")
66+
67+
68+
def test_directory_entries_are_skipped(tmp_path: Path) -> None:
69+
zip_path = tmp_path / "dirs.zip"
70+
with zipfile.ZipFile(zip_path, "w") as zf:
71+
zf.writestr("addons/godot_ai/sub/", b"")
72+
zf.writestr("addons/godot_ai/plugin.cfg", b"[plugin]\nname=ok\n")
73+
target = tmp_path / "target"
74+
extract_addon_from_zip(zip_path, target)
75+
assert (target / "plugin.cfg").is_file()
76+
assert not (target / "sub").exists()
77+
78+
79+
def test_entries_outside_addon_prefix_are_skipped(tmp_path: Path) -> None:
80+
zip_path = tmp_path / "mixed.zip"
81+
_write_zip(
82+
zip_path,
83+
{
84+
"addons/godot_ai/plugin.cfg": b"[plugin]\nname=ok\n",
85+
"README.md": b"not an addon file",
86+
"other/thing.txt": b"also not",
87+
},
88+
)
89+
target = tmp_path / "target"
90+
extract_addon_from_zip(zip_path, target)
91+
assert sorted(p.name for p in target.iterdir()) == ["plugin.cfg"]

0 commit comments

Comments
 (0)