Repository navigation
fix(conversation-card): open VS Code URL with noopener and http(s) only - #1165
juanmichelini wants to merge 8 commits into
Conversation
The VS Code URL from the backend is opened in a new tab. Reject any URL that is not http or https (or fails to parse) and open it with "noopener,noreferrer" so a malicious value (javascript:, data:, file:) cannot reach the opener window. Mirrors OpenHands/OpenHands#19229 for the OHE legacy conversation card. Co-authored-by: openhands <openhands@all-hands.dev>
|
Preview verification finished: NEEDS CLARIFICATION. Evidence: https://github.com/OpenHands/saas-deploy/pull/1934#issuecomment-6622546155 This comment was posted by an AI agent (OpenHands) on behalf of the user. |
…ndle) Add .pr/evidence/bundle-proof.md showing the PR's fix is correctly compiled into the OHE legacy frontend bundle (served at /), while the verifier inspected the /canvas bundle built from OSS v1.35.0 (a separate SPA that the issue excludes from scope). Includes grep proof from the built bundle. Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ PR Artifacts Cleaned Up The |
|
Preview verification failed. Details: https://github.com/OpenHands/saas-deploy/pull/1942#issuecomment-6630180970 This comment was posted by an AI agent (OpenHands) on behalf of the user. |
|
Preview verification finished: NEEDS CLARIFICATION. Evidence: https://github.com/OpenHands/saas-deploy/pull/1944#issuecomment-6630291968 This comment was posted by an AI agent (OpenHands) on behalf of the user. |
transformVSCodeUrl now returns null for non-http(s)/unparsable URLs, which made the VS Code tab render the literal text "null". Report VSCODE$URL_NOT_AVAILABLE instead. Add tests for the hook and for the conversation card opening the URL with noopener,noreferrer. Co-authored-by: openhands <openhands@all-hands.dev>
|
The last preview verification (saas-deploy evidence) returned NEEDS CLARIFICATION because the preview went down mid-run. Changes in bbba635:
I re-applied This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
Review posted for
|
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
The code changes look correct and implement the security fix properly:
- ✅
transformVSCodeUrlnow validates URLs and rejects non-http(s) protocols - ✅
window.openusesnoopener,noreferrer - ✅
useUnifiedVSCodeUrlhandles rejected URLs with proper error message - ✅ Tests comprehensively cover valid URLs, invalid URLs, and malicious payloads (
javascript:,data:,file:) - ✅ PR follows the template with all required sections
- ✅ Python tests, linting, and E2E tests pass
However, there is a blocking issue:
❌ FE Unit Tests (22) is failing
The FE Unit Tests check shows a failure. This must be resolved before the PR can be approved. Please review the test output to identify and fix the failing test.
🔄 CHANGES REQUESTED
LLM profile: sonnet-4-5 · Model: openhands/claude-sonnet-4-5-20250929
|
Preview verification finished: PASS. Evidence: https://github.com/OpenHands/saas-deploy/pull/2025#issuecomment-6674872628 This comment was posted by an AI agent (OpenHands) on behalf of the user. |
Co-authored-by: openhands <openhands@all-hands.dev>
The signup-attribution test from #997 calls rememberCloudEntry under the location stub from this file, which had only href. With origin undefined, redirectUrl?.origin === window.location.origin was true for a null redirectUrl and the code read null.searchParams. Browsers always set location.origin, so stub it like a real Location. Co-authored-by: openhands <openhands@all-hands.dev>
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Previous review status: The commit 4b429633dde5 was approved after fixing a test failure.
Changes since previous review:
- ✅ Only cleanup commit: The new head
da6fbd37bcontains a single automated commit that removes.pr/evidence/bundle-proof.md - ✅ Core security fix unchanged: All security changes remain intact (protocol validation,
noopener,noreferrer, error handling) - ✅ PR marked ready: No longer in draft,
verify-thislabel removed, indicating validation is complete - ✅ Live evidence provided: Screenshots added to PR description showing preview deployment verification
Summary:
The automated cleanup removed the .pr/ directory artifacts after the previous approval, as documented in AGENTS.md. The core security fix (protocol allow-list, noopener,noreferrer, rejected URL handling) remains unchanged and correctly implemented. All tests pass, and the PR has been validated on a preview deployment.
✅ APPROVED
LLM profile: unknown · Model: openhands/claude-sonnet-4-5-20250929
|
Blocked, @juanmichelini: preview verification produced no verdict 2 times in a row (verifier or preview problem, not necessarily this PR). Remove This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
|
Preview verification failed: no result 60 minutes after This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
|
Preview verification failed: no result 60 minutes after This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
|
Blocked, @juanmichelini: preview verification produced no verdict 2 times in a row (verifier or preview problem, not necessarily this PR). Remove This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
|
Preview verification failed: no result 60 minutes after This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
|
Preview verification failed. Details: https://github.com/OpenHands/saas-deploy/pull/2157#issuecomment-6774465746 This comment was posted by an AI agent (OpenHands) on behalf of the user. |
|
Blocked, @juanmichelini: preview verification produced no verdict 2 times in a row (verifier or preview problem, not necessarily this PR). Remove This comment was posted by an AI agent (OpenHands dev-coordinator) on behalf of the user. |
🔍 Preview Verification — PASSTarget: Replicated fleet VM shared-3 App: https://app.shared-3.replicated.all-hands-testing.dev Result: PASS(Decided from the baseline alone, as instructed. The feature itself was not verified.) Baseline smoke
Feature test — VS Code URL protocol allow-list / noopener
Key screenshotsWhat was tested
Skipped / manual review
EvidenceFull log: evidence/run-log.json Generated by an AI agent (OpenHands). |



