Skip to content

Enforce the safety limits in the engines, not only in Ops - #198

Merged
fstubner merged 1 commit into
mainfrom
fix/core-safety-bypasses
Aug 18, 2026
Merged

fstubner merged 1 commit into
mainfrom
fix/core-safety-bypasses

Conversation

@fstubner

Copy link
Copy Markdown
Owner

Five bypasses found by the independent core review. Same shape in every case: Ops validated, the engine underneath did not — and the engines are re-exported at the crate root, so a consumer building input by hand reached them directly and the documented limits simply did not apply.

entry point what was missing
PortScanner::scan_host port cap, port-0 rejection
InspectEngine::inspect same
DiscoverEngine::scan_subnet /16 subnet cap
PcapEngine::capture any required stopping condition
PcapEngine::parse_file bound on retained packets
five constructors panicked instead of clamping concurrency

Ops::resolve_ports even carried a doc comment claiming "every scanning entry point funnels through here so the limits hold no matter which surface the call arrives on". Two of them did not.

What each one cost

  • A hand-built Vec<u16> skipped the 4,096-port cap and the port-0 rejection.
  • scan_subnet("0.0.0.0/0") collects 4,294,967,294 addresses, roughly 73 GB, before sending a packet.
  • PcapConfig has all-public fields and the helper guaranteeing a stopping condition lives in ops — so duration: None, max_packets: None gave a loop that only breaks on cancel, and capture takes no cancel token. It wrote until the disk filled.
  • parse_file treated None as "retain every packet": a 500 MB file of 10M small frames built 10M summaries, ~5 GB, each with a 191-char hex preview.
  • concurrency.max(1) left the upper bound to Semaphore::new, which asserts above usize::MAX >> 3. Absurd input, but a constructor returning Self cannot report an error, so it aborted the process.

Signature changes

Both land in the unreleased 0.3.0:

PortScanner::scan_host{,_with_progress}       -> Result<Vec<PortResult>>
DiscoverEngine::scan_subnet{,_with_progress}  -> Result<Vec<Host>>

SweepEngine treats a per-host scan error as "no open ports" rather than aborting the sweep: the port list is validated once by the caller, so a failure there would be the same error repeated for every host.

capture::capture was left unreachable by routing capture through capture_with_cancel, so it is deleted rather than kept as dead code.

Verification

Four new tests in config_safety.rs pin the engine-level checks, because every one of these is silent — the old code returned results rather than failing.

151 tests pass; cargo fmt and clippy clean with and without pcap.

Five bypasses found by the independent core review. Same shape in every
case: `Ops` validated, the engine underneath did not, and the engines are
re-exported at the crate root -- so a consumer building input by hand
reached them directly and the documented limits simply did not apply.

  PortScanner::scan_host          no port cap, no port-0 rejection
  InspectEngine::inspect          same
  DiscoverEngine::scan_subnet     no /16 subnet cap
  PcapEngine::capture             no stopping condition required
  PcapEngine::parse_file          no bound on retained packets
  five constructors               panic instead of clamping concurrency

`Ops::resolve_ports` even carried a doc comment claiming "every scanning
entry point funnels through here so the limits hold no matter which surface
the call arrives on". Two of them did not.

What each one cost:

- A hand-built `Vec<u16>` skipped the 4,096-port cap and port-0 rejection.
- `scan_subnet("0.0.0.0/0")` collects 4,294,967,294 addresses, roughly
  73 GB, before sending a packet.
- `PcapConfig` has all-public fields and the helper guaranteeing a stopping
  condition lives in `ops`, so `duration: None, max_packets: None` gave a
  loop that only breaks on cancel -- and `capture` takes no cancel token. It
  wrote until the disk filled.
- `parse_file` treated `None` as "retain every packet": a 500 MB file of 10M
  small frames built 10M summaries, ~5 GB, each with a 191-char hex preview.
- `concurrency.max(1)` left the upper bound to `Semaphore::new`, which
  asserts above `usize::MAX >> 3`. Absurd input, but a constructor returning
  `Self` cannot report an error, so it aborted the process.

Signature changes, all in the unreleased 0.3.0:

  PortScanner::scan_host{,_with_progress}   -> Result<Vec<PortResult>>
  DiscoverEngine::scan_subnet{,_with_progress} -> Result<Vec<Host>>

`SweepEngine` treats a per-host scan error as "no open ports" rather than
aborting the sweep: the port list is validated once by the caller, so a
failure there would be the same error repeated for every host.

`capture::capture` was left unreachable by routing `capture` through
`capture_with_cancel`, so it is deleted rather than kept as dead code.

