diff --git a/crates/nono-cli/src/exec_strategy.rs b/crates/nono-cli/src/exec_strategy.rs index c8924c146..9dc055fe1 100644 --- a/crates/nono-cli/src/exec_strategy.rs +++ b/crates/nono-cli/src/exec_strategy.rs @@ -25,7 +25,7 @@ use nono::supervisor::{ApprovalDecision, AuditEntry, SupervisorMessage, Supervis use nono::{ ApprovalBackend, CapabilitySet, DenialReason, DenialRecord, NonoError, Result, Sandbox, SessionDiagnosticReport, SupervisorListener, SupervisorSocket, UnixSocketCapability, - UnixSocketMode, + UnixSocketMode, UrlDenialReason, UrlDenialRecord, }; use std::collections::HashSet; use std::ffi::{CString, OsStr}; @@ -76,42 +76,27 @@ const MAX_DENIAL_RECORDS: usize = 1000; const MAX_TRACKED_REQUEST_IDS: usize = 4096; use crate::timeouts; -struct ProfileSaveOffer<'a> { - policy_explanations: &'a [crate::diagnostic::PolicyExplanation], - error_observation: &'a crate::diagnostic::ErrorObservation, - caps: &'a CapabilitySet, - command: &'a [String], - compared_profile: Option<&'a str>, - sandbox_violations: &'a [nono::SandboxViolation], - ignored_denial_paths: &'a [std::path::PathBuf], +pub(crate) struct ProfileSaveOffer<'a> { + pub(crate) policy_explanations: &'a [crate::diagnostic::PolicyExplanation], + pub(crate) error_observation: &'a crate::diagnostic::ErrorObservation, + pub(crate) caps: &'a CapabilitySet, + pub(crate) command: &'a [String], + pub(crate) compared_profile: Option<&'a str>, + pub(crate) sandbox_violations: &'a [nono::SandboxViolation], + pub(crate) ignored_denial_paths: &'a [std::path::PathBuf], + pub(crate) url_denials: &'a [UrlDenialRecord], } fn offer_profile_save_for_child( pty: Option<&mut crate::pty_proxy::PtyProxy>, - offer: ProfileSaveOffer<'_>, + offer: &ProfileSaveOffer<'_>, ) -> Result<()> { if let Some(proxy) = pty { let _released_terminal = proxy.release_terminal_for_prompt(); - return crate::profile_save_runtime::offer_save_run_profile( - offer.policy_explanations, - offer.error_observation, - offer.caps, - offer.command, - offer.compared_profile, - offer.sandbox_violations, - offer.ignored_denial_paths, - ); + return crate::profile_save_runtime::offer_save_run_profile(offer); } - crate::profile_save_runtime::offer_save_run_profile( - offer.policy_explanations, - offer.error_observation, - offer.caps, - offer.command, - offer.compared_profile, - offer.sandbox_violations, - offer.ignored_denial_paths, - ) + crate::profile_save_runtime::offer_save_run_profile(offer) } /// Linux procfs context for resolving child-relative procfs paths in the supervisor. @@ -1328,11 +1313,11 @@ pub fn execute_supervised( }; let mut killed_by_timeout = false; - let (status, denials, ipc_denials) = + let (status, denials, ipc_denials, url_denials) = if let (Some(sup_cfg), Some(mut sup_sock)) = (supervisor, supervisor_sock) { #[cfg(target_os = "linux")] { - let (status, denials, ipc_denials) = run_supervisor_loop( + let (status, denials, ipc_denials, url_denials) = run_supervisor_loop( child, &mut sup_sock, sup_cfg, @@ -1345,11 +1330,11 @@ pub fn execute_supervised( url_listener.as_ref().map(|(l, _)| l), &mut killed_by_timeout, )?; - (status, denials, ipc_denials) + (status, denials, ipc_denials, url_denials) } #[cfg(not(target_os = "linux"))] { - let (status, denials) = run_supervisor_loop( + let (status, denials, url_denials) = run_supervisor_loop( child, &mut sup_sock, sup_cfg, @@ -1359,7 +1344,7 @@ pub fn execute_supervised( url_listener.as_ref().map(|(l, _)| l), &mut killed_by_timeout, )?; - (status, denials, Vec::new()) + (status, denials, Vec::new(), url_denials) } } else { let status = wait_for_child_with_pty( @@ -1368,7 +1353,7 @@ pub fn execute_supervised( config.startup_timeout, &mut killed_by_timeout, )?; - (status, Vec::new(), Vec::new()) + (status, Vec::new(), Vec::new(), Vec::new()) }; // Close the attach listener immediately so no new attach @@ -1541,23 +1526,23 @@ pub fn execute_supervised( &prompt_policy_explanations, &prompt_error_observation, &visible_sandbox_violations, + &url_denials, ) { // Clear the forwarding target before prompting. The child is // already dead; keeping CHILD_PID set would cause forward_signal // to send Ctrl-C to the dead PID, swallowing it silently. clear_signal_forwarding_target(); - offer_profile_save_for_child( - pty_proxy.as_mut(), - ProfileSaveOffer { - policy_explanations: &prompt_policy_explanations, - error_observation: &prompt_error_observation, - caps: config.caps, - command: config.command, - compared_profile: config.profile_save_base, - sandbox_violations: &visible_sandbox_violations, - ignored_denial_paths: config.ignored_denial_paths, - }, - )?; + let offer = ProfileSaveOffer { + policy_explanations: &prompt_policy_explanations, + error_observation: &prompt_error_observation, + caps: config.caps, + command: config.command, + compared_profile: config.profile_save_base, + sandbox_violations: &visible_sandbox_violations, + ignored_denial_paths: config.ignored_denial_paths, + url_denials: &url_denials, + }; + offer_profile_save_for_child(pty_proxy.as_mut(), &offer)?; } Ok(exit_code) @@ -1750,12 +1735,24 @@ fn should_offer_profile_save( policy_explanations: &[crate::diagnostic::PolicyExplanation], error_observation: &crate::diagnostic::ErrorObservation, sandbox_violations: &[nono::SandboxViolation], + url_denials: &[UrlDenialRecord], ) -> bool { !no_diagnostics && (exit_code != 0 || !policy_explanations.is_empty() || !error_observation.path_hints.is_empty() - || crate::profile_save_runtime::has_saveable_system_service_rules(sandbox_violations)) + || crate::profile_save_runtime::has_saveable_system_service_rules(sandbox_violations) + || has_fixable_url_denials(url_denials)) +} + +/// Returns true if any URL denial is fixable (OriginNotAllowed or LocalhostNotAllowed). +fn has_fixable_url_denials(url_denials: &[UrlDenialRecord]) -> bool { + url_denials.iter().any(|r| { + matches!( + r.reason, + UrlDenialReason::OriginNotAllowed | UrlDenialReason::LocalhostNotAllowed + ) + }) } /// Close inherited file descriptors, keeping stdin/stdout/stderr and specified FDs. @@ -2333,7 +2330,7 @@ fn run_supervisor_loop( mut pty: Option<&mut crate::pty_proxy::PtyProxy>, url_listener: Option<&SupervisorListener>, killed_by_timeout: &mut bool, -) -> Result<(WaitStatus, Vec)> { +) -> Result<(WaitStatus, Vec, Vec)> { // Start the macOS tool-sandbox background listener thread (no-op if tool-sandbox not active). #[cfg(target_os = "macos")] if let Some(tool_sandbox_runtime) = config.tool_sandbox_runtime @@ -2345,11 +2342,13 @@ fn run_supervisor_loop( { debug!("tool-sandbox handle_listener error: {e}"); } - let mut sock_fd = sock.as_raw_fd(); let listener_fd = url_listener.map_or(-1, |l| l.as_raw_fd()); - let mut denials = Vec::new(); - let mut seen_request_ids = HashSet::new(); + let mut denials = SupervisorDenials { + fs: Vec::new(), + url: Vec::new(), + seen_request_ids: HashSet::new(), + }; let startup_deadline = startup_timeout.map(|cfg| (Instant::now() + cfg.timeout, cfg)); loop { @@ -2409,7 +2408,6 @@ fn run_supervisor_loop( child, config, &mut denials, - &mut seen_request_ids, trust_interceptor.as_mut(), ) { warn!("Error handling supervisor message: {}", e); @@ -2427,7 +2425,7 @@ fn run_supervisor_loop( && pfds[5].revents & libc::POLLIN != 0 && let Some(listener) = url_listener { - handle_url_listener_connection(listener, config, &mut denials); + handle_url_listener_connection(listener, config, &mut denials.url); } if let Some(ref mut p) = pty @@ -2477,7 +2475,7 @@ fn run_supervisor_loop( ); *killed_by_timeout = true; let _ = signal::kill(child, Signal::SIGKILL); - return Ok((wait_for_child(child)?, denials)); + return Ok((wait_for_child(child)?, denials.fs, denials.url)); } continue; } @@ -2489,11 +2487,11 @@ fn run_supervisor_loop( debug!("Child continued, keeping supervisor alive"); continue; } - Ok(status) => return Ok((status, denials)), + Ok(status) => return Ok((status, denials.fs, denials.url)), Err(nix::errno::Errno::EINTR) => continue, Err(nix::errno::Errno::ECHILD) => { warn!("Child already reaped in supervisor loop"); - return Ok((WaitStatus::Exited(child, 1), denials)); + return Ok((WaitStatus::Exited(child, 1), denials.fs, denials.url)); } Err(e) => { return Err(NonoError::SandboxInit(format!( @@ -2505,9 +2503,19 @@ fn run_supervisor_loop( } let status = wait_for_child(child)?; - Ok((status, denials)) + Ok((status, denials.fs, denials.url)) } +/// Result of the Linux supervisor loop: child wait status plus the filesystem, +/// IPC, and URL denial records collected during the run. +#[cfg(target_os = "linux")] +type SupervisorLoopResult = ( + WaitStatus, + Vec, + Vec, + Vec, +); + /// Supervisor IPC event loop for capability expansion (Linux). /// /// Multiplexes between: @@ -2540,11 +2548,7 @@ fn run_supervisor_loop( mut pty: Option<&mut crate::pty_proxy::PtyProxy>, url_listener: Option<&SupervisorListener>, killed_by_timeout: &mut bool, -) -> Result<( - WaitStatus, - Vec, - Vec, -)> { +) -> Result { struct LoopTimer { start: Instant, iterations: u64, @@ -2581,9 +2585,12 @@ fn run_supervisor_loop( let tool_sandbox_url_listener_fd = tool_sandbox_runtime.and_then(|runtime| runtime.url_listener_fd()); let mut rate_limiter = supervisor_linux::RateLimiter::new(10, 5); - let mut denials = Vec::new(); + let mut denials = SupervisorDenials { + fs: Vec::new(), + url: Vec::new(), + seen_request_ids: HashSet::new(), + }; let mut ipc_denials = Vec::new(); - let mut seen_request_ids = HashSet::new(); let mut sock_fd_active = true; let startup_deadline = startup_timeout.map(|cfg| (Instant::now() + cfg.timeout, cfg)); @@ -2693,7 +2700,6 @@ fn run_supervisor_loop( child, config, &mut denials, - &mut seen_request_ids, trust_interceptor.as_mut(), ) { warn!("Error handling supervisor message: {}", e); @@ -2724,7 +2730,7 @@ fn run_supervisor_loop( config, initial_caps, &mut rate_limiter, - &mut denials, + &mut denials.fs, trust_interceptor.as_mut(), ) { @@ -2738,7 +2744,7 @@ fn run_supervisor_loop( pfd, config, &mut rate_limiter, - &mut denials, + &mut denials.fs, &mut ipc_denials, ) { @@ -2750,7 +2756,7 @@ fn run_supervisor_loop( && pfds[listener_idx].revents & libc::POLLIN != 0 && let Some(listener) = url_listener { - handle_url_listener_connection(listener, config, &mut denials); + handle_url_listener_connection(listener, config, &mut denials.url); } if let (Some(tool_sandbox_idx), Some(runtime)) = @@ -2827,7 +2833,7 @@ fn run_supervisor_loop( ); *killed_by_timeout = true; let _ = signal::kill(child, Signal::SIGKILL); - return Ok((wait_for_child(child)?, denials, ipc_denials)); + return Ok((wait_for_child(child)?, denials.fs, ipc_denials, denials.url)); } continue; } @@ -2840,12 +2846,31 @@ fn run_supervisor_loop( continue; } Ok(status) => { - return Ok((status, denials, ipc_denials)); + drain_pending_network_notifications( + proxy_notify_raw_fd, + config, + &mut rate_limiter, + &mut denials.fs, + &mut ipc_denials, + ); + return Ok((status, denials.fs, ipc_denials, denials.url)); } Err(nix::errno::Errno::EINTR) => continue, Err(nix::errno::Errno::ECHILD) => { warn!("Child already reaped in supervisor loop"); - return Ok((WaitStatus::Exited(child, 1), denials, ipc_denials)); + drain_pending_network_notifications( + proxy_notify_raw_fd, + config, + &mut rate_limiter, + &mut denials.fs, + &mut ipc_denials, + ); + return Ok(( + WaitStatus::Exited(child, 1), + denials.fs, + ipc_denials, + denials.url, + )); } Err(e) => { return Err(NonoError::SandboxInit(format!( @@ -2857,7 +2882,53 @@ fn run_supervisor_loop( } let status = wait_for_child(child)?; - Ok((status, denials, ipc_denials)) + Ok((status, denials.fs, ipc_denials, denials.url)) +} + +#[cfg(target_os = "linux")] +fn drain_pending_network_notifications( + proxy_notify_raw_fd: Option, + config: &SupervisorConfig<'_>, + rate_limiter: &mut supervisor_linux::RateLimiter, + denials: &mut Vec, + ipc_denials: &mut Vec, +) { + let Some(fd) = proxy_notify_raw_fd else { + return; + }; + + loop { + let mut pfd = libc::pollfd { + fd, + events: libc::POLLIN, + revents: 0, + }; + let ret = unsafe { libc::poll(&mut pfd, 1, 0) }; + if ret <= 0 || pfd.revents & libc::POLLIN == 0 { + return; + } + if let Err(err) = supervisor_linux::handle_network_notification( + fd, + config, + rate_limiter, + denials, + ipc_denials, + ) { + debug!("Error draining pending proxy seccomp notification: {}", err); + return; + } + } +} + +/// Mutable denial accumulators threaded through the supervisor loop. +/// +/// Groups the three vecs/maps the supervisor populates while handling IPC +/// messages, so `handle_supervisor_message` takes a single `&mut` instead +/// of three (keeping it under clippy's `too_many_arguments` threshold). +struct SupervisorDenials { + fs: Vec, + url: Vec, + seen_request_ids: HashSet, } /// Handle a single supervisor IPC message. @@ -2873,17 +2944,16 @@ fn handle_supervisor_message( msg: SupervisorMessage, child: Pid, config: &SupervisorConfig<'_>, - denials: &mut Vec, - seen_request_ids: &mut HashSet, + denials: &mut SupervisorDenials, mut trust_interceptor: Option<&mut crate::trust_intercept::TrustInterceptor>, ) -> Result<()> { match msg { SupervisorMessage::Request(request) => { let decision_started = Instant::now(); // Replay detection and bounded request-id cache. - let replay_denial_reason = if seen_request_ids.contains(&request.request_id) { + let replay_denial_reason = if denials.seen_request_ids.contains(&request.request_id) { Some("Duplicate request_id rejected (replay detected)") - } else if seen_request_ids.len() >= MAX_TRACKED_REQUEST_IDS { + } else if denials.seen_request_ids.len() >= MAX_TRACKED_REQUEST_IDS { Some("Request replay cache is full; refusing request") } else { None @@ -2891,7 +2961,7 @@ fn handle_supervisor_message( if let Some(reason) = replay_denial_reason { record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -2913,7 +2983,7 @@ fn handle_supervisor_message( )?; return Ok(()); } - seen_request_ids.insert(request.request_id.clone()); + denials.seen_request_ids.insert(request.request_id.clone()); // Digest from trust verification, used for TOCTOU re-check at open time. // Set by the trust interceptor branch when an instruction file is verified. @@ -2931,7 +3001,7 @@ fn handle_supervisor_message( protected_root.display() ); record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -2966,7 +3036,7 @@ fn handle_supervisor_message( Ok(d) => { if d.is_denied() { record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -2979,7 +3049,7 @@ fn handle_supervisor_message( Err(e) => { warn!("Approval backend error: {}", e); record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -3000,7 +3070,7 @@ fn handle_supervisor_message( reason ); record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -3021,7 +3091,7 @@ fn handle_supervisor_message( Ok(d) => { if d.is_denied() { record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -3034,7 +3104,7 @@ fn handle_supervisor_message( Err(e) => { warn!("Approval backend error: {}", e); record_denial( - denials, + &mut denials.fs, DenialRecord { path: request.path.clone(), access: request.access, @@ -3112,19 +3182,25 @@ fn handle_supervisor_message( SupervisorMessage::OpenUrl(url_request) => { let request_id = url_request.request_id.clone(); - let (success, error) = match validate_and_open_url(&url_request.url, config) { - Ok(()) => { - info!("Supervisor: opened URL {} for child", url_request.url); - (true, None) - } - Err(reason) => { - warn!( - "Supervisor: URL open denied for {}: {}", - url_request.url, reason - ); - (false, Some(reason)) - } - }; + let (success, error, push_denial) = + match validate_and_open_url(&url_request.url, config) { + Ok(()) => { + info!("Supervisor: opened URL {} for child", url_request.url); + (true, None, None) + } + Err(denial) => { + let msg = denial.to_warn_message(); + warn!( + "Supervisor: URL open denied for {}: {}", + url_request.url, msg + ); + let record = denial.to_record(); + (false, Some(msg), record) + } + }; + if let Some(record) = push_denial { + record_url_denial(&mut denials.url, record); + } let response = SupervisorResponse::UrlOpened { request_id, success, @@ -3150,7 +3226,7 @@ fn handle_supervisor_message( fn handle_url_listener_connection( listener: &SupervisorListener, config: &SupervisorConfig<'_>, - _denials: &mut Vec, + url_denials: &mut Vec, ) { let mut sock = match listener.accept() { Ok(Some(s)) => s, @@ -3172,22 +3248,28 @@ fn handle_url_listener_connection( match msg { SupervisorMessage::OpenUrl(url_request) => { let request_id = url_request.request_id.clone(); - let (success, error) = match validate_and_open_url(&url_request.url, config) { - Ok(()) => { - info!( - "Supervisor (listener): opened URL {} for child", - url_request.url - ); - (true, None) - } - Err(reason) => { - warn!( - "Supervisor (listener): URL open denied for {}: {}", - url_request.url, reason - ); - (false, Some(reason)) - } - }; + let (success, error, push_denial) = + match validate_and_open_url(&url_request.url, config) { + Ok(()) => { + info!( + "Supervisor (listener): opened URL {} for child", + url_request.url + ); + (true, None, None) + } + Err(denial) => { + let msg = denial.to_warn_message(); + warn!( + "Supervisor (listener): URL open denied for {}: {}", + url_request.url, msg + ); + let record = denial.to_record(); + (false, Some(msg), record) + } + }; + if let Some(record) = push_denial { + record_url_denial(url_denials, record); + } let response = SupervisorResponse::UrlOpened { request_id, success, @@ -3243,9 +3325,86 @@ fn record_capability_audit( } /// Maximum URL length to prevent abuse via oversized URLs. -#[cfg(test)] const MAX_URL_LENGTH: usize = crate::url_open::MAX_URL_LENGTH; +/// Typed result of URL validation. +/// +/// Separates fixable denials (OriginNotAllowed, LocalhostNotAllowed) from +/// non-fixable ones (bad schemes, oversize, parse errors, browser-launch +/// failures) so the save prompt can decide what to offer. +#[derive(Debug)] +enum UrlDenial { + OriginNotAllowed { + origin: String, + }, + LocalhostNotAllowed, + NonHttpsScheme { + scheme: String, + }, + LocalhostBadScheme { + scheme: String, + }, + Oversized { + len: usize, + }, + ParseError { + message: String, + }, + /// The URL validated but the browser opener failed to launch. + /// Not fixable by a profile grant, so never offered in the save prompt. + BrowserLaunchFailed { + message: String, + }, +} + +impl UrlDenial { + fn to_warn_message(&self) -> String { + match self { + Self::OriginNotAllowed { origin } => { + format!("Origin {origin} is not in the profile's open_urls.allow_origins list") + } + Self::LocalhostNotAllowed => { + "Localhost URLs are not allowed by this profile".to_string() + } + Self::NonHttpsScheme { scheme } => { + format!( + "Only https:// URLs are allowed (got {scheme}://). \ + file://, javascript:, data:, and other schemes are blocked." + ) + } + Self::LocalhostBadScheme { scheme } => { + format!("Localhost URL must use http or https scheme, got: {scheme}") + } + Self::Oversized { len } => { + format!("URL exceeds maximum length ({len} > {MAX_URL_LENGTH})") + } + Self::ParseError { message } => { + format!("Invalid URL: {message}") + } + Self::BrowserLaunchFailed { message } => { + format!("Failed to open URL in browser: {message}") + } + } + } + + /// Map to a persisted record. Returns `None` for non-fixable denials, + /// which are recorded only for the audit log and never offered in the + /// save prompt. + fn to_record(&self) -> Option { + match self { + Self::OriginNotAllowed { origin } => Some(UrlDenialRecord { + origin: origin.clone(), + reason: UrlDenialReason::OriginNotAllowed, + }), + Self::LocalhostNotAllowed => Some(UrlDenialRecord { + origin: String::new(), + reason: UrlDenialReason::LocalhostNotAllowed, + }), + _ => None, + } + } +} + /// Validate a URL against the profile's allowed origins, then open it in the user's browser. /// /// The supervisor (unsandboxed parent) performs this operation so the browser @@ -3253,23 +3412,63 @@ const MAX_URL_LENGTH: usize = crate::url_open::MAX_URL_LENGTH; fn validate_and_open_url( url: &str, config: &SupervisorConfig<'_>, -) -> std::result::Result<(), String> { +) -> std::result::Result<(), UrlDenial> { validate_url(url, config)?; crate::url_open::open_url_in_browser(url) + .map_err(|msg| UrlDenial::BrowserLaunchFailed { message: msg }) } /// Validate a URL against the profile's allowed origins and scheme rules. /// -/// Thin wrapper over [`crate::url_open::validate_url`] that sources the -/// allow-list from the supervisor config. The shared implementation is the -/// single source of truth for this security-critical check so the supervisor -/// and the tool-sandbox runtime cannot diverge. -fn validate_url(url: &str, config: &SupervisorConfig<'_>) -> std::result::Result<(), String> { - crate::url_open::validate_url( - url, - config.open_url_origins, - config.open_url_allow_localhost, - ) +/// Returns `Ok(())` if the URL passes all checks. Does not open the browser. +/// +/// This implementation must stay in sync with [`crate::url_open::validate_url`], +/// which is the shared implementation used by the tool-sandbox runtime. +/// TODO: Refactor `crate::url_open::validate_url` to return `UrlDenial` directly +/// so this function can delegate without duplicating the logic. +fn validate_url(url: &str, config: &SupervisorConfig<'_>) -> std::result::Result<(), UrlDenial> { + // Length check + if url.len() > MAX_URL_LENGTH { + return Err(UrlDenial::Oversized { len: url.len() }); + } + + // Parse URL to extract origin + let parsed = url::Url::parse(url).map_err(|e| UrlDenial::ParseError { + message: e.to_string(), + })?; + + let scheme = parsed.scheme(); + let host = parsed.host_str().unwrap_or(""); + + // Check localhost first + let is_localhost = host == "localhost" || host == "127.0.0.1" || host == "::1"; + if is_localhost { + if scheme != "http" && scheme != "https" { + return Err(UrlDenial::LocalhostBadScheme { + scheme: scheme.to_string(), + }); + } + if !config.open_url_allow_localhost { + return Err(UrlDenial::LocalhostNotAllowed); + } + } else { + // Non-localhost: must be https + if scheme != "https" { + return Err(UrlDenial::NonHttpsScheme { + scheme: scheme.to_string(), + }); + } + + // Check against allowed origins + let url_origin = parsed.origin().unicode_serialization(); + let origin_allowed = config.open_url_origins.contains(&url_origin); + + if !origin_allowed { + return Err(UrlDenial::OriginNotAllowed { origin: url_origin }); + } + } + + Ok(()) } /// Clear `FD_CLOEXEC` on a file descriptor so it survives `execve()`. @@ -3304,6 +3503,16 @@ pub(super) fn record_denial(denials: &mut Vec, record: DenialRecor } } +/// Push a URL denial record to the vec, capping at MAX_DENIAL_RECORDS. +/// +/// Deduplicates inline so a child polling the same blocked URL in a retry loop +/// cannot exhaust the buffer and crowd out other unique denials. +fn record_url_denial(url_denials: &mut Vec, record: UrlDenialRecord) { + if url_denials.len() < MAX_DENIAL_RECORDS && !url_denials.contains(&record) { + url_denials.push(record); + } +} + /// Create a temporary directory containing an `open` shim script on macOS. /// /// The shim intercepts calls to `open` (which the Node.js `open` package @@ -4096,6 +4305,7 @@ mod tests { &explanations, &observation, &[], + &[], )); } @@ -4114,6 +4324,7 @@ mod tests { &explanations, &observation, &violations, + &[], )); } @@ -4135,6 +4346,7 @@ mod tests { &explanations, &observation, &visible, + &[], )); } @@ -4165,6 +4377,66 @@ mod tests { assert_eq!(explanation.reason, "sensitive_path"); } + #[test] + fn test_profile_save_prompt_triggers_on_url_denial_with_zero_exit() { + use nono::UrlDenialReason; + + let explanations = Vec::new(); + let observation = crate::diagnostic::ErrorObservation::default(); + let url_denials = vec![UrlDenialRecord { + origin: "https://accounts.google.com".to_string(), + reason: UrlDenialReason::OriginNotAllowed, + }]; + + // Fixable URL denial must trigger save prompt even with exit code 0 + assert!(should_offer_profile_save( + false, + 0, + &explanations, + &observation, + &[], + &url_denials, + )); + + // No URL denials (and no other signal) must NOT trigger save prompt. + // Non-fixable denials never become records, so the empty slice models + // a run where only non-fixable URL opens were rejected. + assert!(!should_offer_profile_save( + false, + 0, + &explanations, + &observation, + &[], + &[], + )); + } + + #[test] + fn test_record_url_denial_dedupes_repeated_records() { + let mut denials = Vec::new(); + let record = UrlDenialRecord { + origin: "https://accounts.google.com".to_string(), + reason: UrlDenialReason::OriginNotAllowed, + }; + + // A child polling the same blocked URL must not fill the buffer with + // identical records. + for _ in 0..100 { + record_url_denial(&mut denials, record.clone()); + } + assert_eq!(denials.len(), 1); + + // Distinct records are still recorded. + record_url_denial( + &mut denials, + UrlDenialRecord { + origin: String::new(), + reason: UrlDenialReason::LocalhostNotAllowed, + }, + ); + assert_eq!(denials.len(), 2); + } + #[test] fn test_profile_save_prompt_preserves_nonzero_exit_behavior() { let explanations = Vec::new(); @@ -4176,6 +4448,7 @@ mod tests { &explanations, &observation, &[], + &[], )); assert!(!should_offer_profile_save( true, @@ -4183,6 +4456,7 @@ mod tests { &explanations, &observation, &[], + &[], )); } @@ -4503,12 +4777,12 @@ mod tests { ); #[cfg(target_os = "linux")] - let (status, denials, ipc_denials) = result + let (status, denials, ipc_denials, _url_denials) = result .map_err(|e| format!("supervisor loop: {e}")) .expect("supervisor loop failed"); #[cfg(not(target_os = "linux"))] - let (status, denials) = result + let (status, denials, _url_denials) = result .map_err(|e| format!("supervisor loop: {e}")) .expect("supervisor loop failed"); @@ -4612,7 +4886,7 @@ mod tests { &mut false, ); - let (status, denials, ipc_denials) = result + let (status, denials, ipc_denials, _url_denials) = result .map_err(|e| format!("supervisor loop: {e}")) .expect("supervisor loop should not deadlock"); assert!(denials.is_empty()); @@ -4676,13 +4950,9 @@ mod tests { // Disallowed origin: must fail validation let result = validate_url("https://evil.example.com/phishing", &config); - assert!(result.is_err()); assert!( - result - .as_ref() - .err() - .map(|e| e.contains("not in the profile")) - .unwrap_or(false) + matches!(result, Err(UrlDenial::OriginNotAllowed { .. })), + "Expected OriginNotAllowed, got: {result:?}" ); } @@ -4714,17 +4984,84 @@ mod tests { }; let result = validate_url("file:///etc/passwd", &config); - assert!(result.is_err()); assert!( - result - .as_ref() - .err() - .map(|e| e.contains("Only https://")) - .unwrap_or(false) + matches!(result, Err(UrlDenial::NonHttpsScheme { .. })), + "Expected NonHttpsScheme, got: {result:?}" ); let result = validate_url("javascript:alert(1)", &config); - assert!(result.is_err()); + assert!( + matches!(result, Err(UrlDenial::NonHttpsScheme { .. })), + "Expected NonHttpsScheme for javascript:, got: {result:?}" + ); + } + + /// A denied URL must flow through validation into a persistable + /// `UrlDenialRecord`. Fixable denials (disallowed origin, blocked localhost) + /// produce a record with the right reason and an origin-only payload (never + /// the full URL); non-fixable denials produce no record so they are never + /// offered in the save prompt. + #[test] + fn test_url_denial_maps_to_record_for_save_prompt() { + let backend = TestDenyBackend; + let origins = vec!["https://claude.ai".to_string()]; + let config = SupervisorConfig { + protected_roots: &[], + approval_backend: &backend, + session_id: "test", + attach_initial_client: false, + detach_sequence: None, + open_url_origins: &origins, + open_url_allow_localhost: false, + audit_recorder: None, + network_audit_events: None, + redaction_policy: &nono::ScrubPolicy::secure_default(), + allow_launch_services_active: false, + #[cfg(target_os = "linux")] + proxy_port: 0, + #[cfg(target_os = "linux")] + proxy_bind_ports: Vec::new(), + #[cfg(target_os = "linux")] + unix_socket_allowlist: &[], + #[cfg(target_os = "linux")] + linux_network_notify_mode: LinuxNetworkNotifyMode::ProxyOnly, + #[cfg(any(target_os = "linux", target_os = "macos"))] + tool_sandbox_runtime: None, + }; + + // Disallowed origin -> OriginNotAllowed record carrying only the origin, + // not the full URL with its OAuth query string. + let denial = validate_url( + "https://accounts.google.com/o/oauth2/auth?token=secret", + &config, + ) + .expect_err("disallowed origin must be denied"); + let record = denial.to_record().expect("origin denial must be saveable"); + assert_eq!(record.reason, UrlDenialReason::OriginNotAllowed); + assert_eq!(record.origin, "https://accounts.google.com"); + + // Blocked localhost -> LocalhostNotAllowed record with empty origin. + let denial = validate_url("http://localhost:8080/callback", &config) + .expect_err("localhost must be denied when allow_localhost is false"); + let record = denial + .to_record() + .expect("localhost denial must be saveable"); + assert_eq!(record.reason, UrlDenialReason::LocalhostNotAllowed); + assert!(record.origin.is_empty()); + + // Non-fixable denials must NOT produce a record. + let non_fixable = [ + "file:///etc/passwd", + "not a url", + &format!("https://example.com/{}", "a".repeat(MAX_URL_LENGTH)), + ]; + for url in non_fixable { + let denial = validate_url(url, &config).expect_err("must be denied"); + assert!( + denial.to_record().is_none(), + "non-fixable denial must not be saveable: {denial:?}" + ); + } } #[test] @@ -4779,13 +5116,9 @@ mod tests { // Localhost denied when not allowed let result = validate_url("http://localhost:8080/callback", &config_deny); - assert!(result.is_err()); assert!( - result - .as_ref() - .err() - .map(|e| e.contains("not allowed")) - .unwrap_or(false) + matches!(result, Err(UrlDenial::LocalhostNotAllowed)), + "Expected LocalhostNotAllowed, got: {result:?}" ); // Localhost allowed when configured @@ -4825,13 +5158,9 @@ mod tests { let long_url = format!("https://example.com/{}", "a".repeat(MAX_URL_LENGTH)); let result = validate_url(&long_url, &config); - assert!(result.is_err()); assert!( - result - .as_ref() - .err() - .map(|e| e.contains("maximum length")) - .unwrap_or(false) + matches!(result, Err(UrlDenial::Oversized { .. })), + "Expected Oversized, got: {result:?}" ); } diff --git a/crates/nono-cli/src/profile_save_runtime.rs b/crates/nono-cli/src/profile_save_runtime.rs index 26960b904..061bb9c0c 100644 --- a/crates/nono-cli/src/profile_save_runtime.rs +++ b/crates/nono-cli/src/profile_save_runtime.rs @@ -1,10 +1,11 @@ use crate::command_display::format_command_line; use crate::diagnostic::{ErrorObservation, PolicyExplanation}; +use crate::exec_strategy::ProfileSaveOffer; use crate::theme; use crate::{profile, query_ext}; use colored::Colorize; use nono::SandboxViolation; -use nono::{AccessMode, CapabilitySet, NonoError, Result}; +use nono::{AccessMode, CapabilitySet, NonoError, Result, UrlDenialReason, UrlDenialRecord}; use std::collections::{BTreeMap, BTreeSet}; use std::io::{BufRead, IsTerminal, Write}; use std::path::{Path, PathBuf}; @@ -88,12 +89,47 @@ impl ProfileSection { } } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum UrlItemKind { + Origin, + Localhost, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum UrlItemAction { + Grant, + Skip, +} + +impl UrlItemAction { + fn cycle(self) -> Self { + match self { + Self::Grant => Self::Skip, + Self::Skip => Self::Grant, + } + } + + fn padded_label(self) -> &'static str { + match self { + Self::Grant => "grant ", + Self::Skip => "skip ", + } + } +} + #[derive(Clone, Debug)] -struct DenialItem { - path: String, - section: ProfileSection, - is_bypass: bool, - action: ItemAction, +enum DenialItem { + Fs { + path: String, + section: ProfileSection, + is_bypass: bool, + action: ItemAction, + }, + Url { + origin: String, + kind: UrlItemKind, + action: UrlItemAction, + }, } const DENIAL_SELECTOR_MAX_VISIBLE_ITEMS: usize = 15; @@ -135,7 +171,7 @@ fn extract_denial_items(patch: &profile::Profile) -> Vec { for (paths, section) in sections { for path in *paths { let is_bypass = fs.bypass_protection.contains(path); - items.push(DenialItem { + items.push(DenialItem::Fs { path: path.clone(), section: *section, is_bypass, @@ -145,7 +181,7 @@ fn extract_denial_items(patch: &profile::Profile) -> Vec { } for rule in &patch.unsafe_macos_seatbelt_rules { - items.push(DenialItem { + items.push(DenialItem::Fs { path: rule.clone(), section: ProfileSection::UnsafeSeatbelt, is_bypass: false, @@ -153,6 +189,24 @@ fn extract_denial_items(patch: &profile::Profile) -> Vec { }); } + // URL items from open_urls patch + if let Some(ref open_urls) = patch.open_urls { + if open_urls.allow_localhost { + items.push(DenialItem::Url { + origin: String::new(), + kind: UrlItemKind::Localhost, + action: UrlItemAction::Grant, + }); + } + for origin in &open_urls.allow_origins { + items.push(DenialItem::Url { + origin: origin.clone(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Grant, + }); + } + } + items } @@ -176,42 +230,74 @@ pub(crate) fn terminal_prompts_available() -> bool { || std::fs::File::open("/dev/tty").is_ok() } -pub(crate) fn offer_save_run_profile( - policy_explanations: &[PolicyExplanation], - error_observation: &ErrorObservation, - caps: &CapabilitySet, - command: &[String], - compared_profile: Option<&str>, - sandbox_violations: &[SandboxViolation], - ignored_denial_paths: &[PathBuf], -) -> Result<()> { +pub(crate) fn offer_save_run_profile(offer: &ProfileSaveOffer<'_>) -> Result<()> { if !terminal_prompts_available() { return Ok(()); } - let Some(patch) = build_run_profile_patch( - policy_explanations, - error_observation, - caps, - sandbox_violations, - ignored_denial_paths, + let Some(mut patch) = build_run_profile_patch( + offer.policy_explanations, + offer.error_observation, + offer.caps, + offer.sandbox_violations, + offer.ignored_denial_paths, )? else { - return Ok(()); + // No filesystem patch — but we may still have URL denials + return offer_url_only_save(offer.url_denials, offer.command, offer.compared_profile); }; - let cmd_name = command_name(command)?; + // Merge URL grants into the filesystem patch profile so extract_denial_items + // picks them up alongside filesystem items. The URL patch only sets + // open_urls, so merging touches nothing else in the filesystem patch. + if let Some(url_patch) = build_url_patch(offer.url_denials) { + merge_profile_patch(&mut patch, &url_patch); + } + + let cmd_name = command_name(offer.command)?; // Try the interactive selector first; fall back to the text prompt when // raw mode is unavailable (e.g. a dumb terminal or a redirected TTY). match interactive_denial_selector(&patch)? { + Some(items) => { + let Some(combined_patch) = build_combined_patch_from_items(&items) else { + return Ok(()); + }; + offer_save_with_patch( + &combined_patch, + &cmd_name, + offer.command, + offer.compared_profile, + ) + } + None => offer_save_text_prompt(&patch, &cmd_name, offer.command, offer.compared_profile), + } +} + +/// Offer profile save when only URL denials exist (no filesystem patch). +fn offer_url_only_save( + url_denials: &[UrlDenialRecord], + command: &[String], + compared_profile: Option<&str>, +) -> Result<()> { + if url_denials.is_empty() { + return Ok(()); + } + + let Some(url_patch) = build_url_patch(url_denials) else { + return Ok(()); + }; + + let cmd_name = command_name(command)?; + + match interactive_denial_selector(&url_patch)? { Some(items) => { let Some(combined_patch) = build_combined_patch_from_items(&items) else { return Ok(()); }; offer_save_with_patch(&combined_patch, &cmd_name, command, compared_profile) } - None => offer_save_text_prompt(&patch, &cmd_name, command, compared_profile), + None => offer_save_text_prompt(&url_patch, &cmd_name, command, compared_profile), } } @@ -1120,40 +1206,89 @@ fn render_denial_selector( " ".to_string() }; - let action_str = match item.action { - ItemAction::Grant => { - format!("{}", theme::fg(item.action.padded_label(), t.green).bold()) - } - ItemAction::Suppress => { - format!("{}", theme::fg(item.action.padded_label(), t.yellow)) - } - ItemAction::Skip => { - format!("{}", theme::fg(item.action.padded_label(), t.overlay)) - } - }; + match item { + DenialItem::Fs { + path, + section, + is_bypass, + action, + } => { + let action_str = match action { + ItemAction::Grant => { + format!("{}", theme::fg(action.padded_label(), t.green).bold()) + } + ItemAction::Suppress => { + format!("{}", theme::fg(action.padded_label(), t.yellow)) + } + ItemAction::Skip => { + format!("{}", theme::fg(action.padded_label(), t.overlay)) + } + }; - let bypass_prefix = if item.is_bypass { - format!("{} ", "⚠".red()) - } else { - String::new() - }; + let bypass_prefix = if *is_bypass { + format!("{} ", "⚠".red()) + } else { + String::new() + }; - let path_str = if selected { - format!("{}", theme::fg(&item.path, t.text).bold()) - } else { - format!("{}", theme::fg(&item.path, t.subtext)) - }; + let path_str = if selected { + format!("{}", theme::fg(path, t.text).bold()) + } else { + format!("{}", theme::fg(path, t.subtext)) + }; - let label_str = format!(" ({})", theme::fg(item.section.display_label(), t.overlay)); + let label_str = format!(" ({})", theme::fg(section.display_label(), t.overlay)); - tty_ln!( - " {} {} {}{}{}", - cursor_glyph, - action_str, - bypass_prefix, - path_str, - label_str - ); + tty_ln!( + " {} {} {}{}{}", + cursor_glyph, + action_str, + bypass_prefix, + path_str, + label_str + ); + } + DenialItem::Url { + origin, + kind, + action, + } => { + let action_str = match action { + UrlItemAction::Grant => { + format!("{}", theme::fg(action.padded_label(), t.green).bold()) + } + UrlItemAction::Skip => { + format!("{}", theme::fg(action.padded_label(), t.overlay)) + } + }; + + let (display_origin, label_str) = match kind { + UrlItemKind::Origin => { + let origin_display = if selected { + format!("{}", theme::fg(origin, t.text).bold()) + } else { + format!("{}", theme::fg(origin, t.subtext)) + }; + ( + origin_display, + format!(" ({})", theme::fg("open-url origin", t.overlay)), + ) + } + UrlItemKind::Localhost => ( + String::new(), + format!(" ({})", theme::fg("allow-localhost", t.overlay)), + ), + }; + + tty_ln!( + " {} {} {}{}", + cursor_glyph, + action_str, + display_origin, + label_str + ); + } + } } tty_ln!(""); @@ -1216,35 +1351,53 @@ fn interactive_denial_selector(patch: &profile::Profile) -> Result { - let next = items[cursor].action.cycle(); - // UnsafeSeatbelt items have no suppress mechanism — skip that state - items[cursor].action = if items[cursor].section == ProfileSection::UnsafeSeatbelt - && next == ItemAction::Suppress - { - next.cycle() - } else { - next - }; - } + Key::Space => match &mut items[cursor] { + DenialItem::Fs { + action, section, .. + } => { + let mut next = action.cycle(); + if *section == ProfileSection::UnsafeSeatbelt && next == ItemAction::Suppress { + next = next.cycle(); + } + *action = next; + } + DenialItem::Url { action, .. } => { + *action = action.cycle(); + } + }, Key::Char('a') => { for item in &mut items { - item.action = ItemAction::Grant; + match item { + DenialItem::Fs { action, .. } => *action = ItemAction::Grant, + DenialItem::Url { action, .. } => *action = UrlItemAction::Grant, + } } } Key::Char('d') => { for item in &mut items { - item.action = if item.section == ProfileSection::UnsafeSeatbelt { - ItemAction::Skip - } else { - ItemAction::Suppress - }; + match item { + DenialItem::Fs { + action, section, .. + } => { + *action = if *section == ProfileSection::UnsafeSeatbelt { + ItemAction::Skip + } else { + ItemAction::Suppress + }; + } + // URL items are not suppressible; deny-all skips them + // so Enter cannot accidentally grant the default Grant. + DenialItem::Url { action, .. } => *action = UrlItemAction::Skip, + } } } Key::Enter => break, Key::CtrlC | Key::Char('\x1b') => { for item in &mut items { - item.action = ItemAction::Skip; + match item { + DenialItem::Fs { action, .. } => *action = ItemAction::Skip, + DenialItem::Url { action, .. } => *action = UrlItemAction::Skip, + } } break; } @@ -1259,51 +1412,81 @@ fn interactive_denial_selector(patch: &profile::Profile) -> Result Option { - let has_grants = items.iter().any(|i| i.action == ItemAction::Grant); - let has_suppresses = items - .iter() - .any(|i| i.action == ItemAction::Suppress && i.section != ProfileSection::UnsafeSeatbelt); + let has_grants = items.iter().any(|i| match i { + DenialItem::Fs { action, .. } => *action == ItemAction::Grant, + DenialItem::Url { action, .. } => *action == UrlItemAction::Grant, + }); + let has_suppresses = items.iter().any(|i| { + matches!(i, DenialItem::Fs { action: ItemAction::Suppress, section, .. } if *section != ProfileSection::UnsafeSeatbelt) + }); if !has_grants && !has_suppresses { return None; } let mut patch = profile::Profile::default(); + let mut origin_grants: Vec = Vec::new(); + let mut localhost_grant = false; for item in items { - match item.action { - ItemAction::Grant => { - match item.section { - ProfileSection::Allow => patch.filesystem.allow.push(item.path.clone()), - ProfileSection::Read => patch.filesystem.read.push(item.path.clone()), - ProfileSection::Write => patch.filesystem.write.push(item.path.clone()), - ProfileSection::AllowFile => { - patch.filesystem.allow_file.push(item.path.clone()) - } - ProfileSection::ReadFile => patch.filesystem.read_file.push(item.path.clone()), - ProfileSection::WriteFile => { - patch.filesystem.write_file.push(item.path.clone()) + match item { + DenialItem::Fs { + path, + section, + is_bypass, + action, + } => match action { + ItemAction::Grant => { + match section { + ProfileSection::Allow => patch.filesystem.allow.push(path.clone()), + ProfileSection::Read => patch.filesystem.read.push(path.clone()), + ProfileSection::Write => patch.filesystem.write.push(path.clone()), + ProfileSection::AllowFile => patch.filesystem.allow_file.push(path.clone()), + ProfileSection::ReadFile => patch.filesystem.read_file.push(path.clone()), + ProfileSection::WriteFile => patch.filesystem.write_file.push(path.clone()), + ProfileSection::UnsafeSeatbelt => { + patch.unsafe_macos_seatbelt_rules.push(path.clone()) + } } - ProfileSection::UnsafeSeatbelt => { - patch.unsafe_macos_seatbelt_rules.push(item.path.clone()) + if *is_bypass && !patch.filesystem.bypass_protection.contains(path) { + patch.filesystem.bypass_protection.push(path.clone()); } } - if item.is_bypass && !patch.filesystem.bypass_protection.contains(&item.path) { - patch.filesystem.bypass_protection.push(item.path.clone()); + ItemAction::Suppress => { + if *section != ProfileSection::UnsafeSeatbelt { + patch.filesystem.suppress_save_prompt.push(path.clone()); + } } - } - ItemAction::Suppress => { - if item.section != ProfileSection::UnsafeSeatbelt { - patch - .filesystem - .suppress_save_prompt - .push(item.path.clone()); + ItemAction::Skip => {} + }, + DenialItem::Url { + origin, + kind, + action, + } => { + if *action == UrlItemAction::Grant { + match kind { + UrlItemKind::Origin => { + if !origin.is_empty() && !origin_grants.contains(origin) { + origin_grants.push(origin.clone()); + } + } + UrlItemKind::Localhost => { + localhost_grant = true; + } + } } } - ItemAction::Skip => {} } } + if !origin_grants.is_empty() || localhost_grant { + patch.open_urls = Some(profile::OpenUrlConfig { + allow_origins: origin_grants, + allow_localhost: localhost_grant, + }); + } + Some(patch) } @@ -1509,6 +1692,7 @@ fn patch_is_suppression_only(patch: &profile::Profile) -> bool { && patch.filesystem.write_file.is_empty() && patch.filesystem.bypass_protection.is_empty() && patch.unsafe_macos_seatbelt_rules.is_empty() + && patch.open_urls.is_none() } fn build_suppress_save_prompt_patch(grant_patch: &profile::Profile) -> Option { @@ -1642,6 +1826,60 @@ pub(crate) fn merge_profile_patch(profile: &mut profile::Profile, patch: &profil &profile.unsafe_macos_seatbelt_rules, &patch.unsafe_macos_seatbelt_rules, ); + + // Merge open_urls: origins are dedup-appended, allow_localhost is monotonic (false -> true only) + if let Some(ref patch_urls) = patch.open_urls + && (patch_urls.allow_localhost || !patch_urls.allow_origins.is_empty()) + { + let urls = profile + .open_urls + .get_or_insert_with(|| profile::OpenUrlConfig { + allow_origins: Vec::new(), + allow_localhost: false, + }); + urls.allow_origins = profile::dedup_append(&urls.allow_origins, &patch_urls.allow_origins); + // Monotonic: only flip false -> true, never true -> false + if patch_urls.allow_localhost { + urls.allow_localhost = true; + } + } +} + +/// Build a profile patch containing only open_urls fields from URL denial records. +/// +/// The resulting profile is merged into the filesystem profile patch before +/// `extract_denial_items`, so URL items appear in the same selector alongside +/// filesystem items without changing any existing function signatures. +pub(crate) fn build_url_patch(url_denials: &[UrlDenialRecord]) -> Option { + let mut origin_grants: Vec = Vec::new(); + let mut localhost_grant = false; + + for record in url_denials { + match record.reason { + UrlDenialReason::OriginNotAllowed + if !record.origin.is_empty() && !origin_grants.contains(&record.origin) => + { + origin_grants.push(record.origin.clone()); + } + UrlDenialReason::LocalhostNotAllowed => { + localhost_grant = true; + } + _ => {} + } + } + + if origin_grants.is_empty() && !localhost_grant { + return None; + } + + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: origin_grants, + allow_localhost: localhost_grant, + }), + ..Default::default() + }; + Some(patch) } pub(crate) fn shorten_path_for_profile(path: &Path, home_path: &Path) -> String { @@ -2350,4 +2588,242 @@ mod tests { }); assert!(!leftover, "temp file should be renamed into place"); } + + // ─── URL denial patch tests ─────────────────────────────────────────── + + fn url_record(origin: &str, reason: UrlDenialReason) -> UrlDenialRecord { + UrlDenialRecord { + origin: origin.to_string(), + reason, + } + } + + #[test] + fn build_url_patch_returns_none_for_empty_input() { + // Non-fixable denials never become records, so an empty slice models a + // run where only non-fixable URL opens were rejected: no patch. + assert!(build_url_patch(&[]).is_none()); + } + + #[test] + fn build_url_patch_collects_origin_grants_deduplicated() { + let denials = vec![ + url_record( + "https://accounts.google.com", + UrlDenialReason::OriginNotAllowed, + ), + url_record( + "https://accounts.google.com", + UrlDenialReason::OriginNotAllowed, + ), + url_record("https://github.com", UrlDenialReason::OriginNotAllowed), + ]; + let patch = build_url_patch(&denials).expect("patch for origin denials"); + let urls = patch.open_urls.expect("open_urls set"); + assert_eq!( + urls.allow_origins, + vec!["https://accounts.google.com", "https://github.com"] + ); + assert!(!urls.allow_localhost); + } + + #[test] + fn build_url_patch_enables_localhost_for_localhost_denial() { + let denials = vec![ + // Localhost record carries empty origin. + url_record("", UrlDenialReason::LocalhostNotAllowed), + url_record("", UrlDenialReason::LocalhostNotAllowed), + ]; + let patch = build_url_patch(&denials).expect("patch for localhost denial"); + let urls = patch.open_urls.expect("open_urls set"); + assert!(urls.allow_origins.is_empty()); + assert!(urls.allow_localhost); + } + + #[test] + fn build_url_patch_combines_origins_and_localhost() { + let denials = vec![ + url_record( + "https://oauth.example.com", + UrlDenialReason::OriginNotAllowed, + ), + url_record("", UrlDenialReason::LocalhostNotAllowed), + ]; + let patch = build_url_patch(&denials).expect("combined patch"); + let urls = patch.open_urls.expect("open_urls set"); + assert_eq!(urls.allow_origins, vec!["https://oauth.example.com"]); + assert!(urls.allow_localhost); + } + + #[test] + fn build_combined_patch_from_items_routes_url_origin_grants() { + let items = vec![ + DenialItem::Url { + origin: "https://accounts.google.com".to_string(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Grant, + }, + DenialItem::Url { + origin: "https://github.com".to_string(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Skip, + }, + ]; + let patch = build_combined_patch_from_items(&items).expect("patch from granted items"); + let urls = patch.open_urls.expect("open_urls set"); + // Skipped origin must NOT appear; granted origin must. + assert_eq!(urls.allow_origins, vec!["https://accounts.google.com"]); + assert!(!urls.allow_localhost); + } + + #[test] + fn build_combined_patch_from_items_routes_localhost_grant() { + let items = vec![DenialItem::Url { + origin: String::new(), + kind: UrlItemKind::Localhost, + action: UrlItemAction::Grant, + }]; + let patch = build_combined_patch_from_items(&items).expect("patch from localhost grant"); + let urls = patch.open_urls.expect("open_urls set"); + assert!(urls.allow_origins.is_empty()); + assert!(urls.allow_localhost); + } + + #[test] + fn build_combined_patch_from_items_returns_none_when_all_urls_skipped() { + let items = vec![DenialItem::Url { + origin: "https://accounts.google.com".to_string(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Skip, + }]; + assert!(build_combined_patch_from_items(&items).is_none()); + } + + #[test] + fn build_combined_patch_from_items_dedupes_origin_grants() { + let items = vec![ + DenialItem::Url { + origin: "https://dup.example.com".to_string(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Grant, + }, + DenialItem::Url { + origin: "https://dup.example.com".to_string(), + kind: UrlItemKind::Origin, + action: UrlItemAction::Grant, + }, + ]; + let patch = build_combined_patch_from_items(&items).expect("patch"); + let urls = patch.open_urls.expect("open_urls set"); + assert_eq!(urls.allow_origins, vec!["https://dup.example.com"]); + } + + #[test] + fn merge_profile_patch_appends_origins_deduplicated() { + let mut base = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec!["https://existing.example.com".to_string()], + allow_localhost: false, + }), + ..Default::default() + }; + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec![ + "https://existing.example.com".to_string(), + "https://new.example.com".to_string(), + ], + allow_localhost: false, + }), + ..Default::default() + }; + merge_profile_patch(&mut base, &patch); + let urls = base.open_urls.expect("open_urls present"); + assert_eq!( + urls.allow_origins, + vec!["https://existing.example.com", "https://new.example.com"] + ); + } + + #[test] + fn merge_profile_patch_localhost_is_monotonic_true_stays_true() { + // Base already allows localhost; patch without localhost must not disable it. + let mut base = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec![], + allow_localhost: true, + }), + ..Default::default() + }; + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec!["https://only-in-patch.example.com".to_string()], + allow_localhost: false, + }), + ..Default::default() + }; + merge_profile_patch(&mut base, &patch); + let urls = base.open_urls.expect("open_urls present"); + assert!( + urls.allow_localhost, + "localhost must remain true (monotonic)" + ); + assert_eq!( + urls.allow_origins, + vec!["https://only-in-patch.example.com"] + ); + } + + #[test] + fn merge_profile_patch_localhost_flips_false_to_true() { + let mut base = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec![], + allow_localhost: false, + }), + ..Default::default() + }; + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec![], + allow_localhost: true, + }), + ..Default::default() + }; + merge_profile_patch(&mut base, &patch); + let urls = base.open_urls.expect("open_urls present"); + assert!(urls.allow_localhost, "localhost must flip to true"); + } + + #[test] + fn merge_profile_patch_creates_open_urls_when_base_has_none() { + let mut base = profile::Profile::default(); + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec!["https://brand-new.example.com".to_string()], + allow_localhost: true, + }), + ..Default::default() + }; + merge_profile_patch(&mut base, &patch); + let urls = base.open_urls.expect("open_urls created"); + assert_eq!(urls.allow_origins, vec!["https://brand-new.example.com"]); + assert!(urls.allow_localhost); + } + + #[test] + fn merge_profile_patch_skips_open_urls_when_patch_has_none() { + let mut base = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec!["https://keep.example.com".to_string()], + allow_localhost: true, + }), + ..Default::default() + }; + let patch = profile::Profile::default(); + merge_profile_patch(&mut base, &patch); + let urls = base.open_urls.expect("open_urls preserved"); + assert_eq!(urls.allow_origins, vec!["https://keep.example.com"]); + assert!(urls.allow_localhost); + } } diff --git a/crates/nono/src/diagnostic/mod.rs b/crates/nono/src/diagnostic/mod.rs index 00c6afba6..aacdc4623 100644 --- a/crates/nono/src/diagnostic/mod.rs +++ b/crates/nono/src/diagnostic/mod.rs @@ -17,6 +17,7 @@ pub use observation::{ follow_up_diagnostics, }; pub use records::{ - DenialReason, DenialRecord, IpcDenialRecord, SandboxViolation, seatbelt_operation_to_access, + DenialReason, DenialRecord, IpcDenialRecord, SandboxViolation, UrlDenialReason, + UrlDenialRecord, seatbelt_operation_to_access, }; pub use report::{SessionDiagnosticReport, dedupe_denials, filesystem_denials_from_violations}; diff --git a/crates/nono/src/diagnostic/records.rs b/crates/nono/src/diagnostic/records.rs index 5c6c4fed1..e40847b7a 100644 --- a/crates/nono/src/diagnostic/records.rs +++ b/crates/nono/src/diagnostic/records.rs @@ -79,6 +79,32 @@ impl IpcDenialRecord { } } +/// Why a URL open request was denied during a supervised session. +/// +/// Only carries the fixable reasons that can be persisted as a profile grant. +/// Non-fixable denials (bad scheme, oversize, parse errors, browser-launch +/// failures) are classified CLI-side and never produce a record. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum UrlDenialReason { + /// The URL's origin is not in the profile's allow_origins list. + /// Fixable: user can grant the origin in the save prompt. + OriginNotAllowed, + /// A localhost URL was blocked because allow_localhost is false. + /// Fixable: user can enable allow_localhost in the save prompt. + LocalhostNotAllowed, +} + +/// Record of a denied URL open attempt during a supervised session. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UrlDenialRecord { + /// The URL origin (scheme + host), e.g. "https://accounts.google.com". + /// Derived from the URL via `url::Url::origin().unicode_serialization()`. + /// Never carries the full URL (which may contain OAuth tokens in query strings). + pub origin: String, + /// Why the URL open was denied. + pub reason: UrlDenialReason, +} + /// Best-effort sandbox violation recovered from OS-native logging. /// /// On macOS, Seatbelt does not stream deny events back to the supervisor like diff --git a/crates/nono/src/lib.rs b/crates/nono/src/lib.rs index cfa3293ce..9f9a3308c 100644 --- a/crates/nono/src/lib.rs +++ b/crates/nono/src/lib.rs @@ -70,8 +70,8 @@ pub use capability::{ pub use diagnostic::{ DenialReason, DenialRecord, IpcDenialRecord, NonoDiagnostic, NonoDiagnosticCode, NonoDiagnosticDetail, NonoDiagnosticSeverity, NonoRemediation, SandboxViolation, - SessionDiagnosticReport, SessionObservationInput, StderrObservationKind, dedupe_denials, - filesystem_denials_from_violations, follow_up_diagnostics, + SessionDiagnosticReport, SessionObservationInput, StderrObservationKind, UrlDenialReason, + UrlDenialRecord, dedupe_denials, filesystem_denials_from_violations, follow_up_diagnostics, }; pub use error::{NonoError, Result}; pub use keystore::{