HUMAN:
AGENT:
Why
The legacy OHE frontend opens the backend-provided VS Code URL with no protocol allow-list. A malicious value (
javascript:,data:,file:) could run against the opener window, and an unparsable URL was passed through unchanged. The conversation card also opened it withoutnoopener. This is the OSS→OHE parity gap tracked in #1149, mirroring OpenHands/OpenHands#19229. The Canvas bundle bump does not reach these files (P10: change the right frontend), so the fix goes into OHE's own copy.Summary
frontend/src/utils/vscode-url-helper.ts: reject URLs that fail to parse or are nothttp:/https:(returnnull). The localhost rewrite still applies.frontend/src/components/features/conversation-panel/conversation-card/conversation-card.tsx: open the transformed URL with"noopener,noreferrer".frontend/src/hooks/query/use-unified-vscode-url.ts: when the helper rejects the sandbox'sVSCODEURL, reportVSCODE$URL_NOT_AVAILABLE. Without this the VS Code tab rendered the literal textnull.__tests__/utils/vscode-url-helper.test.ts(allow-list and rejected cases),__tests__/hooks/query/use-unified-vscode-url.test.tsx(rejected URLs → "not available"),__tests__/components/features/conversation-panel/conversation-card.test.tsx(card opens withnoopener,noreferrer; never opens ajavascript:URL).Issue Number
Fixes #1149
How to Test
cd frontend npx vitest run __tests__/utils/vscode-url-helper.test.ts \ __tests__/hooks/query/use-unified-vscode-url.test.tsx \ __tests__/components/features/conversation-panel/conversation-card.test.tsx \ __tests__/routes/vscode-tab.test.tsx --no-coverageAll pass locally.
eslintandprettier --checkpass on the changed files, andnpm run buildsucceeds.How to verify on a preview
Where the change is visible. Only in the OHE legacy frontend, never in
/canvas./canvasis the separate OSSv1.35.0bundle, which this PR does not change./redirects to/canvas, so open the legacy routes below by direct URL.The conversation card is not a UI surface. The sidebar conversation panel renders
ConversationCardwithoutshowOptions, so its "Download via VS Code" menu item is never shown (conversation-card-actions.tsx:conversationId && showOptions ? onDownloadViaVSCode : undefined). Do not try to reach it. The unit tests cover the card.The UI surface is the VS Code ("Code") tab of the legacy conversation page. It runs the same
transformVSCodeUrlon the sandbox'sVSCODEexposed URL (useUnifiedVSCodeUrl→GET /api/v1/sandboxes?id=<sandbox_id>).Below,
$APPis the preview origin, for examplehttps://pr-NNNN.staging.all-hands.dev.Step 0: bundle check (no sandbox needed)
Expected: one
conversation-*.jschunk prints all three markers. The third marker is the new branch inuseUnifiedVSCodeUrl. Onmainit is absent.Step 1: get a conversation with a running sandbox
task-<hex>, callGET $APP/api/v1/app-conversations/start-tasks?ids=<hex>and readapp_conversation_id.GET $APP/api/v1/app-conversations?ids=<conversation_id>untilsandbox_statusisRUNNING. Notesandbox_id.GET $APP/api/v1/sandboxes?id=<sandbox_id>should list anexposed_urlsentry withname: "VSCODE"and anhttps://URL.Step 2: positive case (real URL)
$APP/conversations/<conversation_id>(legacy UI, not/canvas/...). Wait for the agent to be ready.[data-testid="conversation-tab-vscode"]).<iframe title="VS Code">whosesrcis thehttps://sandbox URL. If the protocols differ, an "Open in New Tab" button is shown instead. Either outcome passes. The textVS Code URL not availablemust not be shown.Step 3: negative cases (injected URL)
For each value in
javascript:alert(1),data:text/html,<script>alert(1)</script>,file:///etc/passwd,not-a-valid-url:VSCODEURL. Playwright example:$APP/conversations/<conversation_id>in a fresh page (so the 5-minute sandbox query cache is empty), then click the Code tab.VS Code URL not available. There is noiframe[title="VS Code"], no "Open in New Tab" button, no new page or popup, and noalertdialog.main): forjavascript:/data:/file:the tab shows the cross-origin warning with an "Open in New Tab" button that wouldwindow.openthe injected URL. Fornot-a-valid-urlit shows "Error parsing URL".Verdict rule: PASS if Step 0 prints the three markers, Step 2 shows the editor (iframe or button), and every Step 3 value shows only "VS Code URL not available". If no sandbox reaches
RUNNING, Step 0 plus the unit tests are the evidence. Report that sandbox start failed. It is not a defect in this PR.Video/Screenshots
details
Type
Notes
No dependency on the OSS pin bump; the OSS PR is not in a tagged release. The helper and card diffs mirror OSS, plus an inline
// eslint-disable-next-line no-script-urlonjavascript:test literals (OHE's airbnb eslint config flags it). TheuseUnifiedVSCodeUrlchange is OHE-only. OHE's VS Code tab would otherwise printnullfor a rejected URL.This PR description was created by an AI agent (OpenHands) on behalf of @juanmichelini.
Enterprise server image for this PR: