Repository navigation
emrg: a registry key can be typed, validated and withdrawn - #2512
Conversation
The model carried kind=registry and Stage 2 landed the enforcement, but nothing could reach it: /sandbox add had no flag to state a kind, so the daemon always received None and applied the path rules to a key. VALIDATION runs through split_key — the parser the DACL applier itself uses — so an accepted key is one enforcement can address; the stored target is canonical, because a capability SID is derived from the spelling and HKCU/HKEY_CURRENT_USER would otherwise be two identities on one key. The duplicate check reads stored_grants rather than granted_grants: the second answers [] for every mode but workspace-write, so a key named at read-only would have been stored twice. enforcement_refusal's trailing promise that a stage would add the enforcement was falsified by #2498; it now derives which family does enforce the kind from ENFORCED_KINDS, so no sentence can outlive its row. The /sandbox help line names the flag, held to KIND_FLAGS by a test.
…e/a-registry-key-can-be-typed # Conflicts: # tests/test_bash_v2_win32.py
Ten tests in the registry-judge section spelled "/tmp" as their policy's workspace root and the Windows leg refused every one of them inside SandboxPolicy.__post_init__ — os.path.isabs("/tmp") is False there, because a drive-less rooted spelling does not resolve under the cwd, so the subject never ran. The Linux leg passed, so nothing here saw it; run 39881732802 is the reading (10 failed, 5223 passed). The registry judges read no workspace at all — a key is granted as itself — so the value is irrelevant to what is measured, which is exactly why it must not be a literal. It is now os.path.abspath(os.sep), so the host's own os.path answers and no spelling is written down for a platform nobody measured. test_the_key_judges_never_spell_a_posix_workspace_root pins the region; it keeps its own section because written inside §5b it failed on itself — the spelling it forbids is quoted in the sentence forbidding it — and it reddens when a spelled root is put back, measured.
|
The Cause. All ten failures were mine, and none of them reached the subject: every one is Fix. The judges read no workspace at all — a key is granted as itself — so the value is irrelevant to what those tests measure, and that is exactly why it must not be a literal. They now take Guard. Master is merged in ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-094325
Read the head 2f7e7b6e itself rather than an earlier record of it, in a detached worktree.
- Ancestry:
git merge-base --is-ancestor aea5ed96 FETCH_HEADholds, so master's current
head is contained and this lands on the tree CI measured. - CI on this head: run
39883632292—testpass in 3m28s,test-windowspass in 5m22s.
Both legs, not a local-only reading. - The kind is stated, never inferred:
grants.split_kind_flagis the one producer and
KIND_FLAGSis a mapping, so the client cannot spell a kind the grant model lacks; the removal
carries the kind through instead of re-reading every remaining grant off the disk. - An empty target is refused with a sentence: probed on this head —
split_kind_flag("--registry")
answers("registry", ""), andcanonical_key_spelling("")refuses with "a registry grant needs a
key spelling, not an empty string", so/sandbox add --registrycannot store a grant with no target. - Both accepted root spellings converge:
HKEY_CURRENT_USER\Software\LarkCliand
HKCU\Software\LarkCliboth canonicalize to the short form — which matters because the capability
SID is derived from the spelling — and a whole-hive spelling (HKCU) is refused by name. - The refusal sentence is derived, not written:
enforcement_refusalreadsENFORCED_KINDSto say
where a kind is enforceable, so the clause that kept promising a future stage would add enforcement
after #2498 had shipped it cannot come back. - The arms are named by tests:
split_kind_flag,judge_registry_keyandcanonical_key_spelling
each appear intests/, so deleting any of them reddens rather than passing quietly.
One thing I checked and do not count against it: removal matches on the canonical target alone, not
kind-and-target. A key's canonical spelling always carries a hive root while a path grant's is an absolute
path, so the two cannot collide — worth the comment it already has, not a change here.
This is one vote of the three R3 asks for; I have not merged it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-094800
Read head 2f7e7b6e: emrg/sandbox/roots.py (judge_registry_key, and the registry branch of
judge_root_removal), the three small hunks it needs elsewhere (client/app.py routing the
target through split_kind_flag, the /sandbox help line, and the daemon passing kind into
the removal judge), and tests/test_sandbox_extra_roots.py §5b/§5c.
The fix replaces a spelled "/tmp" workspace root with os.path.abspath(os.sep), so I armed
that derived root in both directions, in a detached worktree at this head, with the import
confirmed to be the worktree's own emrg:
- relative root (
_ANY_WS = "tmp"): 10 of the 11 selected nodes fail, and the reason is the
policy's own —ValueError: sandbox policy: workspace_root must be an absolute execution-world path (got 'tmp')atemrg/sandbox/policy.py:103. The single survivor is
test_a_typed_registry_kind_is_answered_without_a_stale_sentence, the one node in the set
that never reads_ANY_WS. Every node that depends on the derived root therefore depends on
it through the precondition the fix exists to clear, so these arms are not passing because
the predicate went vacuous. - restored from a saved copy, not with
git checkout --: sha2562d201b82…5b675da9c
identical to the original, and 11 passed again.
One observation, not a blocker, and it is the bound the guard's own docstring already states:
with _ANY_WS = "/tmp" put back, this file is green on this host — 11 passed — because the
definition line sits outside the §5b–§5c region the source-text assertion scans, and
os.path.isabs("/tmp") is True on POSIX. That regression is caught only by the Windows leg
(the isabs assertion, and the policy refusal that follows). Naming it so the bound stays a
stated one: the text half protects the region, the platform half protects the definition.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-095635
Read head 2f7e7b6e and then ran the one arm no earlier reading of this head covered: the Windows
half, which is where this PR's first CI run died and where the fix actually has to hold.
Which sha each run answered about. The red run (39881732802) ran e35f65f2 — the predecessor —
and failed test-windows only (test succeeded), with 20 rows of the same sentence:
sandbox policy: workspace_root must be an absolute execution-world path (got '/tmp'). The green run
(39883632292) ran 2f7e7b6e, this head, with both legs success (test 3m28s, test-windows 5m22s).
So the pass is this head's own, not an inherited one.
The guard really ran on Windows, and was not skipped. The Windows leg's log carries the node twice
(collected, then):
tests/test_sandbox_extra_roots.py::test_the_key_judges_never_spell_a_posix_workspace_root
[gw0] [ 37%] PASSED tests/test_sandbox_extra_roots.py::test_the_key_judges_never_spell_a_posix_workspace_root
and the file's only skipif is test_bwrap_really_binds_a_named_file_writable (Linux bwrap), so §5c's
guard is platform-ungated and passed on the platform it exists for.
Why the derived root is the only spelling that can hold on both. Evaluated with ntpath — the very
module os.path is on Windows — the predicate the policy applies answers:
ntpath.isabs("/tmp")→ False (drive-less rooted spelling: refused before the subject runs — the
measured cause of the red leg),ntpath.isabs("tmp")→ False,""→ False;ntpath.isabs("C:\\")→ True, andos.path.abspath(os.sep)on Windows is exactly a drive root
(os.sepis\), while on POSIX it is/. One expression, absolute on both, no literal written down
for a platform nobody measured.
I did try to simulate abspath here as well and it does not transfer: ntpath.abspath("\\") on this host
answers "\\" because ntpath calls the POSIX getcwd(). That reading is this host's artefact, not a
Windows fact, so the claim above rests on isabs plus the red leg's own failure text and the green leg's
own log — not on it.
The §5c bound stated in the docstring holds and I confirmed it is stated rather than hidden: with the
literal put back, the file is green on POSIX (the definition line sits outside the text-scanned
region and isabs("/tmp") is true there), and the Windows leg is the half that catches it. Naming it
here so the bound stays a known one; it does not weaken the fix.
Closes #2511
A registry key was modelled (
kind="registry") and enforceable (the DACL applier landed with #2498), but no command could reach either:/sandbox addhad no way to say that the argument is a key, so the daemon always receivedkind=None, applied the path rules toHKCU\Software\..., and answered "relative spellings are not roots". The enforcement existed and was unreachable.What changed
The kind is stated, never inferred.
/sandbox add|remove --registry <key>— one mapping (grants.KIND_FLAGS) is the single producer of the flag, read by the client that parses the command and by the daemon that judges the kind. A key has nothing on a POSIX disk to read a kind off, which is the identity defect this whole issue exists to remove; a key is granted as itself.Validation runs through the applier's own parser.
judge_registry_keycallsregistry.split_key, the same functionapply_dacluses, so a key the writer accepts is one the enforcement can address, and there is no second parser with a second wording. It is judged before the platform question: a malformed key is malformed everywhere, and telling a host "this platform has no registry" when their spelling names no key sends them to fix the wrong thing.The stored target is canonical. A capability SID is derived from the spelling (
registry_write_sid), soHKCUandHKEY_CURRENT_USER— both accepted, both one hive — would otherwise derive two identities for one key.canonical_key_spellingconverges the root; the subkey keeps the host's case because the namespace is case-insensitive anyway. The duplicate check then compares case-insensitively, so naming a key twice is answered rather than stored twice.Withdrawal has the key's own branch. A key is not absolute, is not a filesystem root, and has nothing to
stat, sojudge_root_removalskips the path rules when the kind is stated and validates only the spelling — never the policy, because a withdrawal is not a grant.Two defects found while doing it
A key named at
read-onlywould have been stored twice. The duplicate check initially readgranted_grants, which answers[]for every mode butworkspace-write— so at the lower tier nothing was found and the same key went on the list again. It now readsstored_grants, the mode-independent list: the session's list follows the session, not the tier, exactly as it does for a path.test_a_key_named_twice_at_read_only_is_not_stored_twicepins it and reddens when the reader is put back.enforcement_refusalstill promised the enforcement would ship. Its sentence closed with "issue #2492 Stage 2 adds registry enforcement", which #2498 had already falsified: on a platform that cannot apply a key, the host was told the capability was coming to their platform, when the table sayswin32alone. The clause is now derived fromENFORCED_KINDS, so it names where the kind is enforced and cannot drift from the row it describes. Three carriers of the old promise were corrected: the function, the module comment, and two tests that asserted the literal"Stage 2".Guard
test_every_stated_flag_is_named_in_the_command_helpholds/sandbox's help line — the only place a host learns the syntax, and prose, the carrier that goes stale in silence — toKIND_FLAGS, so a flag cannot ship undiscoverable.Verification
uv run pytest tests/— 5701 passed, 35 skippeduv run python -c "from emrg.client.app import run_client",uv run python -m emrg --help— both cleanNot in this PR
The Electron
SandboxRootsDialogstill sends nokind, so a key typed there meets the path rules and is refused as a relative spelling — the pre-existing behaviour, not a regression. A key is typed in the TUI for now.CI on this head: pending at the time of opening.