mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
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/<id>, spaces/<id>/threads/<id>). 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 '/'.
This commit is contained in:
parent
9d848cc60a
commit
83f14b2f21
2 changed files with 126 additions and 8 deletions
|
|
@ -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/<id>``) — 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/<id>``, ``spaces/<id>/threads/<id>``) 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/<id>`` and ``spaces/<id>/threads/<id>``), 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,
|
||||
|
|
|
|||
|
|
@ -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/<id>`` and ``spaces/<id>/threads/<id>``,
|
||||
so the routing key ``agent:main:google_chat:<chat_type>:spaces/<id>[:<thread>]``
|
||||
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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue