Skip to content

feat: add interactive network approval and domain rejection#698

Open
klassm wants to merge 5 commits into
nolabs-ai:mainfrom
klassm:network_permission_handling
Open

feat: add interactive network approval and domain rejection#698
klassm wants to merge 5 commits into
nolabs-ai:mainfrom
klassm:network_permission_handling

Conversation

@klassm

@klassm klassm commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

When a sandboxed process requests access to a blocked host, the user is now prompted via an OS dialog to approve or deny the request. Approved hosts are allowed for the session; optionally persisted to the active profile for permanent access.

  • OS-native dialogs: macOS (choose from list), Linux (notify-rust with XDG action buttons), Windows (PowerShell WPF dialog)
  • Four choices: Allow once, Always allow, Deny once, Always deny
  • Always allow/deny persists the decision to the profile config file
  • New reject_domain config field with wildcard support (e.g. *.ad.com) takes priority over allow_domain, enabling explicit host blacklisting
  • Approval is off by default; opt in via --network-approval ask, NONO_NETWORK_APPROVAL=ask, or approval_mode in config/profile

@klassm klassm force-pushed the network_permission_handling branch from db2ec7c to 29e89e2 Compare April 17, 2026 19:17

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a network approval feature, allowing users to interactively approve or deny network requests to unknown hosts via OS-level notifications. The implementation includes cross-platform support (Linux, macOS, Windows), a runtime-mutable host filter, and persistence logic for user decisions. I have identified several issues: the approval backend creates a new Tokio runtime on every call, which is inefficient; persistence errors are handled inconsistently; JSON manipulation in the config writer is prone to panics; and the timeout logic is missing or ignored in the notification backends.

Comment thread crates/nono-cli/src/network_approval.rs Outdated
Comment thread crates/nono-cli/src/network_approval.rs
Comment thread crates/nono-cli/src/network_approval.rs
Comment thread crates/nono-cli/src/notification/linux.rs Outdated
Comment thread crates/nono-cli/src/notification/macos.rs
@klassm klassm force-pushed the network_permission_handling branch from 29e89e2 to 2282f15 Compare April 17, 2026 19:30
@klassm

klassm commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

All review comments done. Would be cool to have this - this makes maintaining the allow list a lot easier.

@lukehinds

Copy link
Copy Markdown
Contributor

This is a really good idea @klassm - I am away this weekend, but will take a good look first thing Monday - out of interest , it might be good to rig this up to the Approval backend on linux (unless you already have), but that could be a follow up patch.

@klassm

klassm commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Great, thanks @lukehinds :-). Approval backend on linux also sounds nice - I would not do it directly, though. I don't have a Linux system, so that's hard to test :-)

@tho

tho commented Apr 20, 2026

Copy link
Copy Markdown

Interesting feature! One small UX thought: "allow once" might be ambiguous since it actually allows for the whole session, not just a single request. Maybe something like:

  1. [Allow/Deny] once (single request)
  2. [Allow/Deny] session
  3. [Allow/Deny] always (persist to profile)

Also, what do you think about a dropdown with those three options plus Allow/Deny buttons instead of four (or more) separate buttons?

@klassm klassm force-pushed the network_permission_handling branch 3 times, most recently from f2586e1 to 10c4c23 Compare April 20, 2026 15:53
@klassm

klassm commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

@tho I changed the buttons to distinguish between once, session and always.
For MacOS you get a dropdown, for Windows I guess as well. For Linux I did not figure out how to do that.

@klassm klassm force-pushed the network_permission_handling branch 2 times, most recently from 1bb15a3 to 273f6a7 Compare April 20, 2026 15:58
@klassm

klassm commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@lukehinds Did you have a chance to have a look again? 😇

@klassm klassm force-pushed the network_permission_handling branch 2 times, most recently from 3ec989e to 8912421 Compare April 24, 2026 07:14
@lukehinds

Copy link
Copy Markdown
Contributor

hi @klassm , I have and I promise I am keeping an eye on this, its just a big change which commits a lot to the supervisor backend arch, so i need to make sure its malable to other planned changes.

@klassm klassm force-pushed the network_permission_handling branch from 8912421 to 170d100 Compare April 27, 2026 13:08
@klassm klassm force-pushed the network_permission_handling branch 3 times, most recently from 82926e1 to 6b1d919 Compare May 9, 2026 16:03
@klassm klassm force-pushed the network_permission_handling branch from 6b1d919 to fc7b7e9 Compare May 28, 2026 06:40
@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +5732
Lines removed -71
Total changed 5803
Classification Large (> 300 lines)

