From c36ae02aef060a11558e61d055c14fb3551df39d Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 01:07:35 +0800 Subject: [PATCH 1/4] fix(tui): re-read the network policy so /network allow lands without a restart `/network allow ` edits config.toml and prints "Retry the command now.", but the engine held the NetworkPolicyDecider captured when the session was spawned, so the retry was refused for the rest of the session and the host only unblocked after a restart. The per-turn tool context and the MCP pool now take a decider rebuilt from the document's own [network] table. The session cache rides along, so an approval already granted through the approval prompt survives the refresh. A session launched without a [network] table stays ungated on purpose: adopting one from disk would hand that session a default = "prompt" policy it never had, and every unrelated host would start prompting. This follows the WorkspaceTrust precedent, which also re-reads per tool-context build so /trust add lands mid-session. As a side effect a hand edit of [network] in config.toml now takes effect on the next turn too. Known limitation: [profiles..network] is not consulted, matching what /network writes. --- crates/tui/src/config.rs | 27 +++++++ crates/tui/src/core/engine.rs | 35 +++++++- .../src/core/engine/tests/test_cases_12.rs | 72 +++++++++++++++++ crates/tui/src/network_policy.rs | 81 +++++++++++++++++++ 4 files changed, 211 insertions(+), 4 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 7382699e3b..689e6d39fb 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -3059,6 +3059,33 @@ impl NetworkPolicyToml { } } +/// Re-read the `[network]` table from a configuration document. +/// +/// Deliberately narrower than [`Config::load`]: this runs on the tool-context +/// build path, where re-applying the environment, managed, and credential +/// layers would be both wasteful and wrong. Only the document's own table comes +/// back — the same table `/network allow ` edits. +/// +/// `None` means there is nothing usable to adopt: the document is missing, +/// unreadable, unparseable, or carries no `[network]` table. Callers keep the +/// policy they already hold, which is the conservative direction for an +/// allow/deny gate. +/// +/// Known limitation: `[profiles..network]` is not consulted. `/network` +/// does not write it either, so a live session and the command agree on this +/// base table. +#[must_use] +pub fn network_policy_from_document(path: &Path) -> Option { + #[derive(Deserialize)] + struct NetworkTable { + network: Option, + } + + let contents = fs::read_to_string(path).ok()?; + let parsed: NetworkTable = toml::from_str(&contents).ok()?; + parsed.network.map(NetworkPolicyToml::into_runtime) +} + /// `[lsp]` table — mirrors [`crate::lsp::LspConfig`]. Documented in /// `config.example.toml`. When omitted, defaults from `LspConfig::default()` /// apply (enabled, 5 s poll, 20 diagnostics/file, errors only, no overrides). diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index f279cf821f..e7cd46b3e1 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -7104,6 +7104,33 @@ impl Engine { ) } + /// The network decider this turn runs under: the configured policy, + /// re-read from the document the session was launched with. + /// + /// `self.config.network_policy` is the snapshot `Engine::new` took when the + /// session was spawned. `/network allow ` edits `config.toml` and + /// promises "Retry the command now.", but it cannot reach that snapshot — + /// so without this re-read the host stayed refused until the next engine + /// spawn. Same shape as the workspace trust list loaded below, which also + /// re-reads per tool-context build so `/trust add` lands mid-session; a + /// hand edit of `[network]` lands through it too. + /// + /// The session cache rides along, so a host approved through the approval + /// prompt survives the refresh. A session launched without a `[network]` + /// table stays ungated on purpose: adopting one from disk here would hand + /// that session a `default = "prompt"` policy it never had, and every + /// unrelated host would start prompting. + fn current_network_decider(&self) -> Option { + let decider = self.config.network_policy.as_ref()?; + let Some(path) = self.api_config.loaded_config_path.as_deref() else { + return Some(decider.clone()); + }; + match crate::config::network_policy_from_document(path) { + Some(policy) => Some(decider.with_policy_refreshed(policy)), + None => Some(decider.clone()), + } + } + /// Build one tool context from the already-resolved turn authority and /// route. A preview owns values that are deliberately not installed on the /// session; rebuilding either from `self.session` would give it the prior @@ -7210,8 +7237,8 @@ impl Engine { ctx.memory_path = Some(self.config.memory_path.clone()); } - if let Some(decider) = self.config.network_policy.as_ref() { - ctx = ctx.with_network_policy(decider.clone()); + if let Some(decider) = self.current_network_decider() { + ctx = ctx.with_network_policy(decider); } // Adaptive evidence routing is engine-native and opt-in @@ -7420,8 +7447,8 @@ impl Engine { } pool = pool.with_backend(crate::mcp::McpBackend::from_config(&self.api_config)); pool = pool.with_disallowed_tools(self.config.disallowed_tools.clone().unwrap_or_default()); - if let Some(decider) = self.config.network_policy.as_ref() { - pool = pool.with_network_policy(decider.clone()); + if let Some(decider) = self.current_network_decider() { + pool = pool.with_network_policy(decider); } // The self-serve login tool honors the same pre-registered redirect // overrides `/mcp login` uses, or providers with pinned callback diff --git a/crates/tui/src/core/engine/tests/test_cases_12.rs b/crates/tui/src/core/engine/tests/test_cases_12.rs index 0028b5b4d4..800283298b 100644 --- a/crates/tui/src/core/engine/tests/test_cases_12.rs +++ b/crates/tui/src/core/engine/tests/test_cases_12.rs @@ -1581,4 +1581,76 @@ fn turn_tool_context_uses_planned_authority_and_route_not_installed_session() { .model, "planned-next-model" ); +} + +/// `/network allow ` edits `config.toml` and tells the operator to retry +/// the command. The engine holds its policy by value, so that retry only works +/// when the tool context re-reads the document — this is the regression guard +/// for a session that kept refusing a host the file already allowed. +#[test] +fn tool_context_network_policy_follows_the_config_document_mid_session() { + use crate::network_policy::{Decision, DecisionToml, NetworkPolicy, NetworkPolicyDecider}; + + let dir = tempdir().expect("temp dir"); + let config_path = dir.path().join("config.toml"); + fs::write( + &config_path, + "[network]\ndefault = \"prompt\"\nallow = [\"api.github.com\"]\n", + ) + .expect("write config"); + + let api_config = Config { + loaded_config_path: Some(config_path), + ..Config::default() + }; + + // The snapshot the engine was spawned with does not allow the host yet. + let engine_config = EngineConfig { + network_policy: Some(NetworkPolicyDecider::new( + NetworkPolicy { + default: DecisionToml::Prompt, + ..NetworkPolicy::default() + }, + None, + )), + ..EngineConfig::default() + }; + let (engine, _handle) = Engine::new(engine_config, &api_config); + + let authority = crate::core::authority::TurnAuthority::from_effective_fields( + AppMode::Agent, + true, + true, + true, + ApprovalMode::Bypass, + ); + let route = TurnRouteContext { + provider: ProviderKind::Deepseek, + model: "network-refresh-model".to_string(), + capabilities: codewhale_config::route::RouteCapabilities::default(), + limits: None, + client: None, + api_config: Box::new(Config::default()), + locale_tag: engine.config.locale_tag.clone(), + role_models: engine.subagent_role_models(), + auto_model: false, + reasoning_effort: None, + reasoning_effort_auto: false, + }; + + let context = engine.build_tool_context_for_turn(&authority, &route); + let decider = context + .network_policy + .as_ref() + .expect("the configured policy is injected"); + assert_eq!( + decider.evaluate("api.github.com", "Bash"), + Decision::Allow, + "the document already allows the host; the retry must see it" + ); + assert_eq!( + decider.evaluate("unlisted.example.com", "Bash"), + Decision::Prompt, + "a host the document does not name still prompts" + ); } \ No newline at end of file diff --git a/crates/tui/src/network_policy.rs b/crates/tui/src/network_policy.rs index 3fa32375a9..207db131ed 100644 --- a/crates/tui/src/network_policy.rs +++ b/crates/tui/src/network_policy.rs @@ -524,6 +524,42 @@ impl NetworkPolicyDecider { Self::new(policy, auditor) } + /// Rebuild this decider against a policy re-read from disk, keeping the + /// session cache so an approval already granted in this session survives + /// the refresh. + /// + /// The engine snapshots its policy when it is spawned, and `/network allow + /// ` only edits the configuration document — it cannot reach that + /// snapshot. Callers hand the re-read table here instead of waiting for a + /// restart. The auditor keeps its log path and follows the new policy's + /// `audit` switch. + /// + /// Extra CIDRs registered through [`Self::with_trusted_fakeip_cidrs`] are + /// not carried over; the refresh re-derives them from `policy`. Configured + /// fake-IP ranges already ride in the policy, and no production caller + /// registers extras. + #[must_use] + pub fn with_policy_refreshed(&self, policy: NetworkPolicy) -> Self { + let auditor = match self.auditor.as_ref() { + Some(existing) => Some(NetworkAuditor::new( + existing.path().to_path_buf(), + policy.audit_enabled(), + )), + None if policy.audit_enabled() => NetworkAuditor::default_path(true), + None => None, + }; + Self { + trusted_fakeip_cidrs: policy + .proxy_fake_ip_cidrs + .iter() + .filter_map(|cidr| parse_trusted_fakeip_cidr(cidr)) + .collect(), + policy, + cache: self.cache.clone(), + auditor, + } + } + /// Inspect the policy. #[must_use] pub fn policy(&self) -> &NetworkPolicy { @@ -844,6 +880,51 @@ mod tests { ); } + #[test] + fn refreshed_policy_adopts_new_table_without_retracting_session_approvals() { + let decider = NetworkPolicyDecider::new(mk(Decision::Prompt, &[], &[]), None); + decider.approve_session("approved.example.com", "fetch_url"); + + // Mid-session `/network allow api.github.com` reaches the session as a + // re-read table rather than a restart. + let refreshed = + decider.with_policy_refreshed(mk(Decision::Prompt, &["api.github.com"], &[])); + + assert_eq!( + refreshed.evaluate("api.github.com", "Bash"), + Decision::Allow, + "the re-read table is in force" + ); + assert_eq!( + refreshed.evaluate("approved.example.com", "fetch_url"), + Decision::Allow, + "an approval already granted this session survives the refresh" + ); + assert_eq!( + refreshed.evaluate("unlisted.example.com", "Bash"), + Decision::Prompt, + "hosts the table does not name keep the default" + ); + } + + #[test] + fn refreshed_policy_honours_a_new_deny_list() { + let decider = NetworkPolicyDecider::new(mk(Decision::Allow, &[], &[]), None); + assert_eq!( + decider.evaluate("evil.example.com", "fetch_url"), + Decision::Allow + ); + + let refreshed = + decider.with_policy_refreshed(mk(Decision::Allow, &[], &["evil.example.com"])); + + assert_eq!( + refreshed.evaluate("evil.example.com", "fetch_url"), + Decision::Deny, + "a deny entry written after spawn is enforced without a restart" + ); + } + #[test] fn approve_persistent_writes_back_to_policy() { let policy = mk(Decision::Prompt, &[], &[]); From 0379673f8bdfc2f0660db6abb007aaabd80c99a4 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 02:21:23 +0800 Subject: [PATCH 2/4] fix(tui): a resolved network authority is not the document's to widen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut re-read `[network]` from the user's document and treated the result as the live policy. That document is only one layer of it: a managed overlay replaces the user table, and a Fleet `network_access = false` never consults it at all — `exec_network_policy` synthesizes a `default = "deny"` decider for exactly that case. Replacing the resolved policy with the raw document therefore widened an administrator's denial to prompt/allow on the first tool-context build, which is every turn. `Config` now carries a runtime-only receipt that a managed overlay supplied `[network]`, and the Fleet-synthesized decider is marked authoritative. When either holds, a mid-session re-read is folded onto the resolved policy instead of replacing it: the document's `allow` list is adopted — `allow ` still works — but the fallback keeps the stricter of the two decisions and a denial is never dropped. This also lands the deny direction: a session that started with no policy at all adopts the table the moment one exists, so `/network deny ` is not ignored in the case where a policy is being tightened. Tests: `folding_a_document_never_widens_an_authority` covers the fold (deny kept, fallback unchanged, allow adopted), `refreshed_policy_keeps_the_ authority_marking` covers the marker surviving a refresh, and `tool_context_adopts_a_policy_written_mid_session_and_never_ungates_one` covers adoption plus the reverse direction (removing the table leaves a gated session gated). Found by the review on this PR. --- crates/tui/src/config.rs | 15 +++ crates/tui/src/core/engine.rs | 50 +++++-- .../src/core/engine/tests/test_cases_12.rs | 80 +++++++++++ crates/tui/src/lib.rs | 20 ++- crates/tui/src/network_policy.rs | 127 ++++++++++++++++++ 5 files changed, 273 insertions(+), 19 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 689e6d39fb..4c5bbe3d41 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -2595,6 +2595,13 @@ pub struct Config { #[serde(skip)] pub loaded_config_path: Option, + /// Runtime-only receipt that a higher-precedence layer supplied `[network]` + /// — a managed overlay. The resolved policy is then not the user + /// document's to replace: a mid-session re-read of that document must not + /// widen what the higher layer set. + #[serde(skip)] + pub(crate) network_layer_is_managed: bool, + /// A resolved startup snapshot never reads remembered route choices again. /// False means an explicit config/profile owns the route instead. #[serde(skip)] @@ -10474,6 +10481,8 @@ fn merge_config(base: Config, override_cfg: Config) -> Config { notifications: override_cfg.notifications.or(base.notifications), approval: override_cfg.approval.or(base.approval), network: override_cfg.network.or(base.network), + network_layer_is_managed: override_cfg.network_layer_is_managed + || base.network_layer_is_managed, verifier: override_cfg.verifier.or(base.verifier), advisor: override_cfg.advisor.or(base.advisor), skills: merge_skills_config(base.skills, override_cfg.skills), @@ -10999,6 +11008,12 @@ fn apply_managed_overrides(config: &mut Config) -> Result<()> { } merged.base_url_env_receipt = BaseUrlEnvReceipt::NoOwner; } + // A managed `[network]` table outranks the user document. Record that the + // resolved policy is not the document's to replace, so a mid-session + // re-read of that document folds onto it instead of widening it. + if managed.network.is_some() { + merged.network_layer_is_managed = true; + } *config = merged; Ok(()) } diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index e7cd46b3e1..d56411c750 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -7104,8 +7104,8 @@ impl Engine { ) } - /// The network decider this turn runs under: the configured policy, - /// re-read from the document the session was launched with. + /// The network decider this turn runs under: the live policy, re-read from + /// the document the session was launched with. /// /// `self.config.network_policy` is the snapshot `Engine::new` took when the /// session was spawned. `/network allow ` edits `config.toml` and @@ -7115,19 +7115,45 @@ impl Engine { /// re-reads per tool-context build so `/trust add` lands mid-session; a /// hand edit of `[network]` lands through it too. /// - /// The session cache rides along, so a host approved through the approval - /// prompt survives the refresh. A session launched without a `[network]` - /// table stays ungated on purpose: adopting one from disk here would hand - /// that session a `default = "prompt"` policy it never had, and every - /// unrelated host would start prompting. + /// Both directions have to land. `/network deny ` writes the same + /// `[network]` table `/network allow` does, and a session that started + /// without one adopts it the moment it appears — otherwise tightening a + /// policy mid-session would need the restart this exists to remove. + /// + /// A session that started gated keeps the policy it holds when the document + /// stops carrying a `[network]` table, so removing the table cannot leave a + /// session ungated by accident. + /// + /// The session cache rides along on a refresh, so a host approved through + /// the approval prompt survives it. An adopted table has no cache to + /// inherit: the session had no decider, so nothing was ever approved under + /// one. fn current_network_decider(&self) -> Option { - let decider = self.config.network_policy.as_ref()?; let Some(path) = self.api_config.loaded_config_path.as_deref() else { - return Some(decider.clone()); + return self.config.network_policy.clone(); + }; + let Some(document) = crate::config::network_policy_from_document(path) else { + // The document carries no `[network]` table. Keep what the session + // resolved: removing a table must not ungate a gated run. + return self.config.network_policy.clone(); }; - match crate::config::network_policy_from_document(path) { - Some(policy) => Some(decider.with_policy_refreshed(policy)), - None => Some(decider.clone()), + match self.config.network_policy.as_ref() { + // A managed overlay or a Fleet denial produced this policy, so the + // document is a lower layer: it may add hosts, but it may not widen + // the fallback or lift a denial. + Some(decider) + if decider.is_authoritative() || self.api_config.network_layer_is_managed => + { + Some( + decider + .with_policy_refreshed(decider.policy().folded_with_lower_layer(document)), + ) + } + Some(decider) => Some(decider.with_policy_refreshed(document)), + // Nothing resolved a policy for this session, so there is no higher + // layer to protect. Adopt the table the moment it exists — that is + // how `/network deny ` lands where there was no policy yet. + None => Some(crate::network_policy::NetworkPolicyDecider::with_default_audit(document)), } } diff --git a/crates/tui/src/core/engine/tests/test_cases_12.rs b/crates/tui/src/core/engine/tests/test_cases_12.rs index 800283298b..ca9d2fbc25 100644 --- a/crates/tui/src/core/engine/tests/test_cases_12.rs +++ b/crates/tui/src/core/engine/tests/test_cases_12.rs @@ -1653,4 +1653,84 @@ fn tool_context_network_policy_follows_the_config_document_mid_session() { Decision::Prompt, "a host the document does not name still prompts" ); +} + +/// `/network deny ` writes the same `[network]` table `/network allow` +/// does. A session that started without one has to adopt it — otherwise +/// tightening the policy mid-session needs the restart this fix removes — and a +/// session that started gated must not lose its policy because the table went +/// away. +#[test] +fn tool_context_adopts_a_policy_written_mid_session_and_never_ungates_one() { + use crate::network_policy::{Decision, DecisionToml, NetworkPolicy, NetworkPolicyDecider}; + + let dir = tempdir().expect("temp dir"); + let config_path = dir.path().join("config.toml"); + fs::write( + &config_path, + "[network]\ndefault = \"prompt\"\ndeny = [\"evil.example.com\"]\n", + ) + .expect("write config"); + + let api_config = Config { + loaded_config_path: Some(config_path.clone()), + ..Config::default() + }; + + // A session that started with no `[network]` table at all adopts the deny. + let (engine, _handle) = Engine::new(EngineConfig::default(), &api_config); + let authority = crate::core::authority::TurnAuthority::from_effective_fields( + AppMode::Agent, + true, + true, + true, + ApprovalMode::Bypass, + ); + let route = TurnRouteContext { + provider: ProviderKind::Deepseek, + model: "network-adopt-model".to_string(), + capabilities: codewhale_config::route::RouteCapabilities::default(), + limits: None, + client: None, + api_config: Box::new(Config::default()), + locale_tag: engine.config.locale_tag.clone(), + role_models: engine.subagent_role_models(), + auto_model: false, + reasoning_effort: None, + reasoning_effort_auto: false, + }; + let context = engine.build_tool_context_for_turn(&authority, &route); + let decider = context + .network_policy + .as_ref() + .expect("a table written mid-session is adopted"); + assert_eq!( + decider.evaluate("evil.example.com", "Bash"), + Decision::Deny, + "the deny direction must land mid-session too" + ); + + // Removing the table must not ungate a session that started gated. + fs::write(&config_path, "# `[network]` removed\n").expect("rewrite config"); + let engine_config = EngineConfig { + network_policy: Some(NetworkPolicyDecider::new( + NetworkPolicy { + default: DecisionToml::Deny, + ..NetworkPolicy::default() + }, + None, + )), + ..EngineConfig::default() + }; + let (gated, _handle) = Engine::new(engine_config, &api_config); + let context = gated.build_tool_context_for_turn(&authority, &route); + let decider = context + .network_policy + .as_ref() + .expect("a gated session stays gated"); + assert_eq!( + decider.evaluate("unlisted.example.com", "Bash"), + Decision::Deny, + "removing the table must not hand a gated session an open policy" + ); } \ No newline at end of file diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 64eb9a00b1..e1023ca55b 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -14734,13 +14734,19 @@ fn exec_network_policy( // Fleet caps are an outer authority boundary: user configuration may // narrow them further, but it may never widen an explicit network denial. if outer_network_access == Some(false) { - return Some(crate::network_policy::NetworkPolicyDecider::new( - crate::network_policy::NetworkPolicy { - default: crate::network_policy::DecisionToml::Deny, - ..crate::network_policy::NetworkPolicy::default() - }, - None, - )); + // A Fleet denial is an outer authority the user's document may never + // widen, so mark the decider authoritative: a mid-session re-read of + // that document folds onto it instead of replacing it. + return Some( + crate::network_policy::NetworkPolicyDecider::new( + crate::network_policy::NetworkPolicy { + default: crate::network_policy::DecisionToml::Deny, + ..crate::network_policy::NetworkPolicy::default() + }, + None, + ) + .with_authoritative(), + ); } config.network.clone().map(|toml_cfg| { crate::network_policy::NetworkPolicyDecider::with_default_audit(toml_cfg.into_runtime()) diff --git a/crates/tui/src/network_policy.rs b/crates/tui/src/network_policy.rs index 207db131ed..82e641ba5d 100644 --- a/crates/tui/src/network_policy.rs +++ b/crates/tui/src/network_policy.rs @@ -122,6 +122,23 @@ fn default_decision() -> DecisionToml { DecisionToml::Prompt } +/// The stricter of two fallback decisions, ordering `Deny` < `Prompt` < `Allow`. +/// Used when a lower-precedence layer is folded onto a policy an authority set. +fn stricter_decision(left: DecisionToml, right: DecisionToml) -> DecisionToml { + fn rank(decision: DecisionToml) -> u8 { + match decision { + DecisionToml::Deny => 0, + DecisionToml::Prompt => 1, + DecisionToml::Allow => 2, + } + } + if rank(left) <= rank(right) { + left + } else { + right + } +} + fn default_audit() -> bool { true } @@ -171,6 +188,33 @@ impl From for DecisionToml { } impl NetworkPolicy { + /// Fold a lower-precedence layer (the user's config document) onto this + /// policy without letting it widen what a higher authority set. + /// + /// A `deny` entry is never dropped, and the fallback keeps the stricter of + /// the two decisions. The lower layer's `allow` list is adopted, so it can + /// grant hosts that the fallback would otherwise prompt for — which is what + /// `/network allow ` writes — but it cannot make an unlisted host + /// more permissive than the authority already decided, and it cannot lift a + /// denial. + #[must_use] + pub fn folded_with_lower_layer(&self, lower: Self) -> Self { + let mut deny = self.deny.clone(); + for entry in lower.deny { + if !deny.iter().any(|existing| existing == &entry) { + deny.push(entry); + } + } + Self { + default: stricter_decision(self.default, lower.default), + allow: lower.allow, + deny, + proxy: lower.proxy, + proxy_fake_ip_cidrs: lower.proxy_fake_ip_cidrs, + audit: lower.audit, + } + } + /// Decide what to do for a single outbound call to `host`. /// /// **Deny-wins precedence**: if `host` matches any entry in `deny`, the @@ -461,6 +505,12 @@ pub struct NetworkPolicyDecider { /// A resolved IP inside one of these ranges bypasses the restricted-IP SSRF /// block; real private/loopback/link-local/metadata IPs are unaffected. trusted_fakeip_cidrs: Vec<(Ipv4Addr, u8)>, + /// The policy was produced by an authority the user's config document may + /// not override — a managed layer, or a Fleet denial the user is not + /// allowed to widen. A mid-session re-read of that document then folds onto + /// this policy instead of replacing it. See + /// [`NetworkPolicy::folded_with_lower_layer`]. + authoritative: bool, } impl NetworkPolicyDecider { @@ -477,9 +527,24 @@ impl NetworkPolicyDecider { cache: NetworkSessionCache::new(), auditor, trusted_fakeip_cidrs, + authoritative: false, } } + /// Mark this policy as set by an authority the user document may not + /// override. See the `authoritative` field. + #[must_use] + pub fn with_authoritative(mut self) -> Self { + self.authoritative = true; + self + } + + /// Whether an authority set this policy rather than the user document. + #[must_use] + pub fn is_authoritative(&self) -> bool { + self.authoritative + } + /// Register IPv4 CIDR ranges to treat as benign fake-IP placeholders. /// Invalid CIDR strings are skipped. See [`Self::is_trusted_fakeip_addr`]. #[must_use] @@ -557,6 +622,7 @@ impl NetworkPolicyDecider { policy, cache: self.cache.clone(), auditor, + authoritative: self.authoritative, } } @@ -925,6 +991,67 @@ mod tests { ); } + #[test] + fn refreshed_policy_keeps_the_authority_marking() { + let decider = + NetworkPolicyDecider::new(mk(Decision::Deny, &[], &[]), None).with_authoritative(); + assert!(decider.is_authoritative()); + let refreshed = decider.with_policy_refreshed(mk(Decision::Allow, &[], &[])); + assert!( + refreshed.is_authoritative(), + "a refreshed authority is still an authority" + ); + } + + #[test] + fn folding_a_document_never_widens_an_authority() { + // What a Fleet denial or a managed overlay resolved to. + let authority = NetworkPolicy { + default: DecisionToml::Deny, + deny: vec!["blocked.example.com".to_string()], + ..NetworkPolicy::default() + }; + // What the user's document asks for on a mid-session re-read. + let document = NetworkPolicy { + default: DecisionToml::Allow, + allow: vec!["api.github.com".to_string()], + ..NetworkPolicy::default() + }; + + let folded = authority.folded_with_lower_layer(document); + + assert_eq!( + folded.default, + DecisionToml::Deny, + "the document must not widen an authority's fallback" + ); + assert!( + folded.deny.iter().any(|host| host == "blocked.example.com"), + "a denial the authority set survives the fold" + ); + assert!( + folded.allow.iter().any(|host| host == "api.github.com"), + "the document may still name hosts the authority did not deny" + ); + + // The fold is conservative by construction; that is why it is used only + // when an authority owns the policy. The plain user path replaces the + // policy outright, so `/network default allow` still lands there. + let plain = NetworkPolicy { + default: DecisionToml::Prompt, + ..NetworkPolicy::default() + }; + assert_eq!( + plain + .folded_with_lower_layer(NetworkPolicy { + default: DecisionToml::Allow, + ..NetworkPolicy::default() + }) + .default, + DecisionToml::Prompt + ); + } + #[test] fn approve_persistent_writes_back_to_policy() { let policy = mk(Decision::Prompt, &[], &[]); From 5a2fa012e483d33b2785b1d4aa5808bb5705e9b0 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 04:50:14 +0800 Subject: [PATCH 3/4] fix(tui): a deny-by-default authority keeps the last word on allow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fold added in the previous commit hardened `default` and `deny` but took the document's `allow`, `proxy`, `proxy_fake_ip_cidrs`, and `audit` wholesale. `decide` checks `allow` *before* falling back to `default`, so under a deny-by-default authority — the canonical managed / Fleet stance — a document listing `allow = ["api.github.com"]` still produced `Allow`. The fold was escapable in the one direction it exists to protect, and the review that found it also caught that the case's own test asserted the leak: it checked `folded.default` stayed `Deny` and that the document's host was in `allow`, never what `decide` returned. The fold is now asymmetric by the authority's own fallback: - `deny`: union. Either layer saying no is a no. - `default`: the stricter of the two, as before. - Authority `default = Deny`: the lower layer contributes nothing that can allow. `allow`, `proxy`, and the fake-IP CIDRs stay the authority's own — no host lifted, no SSRF exception granted. - Authority `default = Prompt`: the lower layer may name hosts, which is what `/network allow ` writes. Those **merge** with the authority's own instead of replacing them, so a refresh cannot erase an administrator's allowances either. - `audit`: either layer asking for the log keeps it. A document cannot silence audit under an authority. `folding_a_document_never_widens_an_authority` now asserts `decide()` verdicts for both fallbacks rather than field shape: the document's host is `Deny` under a denial and `Allow` under a prompt, the authority's own allowance survives, an unlisted host keeps the authority's fallback, and no fake-IP exception or audit silencing crosses the fold. `cargo test -p codewhale-tui network_policy` -> 40 passed (lib) + 28 passed (integration); `cargo test -p codewhale-tui tool_context` -> 24 passed. --- crates/tui/src/network_policy.rs | 131 +++++++++++++++++++++++++------ 1 file changed, 105 insertions(+), 26 deletions(-) diff --git a/crates/tui/src/network_policy.rs b/crates/tui/src/network_policy.rs index 82e641ba5d..503edb73b4 100644 --- a/crates/tui/src/network_policy.rs +++ b/crates/tui/src/network_policy.rs @@ -191,27 +191,56 @@ impl NetworkPolicy { /// Fold a lower-precedence layer (the user's config document) onto this /// policy without letting it widen what a higher authority set. /// - /// A `deny` entry is never dropped, and the fallback keeps the stricter of - /// the two decisions. The lower layer's `allow` list is adopted, so it can - /// grant hosts that the fallback would otherwise prompt for — which is what - /// `/network allow ` writes — but it cannot make an unlisted host - /// more permissive than the authority already decided, and it cannot lift a - /// denial. + /// The fold is deliberately asymmetric, because the two layers are not + /// peers: + /// + /// * `deny` is the **union**. Either layer saying no is a no. + /// * `default` keeps the **stricter** of the two. + /// * When the authority's fallback is `Deny`, the lower layer contributes + /// **nothing that can allow**: `allow`, `proxy`, and the fake-IP CIDRs + /// stay the authority's own. `decide` checks `allow` *before* falling back + /// to `default`, so adopting a lower `allow` entry under a + /// deny-by-default authority would let the document lift exactly the + /// denial the authority set. + /// * Otherwise the authority is prompt-by-default, and the lower layer may + /// do what `/network allow ` writes: name additional hosts. Those + /// merge with the authority's own rather than replacing them, so a + /// refresh cannot erase an administrator's allowances either. + /// * `audit` is the one field that widens by `true`: either layer asking for + /// the audit log keeps it, and a document cannot silence it under an + /// authority. #[must_use] pub fn folded_with_lower_layer(&self, lower: Self) -> Self { - let mut deny = self.deny.clone(); - for entry in lower.deny { - if !deny.iter().any(|existing| existing == &entry) { - deny.push(entry); + fn union(mut base: Vec, extra: Vec) -> Vec { + for entry in extra { + if !base.iter().any(|existing| existing == &entry) { + base.push(entry); + } } + base } + + let deny_by_default = self.default == DecisionToml::Deny; + let (allow, proxy, proxy_fake_ip_cidrs) = if deny_by_default { + ( + self.allow.clone(), + self.proxy.clone(), + self.proxy_fake_ip_cidrs.clone(), + ) + } else { + ( + union(self.allow.clone(), lower.allow), + union(self.proxy.clone(), lower.proxy), + union(self.proxy_fake_ip_cidrs.clone(), lower.proxy_fake_ip_cidrs), + ) + }; Self { default: stricter_decision(self.default, lower.default), - allow: lower.allow, - deny, - proxy: lower.proxy, - proxy_fake_ip_cidrs: lower.proxy_fake_ip_cidrs, - audit: lower.audit, + allow, + deny: union(self.deny.clone(), lower.deny), + proxy, + proxy_fake_ip_cidrs, + audit: self.audit || lower.audit, } } @@ -1005,7 +1034,11 @@ mod tests { #[test] fn folding_a_document_never_widens_an_authority() { - // What a Fleet denial or a managed overlay resolved to. + // Assert what the folded policy *decides*, not the shape of its fields: + // `decide` checks `allow` before falling back to `default`, so a field + // assertion can look safe while the verdict is `Allow`. + + // What a Fleet denial or a managed overlay resolved to: deny by default. let authority = NetworkPolicy { default: DecisionToml::Deny, deny: vec!["blocked.example.com".to_string()], @@ -1015,28 +1048,74 @@ mod tests { let document = NetworkPolicy { default: DecisionToml::Allow, allow: vec!["api.github.com".to_string()], + proxy: vec!["proxy.example.com".to_string()], + proxy_fake_ip_cidrs: vec!["198.18.0.0/15".to_string()], + audit: false, ..NetworkPolicy::default() }; let folded = authority.folded_with_lower_layer(document); assert_eq!( - folded.default, - DecisionToml::Deny, - "the document must not widen an authority's fallback" + folded.decide("api.github.com"), + Decision::Deny, + "a document cannot lift a deny-by-default authority's denial" ); - assert!( - folded.deny.iter().any(|host| host == "blocked.example.com"), + assert_eq!( + folded.decide("blocked.example.com"), + Decision::Deny, "a denial the authority set survives the fold" ); + assert_eq!( + folded.decide("unlisted.example.com"), + Decision::Deny, + "an unlisted host still falls back to the authority's denial" + ); assert!( - folded.allow.iter().any(|host| host == "api.github.com"), - "the document may still name hosts the authority did not deny" + folded.proxy.is_empty() && folded.proxy_fake_ip_cidrs.is_empty(), + "a document cannot hand itself a fake-IP SSRF exception under a denial" + ); + assert!( + folded.audit, + "a document cannot silence the audit log under an authority" + ); + + // A prompt-by-default authority is the case `/network allow` exists for: + // the document may name hosts, and it may not erase the authority's own. + let prompt_authority = NetworkPolicy { + default: DecisionToml::Prompt, + allow: vec!["internal.example.com".to_string()], + deny: vec!["blocked.example.com".to_string()], + ..NetworkPolicy::default() + }; + let folded = prompt_authority.folded_with_lower_layer(NetworkPolicy { + default: DecisionToml::Allow, + allow: vec!["api.github.com".to_string()], + ..NetworkPolicy::default() + }); + assert_eq!( + folded.decide("api.github.com"), + Decision::Allow, + "the document may still name hosts when the authority prompts" + ); + assert_eq!( + folded.decide("internal.example.com"), + Decision::Allow, + "the authority's own allowance survives the fold" + ); + assert_eq!( + folded.decide("unlisted.example.com"), + Decision::Prompt, + "the document cannot widen the authority's fallback to allow" + ); + assert_eq!( + folded.decide("blocked.example.com"), + Decision::Deny, + "either layer's denial wins" ); - // The fold is conservative by construction; that is why it is used only - // when an authority owns the policy. The plain user path replaces the - // policy outright, so `/network default allow` still lands there. + // The plain user path replaces the policy outright, so + // `/network default allow` still lands there. let plain = NetworkPolicy { default: DecisionToml::Prompt, ..NetworkPolicy::default() From 1b9cf23f15991b1770b2ba43a28f3e5b89fc1403 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 05:01:28 +0800 Subject: [PATCH 4/4] test(tui): pin the allow-by-default fold's verdicts The review noted the authority-Allow fallback branch was correct but unpinned: a managed overlay may legitimately resolve `default = "allow"`, and its fold takes the union branch. The test asserts the document's own `default = deny` narrows it and a named host stays allowed -- nothing there is more permissive than the authority's own policy, because that policy already allowed everything. `cargo test -p codewhale-tui network_policy` -> 41 passed (lib) + 29 passed (integration). --- crates/tui/src/network_policy.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/tui/src/network_policy.rs b/crates/tui/src/network_policy.rs index 503edb73b4..2f43cf6aed 100644 --- a/crates/tui/src/network_policy.rs +++ b/crates/tui/src/network_policy.rs @@ -1131,6 +1131,34 @@ mod tests { ); } + /// A managed overlay may legitimately resolve `default = "allow"`. Its fold + /// takes the union branch, and the document's own fallback can only narrow + /// it — nothing there is more permissive than the authority's own policy, + /// because that policy already allowed everything. + #[test] + fn folding_an_allow_by_default_authority_only_narrows() { + let authority = NetworkPolicy { + default: DecisionToml::Allow, + ..NetworkPolicy::default() + }; + let folded = authority.folded_with_lower_layer(NetworkPolicy { + default: DecisionToml::Deny, + allow: vec!["named.example.com".to_string()], + ..NetworkPolicy::default() + }); + + assert_eq!( + folded.decide("named.example.com"), + Decision::Allow, + "the authority allowed everything already; naming a host is not a widening" + ); + assert_eq!( + folded.decide("other.example.com"), + Decision::Deny, + "the document narrowed the fallback, which is the direction it may move" + ); + } + #[test] fn approve_persistent_writes_back_to_policy() { let policy = mk(Decision::Prompt, &[], &[]);