Repository navigation
Conversation
…kspace root
In Docker runtime mode the cookie-or-header workspace router only declared
/{conversation_id}/workspace/{file_path:path}, so GET .../workspace (no
trailing slash) fell through to the header-only conversation proxy and a
cookie-only request got 401. Declare the slashless root on the same
handler, proxying it to the container's /workspace root as local mode
serves it, and drop the F16.docker-root-cookie known-bug marker.
Fixes #5844
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this change belongs in this repository (agent-server Docker runtime, which this repo owns) and the direction is right — the slashless GET /api/conversations/{id}/workspace now lands in the cookie-or-header docker_workspace_router group instead of falling through to the header-only catch-all. Local verification: uv run pytest -q tests/agent_server/docker_runtime tests/agent_server/test_workspace_router.py tests/agent_server/test_workspace_cookie_auth.py -> 140 passed, and all 27 non-skipped checks on the head are green (OpenAPI quality, build, coverage, workspace tests).
One material finding is attached inline. Making file_path an optional parameter of the root route has a side effect the PR does not intend: because the root route (/{conversation_id}/workspace) has no {file_path} path segment, FastAPI infers file_path as a query parameter for that operation. As a result, GET .../workspace?file_path=<anything> is accepted and silently rewritten to an upstream file path — a request that previously received 401 for a cookie-only caller now proxies to a different resource, and file_path=../../... is forwarded verbatim. This is a small security/contract widening of the workspace serving path (the very surface this PR is tightening), it leaks into the generated OpenAPI schema, and it is not covered by the new test. Details and a suggested shape are in the inline comment.
Because the new test asserts only client.get(path) without query parameters, it does not exercise this path, and the behavior is easy to miss. Please address before merge.
🔄 CHANGES REQUESTED
| @docker_workspace_router.get("/{conversation_id}/workspace/{file_path:path}") | ||
| async def proxy_workspace_file( | ||
| conversation_id: UUID, file_path: str, request: Request | ||
| conversation_id: UUID, request: Request, file_path: str | None = None |
There was a problem hiding this comment.
Making file_path optional here has an unintended side effect on the new root route. The decorator above adds /{conversation_id}/workspace, which has no {file_path} path segment, so FastAPI binds file_path as a query parameter for that operation (confirmed in the app's OpenAPI: GET /api/conversations/{conversation_id}/workspace lists file_path with in: query). At runtime this means GET .../workspace?file_path=X is accepted and rewrites the upstream path to .../workspace/X:
GET /api/conversations/<id>/workspace?file_path=secret.txt
-> upstream /api/conversations/<id>/workspace/secret.txt?file_path=secret.txt
GET /api/conversations/<id>/workspace?file_path=../../etc/passwd
-> upstream /api/conversations/<id>/workspace/../../etc/passwd?file_path=..%2F..%2Fetc%2Fpasswd
Before this change that URL was not served by this handler (cookie-only callers got 401), so this is a behavior widening introduced by the fix, on the workspace-serving surface the PR is trying to make consistent. It also leaks a spurious file_path query parameter into the generated OpenAPI contract.
Suggested fix: keep the two routes on separate handlers (mirroring workspace_router.serve_workspace_root / serve_workspace_file), or read the path from the request rather than adding an optional parameter — e.g. compute upstream = _upstream_path(request, request.url.path) for the root handler and leave the file handler unchanged. Adding a test that asserts a query-only ?file_path= request does not redirect to a file path would lock this down.
…-cookie Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S978x74H7Drsnw7bzoGWc8
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this change belongs in this repository (agent-server Docker runtime, which this repo owns) and the direction is right — the slashless GET /api/conversations/{id}/workspace now lands in the cookie-or-header docker_workspace_router group instead of falling through to the header-only catch-all. The PR's own diff (against merge-base 5fa4dea) is unchanged from the earlier revision: only routers.py, its test, and the F16 feature-map prose. Head f2987f2 is a merge of main into the branch.
Local verification on this head: uv run pytest -q tests/agent_server/docker_runtime tests/agent_server/test_workspace_router.py tests/agent_server/test_workspace_cookie_auth.py -> 146 passed, and all 30 checks on the head are green.
The one material finding from the earlier review still reproduces on this head and remains unaddressed, so it is repeated inline. Making file_path an optional parameter of the root route has a side effect the PR does not intend: because the root route (/{conversation_id}/workspace) has no {file_path} path segment, FastAPI infers file_path as a query parameter for that operation. As a result, GET .../workspace?file_path=<anything> is accepted and silently rewritten to an upstream file path — a request that previously received 401 for a cookie-only caller now proxies to a different resource, and file_path=../../... is forwarded verbatim. This is a small security/contract widening of the workspace-serving surface this PR is tightening, it leaks a spurious file_path query parameter into the generated OpenAPI schema, and it is not covered by the new test (which calls client.get(path) with no query string). Please address before merge.
Unrelated to the finding: the PR is currently mergeable: false / mergeable_state: dirty against main (merge conflict), so it will need a rebase before it can merge.
🔄 CHANGES REQUESTED
| @docker_workspace_router.get("/{conversation_id}/workspace/{file_path:path}") | ||
| async def proxy_workspace_file( | ||
| conversation_id: UUID, file_path: str, request: Request | ||
| conversation_id: UUID, request: Request, file_path: str | None = None |
There was a problem hiding this comment.
Making file_path optional here has an unintended side effect on the new root route. The decorator above adds /{conversation_id}/workspace, which has no {file_path} path segment, so FastAPI binds file_path as a query parameter for that operation (confirmed in the app's OpenAPI on this head: GET /api/conversations/{conversation_id}/workspace lists file_path with in: query). At runtime this means GET .../workspace?file_path=X is accepted and rewrites the upstream path to .../workspace/X (reproduced on this head):
GET /api/conversations/<id>/workspace?file_path=secret.txt
-> upstream /api/conversations/<id>/workspace/secret.txt?file_path=secret.txt
GET /api/conversations/<id>/workspace?file_path=../../etc/passwd
-> upstream /api/conversations/<id>/workspace/../../etc/passwd?file_path=..%2F..%2Fetc%2Fpasswd
Before this change that URL was not served by this handler (cookie-only callers got 401), so this is a behavior widening introduced by the fix, on the workspace-serving surface the PR is trying to make consistent. It also leaks a spurious file_path query parameter into the generated OpenAPI contract.
Suggested fix: keep the two routes on separate handlers (mirroring workspace_router.serve_workspace_root / serve_workspace_file), or read the path from the request rather than adding an optional parameter — e.g. compute upstream = _upstream_path(request, request.url.path) for the root handler and leave the file handler unchanged. Adding a test that asserts a query-only ?file_path= request does not redirect to a file path would lock this down.
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Summary
Route the slashless Docker workspace root through the same cookie-or-header authentication as workspace files.
Why
Docker workspace embeds should accept the workspace session cookie with or without the trailing slash.
Issue Number
Fixes #5844
How to Test
Previously recorded verification:
Reproduction commands, results, and limits
uv run pytest -q tests/agent_server/docker_runtime/test_routes.py -k workspace_root_accepts: on main the[](slashless) case fails with 401 while/and/index.htmlpass; on this branch all 3 pass.uv run pytest -q tests/agent_server/docker_runtime tests/agent_server/test_workspace_router.py tests/agent_server/test_workspace_cookie_auth.py: 140 passed.control-agent-server map run --file F16 --only F16.docker-root-cookie --fresh --repeat 2 --checkout <main checkout>->xfailtwice (cookie-onlyGET .../workspaceanswers 401).xpasstwice (404Conversation not found, i.e. the workspace auth group accepted the cookie and the Docker workspace handler answered).control-agent-server map checkok;map run --file F16 --only F16.docker-root-cookie --fresh->pass.serve_workspace_root.uv run pre-commit run --files <changed files>: passed.Video/Screenshots
No UI change; reproduction evidence is above.
Type
Notes
Docker mode only. Local mode and trailing-slash clients already worked; the generated Docker-mode OpenAPI gains the slashless GET route.
Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:f2193fa-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f2193fa-python) is a multi-arch manifest supporting both amd64 and arm64f2193fa-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.35s · commit f2987f2
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 6/6 hunks, 3/3 files.
All estimates and evidence