diff --git a/src/kiro_crew/apps/builtins/design_critique/backend/routes.py b/src/kiro_crew/apps/builtins/design_critique/backend/routes.py index 797f3f41ee0..b029c9f4194 100644 --- a/src/kiro_crew/apps/builtins/design_critique/backend/routes.py +++ b/src/kiro_crew/apps/builtins/design_critique/backend/routes.py @@ -330,6 +330,16 @@ def _served_signature(build_dir: Path) -> _ServedToken | None: symlinked directory nor a symlinked file as one, and skips dot-entries, so none of those is ever reachable over the preview server — and none is signed here. + Matching that test needs ``is_link_or_junction``, not ``os.path.islink``. Node's + Dirent reports a Windows junction as a symbolic link and therefore serves nothing + behind it, while ``os.path.islink`` calls the same junction a plain directory — + so the bare check descended into it and signed bytes the server will not serve. + Nothing outside ``build_dir`` is disclosed by that (the token is a digest, and it + is never served), but it is exactly the "false mismatches and needless + re-captures" this paragraph rules out, it lets an unserved tree raise + ``newest_mtime_ns`` for the mid-capture check, and its files count against + ``_SIGNATURE_MAX_FILES``. + Returns ``None`` whenever the served set cannot be read in full: missing, unreadable, or larger than ``_SIGNATURE_MAX_FILES``. A caller MUST treat ``None`` as "unknown" rather than "unchanged" and refuse reuse — a token over part of a @@ -353,7 +363,8 @@ def _fail(exc: OSError) -> None: dirs[:] = sorted( d for d in dirs - if not d.startswith(".") and not os.path.islink(os.path.join(root, d)) + if not d.startswith(".") + and not platform_compat.is_link_or_junction(os.path.join(root, d)) ) # Each directory's own mtime feeds ``newest_mtime_ns`` but NOT the digest. # It has to feed the former because a DELETION leaves no file behind to @@ -366,7 +377,7 @@ def _fail(exc: OSError) -> None: rel_root = os.path.relpath(root, build_dir) for name in sorted(files): path = os.path.join(root, name) - if name.startswith(".") or os.path.islink(path): + if name.startswith(".") or platform_compat.is_link_or_junction(path): continue seen += 1 if seen > _SIGNATURE_MAX_FILES: diff --git a/src/kiro_crew/apps/builtins/design_critique/tests/test_backend_routes.py b/src/kiro_crew/apps/builtins/design_critique/tests/test_backend_routes.py index e071dd82e8b..d0dbbbaca62 100644 --- a/src/kiro_crew/apps/builtins/design_critique/tests/test_backend_routes.py +++ b/src/kiro_crew/apps/builtins/design_critique/tests/test_backend_routes.py @@ -13,9 +13,21 @@ import pytest from aiohttp import web +from kiro_crew import platform_compat from kiro_crew.apps.builtins.design_critique import register_routes from kiro_crew.apps.builtins.design_critique.backend import routes +if platform_compat.IS_WINDOWS: + import _winapi + + # Resolved at runtime, not as a typed attribute: typeshed guards CreateJunction + # behind sys.platform == "win32", so a direct reference is an attr-defined error + # when mypy checks this in-package test file on Linux. Same shape as + # platform_compat's own `getattr(os.path, "isjunction", None)`. + _create_junction = getattr(_winapi, "CreateJunction", None) +else: # pragma: no cover - junctions exist on Windows only + _create_junction = None + def test_register_routes_mounts_the_three_endpoints() -> None: app = web.Application() @@ -1185,6 +1197,64 @@ def test_served_signature_ignores_what_the_server_will_not_serve(tmp_path) -> No assert behind is not None and behind.digest == sig.digest +def _make_dir_link(link: Path, target: Path) -> None: + # A directory SYMLINK needs SeCreateSymbolicLinkPrivilege on Windows (WinError + # 1314 unelevated), which is why the test above can only skip there. A junction + # needs no privilege and is the reparse point a real build tree would carry, so + # the Windows half of this contract stays exercised instead of being skipped. + # + # A junction, never "a junction OR a symlink": os.symlink SUCCEEDS on a runner + # with Developer Mode on, and a symlink is the shape os.path.islink already + # refused. Degrading to it would turn the Windows red-before green for the + # wrong reason. + if platform_compat.IS_WINDOWS: + assert _create_junction is not None, "_winapi.CreateJunction missing on Windows" + _create_junction(str(target), str(link)) + return + link.symlink_to(target, target_is_directory=True) + + +def test_served_signature_refuses_a_junctioned_directory(tmp_path) -> None: + # capture-build.mjs's Dirent test reports a junction as a symbolic link, so it + # walks nothing behind one and the preview server serves nothing from it. The + # token has to agree, and `os.path.islink` cannot make it agree: it calls a + # junction a plain directory, so the walk descended and signed bytes that are + # not served. Windows is the only platform with junctions and the only one where + # the symlink test above can be skipped for want of a privilege. + build = tmp_path / "dist" + _build_tree(build) + sig = routes._served_signature(build) + assert sig is not None + + outside = tmp_path / "outside" + outside.mkdir() + (outside / "extra.js").write_text("x", encoding="utf-8") + _make_dir_link(build / "vendor", outside) + + # Guard the guard: on Windows the link must really be the shape `os.path.islink` + # misreads. Without this the test could pass on a plain directory and prove + # nothing about the fix. + if platform_compat.IS_WINDOWS: + assert not os.path.islink(build / "vendor") + assert os.path.isdir(build / "vendor") + assert platform_compat.is_link_or_junction(build / "vendor") + + # Only the DIGEST can hold still across the link's creation: that writes a new + # entry into dist/, and newest_mtime_ns reads directory mtimes on purpose. + linked = routes._served_signature(build) + assert linked is not None and linked.digest == sig.digest + + # A change BEHIND the link moves neither field. Rewriting a file leaves its + # parent directory's mtime alone, so newest_mtime_ns is pinned exactly here — + # it feeds the discover-time mid-capture check, which an unserved tree must not + # be able to trip. + (outside / "extra.js").write_text("changed-and-longer", encoding="utf-8") + _bump(outside / "extra.js") + behind = routes._served_signature(build) + assert behind is not None and behind.digest == sig.digest + assert behind.newest_mtime_ns == linked.newest_mtime_ns + + def test_probe_build_dir_rejects_a_path_outside_the_project(tmp_path) -> None: # A manifest path is only ever stat()ed, but a token taken over an unrelated # tree would stand still and permit reuse of a stale PNG for the whole TTL.