Four new tests in config_safety.rs pin the engine-level checks, because
every one of these is silent -- the old code returned results rather than
failing. 151 tests pass, fmt and clippy clean with and without pcap.
@fstubner
fstubner merged commit 97e958b into main Aug 18, 2026
15 checks passed
@fstubner
fstubner deleted the fix/core-safety-bypasses branch August 18, 2026 09:55
fstubner added a commit that referenced this pull request Aug 19, 2026
* Correct AGENTS.md against the current tree

AGENTS.md is the first file an agent reads, so every stale line in it
misdirects work before any code is opened. Verified each claim below
against the source rather than the previous text.

- Dependency versions: clap 4.4 -> 4.6, ratatui 0.29 -> 0.30,
  crossterm 0.27 -> 0.29, and the tui-textarea -> ratatui-textarea 0.9
  swap that was never recorded (apps/netscli-cli/Cargo.toml).
- Traceroute: the Unix path was described as manual ICMP TTL probes.
  crates/netscli-core/src/trace.rs shells out on every platform --
  tracert on Windows, traceroute with a tracepath fallback elsewhere.
- CLI subcommands: `config` and `export` do not exist as subcommands
  (they are TUI slash-commands), and `completions` and `man` were
  missing. List now matches the Commands enum in args.rs.
- File tables: `tui.rs` is now `tui/`, `formatter.rs` is
  `tui_formatter.rs`, CLI-side `trace.rs` is a one-line re-export, and
  cli_dispatch/commands/output were absent. Core table gained trace,
  mdns and error; `scan.rs` is a directory module.
