From 2d7dd7a2455c5e57f8fba8cfec42059dcf57a3e3 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 26 Aug 2026 06:51:59 +0700 Subject: [PATCH] feat(tui): warn when a proposed endpoint is not method- and path-scoped The draft inbox renders an endpoint's scope but never says when that scope is wide. endpoint_layer_label already tags L4 and format_allow_rule already renders an unset method or path as *, so the breadth is displayed while nothing draws a reviewer's eye to a proposal that grants far more than the request that was denied. Add scope_warning, a pure classifier beside endpoint_layer_label, and render it under the endpoint it describes in the detail popup, styled like the existing security-note line. It flags an L4 endpoint, a REST endpoint with no allow rules, and a REST allow rule that leaves the method or path unset. Protocols other than REST scope on command rather than method and path, so they are left alone rather than warned about incorrectly. This stays in the TUI rather than in generate_security_notes because the gateway's security notes feed auto-approval eligibility, so adding a case there is a policy decision rather than a presentation one. Everything needed to classify the endpoint is already on the client in proposed_rule.endpoints. Part of #1098. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- crates/openshell-tui/src/ui/sandbox_draft.rs | 144 +++++++++++++++++++ docs/sandboxes/policy-advisor.mdx | 2 + 2 files changed, 146 insertions(+) diff --git a/crates/openshell-tui/src/ui/sandbox_draft.rs b/crates/openshell-tui/src/ui/sandbox_draft.rs index 470463b7d4..709655c6e3 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,36 @@ fn format_endpoint_details(endpoint: &NetworkEndpoint) -> Vec { details } +/// Why a proposed endpoint is broader than the denial that prompted it. +/// +/// An L4 endpoint allows the whole TCP port. 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` — still permits far more than the single +/// request that was denied. 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( + "L4 rule: allows every connection to this host and port, not only the denied request", + ); + } + 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 +1011,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 +1399,112 @@ 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 l4_endpoint_is_flagged_as_unscoped() { + let warning = scope_warning(&scoped_endpoint("", vec![])); + assert!(warning.is_some_and(|w| w.starts_with("L4 rule:"))); + } + + #[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("allows every connection to this host and port"), + "L4 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("allows every"), "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 b4d607cb33..c1adedd7c2 100644 --- a/docs/sandboxes/policy-advisor.mdx +++ b/docs/sandboxes/policy-advisor.mdx @@ -213,6 +213,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 is L4, that is REST with no allow rules, or whose allow rule leaves the method or path unset. 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