Skip to content

Fix OSC 8 links using Wave terminal web-link handling - #3528

Merged
sawka merged 2 commits into
mainfrom
cosmos/osc8-wave-weblinks
Sep 25, 2026
Merged

sawka merged 2 commits into
mainfrom
cosmos/osc8-wave-weblinks

Conversation

@sawka

@sawka sawka commented Sep 25, 2026

Copy link
Copy Markdown
Member

Summary

  • Route OSC 8 hyperlinks and detected web URLs through the same terminal link handlers.
  • Preserve Cmd-click on macOS / Ctrl-click elsewhere, hover tooltip and context-menu state, and the web:openlinksinternally behavior in openLink.
  • Leave xterm’s default HTTP(S)-only OSC link filtering in place.

Closes #3165.

Verification

  • npm exec -- vitest run frontend/app/view/term/term-links.test.ts frontend/app/view/term/osc-handlers.test.ts --reporter=dot (6 passed)
  • Prettier and ESLint on changed files (passed)
  • task check:ts remains blocked by pre-existing type errors in frontend preview mocks, unrelated to these changes.
  • No runtime click test was performed.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 129f0386-746c-46e4-ae0e-60a665250fe2

📥 Commits

Reviewing files that changed from the base of the PR and between 9f47f65 and 825e2e1.

📒 Files selected for processing (5)
  • frontend/app/view/term/term-links.test.ts
  • frontend/app/view/term/term-links.ts
  • frontend/app/view/term/term-tooltip.test.tsx
  • frontend/app/view/term/term-tooltip.tsx
  • frontend/app/view/term/termwrap.ts
 ________________________________________________________
< Maybe I am just like my mother. She's never satisfied. >
 --------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: a390556c-cd89-49e9-ba1c-a3ba8f25d081

📥 Commits

Reviewing files that changed from the base of the PR and between 215171d and 9f47f65.

📒 Files selected for processing (3)
  • frontend/app/view/term/term-links.test.ts
  • frontend/app/view/term/term-links.ts
  • frontend/app/view/term/termwrap.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

The change adds makeTermLinkHandlers for terminal link activation and hover events. TermWrap passes these handlers to the terminal options and WebLinksAddon. Tests cover platform-specific modifier clicks, default-action prevention, and hover and leave callback values.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 9f47f

Terminal links use the shared click and hover handlers without an identified regression. The change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 9f47f

Terminal links now reach the existing internal or external link-opening flow. Modifier-click remains required, but it has not been established whether the new path excludes non-web URI schemes before they reach that flow.

Retained concerns

  • Medium · security · inferred: The new OSC 8 route depends on upstream scheme filtering that could not be verified. If a non-web URI reaches the custom handler, modifier-click forwards it to the existing internal or external opening sink without another scheme check.
Security review details

Security Blast Radius

  • inferred — The newly reachable input is hyperlink data displayed in a frontend terminal session. A user’s permitted activation can pass it to a web block or the application’s external-opening API; the evidence does not establish broader tenant or credential exposure.

Security Findings and Attack Paths

  • inferred — A terminal producer could supply an OSC 8 URI for a user to activate. Whether non-HTTP(S) schemes can reach the custom callback—and therefore the opening sink—remains unverified; this is not a confirmed exploit path.

Trust Boundaries and Controls

  • observed — Both link consumers receive the same modifier-gated activation callback. Scheme selection is not enforced in that callback or its destination, so the effective OSC 8 scheme boundary depends on terminal-library behavior not verified by the available source.

Resilience and Maintainability Implications

  • inferred — Sharing hover state across two link consumers makes interruption and callback ordering relevant to which URI a context menu sees. The evidence does not establish a newly exploitable stale-state path; context-menu opening was already a separate, explicit user action.

Hardening Proposals

  • proposed — Verify the terminal library’s OSC 8 filtering with real clicks for web and non-web schemes; if custom handlers can receive non-web schemes, enforce the intended scheme policy before opening them.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing OSC 8 links by using Wave terminal web-link handling.
Description check ✅ Passed The description directly explains the link-handler changes, preserved behavior, verification results, and the related issue.
Linked Issues check ✅ Passed Issue [#3165] requires OSC 8 links to open in the system browser after confirmation. TermWrap now assigns the shared linkHandlers to xterm's linkHandler, so OSC 8 activation uses the existing `o…
Out of Scope Changes check ✅ Passed The changed files support issue [#3165]. makeTermLinkHandlers centralizes link activation and hover behavior. TermWrap wires the handler to OSC 8 links and detected web URLs. The added tests verif…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Deploying waveterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 825e2e1
Status:⚡️  Build in progress...

View logs

@sawka
sawka merged commit c58bf7f into main Sep 25, 2026
3 of 8 checks passed
@sawka
sawka deleted the cosmos/osc8-wave-weblinks branch September 25, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OSC 8 hyperlinks show confirmation dialog but do not open browser

1 participant