Security: Unvalidated Workspace Directory in VSCode URL Endpoint

[tomaioo]

Summary

Security: Unvalidated Workspace Directory in VSCode URL Endpoint

Problem

Severity: Medium | File: openhands-agent-server/openhands/agent_server/vscode_router.py:L31

The /vscode/url endpoint accepts a workspace_dir parameter from the client with a default of "workspace". This parameter is passed to get_vscode_url without validation. If this value is used to construct file paths or commands in the underlying service, it could lead to path traversal or local file inclusion vulnerabilities, allowing an attacker to access or interact with arbitrary directories.

Solution

Validate the workspace_dir parameter against a list of allowed workspaces or ensure it is strictly contained within a predefined, secure base directory. Do not trust client-provided paths.

Changes

• openhands-agent-server/openhands/agent_server/vscode_router.py (modified)

[enyst]

Hey, thank you for the PR. To note, the agent does have access to the entire box, so I think checks are better via hooks or security analyzer.

On the other hand, I’m not sure this works? Maybe I’m missing something, but if the path wasn’t transmitted, there’s nothing to check

Could you prove this works? Maybe we can see some logs from a couple of runs or tests.

On a side note, could you tell what LLM and agent you used for this PR?

[all-hands-bot]

🔴 Needs improvement

The proposed boundary is hard-coded, the validation can be bypassed through a second URL-decoding step, and rejected paths currently surface as 500 rather than 400. The PR also adds no regression coverage for a security-sensitive behavior change; please add endpoint tests for an allowed workspace, traversal/absolute paths, encoded traversal (for example a request that reaches the handler as workspace/%2e%2e/etc), and a configured workspace outside the process-relative workspace directory.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This modifies a security boundary but leaves a practical bypass while potentially breaking legitimate deployments with custom workspace paths. Recommendation: Do not auto-merge; request human security/maintainer review after the boundary and URL representation are corrected and tested.

VERDICT:
❌ Needs rework: The validation does not yet enforce the intended boundary reliably.

KEY INSIGHT:
Validation and consumption must use the same canonical, safely encoded path derived from the actual configured workspace boundary.

This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement - This change breaks the URL endpoint it is trying to secure.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 51] Cross-file contract violation: get_vscode_service() returns VSCodeService, but that class has no config attribute; its constructor stores only port, connection_token, and server_base_path. A real enabled service therefore raises AttributeError at vscode_service.config.workspace_path before the guarded try block, so every /vscode/url request fails instead of returning a URL. Pass the configured workspace path through an actual typed interface or obtain configuration explicitly rather than inventing an attribute that the callee does not provide.
• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 52] Breaking default behavior: Even if the missing config attribute is wired in, the defaults are incompatible: workspace_dir="workspace" resolves beside the configured default workspace_path="workspace/project", not beneath it, so the no-argument endpoint returns 400. I reproduced this on the exact PR head with uv run pytest tests/agent_server/test_vscode_router.py::test_get_vscode_url_success -q; the existing success test fails with 400: workspace_dir must be within the workspace directory. Preserve the documented/default request while applying containment to a consistently rooted path.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py] This production behavior change adds no test changes, and an existing focused test fails on the PR head. Add coverage using the real VSCodeService contract plus default and traversal cases so a permissive MagicMock cannot hide nonexistent attributes.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  The intended containment check is security-sensitive, but the current implementation makes a user-facing endpoint unusable and applies inconsistent path semantics. No data exposure was observed; the immediate risk is service regression.

VERDICT:
❌ Needs rework: Fix the service/config boundary and preserve the default endpoint before merging.

KEY INSIGHT:
A security check that is not traced through the real service contract and default path semantics simply replaces a traversal risk with a guaranteed outage.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Broken service boundary: VSCodeService has no config attribute. The real factory constructs it with only port, connection_token, and server_base_path, so every enabled /api/vscode/url request now raises AttributeError before reaching the route's try block. This turns the endpoint into a 500 for all real users. Pass the configured workspace root into VSCodeService explicitly (or obtain it from the actual configuration boundary) and validate against that stored path; do not rely on an attribute that only an unrestricted MagicMock invents.

[IMPROVEMENT OPPORTUNITIES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Lines 69-93] Unnecessary URL surgery: FastAPI has already decoded workspace_dir before validation, and the service builds this URL itself. Hand-parsing and rebuilding the query string adds a second policy layer, special-cases a token key that the service does not use (tkn is the real key), and ends with an assertion that can never fail. Encode the validated folder once in VSCodeService.get_vscode_url with urllib.parse and return the response directly.

[TESTING GAPS]

• [PR change set] No regression test for the real boundary: The existing router tests patch get_vscode_service with an unconstrained MagicMock, which is exactly why the nonexistent .config attribute passed CI. Add focused tests using a real VSCodeService or an autospecced mock, covering the configured default root, an allowed child path, and traversal/symlink escape rejection. The test must fail if the production service and router disagree again.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes security-sensitive path validation on a public endpoint and currently breaks that endpoint for every real VSCodeService instance. Recommendation: Do not auto-merge. Request review from a human security/agent-server maintainer to validate the corrected configuration wiring, URL encoding, and path-boundary tests.

VERDICT:
❌ Needs rework: The validation goal is sound, but the implementation calls an attribute that does not exist in production.

KEY INSIGHT:
A security check is worthless if it is wired to a mock-only object shape and takes the entire endpoint down before validation runs.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Breaking interface mismatch: VSCodeService has no config attribute. Its constructor stores only port, connection_token, server_base_path, process, and the VSCode paths, and get_vscode_service() constructs it without retaining Config. Therefore every enabled /api/vscode/url request reaches vscode_service.config.workspace_path and raises AttributeError before the route's try block. Derive the workspace root from the actual application configuration or add an explicit workspace-root field to VSCodeService and pass it at construction; do not reach through an interface that does not exist.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py, Line 26] Mocks hid the regression: the unconstrained mock auto-creates .config, while this PR adds no tests for traversal rejection, valid in-root paths, the default path, or the real VSCodeService interface. Add route-level tests with a spec/real service so the test fails when the router accesses attributes the production service does not define.

VERDICT:
❌ Needs rework: The security endpoint is broken for every enabled VSCode deployment before the new validation can protect anything.

KEY INSIGHT:
Path validation is useful only after the router and service agree on who owns the workspace root.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes a security-sensitive endpoint and currently introduces a deterministic production failure on its main success path. Recommendation: Do not auto-merge. Request a human reviewer to validate the configuration ownership boundary and exercise the real endpoint after the service contract and tests are fixed.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement - The current head is not valid Python, and the previous service-boundary bug remains underneath that syntax failure.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Lines 1-9 and 132] Unimportable module: Raw assistant reasoning and Markdown code fences were committed around the source. I verified the checked-out head with python3 -m py_compile openhands-agent-server/openhands/agent_server/vscode_router.py; it fails with SyntaxError: unterminated string literal at line 7. Because the API imports this router, this can prevent the agent server from importing at all. Remove the prose and fences and commit only valid Python.
• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 63] The service contract is still invented: After the syntax wrapper is removed, this still reads vscode_service.config.workspace_path, but the real VSCodeService constructor stores only port, connection_token, server_base_path, process state, and VSCode paths; it has no config attribute. An enabled request therefore still raises AttributeError before the guarded try block. Pass an explicit workspace root through the real service interface or obtain it from the authoritative application configuration instead of relying on an attribute that only an unrestricted mock supplies.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py] No regression proof: This PR still changes no tests, and the existing unconstrained mock cannot detect the nonexistent .config contract. Add route coverage using a real or autospecced VSCodeService for the default root, an allowed child, traversal/symlink escape rejection, and successful module import; the current head must fail that test before the fix.

VERDICT:
❌ Needs rework: Restore an importable router, then fix and test workspace-root ownership through the real service contract.

KEY INSIGHT:
Security validation cannot protect an endpoint when the committed module does not parse and the validation root comes from an attribute production objects do not have.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This is a security-sensitive route change with a deterministic syntax failure that can block agent-server import, followed by a deterministic interface mismatch on the endpoint's success path. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer after the module compiles, the workspace-root ownership is explicit, and real endpoint tests pass.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement - The production service contract is still broken on the current head.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Deterministic endpoint failure: The route reads vscode_service.config.workspace_path, but VSCodeService defines no config attribute and get_vscode_service() constructs it with only port, connection_token, and server_base_path. Every enabled /api/vscode/url request therefore raises AttributeError before entering the route's try block. The current tests hide this because get_vscode_service is patched with an unrestricted MagicMock, which invents .config on demand. Make workspace-root ownership explicit in the real service interface (or read the authoritative application configuration directly) rather than relying on a mock-only attribute.

[IMPROVEMENT OPPORTUNITIES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Lines 69-94] Unnecessary URL reconstruction and assertions: VSCodeService.get_vscode_url() emits tkn, not token, so this hand-written parser does not actually preserve the token through its special branch; it merely happens to carry tkn through the generic dictionary. The two assertions are not regression protection, and the URL-shape assertion converts any legitimate non-HTTP base URL into a 500. Pass the validated canonical path into one URL-building implementation and encode its query parameters there instead of adding a second parser in the router.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py, Line 26] Permissive mocks conceal the failure: No tests changed in this security-sensitive PR. Add route tests using a real VSCodeService or an autospecced mock for the configured default root, an allowed child, .. traversal, absolute escape, symlink escape, and canonical query encoding. The existing focused suite is not available in this checkout's unsynchronized environment, but the failure is established directly by the checked-in class and factory signatures.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This modifies a security-sensitive path boundary while deterministically breaking the endpoint's production success path, and it has no regression tests for either the real service interface or traversal behavior. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer after workspace ownership, URL encoding, and real endpoint coverage are fixed.

VERDICT:
❌ Needs rework: The intended validation cannot protect an endpoint that fails before the guarded service call.

KEY INSIGHT:
A path boundary is only real when configuration ownership and URL consumption are expressed through the production interface rather than invented by a permissive mock.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once these are addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Runtime regression: VSCodeService has no config attribute—its constructor initializes port, connection_token, server_base_path, process, and OpenVSCode paths only. Every enabled /vscode/url request therefore raises AttributeError before reaching the route's try block, so this security fix makes the endpoint unusable instead of validating it. Pass the authoritative workspace root into VSCodeService explicitly (or obtain it from an existing configuration owner) and validate against that real field.

[TESTING GAPS]

• [openhands-agent-server/openhands/agent_server/vscode_router.py] This exact head ran only the PR-artifact check path; the substantive server tests were skipped. Add a route-level regression test using a real VSCodeService instance that proves the default request succeeds, an in-root path succeeds, and traversal/out-of-root paths return 400—the current default-success case would immediately catch the nonexistent attribute.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  The intended path-boundary hardening is worthwhile, but the implementation currently takes a user-facing development endpoint completely offline whenever VSCode is enabled. The blast radius is scoped to this endpoint, with no evidence of data corruption or privilege expansion.

VERDICT:
❌ Needs rework: Wire validation to an actual service/configuration field and prove the real route works.

KEY INSIGHT:
Security validation is only useful when it is attached to the real data owner; inventing a configuration attribute at the router boundary breaks the endpoint.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement — The current head is not valid Python, and the production service mismatch remains underneath that syntax failure.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Lines 1-9 and 132] Unimportable module: Raw assistant reasoning and Markdown fences are committed around the Python source. I verified this exact head (083f9a0b) with python3 -m py_compile openhands-agent-server/openhands/agent_server/vscode_router.py; it fails at line 7 with SyntaxError: unterminated string literal. Because the agent-server API imports this router, the server cannot import normally. Remove the prose and code fences and commit only valid Python.
• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 63] Invented service contract: After removing the syntax wrapper, the route still reads vscode_service.config.workspace_path. The real VSCodeService.__init__ stores port, connection_token, server_base_path, process state, and VSCode paths; get_vscode_service() constructs it without a config attribute. Every enabled /vscode/url request would therefore raise AttributeError before the guarded try block. Make workspace-root ownership explicit through the real service interface or read it from the actual configuration owner.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py] Mocks still hide both regressions: This PR changes no tests, while the existing fixture patches the service with an unrestricted mock that invents .config on demand. Add route coverage with a real or autospecced VSCodeService for module import, the configured default root, an allowed child, traversal/absolute escape, and symlink escape. The current head should fail before the fix and pass afterward.

VERDICT:
❌ Needs rework: Restore an importable router, wire validation to a real workspace-root owner, and prove the route with production-shaped tests.

KEY INSIGHT:
A path boundary cannot protect an endpoint when the module does not parse and its root comes from an attribute production objects do not have.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes a security-sensitive public endpoint and currently introduces a deterministic import failure, followed by a deterministic runtime contract failure once parsing is restored. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer after the module compiles, configuration ownership is explicit, and real route tests pass.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Broken Service Contract: Every enabled /vscode/url request now dereferences vscode_service.config.workspace_path, but the real VSCodeService has no config attribute and get_vscode_service() constructs it with only port, connection_token, and server_base_path. This happens before the try, so the route fails before URL generation instead of enforcing the new boundary. Pass the authoritative workspace root into the service (or obtain Config from its actual owner) and make that dependency explicit rather than assuming an attribute that does not exist.

[TESTING GAPS]

• [PR scope] No Regression Proof: This security-sensitive route change adds no tests even though tests/agent_server/test_vscode_router.py already exercises both direct and API-level behavior. Add real route tests for the configured default, an allowed nested path, ../absolute escape attempts, URL-encoded traversal or query delimiters, and a symlink escaping the root; assert the actual HTTP statuses and generated folder value. The production assertions at Lines 91–94 are not regression tests (response is not None is tautological, and assertions disappear under python -O).

VERDICT:
❌ Needs rework: The current implementation breaks every enabled URL request and does not prove the security boundary at the API layer.

KEY INSIGHT:
A filesystem boundary only hardens the endpoint when its root comes from a real, explicit owner and the exact validated path is proven through the route that consumers call.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This modifies a public, security-sensitive filesystem-to-URL boundary and currently introduces a runtime outage before validation can run. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer to validate ownership of the workspace root and the traversal test matrix.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement — The current head changes the commit SHA without changing the code that was previously reviewed.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Unaddressed service-contract failure: e8c7ab23 is tree-identical to the last reviewed head (git diff 74bb5e4a..e8c7ab23 is empty), so the deterministic failure remains. VSCodeService has no config attribute, and get_vscode_service() constructs it without retaining Config; every enabled /vscode/url request therefore raises AttributeError at vscode_service.config.workspace_path before entering the try block. Pass the authoritative workspace root through an explicit typed service field (or read it from its actual configuration owner) instead of dereferencing an attribute that production objects do not provide.

[TESTING GAPS]

• [PR change set] No regression proof: This security-sensitive route still adds no test changes, and the current head only has the PR-artifact check exercising it. Add route coverage against a real or autospecced VSCodeService for the configured default, a valid child, traversal/absolute escape, symlink escape, and URL characters requiring encoding; that test must fail if the router invents another service attribute.

VERDICT:
❌ Needs rework: The empty follow-up commit does not address the production outage or add proof for the path boundary.

KEY INSIGHT:
A new SHA is not an addressed review when its tree is unchanged and the same runtime contract violation remains.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This modifies a public, security-sensitive filesystem-to-URL boundary and still introduces a deterministic outage before validation executes. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer to validate explicit workspace-root ownership and the traversal/encoding test matrix.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement — The new SHA still has the exact tree that was reviewed previously.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Unaddressed service-contract failure: git diff e8c7ab23..f76b710b is empty, so the previous production failure remains unchanged. VSCodeService has no config attribute, and get_vscode_service() constructs it with only port, connection_token, and server_base_path; every enabled /vscode/url request therefore raises AttributeError at vscode_service.config.workspace_path before entering the try block. Pass the authoritative workspace root through an explicit typed service field, or read it from the configuration object that actually owns it.

[TESTING GAPS]

• [PR change set] No regression proof: This security-sensitive route still changes no tests. The unconstrained mock in tests/agent_server/test_vscode_router.py can invent .config, which is why it cannot detect this broken production boundary. Add route coverage using a real or autospecced VSCodeService for the default root, an allowed child, traversal and absolute escapes, a symlink escape, and URL characters requiring encoding.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes a public, security-sensitive filesystem-to-URL boundary and still introduces a deterministic outage before validation runs. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer to validate explicit workspace-root ownership and the route-level traversal tests.

VERDICT:
❌ Needs rework: A new commit SHA without a tree change does not address the endpoint failure.

KEY INSIGHT:
A path boundary cannot protect users when it depends on an attribute the production service does not have.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement — This new head is tree-identical to the last reviewed head, so the deterministic endpoint failure remains unchanged.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/vscode_router.py, Line 54] Unaddressed service-contract failure: fa03653c and the last enyst-reviewed head f76b710b have the same Git tree (35c8818b7c8e451dc0f55b1d745b4a668bd1858b). The route still reads vscode_service.config.workspace_path, but the real VSCodeService has no config attribute and its factory constructs it with only port, connection_token, and server_base_path. I reproduced AttributeError: 'VSCodeService' object has no attribute 'config' against this exact head; because the access happens before the try, every enabled /api/vscode/url request fails before URL generation or validation. Pass the authoritative workspace root through an explicit typed service field, or obtain it from the configuration object that actually owns it.

[TESTING GAPS]

• [tests/agent_server/test_vscode_router.py] The focused suite is already red: uv run pytest -q tests/agent_server/test_vscode_router.py fails 2 tests on this head. This PR still adds no tests, and the unconstrained MagicMock invents .config, masking the production interface mismatch while producing bogus folder paths. Add a real or autospecced service test plus the repository-required live-server route coverage for the configured default, an allowed child, traversal/absolute escapes, and a symlink escape; these tests must exercise actual HTTP status and generated folder values.

VERDICT:
❌ Needs rework: New commit SHAs with an unchanged tree do not address the endpoint outage or provide regression proof.

KEY INSIGHT:
A path boundary cannot secure an endpoint when its root comes from an attribute the production service does not have.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes a public, security-sensitive filesystem-to-URL boundary and still causes a deterministic outage before validation runs. Recommendation: Do not auto-merge. Request review from a human agent-server/security maintainer after workspace-root ownership is explicit and the real route-level traversal tests pass.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.