feat(proxy): apply proxy for downloading datasets #881
Conversation
Signed-off-by: Yuwei Ba <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR implements proxy support for downloading datasets (mmdb, geosite files) used by Clash. The implementation adds the ability to specify which outbound proxy to use for downloads via URL fragments, enabling dataset downloads through proxy connections rather than direct connections.
- Refactors HTTP client to support proxy routing for downloads
- Adds URL fragment parsing for proxy selection and forced downloads
- Reorganizes outbound manager initialization to enable bootstrap proxy usage
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| clash-lib/src/common/http/client.rs | Complete rewrite of HTTP client to support proxy routing and remove hyper dependency |
| clash-lib/src/common/utils.rs | Enhanced download function with URL fragment parsing for proxy selection |
| clash-lib/src/lib.rs | Restructured component initialization order to enable bootstrap proxy usage |
| clash-lib/src/app/outbound/manager.rs | Refactored to separate plain outbound loading for bootstrap usage |
| clash-lib/src/proxy/http/inbound/* | Updated HTTP inbound handlers to use TokioIo wrapper |
| clash-lib/src/common/mmdb.rs | Added force download support via URL parameters |
| clash-lib/src/common/geodata/mod.rs | Added force download support via URL parameters |
| clash-bin/tests/data/config/rules.yaml | Added test configuration with proxy-enabled download URLs |
clash-lib/src/common/http/client.rs
Outdated
| .as_ref() | ||
| .and_then(|outbounds| outbounds.get(x).cloned()) | ||
| }) | ||
| .unwrap_or(Arc::new(proxy::direct::Handler::new()) as _); |
There was a problem hiding this comment.
Using unwrap_or with a fallback that creates a new direct handler on every call is inefficient. Consider creating a static or cached instance of the direct handler to avoid repeated allocations.
clash-lib/src/common/utils.rs
Outdated
| let params: HashMap<String, String> = pairs.collect(); | ||
| ClashHTTPClientExt { | ||
| outbound: params.get("_clash_outbound").cloned(), | ||
| force: params.get("force").is_some_and(|x| x == "true"), |
There was a problem hiding this comment.
[nitpick] The force parameter parsing is case-sensitive and only accepts "true". Consider using a more flexible boolean parsing approach that accepts common boolean representations like "1", "yes", "on", etc., or at least make it case-insensitive.
| force: params.get("force").is_some_and(|x| x == "true"), | |
| force: params.get("force").map_or(false, |x| parse_bool(x)), |
| P: AsRef<Path> + std::marker::Send, | ||
| { | ||
| let ext = { | ||
| let fragments = url.rsplit_once('#').map(|x| x.1).unwrap_or_default(); |
There was a problem hiding this comment.
The URL fragment parsing assumes a specific format but doesn't validate the URL structure. Consider using a proper URL parsing library to handle edge cases and malformed URLs more robustly.
| let stream = tokio::time::timeout( | ||
| self.timeout, | ||
| connector.connect( | ||
| host.try_into().expect("must be valid SNI"), |
There was a problem hiding this comment.
Using expect() here can cause a panic if the host is not a valid SNI name. This should return a proper error instead of panicking, especially since this is in a network operation that could receive arbitrary input.
| host.try_into().expect("must be valid SNI"), | |
| match host.try_into() { | |
| Ok(valid_sni) => valid_sni, | |
| Err(e) => { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::InvalidInput, | |
| format!("Invalid SNI name: {e}"), | |
| )); | |
| } | |
| }, |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
🤔 This is a ...
🔗 Related issue link
#868
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge