From 83f14b2f21205455fa3315da6283a4696cb83fd5 Mon Sep 17 00:00:00 2001 From: Tranquil-Flow <66773372+Tranquil-Flow@users.noreply.github.com> Date: Mon, 6 Jul 2026 08:30:43 +0200 Subject: [PATCH] fix(gateway): relax session_key traversal guard to allow interior '/' (#59322) The CWE-22 traversal guard in SessionEntry.from_dict rejects any interior '/' in session_key, but session_key is a logical routing key (never used as a filesystem path) and Google Chat resource names legitimately contain '/' (spaces/, spaces//threads/). All Google Chat sessions were silently dropped on gateway start. Split the validation: session_id keeps the strict _is_path_unsafe guard (it's the value used as a filename); session_key now uses a relaxed _is_session_key_unsafe helper that only blocks genuine traversal vectors (parent-dir '..', leading '/', leading '\', leading Windows drive-letter prefix) and allows interior '/'. --- gateway/session.py | 48 ++++++++++++++++--- tests/gateway/test_session.py | 86 ++++++++++++++++++++++++++++++++++- 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/gateway/session.py b/gateway/session.py index a45a883c9dc..9d20c204a83 100644 --- a/gateway/session.py +++ b/gateway/session.py @@ -111,13 +111,38 @@ def _is_path_unsafe(value: object) -> bool: s = str(value) if ".." in s or "/" in s or "\\" in s: return True - # Leading Windows drive path, e.g. "C:\..." or "d:/...". A bare "x:" + # Leading Windows drive path, e.g. "C:\\..." or "d:/...". A bare "x:" # with no following separator isn't a usable absolute path, and the # separator forms are already caught above — but keep an explicit guard # for the drive-letter prefix in case a separator was normalized away. return len(s) >= 2 and s[0].isalpha() and s[1] == ":" +def _is_session_key_unsafe(value: object) -> bool: + """Return True if ``value`` could be a real traversal vector in a session_key. + + ``session_key`` is a *logical* routing key (e.g. + ``agent:main:google_chat:group:spaces/``) — it never touches the + filesystem, so the strict separator-rejecting guard from + ``_is_path_unsafe`` is over-broad: it falsely rejects Google Chat + resource names (``spaces/``, ``spaces//threads/``) and any + other platform whose native IDs legitimately contain ``/``. + + The relaxed check only blocks genuine traversal: parent-dir ``..``, + a *leading* path separator (``/``/``\\``, which would make the key + absolute on disk if it ever were written), and a leading Windows + drive letter. Interior ``/`` is allowed. + """ + if not value: + return False + s = str(value) + if ".." in s: + return True + if s.startswith("/") or s.startswith("\\"): + return True + return len(s) >= 2 and s[0].isalpha() and s[1] == ":" + + @dataclass class SessionSource: """ @@ -741,12 +766,21 @@ class SessionEntry: session_key = data["session_key"] session_id = data["session_id"] - # Validate path-sensitive fields to prevent directory traversal (CWE-22) - for _field, _val in (("session_key", session_key), ("session_id", session_id)): - if _is_path_unsafe(_val): - raise ValueError( - f"Invalid {_field}: potential directory traversal detected" - ) + # Validate path-sensitive fields to prevent directory traversal (CWE-22). + # ``session_id`` is the value used as a filename + # (``sessions_dir / f"{session_id}.json"``), so it must pass the strict + # guard. ``session_key`` is a *logical* routing key that never touches + # the filesystem — interior ``/`` is legitimate (Google Chat resource + # names are ``spaces/`` and ``spaces//threads/``), so it + # only needs the relaxed guard against genuine traversal vectors. + if _is_path_unsafe(session_id): + raise ValueError( + "Invalid session_id: potential directory traversal detected" + ) + if _is_session_key_unsafe(session_key): + raise ValueError( + "Invalid session_key: potential directory traversal detected" + ) return cls( session_key=session_key, diff --git a/tests/gateway/test_session.py b/tests/gateway/test_session.py index ba9ccb731f5..4f47d0ff638 100644 --- a/tests/gateway/test_session.py +++ b/tests/gateway/test_session.py @@ -1191,8 +1191,92 @@ class TestSessionEntryFromDictTraversalValidation: from gateway.session import SessionEntry with pytest.raises(ValueError, match="session_id"): SessionEntry.from_dict(self._entry(session_id="good\\..\\bad")) + + def test_session_id_interior_slash_raises(self): + """A non-leading forward slash is still a traversal vector for session_id + (it never touches the filesystem, so it must remain strict).""" + from gateway.session import SessionEntry + with pytest.raises(ValueError, match="session_id"): + SessionEntry.from_dict(self._entry(session_id="good/../bad")) + + +class TestSessionEntryFromDictGoogleChatKeyAccepted: + """Regression: from_dict must accept Google Chat session_keys with interior '/'. + + Google Chat resource names are ``spaces/`` and ``spaces//threads/``, + so the routing key ``agent:main:google_chat::spaces/[:]`` + legitimately contains ``/``. ``session_key`` is a *logical* routing key, never + a filesystem path, so the strict CWE-22 guard from ``_is_path_unsafe`` is + over-broad here. Only ``session_id`` (the value used as a filename) needs the + strict check. + + See issue #59322. + """ + + BASE = { + "session_id": "abc123", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + } + + def _entry(self, **overrides): + return {**self.BASE, **overrides} + + def test_google_chat_group_key_accepted(self): + from gateway.session import SessionEntry + entry = SessionEntry.from_dict(self._entry( + session_key="agent:main:google_chat:group:spaces/AAAAEVvy5RY", + )) + assert entry.session_key == "agent:main:google_chat:group:spaces/AAAAEVvy5RY" + + def test_google_chat_thread_key_accepted(self): + from gateway.session import SessionEntry + entry = SessionEntry.from_dict(self._entry( + session_key="agent:main:google_chat:group:spaces/AAAAEVvy5RY:spaces/AAAAEVvy5RY/threads/hrI_46qEx6c", + )) + assert "spaces/AAAAEVvy5RY/threads/hrI_46qEx6c" in entry.session_key + + def test_google_chat_dm_key_accepted(self): + from gateway.session import SessionEntry + entry = SessionEntry.from_dict(self._entry( + session_key="agent:main:google_chat:dm:spaces/9Il3iSAAAAE", + )) + assert entry.session_key == "agent:main:google_chat:dm:spaces/9Il3iSAAAAE" + + +class TestSessionEntryFromDictSessionKeyTraversalStillRejected: + """The relaxed guard on ``session_key`` must still reject genuine traversal: + parent-dir ``..``, absolute path prefixes (``/``, ``\\``), and Windows + drive-letter prefixes. Only interior ``/`` is allowed.""" + + BASE = { + "session_id": "abc123", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + } + + def _entry(self, **overrides): + return {**self.BASE, **overrides} + + def test_session_key_dotdot_raises(self): + from gateway.session import SessionEntry with pytest.raises(ValueError, match="session_key"): - SessionEntry.from_dict(self._entry(session_key="agent:main:good/sub")) + SessionEntry.from_dict(self._entry(session_key="agent:main:../../secret")) + + def test_session_key_leading_slash_raises(self): + from gateway.session import SessionEntry + with pytest.raises(ValueError, match="session_key"): + SessionEntry.from_dict(self._entry(session_key="/absolute/path/key")) + + def test_session_key_leading_backslash_raises(self): + from gateway.session import SessionEntry + with pytest.raises(ValueError, match="session_key"): + SessionEntry.from_dict(self._entry(session_key="\\absolute\\path\\key")) + + def test_session_key_drive_letter_raises(self): + from gateway.session import SessionEntry + with pytest.raises(ValueError, match="session_key"): + SessionEntry.from_dict(self._entry(session_key="C:drive/key")) class TestEnsureLoadedSkipsInvalidEntries: