diff --git a/docs/guides/windows-install.md b/docs/guides/windows-install.md index 0f027623201..c8e7ea5f326 100644 --- a/docs/guides/windows-install.md +++ b/docs/guides/windows-install.md @@ -172,6 +172,7 @@ while the other 503s. Concretely: | Feature | Status on Windows | |---------|-------------------| | Core gateway / chat / dashboard | works — a source install with a built `website/dist` is linked into `src/kiro_crew/static/dist` at gateway start via a **directory junction** (`platform_compat.symlink_or_junction`), which needs no privilege; a symlink there would need `SeCreateSymbolicLinkPrivilege` and would leave a non-elevated install serving the "not built" page | +| Theme-pack install, detail, assets, overlays, topbars, and removal | works — opened pack files are contained with `GetFinalPathNameByHandleW`; descriptor resolution fails closed instead of trusting a pathname-only check | | LLM cron jobs (the `message` kind) | works | | Script cron jobs | need the `agent.sandbox_allow_unsandboxed_exec` opt-in above — they run through `wrap_argv`, which fail-closes where no OS sandbox backend exists. Without it the job fails with a message naming that setting (it no longer raises an uncaught error) | | Command cron jobs (`sh -c "…"`) | not supported on Windows — the stored command is vetted under POSIX-sh semantics, and Windows ships no shell whose language matches: cmd.exe is not POSIX at all, and Git-for-Windows's `sh.exe` is bash and performs brace expansion that hides `cat ~/.a{w,w}s/credentials` from the vet. The job fails-closed with an explanation. Use a **script cron** or an LLM `message` cron on this platform | diff --git a/docs/system-specs/common/testing-conventions.md b/docs/system-specs/common/testing-conventions.md index 4d33dd6bf44..75322acb6ee 100644 --- a/docs/system-specs/common/testing-conventions.md +++ b/docs/system-specs/common/testing-conventions.md @@ -76,6 +76,11 @@ an unconditional skip drops the whole assertion on Windows. Reach for a skip onl where the *link kind itself* is the subject (a file symlink's `lstat` mode bits, say), and then still pair it with a Windows counterpart. +The shared symlink capability probe skips only when Windows reports +`ERROR_PRIVILEGE_NOT_HELD` (WinError 1314), or when the runtime has no symlink +API. Other filesystem errors propagate so a broken fixture cannot silently +remove the containment assertion from the test run. + ### Patch the defining module, not a re-export `monkeypatch.setattr`/`patch` rebind a NAME in one module namespace. Code diff --git a/docs/system-specs/modules/themes.md b/docs/system-specs/modules/themes.md index 829d2f295be..482d5ddc016 100644 --- a/docs/system-specs/modules/themes.md +++ b/docs/system-specs/modules/themes.md @@ -67,7 +67,11 @@ runtime scoper still removes the pin, so the preference is protected either way. 1. **Source** — a local directory (moved/copied) or an https `github.com` repo shallow-cloned server-side (`_clone_github`, `--depth 1`, 30s timeout, host - allowlist). + allowlist). The clone spawns through the sandbox chokepoint, which fails + **closed** where no OS sandbox backend exists: that refusal answers `503` + with `code: "theme_install_sandbox_unavailable"`, never an unsandboxed + retry — the URL is user-influenced and `git clone` executes remote content. + A **local** source spawns nothing, so it stays available on such a host. 2. **Stage** — the source is copied into a private staging snapshot (`.install-staging-`) via a per-file, symlink-rejecting, byte-bounded loop (`_copy_installed_theme`). The source dir remains @@ -115,6 +119,12 @@ predate this subsystem and remain the color-theme surface.) - **Locked CSP** — overlay/topbar responses carry a fixed `Content-Security-Policy` including a `sandbox` directive; asset responses carry `X-Content-Type-Options: nosniff` and a content-type allowlist. +- **Descriptor-pinned containment** — pack install and serving resolve the + opened file descriptor before trusting bytes: `/proc/self/fd` on Linux, + `fcntl.F_GETPATH` on macOS, and `GetFinalPathNameByHandleW` on Windows. The + resolved path must remain inside the pack root; an unavailable or failed + resolution rejects the read rather than falling back to a pathname-only + check. - **postMessage allowlist** — the parent (`ThemeExperienceLayer.tsx`) accepts only `theme:resize`, `theme:sound`, `theme:visibility`, and `theme:state` messages from a pack iframe; all others are dropped. diff --git a/error-code-baseline.json b/error-code-baseline.json index a54a9d1d1a4..60b67159660 100644 --- a/error-code-baseline.json +++ b/error-code-baseline.json @@ -1,11 +1,11 @@ { "_comment": "Error responses without a machine-readable `code`, per file. Generated - regenerate with `python test/test_error_code_contract.py --update`. This is both the CI ratchet and the Track B worklist: drive `missing_code` to zero, one PR per file or per directory, moving each frontend consumer in the same PR. Never raise a number to make CI pass. See test/test_error_code_contract.py for what each bucket means and which false negatives it accepts.", "_totals": { - "missing_code": 1399, + "missing_code": 1398, "opaque_body": 18, "dynamic_status": 43 }, - "_compliant": 758, + "_compliant": 765, "files": { "apps/builtins/auto_research/handlers.py": { "dynamic_status": 1, @@ -166,7 +166,7 @@ }, "dashboard/handlers/themes.py": { "dynamic_status": 4, - "missing_code": 24 + "missing_code": 23 }, "dashboard/handlers/updates.py": { "missing_code": 9 diff --git a/src/kiro_crew/dashboard/handlers/themes.py b/src/kiro_crew/dashboard/handlers/themes.py index a7580bdb31b..c5a93512821 100644 --- a/src/kiro_crew/dashboard/handlers/themes.py +++ b/src/kiro_crew/dashboard/handlers/themes.py @@ -61,31 +61,18 @@ ) from kiro_crew.executors import discovery_executor from kiro_crew.hooks import safe_read_file_bytes_nolink -from kiro_crew.sandbox import resource_limit_preexec, sandboxed_spawn_argv +from kiro_crew.sandbox import ( + SandboxUnavailableError, + resource_limit_preexec, + sandboxed_spawn_argv, +) from kiro_crew.security import ( is_sensitive_path, redact_credentials, redact_exfiltration_urls, ) -# Theme install/serve traverses the O_NOFOLLOW + fd-real-path chokepoint in -# hooks (safe_read_file_bytes_nolink), which has no Windows implementation -# (_fd_real_path returns None there -> fail-closed on every read). Rather than -# fail opaquely, gate the pack routes with an honest 501 on Windows. -# Tracked: kirodotdev/KiroCrew#311. The editor custom-record (.json) CRUD -# paths never touch that chokepoint, so they are intentionally NOT gated. -_THEMES_WIN_UNSUPPORTED = os.name == "nt" - - -def _win_unsupported_response() -> web.Response: - """501 for pack routes that rely on the POSIX-only nolink chokepoint.""" - return web.json_response( - { - "error": "theme packs are not yet supported on Windows " - "(tracked: kirodotdev/KiroCrew#311)" - }, - status=501, - ) +_THEME_GIT_SANDBOX_UNAVAILABLE = "theme install sandbox is unavailable on this server" def _list_themes_sync() -> list[dict[str, Any]]: @@ -240,9 +227,15 @@ def _clone_github(url: str, dest: Path) -> str | None: # content, so route through the sandbox chokepoint (OS filesystem isolation # + credential-scrubbed env) and apply the fork-bomb/resource ceiling via # preexec_fn — same discipline as git_coord._git. - argv, env, cleanup = sandboxed_spawn_argv( - ["git", "clone", "--depth", "1", "--quiet", "--", url, str(dest)] - ) + try: + argv, env, cleanup = sandboxed_spawn_argv( + ["git", "clone", "--depth", "1", "--quiet", "--", url, str(dest)] + ) + except SandboxUnavailableError: + # Translate the typed sandbox refusal at this boundary. In particular, + # Windows has no process-sandbox backend, but local theme installs and + # every descriptor-contained read route remain supported there. + return _THEME_GIT_SANDBOX_UNAVAILABLE try: proc = subprocess.run( argv, @@ -404,7 +397,8 @@ def _do_install(stype: Any, source: dict[str, Any]) -> tuple[dict[str, Any] | No else: return None, "source.type must be 'local' or 'github'", 400 if err or src is None: - return None, err or "invalid source", 400 + status = 503 if err == _THEME_GIT_SANDBOX_UNAVAILABLE else 400 + return None, err or "invalid source", status # ── Stage-first (TOCTOU class fix) ── # The source dir stays writable by its owner throughout, so a @@ -527,9 +521,6 @@ async def api_themes_install(request: web.Request) -> web.Response: Fetch/move -> validate (data + structure) -> register as ``_themes_dir()//``. """ - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() - # Governance admission gate: installing a pack ingests third-party content # (local move or server-side git clone) and serves sandboxed JS into the # dashboard, so an enterprise POLICY must be able to ban it wholesale @@ -575,7 +566,10 @@ async def api_themes_install(request: web.Request) -> web.Response: discovery_executor(), _do_install, stype, source ) if err or theme is None: - return web.json_response({"error": err or "install failed"}, status=status) + payload = {"error": err or "install failed"} + if err == _THEME_GIT_SANDBOX_UNAVAILABLE: + payload["code"] = "theme_install_sandbox_unavailable" + return web.json_response(payload, status=status) return web.json_response({"ok": True, "slug": theme["slug"], "theme": theme}) @@ -598,9 +592,6 @@ async def api_theme_detail(request: web.Request) -> web.Response: ) return web.json_response({"ok": True}) if dir_target.is_dir(): - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() - # Recursive delete of a many-file theme dir is blocking; run off-loop. # Acquire the per-slug install lock (same key _do_install stages/swaps # under) so we never rmtree mid-reinstall and race its stage→rename; @@ -676,8 +667,6 @@ def _update_locked() -> dict: return web.json_response({"error": "failed to read theme"}, status=500) return web.json_response(data) if dir_target.is_dir(): - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() summary, err = await loop.run_in_executor( discovery_executor(), _validate_theme_dir, dir_target ) @@ -737,8 +726,6 @@ def _theme_html_response(text: str) -> web.Response: async def api_theme_asset(request: web.Request) -> web.Response: """GET /api/theme/{slug}/assets/{path} — serve a static theme asset.""" - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() target, err = _resolve_theme_asset( request.match_info["slug"], request.match_info.get("path", "") ) @@ -767,8 +754,6 @@ async def api_theme_asset(request: web.Request) -> web.Response: async def api_theme_overlay(request: web.Request) -> web.Response: """GET /api/theme/{slug}/overlay/{id} — serve overlay HTML (id = file stem).""" - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() oid = request.match_info["id"].lower() if not oid or _safe_theme_slug(oid) != oid: return web.json_response({"error": "invalid overlay id"}, status=400) @@ -788,8 +773,6 @@ async def api_theme_overlay(request: web.Request) -> web.Response: async def api_theme_topbar(request: web.Request) -> web.Response: """GET /api/theme/{slug}/topbar/{mode} — serve topbar HTML (mode dark|light).""" - if _THEMES_WIN_UNSUPPORTED: - return _win_unsupported_response() mode = request.match_info["mode"] if mode not in ("dark", "light"): return web.json_response({"error": "mode must be dark or light"}, status=400) diff --git a/src/kiro_crew/hooks.py b/src/kiro_crew/hooks.py index 5141d6e9120..e9df8dc7850 100644 --- a/src/kiro_crew/hooks.py +++ b/src/kiro_crew/hooks.py @@ -1798,8 +1798,9 @@ def safe_read_file_bytes_nolink( ``st_nlink > 1`` or a non-regular file type is rejected. When ``within_root`` is given, the OPENED descriptor's real path - (via ``/proc/self/fd`` on Linux, ``fcntl.F_GETPATH`` on macOS) must resolve - inside that root and must not be sensitive. ``O_NOFOLLOW`` only guards the + (via ``/proc/self/fd`` on Linux, ``fcntl.F_GETPATH`` on macOS, or + ``GetFinalPathNameByHandleW`` on Windows) must resolve inside that root and + must not be sensitive. ``O_NOFOLLOW`` only guards the FINAL path component — a nested directory swapped for a symlink between the tree walk and the open would silently escape the approved tree. The fd-path check is pinned to the inode actually opened, so no check-to-use diff --git a/test/test_dashboard_themes_coverage.py b/test/test_dashboard_themes_coverage.py index d573800f607..d482f2479a3 100644 --- a/test/test_dashboard_themes_coverage.py +++ b/test/test_dashboard_themes_coverage.py @@ -6,18 +6,16 @@ the blocking workers they offload to the discovery pool (``_list_themes_sync``, ``_do_install``), the local/GitHub source resolvers, and the refusal branches — invalid JSON, slug traversal, governance denial, read-only installed packs, -unsupported asset types, and the honest 501 the pack routes return on Windows. +unsupported asset types, and cross-platform pack install/serving. Every test points ``KIROCREW_HOME`` at ``tmp_path`` so ``_themes_dir()`` resolves inside the sandbox: nothing is written outside it. No network, no git, no real subprocess — ``_clone_github``'s spawn is replaced with a stub so only its URL guard and error mapping are exercised. -Platform notes: the pack routes are gated behind ``_THEMES_WIN_UNSUPPORTED`` -because the install/serve reads funnel through the POSIX-only nolink chokepoint -(``safe_read_file_bytes_nolink``). Tests that only need the non-nolink half pin -that flag to ``False`` so they run on Windows too; tests that genuinely need the -chokepoint (install promotion, asset bytes, symlink refusals) are skipped there. +Platform notes: install and serving exercise the real descriptor-containment +chokepoint on every supported OS. Tests that need to create symbolic links run +where the process has that capability, including privileged Windows CI runners. """ from __future__ import annotations @@ -33,13 +31,9 @@ from aiohttp.test_utils import make_mocked_request import kiro_crew.platform.governance_profiles as gov_mod +from conftest import requires_symlinks from kiro_crew.dashboard.handlers import themes as th -_NOT_POSIX = os.name == "nt" -_posix_only = pytest.mark.skipif( - _NOT_POSIX, reason="needs the POSIX-only nolink read chokepoint / symlinks" -) - # _validate_theme_data only *requires* --bg/--text/--accent per mode. _VALID_VARS: dict[str, dict[str, str]] = { "dark": {"--bg": "#000000", "--text": "#ffffff", "--accent": "#3366ff"}, @@ -115,18 +109,6 @@ def themes_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: return d -@pytest.fixture -def pack_routes_enabled(monkeypatch: pytest.MonkeyPatch) -> None: - """Pin the platform gate off so the non-nolink half runs on Windows too.""" - monkeypatch.setattr(th, "_THEMES_WIN_UNSUPPORTED", False) - - -@pytest.fixture -def pack_routes_win(monkeypatch: pytest.MonkeyPatch) -> None: - """Pin the platform gate on to exercise the honest-501 branches anywhere.""" - monkeypatch.setattr(th, "_THEMES_WIN_UNSUPPORTED", True) - - @pytest.fixture def allow_install(monkeypatch: pytest.MonkeyPatch) -> None: """Governance admits the install (default-allow standalone), deterministically.""" @@ -142,17 +124,6 @@ class _Allowed: ) -# ── the Windows gate ─────────────────────────────────────────────────────── - - -class TestWinUnsupportedResponse: - def test_is_501_naming_the_tracking_issue(self) -> None: - resp = th._win_unsupported_response() - assert resp.status == 501 - assert "Windows" in _body(resp)["error"] - assert "#311" in _body(resp)["error"] - - # ── _list_themes_sync ────────────────────────────────────────────────────── @@ -205,7 +176,7 @@ def test_dot_prefixed_staging_and_backup_dirs_are_never_listed( _write_json(themes_dir / ".lcars.old-abc" / "theme.json", {"name": "Y"}) assert th._list_themes_sync() == [] - @_posix_only + @requires_symlinks def test_symlinked_directory_is_never_listed(self, themes_dir: Path, tmp_path: Path) -> None: real = _make_pack(tmp_path / "outside") (themes_dir / "linked").symlink_to(real, target_is_directory=True) @@ -324,7 +295,7 @@ def test_non_directory_is_rejected(self, tmp_path: Path) -> None: assert src is None assert err is not None and "not a directory" in err - @_posix_only + @requires_symlinks def test_symlinked_source_is_rejected(self, tmp_path: Path) -> None: real = _make_pack(tmp_path / "real") link = tmp_path / "link" @@ -412,6 +383,22 @@ def test_missing_git_binary_is_reported( err = th._clone_github("https://github.com/o/r", tmp_path / "clone") assert err == "git is not available on the server" + def test_sandbox_unavailable_is_reported_without_spawning( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + def _unavailable(*args: object, **kwargs: object) -> object: + raise th.SandboxUnavailableError( + "no backend", kind="no_backend", detail="unsupported host" + ) + + monkeypatch.setattr(th, "sandboxed_spawn_argv", _unavailable) + seen = self._stub_run(monkeypatch, AssertionError("must not spawn")) + + err = th._clone_github("https://github.com/o/r", tmp_path / "clone") + + assert err == th._THEME_GIT_SANDBOX_UNAVAILABLE + assert seen == [] + def test_timeout_is_reported( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -584,7 +571,6 @@ def test_unsafe_slug_fails_closed(self, themes_dir: Path) -> None: _write_json(target, {}) assert th._read_theme_bytes_nolink("../escape", target) is None - @_posix_only def test_reads_a_regular_file_inside_the_pack(self, themes_dir: Path) -> None: target = themes_dir / "lcars" / "theme.json" _write_json(target, {"slug": "lcars"}) @@ -612,8 +598,21 @@ def test_github_source_error_is_a_400(self, themes_dir: Path) -> None: assert theme is None and status == 400 assert err is not None and "only https" in err + def test_github_sandbox_unavailable_is_a_503( + self, themes_dir: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr( + th, + "_clone_github", + lambda url, dest: th._THEME_GIT_SANDBOX_UNAVAILABLE, + ) + + theme, err, status = th._do_install("github", {"url": "https://github.com/o/r"}) + + assert theme is None and err == th._THEME_GIT_SANDBOX_UNAVAILABLE + assert status == 503 + -@_posix_only class TestDoInstallPromotion: def test_source_containing_the_themes_dir_is_rejected(self, themes_dir: Path) -> None: # The themes directory lives under KIROCREW_HOME, so installing FROM @@ -633,6 +632,7 @@ def test_invalid_pack_is_rejected_without_staging_residue( assert theme is None and status == 400 and err assert list(themes_dir.glob(".install-staging-*")) == [] + @requires_symlinks def test_symlinked_subdirectory_is_refused(self, themes_dir: Path, tmp_path: Path) -> None: src = _make_pack(tmp_path / "packlink") elsewhere = tmp_path / "elsewhere" @@ -645,6 +645,7 @@ def test_symlinked_subdirectory_is_refused(self, themes_dir: Path, tmp_path: Pat assert err is not None and "symlinked directory" in err assert list(themes_dir.glob(".install-staging-*")) == [] + @requires_symlinks def test_non_regular_entry_is_refused(self, themes_dir: Path, tmp_path: Path) -> None: src = _make_pack(tmp_path / "packdangle") # A dangling symlink is walked as a FILE entry, so it is refused by the @@ -703,16 +704,10 @@ def test_installing_the_installed_directory_onto_itself_is_rejected( class TestApiThemesInstall: - @pytest.mark.asyncio - async def test_windows_returns_an_honest_501(self, pack_routes_win: None) -> None: - resp = await th.api_themes_install(_request("POST", "/api/themes/install")) - assert resp.status == 501 - @pytest.mark.asyncio async def test_policy_denial_is_403_and_audited( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: class _Denied: @@ -737,7 +732,6 @@ class _Denied: async def test_governance_failure_fails_closed( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: def _boom(*a: object, **k: object) -> object: @@ -757,7 +751,7 @@ def _boom(*a: object, **k: object) -> object: @pytest.mark.asyncio async def test_malformed_json_is_400( - self, themes_dir: Path, pack_routes_enabled: None, allow_install: None + self, themes_dir: Path, allow_install: None ) -> None: resp = await th.api_themes_install( _request("POST", "/api/themes/install", body=None) @@ -771,7 +765,6 @@ async def test_missing_source_object_is_400( self, payload: object, themes_dir: Path, - pack_routes_enabled: None, allow_install: None, ) -> None: resp = await th.api_themes_install( @@ -784,7 +777,6 @@ async def test_missing_source_object_is_400( async def test_worker_error_and_status_pass_through( self, themes_dir: Path, - pack_routes_enabled: None, allow_install: None, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -801,11 +793,37 @@ async def test_worker_error_and_status_pass_through( assert resp.status == 409 assert _body(resp)["error"] == "nope" + @pytest.mark.asyncio + async def test_sandbox_unavailable_has_retryable_status_and_code( + self, + themes_dir: Path, + allow_install: None, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setattr( + th, + "_do_install", + lambda stype, source: (None, th._THEME_GIT_SANDBOX_UNAVAILABLE, 503), + ) + + resp = await th.api_themes_install( + _request( + "POST", + "/api/themes/install", + body={"source": {"type": "github", "url": "https://github.com/o/r"}}, + ) + ) + + assert resp.status == 503 + assert _body(resp) == { + "error": th._THEME_GIT_SANDBOX_UNAVAILABLE, + "code": "theme_install_sandbox_unavailable", + } + @pytest.mark.asyncio async def test_success_returns_the_descriptor( self, themes_dir: Path, - pack_routes_enabled: None, allow_install: None, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -857,23 +875,12 @@ async def test_removes_a_custom_record(self, themes_dir: Path) -> None: assert not (themes_dir / "sunset.json").exists() @pytest.mark.asyncio - async def test_removes_an_installed_pack( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_removes_an_installed_pack(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") resp = await th.api_theme_detail(_detail("DELETE", "lcars")) assert resp.status == 200 and _body(resp) == {"ok": True} assert not (themes_dir / "lcars").exists() - @pytest.mark.asyncio - async def test_pack_removal_is_501_on_windows( - self, themes_dir: Path, pack_routes_win: None - ) -> None: - _make_pack(themes_dir / "lcars") - resp = await th.api_theme_detail(_detail("DELETE", "lcars")) - assert resp.status == 501 - assert (themes_dir / "lcars").is_dir() - @pytest.mark.asyncio async def test_unknown_slug_is_404(self, themes_dir: Path) -> None: resp = await th.api_theme_detail(_detail("DELETE", "ghost")) @@ -962,7 +969,7 @@ async def test_corrupt_record_is_500(self, themes_dir: Path) -> None: @pytest.mark.asyncio async def test_installed_pack_detail_carries_level_and_assets( - self, themes_dir: Path, pack_routes_enabled: None + self, themes_dir: Path ) -> None: _make_pack(themes_dir / "lcars") resp = await th.api_theme_detail(_detail("GET", "lcars")) @@ -975,17 +982,7 @@ async def test_installed_pack_detail_carries_level_and_assets( assert "assets" in payload @pytest.mark.asyncio - async def test_installed_pack_detail_is_501_on_windows( - self, themes_dir: Path, pack_routes_win: None - ) -> None: - _make_pack(themes_dir / "lcars") - resp = await th.api_theme_detail(_detail("GET", "lcars")) - assert resp.status == 501 - - @pytest.mark.asyncio - async def test_invalid_installed_pack_is_500( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_invalid_installed_pack_is_500(self, themes_dir: Path) -> None: # A directory with a manifest but no formatVersion fails validation on # the READ path too — the route reports 500 rather than a silent empty. _write_json(themes_dir / "lcars" / "theme.json", {"name": "LCARS"}) @@ -997,7 +994,6 @@ async def test_invalid_installed_pack_is_500( async def test_manifest_read_failure_falls_back_to_an_empty_manifest( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: _make_pack(themes_dir / "lcars") @@ -1033,30 +1029,19 @@ def _asset_request(slug: str, path: str) -> web.Request: class TestApiThemeAsset: @pytest.mark.asyncio - async def test_windows_returns_501(self, pack_routes_win: None) -> None: - resp = await th.api_theme_asset(_asset_request("lcars", "branding/logo.svg")) - assert resp.status == 501 - - @pytest.mark.asyncio - async def test_unsafe_slug_is_400( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_unsafe_slug_is_400(self, themes_dir: Path) -> None: resp = await th.api_theme_asset(_asset_request("../etc", "logo.svg")) assert resp.status == 400 assert _body(resp)["error"] == "invalid theme slug" @pytest.mark.asyncio - async def test_missing_asset_is_404( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_missing_asset_is_404(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") resp = await th.api_theme_asset(_asset_request("lcars", "branding/logo.svg")) assert resp.status == 404 @pytest.mark.asyncio - async def test_unsupported_extension_is_400( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_unsupported_extension_is_400(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") _write_text(themes_dir / "lcars" / "notes.txt", "hello\n") resp = await th.api_theme_asset(_asset_request("lcars", "notes.txt")) @@ -1067,7 +1052,6 @@ async def test_unsupported_extension_is_400( async def test_unreadable_bytes_are_404( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: _make_pack(themes_dir / "lcars") @@ -1077,9 +1061,8 @@ async def test_unreadable_bytes_are_404( assert resp.status == 404 @pytest.mark.asyncio - @_posix_only async def test_serves_the_asset_with_a_locked_down_csp( - self, themes_dir: Path, pack_routes_enabled: None + self, themes_dir: Path ) -> None: _make_pack(themes_dir / "lcars") _write_text(themes_dir / "lcars" / "branding" / "logo.svg", "") @@ -1100,32 +1083,23 @@ def _overlay_request(slug: str, oid: str) -> web.Request: class TestApiThemeOverlay: - @pytest.mark.asyncio - async def test_windows_returns_501(self, pack_routes_win: None) -> None: - resp = await th.api_theme_overlay(_overlay_request("lcars", "scanner")) - assert resp.status == 501 - @pytest.mark.asyncio @pytest.mark.parametrize("oid", ["", "../etc", "a/b", "a.b"]) async def test_unsafe_overlay_id_is_400( - self, oid: str, themes_dir: Path, pack_routes_enabled: None + self, oid: str, themes_dir: Path ) -> None: resp = await th.api_theme_overlay(_overlay_request("lcars", oid)) assert resp.status == 400 assert _body(resp)["error"] == "invalid overlay id" @pytest.mark.asyncio - async def test_unsafe_slug_is_400( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_unsafe_slug_is_400(self, themes_dir: Path) -> None: resp = await th.api_theme_overlay(_overlay_request("Bad", "scanner")) assert resp.status == 400 assert _body(resp)["error"] == "invalid theme slug" @pytest.mark.asyncio - async def test_missing_overlay_is_404( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_missing_overlay_is_404(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") resp = await th.api_theme_overlay(_overlay_request("lcars", "scanner")) assert resp.status == 404 @@ -1134,7 +1108,6 @@ async def test_missing_overlay_is_404( async def test_unreadable_overlay_is_404( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: _make_pack(themes_dir / "lcars") @@ -1144,10 +1117,7 @@ async def test_unreadable_overlay_is_404( assert resp.status == 404 @pytest.mark.asyncio - @_posix_only - async def test_serves_overlay_html_sandboxed( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_serves_overlay_html_sandboxed(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") _write_text( themes_dir / "lcars" / "overlays" / "scanner.html", "
scan
" @@ -1167,32 +1137,23 @@ def _topbar_request(slug: str, mode: str) -> web.Request: class TestApiThemeTopbar: - @pytest.mark.asyncio - async def test_windows_returns_501(self, pack_routes_win: None) -> None: - resp = await th.api_theme_topbar(_topbar_request("lcars", "dark")) - assert resp.status == 501 - @pytest.mark.asyncio @pytest.mark.parametrize("mode", ["", "DARK", "sepia", "../dark"]) async def test_unknown_mode_is_400( - self, mode: str, themes_dir: Path, pack_routes_enabled: None + self, mode: str, themes_dir: Path ) -> None: resp = await th.api_theme_topbar(_topbar_request("lcars", mode)) assert resp.status == 400 assert _body(resp)["error"] == "mode must be dark or light" @pytest.mark.asyncio - async def test_unsafe_slug_is_400( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_unsafe_slug_is_400(self, themes_dir: Path) -> None: resp = await th.api_theme_topbar(_topbar_request("Bad", "dark")) assert resp.status == 400 assert _body(resp)["error"] == "invalid theme slug" @pytest.mark.asyncio - async def test_missing_topbar_is_404( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_missing_topbar_is_404(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") resp = await th.api_theme_topbar(_topbar_request("lcars", "light")) assert resp.status == 404 @@ -1201,7 +1162,6 @@ async def test_missing_topbar_is_404( async def test_unreadable_topbar_is_404( self, themes_dir: Path, - pack_routes_enabled: None, monkeypatch: pytest.MonkeyPatch, ) -> None: _make_pack(themes_dir / "lcars") @@ -1211,10 +1171,7 @@ async def test_unreadable_topbar_is_404( assert resp.status == 404 @pytest.mark.asyncio - @_posix_only - async def test_serves_topbar_html_sandboxed( - self, themes_dir: Path, pack_routes_enabled: None - ) -> None: + async def test_serves_topbar_html_sandboxed(self, themes_dir: Path) -> None: _make_pack(themes_dir / "lcars") _write_text(themes_dir / "lcars" / "topbar" / "dark.html", "
bar
") resp = await th.api_theme_topbar(_topbar_request("lcars", "dark")) diff --git a/test/test_hooks_coverage.py b/test/test_hooks_coverage.py index 9f5120a25d0..541a1edb07f 100644 --- a/test/test_hooks_coverage.py +++ b/test/test_hooks_coverage.py @@ -100,7 +100,10 @@ def _same(a: str, b: str) -> bool: returns the long one, so a raw string compare passes on POSIX and fails only on Windows. """ - return os.path.realpath(a) == os.path.realpath(b) + def _normalized(path: str) -> str: + return os.path.normcase(os.path.normpath(os.path.realpath(path))) + + return _normalized(a) == _normalized(b) def _identity(path: Path) -> tuple[int, int]: @@ -484,9 +487,13 @@ def test_resolves_an_open_descriptor(self, tmp_path): got = _fd_real_path(fd) finally: os.close(fd) - # A platform with no supported mechanism fails closed (None); where one - # exists it must name the same file. - assert got is None or _same(got, str(f)) + # Windows is a supported descriptor-containment platform. Other + # platforms without a supported mechanism still fail closed (None). + if os.name == "nt": + assert got is not None + assert _same(got, str(f)) + else: + assert got is None or _same(got, str(f)) class TestSafeReadPrefix: diff --git a/test/test_symlink_capability_probe.py b/test/test_symlink_capability_probe.py new file mode 100644 index 00000000000..7260af5c8f1 --- /dev/null +++ b/test/test_symlink_capability_probe.py @@ -0,0 +1,97 @@ +"""Regression tests for the root conftest's symlink capability probe. + +The probe answers one question — can THIS process create a real symlink — and +it answers it at conftest IMPORT time (``_HAS_SYMLINKS = _can_create_symlink()``). +That placement is what fixes its error contract: a raise here is not a loud +signal on one test, it is a collection error that takes the whole session down, +including every test that never touches a symlink. So the probe treats any +failure to create one as "this host cannot", and the capability tests it guards +skip rather than the suite failing to start. + +The pull the other way is real and worth naming: swallowing everything means an +unexpected filesystem fault silently disables symlink coverage instead of +reporting itself. That trade is settled in favour of the suite still running, +because the alternative failure mode is total and hits contributors who changed +nothing in this area — an errno allowlist has to enumerate every way a +filesystem can decline, and the ones it misses (a read-only or full temp dir, an +overlay/network mount returning EINVAL) are exactly the environments least +likely to have been anticipated. +""" + +from __future__ import annotations + +import errno + +import pytest + +import conftest as root_conftest + + +@pytest.mark.parametrize( + "error_number", + [errno.EPERM, errno.EACCES, getattr(errno, "EOPNOTSUPP", errno.EPERM), errno.ENOSYS], +) +def test_capability_errnos_disable_symlink_tests_without_aborting_collection( + monkeypatch: pytest.MonkeyPatch, error_number: int +) -> None: + """The ordinary ways a host declines: reported as "no capability".""" + + def _unsupported(*args: object, **kwargs: object) -> None: + raise OSError(error_number, "symlink capability unavailable") + + monkeypatch.setattr(root_conftest.os, "symlink", _unsupported) + + assert root_conftest._can_create_symlink() is False + + +def test_windows_privilege_error_disables_symlink_tests( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The Windows shape: ``SeCreateSymbolicLinkPrivilege`` not held. + + Carries an unrelated errno alongside ``winerror`` 1314, which is what an + errno-keyed probe would miss — the privilege case has to survive on the + ``OSError`` type alone. + """ + unavailable = OSError(errno.EIO, "privilege not held") + unavailable.winerror = 1314 # type: ignore[attr-defined] + + def _unsupported(*args: object, **kwargs: object) -> None: + raise unavailable + + monkeypatch.setattr(root_conftest.os, "symlink", _unsupported) + + assert root_conftest._can_create_symlink() is False + + +def test_an_unanticipated_oserror_still_only_disables_the_capability( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """THE ONE THAT MATTERS: an errno nobody enumerated must not abort the run. + + A read-only or full temp dir, or an overlay/network mount that declines with + something outside the usual set, has to degrade to "no symlinks here" like + any other decline. Letting it propagate turns a capability probe into a + conftest import error, and a contributor who touched none of this gets a + suite that will not collect at all. + """ + + def _broken(*args: object, **kwargs: object) -> None: + raise OSError(errno.EIO, "unexpected filesystem failure") + + monkeypatch.setattr(root_conftest.os, "symlink", _broken) + + assert root_conftest._can_create_symlink() is False + + +def test_a_host_that_has_no_symlink_call_at_all_is_handled( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``os.symlink`` is not guaranteed to exist or be implemented everywhere.""" + + def _absent(*args: object, **kwargs: object) -> None: + raise NotImplementedError("no symlink on this platform") + + monkeypatch.setattr(root_conftest.os, "symlink", _absent) + + assert root_conftest._can_create_symlink() is False diff --git a/test/test_theme_install.py b/test/test_theme_install.py index 6964d3341e9..f82c85d74a6 100644 --- a/test/test_theme_install.py +++ b/test/test_theme_install.py @@ -31,6 +31,7 @@ import pytest +from conftest import requires_symlinks from kiro_crew.dashboard.handlers.themes import ( _atomic_write_theme_json, _clone_github, @@ -977,6 +978,7 @@ def test_descriptor_exposes_persona_info(self, tmp_path: Path) -> None: assert info["chars"] == len(text) assert info["text"] == text + @requires_symlinks def test_descriptor_rejects_symlinked_persona(self, tmp_path: Path) -> None: # TOCTOU symlink-swap: persona.md replaced by a symlink to a file outside # the theme dir must NOT be read. The nolink chokepoint (O_NOFOLLOW + @@ -1589,98 +1591,6 @@ def test_symlink_swapped_asset_refused( assert _read_theme_bytes_nolink("mypack", target) is None -class TestWindowsGate: - """Arbiter item 4: the pack routes traverse hooks' POSIX-only - O_NOFOLLOW + fd-real-path chokepoint (no Windows impl), so they honestly - 501 on Windows instead of 500-ing. The flag is monkeypatched True here; - on this (POSIX) host it defaults False, so the rest of the suite exercises - the normal path. The editor custom-record CRUD paths are NOT gated.""" - - @staticmethod - def _req(**match_info: object) -> object: - import types - - async def _json() -> dict: - return {"source": {"type": "local", "path": "/tmp/does-not-matter"}} - - r = types.SimpleNamespace(match_info=match_info) - r.json = _json # type: ignore[attr-defined] - return r - - def test_install_501_on_windows(self, monkeypatch: pytest.MonkeyPatch) -> None: - import asyncio - - from kiro_crew.dashboard.handlers import themes as th_mod - - monkeypatch.setattr(th_mod, "_THEMES_WIN_UNSUPPORTED", True) - resp = asyncio.run(th_mod.api_themes_install(self._req())) - assert resp.status == 501 - assert b"not yet supported on Windows" in resp.body - assert b"KiroCrew#311" in resp.body - - @pytest.mark.parametrize( - "handler,match_info", - [ - ("api_theme_asset", {"slug": "wintheme", "path": "branding/logo.svg"}), - ("api_theme_overlay", {"slug": "wintheme", "id": "scanner"}), - ("api_theme_topbar", {"slug": "wintheme", "mode": "dark"}), - ], - ) - def test_serving_routes_501_on_windows( - self, monkeypatch: pytest.MonkeyPatch, handler: str, match_info: dict - ) -> None: - import asyncio - - from kiro_crew.dashboard.handlers import themes as th_mod - - monkeypatch.setattr(th_mod, "_THEMES_WIN_UNSUPPORTED", True) - resp = asyncio.run(getattr(th_mod, handler)(self._req(**match_info))) - assert resp.status == 501 - assert b"not yet supported on Windows" in resp.body - - def test_delete_installed_dir_501_on_windows( - self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch - ) -> None: - import asyncio - import types - - import kiro_crew.dashboard.theme_validate as tv_mod - from kiro_crew.dashboard.handlers import themes as th_mod - - monkeypatch.setattr(tv_mod, "config_dir", lambda: tmp_path) - slug = "wintheme" - d = tv_mod._installed_theme_dir(slug) - d.mkdir(parents=True) - (d / "theme.json").write_text( - '{"name": "W", "level": 0, "formatVersion": 1}', encoding="utf-8" - ) - monkeypatch.setattr(th_mod, "_THEMES_WIN_UNSUPPORTED", True) - req = types.SimpleNamespace(method="DELETE", match_info={"slug": slug}) - resp = asyncio.run(th_mod.api_theme_detail(req)) - assert resp.status == 501 - assert b"not yet supported on Windows" in resp.body - # The gate short-circuits before any rmtree — the dir is untouched. - assert d.is_dir() - - def test_flag_false_does_not_gate(self, monkeypatch: pytest.MonkeyPatch) -> None: - # With the flag False (the POSIX default), install is NOT short-circuited: - # it proceeds to parse the body, so invalid JSON yields the normal 400. - import asyncio - import types - - from kiro_crew.dashboard.handlers import themes as th_mod - - assert th_mod._THEMES_WIN_UNSUPPORTED is False # POSIX host default - - async def _bad_json() -> dict: - raise ValueError("no body") - - req = types.SimpleNamespace(match_info={}) - req.json = _bad_json # type: ignore[attr-defined] - resp = asyncio.run(th_mod.api_themes_install(req)) - assert resp.status == 400 - - class TestCssParserCorpus: """Shared-corpus guard for the two theme-CSS parsers (PR #107 arbiter item).