Skip to content

Show where a terminal link goes, and ask when its URI is built to be misread - #28

Merged
bharathm03 merged 6 commits into
developmentfrom
fix/terminal-link-disclosure
Aug 27, 2026
Merged

Show where a terminal link goes, and ask when its URI is built to be misread#28
bharathm03 merged 6 commits into
developmentfrom
fix/terminal-link-disclosure

Conversation

@bharathm03

Copy link
Copy Markdown
Contributor

Supersedes #26, which GitHub closed when #25's branch was deleted on merge and would not let me reopen after the rebase. Same branch, same content, now rebased onto development with #25 in it — nothing here is new since #26's last review.

#25 is merged, so this diff stands alone.

Why

#25 makes a plain click on an OSC 8 link open it. That leaves a question it did not answer: the app never shows the user where the link goes. The terminal view underlines the cell and swaps the cursor to a pointer; nothing paints the URI. OSC 8 lets a link's visible text disagree with its target, so the first place the destination appeared was the browser it had already been opened in.

The desktop path was skipping the confirm sheet on the stated grounds that hover already disclosed the target. It never did — that claim is corrected here and in #25's external_url.dart doc.

What

A hover readout (TerminalHyperlinkPreview, wired to the fork's new onHyperlinkHover). A card floats near the pointer naming the destination:

  • the host reads brightest, the prefix before it dimmest — so https://github.com@evil.example/… renders the familiar half as the decoration it is and the half that actually resolves as the one that stands out;
  • shown verbatim, not normalized, because the point is that the characters can be compared against what the terminal printed;
  • below the pointer, flipping above near the bottom edge — the bottom line of the panel is the prompt the user is typing into, which is why this is not the browser's status-bar placement;
  • the anchor is sampled only when the hovered URI changes, so a hover frame costs no rebuild and the card does not slide around under the cursor it describes.

A deceptive-shape check (terminalHyperlinkLooksDeceptive) that pulls a link back to the confirm sheet on desktop too, where the URI is written to be misread at a glance: a userinfo prefix, a punycoded label, or a percent-encoded host (Dart percent-encodes a raw unicode host rather than punycoding it, so both spellings of the same lie have to be caught).

What it does not catch

https://github.com.evil.example/ — an honest subdomain of an honest domain. Only the link's visible text contradicts it, and that text never reaches the app; onOpenHyperlink receives the URI alone. The test names that gap explicitly so it is not mistaken for coverage. Closing it needs the fork to pass the anchor text through, which is a separate change.

Two things the review left open, dug out afterwards

The desktop exemption asked the wrong question. It tested defaultTargetPlatform, which is about the OS, not about what the user was shown — so a finger tap on a Windows or Linux touchscreen took the desktop branch on the grounds that the hover readout had disclosed the destination, and there is no hover on touch. Same hole for a Shift chord and for a link that scrolled out from under a resting pointer. The review called the right predicate unavailable ("the pointer kind lives in the terminal package"); it isn't needed. The terminal already knows whether its readout is up and for which URI, so it passes disclosed and the platform test is gone — along with the class of bug where a new input path silently inherits an exemption nobody measured it for. It defaults to false: "we showed it" belongs to the surface that showed it.

The hover was rebuilding the whole transcript, and it was my regression. The review flagged this and skipped it as fork-internal. It came from #6: putting _explicitHyperlinkAt in the onHover guard means every hover resolves a cell through _positionForOffset, which asked controller.snapshot whether the buffer was empty — and reading the snapshot settles the styled formatter (three full-buffer passes plus a re-parse). Under renderState the painter never reads that snapshot, so the hover paid the whole cost alone, and any output since the last read marks it stale — which in an agent session is every frame.

Measured at ~17ms per hover event on an 1800-line scrollback: 40 frames took 862ms with the pointer moving against 179ms with it still. Fixed in antgrid-ai/dart_terminal#7 by asking the engine's own row total, which _scrollableLineCount already prefers, and guarded by a test that compares those same two loops rather than a millisecond bound that would drift with the machine.

An earlier micro-benchmark of mine looked like it ruled this out. It measured hyperlinkUriAt, which reads the engine grid — a different function on a different path from the one that was costing the time.

Fork pin

dependency_overrides and THIRD-PARTY.md point at 6cd3931, antgrid-ai/dart_terminal master with #7 merged — the hover callback and the _positionForOffset fix. No PR-branch ref is left in the pin.

Review pass

/code-review --fix at max effort found the readout confirming the lie in three ways, all fixed here:

  • locating the host by searching the payload for the parsed value finds the FIRST occurrence, so https://b.com.evil@b.com/x painted the impostor bright;
  • Uri.host percent-encodes a raw unicode host, so it never occurs in the payload and the homoglyph case silently lost its emphasis;
  • one rich run elides its TAIL, which behind a padded userinfo is the host — the card rendered as a clean GitHub URL.

The host is now located positionally and laid out as the only non-flex sibling, so it is cut last. Bidi/control characters are spelled out as the launcher receives them, a userinfo password is masked, and each part is capped so an unbounded payload is not shaped in full on the UI thread. The anchor moved to a pending-URI handshake with an enclosing MouseRegion (the view reports from a region below ours and hit paths dispatch child-first, so the old read was one event stale — and Offset.zero on the first hover); hover state is a ValueNotifier, so crossing a wall of links no longer rebuilds the panel.

Gates

  • flutter test — 2639 pass; fork package +212 -28, the 28 being that tree's pre-existing failures, unchanged
  • flutter analyze lib test — clean
  • npm run check:font-tokens — clean
  • Both new desktop confirm tests negative-checked: reverting the gate line fails exactly those two and nothing else.

…ktop too

Desktop skipped the confirm sheet on the grounds that the browser's address bar discloses the destination. It does -- but disclosure is not the same as being read, and OSC 8 payloads exist that are written specifically to be misread at a glance: a `github.com@` userinfo prefix parks a familiar name where the eye stops, and a punycoded or percent-encoded host spells a lookalike in characters that render as the real thing.

Pull those three shapes back to the sheet, which names the host on its own line. Judged from the URI alone, which is all a terminal hyperlink hands over -- so a lookalike that is honestly its own host (`github.com.evil.example`) is not caught, and the test says so by name rather than leaving the gap to be mistaken for coverage.
The terminal view paints an OSC 8 cell underlined and swaps the cursor to a pointer, and that is the whole affordance -- nothing anywhere shows the URI. Since the link's visible text is free to disagree with its target, the first place the destination appeared was the browser it had already been opened in.

Wire the fork's new `onHyperlinkHover` to a readout that floats near the pointer: the host reads brightest, the prefix before it dimmest, so a userinfo impostor renders as the decoration it is. Parked below the pointer and flipped above near the bottom edge, because the bottom line of the panel is the prompt the user is typing into.

The pointer position is sampled only at the instant the hovered URI changes, so a hover frame costs no rebuild and the card does not slide around under the cursor it belongs to.

The fork pin moves to the hover-callback branch commit; it must be re-pinned to the merged master SHA before this lands.
Review of the first cut found the readout confirming the lie in three ways. Locating the host with `indexOf` of the parsed value finds the FIRST occurrence, so `https://b.com.evil@b.com/x` painted the userinfo copy bright; a raw unicode host never occurs in the payload at all, because `Uri.host` percent-encodes it, so the homoglyph case silently lost its emphasis; and a single rich run elides its TAIL, which behind a padded userinfo IS the host — the card rendered as a clean GitHub URL.

Locate the host positionally instead (scheme, authority, last `@`, bracketed IPv6 or port colon), and lay the readout out as three siblings with the host as the only non-flex child, so the elidable parts surrender their width first and the host is cut last.

Also: spell out bidi and control characters the way the launcher receives them, mask the password half of a userinfo (a hover is not consent to display a secret), and cap each part so an unbounded payload is not shaped in full on the UI thread.

The anchor now comes from an enclosing MouseRegion through a pending-URI handshake: the view reports the URI from a region BELOW ours and hit paths dispatch child-first, so reading the position at that instant was always one event stale — and Offset.zero on the first hover, which parked the card in the corner. That region's onExit is also what clears a card whose terminal was unmounted under the pointer. Hover state moved to a ValueNotifier so crossing a wall of links no longer rebuilds the whole panel.

The preview tests were pumping without the palette extension, so both sides of every colour assertion resolved to the same fallback.
…transcript

PR #6 put a cell resolution in the onHover guard, and that resolution asked the controller's snapshot whether the buffer was empty -- which settles the styled formatter. In renderState mode, which this app uses, the painter never reads that snapshot, so the hover paid a full transcript rebuild alone: ~17ms per hover event over a terminal that is still producing output, which is every hover in an agent session.

antgrid-ai/dart_terminal#7 now asks the engine's own row total instead.
…ination

The desktop branch tested `defaultTargetPlatform`, which answers a question about the OS, not about what the user was shown. A finger tap on a Windows or Linux touchscreen took that branch on the grounds that the hover readout had disclosed the destination -- and there is no hover on touch. Same hole for a Shift chord, and for a link that scrolled out from under a resting pointer between the hover and the click.

The terminal knows the answer without asking anyone: the readout is up, for a specific URI, or it is not. Pass that in as `disclosed` and the platform test disappears -- along with the class of bug where a new input path silently inherits an exemption it was never measured for.

Defaults to false, so a caller with no readout cannot inherit a claim by omission: "we showed it" belongs to the surface that showed it.
The hover callback and its perf fix are on antgrid-ai/dart_terminal master now, so the pin no longer has to name a PR branch that could be rewritten or deleted under us.
@bharathm03
bharathm03 merged commit 756d322 into development Aug 27, 2026
4 checks passed
@bharathm03
bharathm03 deleted the fix/terminal-link-disclosure branch August 27, 2026 13:09
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.

1 participant