Affected crates

  • crates/nono (core library) — careful review required. This is the security-critical sandbox primitive. A bug here bypasses OS-level isolation for every downstream user.
  • crates/nono-proxydownstream consumers depend on this crate. API or behaviour changes will affect external callers; treat any breaking change with extra scrutiny.
  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.
  • bindings/c (C FFI) — ABI changes can silently break C callers. Confirm header and symbol compatibility.

Blast radius — Moderate

This PR touches: source code,documentation


Updated automatically on each push to this PR.

@klassm klassm force-pushed the network_permission_handling branch from cbef37a to 82233c1 Compare May 31, 2026 08:45
lukehinds and others added 3 commits June 1, 2026 20:43
- Deny/Approve buttons with Once/Session/Always duration dropdown
- Blacklist (reject_domain) hosts auto-denied without prompting
- Once approval bypasses runtime filter re-check
- Deny(Session/Always) adds to runtime deny filter and persists
- Tests for filter blacklist, deny scope, approval scope
When multiple concurrent requests arrive for the same host, the dedup
mechanism in NetworkApprovalBackend used a Notify-only pending map.
After the first request's dialog was resolved, waiting requests would
always return Granted(Session) regardless of the actual decision,
because they only checked whether the pending entry was removed.

Replace the pending map value from Arc<Notify> to Arc<PendingEntry>
which stores both the Notify and the actual decision. The first
request writes its decision into the slot before notifying waiters,
and waiters read the actual decision instead of assuming Granted.

This fixes both:
- Repeated approval popups after denying a host (denied waiters
  still got Granted, causing the next request to trigger a new dialog)
- A security bug where denied hosts were approved for concurrent
  waiting requests

Signed-off-by: klassm <klassm@users.noreply.github.com>
When network_profile was None (the default case), config_writer was
None, causing 'Approve Always' to silently degrade to session-only.
The host was only added to the in-memory runtime filter and never
written to the profile JSON, so the approval popup reappeared on
every session.

Now always create a ConfigWriter, falling back to the 'default'
profile name when no network_profile is explicitly configured.

Signed-off-by: klassm <klassm@users.noreply.github.com>
@klassm klassm force-pushed the network_permission_handling branch from 2cbbea4 to fe656d9 Compare June 1, 2026 18:53
klassm added 2 commits June 4, 2026 10:00
The approval channel receiver loop processes requests sequentially.
When multiple CONNECT requests for the same host queued in the channel,
each one would show a separate approval dialog because the approval
backend never re-checked the runtime filter before entering the dialog
path. After the first request was approved (adding the host to the
runtime filter), subsequent queued requests should have been granted
immediately without a dialog.

Now request_network_approval_async checks the runtime filter first:
- If the host is already allowed, returns Granted(Session) immediately
- If the host is explicitly denied (DenyHost/DenyLinkLocal), returns
  Denied immediately
- Otherwise, proceeds to the dialog/dedup path as before

Signed-off-by: klassm <klassm@users.noreply.github.com>
… proxy, CRLF

Bug 4 (HIGH): External proxy path bypassed network approval entirely.
When an external proxy was configured and the host was not bypassed,
handle_external_proxy was called without checking the runtime filter
or sending approval requests. Now routes through
handle_connect_with_approval when approval context is available.

Bug 9 (HIGH): DNS rebinding vulnerability on Once-approved connections.
resolve_host did DNS resolution without checking link-local IPs. A
malicious domain could resolve to 169.254.169.254 on the second
lookup, accessing cloud metadata. Now uses check_host which validates
link-local IPs, and denies the connection if the check fails.

Bug 17 (HIGH): DenyLinkLocal not blocked before approval channel.
handle_connect_with_approval only short-circuited on DenyHost, not
DenyLinkLocal. A link-local IP could reach the approval dialog. Now
both DenyHost and DenyLinkLocal are blocked before the approval
channel in both primary and runtime filter checks.

Bug 2 (MEDIUM): Double check_host call in request_network_approval_async.
The runtime filter was checked twice consecutively (once for
is_allowed, once for DenyHost/DenyLinkLocal match). Now checks once
and branches on the stored result.

Bug 1 (LOW): is_denied() on NetworkApprovalDecision did not consider
Timeout variant. A timeout is semantically a denial, so Timeout is
now included in is_denied().

Bug 12 (LOW): external.rs send_response did not sanitize CRLF in
reason strings, inconsistent with connect.rs. Now sanitizes like
connect.rs does.

Signed-off-by: klassm <klassm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants