Skip to content

fix: audit round 2 -- redirects, resolver, AMS external spool, layer checker - #140

Closed
DLANSAMA wants to merge 13 commits into
mainfrom
feat/audit-ams-external-spool-and-contracts
Closed

DLANSAMA wants to merge 13 commits into
mainfrom
feat/audit-ams-external-spool-and-contracts

Conversation

@DLANSAMA

Copy link
Copy Markdown
Owner

What and why

Second adversarial audit pass on the audit-fix branch. This PR carries the earlier audit-fix commits (network/TLS hardening, slicer XML safety, MQTT retries, archive preservation, AMS external spool, redirect/HTTPError fix) plus a new commit closing what that last fix got wrong or left untested.

Fixes in the new commit:

  • Cross-host redirects forwarded credential headers. urllib copies every request header onto the redirected request. SafeHTTPRedirectHandler now drops Authorization / Proxy-Authorization / Cookie when the host changes and re-picks the User-Agent for the new host. Nothing in platecli sends credentials today; this closes the hole before anyone does. Verified live against a local server (first hop on one host, second on another).
  • Resolver in-flight slot could be poisoned for the process lifetime. A failed Thread.start() never set the done event, so every later _try_resolve_ip for that host waited its full timeout and returned None, surfacing as a bogus "Invalid printer_ip or hostname". Guarded.
  • Resolver cache had no TTL. The resolved IP is handed to paho/ftplib as a literal, so a long-lived TUI session was pinned to a stale DHCP lease. 300 s TTL; the false "transports re-resolve anyway" docstring is corrected.
  • check_layers.py only judged the base of a from-import. from bambu_cli import protocols, from .. import protocols and from bambu_cli.printables import client (a sealed module) all passed. The checker now expands every imported name.

Tests added for lines that mutation testing showed were unpinned: the throttle sleeping the full backlog, the post-redirect URL re-validation in the downloader, the success half of the hostname check through main(), and the oversized slice_info fallback log. Each new test was confirmed to fail against its mutation.

Visible behaviour change carried from the earlier commit: a 404 download now exits 2 (network error) instead of 3.

Checklist

  • uvx ruff check bambu_cli — no lint errors
  • uvx ruff format --check bambu_cli — no formatting drift
  • uvx mypy -p bambu_cli — no type errors
  • uvx bandit -c pyproject.toml -r bambu_cli -ll — no new medium/high findings
  • uv run python -m pytest tests/ -q -m "not live" — 1670 passed, 92.72% coverage (with -W error::ResourceWarning)
  • uv run python tests/python_compat_smoke.py — Python 3.9 floor holds

Also run: check_layers, gen_schemas --check, syntax_smoke, cli_help_smoke, ci_workflow_smoke, privacy_smoke, dependency_resolution_smoke, agent_cli_smoke — all exit 0.

…-HOME first run

README: move "Print something" (step 1 = install OrcaSlicer) above "Try it in
30 seconds", retitle the nav link so no "30 seconds" precedes the first
OrcaSlicer mention, and say the sim needs neither a printer nor OrcaSlicer
while a real print needs both. Anchors unchanged.

Tests (tests/test_first_run_path.py, section 6) lock the empty-HOME contract
a newcomer hits, so the walk is deterministic without a printer:
- `plate preflight` exits 1 and reports config, orca-slicer, and profiles-dir
  as three separate checks (human and --json), each with its own next step.
- `plate setup --sim` with a non-TTY stdin exits 1 with the headless message
  naming --printer-ip/--serial/--access-code-file, no traceback.
- `plate --sim status` with an empty HOME still exits 0 and reports IDLE.

No CLI behaviour changed; no protocol code touched.
The project was credited to "platecli contributors", so the PyPI author
field, the wheel/sdist METADATA, and the copyright line carried no real
name. Replace it with the actual author and a real contact address.

- pyproject.toml: authors and maintainers become Dylan Reed with the
  project's public contact address. PyPI renders it on the project page,
  which is the intent -- someone evaluating the project can reach the
  author without going through GitHub.
- LICENSE: copyright holder is now a named person, which is what makes
  the MIT grant unambiguous.
- tests/package_contents_smoke.py: the LICENSE snippet assertion follows.

tests/privacy_smoke.py gains a PUBLISHED_AUTHOR_IDENTIFIERS allowlist
holding exactly the published contact identity. That gate flags every
email address in the tree and in the release archives, so without an
entry it would red CI on the one address that is supposed to be there.
The allowlist is span-based rather than exact-match so a substring hit --
a local-account-name pattern derived from a contributor's git config can
match inside the published address or name -- is exempt too.

The exception is deliberately narrow, and unit-verified as such: a
personal address, a GitHub noreply address, an arbitrary third-party
address, and a lookalike suffix (the published address followed by
another domain) all still fail the gate. Only the published address
passes.

Verified: twine check passes both artifacts; a byte-level scan of all 344
wheel + sdist members finds the published address in 6 metadata/README
members and no other address anywhere; privacy_smoke --include-dist
reports zero findings against dist/. pytest -m "not live" 1535 passed at
91.1% coverage; ruff check, ruff format, mypy -p bambu_cli, bandit,
package_contents_smoke, ci_workflow_smoke, python_compat_smoke clean.
Someone evaluating the project could not tell who wrote it or how well it
is tested without reading the CI config. Both are now above the fold.

- Tests (1,535 passing) and coverage (91%) badges next to the existing CI
  and PyPI ones, linked to a new "Tested and hardened" section. They are
  static shields.io badges -- no coverage service is wired up -- so an
  HTML comment records the command to re-measure them at release time.
- "Who it's for": four concrete audiences (terminal-first makers, homelab
  and print-farm users, agents/automation, locked-down networks) after the
  pipeline diagram, so the reader self-selects before the spec details.
- "Tested and hardened" section: the measured test and coverage numbers
  against the 86% CI floor, the OS/Python matrix, the blocking gate list,
  the mutation-testing scope, and the wheel/sdist install smokes. Notes
  that the coverage config does not exclude error paths, so the figure is
  not inflated.
- Byline under the nav line, an Author section carrying the contact
  address, and "MIT (c) 2026 Dylan Reed" on the licence line. Project
  questions are still routed to issues, discussions and SECURITY.md so
  the mailbox is for work enquiries rather than support triage.
- "Support & expectations" now names the maintainer instead of saying
  "one person".

Numbers are measured on this tree at v0.5.1, not estimated:
pytest -m "not live" reports 1535 passed and TOTAL 91.1%.
… bounds

build_safe_opener assembled an OpenerDirector by hand and never added
HTTPErrorProcessor. That processor is what routes a non-2xx response into
parent.error(), so without it the opener returned 3xx/4xx/5xx bodies as
successes: redirects were never followed, HTTPError was never raised, and the
redirect hop cap plus the 429/503 retry in polite_open were unreachable code.
Any download behind a redirect failed with "Downloaded file is empty", and a
404 was misreported as "HTML page did not contain a direct model file link"
with no http_status. Latent since the netsafety split.

Raising on the hop cap then exposed a second leak: HTTPRedirectHandler's
http_error_302 calls redirect_request before its own fp.read()/fp.close(), so
bailing out there left the intermediate response's socket to the collector.
redirect_request now closes fp itself.

Other fixes:

- ams: tray_now 254 (external spool) and 255 (nothing loaded) are distinct
  states. Treating both as active put a false active filament in status --json
  and a "play" marker next to the external spool on an idle printer. Both
  sentinels are now named constants.
- cli: the printer_ip hostname check ran _resolve_ip and then a second,
  timeout-less socket.getaddrinfo, reintroducing the unbounded lookup the
  timeout exists to prevent. New utils._try_resolve_ip returns None on failure
  or timeout so the caller can branch without a second lookup; in-flight
  resolutions are shared per host, so a blackholed resolver parks one thread
  rather than one per call.
