fix(setup): validate channel credentials during setup#684
fix(setup): validate channel credentials during setup#684Protocol-zero-0 wants to merge 1 commit intonearai:mainfrom
Conversation
Validate channel setup credentials against the declared validation endpoint so users get immediate feedback before startup failures. Substitute stored secrets into the validation URL, block private or local targets, and warn on failed checks without interrupting setup. Made-with: Cursor
github-actions
bot
added
scope: setup
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the channel setup process by introducing immediate credential validation for WASM channels. Previously, users would only discover invalid credentials after a channel failed to start. By validating credentials during setup, the system now provides early feedback, improving the user experience and reducing potential operational issues, while still allowing setup to proceed with a warning if validation fails. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
risk: high
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding credential validation at setup time for WASM channels, which will improve the user experience by providing earlier feedback. The implementation is well-structured with new functions for placeholder substitution and URL validation, accompanied by good test coverage. However, critical security vulnerabilities have been identified. These include a Time-of-check-to-time-of-use (TOCTOU) race condition (also known as a DNS rebinding vulnerability) and the potential for SSRF protection bypass using IPv4-mapped IPv6 addresses, both of which could allow an attacker to bypass SSRF protections. Furthermore, sensitive secrets are leaked in error messages printed to the console. These issues should be addressed to ensure the validation process does not compromise the security of the user's credentials or the host system.
| fn validate_public_https_url(url: &str) -> Result<Url, ChannelSetupError> { | ||
| use std::net::{IpAddr, ToSocketAddrs}; | ||
|
|
||
| let parsed = Url::parse(url) | ||
| .map_err(|e| ChannelSetupError::Validation(format!("Invalid URL: {}", e)))?; | ||
|
|
||
| if parsed.scheme() != "https" { | ||
| return Err(ChannelSetupError::Validation( | ||
| "Validation endpoint must use https".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| if !parsed.username().is_empty() || parsed.password().is_some() { | ||
| return Err(ChannelSetupError::Validation( | ||
| "Validation endpoint cannot contain userinfo".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let host = parsed | ||
| .host_str() | ||
| .ok_or_else(|| ChannelSetupError::Validation("Validation URL missing host".to_string()))?; | ||
| let host_lower = host.to_lowercase(); | ||
|
|
||
| if host_lower == "localhost" || host_lower.ends_with(".localhost") { | ||
| return Err(ChannelSetupError::Validation( | ||
| "Validation endpoint cannot target localhost".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| if let Ok(ip) = host.parse::<IpAddr>() { | ||
| if is_disallowed_ip(&ip) { | ||
| return Err(ChannelSetupError::Validation(format!( | ||
| "Validation endpoint cannot target private or local IP {}", | ||
| ip | ||
| ))); | ||
| } | ||
| return Ok(parsed); | ||
| } | ||
|
|
||
| let port = parsed.port_or_known_default().unwrap_or(443); | ||
| let socket_addr = format!("{}:{}", host, port); | ||
| if let Ok(addrs) = socket_addr.to_socket_addrs() { | ||
| for addr in addrs { | ||
| if is_disallowed_ip(&addr.ip()) { | ||
| return Err(ChannelSetupError::Validation(format!( | ||
| "Validation hostname '{}' resolves to disallowed IP {}", | ||
| host, | ||
| addr.ip() | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(parsed) | ||
| } |
There was a problem hiding this comment.
This implementation has a Time-of-check-to-time-of-use (TOCTOU) vulnerability, also known as a DNS rebinding attack. The function resolves a hostname to IP addresses for validation but then discards them. The caller, validate_channel_credentials, receives only the URL and performs a new DNS lookup for the actual request. An attacker could return a safe IP during validation and a private IP for the request, bypassing the SSRF protection.
Additionally, if DNS resolution fails (to_socket_addrs() returns an Err), the error is ignored, and the function incorrectly returns Ok, bypassing the check entirely.
To fix this, you should:
- Return the validated
SocketAddrs from this function. - Handle DNS resolution errors.
- In
validate_channel_credentials, usereqwest::ClientBuilder::resolveto pin the request to the validated IP addresses.
Here is a suggested implementation for validate_public_https_url. You will also need to update validate_channel_credentials to handle the new return type and configure the reqwest::Client.
Example for validate_channel_credentials:
let (parsed, resolved_addrs) = validate_public_https_url(&validation_url)?;
let mut client_builder = reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(5));
if let Some(host) = parsed.host_str() {
for addr in resolved_addrs {
client_builder = client_builder.resolve(host, addr);
}
}
let client = client_builder.build()?;
// ... make request with clientfn validate_public_https_url(
url: &str,
) -> Result<(Url, Vec<std::net::SocketAddr>), ChannelSetupError> {
use std::net::{IpAddr, SocketAddr, ToSocketAddrs};
let parsed =
Url::parse(url).map_err(|e| ChannelSetupError::Validation(format!("Invalid URL: {}", e)))?;
if parsed.scheme() != "https" {
return Err(ChannelSetupError::Validation(
"Validation endpoint must use https".to_string(),
));
}
if !parsed.username().is_empty() || parsed.password().is_some() {
return Err(ChannelSetupError::Validation(
"Validation endpoint cannot contain userinfo".to_string(),
));
}
let host = parsed
.host_str()
.ok_or_else(|| ChannelSetupError::Validation("Validation URL missing host".to_string()))?;
let host_lower = host.to_lowercase();
if host_lower == "localhost" || host_lower.ends_with(".localhost") {
return Err(ChannelSetupError::Validation(
"Validation endpoint cannot target localhost".to_string(),
));
}
let port = parsed.port_or_known_default().unwrap_or(443);
if let Ok(ip) = host.parse::<IpAddr>() {
if is_disallowed_ip(&ip) {
return Err(ChannelSetupError::Validation(format!(
"Validation endpoint cannot target private or local IP {}",
ip
)));
}
let addr = SocketAddr::new(ip, port);
return Ok((parsed, vec![addr]));
}
let socket_addr = format!("{}:{}", host, port);
let addrs: Vec<SocketAddr> = socket_addr
.to_socket_addrs()
.map_err(|e| {
ChannelSetupError::Validation(format!("DNS resolution failed for {}: {}", host, e))
})?
.collect();
if addrs.is_empty() {
return Err(ChannelSetupError::Validation(format!(
"Validation hostname '{}' did not resolve to any IP addresses",
host
)));
}
for addr in &addrs {
if is_disallowed_ip(&addr.ip()) {
return Err(ChannelSetupError::Validation(format!(
"Validation hostname '{}' resolves to disallowed IP {}",
host,
addr.ip()
)));
}
}
Ok((parsed, addrs))
}References
- To prevent DNS-based SSRF vulnerabilities, resolve hostnames to IP addresses, validate all resolved IPs against restricted ranges, and pin the validated addresses for the connection to prevent TOCTOU race conditions.
| Err(ChannelSetupError::Validation(format!( | ||
| "Validation endpoint returned HTTP {} for {}", | ||
| response.status(), | ||
| parsed | ||
| ))) |
There was a problem hiding this comment.
The error message constructed here includes the full parsed URL, which contains sensitive secrets substituted from the secrets store (e.g., API tokens). If the validation request fails, this error message is returned and subsequently printed to the console on line 811. This leads to the exposure of sensitive credentials in terminal output or log files.
To remediate this, avoid including the full URL in the error message. Instead, report only the host or a redacted version of the URL where placeholders are replaced with a string like [REDACTED].
| fn is_disallowed_ip(ip: &std::net::IpAddr) -> bool { | ||
| match ip { | ||
| std::net::IpAddr::V4(v4) => { | ||
| v4.is_private() | ||
| || v4.is_loopback() | ||
| || v4.is_link_local() | ||
| || v4.is_multicast() | ||
| || v4.is_unspecified() | ||
| || *v4 == std::net::Ipv4Addr::new(169, 254, 169, 254) | ||
| || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) | ||
| } | ||
| std::net::IpAddr::V6(v6) => { | ||
| v6.is_loopback() | ||
| || v6.is_unique_local() | ||
| || v6.is_unicast_link_local() | ||
| || v6.is_multicast() | ||
| || v6.is_unspecified() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The is_disallowed_ip function does not account for IPv4-mapped IPv6 addresses (e.g., ::ffff:127.0.0.1). An attacker could provide a validation endpoint using such an address to bypass the SSRF protection and target internal services or the loopback interface.
To fix this, you should check if the IPv6 address is an IPv4-mapped address and, if so, validate the embedded IPv4 address against the disallowed list. In Rust, you can use Ipv6Addr::to_ipv4_mapped() (if on nightly) or check if the first 12 bytes are 00 00 00 00 00 00 00 00 00 00 FF FF.
There was a problem hiding this comment.
Pull request overview
Adds setup-time credential validation for WASM channels by substituting stored secrets into a schema-provided validation_endpoint, validating the final URL for SSRF safety, and issuing a short HTTPS request to provide early feedback during the setup wizard.
Changes:
- Perform non-blocking (warning-only) credential validation after saving required secrets when
validation_endpointis present. - Implement placeholder substitution (
{secret_name}) from the secrets store and URL safety checks intended to reject localhost/private targets. - Add regression tests for placeholder substitution and basic URL safety rules (https/localhost/private IPv4).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Ok(addrs) = socket_addr.to_socket_addrs() { | ||
| for addr in addrs { | ||
| if is_disallowed_ip(&addr.ip()) { | ||
| return Err(ChannelSetupError::Validation(format!( | ||
| "Validation hostname '{}' resolves to disallowed IP {}", | ||
| host, | ||
| addr.ip() | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
validate_public_https_url uses ToSocketAddrs (blocking DNS resolution) inside an async setup flow, and it currently fails open when resolution errors (or returns 0 addrs), allowing the subsequent reqwest request to resolve/fetch without having enforced the “public only” constraint. Consider switching to tokio::net::lookup_host, erroring on resolution failure/empty results, and (ideally) pinning the resolved addrs via ClientBuilder::resolve_to_addrs to avoid re-resolution.
| if let Ok(addrs) = socket_addr.to_socket_addrs() { | |
| for addr in addrs { | |
| if is_disallowed_ip(&addr.ip()) { | |
| return Err(ChannelSetupError::Validation(format!( | |
| "Validation hostname '{}' resolves to disallowed IP {}", | |
| host, | |
| addr.ip() | |
| ))); | |
| } | |
| } | |
| } | |
| let addrs = socket_addr | |
| .to_socket_addrs() | |
| .map_err(|e| { | |
| ChannelSetupError::Validation(format!( | |
| "Validation hostname '{}' could not be resolved: {}", | |
| host, e | |
| )) | |
| })?; | |
| let mut saw_addr = bool::false(); | |
| for addr in addrs { | |
| saw_addr = true; | |
| if is_disallowed_ip(&addr.ip()) { | |
| return Err(ChannelSetupError::Validation(format!( | |
| "Validation hostname '{}' resolves to disallowed IP {}", | |
| host, | |
| addr.ip() | |
| ))); | |
| } | |
| } | |
| if !saw_addr { | |
| return Err(ChannelSetupError::Validation(format!( | |
| "Validation hostname '{}' did not resolve to any addresses", | |
| host | |
| ))); | |
| } |
| v6.is_loopback() | ||
| || v6.is_unique_local() | ||
| || v6.is_unicast_link_local() | ||
| || v6.is_multicast() | ||
| || v6.is_unspecified() |
There was a problem hiding this comment.
IPv4-mapped IPv6 addresses (e.g. https://[::ffff:192.168.1.10]/) will parse as IPv6 and currently bypass the V4 private-range checks because the V6 branch doesn’t normalize to_ipv4_mapped(). This is a known SSRF bypass pattern; consider normalizing mapped addresses before evaluating disallowed ranges (see similar handling in src/tools/builtin/skill_tools.rs).
| v6.is_loopback() | |
| || v6.is_unique_local() | |
| || v6.is_unicast_link_local() | |
| || v6.is_multicast() | |
| || v6.is_unspecified() | |
| if let Some(v4) = v6.to_ipv4_mapped() { | |
| // Treat IPv4-mapped IPv6 addresses the same as IPv4 | |
| v4.is_private() | |
| || v4.is_loopback() | |
| || v4.is_link_local() | |
| || v4.is_multicast() | |
| || v4.is_unspecified() | |
| || v4 == std::net::Ipv4Addr::new(169, 254, 169, 254) | |
| || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) | |
| } else { | |
| v6.is_loopback() | |
| || v6.is_unique_local() | |
| || v6.is_unicast_link_local() | |
| || v6.is_multicast() | |
| || v6.is_unspecified() | |
| } |
| for secret_name in placeholder_names { | ||
| let secret_value = secrets.get_secret(&secret_name).await?; | ||
| let placeholder = format!("{{{}}}", secret_name); | ||
| resolved = resolved.replace(&placeholder, secret_value.expose_secret()); | ||
| } |
There was a problem hiding this comment.
Placeholder substitution inserts raw secret values into the URL without percent-encoding. If a token contains reserved characters (?, #, &, /, %, etc.), the resulting URL may be invalid or change semantics (query injection). Consider parsing the URL and percent-encoding substituted values (or constraining placeholders to query values and encoding accordingly).
| #[test] | ||
| fn test_validate_public_https_url_rejects_localhost() { | ||
| let err = validate_public_https_url("https://localhost/api") | ||
| .unwrap_err() | ||
| .to_string(); | ||
| assert!(err.contains("localhost")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_public_https_url_rejects_private_ip() { | ||
| let err = validate_public_https_url("https://192.168.1.10/api") | ||
| .unwrap_err() | ||
| .to_string(); | ||
| assert!(err.contains("private or local IP")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_public_https_url_rejects_http() { | ||
| let err = validate_public_https_url("http://example.com/api") | ||
| .unwrap_err() | ||
| .to_string(); | ||
| assert!(err.contains("must use https")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_public_https_url_accepts_public_https_literal_ip() { | ||
| let parsed = validate_public_https_url("https://8.8.8.8/api").unwrap(); | ||
| assert_eq!(parsed.as_str(), "https://8.8.8.8/api"); | ||
| } |
There was a problem hiding this comment.
Tests cover localhost and private IPv4 literals, but don’t cover common SSRF bypasses this validator is intended to prevent (e.g. localhost. with a trailing dot, and IPv4-mapped IPv6 like https://[::ffff:192.168.1.10]/). Adding focused regression tests for those cases would help lock down the URL safety checks.
| "Validation endpoint returned HTTP {} for {}", | ||
| response.status(), | ||
| parsed |
There was a problem hiding this comment.
The validation error includes the fully substituted URL (parsed) in the message. Since placeholders are replaced with secrets, this can leak credentials to the console/logs via the warning printed during setup. Consider redacting sensitive parts (e.g., strip query/fragment, or avoid printing the URL entirely) in validation/network errors.
| "Validation endpoint returned HTTP {} for {}", | |
| response.status(), | |
| parsed | |
| "Validation endpoint returned HTTP {}", | |
| response.status(), |
| let client = reqwest::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(5)) | ||
| .build() | ||
| .map_err(|e| ChannelSetupError::Network(format!("Failed to build HTTP client: {}", e)))?; |
There was a problem hiding this comment.
reqwest::Client follows redirects by default, so a public HTTPS validation endpoint could redirect to http:// or to a private/localhost target and still be fetched. For SSRF safety, disable redirects (Policy::none()) or enforce a custom redirect policy that re-validates each hop (scheme + host/IP).
Summary
validation_endpointScope
This PR intentionally covers only the first half of issue #651: channel credential validation during setup.
It does not implement the routine webhook trigger endpoint from the same issue, to keep the change small and reviewer-friendly.
Why
Right now the setup wizard accepts channel secrets blindly, so users often only discover a bad token after the channel fails to start. This change moves that feedback into setup while still keeping the flow forgiving.
Behavior
validation_endpoint, setup now validates credentials immediately after saving them{telegram_bot_token}are replaced from the secrets storehttpstargets; localhost, private IPs, and hostnames resolving to private IPs are rejectedTests
cargo test --lib test_substitute_validation_placeholderstarget/debug/deps/ironclaw-ec6ff0546cc1c6a3 --exact setup::channels::tests::test_validate_public_https_url_rejects_localhosttarget/debug/deps/ironclaw-ec6ff0546cc1c6a3 --exact setup::channels::tests::test_validate_public_https_url_rejects_private_iptarget/debug/deps/ironclaw-ec6ff0546cc1c6a3 --exact setup::channels::tests::test_validate_public_https_url_rejects_httptarget/debug/deps/ironclaw-ec6ff0546cc1c6a3 --exact setup::channels::tests::test_validate_public_https_url_accepts_public_https_literal_ipcargo fmt --checkcargo clippy --lib --tests -- -D warningsNotes
I used a constrained
clippyrun (lib + tests) instead of a fresh fullall-featuresrebuild because disk pressure on this machine makes repeated full rebuilds unreliable, but the paths touched here are covered by the targeted test and lint runs above.Made with Cursor