feat(plugins): support direct download_url in registry entries - #480
Conversation
InstaZDLL
added
scope: backend
📝 WalkthroughWalkthroughLe registre des plugins accepte une URL de téléchargement facultative. L’installation valide cette URL avec le contrôle SSRF partagé. Le téléchargement suit uniquement les redirections validées, avec une limite de dix. L’URL GitHub reste utilisée en l’absence d’URL directe. ChangesTéléchargement des plugins
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegistryEntry
participant PluginInstaller
participant SSRFValidator
participant DownloadServer
RegistryEntry->>PluginInstaller: fournir download_url facultatif
PluginInstaller->>SSRFValidator: valider l’URL
SSRFValidator-->>PluginInstaller: URL sûre ou refus
PluginInstaller->>DownloadServer: télécharger l’archive
DownloadServer-->>PluginInstaller: redirection éventuelle
PluginInstaller->>SSRFValidator: valider chaque cible
SSRFValidator-->>PluginInstaller: cible sûre ou refus
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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/app/src/commands/plugin_store.rs`:
- Around line 494-501: Renforcez la validation de l’URL dans le traitement de
`entry.download_url` : parsez-la, exigez HTTPS, rejetez les hôtes localhost,
loopback, privés et link-local ainsi que les formes d’URL avec identifiants
menant vers une adresse interne, puis validez aussi les adresses DNS résolues.
Dans le client `reqwest` utilisé pour le téléchargement, désactivez les
redirections automatiques ou vérifiez chaque destination avant de la suivre,
notamment les redirections HTTP. Ajoutez des tests couvrant ces URL internes et
les redirections non sûres.
🪄 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: ef24e9b5-7a2c-4d23-8372-78fe6a73e552
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/plugin_store.rs
|
@coderabbitai hardened — pushed. The
Even though the URL is registry-sourced (the trust anchor) and the bytes are blake3-verified, this stops a compromised entry from using the app as an SSRF probe. Skipped the "resolve DNS and reject internal resolutions" part: same as the motion-cache guard, DNS-rebinding is a documented accepted residual ("a hostname that resolves to an internal IP at connect time … is a residual we accept — plugins are curated and user-installed"). Adding per-hop DNS resolution is out of scope and a deliberate existing trade-off. On tests: the validation logic IS Validation: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Changed files ---'
git diff --name-only HEAD^ HEAD || true
printf '%s\n' '--- plugin_store outline ---'
ast-grep outline src-tauri/crates/app/src/commands/plugin_store.rs --items all --view expanded || true
printf '%s\n' '--- URL guard references and installer context ---'
rg -n -C 8 'is_safe_motion_url|redirect::Policy|install_plugin_from_registry|download_url' \
src-tauri/crates/app/src/commands/plugin_store.rs src-tauri/crates || true
printf '%s\n' '--- motion_cache outline ---'
guard_file="$(fd -a '^motion_cache\.rs$' src-tauri/crates | head -n 1)"
printf 'guard file: %s\n' "$guard_file"
if [ -n "$guard_file" ]; then
ast-grep outline "$guard_file" --items all --view expanded || true
printf '%s\n' '--- URL guard implementation and tests ---'
rg -n -C 12 'fn is_safe_motion_url|is_safe_motion_url|#\[test\]' "$guard_file" || true
fiLength of output: 49034
Il reste un défaut mineur dans la limite de redirections. Line 527 utilise if attempt.previous().len() >= 10 {
attempt.error("too many redirects")
}✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/crates/app/src/commands/plugin_store.rs (1)
498-534: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFiltrez les adresses après la résolution DNS.
is_safe_motion_urlaccepte tout nom d’hôte non local qui n’est pas un littéral IP. Un registre compromis peut donc fournir un nom DNS qui résout vers une adresse privée, link-local ou loopback.reqwesteffectue alors une requête interne. Les contrôles des lignes 500 et 529 ont la même limite. Ce cas est une SSRF par résolution DNS stable, et non seulement par DNS rebinding.Résolvez et filtrez les
SocketAddrau moment de la connexion. Ajoutez un test avec un résolveur de test qui retourne une adresse interne.reqwestexpose un résolveur personnalisable ; vérifiez l’API exacte dans la version verrouillée avant l’implémentation. (docs.rs)#!/bin/bash set -euo pipefail printf '%s\n' '--- version verrouillée de reqwest ---' rg -n -C 3 '^name = "reqwest"$|^version = ' Cargo.lock printf '%s\n' '--- validation URL et résolution DNS ---' rg -n -C 10 'is_safe_motion_url|dns_resolver|resolve_to_addrs|lookup_host|ToSocketAddrs' \ src-tauri/crates/core/src/artwork/motion_cache.rs \ src-tauri/crates/app/src/commands/plugin_store.rs🤖 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/app/src/commands/plugin_store.rs` around lines 498 - 534, Update the plugin download client flow around is_safe_motion_url and the reqwest Client builder to resolve every hostname and reject any resulting private, loopback, link-local, or otherwise unsafe SocketAddr before connection, including redirect targets. Use reqwest’s custom Resolve API supported by the locked version, and add a test with a resolver returning an internal address to verify the request is refused.
🤖 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.
Duplicate comments:
In `@src-tauri/crates/app/src/commands/plugin_store.rs`:
- Around line 498-534: Update the plugin download client flow around
is_safe_motion_url and the reqwest Client builder to resolve every hostname and
reject any resulting private, loopback, link-local, or otherwise unsafe
SocketAddr before connection, including redirect targets. Use reqwest’s custom
Resolve API supported by the locked version, and add a test with a resolver
returning an internal address to verify the request is refused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4252ba02-dcc1-492c-ae4c-01b0ee70e332
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/plugin_store.rs
|
@coderabbitai skipping the custom- The download URL comes from the registry — the trust anchor. An attacker who can set This matches the existing, documented stance of the shared guard: What IS in place after this PR: https-only + literal loopback/private/link-local/unspecified rejection (incl. IPv6-mapped + userinfo forms) on the initial URL and re-validated on every redirect hop. That covers the practical SSRF surface for a curated, blake3-verified install path. If we later decide to add resolve-time IP filtering, it should land once in |
|
Tip For best results, initiate chat on the files or code changes.
Le risque résiduel est un GET SSRF aveugle après compromission du registre. Le contenu ne peut pas être installé sans correspondre au hash BLAKE3 épinglé dans le registre. Le garde partagé Le filtrage des IP résolues doit rester une amélioration globale de 🐇 ✏️ Learnings added
You are interacting with an AI system. |
1 participant
Adds an optional
download_urlto a registry entry. When present,install_plugin_from_registrydownloads the release asset from that URL instead of the GitHubreleases/downloadURL built fromrepo.This decouples the store's install path from GitHub, so the app-controlled registry endpoint (
waveflow.app/api/plugins/registry) can host a plugin's binary itself — enabling a closed-source, binary-only plugin with no public source repo (the first consumer being the Spotify Canvas plugin, whose source stays private).What changes
RegistryEntry.download_url: Option<String>(serdedefault, so every existing entry decodes unchanged; older/GitHub-listed plugins simply omit it).install_plugin_from_registry: ifdownload_urlis set, use it (required to behttps— no downgrade / loopback target); otherwise the existinggithub.com/{repo}/releases/download/v{version}/{asset}URL. The downloaded bytes are still blake3-verified against the registry pin either way, so the trust model is unchanged — the registry remains the single source of truth,download_urljust moves where the (still-verified) bytes come from.No schema change to the public
waveflow-pluginsregistry is needed: entries carryingdownload_urlare injected by the app-controlled endpoint, not committed to the GitHub registry (they never pass its CI). Older app builds ignore the field and fall back to the GitHub URL.Validation
cargo check -p waveflow+cargo clippy -p waveflow --all-targets— cleanSummary by CodeRabbit
Nouvelles fonctionnalités
Sécurité