diff --git a/CHANGELOG.md b/CHANGELOG.md index f4054f47..055125ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,11 +18,12 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. ### Added -- **`clide://` deep links open files.** `clide://open?path=/repo/file.dart&line=42` - opens the file at the line — handy for CI links, error reports, and cross-tool - integration. The link routes through the existing CLI→IPC path, so it lands in - the running window (no second instance). The scheme is registered on Linux - (`x-scheme-handler/clide`) and macOS. (T-56) +- **`clide://` deep links open files, safely.** + `clide://open?path=/repo/file.dart&line=42` opens the file at that line (CI + links, error reports), routed through the CLI→IPC path into the running + window. As an untrusted external vector it's gated by a default-deny allowlist + (navigation only) and a confirmation prompt before any action. Registered on + Linux + macOS. (T-56, D-90) - **Number keys pick prompt buttons (CLI muscle memory).** In a permission or AskUserQuestion prompt, `1`/`2`/`3`… select the matching button or option (labels are now numbered), and Enter confirms the primary action. Typing in a diff --git a/governance/README.md b/governance/README.md index 989ff34b..dee9fbc8 100644 --- a/governance/README.md +++ b/governance/README.md @@ -131,6 +131,7 @@ You might also want, project-permitting: - [D-87: Output/log dock — bottom, toggled, read-only (logs + problems)](decisions/architecture.md#d-87-outputlog-dock--bottom-toggled-read-only-logs--problems) — _architecture_ - [D-88: clide-owned anchored popover + menu primitive](decisions/design.md#d-88-clide-owned-anchored-popover--menu-primitive) — _design_ - [D-89: inline pasted-image thumbnails that expand to the lightbox](decisions/design.md#d-89-inline-pasted-image-thumbnails-that-expand-to-the-lightbox) — _design_ +- [D-90: clide:// deep links — paranoid allowlist + user confirmation](decisions/architecture.md#d-90-clide-deep-links--paranoid-allowlist--user-confirmation) — _architecture_ ## Open questions diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index 99a1dc4a..1e7959b0 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -451,3 +451,12 @@ Core, rendering, IPC, kernel, panel manager. - **Raised by:** 2026-06-06 — T-54 UX design session (Frame0 wireframe under `docs/design/wireframes/output-dock/`). User chose a status-bar-toggled bottom dock scoped to read-only output, Problems folded in, terminal kept first-class in the editor pane. --- + +### D-90: clide:// deep links — paranoid allowlist + user confirmation +- **Date:** 2026-06-09 +- **Decision:** The `clide://` URL scheme (T-56) is treated as an UNTRUSTED external vector. A clide:// link is NOT translated into a command in the CLI parser; `parseArgv` hands the raw URL to a `deeplink.invoke` handler that is doubly defensive: (1) **default-deny allowlist** — only the actions in `kDeepLinkSafeActions` (today just `open`, a read-only navigation verb) are even parseable; anything else (`run`, `git`, `write`, arbitrary command passthrough, non-`clide` schemes) is rejected outright; and (2) **mandatory user confirmation** — every allowlisted action shows a "an external link wants to: … allow?" modal, framed as untrusted, before it executes. The OS scheme registration (linux `.desktop` `x-scheme-handler/clide`, macOS `CFBundleURLTypes`) + the CLI→IPC route mean a link lands in the running window (single-instance). +- **Rationale:** The `clide` CLI is a *local, trusted* surface (D-1/D-6); a URL handler is the opposite — any webpage can fire `clide://…` at the OS. A generic passthrough to the full command surface would be maximally extensible but would turn a malicious link into a remote control for the IDE (trigger `git push`, file writes, session kills). Paranoid-allowlist + confirm keeps the useful "open this file at this line from a CI link / error report" case while making the dangerous surface unreachable. The allowlist is the security boundary and is unit-tested in isolation. +- **Cost:** Each new deep-link action is an explicit, reviewed addition to `kDeepLinkSafeActions` + its handler branch — extensibility is deliberately gated, not free. The confirmation prompt adds a click to every deep-link open (acceptable for an out-of-band entry point). macOS URL *delivery* (AppDelegate `openURLs` → Dart) is a separate follow-up (T-303); the security model applies once delivery lands. +- **Raised by:** 2026-06-09 — user, on reviewing the T-56 passthrough: "put a heavy blocklist with do-you-want-this prompts on it (a paranoid allowlist)." Spotted that routing clide:// through the CLI path is "incredibly extensible" and that extensibility is exactly the risk for an external vector. + +--- diff --git a/lib/builtin/deeplink/deeplink.dart b/lib/builtin/deeplink/deeplink.dart new file mode 100644 index 00000000..11ffc4b3 --- /dev/null +++ b/lib/builtin/deeplink/deeplink.dart @@ -0,0 +1,5 @@ +/// Built-in `clide://` deep-link handler (T-56, D-90). +library; + +export 'src/deep_link.dart'; +export 'src/extension.dart'; diff --git a/lib/builtin/deeplink/src/deep_link.dart b/lib/builtin/deeplink/src/deep_link.dart new file mode 100644 index 00000000..fc91ad70 --- /dev/null +++ b/lib/builtin/deeplink/src/deep_link.dart @@ -0,0 +1,51 @@ +/// Parsing + the paranoid allowlist for `clide://` deep links (T-56, D-90). +/// +/// A clide:// URL is an UNTRUSTED external vector — any webpage can fire one at +/// the OS handler — so the surface is **default-deny**: only the actions in +/// [kDeepLinkSafeActions] are even parseable, and they are read-only / +/// navigation verbs. Execution is additionally gated by a user prompt (see the +/// deeplink extension). Pure (no Flutter): the allowlist + parsing are unit +/// tested in isolation. +library; + +/// The ONLY actions a clide:// link may request. Default-deny: anything not in +/// this set is rejected outright. Keep it to side-effect-free navigation — +/// NEVER writes, process control, git, or arbitrary command passthrough. +const Set kDeepLinkSafeActions = {'open'}; + +/// A validated, allowlisted deep-link action. +class DeepLinkAction { + const DeepLinkAction({required this.name, required this.path, this.line}); + + final String name; + final String path; + final int? line; + + /// A human-readable description for the confirmation prompt. + String get describe => switch (name) { + 'open' => 'Open $path${line != null ? ' (line $line)' : ''}', + _ => name, + }; +} + +/// Parse [url] into a [DeepLinkAction], or null when it is malformed, not a +/// `clide://` URL, not an allowlisted action, or missing required parameters. +/// Validation only — it never executes anything. +DeepLinkAction? parseDeepLink(String url) { + final uri = Uri.tryParse(url); + if (uri == null || uri.scheme != 'clide') return null; + if (!kDeepLinkSafeActions.contains(uri.host)) return null; + switch (uri.host) { + case 'open': + final path = uri.queryParameters['path']; + if (path == null || path.isEmpty) return null; + int? line; + final lineRaw = uri.queryParameters['line']; + if (lineRaw != null && lineRaw.isNotEmpty) { + line = int.tryParse(lineRaw); + if (line == null || line < 1) return null; // reject junk rather than guess + } + return DeepLinkAction(name: 'open', path: path, line: line); + } + return null; +} diff --git a/lib/builtin/deeplink/src/extension.dart b/lib/builtin/deeplink/src/extension.dart new file mode 100644 index 00000000..442238d9 --- /dev/null +++ b/lib/builtin/deeplink/src/extension.dart @@ -0,0 +1,107 @@ +/// `clide://` deep-link handler (T-56, D-90). +/// +/// Registers the `deeplink.invoke` command that the CLI routes a clide:// URL to +/// (via parseArgv). Because a clide:// link is an UNTRUSTED external vector, the +/// handler is doubly defensive: it only honours [kDeepLinkSafeActions] +/// (default-deny) AND it prompts the user before doing anything. +library; + +import 'package:clide/builtin/deeplink/src/deep_link.dart'; +import 'package:clide/clide.dart'; +import 'package:clide/extension/extension.dart'; +import 'package:clide/kernel/kernel.dart'; +import 'package:clide/widgets/widgets.dart'; +import 'package:flutter/widgets.dart'; + +class DeepLinkExtension extends ClideExtension { + @override + String get id => 'builtin.deeplink'; + @override + String get title => 'Deep links'; + @override + String get version => '0.1.0'; + + ClideExtensionContext? _ctx; + + @override + List get contributions => [ + CommandContribution( + id: 'deeplink.invoke', + command: 'deeplink.invoke', + title: 'Open a clide:// deep link', + run: _invoke, + ), + ]; + + @override + Future activate(ClideExtensionContext ctx) async => _ctx = ctx; + + @override + Future deactivate() async => _ctx = null; + + Future _invoke(List args) async { + final ctx = _ctx; + if (ctx == null) return IpcResponse.ok(id: '', data: const {'status': 'not-activated'}); + + final url = args.isEmpty ? null : args.first; + final action = url == null ? null : parseDeepLink(url); + // Default-deny: a malformed or non-allowlisted link never acts. + if (action == null) { + return IpcResponse.ok(id: '', data: {'status': 'rejected', 'url': url}); + } + + // Always confirm — an external page must not silently drive the IDE (D-90). + final ok = await ctx.dialog.show((c, dismiss) => _DeepLinkConfirmDialog(action: action, onResolve: dismiss)); + if (ok != true) return IpcResponse.ok(id: '', data: const {'status': 'declined'}); + + switch (action.name) { + case 'open': + await ctx.ipc.request('editor.open', args: {'path': action.path, if (action.line != null) 'line': action.line}); + return IpcResponse.ok(id: '', data: {'status': 'opened', 'path': action.path}); + } + return IpcResponse.ok(id: '', data: const {'status': 'rejected'}); + } +} + +/// Confirmation modal for an incoming deep link — frames it as untrusted and +/// requires an explicit Open. Cancel (the default) declines. +class _DeepLinkConfirmDialog extends StatelessWidget { + const _DeepLinkConfirmDialog({required this.action, required this.onResolve}); + + final DeepLinkAction action; + final void Function([bool? result]) onResolve; + + @override + Widget build(BuildContext context) { + final t = ClideTheme.of(context).surface; + return ClideSurface( + width: 440, + color: t.modalSurfaceBackground, + border: t.modalSurfaceBorder, + padding: const EdgeInsets.all(16), + borderRadius: BorderRadius.circular(6), + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + ClideText('Open an external link?', fontSize: clideFontBody, color: t.globalForeground), + const SizedBox(height: 6), + ClideText('A clide:// link from outside the app is asking to:', muted: true, fontSize: clideFontSmall), + const SizedBox(height: 8), + ClideText(action.describe, fontFamily: clideMonoFamily, fontSize: clideFontSmall, color: t.globalForeground), + const SizedBox(height: 8), + ClideText('Only allow this if you trust where the link came from.', fontSize: clideFontMeta, color: t.statusWarning), + const SizedBox(height: 14), + Row( + mainAxisAlignment: MainAxisAlignment.end, + children: [ + ClideButton(label: 'Cancel', onPressed: () => onResolve(false)), + const SizedBox(width: 8), + ClideButton(label: 'Open', variant: ClideButtonVariant.primary, onPressed: () => onResolve(true)), + ], + ), + ], + ), + ); + } +} diff --git a/lib/main.dart b/lib/main.dart index bc917eec..5a57f695 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -7,6 +7,7 @@ import 'package:clide/builtin/claude/claude.dart'; import 'package:clide/builtin/claude_control/claude_control.dart'; import 'package:clide/builtin/cli_install/cli_install.dart'; import 'package:clide/builtin/decisions/decisions.dart'; +import 'package:clide/builtin/deeplink/deeplink.dart'; import 'package:clide/builtin/default_layout/default_layout.dart'; import 'package:clide/builtin/diff/diff.dart'; import 'package:clide/builtin/editor/editor.dart'; @@ -371,6 +372,7 @@ Future main() async { ..register(GitExtension()) ..register(PqlExtension()) ..register(ProblemsExtension()) + ..register(DeepLinkExtension()) // Workspace ..register(ClaudeExtension()) ..register(TerminalExtension()) diff --git a/lib/src/cli/argv_to_request.dart b/lib/src/cli/argv_to_request.dart index 9672f5a1..225cd0f6 100644 --- a/lib/src/cli/argv_to_request.dart +++ b/lib/src/cli/argv_to_request.dart @@ -104,37 +104,14 @@ ArgvParseResult parseArgv(List argv, {required String requestId}) { )); } -/// Translate a `clide://` deep link into an [IpcRequest] (T-56). Only the -/// `open` action is defined today: `clide://open?path=&line=` -/// maps to `editor.open` (which jumps the selection to the 1-based line). The -/// path is passed through verbatim — `editor.open` resolves it against the -/// workspace and applies the usual `files.read` allow-list (D-80). -ArgvParseResult _deepLinkToRequest(String url, String requestId) { - final uri = Uri.tryParse(url); - if (uri == null || uri.scheme != 'clide') { - return ArgvError(_err(requestId, 'malformed clide:// link: $url')); - } - switch (uri.host) { - case 'open': - final path = uri.queryParameters['path']; - if (path == null || path.isEmpty) { - return ArgvError(_err(requestId, 'clide://open requires a ?path=')); - } - final positional = [path]; - final lineRaw = uri.queryParameters['line']; - if (lineRaw != null && lineRaw.isNotEmpty) { - final line = int.tryParse(lineRaw); - if (line == null || line < 1) { - return ArgvError(_err(requestId, 'clide://open: line must be a positive integer, got "$lineRaw"')); - } - positional.add('$line'); - } - // Mirror the CLI's positional form so the editor.open schema maps them. - return ArgvParsed(IpcRequest(id: requestId, cmd: 'editor.open', args: {'positional': positional})); - default: - return ArgvError(_err(requestId, 'unknown clide:// action "${uri.host}" (expected: open)')); - } -} +/// Route a `clide://` deep link to the `deeplink.invoke` command (T-56). A +/// clide:// URL is an UNTRUSTED external vector — any webpage can fire one — so +/// it is NOT translated into a command here. The raw URL is handed to the +/// deeplink handler, which validates it against a paranoid (default-deny) +/// allowlist and prompts the user before doing anything (D-90). +ArgvParseResult _deepLinkToRequest(String url, String requestId) => ArgvParsed(IpcRequest(id: requestId, cmd: 'deeplink.invoke', args: { + 'positional': [url] + })); // -- internals -------------------------------------------------------------- diff --git a/test/builtin/deeplink/deep_link_test.dart b/test/builtin/deeplink/deep_link_test.dart new file mode 100644 index 00000000..7f97cb06 --- /dev/null +++ b/test/builtin/deeplink/deep_link_test.dart @@ -0,0 +1,61 @@ +/// Tests for the clide:// deep-link parser + paranoid allowlist (T-56, D-90). +/// The allowlist is the security boundary, so it gets the bulk of the coverage. +library; + +import 'package:clide/builtin/deeplink/src/deep_link.dart'; +import 'package:test/test.dart'; + +void main() { + group('parseDeepLink — open', () { + test('parses path + optional line', () { + final a = parseDeepLink('clide://open?path=/repo/x.dart')!; + expect(a.name, 'open'); + expect(a.path, '/repo/x.dart'); + expect(a.line, isNull); + + final b = parseDeepLink('clide://open?path=/repo/x.dart&line=42')!; + expect(b.line, 42); + }); + + test('decodes a percent-encoded path', () { + expect(parseDeepLink('clide://open?path=/a%20b/c.dart')!.path, '/a b/c.dart'); + }); + + test('describe reads naturally for the prompt', () { + expect(parseDeepLink('clide://open?path=/x&line=9')!.describe, contains('/x')); + expect(parseDeepLink('clide://open?path=/x&line=9')!.describe, contains('9')); + }); + + test('missing/empty path is rejected', () { + expect(parseDeepLink('clide://open'), isNull); + expect(parseDeepLink('clide://open?path='), isNull); + }); + + test('a non-positive or non-numeric line is rejected (no guessing)', () { + expect(parseDeepLink('clide://open?path=/x&line=0'), isNull); + expect(parseDeepLink('clide://open?path=/x&line=-3'), isNull); + expect(parseDeepLink('clide://open?path=/x&line=abc'), isNull); + }); + }); + + group('paranoid allowlist (default-deny) — the security boundary', () { + test('the allowlist is exactly the safe navigation set', () { + expect(kDeepLinkSafeActions, {'open'}); + }); + + test('a non-allowlisted action is rejected even if well-formed', () { + // These are the kind of things a malicious page might try. + expect(parseDeepLink('clide://run?cmd=rm'), isNull); + expect(parseDeepLink('clide://git?verb=push'), isNull); + expect(parseDeepLink('clide://write?path=/x&content=evil'), isNull); + expect(parseDeepLink('clide://exec?path=/x'), isNull); + }); + + test('a non-clide scheme or garbage is rejected', () { + expect(parseDeepLink('https://evil.com/open?path=/x'), isNull); + expect(parseDeepLink('file:///etc/passwd'), isNull); + expect(parseDeepLink('not a url at all'), isNull); + expect(parseDeepLink(''), isNull); + }); + }); +} diff --git a/test/builtin/deeplink/extension_test.dart b/test/builtin/deeplink/extension_test.dart new file mode 100644 index 00000000..9eaa6f5e --- /dev/null +++ b/test/builtin/deeplink/extension_test.dart @@ -0,0 +1,56 @@ +/// Tests for the deeplink handler's gating (T-56, D-90): an allowlisted link +/// prompts before acting; a non-allowlisted one is rejected with no prompt. +library; + +import 'dart:async'; + +import 'package:clide/builtin/deeplink/deeplink.dart'; +import 'package:flutter/widgets.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../helpers/kernel_fixture.dart'; +import '../../helpers/widget_harness.dart'; + +void main() { + late KernelFixture f; + setUp(() async { + f = await KernelFixture.create(); + f.services.extensions.register(DeepLinkExtension()); + await f.services.extensions.activate('builtin.deeplink'); + }); + tearDown(() => f.dispose()); + + testWidgets('an allowlisted link prompts before doing anything', (tester) async { + await tester.pumpWidget(harness(f, const SizedBox())); + await tester.pump(); + expect(f.services.dialog.isOpen, isFalse); + + // The handler blocks on the confirmation, so don't await it. + unawaited(f.services.commands.execute('deeplink.invoke', args: ['clide://open?path=/x.dart'])); + await tester.pump(); + + expect(f.services.dialog.isOpen, isTrue, reason: 'a confirmation must be shown before acting'); + f.services.dialog.dismiss(false); // decline → cleanup + await tester.pump(); + }); + + testWidgets('a non-allowlisted link is rejected with no prompt', (tester) async { + await tester.pumpWidget(harness(f, const SizedBox())); + await tester.pump(); + + final r = await f.services.commands.execute('deeplink.invoke', args: ['clide://run?cmd=rm%20-rf']); + await tester.pump(); + + expect(f.services.dialog.isOpen, isFalse, reason: 'no dialog for a rejected link'); + expect(r.data['status'], 'rejected'); + }); + + testWidgets('a malformed link is rejected with no prompt', (tester) async { + await tester.pumpWidget(harness(f, const SizedBox())); + await tester.pump(); + + final r = await f.services.commands.execute('deeplink.invoke', args: ['https://evil.example/open?path=/x']); + expect(f.services.dialog.isOpen, isFalse); + expect(r.data['status'], 'rejected'); + }); +} diff --git a/test/cli/argv_to_request_test.dart b/test/cli/argv_to_request_test.dart index 6f40b9a6..d151ea2c 100644 --- a/test/cli/argv_to_request_test.dart +++ b/test/cli/argv_to_request_test.dart @@ -150,37 +150,18 @@ void main() { }); }); - group('parseArgv — clide:// deep links (T-56)', () { - test('clide://open?path= maps to editor.open', () { - final req = _expectOk(parseArgv(['clide://open?path=/repo/x.md'], requestId: '1')); - expect(req.cmd, 'editor.open'); - expect(req.args['positional'], ['/repo/x.md']); + group('parseArgv — clide:// deep links route to the gated handler (T-56)', () { + test('a clide:// URL is handed verbatim to deeplink.invoke (not translated)', () { + // Validation + the user prompt happen in the handler (D-90), not here. + final req = _expectOk(parseArgv(['clide://open?path=/repo/x.md&line=42'], requestId: '1')); + expect(req.cmd, 'deeplink.invoke'); + expect(req.args['positional'], ['clide://open?path=/repo/x.md&line=42']); }); - test('a &line= becomes the second positional', () { - final req = _expectOk(parseArgv(['clide://open?path=/repo/x.md&line=42'], requestId: '2')); - expect(req.cmd, 'editor.open'); - expect(req.args['positional'], ['/repo/x.md', '42']); - }); - - test('an encoded path is decoded', () { - final req = _expectOk(parseArgv(['clide://open?path=/a%20b/c.dart'], requestId: '3')); - expect(req.args['positional'], ['/a b/c.dart']); - }); - - test('missing path errors', () { - final err = _expectErr(parseArgv(['clide://open'], requestId: '4')); - expect(err.error?.message, contains('requires a ?path')); - }); - - test('a non-positive or non-numeric line errors', () { - expect(_expectErr(parseArgv(['clide://open?path=/x&line=0'], requestId: '5')).error?.message, contains('positive integer')); - expect(_expectErr(parseArgv(['clide://open?path=/x&line=abc'], requestId: '6')).error?.message, contains('positive integer')); - }); - - test('an unknown action errors', () { - final err = _expectErr(parseArgv(['clide://frobnicate?x=1'], requestId: '7')); - expect(err.error?.message, contains('unknown clide:// action')); + test('even an unknown action is passed through (the handler rejects it)', () { + final req = _expectOk(parseArgv(['clide://frobnicate?x=1'], requestId: '2')); + expect(req.cmd, 'deeplink.invoke'); + expect(req.args['positional'], ['clide://frobnicate?x=1']); }); }); }