Validate the scheme before shell.openExternal - #72
Open
rafaelfiguereod-stack wants to merge 1 commit into
Open
rafaelfiguereod-stack wants to merge 1 commit into
rafaelfiguereod-stack wants to merge 1 commit into
Conversation
The APP_OPEN_EXTERNAL handler passed the renderer's URL straight to the shell. The chat's markdown renderer sends it anchor.href for any link the assistant wrote, and a relative link has no scheme for DOMPurify to reject, so it resolves against the renderer's base. In a packaged build that base is file://, which makes [x](../../payload.exe) arrive here as a path on disk the shell would open. Only http, https and mailto get through now. Those cover every link the app opens: the sign-in redirect, checkout, the billing portal, and ordinary links in chat. A refused link is logged by scheme alone, so a token in a rejected URL stays out of the log. Explicit file://, javascript: and custom schemes were already stripped by the sanitizer, so relative links were the way through. Refs diffusionstudio#71 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This branch was previously deployed
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.
Fixes the hardening report in #71.
The problem
APP_OPEN_EXTERNALpassed the renderer's URL straight to the shell:The chat's markdown renderer sends it
anchor.hreffor any link the assistant writes. A relative link has no scheme for DOMPurify to reject, so it survives and resolves against the renderer's base. In a packaged build that base isfile://, so[x](../../../Downloads/payload.exe)reaches this handler as an absolute path on disk, andshell.openExternalopens it with the OS handler.Explicit
file:///...,javascript:and custom schemes were already stripped by the sanitizer. Relative links were the way through.The change
A small predicate in
apps/desktop/src/external-url.ts, and the handler consults it. Onlyhttp:,https:andmailto:get through.That covers every link the app opens today: the sign-in redirect in
auth.tsx, checkout and the billing portal incheckout.ts, and ordinary links in chat. I includedmailto:because DOMPurify already permits it and you have support addresses in the UI, so an assistant writing one should keep working. If you would rather be strict, deleting"mailto:"from the set is the whole change.A refused link is logged by scheme alone, never the full URL, so a token in a rejected link stays out of the terminal.
I put the check in main rather than in the renderer's click handler on purpose. Main is the trust boundary, so it is the side that still holds if the renderer is ever wrong. A renderer side check would give a nicer no-op on click and could be added on top.
Verified
Windows 11, Node 24.1.0, Electron 43, on top of b317412.
I reproduced the original chain first, by running the repo's own
render()and DOMPurify hook bundled against the repo'sdompurifyandmarkedinside aBrowserWindowloaded overfile://, and reading backanchor.href. Then I fed those exact observed hrefs through the new predicate:Also:
npm run check --workspace=@diffusionstudio/desktop: cleannpm test --workspace=@diffusionstudio/desktop: 63 passed across 9 files, up from 59 across 8, the 4 new ones areexternal-url.test.tsdist/main.jsand launched the app, which comes up and answersdapi contextanddapi screenshotas beforeOne note on the branch: this is cut from
main, sonpm run checkacross all workspaces still fails here for the unrelated missing type dependency in #70. The desktop workspace itself is clean.🤖 Generated with Claude Code