diff --git a/emrg/sandbox/fence.py b/emrg/sandbox/fence.py index 0b82b0d5..7e4b82f9 100644 --- a/emrg/sandbox/fence.py +++ b/emrg/sandbox/fence.py @@ -53,7 +53,7 @@ from emrg.sandbox.grants import GrantKindNotEnforced from emrg.sandbox.policy import DANGER_FULL_ACCESS, SandboxPolicy from emrg.sandbox.roots import canonical_path, named_grants, writable_roots -from emrg.tools.file_policy import protected_paths +from emrg.tools.file_policy import protected_hit def file_refusal(file_path: str, policy: SandboxPolicy) -> str | None: @@ -79,7 +79,8 @@ def file_refusal(file_path: str, policy: SandboxPolicy) -> str | None: # unconfined. return None target = canonical_path(file_path) - if target in protected_paths(): + protected = protected_hit(target) + if protected is not None: # The host's own daemon state (``~/.emrg/config.toml`` and its four # neighbours). Containment already refuses all five in every deployment # this repo runs, because none of them sits inside a session's workspace @@ -87,12 +88,21 @@ def file_refusal(file_path: str, policy: SandboxPolicy) -> str | None: # session whose cwd is ``~`` would make the OS fence grant them. So this # is the one place the in-process fence is **stricter** than the kernel # profile rather than identical to it: it can refuse a write bash would - # be allowed to make, never the reverse. The list stays in - # ``emrg/tools/file_policy.py`` because this module and - # ``emrg/sandbox/roots.py`` are its two readers — the in-process fence - # and the kernel profile refuse the same five paths because they ask the - # same derivation, not because two lists happen to agree. - return f"{policy.mode} sandbox: blocked write to protected daemon file {file_path!r}" + # be allowed to make, never the reverse. The list itself lives in + # ``emrg/tools/file_policy.py``, and ``protected_hit`` there is the one + # predicate this module and ``emrg/sandbox/roots.py`` both ask — the + # in-process fence and the kernel profile refuse the same five paths + # because they ask one derivation, not because two lists happen to agree. + # + # Asked by file identity, not by comparing canonical strings: on a volume + # that folds case two spellings are one file, and `realpath` hands back + # whichever spelling it was given — so the string test admitted + # `~/.emrg/CONFIG.TOML` while refusing `~/.emrg/config.toml` (#2551). + resolved = f" (resolves to {protected!r})" if protected != target else "" + return ( + f"{policy.mode} sandbox: blocked write to protected daemon " + f"file {file_path!r}{resolved}" + ) try: # The enforcement question is asked here rather than left to # ``writable_roots``, which agents paths and so has no answer for a diff --git a/emrg/sandbox/roots.py b/emrg/sandbox/roots.py index 582da04f..1ade75b5 100644 --- a/emrg/sandbox/roots.py +++ b/emrg/sandbox/roots.py @@ -510,14 +510,16 @@ def judge_extra_root( visibly, rather than assembled out of a file grant. 4. **Not the home directory** — same shape, one level in. The grant is meant to be the size of what was named. - 5. **Never over a protected daemon state file** (:func:`~emrg.tools.file_policy.protected_paths`) + 5. **Never over a protected daemon state file** (:func:`~emrg.tools.file_policy.protected_hit`) — those five are unreachable today by *geometry* (none sits inside a session workspace or a temp area). A host-named root makes them reachable as an ordinary write, so the guard has to be explicit rather than - incidental. The kernel profiles would grant them; the in-process fence - would refuse (``fence.file_refusal`` is deliberately the stricter of the - two) — that split is the "user-visible strange" the rant names, and this - rule is what keeps it unreachable. + incidental. It asks by **file identity**, so the file is refused under + every spelling that reaches it and the directories above it too — a + spelling a volume folds names the same file. The kernel profiles would + grant them; the in-process fence would refuse (``fence.file_refusal`` is + deliberately the stricter of the two) — that split is the "user-visible + strange" the rant names, and this rule is what keeps it unreachable. 6. **The path must exist** — a grant is spelled differently for a directory (``subpath``) and a file (``literal``), and for a name that is not there yet that choice has to be guessed. Refusing it means every stored root @@ -549,7 +551,7 @@ def judge_extra_root( # Imported here rather than at module scope: ``emrg.session`` imports this # module for ``canonical_path``, and that happens before the tool package is # on the import path. The list still has one home; this is a reference. - from emrg.tools.file_policy import protected_paths + from emrg.tools.file_policy import protected_hit if kind is not None and kind not in (GRANT_DIRECTORY, GRANT_FILE, GRANT_REGISTRY): return ExtraRootVerdict("", refusal=( @@ -579,12 +581,15 @@ def judge_extra_root( f"{path!r} is the home directory — name the file or directory you " "mean, so the grant is the size you asked for" )) - for protected in protected_paths(): - if canonical == protected or protected.startswith(canonical.rstrip(os.sep) + os.sep): - return ExtraRootVerdict(canonical, refusal=( - f"{path!r} would grant {protected!r}, a protected daemon state " - "file — those are refused at every tier" - )) + if (protected := protected_hit(canonical, as_directory=True)) is not None: + # By file identity, for the file *and* for any directory above it: on a + # volume that folds case the string test admitted `~/.EMRG` beside a + # protected `~/.emrg/config.toml`, which grants the very files this rule + # exists to keep unreachable (#2551, requirement 4). + return ExtraRootVerdict(canonical, refusal=( + f"{path!r} would grant {protected!r}, a protected daemon state " + "file — those are refused at every tier" + )) if not os.path.exists(canonical): return ExtraRootVerdict(canonical, refusal=( f"{path!r} does not exist — a root names what is there, because a " diff --git a/emrg/tools/file_policy.py b/emrg/tools/file_policy.py index a15129a6..f1ec1877 100644 --- a/emrg/tools/file_policy.py +++ b/emrg/tools/file_policy.py @@ -74,6 +74,65 @@ def protected_paths() -> list[str]: return out +def same_file(a: str, b: str) -> bool: + """Whether two paths name one file, by the filesystem's own identity. + + ``os.path.samefile`` compares ``st_dev``/``st_ino`` — the file-id pair on + Windows — which is the only answer that survives a spelling a volume folds. + On a volume that folds case (macOS's APFS default, and NTFS) ``config.toml`` + and ``CONFIG.TOML`` are **one file with two spellings**, while + ``os.path.realpath`` returns whichever spelling it was handed: a comparison + of canonical *strings* answers "different file" for one file (#2551). + + A path that is not there has no identity to compare, so this answers False: + it is not the (existing) file it is being compared with, and each caller's + other rules still give that path its own verdict. + + :param a: one path. + :param b: the other. + :returns: whether both name the same file on disk. + """ + if a == b: + return True + try: + return os.path.samefile(a, b) + except (OSError, ValueError): + return False + + +def protected_hit(canonical: str, *, as_directory: bool = False) -> str | None: + """The protected daemon file this canonical path reaches, compared by identity. + + The comparison is :func:`same_file` rather than string equality, because a + protected file is protected under **every** spelling that reaches it — the + alias a volume folds, and the hard link a case-sensitive volume cannot fold + away. Asking by identity is also what keeps a genuinely different file whose + name differs only in case reachable: ``normcase`` is the identity function on + POSIX, and lowering both sides would refuse it (#2551, requirement 2). + + :param canonical: the target, already canonicalized by the caller. + :param as_directory: judge it as a root that would be granted as a tree, so + a directory **above** a protected file is a hit as well as the file + itself. The ancestor walk is by identity for the same reason the file + test is: ``~/.EMRG`` is ``~/.emrg`` on a folding volume. + :returns: the protected path that was reached, or ``None``. + """ + for protected in protected_paths(): + if same_file(canonical, protected): + return protected + if not as_directory: + continue + node = os.path.dirname(protected) + while True: + if same_file(canonical, node): + return protected + parent = os.path.dirname(node) + if parent == node: + break + node = parent + return None + + def trusted_write_zones() -> list[str]: """Canonicalized write roots that a ``workspace-write`` session may target. diff --git a/tests/test_file_policy_has_one_home.py b/tests/test_file_policy_has_one_home.py index 3a901411..f560d735 100644 --- a/tests/test_file_policy_has_one_home.py +++ b/tests/test_file_policy_has_one_home.py @@ -52,7 +52,8 @@ #: The helpers the *mechanics* keep in ``emrg/tools/file_policy.py``: where a #: relative spelling resolves, what "inside" means, and the host's protected #: files. The policy itself is no longer one of them. -MECHANICS = ("resolve_file_target", "is_within", "is_absolute_path", "protected_paths") +MECHANICS = ("resolve_file_target", "is_within", "is_absolute_path", "protected_paths", + "same_file", "protected_hit") def _run(coro): @@ -104,9 +105,140 @@ def test_the_legacy_predicates_are_gone_everywhere(name): assert not hasattr(module, name), f"{module.__name__} re-binds {name}" -def test_the_fence_imports_the_home_s_protected_paths(): - """The fence names the host's protected files through the home, not around it.""" - assert fence.protected_paths is file_policy.protected_paths +def test_the_fence_asks_the_home_which_files_are_protected(): + """The fence names the host's protected files through the home, not around it. + + The name this pins moved when the comparison became an identity one (#2551): + the fence used to import the *list* and compare canonical strings, and it now + imports the lookup the home states the rule in. The requirement is unchanged + — one home, and the fence bound to the home's own object rather than a copy. + """ + assert fence.protected_hit is file_policy.protected_hit + + +def _fence_with_protected(monkeypatch, protected: Path, workspace: Path): + """A ``workspace-write`` policy whose protected list is one temporary file. + + ``protected_paths()`` is derived from the *real* home, and no test here may + touch it; the seam is the home's own function, so what a test arms is a list + of temporary paths and the derivation keeps its own guard elsewhere. The + workspace is the same temporary directory on purpose: the file tools' fence + is deliberately stricter than containment, and a target the workspace grants + is the only place that difference is visible. + """ + monkeypatch.setattr( + file_policy, "protected_paths", lambda: [os.path.realpath(protected)] + ) + return resolve_policy(mode="workspace-write", workspace_root=str(workspace)) + + +def test_a_second_name_for_a_protected_file_is_refused(tmp_path, monkeypatch): + """One file, one verdict — by identity, not by the spelling that reached it. + + A hard link asks this question with case folding taken out: two names, one + inode, and ``realpath`` returns each name as written, so the string + comparison this replaces answered "inside the workspace and not on the list" + and allowed the write. No volume can help here — a hard link is one file on + every filesystem this repo runs on — so this is the arm that holds on a + case-sensitive runner too (#2551). + """ + victim = tmp_path / "config.toml" + victim.write_text("host state", encoding="utf-8") + alias = tmp_path / "not-the-config" + os.link(victim, alias) + policy = _fence_with_protected(monkeypatch, victim, tmp_path) + + assert file_refusal(str(victim), policy) is not None + refusal = file_refusal(str(alias), policy) + + assert refusal is not None and "protected daemon file" in refusal + # A judgment names the file it judged: the alias and the protected file + # differ in spelling, and a refusal printing only the spelling would send its + # reader looking for a second file that is not there. + # + # The clause is read, not grepped for the bare path. The message renders the + # resolved identity with `!r`, so on Windows it carries escaped backslashes + # and `realpath` is *not* a substring of it: the bare-path form of this + # assertion passed here and failed `test-windows` (measured on #2552's run + # 39962749524, `1 failed, 5346 passed`). Comparing the clause with the same + # rendering asks the question the sentence asks — is the file it names the + # one that was reached — on every platform. + resolved = refusal.split("(resolves to ", 1)[1].rstrip(")") + assert resolved == repr(os.path.realpath(victim)), refusal + + # The same construction on a file the list does not name. Identity is what + # the answer is keyed to, so a second name for an ordinary file stays + # writable: the arm is "this IS a protected file", never "this file has two + # names" — and that direction is measurable on every volume, unlike the + # case-folding one below. + mine = tmp_path / "notes.md" + mine.write_text("mine", encoding="utf-8") + mine_alias = tmp_path / "notes-alias.md" + os.link(mine, mine_alias) + assert file_refusal(str(mine_alias), policy) is None + + +def test_a_case_variant_spelling_of_a_protected_file_is_refused(tmp_path, monkeypatch): + """The same question on the volume the defect was reported against. + + The volume is asked, not assumed: where it keeps the two spellings apart they + are two files and the old answer was already right, so the case is skipped + with that reason. On a volume that folds them — APFS's default, and NTFS — + ``CONFIG.TOML`` is ``config.toml``, and it was *allowed* before this fix + while its own spelling was refused (#2551, requirement 1). + """ + victim = tmp_path / "config.toml" + victim.write_text("host state", encoding="utf-8") + variant = tmp_path / "CONFIG.TOML" + if not file_policy.same_file(str(variant), str(victim)): + pytest.skip("this volume keeps CONFIG.TOML and config.toml apart as two files") + policy = _fence_with_protected(monkeypatch, victim, tmp_path) + + refusal = file_refusal(str(variant), policy) + + assert refusal is not None and "protected daemon file" in refusal + + +def test_a_distinct_file_whose_name_differs_only_in_case_stays_writable( + tmp_path, monkeypatch +): + """The other direction, and why the mechanism is identity rather than lowering. + + On a volume that tells the two names apart, ``notes.md`` beside ``NOTES.md`` + is a second file the session owns, and refusing it would be the false + positive that makes a case-insensitive comparison unusable on Linux: + ``normcase`` is the identity function on POSIX, so a blanket ``.lower()`` is + the shape this test refuses (#2551, requirement 2). Skipped where the volume + folds the names, because then there is no second file to ask about. + """ + victim = tmp_path / "config.toml" + victim.write_text("host state", encoding="utf-8") + sibling = tmp_path / "NOTES.md" + sibling.write_text("mine", encoding="utf-8") + shout = tmp_path / "notes.md" + if file_policy.same_file(str(shout), str(sibling)): + pytest.skip("this volume folds case, so notes.md and NOTES.md are one file") + policy = _fence_with_protected(monkeypatch, victim, tmp_path) + + assert file_refusal(str(shout), policy) is None + + +def test_a_target_that_is_not_there_still_gets_a_verdict(tmp_path, monkeypatch): + """An identity test cannot compare what is absent, and that must not read as a pass. + + ``same_file`` answers False for a path with no inode, which leaves the target + to the containment rule it was always judged by: inside the workspace it is + writable, outside every granted root it is refused, and neither answer raises + (#2551, requirement 3). + """ + victim = tmp_path / "config.toml" + victim.write_text("host state", encoding="utf-8") + policy = _fence_with_protected(monkeypatch, victim, tmp_path) + + assert file_refusal(str(tmp_path / "not-there-yet.txt"), policy) is None + outside = os.path.join(ABSOLUTE_ELSEWHERE, "not-there-either.txt") + refusal = file_refusal(outside, policy) + assert refusal is not None and "outside every root" in refusal @pytest.mark.parametrize( diff --git a/tests/test_sandbox_extra_roots.py b/tests/test_sandbox_extra_roots.py index 3bf7c87e..cdd5b012 100644 --- a/tests/test_sandbox_extra_roots.py +++ b/tests/test_sandbox_extra_roots.py @@ -56,6 +56,7 @@ ) from emrg.server.daemon import EmrgServer from emrg.session import Session +from emrg.tools import file_policy class _RecordingWs: @@ -298,6 +299,41 @@ def test_the_protected_file_itself_is_refused_too(outside): assert verdict.refusal and "agents.yml" in verdict.refusal +def test_a_root_naming_the_protected_directory_by_another_spelling_is_refused( + outside, tmp_path, monkeypatch +): + """The grant arm reads identity too, and the directory is where it matters. + + A root is granted as a tree, so the question is not only "is this the file" + but "does this name a directory above one". On a volume that folds case, + ``~/.EMRG`` beside a protected ``~/.emrg/config.toml`` is that directory, and + the string comparison this replaces admitted it — the grant the rule exists + to refuse (#2551, requirement 4). + + The volume is asked rather than assumed: where it keeps the two spellings + apart they are two directories, the grant is the size the host asked for, and + the case is skipped with that reason. The protected list is a temporary one, + because ``protected_paths()`` derives from the real home and no test here + touches it. + """ + ws, _ = outside + home = tmp_path / "home" + (home / ".emrg").mkdir(parents=True) + victim = home / ".emrg" / "config.toml" + victim.write_text("host state", encoding="utf-8") + variant = home / ".EMRG" + if not file_policy.same_file(str(variant), str(home / ".emrg")): + pytest.skip("this volume keeps .EMRG and .emrg apart as two directories") + monkeypatch.setattr( + file_policy, "protected_paths", lambda: [os.path.realpath(victim)] + ) + + verdict = judge_extra_root(str(variant), _ws_policy(ws)) + + assert verdict.refusal, verdict + assert "protected daemon state file" in verdict.refusal + + # ── §2 one derivation point ───────────────────────────────────────────────