From 9c78ffb117162613076e3e3435b409b03caaa5f1 Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:50:07 +0530 Subject: [PATCH 1/6] fix: confirm a terminal link whose URI is built to be misread, on desktop 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. --- app/lib/util/external_url.dart | 40 ++++++++- app/test/terminal_hyperlink_test.dart | 119 +++++++++++++++++++++++++- 2 files changed, 157 insertions(+), 2 deletions(-) diff --git a/app/lib/util/external_url.dart b/app/lib/util/external_url.dart index ecec7932..c494c84e 100644 --- a/app/lib/util/external_url.dart +++ b/app/lib/util/external_url.dart @@ -105,7 +105,8 @@ Future openTerminalHyperlink( } return; } - if (_browserRevealsDestination) { + if (_browserRevealsDestination && + !terminalHyperlinkLooksDeceptive(target)) { await open(context, target.toString()); return; } @@ -130,6 +131,39 @@ Future openTerminalHyperlink( } } +/// Whether [target] is shaped like a link trying to pass as another one. +/// +/// Judged from the URI alone, which is all a terminal hyperlink hands over. +/// Three shapes qualify, and each is a host claiming to be a host it is not: +/// +/// * A userinfo prefix — `https://github.com@evil.example/` resolves to +/// `evil.example` while reading as GitHub. Dart parks the impostor in +/// [Uri.userInfo], where nothing in a URL bar's first glance looks at it. +/// * A punycoded label — `xn--pple-43d.com` renders as `apple.com` +/// with a Cyrillic first letter (U+0430). +/// * A percent-encoded host — Dart does NOT punycode a raw unicode host, it +/// percent-encodes it, so the same lie arrives spelled the other way and a +/// check for `xn--` alone misses half of it. +/// +/// Deliberately not a blocklist of hosts, and deliberately silent about the +/// lookalike it cannot see: `https://github.com.evil.example/` is an honest +/// subdomain of an honest domain, and only the link's visible TEXT contradicts +/// it — text that never reaches this app. A false negative here costs the user +/// the extra confirmation, not the disclosure: the sheet names the host either +/// way, and on touch it is shown unconditionally. +bool terminalHyperlinkLooksDeceptive(Uri target) { + if (target.userInfo.isNotEmpty) { + return true; + } + final host = target.host; + if (host.contains('%')) { + return true; + } + // `Uri` lower-cases the host as it parses, but the loop is what a reader + // checks, not the parser's contract two files away. + return host.split('.').any((label) => label.toLowerCase().startsWith('xn--')); +} + /// Whether opening the link lands somewhere that reads the destination back. /// /// Desktop hands the URI to a browser whose address bar does, which is the same @@ -142,6 +176,10 @@ Future openTerminalHyperlink( /// the destination can be acted on without ever being shown. That is why the /// mobile path asks first, and why the answer to a spoofed target on desktop is /// a hover preview rather than the same sheet on both. +/// +/// An address bar only discloses what the reader then has to judge, so this is +/// not the whole desktop rule: [terminalHyperlinkLooksDeceptive] pulls the +/// cases back to the sheet where the URI is built to be misread. bool get _browserRevealsDestination => defaultTargetPlatform != TargetPlatform.android && defaultTargetPlatform != TargetPlatform.iOS; diff --git a/app/test/terminal_hyperlink_test.dart b/app/test/terminal_hyperlink_test.dart index f942d3b3..7e7faaee 100644 --- a/app/test/terminal_hyperlink_test.dart +++ b/app/test/terminal_hyperlink_test.dart @@ -66,6 +66,72 @@ void main() { }); }); + group('terminalHyperlinkLooksDeceptive', () { + test('catches a host wearing another host as userinfo', () { + expect( + terminalHyperlinkLooksDeceptive( + Uri.parse('https://github.com@evil.example/antgrid/pull/13'), + ), + isTrue, + ); + expect( + terminalHyperlinkLooksDeceptive( + Uri.parse('https://user:pw@evil.example/'), + ), + isTrue, + ); + }); + + // Both spellings of the same lie: Dart punycodes nothing, so a raw unicode + // host arrives percent-encoded and an `xn--` check alone would pass it. + test('catches a homoglyph host, however it was spelled', () { + expect( + terminalHyperlinkLooksDeceptive( + Uri.parse('https://xn--pple-43d.com/login'), + ), + isTrue, + ); + final raw = Uri.parse('https://\u0430pple.com/login'); + expect(raw.host, contains('%')); + expect(terminalHyperlinkLooksDeceptive(raw), isTrue); + // A label merely containing the marker is not one starting with it. + expect( + terminalHyperlinkLooksDeceptive(Uri.parse('https://myxn--co.com/a')), + isFalse, + ); + }); + + // The cost of a false positive is a sheet on every ordinary link, which is + // how a confirmation stops being read at all. + test('passes ordinary links, ports and case included', () { + for (final ok in [ + 'https://github.com/antgrid-ai/antgrid/pull/13', + 'https://sub.github.com/ok', + 'https://github.com:443/ok', + 'https://gitHUB.com/OK', + 'http://192.168.1.9:8080/admin', + 'http://localhost:3000/', + ]) { + expect( + terminalHyperlinkLooksDeceptive(Uri.parse(ok)), + isFalse, + reason: ok, + ); + } + }); + + // Named so the gap is not mistaken for coverage: only the link's visible + // text contradicts this one, and that text never reaches the app. + test('does NOT catch a lookalike that is honestly its own host', () { + expect( + terminalHyperlinkLooksDeceptive( + Uri.parse('https://github.com.evil.example/antgrid'), + ), + isFalse, + ); + }); + }); + group('openTerminalHyperlink', () { late Directory tmp; late String logPath; @@ -218,7 +284,7 @@ void main() { expect(asked?.host, 'evil.example'); }); - testWidgets('desktop opens without asking -- hover already showed it', ( + testWidgets('desktop opens an ordinary link without asking', ( tester, ) async { // Cleared inside the body, not in addTearDown: the framework asserts @@ -245,6 +311,57 @@ void main() { expect(asked, 0); expect(launched, ['https://example.com/a']); }); + + testWidgets('desktop still asks when the URI is built to be misread', ( + tester, + ) async { + debugDefaultTargetPlatformOverride = TargetPlatform.macOS; + + final context = await pumpHost(tester); + final launched = []; + Uri? asked; + + await openTerminalHyperlink( + context, + 'https://github.com@evil.example/antgrid/pull/13', + open: (_, url) async => launched.add(url), + confirm: (_, target) async { + asked = target; + return true; + }, + ); + await tester.pump(); + + debugDefaultTargetPlatformOverride = null; + + // An address bar would have shown this too -- and been read as GitHub, + // which is the whole reason the sheet names the host on its own line. + expect(asked?.host, 'evil.example'); + expect(launched, [ + 'https://github.com@evil.example/antgrid/pull/13', + ]); + }); + + testWidgets('desktop cancelling a deceptive link launches nothing', ( + tester, + ) async { + debugDefaultTargetPlatformOverride = TargetPlatform.macOS; + + final context = await pumpHost(tester); + var launched = 0; + + await openTerminalHyperlink( + context, + 'https://xn--pple-43d.com/login', + open: (_, _) async => launched++, + confirm: (_, _) async => false, + ); + await tester.pump(); + + debugDefaultTargetPlatformOverride = null; + + expect(launched, 0); + }); }); group('showTerminalHyperlinkSheet', () { From 22cd1d6d4a5d744f3830718cf3c4b6d97cf899b5 Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:50:18 +0530 Subject: [PATCH 2/6] feat: show where a terminal link goes before it is opened 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. --- THIRD-PARTY.md | 2 +- .../widgets/terminal_hyperlink_preview.dart | 184 ++++++++++++++++++ app/lib/widgets/terminal_view_wrapper.dart | 138 +++++++++---- app/pubspec.lock | 12 +- app/pubspec.yaml | 6 +- app/test/terminal_hyperlink_preview_test.dart | 157 +++++++++++++++ 6 files changed, 450 insertions(+), 49 deletions(-) create mode 100644 app/lib/widgets/terminal_hyperlink_preview.dart create mode 100644 app/test/terminal_hyperlink_preview_test.dart diff --git a/THIRD-PARTY.md b/THIRD-PARTY.md index 681c474f..2eff2f5c 100644 --- a/THIRD-PARTY.md +++ b/THIRD-PARTY.md @@ -114,7 +114,7 @@ ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d ``` `ghostty_vte_flutter` and `portable_pty` are pinned to the same repository and diff --git a/app/lib/widgets/terminal_hyperlink_preview.dart b/app/lib/widgets/terminal_hyperlink_preview.dart new file mode 100644 index 00000000..51352e6f --- /dev/null +++ b/app/lib/widgets/terminal_hyperlink_preview.dart @@ -0,0 +1,184 @@ +import 'package:flutter/widgets.dart'; + +import '../design/ab_colors.dart'; +import '../design/ab_icons.dart'; +import '../design/ab_tokens.dart'; +import '../design/widgets/ab_icon.dart'; + +/// The destination readout for the terminal link under the pointer. +/// +/// OSC 8 lets a link's visible text say one thing and its target say another, +/// and the terminal view paints only an underline — so without this the first +/// place the real URI appears is the browser it has already been opened in. +/// This is the desktop half of that disclosure; touch has no hover and gets the +/// confirm sheet instead (`showTerminalHyperlinkSheet`). +/// +/// Shaped as a twin of `TerminalUploadStrip` so the terminal's floating +/// overlays read as one family, and [IgnorePointer] for the same reason: it +/// floats over live terminal text, and the pointer it is describing is by +/// definition somewhere underneath it. +class TerminalHyperlinkPreview extends StatelessWidget { + const TerminalHyperlinkPreview({ + super.key, + required this.uri, + required this.anchor, + }); + + /// The raw OSC 8 payload, exactly as the program wrote it. + /// + /// Not a parsed [Uri]: this reports what the link SAYS, including a payload + /// no launcher would accept, and normalizing it here would show the user a + /// string the terminal does not contain. + final String uri; + + /// Where the pointer was when this URI became the hovered one, in the + /// enclosing stack's coordinates. + final Offset anchor; + + /// Gap between the pointer and the card, in logical pixels. + /// + /// Roughly a line of terminal text — enough that the card clears the row it + /// describes rather than covering the link the user is reading. + static const double _gap = 18; + + /// Distance kept from the panel's edges when the anchor is near one. + static const double _margin = AbTokens.space8; + + /// The card itself, so a test can assert where the delegate put it. + static const Key cardKey = ValueKey('terminal.hyperlink.preview.card'); + + /// Widest the card may grow before its tail is elided. + /// + /// A URI is unbounded program-chosen text, so something has to stop it: the + /// cap is what keeps a long one from spanning the terminal it overlays. The + /// host survives the elision — see [_spans]. + static const double _maxWidth = 420; + + @override + Widget build(BuildContext context) { + final p = context.antgrid; + return IgnorePointer( + child: CustomSingleChildLayout( + delegate: _AnchoredNearPointer(anchor: anchor), + child: Container( + key: cardKey, + constraints: const BoxConstraints(maxWidth: _maxWidth), + padding: const EdgeInsets.symmetric( + horizontal: AbTokens.space10, + vertical: AbTokens.space6, + ), + decoration: BoxDecoration( + color: p.bgElevated, + borderRadius: AbTokens.borderRadius5, + border: Border.all(color: p.accent.withValues(alpha: 0.3)), + ), + child: Row( + mainAxisSize: MainAxisSize.min, + children: [ + AbIcon(AbIcons.link, size: 12, color: p.accent), + const SizedBox(width: AbTokens.space6), + Flexible( + child: Text.rich( + TextSpan(children: _spans(context, uri)), + maxLines: 1, + overflow: TextOverflow.ellipsis, + // Mono: this is a URL, and the whole point is that its exact + // characters can be compared. + style: AbTokens.monoStyle( + fontSize: AbTokens.fontXs, + color: p.textSecondary, + ), + ), + ), + ], + ), + ), + ), + ); + } +} + +/// Splits [uri] so the HOST is the part that reads brightest. +/// +/// The host is the only span that decides where a click actually lands, and it +/// is the span a spoofed URI works hardest to bury: a `github.com@` userinfo +/// prefix puts a familiar name where a glance stops reading, and the real +/// destination after it. The prefix is dim here; the host is not. +/// +/// Falls back to one flat span when the payload does not parse or names no +/// host — there is nothing to emphasize, and a guess about which characters are +/// the host would be exactly the wrong thing to be confident about. +List _spans(BuildContext context, String uri) { + final p = context.antgrid; + final parsed = Uri.tryParse(uri.trim()); + if (parsed == null || parsed.host.isEmpty) { + return [TextSpan(text: uri)]; + } + final host = parsed.host; + // Located in the ORIGINAL text, not rebuilt from the parse: `Uri` lower-cases + // and percent-encodes as it goes, so a reconstruction would show the user a + // string the terminal never printed — on precisely the characters this exists + // to let them compare. + final start = uri.toLowerCase().indexOf(host.toLowerCase()); + if (start < 0) { + return [TextSpan(text: uri)]; + } + final end = start + host.length; + return [ + TextSpan( + text: uri.substring(0, start), + style: TextStyle(color: p.textMuted), + ), + TextSpan( + text: uri.substring(start, end), + style: TextStyle(color: p.textPrimary), + ), + TextSpan(text: uri.substring(end)), + ]; +} + +/// Parks the card just below the pointer, flipping above it near the bottom +/// edge and sliding inward near the sides. +/// +/// Below by default because the terminal's live prompt is at the BOTTOM of the +/// panel: a fixed readout down there — the browser status-bar placement — would +/// sit on the one line the user is typing into. +class _AnchoredNearPointer extends SingleChildLayoutDelegate { + const _AnchoredNearPointer({required this.anchor}); + + final Offset anchor; + + @override + BoxConstraints getConstraintsForChild(BoxConstraints constraints) => + BoxConstraints.loose( + Size( + (constraints.maxWidth - TerminalHyperlinkPreview._margin * 2).clamp( + 0.0, + double.infinity, + ), + constraints.maxHeight, + ), + ); + + @override + Offset getPositionForChild(Size size, Size childSize) { + const margin = TerminalHyperlinkPreview._margin; + const gap = TerminalHyperlinkPreview._gap; + // `clamp` with a collapsed range throws, so the panel being narrower than + // the card has to resolve to the margin rather than to an assertion. + final maxDx = size.width - childSize.width - margin; + final dx = maxDx <= margin + ? margin + : anchor.dx.clamp(margin, maxDx).toDouble(); + var dy = anchor.dy + gap; + if (dy + childSize.height > size.height - margin) { + dy = anchor.dy - gap - childSize.height; + } + final maxDy = size.height - childSize.height - margin; + return Offset(dx, maxDy <= margin ? margin : dy.clamp(margin, maxDy)); + } + + @override + bool shouldRelayout(_AnchoredNearPointer oldDelegate) => + oldDelegate.anchor != anchor; +} diff --git a/app/lib/widgets/terminal_view_wrapper.dart b/app/lib/widgets/terminal_view_wrapper.dart index 52f8d96a..7c782730 100644 --- a/app/lib/widgets/terminal_view_wrapper.dart +++ b/app/lib/widgets/terminal_view_wrapper.dart @@ -26,6 +26,7 @@ import 'send_to_agent_button.dart'; import 'send_to_agent_comment.dart'; import 'terminal_attachment_uploader.dart'; import 'terminal_drop_target.dart'; +import 'terminal_hyperlink_preview.dart'; import 'terminal_quick_actions_bar.dart'; import 'terminal_upload_button.dart'; import 'terminal_upload_strip.dart'; @@ -68,6 +69,28 @@ class _TerminalViewWrapperState extends ConsumerState { /// overlay button only when the user has a non-empty selection. String? _selectedText; + /// The OSC 8 URI under the pointer, from the view's `onHyperlinkHover`, or + /// null when the pointer is over no link. + /// + /// Held here rather than read from the view because the view paints only an + /// underline: the destination itself is disclosed by + /// [TerminalHyperlinkPreview] or by nothing at all. + String? _hoveredLinkUri; + + /// Where the pointer was when [_hoveredLinkUri] last CHANGED. + /// + /// Sampled at the change rather than tracked live so the readout stays put + /// while the pointer travels along one link — a card sliding under the moving + /// cursor is unreadable, and re-laying it out on every hover frame would be + /// work for nothing. + Offset _hoveredLinkAnchor = Offset.zero; + + /// Latest hover position, updated with no rebuild. + /// + /// A `setState` per hover frame is what that avoids; the value matters only + /// at the instant [_hoveredLinkUri] changes. + Offset _lastHoverPosition = Offset.zero; + /// Wraps the terminal subtree so we can detect when the user's primary /// focus is inside this view. Required for the paste interceptor, which /// must scope its effect to the focused terminal — `HardwareKeyboard` @@ -510,6 +533,9 @@ class _TerminalViewWrapperState extends ConsumerState { Widget _buildTerminal(BuildContext context) { final agentTab = ref.watch(agentTerminalProvider); final showSendButton = _hasSelection && agentTab != null; + // Read once so the null check and the use are the same value — the field + // moves from a callback, not from this build. + final hoveredLink = _hoveredLinkUri; // Desktop's only attach route. Mobile already has one in the quick-actions // bar, and a LOCAL session needs none: the agent reads the user's own disk, // so a path typed by hand or dropped by the OS already works. @@ -595,6 +621,10 @@ class _TerminalViewWrapperState extends ConsumerState { // fails. Route through the app's helper so a link opens externally and a // failure is visible, and so terminal-authored URIs are scheme-checked. onOpenHyperlink: (uri) => openTerminalHyperlink(context, uri), + // The other half of that disclosure: `openTerminalHyperlink` asks before + // opening only where the destination would otherwise never be shown at + // all, and this is what shows it everywhere else. + onHyperlinkHover: _onHyperlinkHover, showHeader: false, showFocusRing: false, // Thin terminal-native scrollbar — thumb tracks @@ -652,47 +682,57 @@ class _TerminalViewWrapperState extends ConsumerState { node: _focusScope, child: Stack( children: [ - LayoutBuilder( - builder: (context, constraints) { - _maybeSendResize( - tab.terminalId, - constraints, - amDriver, - tab.driverClientId, - ); - - // Driver (or metrics not yet known) → fill the viewport. Pin - // the grid to the last settled width via `_TerminalGridFreeze` - // so transient resizes (e.g. dragging the agent/workspace - // divider) don't spam Ghostty grid resizes — - // `ghostty_vte_flutter` doesn't reflow soft-wrapped lines, and - // Ink-style TUI redraws (Claude Code) leak stale fragments - // when the grid changes underneath them. - final charWidth = _charWidth; - if (amDriver || charWidth == null || charWidth <= 0) { - return _TerminalGridFreeze( - onSettled: _onRenderSizeSettled, + // Wraps the terminal, not the whole subtree: this is the + // Stack's only non-positioned child, so it shares the Stack's + // origin — which is the space the preview's anchor is read in. + // Translucent so it joins the hit path above the view's own + // MouseRegion without taking anything from it. + Listener( + behavior: HitTestBehavior.translucent, + onPointerHover: (event) => + _lastHoverPosition = event.localPosition, + child: LayoutBuilder( + builder: (context, constraints) { + _maybeSendResize( + tab.terminalId, + constraints, + amDriver, + tab.driverClientId, + ); + + // Driver (or metrics not yet known) → fill the viewport. Pin + // the grid to the last settled width via `_TerminalGridFreeze` + // so transient resizes (e.g. dragging the agent/workspace + // divider) don't spam Ghostty grid resizes — + // `ghostty_vte_flutter` doesn't reflow soft-wrapped lines, and + // Ink-style TUI redraws (Claude Code) leak stale fragments + // when the grid changes underneath them. + final charWidth = _charWidth; + if (amDriver || charWidth == null || charWidth <= 0) { + return _TerminalGridFreeze( + onSettled: _onRenderSizeSettled, + child: terminalView, + ); + } + + // Non-driver → size the grid to the driver's authoritative + // cols so wrapping matches exactly. Letterbox (center) when it + // fits; horizontal-scroll when the driver is wider than this + // viewport. No `_TerminalGridFreeze` here — a viewer must + // track the authoritative width, not pin a local one. + final authWidth = tab.cols * charWidth + _hPad; + final grid = SizedBox( + width: authWidth, child: terminalView, ); - } - - // Non-driver → size the grid to the driver's authoritative - // cols so wrapping matches exactly. Letterbox (center) when it - // fits; horizontal-scroll when the driver is wider than this - // viewport. No `_TerminalGridFreeze` here — a viewer must - // track the authoritative width, not pin a local one. - final authWidth = tab.cols * charWidth + _hPad; - final grid = SizedBox( - width: authWidth, - child: terminalView, - ); - return authWidth <= constraints.maxWidth - ? Align(alignment: Alignment.center, child: grid) - : SingleChildScrollView( - scrollDirection: Axis.horizontal, - child: grid, - ); - }, + return authWidth <= constraints.maxWidth + ? Align(alignment: Alignment.center, child: grid) + : SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: grid, + ); + }, + ), ), // Top-LEFT, deliberately: `SendToAgentButton` owns top-right and // both can be live at once, while the bottom edge is where the @@ -729,6 +769,13 @@ class _TerminalViewWrapperState extends ConsumerState { ), if (showSendButton) SendToAgentButton(onPressed: _onSendToAgent), + if (hoveredLink != null) + Positioned.fill( + child: TerminalHyperlinkPreview( + uri: hoveredLink, + anchor: _hoveredLinkAnchor, + ), + ), ], ), ), @@ -737,6 +784,19 @@ class _TerminalViewWrapperState extends ConsumerState { ); } + /// Shows, moves or hides the destination readout as the pointer enters and + /// leaves links. + /// + /// The view fires this only on a real change, so there is no same-value + /// rebuild to guard against here. + void _onHyperlinkHover(String? uri) { + if (!mounted) return; + setState(() { + _hoveredLinkUri = uri; + if (uri != null) _hoveredLinkAnchor = _lastHoverPosition; + }); + } + /// Sends a `terminal:resize` derived from the local viewport + cell metrics /// when this view just claimed focus, or when it is the driver and its /// native grid changed. The send is the SOLE resize source (the engine's diff --git a/app/pubspec.lock b/app/pubspec.lock index 915a3899..4a9ada28 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -682,8 +682,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/vte/ghostty_vte" - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c - resolved-ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -691,8 +691,8 @@ packages: dependency: "direct main" description: path: "pkgs/vte/ghostty_vte_flutter" - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c - resolved-ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -1204,8 +1204,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/pty/portable_pty" - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c - resolved-ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.0.6+antgrid.2" diff --git a/app/pubspec.yaml b/app/pubspec.yaml index 84515562..b93714ef 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -191,17 +191,17 @@ dependency_overrides: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte_flutter - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d portable_pty: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/pty/portable_pty - ref: f8e2e8201c7bef116ec04baa4cfc039acb53e29c + ref: dc5a33376eb5fe6679d56927f4639d46d91b422d # Microsoft Store packaging (MSIX). The Store signs the package itself, so no # code-signing cert is used on this path (unlike the Inno Setup installer in diff --git a/app/test/terminal_hyperlink_preview_test.dart b/app/test/terminal_hyperlink_preview_test.dart new file mode 100644 index 00000000..c9f97bf5 --- /dev/null +++ b/app/test/terminal_hyperlink_preview_test.dart @@ -0,0 +1,157 @@ +import 'package:antgrid/design/ab_colors.dart'; +import 'package:antgrid/widgets/terminal_hyperlink_preview.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +const Size _panel = Size(600, 400); + +Future _pump(WidgetTester tester, String uri, Offset anchor) { + return tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Center( + child: SizedBox( + width: _panel.width, + height: _panel.height, + child: Stack( + children: [ + Positioned.fill( + child: TerminalHyperlinkPreview(uri: uri, anchor: anchor), + ), + ], + ), + ), + ), + ), + ), + ); +} + +/// The card's rect relative to the panel it floats in. +/// +/// The widget itself is `Positioned.fill`, so its own rect IS the panel. +Rect _card(WidgetTester tester) { + final panel = tester.getTopLeft(find.byType(TerminalHyperlinkPreview)); + return tester + .getRect(find.byKey(TerminalHyperlinkPreview.cardKey)) + .shift(-panel); +} + +/// Every span in the readout, paired with the color it is painted in. +List<(String, Color?)> _spans(WidgetTester tester) { + final rich = tester.widget( + find.descendant( + of: find.byType(TerminalHyperlinkPreview), + matching: find.byType(RichText), + ), + ); + final out = <(String, Color?)>[]; + rich.text.visitChildren((span) { + if (span is TextSpan && (span.text ?? '').isNotEmpty) { + out.add((span.text!, span.style?.color)); + } + return true; + }); + return out; +} + +void main() { + group('TerminalHyperlinkPreview', () { + const link = 'https://github.com/antgrid-ai/antgrid/pull/13'; + + testWidgets('shows the payload verbatim', (tester) async { + await _pump(tester, link, const Offset(100, 100)); + + expect(_spans(tester).map((s) => s.$1).join(), link); + }); + + // A readout that normalized would show the user a string the terminal does + // not contain, on exactly the characters they are here to compare. + testWidgets('does not normalize the case it was handed', (tester) async { + await _pump(tester, 'https://GitHub.COM/OK', const Offset(100, 100)); + + expect(_spans(tester).map((s) => s.$1).join(), 'https://GitHub.COM/OK'); + }); + + // The point of the split: an impostor parked in userinfo reads dim and the + // host that actually resolves reads bright, which is the reverse of how the + // URI is written. + testWidgets('paints the host brightest, userinfo prefix dimmest', ( + tester, + ) async { + late AbColors palette; + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Builder( + builder: (context) { + palette = context.antgrid; + return const SizedBox.shrink(); + }, + ), + ), + ), + ); + await _pump( + tester, + 'https://github.com@evil.example/antgrid/pull/13', + const Offset(100, 100), + ); + + final spans = _spans(tester); + expect(spans.first, ('https://github.com@', palette.textMuted)); + expect(spans[1], ('evil.example', palette.textPrimary)); + expect(spans.last.$1, '/antgrid/pull/13'); + }); + + // Nothing to emphasize, and a guess about which characters are the host is + // exactly the wrong thing to be confident about. + testWidgets('renders an unparseable payload flat', (tester) async { + await _pump(tester, 'not a url at all', const Offset(100, 100)); + + final spans = _spans(tester); + expect(spans.length, 1); + expect(spans.single.$1, 'not a url at all'); + }); + + testWidgets('parks below the pointer, left edge on it', (tester) async { + await _pump(tester, link, const Offset(100, 100)); + + final card = _card(tester); + expect(card.left, 100); + expect(card.top, greaterThan(100)); + }); + + // The prompt the user is typing into is the bottom line of the panel, so + // the card has to leave it alone rather than settle over it. + testWidgets('flips above the pointer near the bottom edge', (tester) async { + await _pump(tester, link, Offset(100, _panel.height - 4)); + + final card = _card(tester); + expect(card.bottom, lessThan(_panel.height - 4)); + expect(card.top, greaterThanOrEqualTo(0)); + }); + + testWidgets('slides inward rather than off the right edge', (tester) async { + await _pump(tester, link, Offset(_panel.width - 4, 100)); + + final card = _card(tester); + expect(card.right, lessThanOrEqualTo(_panel.width)); + expect(card.left, lessThan(_panel.width - 4)); + }); + + // A URI is unbounded program-chosen text; without the cap one long enough + // would span the terminal it overlays. + testWidgets('caps its width on a very long URI', (tester) async { + await _pump( + tester, + 'https://example.com/${'segment/' * 200}', + const Offset(100, 100), + ); + + final card = _card(tester); + expect(card.width, lessThan(_panel.width * 0.75)); + expect(card.right, lessThanOrEqualTo(_panel.width)); + }); + }); +} From 91b3de50732a502b71c724564362e4e9658ee647 Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:31:29 +0530 Subject: [PATCH 3/6] fix: make the hover readout expose the shapes it exists to expose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/lib/util/external_url.dart | 46 ++- .../widgets/terminal_hyperlink_preview.dart | 324 +++++++++++++----- app/lib/widgets/terminal_hyperlink_sheet.dart | 5 +- app/lib/widgets/terminal_upload_strip.dart | 2 +- app/lib/widgets/terminal_view_wrapper.dart | 116 +++++-- app/test/terminal_hyperlink_preview_test.dart | 130 +++++-- app/test/terminal_hyperlink_test.dart | 32 +- 7 files changed, 471 insertions(+), 184 deletions(-) diff --git a/app/lib/util/external_url.dart b/app/lib/util/external_url.dart index c494c84e..e1f1cd2f 100644 --- a/app/lib/util/external_url.dart +++ b/app/lib/util/external_url.dart @@ -79,8 +79,9 @@ Uri? openableTerminalHyperlink(String uri) { /// returns, so a rejection would reach `PlatformDispatcher.onError` as a fatal /// carrying no in-app frames. /// -/// On touch the destination is confirmed first — see -/// [showTerminalHyperlinkSheet] for why that is not merely a nag. +/// The destination is confirmed first on touch, and on any platform when the +/// URI is shaped to be misread — see [showTerminalHyperlinkSheet] for why that +/// is not merely a nag, and [terminalHyperlinkLooksDeceptive] for the shapes. /// /// [open] and [confirm] are injectable so tests can assert what would be /// launched instead of handing a URL to the real browser, matching @@ -105,8 +106,7 @@ Future openTerminalHyperlink( } return; } - if (_browserRevealsDestination && - !terminalHyperlinkLooksDeceptive(target)) { + if (_shownBeforeActivated && !terminalHyperlinkLooksDeceptive(target)) { await open(context, target.toString()); return; } @@ -164,22 +164,32 @@ bool terminalHyperlinkLooksDeceptive(Uri target) { return host.split('.').any((label) => label.toLowerCase().startsWith('xn--')); } -/// Whether opening the link lands somewhere that reads the destination back. +/// Whether this platform showed the destination before the link was activated. /// -/// Desktop hands the URI to a browser whose address bar does, which is the same -/// disclosure every terminal leans on. The terminal's own hover affordance is -/// NOT it and never was: the view only swaps the cursor to a pointer, and -/// nothing there or here paints the URI. +/// Desktop has a pointer, so `TerminalHyperlinkPreview` has already painted the +/// URI under it by the time a click lands — that readout is the disclosure, and +/// it is the only reason this path may skip the sheet. Deliberately NOT the +/// browser's address bar: `LaunchMode.externalApplication` resolves through +/// `ShellExecuteW` on Windows, `NSWorkspace.open` on macOS and +/// `g_app_info_launch_default_for_uri` on Linux, every one of which honours a +/// registered `https:` handler — so a desktop click can land in an app with no +/// address bar just as a mobile one can (see [openableTerminalHyperlink]). /// -/// Mobile has no such guarantee. An `https:` host holding a verified App Link -/// opens that app instead of any browser (see [openableTerminalHyperlink]), so -/// the destination can be acted on without ever being shown. That is why the -/// mobile path asks first, and why the answer to a spoofed target on desktop is -/// a hover preview rather than the same sheet on both. +/// Touch has no hover and therefore no readout, which is why it asks every time. /// -/// An address bar only discloses what the reader then has to judge, so this is -/// not the whole desktop rule: [terminalHyperlinkLooksDeceptive] pulls the -/// cases back to the sheet where the URI is built to be misread. -bool get _browserRevealsDestination => +/// A BARE URL — one the view matched by regex rather than by OSC 8 markup — is +/// not a gap here even though the readout may not have marked it: its visible +/// text IS the URI, which is how it was found, so there is nothing the target +/// can disagree with. +/// +/// Known gap, reported rather than papered over: this asks the OS, not the +/// input device, so a finger tap on a desktop touchscreen takes the desktop +/// branch having been shown nothing. The pointer kind is the right predicate +/// and lives in the terminal package, not here. +/// +/// A readout only discloses what the reader then has to judge, so this is not +/// the whole desktop rule: [terminalHyperlinkLooksDeceptive] pulls the cases +/// back to the sheet where the URI is built to be misread. +bool get _shownBeforeActivated => defaultTargetPlatform != TargetPlatform.android && defaultTargetPlatform != TargetPlatform.iOS; diff --git a/app/lib/widgets/terminal_hyperlink_preview.dart b/app/lib/widgets/terminal_hyperlink_preview.dart index 51352e6f..97e0554a 100644 --- a/app/lib/widgets/terminal_hyperlink_preview.dart +++ b/app/lib/widgets/terminal_hyperlink_preview.dart @@ -1,3 +1,6 @@ +import 'dart:convert'; +import 'dart:math' as math; + import 'package:flutter/widgets.dart'; import '../design/ab_colors.dart'; @@ -5,6 +8,99 @@ import '../design/ab_icons.dart'; import '../design/ab_tokens.dart'; import '../design/widgets/ab_icon.dart'; +/// Gap between the pointer and the card, in logical pixels. +/// +/// Roughly a line of terminal text — enough that the card clears the row it +/// describes rather than covering the link the user is reading. +const double _gap = 18; + +/// Distance kept from the panel's edges when the anchor is near one. +const double _margin = AbTokens.space8; + +/// Longest run of prefix, host or trailing text laid out. +/// +/// `maxLines: 1` and [TextOverflow.ellipsis] bound what is PAINTED, not what is +/// measured — the engine shapes the whole run before eliding it. A payload is +/// unbounded program-chosen text, so without this a single hover shapes all of +/// it on the UI thread. Same trap `_elide` guards in `external_url.dart`, which +/// documents it for the same data. +const int _maxPartChars = 128; + +/// Characters that can reorder or hide the rest of the readout. +/// +/// Bidi overrides and isolates re-order glyphs ACROSS span boundaries, so a +/// payload carrying U+202E paints a reading order that is not the URI's — on +/// the one surface whose whole job is letting exact characters be compared. +/// [terminalHyperlinkLooksDeceptive] cannot backstop it: the host itself is +/// clean, so such a link takes the desktop no-sheet path. +bool _isDisplayUnsafe(int rune) => + rune < 0x20 || + rune == 0x7F || + rune == 0x061C || + (rune >= 0x200E && rune <= 0x200F) || + (rune >= 0x202A && rune <= 0x202E) || + (rune >= 0x2066 && rune <= 0x2069); + +/// Renders [part] with the invisible characters spelled out. +/// +/// Percent-encoded rather than dropped, because that is what `Uri.toString()` +/// hands the launcher — so the card and the thing it describes agree on these +/// characters instead of diverging on exactly them. +String _display(String part) { + if (!part.runes.any(_isDisplayUnsafe)) return part; + final out = StringBuffer(); + for (final rune in part.runes) { + if (!_isDisplayUnsafe(rune)) { + out.writeCharCode(rune); + continue; + } + for (final byte in utf8.encode(String.fromCharCode(rune))) { + out.write('%${byte.toRadixString(16).toUpperCase().padLeft(2, '0')}'); + } + } + return out.toString(); +} + +/// Replaces the password half of a userinfo with a fixed mask. +/// +/// A hover is not consent to display a secret: `https://x:TOKEN@host/` is a +/// shape real tooling prints, and before this readout existed the view painted +/// only an underline. The NAME half survives — it is what an impostor prefix +/// uses to read as a familiar host, which is the thing this card exists to +/// expose. +String _maskPassword(String prefix) { + final at = prefix.lastIndexOf('@'); + if (at < 0) return prefix; + final schemeEnd = prefix.indexOf('://'); + if (schemeEnd < 0) return prefix; + final colon = prefix.indexOf(':', schemeEnd + 3); + if (colon < 0 || colon > at) return prefix; + return '${prefix.substring(0, colon + 1)}•••${prefix.substring(at)}'; +} + +/// Caps [part] to [_maxPartChars], keeping the end when [keepTail] is set. +/// +/// The prefix keeps its TAIL: what matters there is the `@` and the characters +/// immediately before the host, not the start of a padded userinfo. +String _cap(String part, {bool keepTail = false}) { + if (part.length <= _maxPartChars) return part; + // Back off a stranded surrogate half: `substring` cuts UTF-16 code units, and + // half a pair renders as a replacement glyph on exactly the characters a + // reader is trying to identify. + if (keepTail) { + var start = part.length - _maxPartChars; + if (_isLowSurrogate(part.codeUnitAt(start))) start += 1; + return '…${part.substring(start)}'; + } + var end = _maxPartChars; + if (_isHighSurrogate(part.codeUnitAt(end - 1))) end -= 1; + return '${part.substring(0, end)}…'; +} + +bool _isHighSurrogate(int unit) => unit >= 0xD800 && unit <= 0xDBFF; + +bool _isLowSurrogate(int unit) => unit >= 0xDC00 && unit <= 0xDFFF; + /// The destination readout for the terminal link under the pointer. /// /// OSC 8 lets a link's visible text say one thing and its target say another, @@ -24,36 +120,34 @@ class TerminalHyperlinkPreview extends StatelessWidget { required this.anchor, }); - /// The raw OSC 8 payload, exactly as the program wrote it. + /// The raw OSC 8 payload, as the program wrote it apart from a masked + /// password and spelled-out control characters. /// /// Not a parsed [Uri]: this reports what the link SAYS, including a payload - /// no launcher would accept, and normalizing it here would show the user a - /// string the terminal does not contain. + /// no launcher would accept, and normalizing it would show the user a string + /// the terminal does not contain. final String uri; /// Where the pointer was when this URI became the hovered one, in the /// enclosing stack's coordinates. final Offset anchor; - /// Gap between the pointer and the card, in logical pixels. - /// - /// Roughly a line of terminal text — enough that the card clears the row it - /// describes rather than covering the link the user is reading. - static const double _gap = 18; - - /// Distance kept from the panel's edges when the anchor is near one. - static const double _margin = AbTokens.space8; - /// The card itself, so a test can assert where the delegate put it. static const Key cardKey = ValueKey('terminal.hyperlink.preview.card'); - /// Widest the card may grow before its tail is elided. + /// Widest the card may grow before its prefix and tail are elided. /// /// A URI is unbounded program-chosen text, so something has to stop it: the - /// cap is what keeps a long one from spanning the terminal it overlays. The - /// host survives the elision — see [_spans]. + /// cap is what keeps a long one from spanning the terminal it overlays. static const double _maxWidth = 420; + /// Widest the HOST span may grow. + /// + /// Held well under [_maxWidth] so the host is laid out before the elidable + /// spans get what is left — see [_HyperlinkText] for why it must never be the + /// part that gets cut. + static const double _maxHostWidth = 280; + @override Widget build(BuildContext context) { final p = context.antgrid; @@ -77,19 +171,7 @@ class TerminalHyperlinkPreview extends StatelessWidget { children: [ AbIcon(AbIcons.link, size: 12, color: p.accent), const SizedBox(width: AbTokens.space6), - Flexible( - child: Text.rich( - TextSpan(children: _spans(context, uri)), - maxLines: 1, - overflow: TextOverflow.ellipsis, - // Mono: this is a URL, and the whole point is that its exact - // characters can be compared. - style: AbTokens.monoStyle( - fontSize: AbTokens.fontXs, - color: p.textSecondary, - ), - ), - ), + Flexible(child: _HyperlinkText(uri: uri)), ], ), ), @@ -98,43 +180,128 @@ class TerminalHyperlinkPreview extends StatelessWidget { } } -/// Splits [uri] so the HOST is the part that reads brightest. +/// The URI itself, split so the HOST is both the part that reads brightest and +/// the part that survives when the card runs out of room. /// /// The host is the only span that decides where a click actually lands, and it /// is the span a spoofed URI works hardest to bury: a `github.com@` userinfo /// prefix puts a familiar name where a glance stops reading, and the real -/// destination after it. The prefix is dim here; the host is not. +/// destination after it. /// -/// Falls back to one flat span when the payload does not parse or names no -/// host — there is nothing to emphasize, and a guess about which characters are -/// the host would be exactly the wrong thing to be confident about. -List _spans(BuildContext context, String uri) { - final p = context.antgrid; - final parsed = Uri.tryParse(uri.trim()); - if (parsed == null || parsed.host.isEmpty) { - return [TextSpan(text: uri)]; +/// Three separate [Text]s rather than one [Text.rich], because a single rich +/// run elides its TAIL — which behind a padded userinfo +/// (`https://github.com.login.oauth.…@evil.example/`) is the host itself, so +/// the readout would render as a clean GitHub URL and confirm the lie. Here the +/// host is the only non-flex child, so the elidable spans surrender their width +/// first and the host is cut last. +class _HyperlinkText extends StatelessWidget { + const _HyperlinkText({required this.uri}); + + final String uri; + + @override + Widget build(BuildContext context) { + final p = context.antgrid; + // Mono: this is a URL, and the whole point is that its exact characters can + // be compared. + final base = AbTokens.monoStyle( + fontSize: AbTokens.fontXs, + color: p.textSecondary, + ); + final range = _hostRange(uri); + // Nothing to emphasize, and a guess about which characters are the host + // would be exactly the wrong thing to be confident about. + if (range == null) { + return Text( + _display(_cap(uri)), + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: base, + ); + } + final (start, end) = range; + return Row( + mainAxisSize: MainAxisSize.min, + children: [ + Flexible( + child: Text( + _display( + _cap(_maskPassword(uri.substring(0, start)), keepTail: true), + ), + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: base.copyWith(color: p.textMuted), + ), + ), + ConstrainedBox( + constraints: const BoxConstraints( + maxWidth: TerminalHyperlinkPreview._maxHostWidth, + ), + child: Text( + _display(_cap(uri.substring(start, end))), + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: base.copyWith(color: p.textPrimary), + ), + ), + Flexible( + child: Text( + _display(_cap(uri.substring(end))), + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: base, + ), + ), + ], + ); + } +} + +/// The `[start, end)` span of the authority's host inside [uri], or null when +/// the payload names no host. +/// +/// Located POSITIONALLY in the original text, never by searching it for the +/// parsed host. Two reasons, and each is the difference between disclosing the +/// lie and repeating it: +/// +/// * `indexOf(host)` finds the FIRST occurrence, which an impostor puts in the +/// userinfo — `https://github.com.evil.tld@evil.tld/` would paint the +/// userinfo copy bright and leave the real authority reading as body text. +/// * `Uri.host` percent-encodes a raw unicode host, so it does not occur in +/// the original text at all and the search silently finds nothing, dropping +/// the emphasis on precisely the homoglyph it exists to expose. +/// +/// Rebuilding the string from the parse is not an option either: `Uri` +/// lower-cases and percent-encodes as it goes, and this exists to let the user +/// compare the characters the terminal actually printed. +(int, int)? _hostRange(String uri) { + final schemeEnd = uri.indexOf('://'); + if (schemeEnd < 0) return null; + final authorityStart = schemeEnd + 3; + var authorityEnd = uri.length; + for (var i = authorityStart; i < uri.length; i++) { + final c = uri[i]; + if (c == '/' || c == '?' || c == '#') { + authorityEnd = i; + break; + } } - final host = parsed.host; - // Located in the ORIGINAL text, not rebuilt from the parse: `Uri` lower-cases - // and percent-encodes as it goes, so a reconstruction would show the user a - // string the terminal never printed — on precisely the characters this exists - // to let them compare. - final start = uri.toLowerCase().indexOf(host.toLowerCase()); - if (start < 0) { - return [TextSpan(text: uri)]; + // LAST `@`: a userinfo may contain one, and the host is what follows the + // final separator — the same rule the parser applies. + final at = uri.substring(authorityStart, authorityEnd).lastIndexOf('@'); + final start = authorityStart + (at < 0 ? 0 : at + 1); + var end = authorityEnd; + final rest = uri.substring(start, authorityEnd); + if (rest.startsWith('[')) { + // IPv6 literal: its colons belong to the host, and only one after `]` + // starts a port. + final close = rest.indexOf(']'); + if (close >= 0) end = start + close + 1; + } else { + final colon = rest.indexOf(':'); + if (colon >= 0) end = start + colon; } - final end = start + host.length; - return [ - TextSpan( - text: uri.substring(0, start), - style: TextStyle(color: p.textMuted), - ), - TextSpan( - text: uri.substring(start, end), - style: TextStyle(color: p.textPrimary), - ), - TextSpan(text: uri.substring(end)), - ]; + return end > start ? (start, end) : null; } /// Parks the card just below the pointer, flipping above it near the bottom @@ -150,32 +317,31 @@ class _AnchoredNearPointer extends SingleChildLayoutDelegate { @override BoxConstraints getConstraintsForChild(BoxConstraints constraints) => - BoxConstraints.loose( - Size( - (constraints.maxWidth - TerminalHyperlinkPreview._margin * 2).clamp( - 0.0, - double.infinity, - ), - constraints.maxHeight, - ), + constraints.loosen().copyWith( + maxWidth: math.max(0, constraints.maxWidth - _margin * 2), ); @override Offset getPositionForChild(Size size, Size childSize) { - const margin = TerminalHyperlinkPreview._margin; - const gap = TerminalHyperlinkPreview._gap; - // `clamp` with a collapsed range throws, so the panel being narrower than - // the card has to resolve to the margin rather than to an assertion. - final maxDx = size.width - childSize.width - margin; - final dx = maxDx <= margin - ? margin - : anchor.dx.clamp(margin, maxDx).toDouble(); - var dy = anchor.dy + gap; - if (dy + childSize.height > size.height - margin) { - dy = anchor.dy - gap - childSize.height; + // `math.max` on the upper bound rather than a branch: `clamp` throws on an + // inverted range, so a panel narrower than the card has to resolve to the + // margin instead of to an assertion. Hoisted into `double` locals because + // `clamp` takes `num`: an inline `math.max` infers that from the parameter + // and hands back a `num` no `Offset` will take. + final double maxDx = math.max( + _margin, + size.width - childSize.width - _margin, + ); + final double maxDy = math.max( + _margin, + size.height - childSize.height - _margin, + ); + final dx = anchor.dx.clamp(_margin, maxDx); + var dy = anchor.dy + _gap; + if (dy + childSize.height > size.height - _margin) { + dy = anchor.dy - _gap - childSize.height; } - final maxDy = size.height - childSize.height - margin; - return Offset(dx, maxDy <= margin ? margin : dy.clamp(margin, maxDy)); + return Offset(dx, dy.clamp(_margin, maxDy)); } @override diff --git a/app/lib/widgets/terminal_hyperlink_sheet.dart b/app/lib/widgets/terminal_hyperlink_sheet.dart index ebe75339..1412cc54 100644 --- a/app/lib/widgets/terminal_hyperlink_sheet.dart +++ b/app/lib/widgets/terminal_hyperlink_sheet.dart @@ -11,8 +11,9 @@ import '../design/widgets/ab_dialog.dart'; /// OSC 8 lets a link's visible text disagree with its destination, so the text /// a user taps is not evidence of anything: `https://github.com@evil.example/` /// reads as GitHub and resolves to `evil.example`. Desktop gets its disclosure -/// from the browser's address bar; a tap here may never reach a browser at all, -/// because a verified App Link opens its own app instead. +/// from the hover readout instead (`TerminalHyperlinkPreview`); touch has no +/// hover, and a tap there may never reach a browser at all, because a verified +/// App Link opens its own app. /// /// Returns false when dismissed, so a stray tap outside the sheet cancels. Future showTerminalHyperlinkSheet( diff --git a/app/lib/widgets/terminal_upload_strip.dart b/app/lib/widgets/terminal_upload_strip.dart index 5d220668..39adcff0 100644 --- a/app/lib/widgets/terminal_upload_strip.dart +++ b/app/lib/widgets/terminal_upload_strip.dart @@ -8,7 +8,7 @@ import '../design/widgets/ab_progress_rule.dart'; import 'terminal_attachment_uploader.dart'; /// The in-flight attach readout, shaped as a twin of `SendToAgentButton` so the -/// terminal's two floating overlays read as one family. +/// terminal's floating overlays read as one family. /// /// [IgnorePointer] by construction: it floats over live terminal text, and a /// tap target there would compete with drag-selection for the same pixels. diff --git a/app/lib/widgets/terminal_view_wrapper.dart b/app/lib/widgets/terminal_view_wrapper.dart index 7c782730..20dbc999 100644 --- a/app/lib/widgets/terminal_view_wrapper.dart +++ b/app/lib/widgets/terminal_view_wrapper.dart @@ -69,27 +69,30 @@ class _TerminalViewWrapperState extends ConsumerState { /// overlay button only when the user has a non-empty selection. String? _selectedText; - /// The OSC 8 URI under the pointer, from the view's `onHyperlinkHover`, or - /// null when the pointer is over no link. + /// The OSC 8 link under the pointer and where to park its readout, or null + /// when the pointer is over no link. /// /// Held here rather than read from the view because the view paints only an /// underline: the destination itself is disclosed by - /// [TerminalHyperlinkPreview] or by nothing at all. - String? _hoveredLinkUri; - - /// Where the pointer was when [_hoveredLinkUri] last CHANGED. - /// - /// Sampled at the change rather than tracked live so the readout stays put - /// while the pointer travels along one link — a card sliding under the moving - /// cursor is unreadable, and re-laying it out on every hover frame would be - /// work for nothing. - Offset _hoveredLinkAnchor = Offset.zero; - - /// Latest hover position, updated with no rebuild. + /// [TerminalHyperlinkPreview] or by nothing at all. A notifier rather than + /// `setState` because this is the terminal's primary surface and a hover + /// crossing a wall of links would otherwise relayout the whole panel — three + /// nested `LayoutBuilder`s and a fresh `GhosttyTerminalView` — per link, to + /// toggle one floating card. Same shape the sibling upload strip already uses. + final ValueNotifier<({String uri, Offset at})?> _hoveredLink = ValueNotifier( + null, + ); + + /// A hovered URI still waiting for the pointer position that produced it. /// - /// A `setState` per hover frame is what that avoids; the value matters only - /// at the instant [_hoveredLinkUri] changes. - Offset _lastHoverPosition = Offset.zero; + /// The view reports the URI from a `MouseRegion` BELOW this widget's own + /// hover handler, and Flutter walks a hit-test path child-first — so at the + /// instant the URI arrives, the position for that same event has not reached + /// us yet, and on the first hover into the panel there is no earlier position + /// to fall back on. Parking the URI here until the enclosing handler supplies + /// the matching position is what keeps the card on the link it describes + /// rather than one hover behind it. + String? _pendingHoverUri; /// Wraps the terminal subtree so we can detect when the user's primary /// focus is inside this view. Required for the paste interceptor, which @@ -227,6 +230,7 @@ class _TerminalViewWrapperState extends ConsumerState { _focusScope.removeListener(_onFocusChange); _focusScope.dispose(); _uploader.dispose(); + _hoveredLink.dispose(); super.dispose(); } @@ -533,9 +537,6 @@ class _TerminalViewWrapperState extends ConsumerState { Widget _buildTerminal(BuildContext context) { final agentTab = ref.watch(agentTerminalProvider); final showSendButton = _hasSelection && agentTab != null; - // Read once so the null check and the use are the same value — the field - // moves from a callback, not from this build. - final hoveredLink = _hoveredLinkUri; // Desktop's only attach route. Mobile already has one in the quick-actions // bar, and a LOCAL session needs none: the agent reads the user's own disk, // so a path typed by hand or dropped by the OS already works. @@ -685,12 +686,21 @@ class _TerminalViewWrapperState extends ConsumerState { // Wraps the terminal, not the whole subtree: this is the // Stack's only non-positioned child, so it shares the Stack's // origin — which is the space the preview's anchor is read in. - // Translucent so it joins the hit path above the view's own + // Non-opaque so it joins the hit path above the view's own // MouseRegion without taking anything from it. - Listener( - behavior: HitTestBehavior.translucent, - onPointerHover: (event) => - _lastHoverPosition = event.localPosition, + MouseRegion( + opaque: false, + onHover: _onHoverPosition, + // The view's own exit report cannot be relied on alone: a + // MouseRegion unmounted while hovered never fires onExit, and + // the branches below swap widget types under the pointer + // (grid-freeze vs letterbox vs h-scroll), so a card can + // outlive the terminal that reported it. This is the one + // handler that survives those swaps. + onExit: (_) { + _pendingHoverUri = null; + _hoveredLink.value = null; + }, child: LayoutBuilder( builder: (context, constraints) { _maybeSendResize( @@ -769,13 +779,17 @@ class _TerminalViewWrapperState extends ConsumerState { ), if (showSendButton) SendToAgentButton(onPressed: _onSendToAgent), - if (hoveredLink != null) - Positioned.fill( - child: TerminalHyperlinkPreview( - uri: hoveredLink, - anchor: _hoveredLinkAnchor, - ), + Positioned.fill( + child: ValueListenableBuilder<({String uri, Offset at})?>( + valueListenable: _hoveredLink, + builder: (context, link, _) => link == null + ? const SizedBox.shrink() + : TerminalHyperlinkPreview( + uri: link.uri, + anchor: link.at, + ), ), + ), ], ), ), @@ -784,17 +798,43 @@ class _TerminalViewWrapperState extends ConsumerState { ); } - /// Shows, moves or hides the destination readout as the pointer enters and - /// leaves links. + /// Shows or hides the destination readout as the pointer enters and leaves + /// links. /// /// The view fires this only on a real change, so there is no same-value - /// rebuild to guard against here. + /// rebuild to guard against here. A new link is only PENDING until + /// [_onHoverPosition] supplies the position it was hovered at — see + /// [_pendingHoverUri] for why the position cannot be read here. void _onHyperlinkHover(String? uri) { if (!mounted) return; - setState(() { - _hoveredLinkUri = uri; - if (uri != null) _hoveredLinkAnchor = _lastHoverPosition; - }); + if (uri == null) { + _pendingHoverUri = null; + _hoveredLink.value = null; + return; + } + _pendingHoverUri = uri; + // Output can scroll a DIFFERENT link under a STATIONARY pointer, and no + // hover event follows to carry it — so repoint the card at once against + // the anchor it already has, rather than let it go on naming a + // destination the click would not open. The pending URI still re-anchors + // it on the next real move. + final shown = _hoveredLink.value; + if (shown != null && shown.uri != uri) { + _hoveredLink.value = (uri: uri, at: shown.at); + } + } + + /// Anchors a pending link to the pointer position of the event that produced + /// it. + /// + /// Sampled once, at the change, rather than tracked live: a card sliding + /// under the cursor it belongs to is unreadable, and re-laying it out every + /// hover frame would be work for nothing. + void _onHoverPosition(PointerHoverEvent event) { + final pending = _pendingHoverUri; + if (pending == null) return; + _pendingHoverUri = null; + _hoveredLink.value = (uri: pending, at: event.localPosition); } /// Sends a `terminal:resize` derived from the local viewport + cell metrics diff --git a/app/test/terminal_hyperlink_preview_test.dart b/app/test/terminal_hyperlink_preview_test.dart index c9f97bf5..c0d33859 100644 --- a/app/test/terminal_hyperlink_preview_test.dart +++ b/app/test/terminal_hyperlink_preview_test.dart @@ -1,6 +1,7 @@ -import 'package:antgrid/design/ab_colors.dart'; +import 'package:antgrid/design/theme_presets.dart'; import 'package:antgrid/widgets/terminal_hyperlink_preview.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; import 'package:flutter_test/flutter_test.dart'; const Size _panel = Size(600, 400); @@ -8,6 +9,13 @@ const Size _panel = Size(600, 400); Future _pump(WidgetTester tester, String uri, Offset anchor) { return tester.pumpWidget( MaterialApp( + // The shipped palette, not `context.antgrid`'s tests-only fallback: + // without the extension both the widget and the assertions resolve to + // `_zincFallback`, so a colour assertion compares the fallback against + // itself and stays green however the emphasis is wired. + theme: ThemeData.dark().copyWith( + extensions: >[kDefaultPalette], + ), home: Scaffold( body: Center( child: SizedBox( @@ -37,24 +45,31 @@ Rect _card(WidgetTester tester) { .shift(-panel); } -/// Every span in the readout, paired with the color it is painted in. +/// Every painted run in the readout, paired with the colour it is drawn in. +/// +/// The URI is three sibling [Text]s, not one rich run — see the widget for why +/// the host has to be laid out on its own. List<(String, Color?)> _spans(WidgetTester tester) { - final rich = tester.widget( + final riches = tester.widgetList( find.descendant( of: find.byType(TerminalHyperlinkPreview), matching: find.byType(RichText), ), ); final out = <(String, Color?)>[]; - rich.text.visitChildren((span) { - if (span is TextSpan && (span.text ?? '').isNotEmpty) { - out.add((span.text!, span.style?.color)); - } - return true; - }); + for (final rich in riches) { + rich.text.visitChildren((span) { + if (span is TextSpan && (span.text ?? '').isNotEmpty) { + out.add((span.text!, span.style?.color)); + } + return true; + }); + } return out; } +String _text(WidgetTester tester) => _spans(tester).map((s) => s.$1).join(); + void main() { group('TerminalHyperlinkPreview', () { const link = 'https://github.com/antgrid-ai/antgrid/pull/13'; @@ -62,7 +77,7 @@ void main() { testWidgets('shows the payload verbatim', (tester) async { await _pump(tester, link, const Offset(100, 100)); - expect(_spans(tester).map((s) => s.$1).join(), link); + expect(_text(tester), link); }); // A readout that normalized would show the user a string the terminal does @@ -70,7 +85,7 @@ void main() { testWidgets('does not normalize the case it was handed', (tester) async { await _pump(tester, 'https://GitHub.COM/OK', const Offset(100, 100)); - expect(_spans(tester).map((s) => s.$1).join(), 'https://GitHub.COM/OK'); + expect(_text(tester), 'https://GitHub.COM/OK'); }); // The point of the split: an impostor parked in userinfo reads dim and the @@ -79,19 +94,6 @@ void main() { testWidgets('paints the host brightest, userinfo prefix dimmest', ( tester, ) async { - late AbColors palette; - await tester.pumpWidget( - MaterialApp( - home: Scaffold( - body: Builder( - builder: (context) { - palette = context.antgrid; - return const SizedBox.shrink(); - }, - ), - ), - ), - ); await _pump( tester, 'https://github.com@evil.example/antgrid/pull/13', @@ -99,11 +101,87 @@ void main() { ); final spans = _spans(tester); - expect(spans.first, ('https://github.com@', palette.textMuted)); - expect(spans[1], ('evil.example', palette.textPrimary)); + expect(spans.first, ('https://github.com@', kDefaultPalette.textMuted)); + expect(spans[1], ('evil.example', kDefaultPalette.textPrimary)); expect(spans.last.$1, '/antgrid/pull/13'); }); + // Searching the raw text for the parsed host finds the FIRST occurrence, + // which an attacker puts in the userinfo -- lighting up the impostor and + // leaving the real destination reading as body text. + testWidgets('a userinfo wearing the host does not steal the highlight', ( + tester, + ) async { + await _pump(tester, 'https://b.com.evil@b.com/x', const Offset(100, 100)); + + final spans = _spans(tester); + expect(spans.first, ('https://b.com.evil@', kDefaultPalette.textMuted)); + expect(spans[1], ('b.com', kDefaultPalette.textPrimary)); + expect(spans.last.$1, '/x'); + }); + + // `Uri.host` percent-encodes a raw unicode host, so it does not occur in + // the payload at all -- a search for it finds nothing and silently drops + // the emphasis on the one shape it exists to expose. + testWidgets('emphasizes a raw unicode host too', (tester) async { + await _pump(tester, 'https://\u0430pple.com/login', const Offset(10, 10)); + + final spans = _spans(tester); + expect(spans[1], ('\u0430pple.com', kDefaultPalette.textPrimary)); + }); + + // A single rich run elides its TAIL, and behind a padded userinfo the tail + // IS the host -- so the card would render as a clean GitHub URL and confirm + // the lie it exists to expose. + testWidgets('keeps the host when a padded userinfo overflows the card', ( + tester, + ) async { + const host = 'evil.example'; + await _pump( + tester, + 'https://github.com.${'padding.' * 40}@$host/pull/13', + const Offset(10, 10), + ); + + final paragraph = tester.renderObject(find.text(host)); + expect(paragraph.didExceedMaxLines, isFalse); + expect(paragraph.size.width, greaterThan(0)); + // The elidable prefix is what gave up its width instead. + expect(_card(tester).width, lessThanOrEqualTo(_panel.width)); + }); + + // Bidi overrides re-order glyphs across span boundaries, so a payload + // carrying one paints a reading order that is not the URI's -- and the + // deception check cannot see it, because the host itself is clean. + testWidgets('spells out a bidi control rather than obeying it', ( + tester, + ) async { + await _pump( + tester, + 'https://evil.example/\u202Egro.dirgtna', + const Offset(10, 10), + ); + + final text = _text(tester); + expect(text, contains('%E2%80%AE')); + expect(text, isNot(contains('\u202E'))); + }); + + // A hover is not consent to display a secret, and before this readout + // existed the view painted only an underline. + testWidgets('masks the password half of a userinfo', (tester) async { + await _pump( + tester, + 'https://oauth2:ghp_LIVE_TOKEN@github.com/org/repo', + const Offset(10, 10), + ); + + final text = _text(tester); + expect(text, isNot(contains('ghp_LIVE_TOKEN'))); + expect(text, startsWith('https://oauth2:')); + expect(_spans(tester)[1].$1, 'github.com'); + }); + // Nothing to emphasize, and a guess about which characters are the host is // exactly the wrong thing to be confident about. testWidgets('renders an unparseable payload flat', (tester) async { diff --git a/app/test/terminal_hyperlink_test.dart b/app/test/terminal_hyperlink_test.dart index 7e7faaee..378334a6 100644 --- a/app/test/terminal_hyperlink_test.dart +++ b/app/test/terminal_hyperlink_test.dart @@ -4,7 +4,6 @@ import 'dart:io'; import 'package:antgrid/util/ab_log.dart'; import 'package:antgrid/util/external_url.dart'; import 'package:antgrid/widgets/terminal_hyperlink_sheet.dart'; -import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -284,13 +283,15 @@ void main() { expect(asked?.host, 'evil.example'); }); + // TargetPlatformVariant, not a hand-placed + // `debugDefaultTargetPlatformOverride`: the variant sets and restores at + // the binding's own lifecycle points, so a throw between the override and + // its reset can no longer leak macOS into the touch-path tests above. + final desktop = TargetPlatformVariant.only(TargetPlatform.macOS); + testWidgets('desktop opens an ordinary link without asking', ( tester, ) async { - // Cleared inside the body, not in addTearDown: the framework asserts - // the foundation debug vars are unset before teardown runs. - debugDefaultTargetPlatformOverride = TargetPlatform.macOS; - final context = await pumpHost(tester); final launched = []; var asked = 0; @@ -306,17 +307,13 @@ void main() { ); await tester.pump(); - debugDefaultTargetPlatformOverride = null; - expect(asked, 0); expect(launched, ['https://example.com/a']); - }); + }, variant: desktop); testWidgets('desktop still asks when the URI is built to be misread', ( tester, ) async { - debugDefaultTargetPlatformOverride = TargetPlatform.macOS; - final context = await pumpHost(tester); final launched = []; Uri? asked; @@ -332,21 +329,18 @@ void main() { ); await tester.pump(); - debugDefaultTargetPlatformOverride = null; - - // An address bar would have shown this too -- and been read as GitHub, - // which is the whole reason the sheet names the host on its own line. + // The hover readout would have shown this too -- and been read as + // GitHub, which is the whole reason the sheet names the host on its own + // line. expect(asked?.host, 'evil.example'); expect(launched, [ 'https://github.com@evil.example/antgrid/pull/13', ]); - }); + }, variant: desktop); testWidgets('desktop cancelling a deceptive link launches nothing', ( tester, ) async { - debugDefaultTargetPlatformOverride = TargetPlatform.macOS; - final context = await pumpHost(tester); var launched = 0; @@ -358,10 +352,8 @@ void main() { ); await tester.pump(); - debugDefaultTargetPlatformOverride = null; - expect(launched, 0); - }); + }, variant: desktop); }); group('showTerminalHyperlinkSheet', () { From 086993fdd9dc66d98b651a3febff1f5368d1871a Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:41:39 +0530 Subject: [PATCH 4/6] chore: bump the dart_terminal pin so hovering no longer rebuilds the 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. --- THIRD-PARTY.md | 2 +- app/pubspec.lock | 12 ++++++------ app/pubspec.yaml | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/THIRD-PARTY.md b/THIRD-PARTY.md index 2eff2f5c..58f82e34 100644 --- a/THIRD-PARTY.md +++ b/THIRD-PARTY.md @@ -114,7 +114,7 @@ ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 ``` `ghostty_vte_flutter` and `portable_pty` are pinned to the same repository and diff --git a/app/pubspec.lock b/app/pubspec.lock index 4a9ada28..4dfc05e7 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -682,8 +682,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/vte/ghostty_vte" - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d - resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -691,8 +691,8 @@ packages: dependency: "direct main" description: path: "pkgs/vte/ghostty_vte_flutter" - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d - resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -1204,8 +1204,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/pty/portable_pty" - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d - resolved-ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.0.6+antgrid.2" diff --git a/app/pubspec.yaml b/app/pubspec.yaml index b93714ef..69bdc3c4 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -191,17 +191,17 @@ dependency_overrides: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte_flutter - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 portable_pty: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/pty/portable_pty - ref: dc5a33376eb5fe6679d56927f4639d46d91b422d + ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 # Microsoft Store packaging (MSIX). The Store signs the package itself, so no # code-signing cert is used on this path (unlike the Inno Setup installer in From 6e8f180d33cee628dcc1b497f33a22d210275d06 Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:46:51 +0530 Subject: [PATCH 5/6] fix: skip the confirmation only when the app actually showed the destination 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. --- app/lib/util/external_url.dart | 48 ++++------------ app/lib/widgets/terminal_hyperlink_sheet.dart | 9 +-- app/lib/widgets/terminal_view_wrapper.dart | 15 +++-- app/test/terminal_hyperlink_test.dart | 56 +++++++++++++------ 4 files changed, 67 insertions(+), 61 deletions(-) diff --git a/app/lib/util/external_url.dart b/app/lib/util/external_url.dart index e1f1cd2f..82c2f525 100644 --- a/app/lib/util/external_url.dart +++ b/app/lib/util/external_url.dart @@ -1,4 +1,3 @@ -import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:url_launcher/url_launcher.dart'; @@ -79,9 +78,15 @@ Uri? openableTerminalHyperlink(String uri) { /// returns, so a rejection would reach `PlatformDispatcher.onError` as a fatal /// carrying no in-app frames. /// -/// The destination is confirmed first on touch, and on any platform when the -/// URI is shaped to be misread — see [showTerminalHyperlinkSheet] for why that -/// is not merely a nag, and [terminalHyperlinkLooksDeceptive] for the shapes. +/// The destination is confirmed first unless it was already on screen, and +/// whenever the URI is shaped to be misread — see [showTerminalHyperlinkSheet] +/// for why that is not merely a nag, and [terminalHyperlinkLooksDeceptive] for +/// the shapes. +/// +/// [disclosed] is the caller saying it had this exact URI painted when the +/// activation landed — `TerminalHyperlinkPreview`, in practice. It defaults to +/// false because "we showed it" is a claim only the surface that showed it can +/// make, and a caller that has no readout must not inherit one by omission. /// /// [open] and [confirm] are injectable so tests can assert what would be /// launched instead of handing a URL to the real browser, matching @@ -89,6 +94,7 @@ Uri? openableTerminalHyperlink(String uri) { Future openTerminalHyperlink( BuildContext context, String uri, { + bool disclosed = false, Future Function(BuildContext, String) open = openExternalUrl, Future Function(BuildContext, Uri) confirm = showTerminalHyperlinkSheet, }) async { @@ -106,7 +112,7 @@ Future openTerminalHyperlink( } return; } - if (_shownBeforeActivated && !terminalHyperlinkLooksDeceptive(target)) { + if (disclosed && !terminalHyperlinkLooksDeceptive(target)) { await open(context, target.toString()); return; } @@ -150,7 +156,7 @@ Future openTerminalHyperlink( /// subdomain of an honest domain, and only the link's visible TEXT contradicts /// it — text that never reaches this app. A false negative here costs the user /// the extra confirmation, not the disclosure: the sheet names the host either -/// way, and on touch it is shown unconditionally. +/// way, and a link that was never on screen is confirmed regardless of shape. bool terminalHyperlinkLooksDeceptive(Uri target) { if (target.userInfo.isNotEmpty) { return true; @@ -163,33 +169,3 @@ bool terminalHyperlinkLooksDeceptive(Uri target) { // checks, not the parser's contract two files away. return host.split('.').any((label) => label.toLowerCase().startsWith('xn--')); } - -/// Whether this platform showed the destination before the link was activated. -/// -/// Desktop has a pointer, so `TerminalHyperlinkPreview` has already painted the -/// URI under it by the time a click lands — that readout is the disclosure, and -/// it is the only reason this path may skip the sheet. Deliberately NOT the -/// browser's address bar: `LaunchMode.externalApplication` resolves through -/// `ShellExecuteW` on Windows, `NSWorkspace.open` on macOS and -/// `g_app_info_launch_default_for_uri` on Linux, every one of which honours a -/// registered `https:` handler — so a desktop click can land in an app with no -/// address bar just as a mobile one can (see [openableTerminalHyperlink]). -/// -/// Touch has no hover and therefore no readout, which is why it asks every time. -/// -/// A BARE URL — one the view matched by regex rather than by OSC 8 markup — is -/// not a gap here even though the readout may not have marked it: its visible -/// text IS the URI, which is how it was found, so there is nothing the target -/// can disagree with. -/// -/// Known gap, reported rather than papered over: this asks the OS, not the -/// input device, so a finger tap on a desktop touchscreen takes the desktop -/// branch having been shown nothing. The pointer kind is the right predicate -/// and lives in the terminal package, not here. -/// -/// A readout only discloses what the reader then has to judge, so this is not -/// the whole desktop rule: [terminalHyperlinkLooksDeceptive] pulls the cases -/// back to the sheet where the URI is built to be misread. -bool get _shownBeforeActivated => - defaultTargetPlatform != TargetPlatform.android && - defaultTargetPlatform != TargetPlatform.iOS; diff --git a/app/lib/widgets/terminal_hyperlink_sheet.dart b/app/lib/widgets/terminal_hyperlink_sheet.dart index 1412cc54..5c607d89 100644 --- a/app/lib/widgets/terminal_hyperlink_sheet.dart +++ b/app/lib/widgets/terminal_hyperlink_sheet.dart @@ -10,10 +10,11 @@ import '../design/widgets/ab_dialog.dart'; /// /// OSC 8 lets a link's visible text disagree with its destination, so the text /// a user taps is not evidence of anything: `https://github.com@evil.example/` -/// reads as GitHub and resolves to `evil.example`. Desktop gets its disclosure -/// from the hover readout instead (`TerminalHyperlinkPreview`); touch has no -/// hover, and a tap there may never reach a browser at all, because a verified -/// App Link opens its own app. +/// reads as GitHub and resolves to `evil.example`. This is what names the real +/// host wherever nothing else did — a pointer gets `TerminalHyperlinkPreview` +/// instead, and a finger gets no readout at all. Nor is a browser a backstop: +/// an activation may never reach one, because a verified App Link opens its own +/// app. /// /// Returns false when dismissed, so a stray tap outside the sheet cancels. Future showTerminalHyperlinkSheet( diff --git a/app/lib/widgets/terminal_view_wrapper.dart b/app/lib/widgets/terminal_view_wrapper.dart index 20dbc999..6e30c1cb 100644 --- a/app/lib/widgets/terminal_view_wrapper.dart +++ b/app/lib/widgets/terminal_view_wrapper.dart @@ -621,10 +621,17 @@ class _TerminalViewWrapperState extends ConsumerState { // uses the platform-default launch mode and reports nothing when it // fails. Route through the app's helper so a link opens externally and a // failure is visible, and so terminal-authored URIs are scheme-checked. - onOpenHyperlink: (uri) => openTerminalHyperlink(context, uri), - // The other half of that disclosure: `openTerminalHyperlink` asks before - // opening only where the destination would otherwise never be shown at - // all, and this is what shows it everywhere else. + // `disclosed` is this widget answering for its own readout, not a guess + // from the platform: the card is up for THIS uri, so the destination was + // on screen when the activation landed. A desktop touchscreen, a Shift + // chord and a link that scrolled out from under a resting pointer all + // reach here with nothing shown, and all of them get the sheet — which a + // `defaultTargetPlatform` test silently exempted the first of. + onOpenHyperlink: (uri) => openTerminalHyperlink( + context, + uri, + disclosed: _hoveredLink.value?.uri == uri, + ), onHyperlinkHover: _onHyperlinkHover, showHeader: false, showFocusRing: false, diff --git a/app/test/terminal_hyperlink_test.dart b/app/test/terminal_hyperlink_test.dart index 378334a6..8350e608 100644 --- a/app/test/terminal_hyperlink_test.dart +++ b/app/test/terminal_hyperlink_test.dart @@ -232,10 +232,11 @@ void main() { expect(line['error'], contains('launcher exploded')); }); - // flutter_test reports android by default, so these run on the touch path - // unless they say otherwise. + // No platform override anywhere in this group any more: what decides is + // whether the caller says it had the destination on screen, and a caller + // that says nothing is a caller that showed nothing. testWidgets( - 'asks before opening on touch, and cancelling launches nothing', + 'asks when nothing disclosed the target, and cancelling launches nothing', (tester) async { final context = await pumpHost(tester); var launched = 0; @@ -283,13 +284,7 @@ void main() { expect(asked?.host, 'evil.example'); }); - // TargetPlatformVariant, not a hand-placed - // `debugDefaultTargetPlatformOverride`: the variant sets and restores at - // the binding's own lifecycle points, so a throw between the override and - // its reset can no longer leak macOS into the touch-path tests above. - final desktop = TargetPlatformVariant.only(TargetPlatform.macOS); - - testWidgets('desktop opens an ordinary link without asking', ( + testWidgets('opens an ordinary link that was on screen, without asking', ( tester, ) async { final context = await pumpHost(tester); @@ -299,6 +294,7 @@ void main() { await openTerminalHyperlink( context, 'https://example.com/a', + disclosed: true, open: (_, url) async => launched.add(url), confirm: (_, _) async { asked++; @@ -309,9 +305,9 @@ void main() { expect(asked, 0); expect(launched, ['https://example.com/a']); - }, variant: desktop); + }); - testWidgets('desktop still asks when the URI is built to be misread', ( + testWidgets('still asks when the URI is built to be misread', ( tester, ) async { final context = await pumpHost(tester); @@ -321,6 +317,7 @@ void main() { await openTerminalHyperlink( context, 'https://github.com@evil.example/antgrid/pull/13', + disclosed: true, open: (_, url) async => launched.add(url), confirm: (_, target) async { asked = target; @@ -336,24 +333,49 @@ void main() { expect(launched, [ 'https://github.com@evil.example/antgrid/pull/13', ]); - }, variant: desktop); + }); - testWidgets('desktop cancelling a deceptive link launches nothing', ( - tester, - ) async { + testWidgets('cancelling a deceptive link launches nothing', (tester) async { final context = await pumpHost(tester); var launched = 0; await openTerminalHyperlink( context, 'https://xn--pple-43d.com/login', + disclosed: true, open: (_, _) async => launched++, confirm: (_, _) async => false, ); await tester.pump(); expect(launched, 0); - }, variant: desktop); + }); + + // The gap a `defaultTargetPlatform` test silently exempted: a finger tap on + // a desktop touchscreen took the desktop branch on the grounds that the + // hover readout had disclosed the destination, and there is no hover on + // touch. Asking the CALLER what it showed cannot answer that wrongly -- + // and it covers the Shift chord and a link scrolled out from under a + // resting pointer, which the platform test missed for the same reason. + testWidgets('asks on desktop when nothing was on screen', (tester) async { + final context = await pumpHost(tester); + var launched = 0; + var asked = 0; + + await openTerminalHyperlink( + context, + 'https://example.com/a', + open: (_, _) async => launched++, + confirm: (_, _) async { + asked++; + return false; + }, + ); + await tester.pump(); + + expect(asked, 1); + expect(launched, 0); + }, variant: TargetPlatformVariant.desktop()); }); group('showTerminalHyperlinkSheet', () { From 36a1c1ef862142e277b799ced2954650dfd40662 Mon Sep 17 00:00:00 2001 From: Bharath Mohan <2254476+bharathm03@users.noreply.github.com> Date: Thu, 27 Aug 2026 18:16:03 +0530 Subject: [PATCH 6/6] chore: re-pin dart_terminal to the merged master commit 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. --- THIRD-PARTY.md | 2 +- app/pubspec.lock | 12 ++++++------ app/pubspec.yaml | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/THIRD-PARTY.md b/THIRD-PARTY.md index 58f82e34..cc2887e9 100644 --- a/THIRD-PARTY.md +++ b/THIRD-PARTY.md @@ -114,7 +114,7 @@ ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 + ref: 6cd393196ed301afa1d8ada7a996cc345b899b4a ``` `ghostty_vte_flutter` and `portable_pty` are pinned to the same repository and diff --git a/app/pubspec.lock b/app/pubspec.lock index 4dfc05e7..d9114c67 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -682,8 +682,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/vte/ghostty_vte" - ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" - resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" + resolved-ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -691,8 +691,8 @@ packages: dependency: "direct main" description: path: "pkgs/vte/ghostty_vte_flutter" - ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" - resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" + resolved-ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.1.4+antgrid.1" @@ -1204,8 +1204,8 @@ packages: dependency: "direct overridden" description: path: "pkgs/pty/portable_pty" - ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" - resolved-ref: "614e05d9e1f927ef9a64d34cde67e094e56659c0" + ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" + resolved-ref: "6cd393196ed301afa1d8ada7a996cc345b899b4a" url: "https://github.com/antgrid-ai/dart_terminal.git" source: git version: "0.0.6+antgrid.2" diff --git a/app/pubspec.yaml b/app/pubspec.yaml index 69bdc3c4..64c921cf 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -191,17 +191,17 @@ dependency_overrides: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte_flutter - ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 + ref: 6cd393196ed301afa1d8ada7a996cc345b899b4a ghostty_vte: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/vte/ghostty_vte - ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 + ref: 6cd393196ed301afa1d8ada7a996cc345b899b4a portable_pty: git: url: https://github.com/antgrid-ai/dart_terminal.git path: pkgs/pty/portable_pty - ref: 614e05d9e1f927ef9a64d34cde67e094e56659c0 + ref: 6cd393196ed301afa1d8ada7a996cc345b899b4a # Microsoft Store packaging (MSIX). The Store signs the package itself, so no # code-signing cert is used on this path (unlike the Inno Setup installer in