mirror of
https://github.com/infiniflow/ragflow.git
synced 2026-06-29 15:31:05 +08:00
fix(agent): bind session_id to path agent_id on GET/DELETE agent sessions (#15374)
## Related issues Closes #15128 ### What problem does this PR solve? `GET` and `DELETE` `/api/v1/agents/<agent_id>/sessions/<session_id>` 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 <cursoragent@cursor.com> Co-authored-by: Zhichang Yu <yuzhichang@gmail.com>
This commit is contained in:
@@ -452,7 +452,7 @@ async def create_agent_session(agent_id, tenant_id):
|
|||||||
@_require_canvas_access_sync
|
@_require_canvas_access_sync
|
||||||
def get_agent_session(agent_id, session_id, tenant_id):
|
def get_agent_session(agent_id, session_id, tenant_id):
|
||||||
exists, conv = API4ConversationService.get_by_id(session_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_data_error_result(message="Session not found!")
|
||||||
return get_json_result(data=conv.to_dict())
|
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
|
@add_tenant_id_to_kwargs
|
||||||
@_require_canvas_access_sync
|
@_require_canvas_access_sync
|
||||||
def delete_agent_session_item(agent_id, session_id, tenant_id):
|
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))
|
return get_json_result(data=API4ConversationService.delete_by_id(session_id))
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -13,7 +13,7 @@
|
|||||||
# See the License for the specific language governing permissions and
|
# See the License for the specific language governing permissions and
|
||||||
# limitations under the License.
|
# 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 importlib.util
|
||||||
import sys
|
import sys
|
||||||
@@ -36,12 +36,14 @@ def _stub(monkeypatch, name, **attrs):
|
|||||||
return mod
|
return mod
|
||||||
|
|
||||||
|
|
||||||
def _load_agent_api(monkeypatch, get_by_id_result):
|
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.
|
"""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", current_user=SimpleNamespace(id="tenant-1"), login_required=lambda func: func)
|
||||||
_stub(monkeypatch, "api.apps.services.canvas_replica_service", CanvasReplicaService=SimpleNamespace())
|
_stub(monkeypatch, "api.apps.services.canvas_replica_service", CanvasReplicaService=SimpleNamespace())
|
||||||
_stub(monkeypatch, "api.db", CanvasCategory=SimpleNamespace())
|
_stub(monkeypatch, "api.db", CanvasCategory=SimpleNamespace())
|
||||||
@@ -49,7 +51,12 @@ def _load_agent_api(monkeypatch, get_by_id_result):
|
|||||||
_stub(
|
_stub(
|
||||||
monkeypatch,
|
monkeypatch,
|
||||||
"api.db.services.api_service",
|
"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(
|
_stub(
|
||||||
monkeypatch,
|
monkeypatch,
|
||||||
@@ -89,24 +96,16 @@ def _load_agent_api(monkeypatch, get_by_id_result):
|
|||||||
module.manager = _PassthroughManager()
|
module.manager = _PassthroughManager()
|
||||||
monkeypatch.setitem(sys.modules, "test_get_agent_session_agent_api", module)
|
monkeypatch.setitem(sys.modules, "test_get_agent_session_agent_api", module)
|
||||||
spec.loader.exec_module(module)
|
spec.loader.exec_module(module)
|
||||||
return module
|
return module, delete_calls
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.p1
|
@pytest.mark.p1
|
||||||
class TestGetAgentSession:
|
class TestGetAgentSession:
|
||||||
"""Regression for #14989: GET /agents/<id>/sessions/<sid> must not crash
|
"""Regression for missing sessions and IDOR on GET /agents/<id>/sessions/<sid>."""
|
||||||
with `AttributeError: 'NoneType' object has no attribute 'to_dict'` when
|
|
||||||
the session_id does not exist."""
|
|
||||||
|
|
||||||
@pytest.mark.p1
|
@pytest.mark.p1
|
||||||
def test_returns_error_when_session_missing(self, monkeypatch):
|
def test_returns_error_when_session_missing(self, monkeypatch):
|
||||||
"""Missing session must return a data-error JSON, not raise AttributeError.
|
module, _ = _load_agent_api(monkeypatch, get_by_id_result=(False, None))
|
||||||
|
|
||||||
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))
|
|
||||||
|
|
||||||
result = module.get_agent_session(agent_id="agent-1", session_id="does-not-exist", tenant_id="tenant-1")
|
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
|
@pytest.mark.p1
|
||||||
def test_returns_session_dict_when_found(self, monkeypatch):
|
def test_returns_session_dict_when_found(self, monkeypatch):
|
||||||
"""When the session exists, the route returns its `to_dict()` payload."""
|
conv = SimpleNamespace(dialog_id="agent-1", to_dict=lambda: {"id": "sess-1", "messages": []})
|
||||||
conv = SimpleNamespace(to_dict=lambda: {"id": "sess-1", "messages": []})
|
module, _ = _load_agent_api(monkeypatch, get_by_id_result=(True, conv))
|
||||||
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")
|
result = module.get_agent_session(agent_id="agent-1", session_id="sess-1", tenant_id="tenant-1")
|
||||||
|
|
||||||
@@ -129,3 +127,38 @@ class TestGetAgentSession:
|
|||||||
"message": "",
|
"message": "",
|
||||||
"data": {"id": "sess-1", "messages": []},
|
"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/<id>/sessions/<sid>."""
|
||||||
|
|
||||||
|
@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"]
|
||||||
|
|||||||
Reference in New Issue
Block a user