- download: the preserved multi-model archive name skipped
  _sanitize_download_filename, so a long URL path failed os.replace with
  ENAMETOOLONG into a swallowed except and stranded the archive under its
  hidden temp name. Windows reserved stems and control characters got through
  too. The swallowed OSError now logs.
- check_layers: relative imports were skipped as "same unit by construction",
  which only holds at level=1 in a top-level module. From a subpackage,
  "from ..protocols.ftps import x" crossed units unseen - the exact breach the
  checker exists to catch. They are now resolved to absolute paths. Verified
  against an injected violation.
- netsafety: _throttle_host clamped its sleep to one interval, so queued
  callers under-slept and burst past the throttle; the bound is now
  MAX_RETRY_AFTER_WAIT.
- slicer: an oversized slice_info.config now logs when it falls back to the
  gcode header instead of degrading silently.

Tests: new tests/test_netsafety_opener.py drives a live local server rather
than calling handlers directly - the existing redirect tests all invoked
redirect_request() by hand or asserted handler registration, both of which
pass against a chain that can never invoke the handler. Confirmed the core
cases fail with the fix reverted. Also covers per-hop SSRF re-validation,
which was untestable while redirects were inert.

The fd-counting variant of the socket-leak test was written and then removed:
wrap_socket detaches the socket and refcounting closes the fd as the exception
propagates, so it passed against a deliberately reintroduced leak. The
explicit close assertion is what catches that regression; the file now says so.

Note: a 404 download now exits 2 (EXIT_NETWORK_ERROR) instead of 3. That is
what the handler always intended, but it is visible in --json.

1652 passed, 92.69% coverage; ruff, mypy, bandit, check_layers, gen_schemas
and all smokes green.
…resolver, see through from-imports

Follow-up to b25607b from a second adversarial pass. Each production change
below had a mutation that left the suite green; each now has a test that goes
red against that mutation.

netsafety: urllib copies every request header onto a redirected request
whatever host it names, so an Authorization or Cookie header sent to host A
followed a 302 to host B. Nothing in platecli sends credentials today; the
redirect handler now drops them on a host change so the first caller that does
cannot leak a token, and re-picks the User-Agent for the new host (the policy is
per host: honest token for first-party API hosts, browser-shaped elsewhere).

utils._try_resolve_ip: a failed Thread.start() left an in-flight entry whose
done event was never set, so every later call for that host waited its full
timeout and returned None for the rest of the process -- surfacing as a bogus
"Invalid printer_ip or hostname". The start is now guarded. The resolver cache
also gets a 300 s TTL: the resolved IP is handed to paho and ftplib as a literal
(they never re-resolve, contrary to what the old docstring claimed), so a
permanent cache pinned a long-lived TUI session to a stale DHCP lease.

check_layers: a from-import names a base and a list of names, and a name can be
a submodule. Judging the base alone let `from bambu_cli import protocols`,
`from .. import protocols` and `from bambu_cli.printables import client` (the
sealed module) pass the checker. import_targets now yields base.name for every
name; the checker still passes on the real tree.

Tests added for the untested lines found by mutation: the throttle sleeping the
full backlog rather than one interval, the post-redirect URL re-validation in
the downloader, the success half of the hostname check through main(), and the
oversized slice_info fallback log. test_jsonio adjusted for the cache tuple.

1670 passed, 92.72% coverage; ruff, ruff format, mypy, bandit, check_layers,
gen_schemas and all smokes green.
@DLANSAMA
DLANSAMA marked this pull request as ready for review September 18, 2026 01:45
The reserved-range check refused all of 64:ff9b::/96, breaking every
download on DNS64 (IPv6-only) networks. Unwrap the embedded IPv4 instead
so public hosts pass and private ones are refused. Also reject fec0::/10,
which stdlib ipaddress reports as global. Changelog wording for the FTP
path check corrected to defense in depth.
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