feat(plugin-sdk): bind host imports + permission gates (phase 2b)#222
Open
InstaZDLL wants to merge 8 commits into
Open
feat(plugin-sdk): bind host imports + permission gates (phase 2b)#222InstaZDLL wants to merge 8 commits into
InstaZDLL wants to merge 8 commits into
Conversation
… 2b) Phase 2b lights up the wasm side of the SDK. The wasmtime `bindgen!` macro now consumes `waveflow:source/plugin` (Phase 1's declared world for sources like Web Radio) and the host implements the three `waveflow:host/*` interfaces it imports — HTTP, log, storage — against `HostCtx`. Permission decisions happen at every call against a snapshot taken at instantiate time, so a manifest swapped on disk mid-session can't sneak new permissions in until the host reloads. What lands: - `plugin::bindings::source` — `bindgen!` invocation pointed at `crates/plugin-sdk/wit/source/`. Trappable imports so the Host trait methods can return `wasmtime::Result<...>`. - `plugin::host_impl` — `HostPermissions` (allowlist compiled to a `GlobSet`, storage flags), `StateStore` (FS-backed scratch with the 10 MB quota the WIT promises), and Host impls for http / log / storage. `add_to_linker(linker)` registers every host binding in one call. - `plugin::runtime` — `HostCtx` gains `plugin_id` / `permissions` / `assets` / `state` / `http_client`. `PluginRuntime::new_store_for_plugin` builds the full context from a `LoadedPlugin` + `PluginPaths`; `PluginRuntime::build_linker` pre-registers every host import. Sub-changes pulled along: - `PluginPaths` splits the single `root` field into `plugins_root` + `data_root`. The scratch store lives under `<app-data>/waveflow/plugin-data/<plugin-id>/` so reinstalls don't risk clobbering user state. `state_dir(plugin_id)` shares the same id sanitisation as `plugin_dir`. - WIT layout restructured to per-world subdirs because wit-parser refuses multiple packages in one directory. `host.wit` is duplicated under each `<world>/deps/host/` since cross-platform symlinks are a Windows headache. - `reqwest` gains the `blocking` feature so the sync `http.send` WIT signature is callable without bridging through a tokio block_on inside a wasmtime callback. `globset` added for the HTTP allowlist matcher. 10 new tests cover HTTP allowlist match/deny, glob compile errors, state roundtrip, quota overflow, quota in-place overwrite, nul-key rejection, and the linker build smoke test. 30/30 plugin tests pass; workspace check clean. Phase 3 will wire Tauri commands + the Settings → Plugins UI. Phase 4 ports receiver-core as the first WASM plugin. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
added
scope: backend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplémentation des contrats WIT host (http/log/storage), bindings Wasmtime, implémentations hôtes avec permissions et StateStore persistant, refonte runtime par-plugin (Store/Linker/HostCtx) et activation de reqwest::blocking. ChangesPlugin Host API and Runtime Integration
Sequence Diagram(s)sequenceDiagram
participant Plugin as WASM Plugin
participant HostCtx
participant Permissions as HostPermissions
participant HTTP as reqwest_blocking_Client
participant StateStore
Plugin->>HostCtx: send(request)
HostCtx->>Permissions: http_allowed(url)?
Permissions-->>HostCtx: bool
alt denied
HostCtx-->>Plugin: Err("permission denied")
else allowed
HostCtx->>HTTP: execute(request)
HTTP-->>HostCtx: response
HostCtx-->>Plugin: Ok(response)
end
Plugin->>HostCtx: write_state(key,value)
HostCtx->>StateStore: validate key & check quota
alt invalid or quota exceeded
StateStore-->>HostCtx: Err(String)
HostCtx-->>Plugin: Err(String)
else
StateStore->>StateStore: write SHA-256 file (tmp -> rename)
StateStore-->>HostCtx: Ok(())
HostCtx-->>Plugin: Ok(())
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The shared blocking client was built with reqwest's default redirect policy (follow up to 10 hops). The HTTP allowlist only gates the initial `req.url` — once a request lands at an allowlisted host, that server can 302 to any URL it wants and the client would happily follow, granting transitive access to arbitrary destinations the manifest never authorised. Build the client on `PluginRuntime` (one place, configured at startup) with `redirect(Policy::none())`. The plugin sees 3xx + `Location` in the response and can re-issue through the allowlist if it wants to follow — that way every hop is gated, not just the first. Per-Store HostCtx clones the runtime's client; reqwest's internal Arc pool means clones share one warm TLS / DNS cache. `HostCtx::stub()` matches the production policy so tests don't pass with permissive defaults the production runtime explicitly disables. New `RuntimeError::HttpClient(reqwest::Error)` variant for the (theoretical) builder failure path. 30/30 plugin tests pass. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 247-291: The HTTP host implementation (HostCtx::send) currently
checks permissions but does not heed offline mode; before performing
builder.send() call, call offline::is_offline() and short-circuit to return an
appropriate empty/cached wit_host::http::Response (e.g., zero-length body, empty
headers and a suitable status) instead of issuing the network request; keep the
existing permission check (permissions.http_allowed) and ensure errors still
return Err(String) where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 607a19d8-8be2-4943-a9e7-791eae51e6e7
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock,!src-tauri/Cargo.lock
📒 Files selected for processing (11)
src-tauri/crates/core/Cargo.tomlsrc-tauri/crates/core/src/plugin/bindings.rssrc-tauri/crates/core/src/plugin/host_impl.rssrc-tauri/crates/core/src/plugin/mod.rssrc-tauri/crates/core/src/plugin/runtime.rssrc-tauri/crates/plugin-sdk/wit/metadata/deps/host/host.witsrc-tauri/crates/plugin-sdk/wit/metadata/plugin.witsrc-tauri/crates/plugin-sdk/wit/source/deps/host/host.witsrc-tauri/crates/plugin-sdk/wit/source/plugin.witsrc-tauri/crates/plugin-sdk/wit/ui/deps/host/host.witsrc-tauri/crates/plugin-sdk/wit/ui/plugin.wit
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/core/src/plugin/runtime.rs`:
- Around line 112-125: The shared blocking HTTP client (http_client) is built
with redirect following disabled via Policy::none() but lacks an explicit
timeout; update the client construction to set a reasonable request timeout
(e.g., a few seconds) using reqwest::blocking::Client::builder().timeout(...) so
requests are bounded even if wasmtime's epoch deadline isn't hit, keeping
Policy::none() and the redirect behavior intact; locate the client build code
that sets redirect to Policy::none() and add the timeout configuration there
(affecting the http_client field).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d92bbbe-ed93-45d4-bffe-e618503322a6
📒 Files selected for processing (1)
src-tauri/crates/core/src/plugin/runtime.rs
Two CR findings on Phase 2b's HTTP host import: 1) The shared blocking client had `Policy::none()` for redirects but no request / connect timeout. A misbehaving allowlisted host would hold the call open until wasmtime's epoch deadline (~30 s) trapped the entire guest call — clean error surfacing is better than a trap. Add `timeout(15 s)` + `connect_timeout(5 s)` so a stalled server returns a recoverable `reqwest::Error` the plugin sees as `Err(...)` while the epoch stays the last-resort catch. 2) CLAUDE.md spells "every outbound HTTP path checks `offline::is_offline()` first and short-circuits to an empty payload or cache". Plugin HTTP was missing this hook. The real `offline::is_offline` lives in the app crate so `waveflow-core` can't call it directly (circular dep, and core also feeds `waveflow-server`). Solution: introduce `OfflineProbe = Arc<dyn Fn() -> bool + Send + Sync>` as the public hook + `always_online()` default for tests + core consumers + `PluginRuntime::new_with_offline_probe(config, probe)` constructor for the host to inject its real probe. Phase 3's host wiring will pass the desktop's `offline::is_offline`. The HTTP send impl short-circuits to status=503 + empty headers + empty body when the probe returns true — matches the empty-payload convention from the rest of the workspace, and plugins keep their normal HTTP error path (status + retry-later) instead of needing a separate offline branch. New test `http_send_short_circuits_when_offline` builds a HostCtx with an always-true probe + a permissive allowlist and verifies the response is the synthetic empty 503 without touching the network. 31/31 plugin tests pass; workspace check clean. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/crates/core/src/plugin/host_impl.rs (1)
192-223:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLe quota du
StateStorese contourne sous écritures concurrentes.
write()fait un scan du répertoire puis écrit plus loin. Deux stores du même plugin peuvent donc calculer le mêmeexisting_total, passer tous les deux le check, puis finir au-dessus dequota_bytes. En plus,fs::writeremplace le contenu en place, donc un read concurrent peut observer un fichier tronqué pendant un overwrite. Il faut sérialiser les writes par plugin et écrire via fichier temporaire +rename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/host_impl.rs` around lines 192 - 223, The current write() races: two concurrent writers can both compute existing_total and pass the quota check, and fs::write does in-place overwrites that can expose truncated reads; fix by serializing writes per plugin and using atomic replace via a temp file and rename: acquire an exclusive lock for the plugin directory (e.g., open a dedicated lock file in self.dir and lock it) at the start of write() before scanning for existing_total and keep the lock until after rename completes; recompute the quota under that lock, write the value to a temp file in the same directory (use a unique temp name derived from key_path()), fsync the temp file if possible, then atomically rename the temp file to path (key_path(key)) to replace the old file; keep the existing check_key(), skip-equality logic for size calc, and return the same HostError::QuotaExceeded when projected > quota_bytes, but perform these checks while holding the exclusive lock to prevent races.src-tauri/crates/core/src/plugin/runtime.rs (1)
296-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
new_store_for_pluginaccepte un identifiant incohérent avecloaded.La fonction dérive
permissionset les déclarations d’assets depuisloaded.manifest, maisplugin_dir,state_diretHostCtx.plugin_iddepuis unplugin_idséparé. Si l’appelant se trompe d’id, le store d’un plugin A peut écrire dans le state dir de B et logger sous le mauvais identifiant. Dérive l’id depuisloaded.manifest.plugin.idou rejette explicitement tout mismatch avant de construireHostCtx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/runtime.rs` around lines 296 - 329, The new_store_for_plugin function uses a separate plugin_id for plugin_dir, state_dir and HostCtx.plugin_id while assets and permissions come from loaded.manifest, which can cause mismatches; update new_store_for_plugin to obtain the canonical id from loaded.manifest.plugin.id (or at minimum validate that the provided plugin_id equals loaded.manifest.plugin.id and return an error on mismatch) before computing plugin_dir/state_dir and constructing HostCtx.plugin_id, keeping references to ResourceTable, StateStore, AssetResolver, HostPermissions, http_client and offline_probe as-is.
♻️ Duplicate comments (1)
src-tauri/crates/core/src/plugin/host_impl.rs (1)
252-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLe court-circuit hors-ligne doit passer avant la liste d’autorisation HTTP.
Ici, un appel fait en mode hors-ligne vers une URL non autorisée renverra encore
permission deniedau lieu de la réponse vide503attendue, parce quehttp_allowedest évalué avantoffline_probe. Ça casse le contrat offline global, et le test ajouté ne verrouille pas cette régression puisqu’il n’exerce qu’une URL autorisée.🔧 Correctif minimal
- if !self.permissions.http_allowed(&req.url) { - return Ok(Err(format!("permission denied: {}", req.url))); - } if (self.offline_probe)() { return Ok(Ok(wit_host::http::Response { status: 503, headers: Vec::new(), body: Vec::new(), })); } + if !self.permissions.http_allowed(&req.url) { + return Ok(Err(format!("permission denied: {}", req.url))); + }As per coding guidelines, « Every outbound HTTP path (Deezer, Last.fm, similar, LRCLIB) must check
offline::is_offline()first and short-circuit to an empty payload or cache. »🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/host_impl.rs` around lines 252 - 267, The offline short-circuit is checked after the permission check, causing offline calls to unauthorized URLs to return "permission denied" instead of the expected empty 503; move the offline check (use (self.offline_probe)()) to run before calling self.permissions.http_allowed(&req.url) in the same function in host_impl.rs so that the function returns the empty 503 Response early for all outbound HTTP paths; ensure you preserve the existing returned Response shape (status 503, empty headers/body) and adjust any related tests to cover an offline call to a non-whitelisted URL.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 192-223: The current write() races: two concurrent writers can
both compute existing_total and pass the quota check, and fs::write does
in-place overwrites that can expose truncated reads; fix by serializing writes
per plugin and using atomic replace via a temp file and rename: acquire an
exclusive lock for the plugin directory (e.g., open a dedicated lock file in
self.dir and lock it) at the start of write() before scanning for existing_total
and keep the lock until after rename completes; recompute the quota under that
lock, write the value to a temp file in the same directory (use a unique temp
name derived from key_path()), fsync the temp file if possible, then atomically
rename the temp file to path (key_path(key)) to replace the old file; keep the
existing check_key(), skip-equality logic for size calc, and return the same
HostError::QuotaExceeded when projected > quota_bytes, but perform these checks
while holding the exclusive lock to prevent races.
In `@src-tauri/crates/core/src/plugin/runtime.rs`:
- Around line 296-329: The new_store_for_plugin function uses a separate
plugin_id for plugin_dir, state_dir and HostCtx.plugin_id while assets and
permissions come from loaded.manifest, which can cause mismatches; update
new_store_for_plugin to obtain the canonical id from loaded.manifest.plugin.id
(or at minimum validate that the provided plugin_id equals
loaded.manifest.plugin.id and return an error on mismatch) before computing
plugin_dir/state_dir and constructing HostCtx.plugin_id, keeping references to
ResourceTable, StateStore, AssetResolver, HostPermissions, http_client and
offline_probe as-is.
---
Duplicate comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 252-267: The offline short-circuit is checked after the permission
check, causing offline calls to unauthorized URLs to return "permission denied"
instead of the expected empty 503; move the offline check (use
(self.offline_probe)()) to run before calling
self.permissions.http_allowed(&req.url) in the same function in host_impl.rs so
that the function returns the empty 503 Response early for all outbound HTTP
paths; ensure you preserve the existing returned Response shape (status 503,
empty headers/body) and adjust any related tests to cover an offline call to a
non-whitelisted URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 59b68c2a-ea0d-4a89-a378-7a5f1efe2995
📒 Files selected for processing (2)
src-tauri/crates/core/src/plugin/host_impl.rssrc-tauri/crates/core/src/plugin/runtime.rs
Three CR findings on Phase 2b: 1) Offline short-circuit was placed AFTER the HTTP allowlist check, so a guest probing a non-allowlisted URL while the host is offline saw "permission denied" instead of the empty 503 every other call gets. Worse, it let a plugin probe the allowlist state while offline — a small information leak. Swap order so the offline branch always wins. 2) `new_store_for_plugin` took a separate `plugin_id: &str` param alongside `loaded: &LoadedPlugin`, letting the install-dir id drift from `loaded.manifest.plugin.id` (assets would resolve against one tree while permissions + log scope referenced another). Drop the param; the manifest id is the single canonical source — already validated to `[a-z0-9-]+` by the parser and re-checked by `PluginPaths::sanitise_id`. 3) `StateStore::write` raced on two fronts: two concurrent writers could both observe the pre-write total + both pass the quota check, leaving the store over quota; and `fs::write` is not atomic, so a reader could see a truncated value mid-write. Hold an OS-level exclusive lock (`File::lock`, stable since Rust 1.89) on `.write-lock` for the duration of the quota check + the rename, write the value to a sibling `<hash>.tmp` first, then `fs::rename` onto the canonical path (atomic on both POSIX `rename` and Windows `MoveFileEx REPLACE_EXISTING` when source + dest live in the same dir on the same filesystem). The lock file is excluded from the quota tally; a shared `sum_state_dir_bytes` helper backs both `write`'s pre-flight check and the test-only `used_bytes`. 4 new tests: - `http_send_offline_supersedes_permission_denial` — offline wins over the allowlist branch. - `state_store_lock_file_excluded_from_quota` — sentinel doesn't count against the cap. - `state_store_serialises_concurrent_writes` — 2 threads writing the same key 10× each; final value is one pure side (no torn write), final usage is exactly one record (lock-serialised quota accounting). 34/34 plugin tests pass; workspace check clean. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 238-252: The temp write path (tmp_path created with
with_extension("tmp") in the write logic) can be left behind after a crash and
is currently counted by sum_state_dir_bytes(), so update sum_state_dir_bytes()
to ignore files with the ".tmp" extension (or match the same tmp naming rule
used when creating tmp_path) when computing quota; additionally, in the write
code paths that create tmp_path (the block using fs::write(&tmp_path, value) and
fs::rename(&tmp_path, &path) and the similar block around lines 274-293), ensure
tmp files are either cleaned up on startup or documented/consistent with the
ignore rule so orphaned "<hash>.tmp" files do not inflate the counted state
size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b2fc002-a350-492e-88db-44157e17d74c
📒 Files selected for processing (2)
src-tauri/crates/core/src/plugin/host_impl.rssrc-tauri/crates/core/src/plugin/runtime.rs
A host crash between `fs::write(tmp_path)` and `fs::rename(tmp_path, path)` in `StateStore::write` leaves an orphaned `<hash>.tmp` next to the canonical file. The next successful write of that key overwrites the same tmp path, so the on-disk surface is bounded — but `sum_state_dir_bytes` was counting orphans toward the quota. Over the lifetime of a long- lived install, that's a real footgun: every interrupted write the store ever survived eats into the user's available space until the plugin can no longer make new writes at all. Skip `.tmp` siblings in the quota tally. The store layout is self-healing — orphans get overwritten on the next write of the same key, so we don't need a startup sweep; we only need to stop counting them. New test `state_store_ignores_orphan_tmp_files` synthesises an orphan via direct `fs::write`, asserts `used_bytes()` reports 0, and confirms a quota-saturating write still succeeds. 35/35 plugin tests pass. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/crates/core/src/plugin/host_impl.rs (1)
314-330:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPréservez le contrat WIT pour les URL refusées.
http.senddoit renvoyerErr("permission denied: <url>")pour une URL hors allowlist, sans dépendre de l’état offline. Avec l’ordre actuel, la même requête renvoie un503synthétique dès que le host est hors-ligne, ce qui casse le contrat observable côté guest. Inversez l’ordre : vérifiez d’abordhttp_allowed, puis court-circuitez en503uniquement pour les URL déjà autorisées avantbuilder.send().Correctif minimal
- if (self.offline_probe)() { - return Ok(Ok(wit_host::http::Response { - status: 503, - headers: Vec::new(), - body: Vec::new(), - })); - } if !self.permissions.http_allowed(&req.url) { return Ok(Err(format!("permission denied: {}", req.url))); } + if (self.offline_probe)() { + return Ok(Ok(wit_host::http::Response { + status: 503, + headers: Vec::new(), + body: Vec::new(), + })); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/host_impl.rs` around lines 314 - 330, The WIT contract requires http.send to return Err("permission denied: <url>") for non-allowlisted URLs regardless of offline state; currently self.offline_probe is checked before permission which turns denied URLs into 503s when offline. Move the permission check (self.permissions.http_allowed(&req.url)) ahead of the offline short-circuit, so http_allowed is evaluated first and returns Err(...) for disallowed URLs, and only for allowed URLs perform the self.offline_probe() check to return the synthetic 503 before calling builder.send()/the outbound path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 314-330: The WIT contract requires http.send to return
Err("permission denied: <url>") for non-allowlisted URLs regardless of offline
state; currently self.offline_probe is checked before permission which turns
denied URLs into 503s when offline. Move the permission check
(self.permissions.http_allowed(&req.url)) ahead of the offline short-circuit, so
http_allowed is evaluated first and returns Err(...) for disallowed URLs, and
only for allowed URLs perform the self.offline_probe() check to return the
synthetic 503 before calling builder.send()/the outbound path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76d896d8-67da-4bd3-b79d-f420e376aa57
📒 Files selected for processing (1)
src-tauri/crates/core/src/plugin/host_impl.rs
Two findings flipped earlier decisions on Phase 2b:
1) The previous CR pass moved the offline short-circuit AHEAD of
the HTTP allowlist check. That broke the WIT contract spelled in
`host.wit`: "the host validates `url` against the plugin's
`permissions.http` allowlist BEFORE dispatching. A url that's
not on the list returns `Err(\"permission denied: <url>\")`".
The permission gate is a protocol invariant the guest is
allowed to rely on; offline mode is a host-state short-circuit
that runs after it. The information-leak argument that
motivated the swap doesn't hold — the guest ships its own
manifest, so it always knows its own allowlist.
Restore: permission denial first, offline short-circuit second.
Test `http_send_offline_supersedes_permission_denial` renamed +
inverted to `http_send_permission_denial_wins_over_offline`
(offline + non-allowlisted now correctly returns
`Err("permission denied: ...")`). The
`http_send_short_circuits_when_offline` test still covers the
offline-with-permission path.
2) `new_store_for_plugin` now sources the plugin id from
`loaded.manifest.plugin.id`, but `load_plugin(paths, plugin_id)`
still reads the wasm + manifest from `plugins/<plugin_id>/`
without verifying that the manifest's declared id matches.
Without a load-time pin, a plugin installed under `plugins/foo/`
declaring `id = "bar"` would later read assets from
`plugins/bar/` and write state into `plugin-data/bar/` once the
store keyed off the manifest id — the install dir effectively
forgers a different plugin's resources.
New `RuntimeError::PluginIdMismatch { expected, found }` variant
+ a check in `load_plugin` right after the manifest parse.
New test `load_plugin_rejects_id_mismatch_between_dir_and_manifest`
lays down `plugins/foo/manifest.toml` with `id = "bar"` and
confirms the load fails before the wasm is even read.
36/36 plugin tests pass.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/crates/core/src/plugin/host_impl.rs (1)
351-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBornez la taille du body HTTP côté hôte.
resp.bytes()bufferise toute la réponse en mémoire hôte avant le retour WIT. Un plugin autorisé peut donc forcer une allocation arbitrairement grosse, ce qui contournemax_memory_bytesdu store et peut faire OOM le process. Lisez le flux via untake()borné et retournezErr(...)au-delà d’un plafond explicite.Correctif minimal
+const MAX_HTTP_RESPONSE_BYTES: usize = 8 * 1024 * 1024; + - let body = match resp.bytes() { - Ok(b) => b.to_vec(), - Err(e) => return Ok(Err(e.to_string())), - }; + let mut body = Vec::new(); + if let Err(e) = resp + .take((MAX_HTTP_RESPONSE_BYTES as u64) + 1) + .read_to_end(&mut body) + { + return Ok(Err(e.to_string())); + } + if body.len() > MAX_HTTP_RESPONSE_BYTES { + return Ok(Err(format!( + "http response too large: {} bytes (cap {})", + body.len(), + MAX_HTTP_RESPONSE_BYTES + ))); + }Ajoutez aussi
use std::io::Read;aux imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/host_impl.rs` around lines 351 - 369, The code currently calls resp.bytes() which buffers the entire response into memory; change this to stream-read with a hard cap: import std::io::Read, wrap the response body reader with resp.take(MAX_BODY_BYTES) (choose a sensible MAX_BODY_BYTES constant), read into a Vec by repeatedly reading until EOF, and if a subsequent read returns 0 bytes because the take limit was reached (or detect that total_bytes_read == MAX_BODY_BYTES and further data exists) return Ok(Err("response body too large".to_string())); keep the existing handling of status and headers but replace usage of resp.bytes() with the bounded read from the resp reader; ensure errors from reading are converted to Err(e.to_string()) as before.src-tauri/crates/core/src/plugin/runtime.rs (1)
393-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFermez
HostCtxaux mutations externes.Avec des champs
pub, n’importe quel appelant qui récupère unStore<HostCtx>peut réécrirepermissions,plugin_id,assetsouoffline_probeaprès l’instanciation. Ça casse précisément les invariants que ce runtime vient de pinner au chargement (PluginIdMismatch, snapshot des permissions). Passez ces champs enpub(crate)au minimum.Correctif minimal
pub struct HostCtx { - pub table: ResourceTable, - pub limits: StoreLimits, - pub plugin_id: String, - pub permissions: HostPermissions, - pub assets: Option<AssetResolver>, - pub state: StateStore, - pub http_client: reqwest::blocking::Client, - pub offline_probe: OfflineProbe, + pub(crate) table: ResourceTable, + pub(crate) limits: StoreLimits, + pub(crate) plugin_id: String, + pub(crate) permissions: HostPermissions, + pub(crate) assets: Option<AssetResolver>, + pub(crate) state: StateStore, + pub(crate) http_client: reqwest::blocking::Client, + pub(crate) offline_probe: OfflineProbe, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/runtime.rs` around lines 393 - 429, Les champs exposés de HostCtx permettent des mutations externes et cassent les invariants; changez au minimum la visibilité de plugin_id, permissions, assets et offline_probe de pub à pub(crate) dans la définition de struct HostCtx pour empêcher des réécritures hors du crate tout en gardant l'accès interne nécessaire (conserver les autres champs tels quels). Assurez-vous de mettre à jour toute référence externe si elle compile différemment après ce changement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 351-369: The code currently calls resp.bytes() which buffers the
entire response into memory; change this to stream-read with a hard cap: import
std::io::Read, wrap the response body reader with resp.take(MAX_BODY_BYTES)
(choose a sensible MAX_BODY_BYTES constant), read into a Vec by repeatedly
reading until EOF, and if a subsequent read returns 0 bytes because the take
limit was reached (or detect that total_bytes_read == MAX_BODY_BYTES and further
data exists) return Ok(Err("response body too large".to_string())); keep the
existing handling of status and headers but replace usage of resp.bytes() with
the bounded read from the resp reader; ensure errors from reading are converted
to Err(e.to_string()) as before.
In `@src-tauri/crates/core/src/plugin/runtime.rs`:
- Around line 393-429: Les champs exposés de HostCtx permettent des mutations
externes et cassent les invariants; changez au minimum la visibilité de
plugin_id, permissions, assets et offline_probe de pub à pub(crate) dans la
définition de struct HostCtx pour empêcher des réécritures hors du crate tout en
gardant l'accès interne nécessaire (conserver les autres champs tels quels).
Assurez-vous de mettre à jour toute référence externe si elle compile
différemment après ce changement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: deb5915f-bbfb-41ef-913e-e6cbd465afaa
📒 Files selected for processing (2)
src-tauri/crates/core/src/plugin/host_impl.rssrc-tauri/crates/core/src/plugin/runtime.rs
Two CR findings on Phase 2b's wasm-facing surface:
1) `wit_host::http::Host::send` was calling `resp.bytes()` which
pulls the whole response into a `Vec<u8>` in one go. A
misbehaving (or malicious) allowlisted server could stream
multi-GB into our buffer before wasmtime's epoch deadline ever
trapped the call — we'd have allocated the worst-case payload
first. Add `MAX_BODY_BYTES = 10 MiB` (3× margin over the
biggest realistic JSON catalogue we know about — radio-browser
station lists) and stream-read via `Read::take(MAX + 1)`
so we never hold more than `MAX + 1` bytes; a saturating count
surfaces as a clean `Err("response body too large")` the plugin
can pivot on. Same pattern Phase 2a uses for `plugin.wasm`.
2) `HostCtx` exposed `plugin_id` / `permissions` / `assets` /
`offline_probe` as `pub`. External crates (app, server)
instantiate `HostCtx` via `PluginRuntime::new_store_for_plugin`
but should never be able to rewrite those fields post-hoc —
the permission snapshot pinned at instantiate time is a runtime
invariant the WIT contract relies on, and a caller flipping
`offline_probe` to `|| false` could bypass the host's offline
switch. Narrow visibility to `pub(crate)`. `table`, `limits`,
`state`, `http_client` stay `pub`: wasmtime + the host imports
legitimately reach into them and there's no equivalent
invariant to protect.
36/36 plugin tests pass; workspace check clean.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/crates/core/src/plugin/host_impl.rs (1)
327-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFaites passer le court-circuit offline avant le contrôle d’allowlist.
Ici, un runtime offline renvoie encore
permission deniedpour une URL non autorisée, alors que la règle repo impose qu’un chemin HTTP sortant coupe d’abord sur le mode hors-ligne avec une payload vide/cache. Le testhttp_send_permission_denial_wins_over_offlineverrouille aussi l’ordre inverse, donc il faudra le réaligner en même temps.🔧 Correctif minimal
- if !self.permissions.http_allowed(&req.url) { - return Ok(Err(format!("permission denied: {}", req.url))); - } - // Offline mode short-circuit runs AFTER the allowlist gate - // but BEFORE the network dispatch. Returns an empty 503 so + // Offline mode short-circuit runs BEFORE any other outbound + // HTTP handling. Returns an empty 503 so // plugins can keep their normal HTTP error path (status + // retry-later) instead of needing a separate offline // branch — matches the empty-payload convention CLAUDE.md // spells for every other outbound HTTP path (Deezer, // Last.fm, LRCLIB). if (self.offline_probe)() { return Ok(Ok(wit_host::http::Response { status: 503, headers: Vec::new(), body: Vec::new(), })); } + if !self.permissions.http_allowed(&req.url) { + return Ok(Err(format!("permission denied: {}", req.url))); + }As per coding guidelines, “Every outbound HTTP path (Deezer, Last.fm, similar, LRCLIB) must check
offline::is_offline()first and short-circuit to an empty payload or cache. Process-wide offline mode is persisted inapp_setting['network.offline_mode']”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/host_impl.rs` around lines 327 - 352, Move the offline short-circuit to run before the allowlist check: call the closure self.offline_probe() (or use offline::is_offline() per guideline) first and if it returns true immediately return the empty 503 wit_host::http::Response (status: 503, empty headers/body) so outbound HTTP paths short-circuit on offline mode before any permission checks; then perform self.permissions.http_allowed(&req.url) and return the existing permission-denied Err if not allowed. Also update the test http_send_permission_denial_wins_over_offline to expect the allowlist denial only after you've realigned the check order (or adjust it to assert offline wins if keeping the new behavior).Source: Coding guidelines
src-tauri/crates/core/src/plugin/runtime.rs (1)
423-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNe laissez pas
stateethttp_clientmutables hors crate.
Store<HostCtx>est retourné aux crates appelantes. Avec ces deux champs enpub, un appelant externe peut encore remplacer le backend d’état ou injecter un clientreqwestavec une politique de redirection/timeout différente après l’instanciation, ce qui casse l’isolation et les garde-fous gelés au load-time.host_impl.rsn’a besoin ici que d’un accès crate-local.🔒 Correctif minimal
- pub state: StateStore, + pub(crate) state: StateStore, @@ - pub http_client: reqwest::blocking::Client, + pub(crate) http_client: reqwest::blocking::Client,Le résumé du PR annonce explicitement un verrouillage des invariants de
HostCtx; ces deux champs restent encore réécrivables depuis l’extérieur du crate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/plugin/runtime.rs` around lines 423 - 437, The fields state and http_client on HostCtx are currently public and can be mutated by external crates through the returned Store<HostCtx>, breaking the intended invariants; change their visibility from pub to pub(crate) so only crate-local code (e.g. host_impl.rs) can access/replace the StateStore and reqwest::blocking::Client, and update any internal uses that rely on the new visibility accordingly (look for the HostCtx struct and references to state and http_client in host_impl.rs and callers returning Store<HostCtx>).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/crates/core/src/plugin/host_impl.rs`:
- Around line 327-352: Move the offline short-circuit to run before the
allowlist check: call the closure self.offline_probe() (or use
offline::is_offline() per guideline) first and if it returns true immediately
return the empty 503 wit_host::http::Response (status: 503, empty headers/body)
so outbound HTTP paths short-circuit on offline mode before any permission
checks; then perform self.permissions.http_allowed(&req.url) and return the
existing permission-denied Err if not allowed. Also update the test
http_send_permission_denial_wins_over_offline to expect the allowlist denial
only after you've realigned the check order (or adjust it to assert offline wins
if keeping the new behavior).
In `@src-tauri/crates/core/src/plugin/runtime.rs`:
- Around line 423-437: The fields state and http_client on HostCtx are currently
public and can be mutated by external crates through the returned
Store<HostCtx>, breaking the intended invariants; change their visibility from
pub to pub(crate) so only crate-local code (e.g. host_impl.rs) can
access/replace the StateStore and reqwest::blocking::Client, and update any
internal uses that rely on the new visibility accordingly (look for the HostCtx
struct and references to state and http_client in host_impl.rs and callers
returning Store<HostCtx>).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ee0320d-b67f-4354-8684-008093b92673
📒 Files selected for processing (2)
src-tauri/crates/core/src/plugin/host_impl.rssrc-tauri/crates/core/src/plugin/runtime.rs
Two CR findings — both partially walking back earlier decisions:
1) HTTP ordering: offline short-circuit FIRST, then allowlist. The
previous round flipped to allowlist-first citing the WIT
contract; that was an over-read. The WIT spec pins permission
ahead of the network DISPATCH ("validates url against the
allowlist BEFORE dispatching") — it doesn't say anything about
the order of host-state short-circuits like offline mode. Both
gates still fire before any `builder.send()`, so the WIT
invariant is honoured either way.
CLAUDE.md is the deciding rule: "every outbound HTTP path
(Deezer, Last.fm, similar, LRCLIB) checks `offline::is_offline()`
first and short-circuits to an empty payload or cache. Treat new
HTTP code paths the same way." Plugin HTTP is a new HTTP code
path, so it matches the rest of the workspace.
Restored: offline → 503 → permission → "permission denied" →
dispatch. Test renamed back to
`http_send_offline_supersedes_permission_denial`. An explicit
ORDERING INVARIANT comment in `Host::send` flags the contract +
the deciding rule + the lookback reasoning so future review
rounds don't yo-yo on this again.
2) `HostCtx::state` and `http_client` narrowed from `pub` to
`pub(crate)`. The previous round locked
plugin_id/permissions/assets/offline_probe but reasoned state +
http_client were "legitimately reached into" — true for the host
imports, but those live in the same crate. An external caller
holding `Store<HostCtx>` could still swap `state` to point at
another plugin's scratch (or anywhere on disk), or replace
`http_client` with one that follows redirects + drops timeouts,
undoing the runtime's HTTP policy. Lock both. Only `table` +
`limits` stay `pub` — wasmtime itself reaches into them and
there's no equivalent invariant to protect.
36/36 plugin tests pass; workspace check clean.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2b of RFC-002. Lights up the wasm side of the SDK: the wasmtime
bindgen!macro now consumes thewaveflow:source/pluginworld, and the host implements the threewaveflow:host/*interfaces it imports — HTTP, log, storage — againstHostCtx. Permission decisions happen on every call against a snapshot taken at instantiate time, so a manifest swapped on disk mid-session can't sneak new permissions in until the host reloads.What lands
plugin::bindings::source—bindgen!invocation pointed atcrates/plugin-sdk/wit/source/. Trappable imports so Host methods can return `wasmtime::Result<...>`.plugin::host_impl:plugin::runtime:Sub-changes pulled along
Permission contract
Test plan
What's intentionally NOT here
Refs RFC-002 §6. Builds on #221.
Summary by CodeRabbit
Nouvelles Fonctionnalités
Refactor
Tests