- Safety limits are enforced in ops/validation.rs and common/ports/,
  and re-checked in the engines (#198), not in ops.rs/server.rs.
- The GUI does have a JS/TS linter (eslint.config.js, npm run lint).
- Feature flags listed only `pcap`; `mdns` and `db` also exist and
  decide what a published build actually contains.
- Layout block invented a root data/ directory (the OUI dataset ships
  in crates/netscli-core/data/) and omitted site/, docs/, packaging/.

* Document the packet-capture tab as it actually behaves

PR #193 stopped hiding the Packet Capture tool on builds without capture
support, because no published installer ships with it and every real
user hit a tool that was simply absent with no explanation. The docs
still described the old behaviour, so the two pages that cover build
availability told users to look for a tab that is always there anyway.

desktop.md now says the tab is always listed and shows setup guidance
when it cannot run, and that hiding applies to mDNS Discovery instead.
interface-coverage.md line 58 said Packet Capture "appears only in"
capture builds, contradicting its own line 60 two bullets later; it is
now about whether capture runs, plus the one place the tool genuinely
disappears -- the CLI `pcap` subcommand, which is cfg-gated in args.rs.

Ground truth: apps/netscli-gui/src/hooks/useTauriRuntimeState.ts:97-116.

* Fix the /scan example in the TUI docs

The TUI takes ports positionally. `handle_scan` in
apps/netscli-cli/src/tui/events/scan.rs passes `parts.get(2)` straight
to `parse_ports_checked`, so the documented `/scan 192.168.1.1 -p
22,80,443` feeds "-p" in as the port list and errors out. Anyone
copying the example off the page got a failure on their first command.

Checked every other example on the page against
apps/netscli-cli/src/tui/command_catalog.rs; the rest are correct.

* Stop overstating what the MCP server exposes

Three separate claims about the MCP tool surface were wrong, and an
agent reading any of them would go looking for a tool that is not there.

- README line 37 said ten tools in the `-pcap` build while line 504 said
  13. The pcap block in crates/netscli-mcp/src/server/tools.rs:106-162
  adds four tools, so 13 is right and the two lines now agree.
- README line 508 and crates/netscli-mcp/README.md line 17 both said
  `dns_lookup` does reverse lookups. It does not: ops/host.rs:60-78
  passes the host through unchanged and nothing in dns/lookup.rs
  converts an IP to in-addr.arpa form, so a PTR query on a bare address
  returns nothing. There is no reverse tool on the MCP surface at all,
  which is what docs/interface-coverage.md already said.
- The landing page said every CLI operation is exposed as a structured
  tool. Trace and reverse are not, so it now names the operations that
  are rather than making a blanket claim.

* Record that the Windows packet-capture paths are untested

The pcap test steps in .github/workflows/ci.yml are gated
`if: runner.os == 'Linux'`, and the feature cannot be built on Windows
at all without the Npcap SDK -- the link step fails with LNK1181 on
wpcap.lib. That leaves the Npcap code paths, which are the whole reason
the feature exists on Windows, covered by no test anywhere.

Belongs in "What has never been reviewed" for the same reason the rest
of that section does: someone picking this up should not assume a green
CI run says anything about Windows capture.

* Update the site fork-me instructions to the current layout

site/README.md is the file someone reads before reusing the scaffold,
so every stale pointer in it sends them to a file that no longer holds
what it claims.

- Content moved from src/data/site.ts into src/data/site-content/.
  site.ts is now the assembler, not the thing you edit. Same correction
  applies to the analytics token and the repo slug, both of which live
  in site-content/footer.ts.
- Astro 6 -> Astro 7 with Starlight (site/package.json pins ^7.2.0).
- The download counter no longer leaves an em-dash placeholder on
  failure. src/scripts/landing-page.ts starts every metric hidden and
  reveals it only once a request confirms it, hiding the separator
  beside it too.
- The layout tree predates the docs site: added content/docs, scripts,
  src/scripts, and widened styles beyond global.css.

* Merge the duplicate 0.3.0 changelog sections

The 0.3.0 block carried two `### Changed` headings and two
`### Changed (internal)` headings in a file that declares Keep a
Changelog conformance, which asks for one heading per change type.
Entries are moved under the first heading of their kind with their
wording untouched -- verified by sorting the old and new 0.3.0 blocks
and diffing: the only differences are the removed headings, their blank
lines, and the correction below.

The dead-CSS entry claimed a budget of 210. CI enforces 126 via
`npm run check:css` (site/package.json), and the checker currently
reports exactly 126 of 1643 declarations, so 210 would have read as
headroom that does not exist.

* Align the GUI lint comment in the AGENTS.md command block
fstubner added a commit that referenced this pull request Sep 14, 2026
The 0.3.1 entry was written as diffs between working states during the
cycle, not as a description of what changed for someone on 0.2.6. The
tabbed desktop workspace is new in this release, so eleven entries
describing its parts -- tab reordering, right-click tab menus, the
context menu's danger styling, which tab the app opens on, notification
routing between tabs -- documented constituents of one new thing as
though each were a separate change.

Git archaeology against v0.2.6 sorted the Fixed section by path. The new
GUI never shipped, so a fix touching only apps/netscli-gui was invisible
to users: the Stop camelCase bug (#290), crash-vs-cancel (#289), the
refused-stop message (#291), the concurrency-default-1 preference bug
(#248, #223) and the settings dialog centring all came out. Fixes in
crates/ or apps/netscli-cli shipped in 0.2.6 and stayed: ping on Windows
(#246), IPv6 ping (#247), the three ARP honesty fixes (#262), the
neighbour-table merge (#243), engine safety limits (#198) and the MCP
hardening (#225).

Also dropped: GUI render automation and dependency bumps, which are
internal, and the per-PR Cloudflare previews.

438 lines to 202. No user-facing entry was removed -- the desktop app's
capabilities are now listed under the one entry that introduces it.
fstubner added a commit that referenced this pull request Sep 14, 2026
* Drop the internal-changes section from the changelog

Refactors, CI plumbing and dependency bumps are already in git history and
in the PRs they link to, more precisely than a prose summary states them.
Nobody reading release notes can act on 'internals reduced from monolithic
files into facades' -- a line whose own second half says the public API did
not change.

Removed from 0.3.1 and 0.2.6, the only two releases that carried one.

sectionLabel's 'changed internal' branch goes with it. It was already dead:
the function strips parentheticals before comparing, so 'Changed (internal)'
normalised to 'changed' and matched the branch above it. Nothing referenced
the 'internal changes' label it returned.

* Restructure the 0.3.1 notes around what changed for users

The 0.3.1 entry was written as diffs between working states during the
cycle, not as a description of what changed for someone on 0.2.6. The
tabbed desktop workspace is new in this release, so eleven entries
describing its parts -- tab reordering, right-click tab menus, the
context menu's danger styling, which tab the app opens on, notification
routing between tabs -- documented constituents of one new thing as
though each were a separate change.

Git archaeology against v0.2.6 sorted the Fixed section by path. The new
GUI never shipped, so a fix touching only apps/netscli-gui was invisible
to users: the Stop camelCase bug (#290), crash-vs-cancel (#289), the
refused-stop message (#291), the concurrency-default-1 preference bug
(#248, #223) and the settings dialog centring all came out. Fixes in
crates/ or apps/netscli-cli shipped in 0.2.6 and stayed: ping on Windows
(#246), IPv6 ping (#247), the three ARP honesty fixes (#262), the
neighbour-table merge (#243), engine safety limits (#198) and the MCP
hardening (#225).

Also dropped: GUI render automation and dependency bumps, which are
internal, and the per-PR Cloudflare previews.

438 lines to 202. No user-facing entry was removed -- the desktop app's
capabilities are now listed under the one entry that introduces it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant