fix: security & robustness hardening (path traversal, cross-agent download, MCP lock, task refs, OAuth XSS, memory cache) - #597
Open
angri450 wants to merge 1 commit into
Conversation
…nt download, MCP lock, task refs, oauth XSS, memory cache release)
Security:
- workspace_api_path now rejects any path containing a `..` segment with 403
(previously only lstrip("/")'d, so `../../../etc/passwd` flowed through to
the backend and surfaced as an internal 500; data was contained by
BackendWorkspace.relative_to but the error path was noisy and defence-in-depth
relied on a single check).
- is_allowed_host_download_abs_path no longer returns True for *any*
`/.octop/agents/` path: the allow branch is narrowed to the current agent's
own workspace prefix, so a user with access to one (possibly shared) agent
can no longer read another agent's workspace (memory.sqlite etc).
- OAuth callback HTML escapes redirect_after / state_id before embedding them
in the inline script (user-controlled redirect_after was an unescaped
self-XSS vector).
Robustness:
- MCP tool sync wrapper (`_call`) no longer bypasses the shared per-server lock
when a running event loop exists: it runs the locked coroutine on a
dedicated thread (avoids deadlock vs run_coroutine_threadsafe when invoked
on the loop thread, keeps the loop unblocked).
- Background tasks (agent reload worker, deferred bootstrap, connector reload)
now keep strong references in a task set with done-callbacks, per the
asyncio requirement to hold a reference to every created task.
- Memory dashboard cache closes/tears-down evicted and replaced Memory/Bridge
instances defensively, and logs the real agent_id on eviction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round-2 hardening from a code review of the API/infra layers. Six independent fixes, each verified on the running service.
Security
Workspace path traversal (high) —
workspace_api_pathonly didlstrip("/"); a../../../etc/passwdpath flowed through to the backend. Data was contained byBackendWorkspace._backend_storage_key(ValueError), but surfaced as a noisy 500. Now rejects any..segment with 403, matching the discipline already inhost_dirs.py/ knowledge relpath. Verified: 500 -> 403.Cross-agent file download (high) —
is_allowed_host_download_abs_pathreturnedTruefor any path containing/.octop/agents/without binding it to the requesting agent's workspace, so a user with access to one (possibly shared) agent could read another agent'smemory.sqlite. Allow branch narrowed to the current agent's own workspace prefix. Verified: other-agent path now 403, same-agent download still 200.OAuth callback self-XSS (low) — user-controlled
redirect_afterwas embedded unescaped in the callback<script>. Nowhtml.escape(..., quote=True)before embedding.Robustness
MCP shared-server lock bypass (medium) — the sync
_callwrapper called_tool.invoke(kwargs)directly when a running event loop existed: no shared lock (concurrent interleaving on a shared MCP session) and it blocked the loop. Now runs the locked coroutine on a dedicated thread (no deadlock withrun_coroutine_threadsafewhen the caller is on the loop thread). Tested: no-loop / running-loop / async paths + 12 concurrent calls.Background tasks without strong refs (medium) —
asyncio.create_taskresults were dropped (only bool markers kept); GC could collect reload/bootstrap tasks and config changes would silently never rebuild the harness. Added a task set with done-callbacks (agent reload worker, deferred bootstrap, connector reload).Memory dashboard cache never released (low) — evicted/replaced
Memory/Bridgeinstances were dropped without any close attempt; also fixed the eviction log printing the tuple instead of agent_id. Defensive teardown added (harness-memory has no public close today; sqlite closes with GC, this guards future versions).Files:
api/common/workspace.py,infra/gateway/media/backend_files.py,api/routers/connectors.py,infra/connectors/mcp_tool_cache.py,infra/agents/manager.py,api/routers/providers.py,api/common/memory_client.py.All changes applied on top of develop (base = develop @ 85532ac). Unit-level checks and live service regression (403/200/200, zero errors) pass on the deployment.