From 74c2944ca1caf83c3c7423c6aed37be96a8ab1bc Mon Sep 17 00:00:00 2001 From: jony376 Date: Sat, 27 Jun 2026 21:58:29 -0700 Subject: [PATCH] fix(agent): bind session_id to path agent_id on GET/DELETE agent sessions (#15374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Related issues Closes #15128 ### What problem does this PR solve? `GET` and `DELETE` `/api/v1/agents//sessions/` verified canvas access for `agent_id` in the URL but loaded/deleted sessions only by `session_id`, without checking `conv.dialog_id == agent_id`. Any user with access to **any** agent could read or delete another agent's `API4Conversation` session (messages, references, DSL, etc.) when they knew the session UUID. Agent completions in the same file already enforce this binding; chat sessions do too — these two routes were inconsistent. ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) - [ ] New Feature (non-breaking change which adds functionality) - [ ] Documentation Update - [ ] Refactoring - [ ] Performance Improvement - [ ] Other (please describe): ### Changes | File | Change | |------|--------| | `api/apps/restful_apis/agent_api.py` | Require `conv.dialog_id == agent_id` in `get_agent_session` and `delete_agent_session_item`; return generic `"Session not found!"` on mismatch | | `test/unit_test/api/apps/restful_apis/test_get_agent_session.py` | Add IDOR regression tests for GET/DELETE; fix success fixture to include `dialog_id`; track `delete_by_id` calls | ### Test plan - [x] Unit tests added for GET/DELETE IDOR and success paths - [ ] `pytest test/unit_test/api/apps/restful_apis/test_get_agent_session.py` Co-authored-by: Cursor Co-authored-by: Zhichang Yu --- api/apps/restful_apis/agent_api.py | 5 +- .../restful_apis/test_get_agent_session.py | 75 +++++++++++++------ 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/api/apps/restful_apis/agent_api.py b/api/apps/restful_apis/agent_api.py index c2035f2960..286358be8e 100644 --- a/api/apps/restful_apis/agent_api.py +++ b/api/apps/restful_apis/agent_api.py @@ -452,7 +452,7 @@ async def create_agent_session(agent_id, tenant_id): @_require_canvas_access_sync def get_agent_session(agent_id, session_id, tenant_id): exists, conv = API4ConversationService.get_by_id(session_id) - if not exists: + if not exists or conv.dialog_id != agent_id: return get_data_error_result(message="Session not found!") return get_json_result(data=conv.to_dict()) @@ -462,6 +462,9 @@ def get_agent_session(agent_id, session_id, tenant_id): @add_tenant_id_to_kwargs @_require_canvas_access_sync def delete_agent_session_item(agent_id, session_id, tenant_id): + exists, conv = API4ConversationService.get_by_id(session_id) + if not exists or conv.dialog_id != agent_id: + return get_data_error_result(message="Session not found!") return get_json_result(data=API4ConversationService.delete_by_id(session_id)) diff --git a/test/unit_test/api/apps/restful_apis/test_get_agent_session.py b/test/unit_test/api/apps/restful_apis/test_get_agent_session.py index 8832d9fc83..4c9948b97d 100644 --- a/test/unit_test/api/apps/restful_apis/test_get_agent_session.py +++ b/test/unit_test/api/apps/restful_apis/test_get_agent_session.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. # -"""Regression tests for `get_agent_session` (api/apps/restful_apis/agent_api.py).""" +"""Regression tests for agent session GET/DELETE (api/apps/restful_apis/agent_api.py).""" import importlib.util import sys @@ -36,12 +36,14 @@ def _stub(monkeypatch, name, **attrs): return mod -def _load_agent_api(monkeypatch, get_by_id_result): - """Load api/apps/restful_apis/agent_api.py with the minimum stubs required. +def _load_agent_api(monkeypatch, get_by_id_result, delete_calls=None): + """Load api/apps/restful_apis/agent_api.py with the minimum stubs required.""" + delete_calls = delete_calls if delete_calls is not None else [] + + def _delete_by_id(session_id): + delete_calls.append(session_id) + return True - `get_by_id_result` is the `(exists, conv)` tuple the stub - `API4ConversationService.get_by_id` will return for any session_id. - """ _stub(monkeypatch, "api.apps", current_user=SimpleNamespace(id="tenant-1"), login_required=lambda func: func) _stub(monkeypatch, "api.apps.services.canvas_replica_service", CanvasReplicaService=SimpleNamespace()) _stub(monkeypatch, "api.db", CanvasCategory=SimpleNamespace()) @@ -49,7 +51,12 @@ def _load_agent_api(monkeypatch, get_by_id_result): _stub( monkeypatch, "api.db.services.api_service", - API4ConversationService=SimpleNamespace(get_by_id=lambda _session_id: get_by_id_result, save=lambda **_kwargs: True, delete_by_id=lambda *_args, **_kwargs: True, query=lambda **_kwargs: []), + API4ConversationService=SimpleNamespace( + get_by_id=lambda _session_id: get_by_id_result, + save=lambda **_kwargs: True, + delete_by_id=_delete_by_id, + query=lambda **_kwargs: [], + ), ) _stub( monkeypatch, @@ -89,24 +96,16 @@ def _load_agent_api(monkeypatch, get_by_id_result): module.manager = _PassthroughManager() monkeypatch.setitem(sys.modules, "test_get_agent_session_agent_api", module) spec.loader.exec_module(module) - return module + return module, delete_calls @pytest.mark.p1 class TestGetAgentSession: - """Regression for #14989: GET /agents//sessions/ must not crash - with `AttributeError: 'NoneType' object has no attribute 'to_dict'` when - the session_id does not exist.""" + """Regression for missing sessions and IDOR on GET /agents//sessions/.""" @pytest.mark.p1 def test_returns_error_when_session_missing(self, monkeypatch): - """Missing session must return a data-error JSON, not raise AttributeError. - - In multi-instance deployments, the session row may not yet be visible - on the node servicing the GET. The previous implementation called - `conv.to_dict()` on the `None` returned by `get_by_id` and crashed. - """ - module = _load_agent_api(monkeypatch, get_by_id_result=(False, None)) + module, _ = _load_agent_api(monkeypatch, get_by_id_result=(False, None)) result = module.get_agent_session(agent_id="agent-1", session_id="does-not-exist", tenant_id="tenant-1") @@ -118,9 +117,8 @@ class TestGetAgentSession: @pytest.mark.p1 def test_returns_session_dict_when_found(self, monkeypatch): - """When the session exists, the route returns its `to_dict()` payload.""" - conv = SimpleNamespace(to_dict=lambda: {"id": "sess-1", "messages": []}) - module = _load_agent_api(monkeypatch, get_by_id_result=(True, conv)) + conv = SimpleNamespace(dialog_id="agent-1", to_dict=lambda: {"id": "sess-1", "messages": []}) + module, _ = _load_agent_api(monkeypatch, get_by_id_result=(True, conv)) result = module.get_agent_session(agent_id="agent-1", session_id="sess-1", tenant_id="tenant-1") @@ -129,3 +127,38 @@ class TestGetAgentSession: "message": "", "data": {"id": "sess-1", "messages": []}, } + + @pytest.mark.p1 + def test_get_rejects_session_for_different_agent(self, monkeypatch): + conv = SimpleNamespace(dialog_id="agent-victim", to_dict=lambda: {"id": "sess-1"}) + module, _ = _load_agent_api(monkeypatch, get_by_id_result=(True, conv)) + + result = module.get_agent_session(agent_id="agent-attacker", session_id="sess-1", tenant_id="tenant-1") + + assert result["message"] == "Session not found!" + assert result["data"] is None + + +@pytest.mark.p1 +class TestDeleteAgentSession: + """Regression for IDOR on DELETE /agents//sessions/.""" + + @pytest.mark.p1 + def test_delete_rejects_session_for_different_agent(self, monkeypatch): + conv = SimpleNamespace(dialog_id="agent-victim") + module, delete_calls = _load_agent_api(monkeypatch, get_by_id_result=(True, conv)) + + result = module.delete_agent_session_item(agent_id="agent-attacker", session_id="sess-1", tenant_id="tenant-1") + + assert result["message"] == "Session not found!" + assert delete_calls == [] + + @pytest.mark.p1 + def test_delete_succeeds_when_session_belongs_to_agent(self, monkeypatch): + conv = SimpleNamespace(dialog_id="agent-1") + module, delete_calls = _load_agent_api(monkeypatch, get_by_id_result=(True, conv)) + + result = module.delete_agent_session_item(agent_id="agent-1", session_id="sess-1", tenant_id="tenant-1") + + assert result == {"code": 0, "message": "", "data": True} + assert delete_calls == ["sess-1"]