diff --git a/crates/openshell-tui/src/ui/sandbox_draft.rs b/crates/openshell-tui/src/ui/sandbox_draft.rs index 470463b7d4..fc299523c7 100644 --- a/crates/openshell-tui/src/ui/sandbox_draft.rs +++ b/crates/openshell-tui/src/ui/sandbox_draft.rs @@ -328,6 +328,11 @@ pub fn draw_detail_popup( text_width, ); + if let Some(warning) = scope_warning(ep) { + let warn = t.status_warn.add_modifier(Modifier::BOLD); + push_wrapped(&mut lines, " ! ", warn, warning, warn, text_width); + } + for detail in format_endpoint_details(ep) { push_wrapped(&mut lines, " ", t.text, &detail, t.text, text_width); } @@ -863,6 +868,41 @@ fn format_endpoint_details(endpoint: &NetworkEndpoint) -> Vec { details } +/// Why a proposed endpoint is broader than the denial that prompted it. +/// +/// An endpoint with no protocol is not a raw tunnel: per +/// `openshell_policy::agent_authored_transport_rejection`, omitting the protocol +/// keeps the explicit proxy, which terminates TLS and canonicalizes the HTTP +/// authority, while `protocol: tcp` and `tls: skip` cannot be agent-authored at +/// all. What it lacks is any method or path constraint, so every request to the +/// host and port is permitted. A REST endpoint with no allow rules, or with an +/// allow rule that leaves the method or path unset — rendered as `*` by +/// `format_allow_rule` — is broad for the same reason. Protocols other than REST +/// scope on `command` instead, so they are left alone rather than warned about +/// incorrectly. +fn scope_warning(endpoint: &NetworkEndpoint) -> Option<&'static str> { + if endpoint.protocol.trim().is_empty() { + return Some( + "no protocol set: every request to this host and port is allowed, not only the denied one", + ); + } + if !endpoint.protocol.eq_ignore_ascii_case("rest") { + return None; + } + if endpoint.rules.is_empty() { + return Some("no method or path scope: allows every request to this host"); + } + let unscoped = endpoint + .rules + .iter() + .filter_map(|rule| rule.allow.as_ref()) + .any(|allow| allow.method.trim().is_empty() || allow.path.trim().is_empty()); + if unscoped { + return Some("an allow rule leaves the method or path unset (*), widening the scope"); + } + None +} + fn endpoint_layer_label(endpoint: &NetworkEndpoint) -> &str { if endpoint.protocol.eq_ignore_ascii_case("rest") { "L7 rest" @@ -976,6 +1016,7 @@ fn format_short_time(epoch_ms: i64) -> String { mod tests { use super::*; use crate::theme::Theme; + use openshell_core::proto::{L7Rule, NetworkPolicyRule}; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -1363,4 +1404,115 @@ mod tests { let rows = pack_hints(&hint_units(&chunk, &theme, true), 30); assert!(rows.len() > 1, "hints should wrap at 30 columns"); } + + // --- L4 / no-method-path scope warning --------------------------------- + + fn scoped_endpoint(protocol: &str, rules: Vec) -> NetworkEndpoint { + NetworkEndpoint { + host: "api.github.com".to_string(), + port: 443, + protocol: protocol.to_string(), + rules, + ..Default::default() + } + } + + fn allow_rule(method: &str, path: &str) -> L7Rule { + L7Rule { + allow: Some(L7Allow { + method: method.to_string(), + path: path.to_string(), + ..Default::default() + }), + } + } + + fn chunk_with(endpoint: NetworkEndpoint) -> PolicyChunk { + PolicyChunk { + status: "pending".to_string(), + rule_name: "allow-github".to_string(), + proposed_rule: Some(NetworkPolicyRule { + endpoints: vec![endpoint], + ..Default::default() + }), + ..Default::default() + } + } + + /// Rebuild a matchable string from a rendered buffer. + /// + /// Cells are joined row-major, so a wrapped phrase is split by row padding + /// and by the popup's own box-drawing border. Both become whitespace, then + /// whitespace collapses. + fn squash(text: &str) -> String { + text.chars() + .map(|c| { + if "│┌┐└┘─".contains(c) { + ' ' + } else { + c + } + }) + .collect::() + .split_whitespace() + .collect::>() + .join(" ") + } + + #[test] + fn endpoint_without_a_protocol_is_flagged_as_unscoped() { + let warning = scope_warning(&scoped_endpoint("", vec![])); + assert!(warning.is_some_and(|w| w.starts_with("no protocol set:"))); + } + + #[test] + fn rest_endpoint_without_allow_rules_is_flagged() { + let warning = scope_warning(&scoped_endpoint("rest", vec![])); + assert!(warning.is_some_and(|w| w.contains("no method or path scope"))); + } + + #[test] + fn rest_allow_rule_without_method_or_path_is_flagged() { + for rule in [allow_rule("", "/repos/**"), allow_rule("GET", "")] { + let warning = scope_warning(&scoped_endpoint("rest", vec![rule])); + assert!( + warning.is_some_and(|w| w.contains("leaves the method or path unset")), + "unscoped allow rule should be flagged" + ); + } + } + + #[test] + fn fully_scoped_rest_endpoint_is_not_flagged() { + let endpoint = scoped_endpoint("rest", vec![allow_rule("GET", "/repos/**")]); + assert_eq!(scope_warning(&endpoint), None); + } + + /// Non-REST protocols scope on `command`, so method and path say nothing + /// about how broad they are. + #[test] + fn non_rest_protocol_is_left_alone() { + assert_eq!(scope_warning(&scoped_endpoint("ssh", vec![])), None); + } + + #[test] + fn scope_warning_is_rendered_in_the_detail_popup() { + let (screen, _) = render(&chunk_with(scoped_endpoint("", vec![])), 80, 24, 0); + assert!( + squash(&screen).contains("every request to this host and port is allowed"), + "missing-protocol warning should appear in the popup" + ); + } + + #[test] + fn a_scoped_endpoint_renders_no_warning() { + let endpoint = scoped_endpoint("rest", vec![allow_rule("GET", "/repos/**")]); + let (screen, _) = render(&chunk_with(endpoint), 80, 24, 0); + let seen = squash(&screen); + assert!( + !seen.contains("every request to this"), + "unexpected warning: {seen}" + ); + assert!(!seen.contains("no method or path scope")); + } } diff --git a/docs/sandboxes/policy-advisor.mdx b/docs/sandboxes/policy-advisor.mdx index 80884620d5..0d127d6fc4 100644 --- a/docs/sandboxes/policy-advisor.mdx +++ b/docs/sandboxes/policy-advisor.mdx @@ -232,6 +232,8 @@ The output shows the chunk ID, status, rationale, binary, endpoint summary, prov Endpoints: api.github.com:443 [L7 rest, allow PUT /repos/NVIDIA/OpenShell/contents/docs/**] ``` +The terminal UI flags proposals that are broader than the request that was denied. In the detail popup, `openshell term` shows a warning under any endpoint that omits `protocol`, that is REST with no allow rules, or whose allow rule leaves the method or path unset. Omitting `protocol` still routes through the explicit proxy, so the warning is about the missing method and path scope rather than raw transport. Protocols other than REST scope on `command`, so they are not flagged. + Approve only when the structured rule matches the access you intend to grant: ```shell