feat(plugin): implement built-in HTTP module (task 15)#14
Conversation
Add a native Rust HTTP/HTTPS module that handles direct downloads when no WASM plugin claims the URL. Integrated as fallback in ExtismPluginLoader::resolve_url(). - Add LinkStatus domain model (Online/Offline/Unknown) - Implement HttpModule with check_link, resolve_download_url - Extract filename from Content-Disposition (RFC 5987) or URL path - SSRF protection: validate URLs against internal networks, custom redirect policy blocks redirect-to-internal attacks - Sanitize filenames against path traversal (../, null bytes) - Redact credentials in error messages - 315 tests pass, clippy clean
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a built-in Changes
Sequence DiagramsequenceDiagram
actor Client
participant Loader as ExtismPluginLoader
participant Registry as RegistryPlugins
participant HTTP as HttpModule
participant Remote as ExternalURL
Client->>Loader: resolve_url(url)
Loader->>Registry: iterate enabled plugins
alt plugin matched
Registry-->>Loader: PluginInfo
Loader-->>Client: return PluginInfo
else none matched
Loader->>HTTP: can_handle(url)?
alt http/https
HTTP-->>Loader: true
Loader->>HTTP: check_link(url) / resolve_download_url(url)
HTTP->>Remote: HEAD request(s) (SSRF checks, manual redirects)
Remote-->>HTTP: responses (status, headers, Location)
HTTP->>HTTP: validate hops, extract filename/size/resumable
HTTP-->>Loader: LinkStatus or resolved URL
Loader-->>Client: return result
else unsupported
HTTP-->>Loader: false
Loader-->>Client: return None
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a built-in HTTP/HTTPS module as a fallback URL resolver in the plugin system, including SSRF protection, filename extraction with RFC 5987 support, and credential redaction. The implementation is thorough and well-tested, with two issues in the SSRF validation layer worth addressing before merge:
Confidence Score: 3/5Hold for the IPv6 link-local SSRF gap and blocking DNS call before merging into a security-sensitive code path. Two P1 findings remain: the blocking to_socket_addrs() call can stall the async runtime under load, and the missing fe80::/10 coverage leaves a real SSRF bypass on dual-stack hosts. Both are straightforward to fix and the rest of the implementation is solid. src-tauri/src/adapters/driven/plugin/builtin/http_module.rs — specifically validate_not_internal (DNS blocking and IPv6 link-local coverage)
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driven/plugin/builtin/http_module.rs | New HttpModule with SSRF protection, filename extraction, and HEAD-based link checking; two issues: blocking DNS in async context (P1) and IPv6 link-local not blocked (P1 security) |
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Constructor made failable to propagate HttpModule::new() errors; HTTP fallback wired into resolve_url(); tests updated cleanly |
| src-tauri/src/domain/model/link.rs | New LinkStatus enum (Online/Offline/Unknown) with well-structured fields; derives are correct; good unit coverage |
| src-tauri/src/adapters/driven/plugin/watcher.rs | make_loader updated to unwrap failable ExtismPluginLoader::new(); no logic changes |
| src-tauri/src/lib.rs | HttpModule re-exported as part of the public API; straightforward addition |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolve_url called] --> B{WASM plugin\nclaims URL?}
B -- yes --> C[Return plugin PluginInfo]
B -- no --> D{HttpModule\ncan_handle?}
D -- no --> E[Return None]
D -- yes --> F[Return builtin-http PluginInfo]
G[check_link / resolve_download_url] --> H[send_head]
H --> I{ssrf_protection?}
I -- yes --> J[validate_not_internal\npre-request check]
J -- blocked --> K[Err ValidationError]
J -- ok --> L[client.head with\ncustom redirect policy]
I -- no --> L
L --> M{redirect?}
M -- yes --> N[redirect policy:\nvalidate_not_internal\nper hop]
N -- blocked --> K
N -- ok --> M
M -- no --> O[Response]
O --> P{status code}
P -- 2xx --> Q[LinkStatus Online]
P -- 404/410 --> R[LinkStatus Offline]
P -- other --> S[LinkStatus Unknown]
Reviews (1): Last reviewed commit: "feat(plugin): implement built-in HTTP mo..." | Re-trigger Greptile
| let port = url.port_or_known_default().unwrap_or(80); | ||
| if let Ok(addrs) = (host, port).to_socket_addrs() { | ||
| for addr in addrs { | ||
| if is_forbidden_ip(&addr.ip()) { | ||
| return Err(DomainError::ValidationError( | ||
| "requests to internal networks are forbidden".to_string(), | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocking DNS call on the async runtime thread
to_socket_addrs() is synchronous and blocking. Called from async fn send_head, it occupies a Tokio worker thread for the full duration of the DNS lookup (potentially seconds on a slow resolver). Under concurrent URL checks in a download manager, this can exhaust the thread pool and stall unrelated tasks.
Switch to async DNS resolution — tokio::net::lookup_host is a drop-in replacement:
// validate_not_internal must become async
let addr_str = format!("{host}:{port}");
if let Ok(addrs) = tokio::net::lookup_host(addr_str).await {
for addr in addrs {
if is_forbidden_ip(&addr.ip()) {
return Err(DomainError::ValidationError(
"requests to internal networks are forbidden".to_string(),
));
}
}
}This requires making validate_not_internal async and awaiting it inside send_head.
| IpAddr::V6(v6) => { | ||
| if let Some(v4) = v6.to_ipv4_mapped() { | ||
| return is_private(&IpAddr::V4(v4)); | ||
| } | ||
| (v6.segments()[0] & 0xfe00) == 0xfc00 // fc00::/7 | ||
| } | ||
| } |
There was a problem hiding this comment.
IPv6 link-local addresses (
fe80::/10) not blocked
The IPv6 branch of is_private only rejects fc00::/7 (unique local) and IPv4-mapped ranges. IPv6 link-local addresses (fe80::/10) are neither loopback (is_loopback() returns false for fe80::1) nor matched by the current mask check (0xfe80 & 0xfe00 == 0xfe00 ≠ 0xfc00). A URL like http://[fe80::1]/ would pass SSRF validation on a dual-stack host.
IpAddr::V6(v6) => {
if let Some(v4) = v6.to_ipv4_mapped() {
return is_private(&IpAddr::V4(v4));
}
let seg0 = v6.segments()[0];
(seg0 & 0xfe00) == 0xfc00 // fc00::/7 unique local
|| (seg0 & 0xffc0) == 0xfe80 // fe80::/10 link-local
}| let path = url.split('?').next().unwrap_or(url); | ||
| path.rsplit('/') | ||
| .next() | ||
| .filter(|s| !s.is_empty()) | ||
| .map(|s| s.to_string()) | ||
| } |
There was a problem hiding this comment.
Bare-domain URL leaks the hostname as a filename
When no path segment exists (e.g., https://example.com without a trailing slash), rsplit('/') yields "example.com" as the last element, which is non-empty and passes the filter. The function then returns Some("example.com") instead of None. The existing test only covers the trailing-slash form (https://example.com/).
A simpler fix: require at least one / in the path after the authority before taking the last segment:
let path = url.split('?').next().unwrap_or(url);
let after_scheme = path.find("://").map(|p| p + 3).unwrap_or(0);
if !path[after_scheme..].contains('/') {
return None;
}
path.rsplit('/')
.next()
.filter(|s| !s.is_empty())
.map(|s| s.to_string())There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src-tauri/src/domain/model/mod.rs (1)
7-7: Re-exportLinkStatusfrom the model barrel.Every other domain model added here is also forwarded via
pub use, soLinkStatuscurrently becomes a special-case import. Re-exporting it keeps the public model API consistent.Suggested change
pub use download::{Download, DownloadId, DownloadState, FileSize, Speed, Url}; pub use http::HttpResponse; +pub use link::LinkStatus; pub use meta::{DownloadMeta, SegmentMeta};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/model/mod.rs` at line 7, Add a pub re-export for LinkStatus in the model barrel: update the module file that currently declares pub mod link; to also publicly re-export LinkStatus (e.g., pub use link::LinkStatus;) so consumers can import LinkStatus from the top-level model module like the other domain types; modify the file containing pub mod link to include the pub use link::LinkStatus re-export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/builtin/http_module.rs`:
- Around line 263-266: The current extract_filename implementation returns
Some("") when sanitize_filename strips all characters (e.g., filename="../"), so
change extract_filename (and the similar logic at the other occurrence around
lines 289-295) to return None if sanitize_filename(&raw) yields an empty string;
specifically, after calling extract_raw_filename(headers, url)? and computing
let sanitized = sanitize_filename(&raw), check if sanitized.is_empty() and
return None in that case, otherwise return Some(sanitized).
- Around line 163-175: The current DNS/IP check silently ignores errors from
(host, port).to_socket_addrs(), allowing requests to proceed; change this to
fail closed by returning Err(DomainError::ValidationError(...)) when
to_socket_addrs() returns Err. Specifically, in the block that now uses `if let
Ok(addrs) = (host, port).to_socket_addrs() { ... }`, replace the fallthrough
with explicit error handling: on Err(e) return
Err(DomainError::ValidationError(format!("failed to resolve host for SSRF
protection: {}", e))) (or match the existing error wording used in
host_functions.rs), and keep the existing loop using is_forbidden_ip(&addr.ip())
unchanged so any forbidden IP still returns the same ValidationError.
- Around line 178-207: The IPv6 link-local range fe80::/10 is not being blocked;
update the is_private(ip: &IpAddr) function's IPv6 branch to treat IPv6
link-local addresses as private by checking v6.is_unicast_link_local() (after
handling to_ipv4_mapped()), so that is_private returns true for fe80::/10 as
well as the existing fc00::/7 check and mapped-IPv4 handling used by
normalize_ip and is_forbidden_ip.
---
Nitpick comments:
In `@src-tauri/src/domain/model/mod.rs`:
- Line 7: Add a pub re-export for LinkStatus in the model barrel: update the
module file that currently declares pub mod link; to also publicly re-export
LinkStatus (e.g., pub use link::LinkStatus;) so consumers can import LinkStatus
from the top-level model module like the other domain types; modify the file
containing pub mod link to include the pub use link::LinkStatus re-export.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d81142c-2925-48e9-b17a-f801ca0859e3
📒 Files selected for processing (8)
src-tauri/src/adapters/driven/plugin/builtin/http_module.rssrc-tauri/src/adapters/driven/plugin/builtin/mod.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/mod.rssrc-tauri/src/adapters/driven/plugin/watcher.rssrc-tauri/src/domain/model/link.rssrc-tauri/src/domain/model/mod.rssrc-tauri/src/lib.rs
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs:165">
P2: Avoid synchronous `to_socket_addrs()` inside the async request path; blocking DNS resolution here can stall Tokio worker threads under concurrent link checks.</violation>
<violation number="2" location="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs:206">
P1: SSRF filtering misses IPv6 link-local addresses (`fe80::/10`), allowing requests to internal interfaces.</violation>
<violation number="3" location="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs:265">
P2: Return `None` when filename sanitization yields an empty string; `Some("")` can propagate an invalid filename.</violation>
<violation number="4" location="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs:282">
P3: URL-path fallback should read from the parsed URL path, not by splitting the full URL string; otherwise bare-domain URLs like `https://example.com` are misidentified as a filename.</violation>
</file>
<file name="src-tauri/src/adapters/driven/plugin/extism_loader.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/extism_loader.rs:239">
P3: This test duplicates an existing magnet-scheme test in the same file. Change it to cover a different unsupported scheme (e.g., FTP) so it adds unique coverage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Switch to async DNS resolution (tokio::net::lookup_host) to avoid blocking tokio worker threads during SSRF validation - Fail closed on DNS resolution errors instead of silently allowing - Block IPv6 link-local addresses (fe80::/10) in SSRF filter - Handle URL host_str() returning bracketed IPv6 (e.g. "[fe80::1]") - Fix bare-domain URL (https://example.com) leaking hostname as filename - Return None when filename sanitization yields empty string - Re-export LinkStatus from domain model barrel - Replace duplicate magnet test with FTP scheme coverage
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/adapters/driven/plugin/extism_loader.rs (1)
114-125:⚠️ Potential issue | 🟠 MajorDon't route to
builtin-httpaftercan_handlefailures.A site-specific WASM plugin that errors in
can_handlenow gets silently bypassed for everyhttp(s)URL, because this code falls through to the built-in handler. That changes a plugin failure into a successful but wrong handler selection.🛠️ Minimal safe guard
fn resolve_url(&self, url: &str) -> Result<Option<PluginInfo>, DomainError> { + let mut had_can_handle_error = false; let mut infos: Vec<_> = self .registry .list_info() .into_iter() .filter(|i| i.is_enabled()) @@ for info in infos { let name = info.name().to_string(); match self.registry.call_plugin(&name, "can_handle", url) { Ok(result) if result.trim() == "true" => return Ok(Some(info)), Ok(_) => {} Err(e) => { + had_can_handle_error = true; tracing::warn!("plugin '{name}' failed can_handle call: {e}"); } } } - if HttpModule::can_handle(url) { + if !had_can_handle_error && HttpModule::can_handle(url) { return Ok(Some(HttpModule::plugin_info())); } Ok(None) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs` around lines 114 - 125, When a plugin's can_handle call fails you must not fall back to the builtin HTTP handler; instead propagate the plugin error so a failed plugin doesn't silently handle requests. In the match on self.registry.call_plugin(&name, "can_handle", url) (the branch that currently does Err(e) => tracing::warn!(...)), change that branch to return Err(e) (or wrap and return a suitable error type) so the failure is returned to the caller rather than continuing to check HttpModule::can_handle()/HttpModule::plugin_info(); keep the Ok(true) => Ok(Some(info)) path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/builtin/http_module.rs`:
- Around line 42-52: The current use of builder.redirect with a synchronous
validate_not_internal allows hostname-based redirects to bypass SSRF checks;
disable reqwest's automatic redirects and implement manual async redirect
handling so each hop is validated with validate_not_internal_async.
Specifically, remove or replace the builder.redirect(...) usage in the http
module, set the reqwest::Client to not follow redirects, and in the async
request execution (the code paths around the current redirect closure and the
similar block at lines ~198-223) implement a loop that sends the request,
inspects 3xx responses, resolves and run validate_not_internal_async on the
redirect target URL before following, limit hops (e.g., 10), and error out on
validation failures or excessive redirects to enforce SSRF protection.
- Around line 339-348: The fallback filename extraction currently strips query
strings but not fragments, so a URL like "https://.../file.zip#section" yields
"file.zip#section"; update the logic around path extraction (variables: url,
path, after_scheme and the rsplit() fallback) to also remove the fragment
identifier by splitting on '#' (or otherwise truncating at the first '#') before
checking for '/' and deriving the last path segment, ensuring the fragment can't
appear in the returned filename.
---
Outside diff comments:
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 114-125: When a plugin's can_handle call fails you must not fall
back to the builtin HTTP handler; instead propagate the plugin error so a failed
plugin doesn't silently handle requests. In the match on
self.registry.call_plugin(&name, "can_handle", url) (the branch that currently
does Err(e) => tracing::warn!(...)), change that branch to return Err(e) (or
wrap and return a suitable error type) so the failure is returned to the caller
rather than continuing to check
HttpModule::can_handle()/HttpModule::plugin_info(); keep the Ok(true) =>
Ok(Some(info)) path unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 357df963-7a3f-45c5-97c3-658473de0012
📒 Files selected for processing (3)
src-tauri/src/adapters/driven/plugin/builtin/http_module.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/domain/model/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/domain/model/mod.rs
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/builtin/http_module.rs:215">
P0: Redirect SSRF validation no longer resolves redirect hostnames, so a redirect to a domain that resolves to an internal IP can bypass the internal-network block.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Implement manual async redirect following with per-hop SSRF validation to prevent hostname-based redirect bypass (redirect to domain resolving to internal IP now blocked) - Remove sync redirect policy — all redirect handling is now async - Strip URL fragments (#section) from fallback filenames
Add audit.toml to ignore 12 transitive dependency advisories: - wasmtime 41.0.4 (via extism 1.21.0): 11 RUSTSEC-2026-* advisories requiring >=42.0.2, but extism hasn't released an update yet - rsa 0.9.10 (via sqlx-mysql → sea-orm): no fix available upstream These are tracked and will be removed when upstream releases fixes.
audit.toml isn't picked up by cargo-audit with --file flag in CI. Use explicit --ignore CLI flags to suppress 12 transitive dependency advisories (wasmtime 41.0.4 via extism, rsa 0.9.10 via sea-orm) where upstream hasn't released fixes yet.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 52-68: Remove the hardcoded --ignore flags from the CI "run" step
that invokes cargo audit and instead add all 12 advisory IDs currently listed in
the workflow (RUSTSEC-2026-0085..RUSTSEC-2026-0096 and RUSTSEC-2023-0071) into
the central .cargo/audit.toml ignore list; update .cargo/audit.toml to include
those advisory entries and delete the corresponding --ignore arguments from the
cargo audit invocation (leave the command as simply `cargo audit --file
src-tauri/Cargo.lock`), and remove or reconcile the duplicate
src-tauri/audit.toml so the single .cargo/audit.toml is authoritative.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f659b19-0002-419c-b21a-9b09ccbc3014
📒 Files selected for processing (1)
.github/workflows/ci.yml
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:57">
P2: The advisory exceptions are duplicated in CI and `src-tauri/audit.toml`, creating two sources of truth for security ignore policy. Keep the ignore list in a single place to avoid drift.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Move all advisory ignore entries to .cargo/audit.toml (native cargo-audit config) and remove --ignore CLI flags from CI workflow. Delete redundant src-tauri/audit.toml. Single source of truth for security ignore policy.
Summary
LinkStatusdomain model (Online/Offline/Unknown) for URL checking resultsHttpModulewithcheck_link,resolve_download_url, filename extraction (Content-Disposition RFC 5987 + URL path fallback)ExtismPluginLoader::resolve_url()— when no WASM plugin claims a URL, the built-in HTTP module handles http:// and https://../, null bytes, slashes)user:pass@hostin logs)can_handlereturns false for ftp://)Files changed
domain/model/link.rsLinkStatusenumadapters/driven/plugin/builtin/mod.rsadapters/driven/plugin/builtin/http_module.rsHttpModuleimplementation + 22 testsadapters/driven/plugin/extism_loader.rsresolve_url(), failable constructoradapters/driven/plugin/mod.rspub mod builtinadapters/driven/plugin/watcher.rsExtismPluginLoader::new()domain/model/mod.rspub mod linklib.rsHttpModuleTest plan
cargo test --workspace)cargo clippy --workspace -- -D warningscleancargo fmt --checkcleanSummary by cubic
Adds a built-in HTTP/HTTPS module with async, per-hop SSRF-safe redirects for direct downloads and introduces
LinkStatus. It is the secure fallback inExtismPluginLoader::resolve_url()for task 15, andExtismPluginLoader::new()now returns aResult.New Features
HttpModulehandleshttp://andhttps://when no WASM plugin claims the URL; returnsPluginInfo(builtin-http). FTP not supported.check_link(HEAD) returnsLinkStatuswith optional filename, size, and resumable;resolve_download_urlfollows redirects with per-hop async DNS SSRF validation.None.Dependencies
cargo-auditignores in.cargo/audit.toml; CI no longer passes--ignoreflags.RUSTSEC-2023-0071(unused MySQL path) andwasmtimeadvisoriesRUSTSEC-2026-0085–0096pulled viaextismuntil upstream updates.Written for commit 8226f5e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Security
Tests
Chores