Skip to content

Let users approve sidecars outside ALLOWED_SERVICES via a native prompt - #171

Merged
DianaSensei merged 4 commits into
mainfrom
feat/user-approved-sidecars
Oct 6, 2026
Merged

DianaSensei merged 4 commits into
mainfrom
feat/user-approved-sidecars

Conversation

@DianaSensei

@DianaSensei DianaSensei commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Adding a plugin that ships a sidecar no longer requires a DevTool release.

What changes

  • A sidecar name outside ALLOWED_SERVICES that matches devtool-svc-<kebab-case> is no longer refused outright. The first time it is about to be spawned, Rust shows a native dialog with the binary's path and SHA-256 and asks the user to allow it. The webview can trigger the question but cannot answer it.
  • The decision is stored in the OS keychain (devtool / trusted-services), bound to the binary's SHA-256: a different build asks again, and uninstalling the sidecar revokes it. Where no keychain is available the decision lasts for the session only (asks again rather than never).
  • Names that cannot be a sidecar (/bin/sh, devtool-svc-../x, …) are still rejected before anything else.
  • The bundled sidecars in ALLOWED_SERVICES keep running without a prompt.

The dialog

Native on every platform — NSAlert (macOS), TaskDialog (Windows), GTK (Linux) — never a webview UI, which the webview could answer itself. In the language chosen in the app:

Allow “service-list-automation” to run?
A plugin wants to run this program on your computer, with your full user permissions.
Folder: ~/…/services/devtool-svc-service-list-automation/0.1.0 · SHA-256: 2009009a8a45…4529e8 · Source: github.com/…/service.manifest.json
Allow it only if you trust its source. DevTool will ask again if the program changes.

[Don’t Allow] [Allow]

Why the keychain and not a file

The default capability lets the webview write anywhere in app_data (fs:allow-write-file + fs:scope-appdata-recursive), so a file there — including extensions/index.json — could be edited to grant itself permission.

Notes

  • Consent happens before the registry lock is taken, so other sidecars keep working while the dialog is open; concurrent calls to the same unapproved sidecar raise a single dialog.
  • plugin-sdk docs (01/02/04/05) and the CHANGELOG are updated.
  • Tests: service_trust (remembered across restarts, denial, re-ask on a changed hash, one dialog for concurrent calls, no keychain, revoke) and the tightened name check; full cargo test passes. The native dialog and the real keychain are not covered by automated tests.

Also in this PR

npm audit fix (lockfile only, no --force, package.json unchanged): undici, sharp, source-map-js, fast-uri and smol-toml move to patched versions. The Security workflow's npm audit had been failing on main since those advisories were published. Full vitest suite (1452 tests) and tsc && vite build pass with the new lockfile.

🤖 Generated with Claude Code

Adding a plugin that ships a sidecar no longer requires an app release.
A name matching devtool-svc-<kebab-case> that is not in ALLOWED_SERVICES is
allowed only after the user approves the exact file (path + SHA-256) in a
native dialog raised from Rust. The decision is stored in the OS keychain,
not app_data, because the webview can write anywhere in app_data
(fs:allow-write-file on the appdata scope). It is bound to the SHA-256, so a
different binary asks again; uninstalling revokes it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.38951% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.92%. Comparing base (24b4932) to head (bcbd96b).

Files with missing lines Patch % Lines
src-tauri/src/service_host.rs 77.32% 39 Missing ⚠️
src-tauri/src/service_trust.rs 93.71% 21 Missing ⚠️
src-tauri/src/artifact_installer.rs 92.85% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
+ Coverage   42.75%   43.92%   +1.16%     
==========================================
  Files         302      303       +1     
  Lines       20222    20745     +523     
  Branches     5049     5049              
==========================================
+ Hits         8646     9112     +466     
- Misses      10535    10592      +57     
  Partials     1041     1041              
