Skip to content

emrg: a session id is one path component, checked where every message crosses - #2440

Merged
argszero merged 3 commits into
masterfrom
fix/session-id-is-one-path-component
Oct 21, 2026
Merged

argszero merged 3 commits into
masterfrom
fix/session-id-is-one-path-component

Conversation

@argszero

Copy link
Copy Markdown
Owner

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 an
absolute id at that point, so an id of ../.. names the project tree rather than a
session directory — and delete_session hands what it resolved to shutil.rmtree.

Measured before the fix, in a throwaway tree holding .emrg/sessions/keep/ and
victim/precious.txt: Session.delete("../..", <project>) walked
<project>/.emrg/sessions/../.. and left the tree empty (victim/ and .emrg/ alike),
then raised FileNotFoundError from shutil.rmtree's final os.rmdir — the error arrives
after 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, because
    os.path.splitdrive("C:evil") answers ("", "C:evil") on this host while
    ntpath.splitdrive answers ("C:", "evil") — and a C:-relative id is a path on the
    platform 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 the
    directory), _handle_resume_session's meta read, the two session-scope memory readers
    and delete_session all take the id from this same message field, so a handler added
    later inherits the check instead of repeating it. The refusal is a frame, not a raise —
    a raise out of _process_message ends the client's read loop and cancels the turn it
    was watching.
  • emrg/session.py — Session.delete asks it too. It is a public static method, and a
    caller 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 daemon
generates and refuses 12 shapes; Session.delete("../..", project) deletes nothing and
leaves victim/precious.txt and .emrg/sessions/keep in place; Session.delete("keep", project) still deletes its own directory and leaves the neighbour alone.

tests/test_daemon.py (3 tests, driving _process_message through the existing _drive
helper): a delete_session carrying ../.. answers one frame and the project survives; a
delete_session carrying keep still answers session_deleted with no error; a task
carrying ../.. creates no session directory.

Mutation arms — each on the committed tree, each restored byte-identically:

arm what it removes result
1 the message-seam check (if False and …) 2 failed, 1 passed — both refusal tests red
2 the Session.delete check test_an_escaping_id_deletes_nothing red
3 the ntpath half of the drive test the C:evil case red

Arm 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 (on f09727f8).
  • uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help — both clean.
  • No daemon.py behaviour outside this seam, no test that starts or stops a server, no
    ~/.emrg/config.toml, no auto-upgrade path.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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.
@argszero

Copy link
Copy Markdown
Owner Author

The veto's cause is answered in e78981dc, pushed to this branch.

What was wrong: _handle_client intercepts {"type": "task"} itself
(emrg/server/daemon.py:1570) and never reaches _process_message, so the guard covered every
message type except the one that creates a session directory. Measured on f09727f8, driving the
real read loop with an auth frame and one task frame carrying session_id="../../../evil":

OUTER DIR CREATED BY THE REAL TASK PATH: True    # <tmp>/evil, one level above the project root
frames: ['auth_ok']                              # and the client is told nothing

The test that was supposed to prove otherwise,
test_a_task_carrying_an_unsafe_session_id_creates_no_directory, handed a task frame to
_process_message — which has no task branch at all, answering unknown message type — so it
greened on a combination production never produces.

What the push changes:

  • The rule now has one writer, EmrgServer._refuse_an_unsafe_session_id, and two callers: the top of
    _process_message, and the task branch beside its existing session_id/cwd check. The
    docstring records why there are two doors, with the measurement above.
  • That misleading test is replaced by two that drive _handle_client with a fake socket — one per
    direction. The unsafe one asserts nothing is created outside the project (the id walks two
    levels up, so that is the assertion that can fail); the safe one asserts no refusal and that the
    session directory the branch creates exists.
  • The arm was run: removing the task-branch call reddens
    test_the_read_loop_refuses_a_task_carrying_an_unsafe_session_id and leaves the safe direction
    green — 1 failed, 1 passed — then the file was restored.

Verification on the pushed head e78981dc (this checkout, one interpreter, sandbox default):
pytest tests/ 5415 passed, 35 skipped; from emrg.client.app import run_client OK;
python -m emrg --help OK. Two files changed, both inside the PR's subject.

The predicate itself is unchanged and was checked in both directions in the veto: emrg-task
accepted, ../.., a/b, a\b, C:evil, . and .. refused, and the Session.delete arm reddens
tests/test_session_delete_path.py.

Note that the branch is stacked on #2438's head 1a1da723, so its rendered diff still shows that
prompt rewrite as its own and merging this lands both. Whoever merges should re-read
scripts/check-merge-plan-suite.py 2440 on the master of that day rather than trust this reading.

@how2how2how2-arch how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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, base ffc51f03, 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: test pass 3m16s, test-windows pass 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 → base ffc51f03, final tree cee92d28d6bb,
    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: test pass 3m16s, test-windows pass 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.

@argszero
argszero merged commit f3f9a6a into master Oct 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a session id is walked as a path, so delete_session can delete the project tree

2 participants