fix(exporter): refuse symlinks at file targets and skip tests on Windows
Address Copilot review on #1156: - Per-file symlink check via new _safe_open_for_write() helper. Uses O_NOFOLLOW on POSIX (close TOCTOU window between islink check and open) and falls back to islink + open on Windows. Applied to room files and index.md, mirroring the existing dir-level check. - Tests now wrap os.symlink() in _try_symlink_or_skip() so Windows without Developer Mode and restricted CI sandboxes skip rather than hard-fail. Added two regression tests for the file-level cases (room file, index.md).
This commit is contained in:
+27
-2
@@ -11,6 +11,7 @@ Streams drawers in paginated batches so memory usage stays bounded
|
|||||||
regardless of palace size.
|
regardless of palace size.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import errno
|
||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
from collections import defaultdict
|
from collections import defaultdict
|
||||||
@@ -40,6 +41,30 @@ def _reject_symlink(path: str, label: str) -> None:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _safe_open_for_write(path: str, mode: str, encoding: str = "utf-8"):
|
||||||
|
"""Open a file for writing, refusing to follow a symlink at the target path.
|
||||||
|
|
||||||
|
On POSIX (O_NOFOLLOW available) the open itself fails with ELOOP if path is
|
||||||
|
a symlink — closing the TOCTOU window between an islink check and the open.
|
||||||
|
On platforms without O_NOFOLLOW (Windows), pre-checks ``os.path.islink``,
|
||||||
|
which is narrower than no check at all.
|
||||||
|
"""
|
||||||
|
o_nofollow = getattr(os, "O_NOFOLLOW", 0)
|
||||||
|
if o_nofollow:
|
||||||
|
flags = os.O_WRONLY | os.O_CREAT | o_nofollow
|
||||||
|
flags |= os.O_APPEND if "a" in mode else os.O_TRUNC
|
||||||
|
try:
|
||||||
|
fd = os.open(path, flags, 0o600)
|
||||||
|
except OSError as e:
|
||||||
|
if e.errno == errno.ELOOP:
|
||||||
|
raise ValueError(f"refusing to write: {path!r} is a symbolic link.") from None
|
||||||
|
raise
|
||||||
|
return os.fdopen(fd, mode, encoding=encoding)
|
||||||
|
if os.path.islink(path):
|
||||||
|
raise ValueError(f"refusing to write: {path!r} is a symbolic link.")
|
||||||
|
return open(path, mode, encoding=encoding)
|
||||||
|
|
||||||
|
|
||||||
def export_palace(palace_path: str, output_dir: str, format: str = "markdown") -> dict:
|
def export_palace(palace_path: str, output_dir: str, format: str = "markdown") -> dict:
|
||||||
"""Export all palace drawers as markdown files organized by wing/room.
|
"""Export all palace drawers as markdown files organized by wing/room.
|
||||||
|
|
||||||
@@ -118,7 +143,7 @@ def export_palace(palace_path: str, output_dir: str, format: str = "markdown") -
|
|||||||
key = (wing, room)
|
key = (wing, room)
|
||||||
is_new = key not in opened_rooms
|
is_new = key not in opened_rooms
|
||||||
|
|
||||||
with open(room_path, "a" if not is_new else "w", encoding="utf-8") as f:
|
with _safe_open_for_write(room_path, "a" if not is_new else "w") as f:
|
||||||
if is_new:
|
if is_new:
|
||||||
f.write(f"# {wing} / {room}\n\n")
|
f.write(f"# {wing} / {room}\n\n")
|
||||||
opened_rooms.add(key)
|
opened_rooms.add(key)
|
||||||
@@ -168,7 +193,7 @@ def export_palace(palace_path: str, output_dir: str, format: str = "markdown") -
|
|||||||
index_lines.append("")
|
index_lines.append("")
|
||||||
|
|
||||||
index_path = os.path.join(output_dir, "index.md")
|
index_path = os.path.join(output_dir, "index.md")
|
||||||
with open(index_path, "w", encoding="utf-8") as f:
|
with _safe_open_for_write(index_path, "w") as f:
|
||||||
f.write("\n".join(index_lines))
|
f.write("\n".join(index_lines))
|
||||||
|
|
||||||
stats = {
|
stats = {
|
||||||
|
|||||||
+61
-2
@@ -136,6 +136,22 @@ def test_export_empty_palace():
|
|||||||
shutil.rmtree(tmpdir, ignore_errors=True)
|
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||||
|
|
||||||
|
|
||||||
|
def _try_symlink_or_skip(target: str, link: str):
|
||||||
|
"""Create a symlink, skipping the test if the runtime forbids it.
|
||||||
|
|
||||||
|
Windows without Developer Mode/admin and some restricted CI sandboxes
|
||||||
|
refuse os.symlink with OSError or NotImplementedError. The exporter
|
||||||
|
hardening is meaningful only where symlinks can be created at all, so
|
||||||
|
skipping is preferable to a hard failure.
|
||||||
|
"""
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
try:
|
||||||
|
os.symlink(target, link)
|
||||||
|
except (OSError, NotImplementedError) as e:
|
||||||
|
pytest.skip(f"symlink creation not supported in this environment: {e}")
|
||||||
|
|
||||||
|
|
||||||
def test_export_refuses_symlinked_output_dir():
|
def test_export_refuses_symlinked_output_dir():
|
||||||
"""A symlink at the output path must not be followed (defense-in-depth)."""
|
"""A symlink at the output path must not be followed (defense-in-depth)."""
|
||||||
import pytest
|
import pytest
|
||||||
@@ -146,7 +162,7 @@ def test_export_refuses_symlinked_output_dir():
|
|||||||
decoy_target = os.path.join(tmpdir, "decoy_target")
|
decoy_target = os.path.join(tmpdir, "decoy_target")
|
||||||
os.makedirs(decoy_target)
|
os.makedirs(decoy_target)
|
||||||
output_dir = os.path.join(tmpdir, "export")
|
output_dir = os.path.join(tmpdir, "export")
|
||||||
os.symlink(decoy_target, output_dir)
|
_try_symlink_or_skip(decoy_target, output_dir)
|
||||||
|
|
||||||
with pytest.raises(ValueError, match="symbolic link"):
|
with pytest.raises(ValueError, match="symbolic link"):
|
||||||
export_palace(palace_path, output_dir)
|
export_palace(palace_path, output_dir)
|
||||||
@@ -168,7 +184,7 @@ def test_export_refuses_symlinked_wing_dir():
|
|||||||
os.makedirs(decoy_target)
|
os.makedirs(decoy_target)
|
||||||
output_dir = os.path.join(tmpdir, "export")
|
output_dir = os.path.join(tmpdir, "export")
|
||||||
os.makedirs(output_dir)
|
os.makedirs(output_dir)
|
||||||
os.symlink(decoy_target, os.path.join(output_dir, "alpha"))
|
_try_symlink_or_skip(decoy_target, os.path.join(output_dir, "alpha"))
|
||||||
|
|
||||||
with pytest.raises(ValueError, match="symbolic link"):
|
with pytest.raises(ValueError, match="symbolic link"):
|
||||||
export_palace(palace_path, output_dir)
|
export_palace(palace_path, output_dir)
|
||||||
@@ -176,3 +192,46 @@ def test_export_refuses_symlinked_wing_dir():
|
|||||||
assert os.listdir(decoy_target) == []
|
assert os.listdir(decoy_target) == []
|
||||||
finally:
|
finally:
|
||||||
shutil.rmtree(tmpdir, ignore_errors=True)
|
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||||
|
|
||||||
|
|
||||||
|
def test_export_refuses_symlinked_room_file():
|
||||||
|
"""A symlink pre-placed at a room file path must not be followed."""
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
tmpdir = tempfile.mkdtemp()
|
||||||
|
try:
|
||||||
|
palace_path = _setup_palace(tmpdir)
|
||||||
|
decoy_target = os.path.join(tmpdir, "decoy_target.md")
|
||||||
|
Path(decoy_target).write_text("untouched\n", encoding="utf-8")
|
||||||
|
output_dir = os.path.join(tmpdir, "export")
|
||||||
|
os.makedirs(os.path.join(output_dir, "alpha"))
|
||||||
|
_try_symlink_or_skip(decoy_target, os.path.join(output_dir, "alpha", "backend.md"))
|
||||||
|
|
||||||
|
with pytest.raises(ValueError, match="symbolic link"):
|
||||||
|
export_palace(palace_path, output_dir)
|
||||||
|
|
||||||
|
# Decoy file must remain unchanged — open did not follow the symlink.
|
||||||
|
assert Path(decoy_target).read_text(encoding="utf-8") == "untouched\n"
|
||||||
|
finally:
|
||||||
|
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||||
|
|
||||||
|
|
||||||
|
def test_export_refuses_symlinked_index_file():
|
||||||
|
"""A symlink pre-placed at output_dir/index.md must not be followed."""
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
tmpdir = tempfile.mkdtemp()
|
||||||
|
try:
|
||||||
|
palace_path = _setup_palace(tmpdir)
|
||||||
|
decoy_target = os.path.join(tmpdir, "decoy_index.md")
|
||||||
|
Path(decoy_target).write_text("untouched\n", encoding="utf-8")
|
||||||
|
output_dir = os.path.join(tmpdir, "export")
|
||||||
|
os.makedirs(output_dir)
|
||||||
|
_try_symlink_or_skip(decoy_target, os.path.join(output_dir, "index.md"))
|
||||||
|
|
||||||
|
with pytest.raises(ValueError, match="symbolic link"):
|
||||||
|
export_palace(palace_path, output_dir)
|
||||||
|
|
||||||
|
assert Path(decoy_target).read_text(encoding="utf-8") == "untouched\n"
|
||||||
|
finally:
|
||||||
|
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||||
|
|||||||
Reference in New Issue
Block a user