Flag Coverage Δ
frontend 38.12% <ø> (ø)
rust 68.56% <88.38%> (+3.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src-tauri/src/artifact_installer.rs 84.72% <92.85%> (+0.34%) ⬆️
src-tauri/src/service_trust.rs 93.71% <93.71%> (ø)
src-tauri/src/service_host.rs 79.01% <77.32%> (-0.18%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

The native consent prompt is a good direction, but the approval is currently bound to a SHA-256 that is computed before the prompt while the file is re-resolved and spawned after it, so the bytes the user approves are not guaranteed to be the bytes that run. Details inline.

Reviewed changes

  • service_trust.rs (new) — consent store bound to (bin, sha256), with a TrustBackend trait, a keychain backend (devtool / trusted-services), session-only fallback when the keychain is unavailable, a one-dialog gate, grant/revoke/ensure, and a unit-test suite covering restart persistence, denial, hash change, no-keychain, concurrent calls, revoke, and the dialog message.
  • service_host.rs — replaces "must be in ALLOWED_SERVICES" in reject_reason with a name-format check (is_valid_service_name), adds ask_user (native tauri-plugin-dialog) and ensure_trusted, adds ServiceRegistry.trust, and has get_or_spawn fast-path running bins then consent before spawning.
  • artifact_installer.rs — exposes sha256_hex as pub(crate), adds display-only installed_service_source_url(_at), and revokes trust on service uninstall.
  • main.rs — registers the service_trust module.
  • Docs + CHANGELOG — rewritten around the two-sources ("bundled allowlist" + "user consent") model.

ℹ️ URL install can still grant execution under an ALLOWED_SERVICES name with no prompt

This is pre-existing behavior, not introduced by this diff, but it directly contradicts the security framing the new docs assert, so it is worth resolving before the "two sources of authority" claim is treated as a real boundary.

Technical details
# The bundled allowlist is bypassable by a URL install

## Affected sites
- `src-tauri/src/service_host.rs:428-430` — `ensure_trusted` returns `Ok(())` for any `ALLOWED_SERVICES` name, skipping the prompt.
- `src-tauri/src/artifact_installer.rs:568-647` — `stage_install_service` never checks `ALLOWED_SERVICES` (or `is_valid_service_name`); any `kind=service` manifest is written to `extensions/index.json`.
- `src-tauri/src/service_host.rs:260-271` — `resolve_sidecar_path` prefers the URL-installed record over the bundled binary.

## Required outcome
Either reject URL installs whose `bin` is an `ALLOWED_SERVICES` name (forcing them through consent), or reflect in `01-architecture.md` / `05-external-install.md` that an `ALLOWED` name can be replaced by a URL install boundary is bypassable, and restore the prompt for URL-installed binaries.

## Open questions for the human (optional)
- Was the "URL install never grants run permission" wording in `05-external-install.md` meant to cover the `ALLOWED`-name case, or only novel names? The current text implies the former but the code only enforces the latter.

ℹ️ Nitpicks

  • src/platform/installer.ts:89 and :244 still state that a URL-installed bin "vẫn phải nằm trong ALLOWED_SERVICES" / is "khoá theo bin qua ALLOWED_SERVICES". That is no longer true after this PR — the comments should describe the name-format + consent model.

Important

Pullfrog covered this run's model usage. DeepSeek Flash is fast and cheap — expect lighter work than a frontier model. This model allowance is temporary and separate from your Pullfrog plan. Connect a model-provider subscription or API key →

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (model usage covered by Pullfrog) | 𝕏

let bytes = tokio::fs::read(&path)
.await
.map_err(|e| format!("Không đọc được \"{}\": {e}", path.display()))?;
let sha256 = crate::artifact_installer::sha256_hex(&bytes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The consent approval is bound to a SHA-256 computed here, before the prompt, but after the user clicks Allow, get_or_spawn re-resolves sidecar_path and spawns at line 457 with no re-hash. Because the sidecar lives under webview-writable app_data (capabilities/default.json:17,20) and the webview does not block on invoke, it can swap the binary — or index.json's bin_path — while the dialog is open, so the approved bytes are not the spawned bytes. The docs' claim that this is a "rất ngắn" (very short) window is inaccurate: it is however long the modal dialog stays open.

Technical details
# Consent is not bound to the bytes that are spawned

## Affected sites
- `src-tauri/src/service_host.rs:431-441` — `ensure_trusted` resolves the path and hashes the file, then hands control to `trust.ensure`, which blocks on the dialog.
- `src-tauri/src/service_host.rs:450` — `ensure_trusted(...).await?` returns on approval; nothing pins the approved file.
- `src-tauri/src/service_host.rs:457` — `get_or_spawn` calls `sidecar_path(app, bin)` again and spawns whatever is there, without comparing against the trusted digest.
- `docs/plugin-sdk/01-architecture.md:95-97` — states the hash-to-spawn window is "rất ngắn" and that the file is hashed "ngay trước khi spawn"; it is hashed before the prompt instead.

## Required outcome
Spawn exactly the file whose SHA-256 the user approved. A webview that can rewrite `services/**` can currently change the on-disk bytes at any point after approval.

## Suggested approach (optional)
Return the resolved `PathBuf` from `ensure_trusted` and spawn that, rather than re-resolving `sidecar_path` at line 457, then re-read/re-hash that exact path immediately before `Command::new` and abort if it no longer equals the trusted digest. Since the binary sits inside the webview-writable scope, the robust fix is to keep the approved copy somewhere the webview cannot rewrite (or narrow the `fs` scope to exclude `services/` and `extensions/`).

- Title is a question naming the program (devtool-svc- prefix dropped);
  the body is one sentence, three identifying lines — folder, a shortened
  SHA-256, source — and one reminder; buttons say Allow / Don't Allow.
- Follows the language chosen in the app (devtool-locale), English otherwise.
- Paths are shortened with both / and \ separators, home written as ~ on
  macOS/Linux and %USERPROFILE% on Windows; non-ASCII account names cannot
  panic the shortening.

Custom button labels are supported by the dialog backend on all three:
NSAlert (macOS), TaskDialog (Windows), GTK (Linux).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ No new issues found in the reviewed commits. This delta reworks only the consent dialog's presentation and localization; the trust/hash path is untouched. The previously raised digest-binding concern on service_host.rs is unchanged by this commit, so this is not an approval.

Reviewed changes

  • Consent dialog extracted into a value type — consent_message is replaced by ConsentDialog { title, message, allow, deny } built by consent_dialog(...), and ask_user now consumes that struct to drive the native tauri-plugin-dialog prompt.
  • Dialog localized to the app's chosen language — new Locale enum and locale_from_settings, plus ui_locale in service_host.rs, which reads devtool-locale from app_data_dir/app-settings.json (verified this is the file plugin-store writes) and defaults to English. The webview can change the language but not the answer.
  • Display shortened for readability — shorten_home (~ / %USERPROFILE%), short_location (~/…/services/<bin>/<version>), short_hash (12…6 hex chars), and short_source (scheme/query stripped, host + last segment when long). All operate on the display string; trust remains bound to the full SHA-256.
  • Tests updated — the old message test is replaced by exact-string assertions over the dialog text, path shortening for macOS/Linux/Windows, locale parsing, and short-value passthrough (including a non-ASCII home name that must not panic).

Important

Pullfrog covered this run's model usage. DeepSeek Flash is fast and cheap — expect lighter work than a frontier model. This model allowance is temporary and separate from your Pullfrog plan. Connect a model-provider subscription or API key →

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (model usage covered by Pullfrog) | 𝕏

DianaSensei and others added 2 commits October 7, 2026 01:14
undici, sharp, source-map-js, fast-uri and smol-toml move to their patched
versions within the existing semver ranges (no --force, package.json
unchanged). The Security workflow's npm audit already failed on main since
these advisories were published; full vitest suite (1452 tests) and
tsc + vite build pass with the new lockfile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ensure_trusted is now a thin wrapper over ensure_trusted_with, which takes
the directories, home and the way to ask as parameters. New tests cover a
bundled sidecar never asking, asking once then remembering, a denial, a
changed binary asking again, a missing binary, the dialog's language from
app-settings.json, and reading the install source from the index. The
AppHandle-only installed_service_source_url wrapper is gone.

What stays untested is glue that needs the running app or the OS keychain:
showing the native dialog, resolving app directories, the keyring backend.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2026

Copy link
Copy Markdown

@DianaSensei
DianaSensei enabled auto-merge (rebase) October 6, 2026 18:22

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ No new issues found in the reviewed commits. This delta extracts the consent decision behind a testable seam and adds an end-to-end suite for it; the trust/hash path itself is unchanged. The previously raised digest-binding concern on service_host.rs is still open, so this is not an approval.

Reviewed changes

  • ensure_trusted split for testability — the AppHandle-bound function now only gathers app data (dirs, home, locale) and delegates to a new ensure_trusted_with(trust, bin, index_dir, beside_dir, app_data, home, ask), which takes the ask callback as a parameter instead of reading a ServiceRegistry/AppHandle. Production behavior is unchanged: same resolve → read → hash → trust.ensure sequence.
  • Shared directory lookup extracted — sidecar_dirs(app) backs both sidecar_path and ensure_trusted, so the index/beside-exe resolution cannot drift between the two call sites.
  • ui_locale no longer takes an AppHandle — ui_locale_at(Option<&Path>) reads app-settings.json from the passed-in app_data, letting tests drive locale without an app.
  • installed_service_source_url app wrapper removed — installed_service_source_url_at is now the single implementation; ensure_trusted_with passes its already-resolved index_dir rather than re-deriving it.
  • MemoryBackend moved into service_trust.rs behind #[cfg(test)] (pub(crate)) so the service_host tests can reuse it without shipping it in release builds.
  • New end-to-end consent tests — ensure_trusted_with now covers: a bundled name never asks (panics if it does), ask-once-then-remembered with exact dialog title and SHA/Source: assertions, denial returning an error followed by a re-ask after the file's bytes change, a missing sidecar erroring without asking, and a Vietnamese dialog title from app-settings.json. These assert exact values and fail loudly if the ask callback fires when it shouldn't, so they can genuinely catch a regression. Also adds an installed_service_source_url_at test and a tightened bad-name rejection loop (/bin/sh, sh, devtool-svc-../x, devtool-svc-A, …).

Important

Pullfrog covered this run's model usage. DeepSeek Flash is fast and cheap — expect lighter work than a frontier model. This model allowance is temporary and separate from your Pullfrog plan. Connect a model-provider subscription or API key →

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (model usage covered by Pullfrog) | 𝕏

@DianaSensei
DianaSensei merged commit 1efc693 into main Oct 6, 2026
20 checks passed
@DianaSensei
DianaSensei deleted the feat/user-approved-sidecars branch October 6, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant