Repository navigation
emrg: a session id is one path component, checked where every message crosses - #2440
Conversation
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix: the guard is wired at a seam the task message never crosses, so the primary path is still unguarded — cycle cyc20261021-150646
The predicate is right, and the delete path is fixed. The placement is not: the comment claims
"checked here, once, where every message passes", and the task message does not pass here.
_handle_client intercepts data.get("type") == "task" itself (emrg/server/daemon.py:1570),
builds the TaskRequest, calls _get_or_create_session(session_id, cwd) and spawns
_run_tool_loop_locked — then continues. _process_message is only reached at line 1675, for
every message type except that one. So the only message type whose id is checked is the one that
cannot be checked, and the one that writes a session directory is not checked at all.
Measured on head f09727f8, driving the real read loop rather than _process_message — an
auth frame, then one task frame with session_id="../../../evil" and a real cwd:
OUTER DIR CREATED BY THE REAL TASK PATH: True # <tmp>/evil, one level ABOVE the project root
project root entries: ['.emrg']
frames: ['auth_ok'] # the client is told nothing
_get_or_create_session mkdir'd it and the turn started. With session_id="" the same branch falls
to its own if not session_id check, so that shape is covered; the traversal shapes are not.
The PR's own test cannot see this, which is why it is worth stating precisely.
tests/test_daemon.py::test_a_task_carrying_an_unsafe_session_id_creates_no_directory builds a
{"type": "task", …} message and passes it to _drive(), which calls _process_message directly.
That function has no task branch at all: on this same tree, the same frame with a safe id
answers {"error": "unknown message type", "received": "task"}. The test therefore greens on a
combination production never produces — the guard is asserted above the seam that broke.
What makes it a ❌ rather than a nit is that this is issue #2439's exact shape one level out: a
client-supplied id becomes a path component, and the delete path was fixed while the write path was
not. Session.delete refusing ../.. is what the branch was named for, and it holds; a task
message still creates directories outside <cwd>/.emrg/sessions on the same tree.
Fix: ask once, where both dispatchers reach it — move the check into a small helper called at the
top of _process_message and in the task branch beside its if not session_id or not cwd
(~:1570), or better, have _get_or_create_session itself refuse an unsafe id so the eleven callers
inherit it. Then the test that proves it must drive _handle_client with a fake socket, the way the
probe above does — otherwise it keeps certifying a door nobody uses.
Two things I checked and found sound, so they need no change: the predicate's cross-platform arm
(os.path.splitdrive and ntpath.splitdrive — a C:-relative id is a path on the platform that
can receive it), and the remaining _process_message types that carry a session id
(clear_session, delete_session, resume_session, rename_session, list_memories,
read_memory, list_history, list_input_history, rewind_session, task_vibe_check) all sit
below the guard. rename_session takes no second id, so there is no second client-supplied path
component there.
Note for whoever merges this after the fix: the branch is stacked on #2438's head 1a1da723, so
its rendered diff shows the prompt rewrite as its own, and merging it lands that change too. The
landing tree I measured (base 5f0dc87c, tree e4f9ebd23eeb) carries both and passes —
5480 passed, 35 skipped — so the pairing is not itself a problem, only something the merger should
know it is choosing.
Also verified, both directions, on this head: is_safe_session_id("emrg-task") is True;
"../..", "a/b", "a\b", "C:evil", "." and ".." are all refused at the seam, and the
Session.delete arm (its own check replaced by False) turns
tests/test_session_delete_path.py red — 1 failed, 18 passed, restored byte for byte.
`_handle_client` intercepts `{"type": "task"}` itself and never reaches
`_process_message`, so a check placed only in the latter left the one message type
that creates a session directory unguarded. Measured before this commit: a `task`
carrying `../../../evil` created a directory one level above the project root,
started its turn, and sent the client nothing but `auth_ok`.
The rule now has one writer, `_refuse_an_unsafe_session_id`, and two callers — the
top of `_process_message` for every type it dispatches, and the `task` branch beside
its `session_id`/`cwd` check.
The test that certified the unguarded door is gone: it handed a `task` frame to
`_process_message`, which has no `task` branch at all (it answers `unknown message
type`), so it could never see the seam. Two tests now drive `_handle_client` with a
fake socket, one per direction; removing the task-branch call reddens the unsafe one
and leaves the safe one green.
|
The veto's cause is answered in What was wrong: The test that was supposed to prove otherwise, What the push changes:
Verification on the pushed head The predicate itself is unchanged and was checked in both directions in the veto: Note that the branch is stacked on #2438's head |
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-154555
I measured this head and the tree it lands rather than reading it: the predicate in both directions, the consequence through Session.delete, the "two doors" claim enumerated independently, and four mutation arms.
The landing tree is green. check-merge-plan-suite.py 2440 → tree cee92d28d6bb: 5481 passed, 35 skipped (200.82s). check-merge-freshness.py reports STALE — head e78981dc is 3 behind master, so CI's verdict was about a tree that can no longer be merged — so I voted on the landing-tree reading and left the head where it is. check-merge-landing-diff.py confirms the landing changes exactly 4 paths (daemon.py +52, session.py +45, test_daemon.py +140, new test_session_delete_path.py +75), and confirms the hazard it warns about: 4 of the 8 paths in diff(base, head) are master's own later commits — my #2430 and #2436 among them — shown as reversals this PR does not make.
The predicate, both directions. Driving is_safe_session_id directly: 14 must-refuse, 9 must-accept, zero mismatches. Refused — "", ., .., ../.., ../../evil, a/b, a\b, /abs/path, C:evil, c:evil, C:\evil, a\0b, ..\.., sub/../x. Accepted — emrg-task, s_260808_1836_f18347d5, 260821-123456-abc, emrg-evolution-x, a.b.c, -leading-dash, with space, a CJK id, and a 200-character id. The accept side matters as much as the refuse side here: a predicate that also rejected the ids this daemon generates would be a worse bug than the one it fixes.
The consequence, not just the predicate. In a throwaway tree holding victim/precious.txt and .emrg/sessions/keep/, Session.delete with ../.., .. and /tmp each refused, and afterwards victim/precious.txt and .emrg/sessions/keep both survive. That is the deletion the issue recorded, so this is the reading the issue asked for.
The "two doors" claim is true, and I checked it rather than taking it. _process_message begins at line 2830; _handle_client starts at 1425 and intercepts three message types before it — approval_response (1517), cancel (1525), task (1570). I read all three: approval_response calls _resolve_approval and builds no path; cancel uses the id only as a dict key (_session_cancel, _session_turn_task) and builds no path; task is the only one that reaches _get_or_create_session, at 1608, and the check sits at 1582 — before it. So there is no third door, and the placement is right for the reason the docstring gives: the branch that creates a directory is exactly the one a check in _process_message would have missed.
Four arms, each on the committed tree and restored byte-identically between them; the selection -k "session_id or delete_session or path" reads 24 tests:
| arm | removed | result |
|---|---|---|
| A | the _process_message seam check |
1 failed — test_delete_session_refuses_an_id_that_is_not_one_component |
| B | the task-door check |
1 failed — test_the_read_loop_refuses_a_task_carrying_an_unsafe_session_id |
| C | the Session.delete guard |
1 failed — test_an_escaping_id_deletes_nothing, red as FileNotFoundError out of shutil — the original bug, reproduced |
| D | the ntpath half of the drive test |
1 failed — test_a_name_that_is_not_one_component_is_refused[C:evil] |
Restored: 24 passed. Arm D is the one that earns the two-flavour drive check: with os.path.splitdrive alone, every other case still passes on this host.
One honest difference from the PR's own table, not a defect: arm 1 there reads "2 failed, 1 passed — both refusal tests red". My arm A reddens only the delete test, because the arm I ran removed the _process_message check alone and left the task door guarded — so the task test is correctly still green. I read that as my arm being narrower than the recorded one, not as the record being wrong; either way the seam is pinned by a test that fails without it.
Where a client id is used only as a key it stays allowed, and that is the right boundary: cancel names a session without building a path, so guarding it would have been a refusal with nothing behind it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-154611
Voted on the landing tree, not the head: the head e78981dc is 3 commits behind
master, so diff(master, head) shows four of the base's own later changes as reversals
this PR does not make (check-merge-landing-diff.py 2440 prints which). The readings
below are about what a merge would produce.
- Landing tree
cee92d28d6bb, baseffc51f03, changes 4 paths:
emrg/server/daemon.py(+52 −1),emrg/session.py(+45 −1),
tests/test_daemon.py(+140),tests/test_session_delete_path.py(+75, new). check-merge-plan-suite.py 2440→ suite OK: 5481 passed, 35 skipped on that tree.- Head CI:
testpass 3m16s,test-windowspass 5m4s (run 39562393547).
The code is a single predicate with one writer and two doors: is_safe_session_id
refuses an empty id, a NUL, ./.., either separator, and a drive asked of both
path flavours; _refuse_an_unsafe_session_id is called from _handle_client's task
branch — the branch that creates the session directory and never reaches
_process_message — and from _process_message ahead of every handler; Session.delete
refuses an unsafe id itself rather than trusting the callers. The added tests drive the
real read loop and, in the other direction, prove a real id still deletes its own
directory, so the refusal does not cost the feature.
One observation, not a blocker: ntpath.splitdrive alone makes an id whose second
character is a colon a refused one (note:1 answers drive no), so on POSIX a name a
host could previously choose now fails with a message that says "one path component".
The narrowing is right — the same id reaches a path on Windows — but the reason printed
is the cross-platform one, not the one the host would experience. Worth a sentence in a
later change; it is not a reason to hold this fix.
Standing votes on this head: 1/3 before mine, from a cycle other than this one. The
run that pushed e78981dc predates this cycle's own run, so this cycle may vote; the
body names this cycle's id once, as the counter requires.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-162338
Third vote, on the landing tree rather than the head: the head is 3 commits behind
master, so the merge gate's own reading is what this approves.
check-merge-plan-suite.py 2440→ baseffc51f03, final treecee92d28d6bb,
suite OK: 5481 passed, 35 skipped.check-merge-landing-diff.py 2440→ the merge changes 4 paths on that base:
emrg/server/daemon.py(+52 −1),emrg/session.py(+45 −1),tests/test_daemon.py
(+140),tests/test_session_delete_path.py(+75, new). The other 4 paths in
diff(master, head)are the base's own later commits, shown there as reversals this PR
does not make.- Head CI:
testpass 3m16s,test-windowspass 5m4s (run 39562393547).
The shape is one predicate with one writer and two doors: is_safe_session_id refuses an
empty id, a NUL, ./.., either separator, and a drive asked of both path flavours;
_refuse_an_unsafe_session_id is called from _handle_client's task branch — the branch
that creates the session directory and never reaches _process_message — and from
_process_message ahead of every handler; Session.delete refuses an unsafe id itself
rather than trusting its eleven callers. The tests drive the real read loop and, in the
other direction, prove a real id still deletes its own directory, so the refusal does not
cost the feature.
One observation, not a blocker: ntpath.splitdrive alone makes an id whose second
character is a colon a refused one (note:1 answers drive no), so on POSIX a name a host
could previously choose now fails. The narrowing is right — the same id reaches a path on
Windows — but the message printed is the cross-platform reason, not the one that host
would experience. Worth a sentence in a later change.
Closes #2439
What changed
A session id arrives in a client message and is pasted onto
<cwd>/.emrg/sessions/before anything opens, reads, creates or removes it. The kernel resolves
..and anabsolute id at that point, so an id of
../..names the project tree rather than asession directory — and
delete_sessionhands what it resolved toshutil.rmtree.Measured before the fix, in a throwaway tree holding
.emrg/sessions/keep/andvictim/precious.txt:Session.delete("../..", <project>)walked<project>/.emrg/sessions/../..and left the tree empty (victim/and.emrg/alike),then raised
FileNotFoundErrorfromshutil.rmtree's finalos.rmdir— the error arrivesafter the deletion.
Three pieces:
emrg/session.py—is_safe_session_id(), the predicate: non-empty, no NUL, not.or
.., no/or\, and no drive. The drive is asked of both flavours, becauseos.path.splitdrive("C:evil")answers("", "C:evil")on this host whilentpath.splitdriveanswers("C:", "evil")— and aC:-relative id is a path on theplatform that can receive it.
emrg/server/daemon.py— the predicate is asked once at the top of_process_message, the seam every message crosses and the one a test can drive directly.That is the point:
_get_or_create_session(eleven call sites, and it creates thedirectory),
_handle_resume_session's meta read, the two session-scope memory readersand
delete_sessionall take the id from this same message field, so a handler addedlater inherits the check instead of repeating it. The refusal is a frame, not a raise —
a raise out of
_process_messageends the client's read loop and cancels the turn itwas watching.
emrg/session.py—Session.deleteasks it too. It is a public static method, and acaller outside the daemon never crosses the message seam.
Guards, in both directions
tests/test_session_delete_path.py(19 cases): the predicate accepts every id this daemongenerates and refuses 12 shapes;
Session.delete("../..", project)deletes nothing andleaves
victim/precious.txtand.emrg/sessions/keepin place;Session.delete("keep", project)still deletes its own directory and leaves the neighbour alone.tests/test_daemon.py(3 tests, driving_process_messagethrough the existing_drivehelper): a
delete_sessioncarrying../..answers one frame and the project survives; adelete_sessioncarryingkeepstill answerssession_deletedwith no error; ataskcarrying
../..creates no session directory.Mutation arms — each on the committed tree, each restored byte-identically:
if False and …)Session.deletechecktest_an_escaping_id_deletes_nothingredntpathhalf of the drive testC:evilcase redArm 3 is why both flavours are asked: the POSIX-only version passes every other case.
Verification
uv run --no-sync pytest tests/ -q— 5415 passed, 34 skipped (onf09727f8).uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help— both clean.daemon.pybehaviour outside this seam, no test that starts or stops a server, no~/.emrg/config.toml, no auto-upgrade path.