Documenting & Simplifying Error Variants #393
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,26 +270,40 @@ fn is_cgn_or_reserved_v4(octets: [u8; 4]) -> bool { | |
| false | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| enum InvalidUrlError { | ||
| #[error("URL has no host")] | ||
| NoHost, | ||
|
|
||
| #[error("DNS resolution failed for {host}: {source}")] | ||
| DnsResolutionFailed { | ||
| host: String, | ||
| source: std::io::Error, | ||
| }, | ||
|
|
||
| #[error("URL resolves to a private/reserved IP address: {0}")] | ||
| PrivateIp(std::net::IpAddr), | ||
| } | ||
|
|
||
| /// Resolves a URL's hostname via DNS and rejects it if any resolved address is | ||
| /// private/reserved. This prevents SSRF attacks where a user-supplied URL could reach | ||
| /// internal services (e.g. cloud metadata at 169.254.169.254, internal APIs, etc.). | ||
| async fn validate_url_target(url: &Url) -> Result<(), InvalidUrlError> { | ||
| let host = url.host_str().ok_or(InvalidUrlError::NoHost)?; | ||
| let port = url.port_or_known_default().unwrap_or(443); | ||
| let addr = format!("{host}:{port}"); | ||
|
|
||
| let resolved = | ||
| tokio::net::lookup_host(&addr) | ||
| .await | ||
| .map_err(|e| InvalidUrlError::DnsResolutionFailed { | ||
| host: host.to_string(), | ||
| source: e, | ||
| })?; | ||
|
Comment on lines
+291
to
+302
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query:
💡 Result: In reqwest 0.13.x, Client::get(url).send performs DNS resolution independently of any prior tokio::net::lookup_host call. Reqwest uses its own configurable DNS resolver (via ClientBuilder::dns_resolver), which resolves the host from the URL's http::Uri during connection setup. A prior lookup_host does not affect or share state with reqwest's internal resolution process. tokio::net::lookup_host((host, port)) is the correct way to resolve an IPv6 literal returned by url::Url::host_str. The host_str returns the raw host string (e.g., "[::1]" for IPv6 literals). lookup_host uses std::net::ToSocketAddrs, which parses IP literals (IPv4/IPv6) directly without DNS lookup, combining them with the provided port to yield SocketAddrs. For domain names, it performs DNS resolution. This matches reqwest's expected SocketAddr format from its resolver. Citations:
🏁 Script executed: # First, check the file structure and see lines around the mentioned locations
wc -l crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 109 🏁 Script executed: # Check the validate_url_target function (lines 291-302)
sed -n '285,310p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 984 🏁 Script executed: # Check lines 370-372 and 498-500 for similar patterns
sed -n '365,375p' crates/lib/src/core/v_latest/workspaces/files.rs
echo "---"
sed -n '493,505p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 891 🏁 Script executed: # Find how the validated result from validate_url_target is used (if at all)
rg "validate_url_target" -A 10 -B 2 crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 1457 🏁 Script executed: # Check if reqwest::Client is used to send requests with the original URL
rg "reqwest.*Client|\.get\(" crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 234 🏁 Script executed: # Let me check the URL library documentation and test how host_str() works for IPv6
# Search for url crate usage in the file
rg "use.*url|from_str.*Url" crates/lib/src/core/v_latest/workspaces/files.rs | head -20Repository: Oxen-AI/Oxen Length of output: 69 🏁 Script executed: # Check Cargo.toml for url crate version
fd Cargo.toml crates/lib | xargs grep -A 10 "^\[dependencies\]" | head -30Repository: Oxen-AI/Oxen Length of output: 417 🏁 Script executed: # Let's search for how IPv6 URLs would be handled - check if there are any tests
fd "test.*files\|files.*test" --type f | xargs grep -l "validate_url_target\|ipv6\|::" 2>/dev/null | head -5Repository: Oxen-AI/Oxen Length of output: 38 🏁 Script executed: # Look for actual uses of fetch_file and how URLs are constructed to understand the full flow
sed -n '320,350p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 946 🏁 Script executed: # Check if there's a return value or stored SocketAddrs from validate_url_target
ast-grep --pattern 'fn validate_url_target($_) -> $_ {
$$$
Ok($_)
}'Repository: Oxen-AI/Oxen Length of output: 38 🏁 Script executed: # Let's check what the actual fetch function does and how it uses the URL
sed -n '380,420p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 1136 🏁 Script executed: # Let's examine the fetch_file function to confirm the DNS-rebinding issue
sed -n '251,290p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 1263 🏁 Script executed: # Check if resolved SocketAddrs are ever used after validation
rg "resolved|SocketAddr" crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 188 🏁 Script executed: # Let's check the fetch_file signature and how it uses current_url
sed -n '330,360p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 906 🏁 Script executed: # Confirm that validate_url_target result is not reused - check the full context around the call
sed -n '365,385p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 598 🏁 Script executed: # Let's verify how the URL and SocketAddrs would flow through - check if resolved is ever stored/returned
sed -n '291,315p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 857 🏁 Script executed: # Let me double-check the fetch_file implementation to see how it uses the URL
sed -n '450,480p' crates/lib/src/core/v_latest/workspaces/files.rsRepository: Oxen-AI/Oxen Length of output: 1103 🏁 Script executed: # Look for the complete fetch_file function signature
rg "async fn fetch_file\|client\.get" -A 2 crates/lib/src/core/v_latest/workspaces/files.rs | head -20Repository: Oxen-AI/Oxen Length of output: 38
The function validates that a hostname resolves to a non-private IP, but returns only Also applies to: 370-372, 498-500 🤖 Prompt for AI Agents |
||
|
|
||
| for socket_addr in resolved { | ||
| if is_private_ip(&socket_addr.ip()) { | ||
| return Err(InvalidUrlError::PrivateIp(socket_addr.ip())); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -353,7 +367,9 @@ pub async fn import( | |
| )); | ||
| } | ||
|
|
||
| validate_url_target(&parsed_url) | ||
| .await | ||
| .map_err(|e| OxenError::ImportFileError(format!("{e}").into()))?; | ||
|
|
||
| let auth_header_value = HeaderValue::from_str(auth) | ||
| .map_err(|_e| OxenError::file_import_error(format!("Invalid header auth value {auth}")))?; | ||
|
|
@@ -407,12 +423,12 @@ pub async fn upload_zip( | |
| log::debug!("workspace::commit ✅ success! commit {commit:?}"); | ||
| Ok(commit) | ||
| } | ||
| workspace_behind_error @ Err(OxenError::WorkspaceBehind(_)) => { | ||
| log::error!( | ||
| "unable to commit branch {:?}. Workspace behind", | ||
| branch.name | ||
| ); | ||
| workspace_behind_error | ||
|
Comment on lines
+426
to
+431
Contributor
There was a problem hiding this comment. The zip-import path still needs the This branch re-propagates the raw 🔎 Quick verification#!/bin/bash
rg -n --type=rust "workspaces::files::upload_zip|WorkspaceBehind" crates/server/src/controllers -C3🤖 Prompt for AI Agents |
||
| } | ||
| Err(err) => { | ||
| log::error!("unable to commit branch {:?}. Err: {}", branch.name, err); | ||
|
|
@@ -479,7 +495,9 @@ async fn fetch_file( | |
| "Redirect to non-HTTP(S) URL is not allowed", | ||
| )); | ||
| } | ||
| validate_url_target(&next_url) | ||
| .await | ||
| .map_err(|e| OxenError::ImportFileError(format!("{e}").into()))?; | ||
|
|
||
| current_url = next_url; | ||
| continue; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BranchNotFoundnow renders only the raw branch name.crates/lib/src/error.rsformatsOxenError::BranchNotFoundas"{0}", so this path currently returns justmain/feature/fooinstead of a full not-found message. Please keep using the local-branch helper here or format the message before constructing the variant.💡 Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents