fix(security): default webhook to loopback when tunnel active#1194
fix(security): default webhook to loopback when tunnel active#1194ilblackdragon merged 2 commits intostagingfrom
Conversation
…igured When a tunnel provider (ngrok, cloudflare, tailscale, etc.) or static TUNNEL_URL is configured, external traffic arrives through the tunnel, so binding 0.0.0.0 is unnecessary attack surface. The webhook server now defaults to 127.0.0.1 when a tunnel is active. Explicit HTTP_HOST still overrides the default in all cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
scope: channel/cli
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 implements a security improvement by adjusting the default binding address for the webhook server. When a tunnel is detected as active, the server will now default to listening on the loopback interface (127.0.0.1), minimizing unnecessary exposure to the network. This change ensures that the application adheres to the principle of least privilege by only binding to 0.0.0.0 when a tunnel is not in use, while still allowing explicit host configurations to take precedence. Highlights
Changelog
Activity
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
|
![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 sensible security enhancement by defaulting the webhook server to bind to the loopback interface (127.0.0.1) when a tunnel is active, reducing unnecessary attack surface. The implementation correctly propagates the tunnel status through the configuration layers. My review includes one high-severity comment regarding the new tests, which manipulate shared environment variables without proper locking, potentially causing race conditions and test flakiness, in violation of the rule regarding serialization of tests modifying shared global state. I've provided a code suggestion to fix this by adding the required mutex and ensuring consistent test cleanup.
| /// When a tunnel is active and HTTP_HOST is not explicitly set, the | ||
| /// webhook server should default to loopback to avoid unnecessary exposure. | ||
| #[test] | ||
| fn http_host_defaults_to_loopback_with_tunnel() { | ||
| // Set HTTP_PORT to trigger HttpConfig creation, but leave HTTP_HOST unset | ||
| // so the default kicks in. | ||
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9999"); | ||
| std::env::remove_var("HTTP_HOST"); | ||
| } | ||
| let settings = crate::settings::Settings::default(); | ||
| let cfg = ChannelsConfig::resolve(&settings, true).unwrap(); | ||
| unsafe { | ||
| std::env::remove_var("HTTP_PORT"); | ||
| } | ||
| let http = cfg.http.expect("HttpConfig should be present"); | ||
| assert_eq!( | ||
| http.host, "127.0.0.1", | ||
| "tunnel active should default to loopback" | ||
| ); | ||
| assert_eq!(http.port, 9999); | ||
| } | ||
|
|
||
| /// Without a tunnel, the webhook server defaults to 0.0.0.0 so external | ||
| /// services can reach it directly. | ||
| #[test] | ||
| fn http_host_defaults_to_all_interfaces_without_tunnel() { | ||
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9998"); | ||
| std::env::remove_var("HTTP_HOST"); | ||
| } | ||
| let settings = crate::settings::Settings::default(); | ||
| let cfg = ChannelsConfig::resolve(&settings, false).unwrap(); | ||
| unsafe { | ||
| std::env::remove_var("HTTP_PORT"); | ||
| } | ||
| let http = cfg.http.expect("HttpConfig should be present"); | ||
| assert_eq!( | ||
| http.host, "0.0.0.0", | ||
| "no tunnel should default to all interfaces" | ||
| ); | ||
| } | ||
|
|
||
| /// An explicit HTTP_HOST always wins regardless of tunnel state. | ||
| #[test] | ||
| fn explicit_http_host_overrides_tunnel_default() { | ||
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9997"); | ||
| std::env::set_var("HTTP_HOST", "192.168.1.50"); | ||
| } | ||
| let settings = crate::settings::Settings::default(); | ||
| let cfg = ChannelsConfig::resolve(&settings, true).unwrap(); | ||
| unsafe { | ||
| std::env::remove_var("HTTP_PORT"); | ||
| std::env::remove_var("HTTP_HOST"); | ||
| } | ||
| let http = cfg.http.expect("HttpConfig should be present"); | ||
| assert_eq!( | ||
| http.host, "192.168.1.50", | ||
| "explicit host should override tunnel default" | ||
| ); | ||
| } |
There was a problem hiding this comment.
These new tests manipulate environment variables, which is a shared global state. This can lead to flaky tests if not handled carefully.
- Race Conditions: The tests should acquire a lock on
crate::config::helpers::ENV_MUTEXbefore modifying environment variables to prevent race conditions when tests are run in parallel, as documented insrc/config/helpers.rs. - Incomplete Cleanup: The cleanup logic in
http_host_defaults_to_loopback_with_tunnelandhttp_host_defaults_to_all_interfaces_without_tunnelis incomplete. They modifyHTTP_HOSTbut don't clean it up, which can interfere with other tests.
The suggestion below adds the required mutex lock to all three tests and makes the cleanup logic consistent.
/// When a tunnel is active and HTTP_HOST is not explicitly set, the
/// webhook server should default to loopback to avoid unnecessary exposure.
#[test]
fn http_host_defaults_to_loopback_with_tunnel() {
let _guard = crate::config::helpers::ENV_MUTEX.lock().unwrap();
// Set HTTP_PORT to trigger HttpConfig creation, but leave HTTP_HOST unset
// so the default kicks in.
unsafe {
std::env::set_var("HTTP_PORT", "9999");
std::env::remove_var("HTTP_HOST");
}
let settings = crate::settings::Settings::default();
let cfg = ChannelsConfig::resolve(&settings, true).unwrap();
unsafe {
std::env::remove_var("HTTP_PORT");
std::env::remove_var("HTTP_HOST");
}
let http = cfg.http.expect("HttpConfig should be present");
assert_eq!(
http.host, "127.0.0.1",
"tunnel active should default to loopback"
);
assert_eq!(http.port, 9999);
}
/// Without a tunnel, the webhook server defaults to 0.0.0.0 so external
/// services can reach it directly.
#[test]
fn http_host_defaults_to_all_interfaces_without_tunnel() {
let _guard = crate::config::helpers::ENV_MUTEX.lock().unwrap();
unsafe {
std::env::set_var("HTTP_PORT", "9998");
std::env::remove_var("HTTP_HOST");
}
let settings = crate::settings::Settings::default();
let cfg = ChannelsConfig::resolve(&settings, false).unwrap();
unsafe {
std::env::remove_var("HTTP_PORT");
std::env::remove_var("HTTP_HOST");
}
let http = cfg.http.expect("HttpConfig should be present");
assert_eq!(
http.host, "0.0.0.0",
"no tunnel should default to all interfaces"
);
}
/// An explicit HTTP_HOST always wins regardless of tunnel state.
#[test]
fn explicit_http_host_overrides_tunnel_default() {
let _guard = crate::config::helpers::ENV_MUTEX.lock().unwrap();
unsafe {
std::env::set_var("HTTP_PORT", "9997");
std::env::set_var("HTTP_HOST", "192.168.1.50");
}
let settings = crate::settings::Settings::default();
let cfg = ChannelsConfig::resolve(&settings, true).unwrap();
unsafe {
std::env::remove_var("HTTP_PORT");
std::env::remove_var("HTTP_HOST");
}
let http = cfg.http.expect("HttpConfig should be present");
assert_eq!(
http.host, "192.168.1.50",
"explicit host should override tunnel default"
);
}References
- Tests that modify shared global state should be serialized using a mutex to prevent race conditions and flakiness when run in parallel.
There was a problem hiding this comment.
Pull request overview
This PR reduces webhook server exposure by defaulting HTTP_HOST to loopback (127.0.0.1) when a tunnel is configured, while preserving the existing 0.0.0.0 default when no tunnel is active and keeping HTTP_HOST as an explicit override.
Changes:
- Resolve tunnel configuration early and pass
tunnel_enabledinto channel config resolution. - Update
ChannelsConfig::resolveto default webhook bind host to loopback whentunnel_enabled. - Add unit tests covering the new host-defaulting behavior under tunnel/no-tunnel/explicit-host scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/config/mod.rs | Resolve tunnel first and pass tunnel.is_enabled() into channel config resolution. |
| src/config/channels.rs | Add tunnel_enabled parameter; change webhook host default based on tunnel state; add tests. |
| src/cli/doctor.rs | Update doctor gateway check to use the new ChannelsConfig::resolve(..., tunnel_enabled) signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9997"); | ||
| std::env::set_var("HTTP_HOST", "192.168.1.50"); | ||
| } |
| let tunnel_enabled = crate::config::TunnelConfig::resolve(settings) | ||
| .map(|t| t.is_enabled()) | ||
| .unwrap_or(false); |
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9999"); | ||
| std::env::remove_var("HTTP_HOST"); | ||
| } |
| unsafe { | ||
| std::env::set_var("HTTP_PORT", "9998"); | ||
| std::env::remove_var("HTTP_HOST"); | ||
| } |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: APPROVE
Clean, well-scoped security hardening fix. When a tunnel (ngrok/cloudflare/tailscale/custom) is configured, the webhook HTTP server now binds to 127.0.0.1 instead of 0.0.0.0. Explicit HTTP_HOST still overrides.
Security analysis — sound:
TunnelConfig::is_enabled()covers both staticTUNNEL_URLand managed providers- Gateway config already defaults to loopback, no gap
- WASM channel webhooks benefit automatically
- Edge case:
TUNNEL_PROVIDERset but tunnel not yet connected — correctly binds to loopback since user intent is tunnel
Code quality:
- Minimal diff (3 files), clean resolution ordering in
Config::build() - Three targeted tests cover: tunnel active, no tunnel, explicit override
One note (non-blocking): Tests use unsafe { std::env::set_var() } without #[serial_test::serial], which could cause flaky parallel test runs. Pre-existing pattern in this file though — not a regression from this PR.
CI all green.
Upstream refactored ChannelsConfig::resolve() to use settings-based defaults (env > settings > default). Merged our tunnel_enabled param into the new resolution chain and updated all new upstream test call sites to pass the tunnel_enabled argument. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…igured (nearai#1194) When a tunnel provider (ngrok, cloudflare, tailscale, etc.) or static TUNNEL_URL is configured, external traffic arrives through the tunnel, so binding 0.0.0.0 is unnecessary attack surface. The webhook server now defaults to 127.0.0.1 when a tunnel is active. Explicit HTTP_HOST still overrides the default in all cases. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…igured (nearai#1194) When a tunnel provider (ngrok, cloudflare, tailscale, etc.) or static TUNNEL_URL is configured, external traffic arrives through the tunnel, so binding 0.0.0.0 is unnecessary attack surface. The webhook server now defaults to 127.0.0.1 when a tunnel is active. Explicit HTTP_HOST still overrides the default in all cases. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
TUNNEL_URL) is configured, the webhook server now defaultsHTTP_HOSTto127.0.0.1instead of0.0.0.0HTTP_HOSTenv var still overrides the default in all casesTest plan
http_host_defaults_to_loopback_with_tunnel— tunnel active + no explicit host =127.0.0.1http_host_defaults_to_all_interfaces_without_tunnel— no tunnel =0.0.0.0(unchanged behavior)explicit_http_host_overrides_tunnel_default— explicitHTTP_HOSTalways winscargo clippy --all --all-features— zero warnings🤖 Generated with Claude Code