Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions crates/openshell-tui/src/ui/sandbox_draft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -863,6 +868,41 @@ fn format_endpoint_details(endpoint: &NetworkEndpoint) -> Vec<String> {
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"
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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<L7Rule>) -> 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::<String>()
.split_whitespace()
.collect::<Vec<_>>()
.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"));
}
}
2 changes: 2 additions & 0 deletions docs/sandboxes/policy-advisor.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading