From 4f9d7545dd67d210da8078d7e156bfa3c527675b Mon Sep 17 00:00:00 2001 From: argszero Date: Mon, 26 Oct 2026 21:05:02 +0800 Subject: [PATCH] emrg: a session id that names no directory is refused by name --- emrg/server/daemon.py | 17 ++- emrg/session.py | 99 +++++++++++++-- tests/test_session_id_names_a_directory.py | 141 +++++++++++++++++++++ 3 files changed, 239 insertions(+), 18 deletions(-) create mode 100644 tests/test_session_id_names_a_directory.py diff --git a/emrg/server/daemon.py b/emrg/server/daemon.py index 93cd3796..9c59c82d 100644 --- a/emrg/server/daemon.py +++ b/emrg/server/daemon.py @@ -107,6 +107,7 @@ last_n_messages, records_to_messages, session_dir, + session_id_refusal_reason, ) @@ -2880,16 +2881,24 @@ async def _refuse_an_unsafe_session_id( directory one level above the agent root and started its turn, telling the client nothing. Both call sites ask this method, so the rule has one writer even though it has two doors. + + The sentence the client reads is `session_id_refusal_reason`'s, because the rules + are not all about shape: a reserved device name (`con`, `nul`, `com1`, with or + without an extension) is one path component and names no directory on Windows, + and a name ending in a space or a dot names a different directory than it says. """ if not session_id or is_safe_session_id(session_id): return False + # The reason, not a fixed sentence: `a session id is one path component` is true of + # a path escape and false of a device name, which is exactly one component and + # still names no directory (issue #2598). + reason = session_id_refusal_reason(session_id) logger.warning( - "refused a %r message carrying session id %r: a session id is one " - "path component inside /.emrg/sessions", msg_type, session_id, + "refused a %r message carrying session id %r: %s", + msg_type, session_id, reason, ) await self._send(ws, { - "error": f"invalid session id: {session_id!r} (a session id is " - f"one path component)", + "error": f"invalid session id: {session_id!r} ({reason})", }) return True diff --git a/emrg/session.py b/emrg/session.py index 61f72172..86979d2d 100644 --- a/emrg/session.py +++ b/emrg/session.py @@ -315,6 +315,71 @@ def _refuse_a_non_regular_subject(path: Path) -> None: #: whichever platform runs it. _NAME_SEPARATORS = ("/", "\\") +#: Names Windows resolves to a device rather than to a directory, so `mkdir` answers +#: `NotADirectoryError [WinError 267]` — an operating-system error about a name the +#: predicate had already accepted. An extension does not help (`con.txt` is the console +#: too), which is why the base name is what gets judged. `COM0`, `COM10` and `LPT0` are +#: not in the set, and neither is a name that merely contains one (`console`). +_RESERVED_DEVICE_BASES = frozenset( + {"con", "prn", "aux", "nul"} + | {f"com{i}" for i in range(1, 10)} + | {f"lpt{i}" for i in range(1, 10)} +) + +#: What Windows does to the *end* of a name: it strips trailing spaces and dots, so an id +#: ending in one names a directory the id does not spell. +_TRAILING_STRIPPED = (" ", ".") + + +class UnsafeSessionId(ValueError): + """A session id that names no directory, carrying the rule it broke. + + Raised where a directory would otherwise be attempted, so a caller reads the name as + the cause instead of a platform error about a path it never chose. + """ + + +def session_id_refusal_reason(session_id: object) -> str | None: + """The rule `session_id` breaks, or `None` when it names one entry in the directory. + + One producer for the rule **and** for the sentence that reports it: the daemon sends + this text to the client, `Session.delete` logs it and `create_with_id` raises it, so + the three cannot drift into three accounts of one refusal — and the daemon cannot go + on saying "one path component" about a device name, which *is* one. + + Platform-independent on purpose, on the predicate's existing ground: an id reaches a + path on whichever platform runs it, and a name that names no directory on one of them + is not portable. So the device set and the trailing-strip rule are refused on every + host, not only on Windows — a POSIX leg that accepted them would certify an id its + Windows leg cannot create. + + The bound that follows, stated rather than hidden: an id whose directory already + exists on a platform where it can is refused when deleted, because the refusal is + about the name. `Session.load` does not consult this, so a directory already on disk + keeps loading. + """ + if not isinstance(session_id, str) or not session_id: + return "a session id is a non-empty name" + if "\0" in session_id: + return "a name no filesystem accepts: it carries a NUL" + if session_id in (".", ".."): + return "a session id is one path component" + if any(sep in session_id for sep in _NAME_SEPARATORS): + return "a session id is one path component" + if os.path.splitdrive(session_id)[0] or ntpath.splitdrive(session_id)[0]: + return "a session id is one path component" + if session_id.split(".", 1)[0].lower() in _RESERVED_DEVICE_BASES: + return ( + "a device name is one path component, but it names no directory on Windows " + "(CON, PRN, AUX, NUL, COM1-COM9, LPT1-LPT9, with or without an extension)" + ) + if session_id[-1] in _TRAILING_STRIPPED: + return ( + "Windows strips a trailing space or dot, so the directory this id names is " + "not the one it says" + ) + return None + def is_safe_session_id(session_id: object) -> bool: """True when `session_id` names exactly one entry inside the sessions directory. @@ -330,16 +395,15 @@ def is_safe_session_id(session_id: object) -> bool: separator, and no drive. The drive is asked of **both** flavours — `os.path.splitdrive` answers `("", "C:evil")` on this host and `ntpath.splitdrive` answers `("C:", "evil")`, and a `C:`-relative id is a path, not a name, on the platform that can receive it. + + The rules themselves live in `session_id_refusal_reason`, which also says *which* one + fired: a name can be one path component and still name no directory — `con` is one + component on Windows and is the console there, and `a ` loses its space. Measured + 2026-10-26 on Windows Server 2022: this function answered True for `con`, `CON`, + `con.txt`, `com1` and `nul`, and the mkdir that followed raised `NotADirectoryError + [WinError 267]` naming the platform rather than the id. """ - if not isinstance(session_id, str) or not session_id: - return False - if "\0" in session_id or session_id in (".", ".."): - return False - if any(sep in session_id for sep in _NAME_SEPARATORS): - return False - if os.path.splitdrive(session_id)[0] or ntpath.splitdrive(session_id)[0]: - return False - return True + return session_id_refusal_reason(session_id) is None def session_dir(cwd: Path, session_id: str) -> Path: @@ -517,7 +581,15 @@ def create(cls, cwd: Path) -> Session: @classmethod def create_with_id(cls, session_id: str, cwd: Path) -> Session: - """Create a new session with a specific session ID (from client).""" + """Create a new session with a specific session ID (from client). + + The id is asked **before** the session is constructed, because constructing one + creates its directory: a refused name would reach `mkdir` and come back as a + platform error naming neither the id nor the rule it broke (issue #2598). + """ + reason = session_id_refusal_reason(session_id) + if reason is not None: + raise UnsafeSessionId(f"{session_id!r} cannot name a session: {reason}") session = cls(session_id, cwd) now = datetime.now().isoformat() session._created_at = now @@ -1236,11 +1308,10 @@ def delete(session_id: str, cwd: Path) -> bool: """ import shutil - if not is_safe_session_id(session_id): + reason = session_id_refusal_reason(session_id) + if reason is not None: logger.warning( - "refusing to delete a session named %r: a session id is one path " - "component, so nothing outside the sessions directory is reachable", - session_id, + "refusing to delete a session named %r: %s", session_id, reason, ) return False diff --git a/tests/test_session_id_names_a_directory.py b/tests/test_session_id_names_a_directory.py new file mode 100644 index 00000000..4b5d82a0 --- /dev/null +++ b/tests/test_session_id_names_a_directory.py @@ -0,0 +1,141 @@ +"""A session id must name a directory on the platform that runs it. + +`is_safe_session_id` answers "is this one entry inside the sessions directory", and on +Windows a whole class of name passes that question and still cannot be a directory: the +reserved device names. Measured on Windows Server 2022 at master `6ddb7cc` — +`is_safe_session_id('con')` returned True, and `Session.create_with_id('con', )` +raised `NotADirectoryError [WinError 267] The directory name is invalid`, naming the +operating system instead of the name. `CON`, `con.txt` and `com1` fail the same way; `nul` +gets one step further and fails on the `memory` subdirectory. + +The trailing half is quieter and fails without raising: Windows strips trailing spaces and +dots from a name, so `a ` and `a.` name a directory the id does not spell. Measured: +`.. ` mkdir'd "successfully" (it resolved to the parent) and the first write into it +raised `FileNotFoundError`. + +Both directions are pinned here, because a rule that refused everything would pass the +refusing half on its own: names that are ordinary on every platform stay accepted, the +reserved set is exactly the documented one, and a directory already on disk still loads. +""" + +import sys + +import pytest + +from emrg.session import ( + Session, + UnsafeSessionId, + is_safe_session_id, + session_id_refusal_reason, +) + + +@pytest.mark.parametrize("session_id", [ + "emrg-task", + "s_260727_1103_866d", + "sess-1.2", # a dot inside the name is ordinary + "My Task", # spaces inside the name, not at the end + "中文任务", + "console", # contains a reserved base, and is not one + "con-artist", + "com0", # not in the documented set + "com10", + "lpt0", + "auxiliary", +]) +def test_a_name_a_directory_can_carry_is_accepted(session_id): + """The names the daemon generates, host-chosen names, and the near-misses.""" + assert is_safe_session_id(session_id) is True + assert session_id_refusal_reason(session_id) is None + + +@pytest.mark.parametrize("session_id", [ + "con", + "CON", + "Con", + "con.txt", # the extension does not help: it is the console too + "CON.tar.gz", + "prn", + "aux", + "nul", + "nul.txt", + "com1", + "COM9", + "lpt1", + "LPT9", + "a ", # Windows strips the space, so this names `a` + "a.", + "con ", +]) +def test_a_name_no_directory_can_carry_is_refused(session_id): + assert is_safe_session_id(session_id) is False + assert session_id_refusal_reason(session_id) + + +@pytest.mark.parametrize("session_id, expected", [ + # The shapes that were refused before this rule keep their sentence, byte for byte: + # the daemon's own test pins it, and a path escape really is more than one component. + ("../..", "a session id is one path component"), + ("a/b", "a session id is one path component"), + ("C:evil", "a session id is one path component"), + (".", "a session id is one path component"), + # The two new classes say something else, because for them that sentence is false. + ("con", "a device name is one path component"), + ("a ", "Windows strips a trailing space or dot"), +]) +def test_the_reason_names_the_rule_that_fired(session_id, expected): + """A refusal a reader cannot act on is only half a refusal.""" + assert session_id_refusal_reason(session_id).startswith(expected) + + +def test_an_ordinary_name_still_creates_its_directory(tmp_path): + """The other direction: the new rules must not refuse the names hosts use.""" + session = Session.create_with_id("My Task", tmp_path) + assert session.session_id == "My Task" + assert (tmp_path / ".emrg" / "sessions" / "My Task" / "meta.json").is_file() + + +@pytest.mark.parametrize("session_id", ["con", "nul", "com1", "a ", "a."]) +def test_a_refused_name_never_reaches_the_filesystem(session_id, tmp_path): + """Requirement 3: a named error before a directory is attempted, not after it failed. + + Constructing a `Session` is what creates the directory, so the refusal has to come + first — and the assertion is that no directory was left behind, which is what a caller + sees when the platform error is replaced by this one. + """ + with pytest.raises(UnsafeSessionId) as raised: + Session.create_with_id(session_id, tmp_path) + assert "cannot name a session" in str(raised.value) + assert not (tmp_path / ".emrg" / "sessions" / session_id).exists() + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="the directory cannot exist on Windows, which is the whole point of the rule", +) +def test_a_directory_already_on_disk_still_loads(tmp_path): + """The rule is about creating a name, not about reading what is already there.""" + target = tmp_path / ".emrg" / "sessions" / "con" + target.mkdir(parents=True) + (target / "meta.json").write_text('{"session_id": "con"}', encoding="utf-8") + loaded = Session.load("con", tmp_path) + assert loaded.session_id == "con" + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="the directory cannot exist on Windows, which is the whole point of the rule", +) +def test_deleting_such_a_directory_is_refused_by_the_same_name_rule(tmp_path): + """The stated cost of a platform-independent rule, pinned so it is a decision. + + An id the predicate refuses cannot be deleted through `Session.delete` even where its + directory exists, because the refusal is about the name. Nothing outside the sessions + directory is reachable either way, and the alternative — a platform-conditional rule — + would certify on POSIX an id that Windows cannot create. + """ + target = tmp_path / ".emrg" / "sessions" / "con" + target.mkdir(parents=True) + (target / "meta.json").write_text('{"session_id": "con"}', encoding="utf-8") + assert Session.delete("con", tmp_path) is False + assert (target / "meta.json").is_file()