Skip to content

fix(desktop): open safe local and Obsidian response links - #75457

Draft
nullbiscuits wants to merge 2 commits into
NousResearch:mainfrom
nullbiscuits:fix/desktop-local-links-current-v3
Draft

fix(desktop): open safe local and Obsidian response links#75457
nullbiscuits wants to merge 2 commits into
NousResearch:mainfrom
nullbiscuits:fix/desktop-local-links-current-v3

Conversation

@nullbiscuits

Copy link
Copy Markdown

Summary

  • enable approved local document/media links and obsidian://open links in assistant response Markdown
  • validate links independently in the renderer and Electron main process
  • preserve the existing trusted artifact opener through a separate response-link IPC route
  • avoid command-shell parsing on WSL by invoking rundll32.exe directly

Security boundaries

  • rejects UNC/network and credentialed file: URLs
  • permits only a passive local-file extension allowlist for model-authored response links
  • rejects encoded path separators and active/double-extension targets
  • restricts Obsidian links to the open action
  • keeps renderer checks for UX and main-process checks as the security boundary

Verification

  • npm run check:lint — passed (0 errors; existing warnings only)
  • focused Vitest suite — 39/39 passed
  • git diff --check origin/main..HEAD — passed
  • npm run build — passed
  • independent fail-closed review — passed with no security concerns or logic errors

The broader UI run passed 3,164/3,172 tests. Six transient failures passed in isolation; the remaining two Billing failures were reproduced unchanged on a pristine origin/main baseline and are unrelated to this patch.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-decision Awaiting maintainer decision before any implementation labels Jul 31, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the response-link IPC capability and keeping the main-process policy fail-closed. The Obsidian part addresses a current-main gap: apps/desktop/electron/main.ts:1321-1323 only permits http:, https:, and mailto: after the file: branch.

Problems

  • apps/desktop/src/components/assistant-ui/markdown-text.tsx:632 passes remarkPlugins={[remarkDesktopLinks]}. Streamdown treats a supplied remarkPlugins value as the complete plugin list, so this replaces its default remark plugins rather than extending them. That regresses default GFM parsing for every Desktop markdown response.

Suggested changes

  • Extend Streamdown's exported defaultRemarkPlugins with remarkDesktopLinks, rather than supplying only the new plugin, and add a regression test for an existing GFM construct alongside the local-link tests.

This is an automated hermes-sweeper review.

@@ -612,6 +632,7 @@ function MarkdownTextSurface({
parseMarkdownIntoBlocksFn={parseMarkdownIntoBlocksCached}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This custom list replaces Streamdown's default remarkPlugins; it does not append to them. Please extend defaultRemarkPlugins with remarkDesktopLinks so existing GFM parsing remains enabled for every Desktop response.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants