feat(peer): harden reflexive discovery (cross-family reject, stun host, mirror-advertise control)

[MichaelTaylor3d]

Four items in the reflexive-discovery family, batched into one PR/version bump per
CLAUDE.md's PR-batching rule (every root Cargo.toml bump dirties every sibling PR).

1. Reject a cross-family STUN answer and fall through the ladder
   (crates/dig-node-core/src/seams/dig_peer/net.rs).
2. Correct the "INTERIM" framing on the public-STUN fallback tier (it is standing).
3. Prefer stun.<relay-host>, falling back to the bare relay host
   (prefer_dedicated_stun_host, same file).
4. Serve control.config.setMirrorAdvertiseUrls (dig-node-control-interface 0.33.0
   adopted; crates/dig-node-service/src/control.rs, control_cli.rs, a new
   dign config set-mirror-advertise-urls verb).

requires_restart — verified true is the honest answer, and WHY

Not because of the env var alone (the persisted form works fine live for
control.config.get) but because of a real structural fact: server.rs's mirror
lifecycle task captures configured_operator_urls() in a local binding once,
before its pass loop starts (server.rs:2776, comment: "read ONCE for the life of
the task ... re-reading it per pass would buy nothing"). No in-process call can
rewrite that captured value; only a restart re-enters the function. The write IS
genuinely persisted to config.json (set_mirror_advertise_urls_override /
read_mirror_advertise_urls_override_from, control.rs:921-955) and picked up by
advertised_urls_from_env's env-else-persisted precedence on the NEXT start, so the
requires_restart: true promise is real, not a stand-in for unimplemented work —
same class of honest-true as config.setUpstream's existing answer.

Breaking-change (!) justification — re-verified, not just carried over

Two real behaviour changes justify it, both "changed defaults" under this repo's own
SemVer convention (§2.4):

• Cross-family STUN rejection replaces an implicit accept-anything read: a node
  that previously got a (possibly wrong) non-null address from a cross-family answer
  can now get NoPublicAddress/fall through instead. A prior test asserting
  first-answer-wins had to be updated for exactly this reason — see below.
• STUN source precedence changes from one fixed source (relay's co-located STUN)
  to a three-tier plan (operator override -> relay's stun.<host>/bare host ->
  public Google/Cloudflare fallback), which can change which address a node ends up
  advertising.

Fixed since the last push (this session)

1. Version increment — branch sat at 0.254.83, equal to main (main had
   caught up). Bumped root Cargo.toml to 0.254.84, cargo update -w --offline
   to sync Cargo.lock (only dig-node-service moved, as expected — it is the one
   crate on version.workspace = true).
2. Commitlint — the failing check was the PR TITLE (144 chars, max 100), not
   a commit message (both commits only carried a non-fatal footer-leading-blank
   warning). Retitled to 98 chars, keeping the ! (justified above).
3. Test + coverage — control_contract_conformance::the_unpublished_list_still_ describes_real_drift failed: control.wallet.resetCoinDb is published now -- remove it from KNOWN_UNPUBLISHED. Verified against the real crate source
   (dig-node-control-interface 0.32+ declares ControlMethod::WalletResetCoinDb)
   that this is genuine catch-up drift from the 0.33 bump, not something this PR's
   own logic caused. Removed the entry (control.rs:337) and rewrote the stale doc
   comment. Confirmed safe by tracing every reference to resetCoinDb (15 hits) and
   the tier-delegation chain: requires_master_token_given now delegates to
   ControlMethod::WalletResetCoinDb.requires_master_token(), which the crate
   returns false for — the same PAIRED tier the exemption used to grant by name,
   now delegated to the contract instead of carved out locally (exactly the end
   state the exemption's own doc comment said to reach). No count-based assertion
   on the list's length exists anywhere (grepped).

Blast radius checked

Single-array-literal edit (KNOWN_UNPUBLISHED_CONTROL_METHODS) plus its doc comment.
Traced all 15 references to control.wallet.resetCoinDb across control.rs,
control_cli.rs, entrypoint.rs by hand (no fresh gitnexus index built for this
worktree -- would have contended with the running test compile for CPU; grep-based
trace was exhaustive for a change this narrow). No other file references
KNOWN_UNPUBLISHED_CONTROL_METHODS's contents or length outside
control_contract_conformance.rs and control.rs's own test module, both read in
full.

Verified with the exact CI commands (all local, all after the fixes above)

• cargo fmt --all -- --check -- clean, exit 0.
• cargo clippy --workspace --all-targets --all-features --locked -- -D warnings --
  clean, exit 0 (1m30s).
• cargo test --workspace --all-targets --all-features --locked -- exit 0,
  every test result: line across every binary reads ok with 0 failed
  (~3132 passed total, matching CI's ~3142 baseline; this run used cargo test
  rather than nextest so the per-binary grouping differs slightly, but the count
  is real and every binary's own summary was checked, not just the final exit
  code). Confirmed by name: test the_unpublished_list_still_describes_real_drift ... ok, alongside its 3 sibling conformance tests in the same binary, all ok.
  (First attempt at full workspace parallelism OOM'd this box mid-link --
  rustc-LLVM ERROR: out of memory / undersized pagefile, zero tests executed,
  exit 101 -- an environment artifact of this machine's 32-core default
  parallelism against many heavy debug-info links at once, not a code defect.
  Retried with --jobs 4; clean.)

Not in scope (per the brief)

• Reflexive address is believed from ONE STUN source — require agreement between two before any durable or on-chain use #566 (agreement between independent sources) is the next lane's ticket, not this
  one's.
• No routability check added on the operator's URL input to
  setMirrorAdvertiseUrls -- feat(mirror): derive the advertised mirror URL from this node's reflexive peer address #562 deliberately lets an operator's LAN address stay
  publishable while a derived one must be global unicast; unchanged here.

closes=NONE -- the parent is a super-repo issue this submodule PR structurally
cannot close; closed by hand once merged.

[MichaelTaylor3d]

IN PROGRESS — not the verdict. Auditing head e966e35.

net.rs reviewed (STUN cross-family + stun. derivation):

• Cross-family reject (net.rs ~L787-800): discards only when dig_ip::Family::of(&result) != dig_ip::Family::of(&server). Verified both directions covered by revert-proof tests: IPv4-bound server answering IPv6 discarded (a_cross_family_stun_answer_is_discarded_and_the_ladder_falls_through), and the reverse (a_cross_family_stun_answer_is_discarded_the_other_direction_too). A legitimate same-family IPv6→IPv6 answer is NOT touched by this predicate — clean.
• No port-equality check introduced anywhere in the diff — confirmed by reading the full StunPlan::discover_reflexive body.
• stun. derivation (net.rs L440-480ish): built from parse_relay_host(relay_endpoint) — i.e. the OPERATOR's configured relay endpoint — never a hardcoded stun.relay.dig.net literal. Comment at L34-42 states the GPL-2.0/self-host rationale explicitly.
• Single shared helper confirmed: stun_server_from_relay (single, IPv6-first, traversal-ladder+DHT-transport) now DELEGATES to stun_servers_from_relay (the per-family list used by reflexive discovery) instead of re-deriving its own host — so both consumers can never diverge on which host they dial. This satisfies audit item 4.
• Comment-only fix (INTERIM -> STANDING, LAST-RESORT) verified at StunSource::Public doc + PUBLIC_STUN_SERVERS doc + stun_fallback_warning message — reads accurately, no behavior change alongside it.

Still to verify: control.rs (setMirrorAdvertiseUrls validation, Some(vec![])/None/Some(non-empty) semantics, requires_restart honesty), the breaking-change marker, and the fmt/string-continuation trap.

[MichaelTaylor3d]

IN PROGRESS -- not the verdict. Continuing audit of head e966e35.

control.rs / advertise.rs / control_cli.rs / entrypoint.rs reviewed:

• Item 5 (well-formedness ONLY, no routability check) -- CONFIRMED at the source: dig-node-control-interface v0.33.0 tag, src/params.rs:95-118, is_well_formed_advertise_url checks scheme+host only via url::Url::parse, with an explicit doc citing dig-node#562's routability-vs-well-formedness split and stating it is "deliberately NOT a routability or 'this machine only' check." dig-node's own config_set_mirror_advertise_urls (control.rs L231-281) adds no additional check beyond calling .validated() -- confirmed no routability check crept in at the consumer layer either.

• Item 6 (Some(vec![]) refused / None clears / Some(non-empty) sets) -- CONFIRMED end-to-end: SetMirrorAdvertiseUrlsParams::validated() (dig-node-control-interface v0.33.0 params.rs:141-165) refuses Some(empty) as INVALID_PARAMS, control.rs persists via set_mirror_advertise_urls_override (None removes key / Some writes key), and the CLI (entrypoint.rs L1035-1093, control_cli.rs L305-311) maps an empty Vec<String> CLI invocation to None -- so Some(vec![]) is structurally unreachable from the CLI and refused on the wire if sent directly. Round-trip persistence tests present (control.rs L7116-7146).

• Item 7 (requires_restart honesty) -- VERIFIED TRUE BY READING THE CODE PATH, not assumed: server.rs L2768-2776 reads configured_operator_urls() (which now consults the persisted override) exactly ONCE, before the mirror-lifecycle loop {, with an explicit comment explaining why ("read ONCE at bring-up ... configuration rather than an observation"). The DERIVED half (reflexive address) IS re-read every pass (L2882), but the OPERATOR override is not. So a live process genuinely cannot observe a setMirrorAdvertiseUrls write; only a restart re-reads it. requires_restart: true (control.rs L277) is unconditional and honest. ANSWER FOR THE DIG-APP LANE: requires_restart is TRUE, correctly so -- the UI must tell the operator a restart is needed; there is no live-apply path today.

• Authorization note (my own check, not in the original list): ControlMethod::ConfigSetMirrorAdvertiseUrls.requires_master_token() returns false (ordinary/paired-token tier), verified in dig-node-control-interface v0.33.0 method.rs:449-458. This persists past pairing.revoke (same as setUpstream), but the crate's own doc explains why deliberately: it "installs no principal this node will thereafter believe, obey, or forward requests to" -- it only changes what the node broadcasts about itself. Judged a considered, documented design tradeoff (precedented by cache.setCap/log.setLevel, cross-referenced to the repo's own open feat(updater): widen control.updater.setChannel doc + forwarding tests to nightly|stable #40 debate about setUpstream), not an oversight. Flagging as defense-in-depth, NOT gating: a compromised paired token could still make an operator's own node mis-advertise its mirror URL until explicitly cleared/token revoked+re-set, which is a self-griefing/availability risk for that operator's own mirror rewards, not a path to controlling or exfiltrating from other nodes/peers, since verified-content-not-safe-content still gates every actual fetch.

• Also-items: (a) grepped the whole diff for added lines ending in a bare string-continuation backslash and for anomalous double-spaced string literals (the fmt-collapse trap) -- none found; the new operator-facing CLI/help text reads cleanly. (b) The feat(peer)!: breaking marker: version only bumped PATCH (0.254.83->0.254.84), which is inconsistent with CLAUDE.md's stated SemVer-major-for-breaking convention if taken literally -- but none of the four changes actually break a wire/API contract (cross-family STUN rejection is a runtime behavior tightening/fix, stun. preference is additive-if-adopted, the new control method + mirror_advertise field are purely additive). Judged a labeling nit (arguably should have been fix(peer): or a plain feat(peer):), not a security defect -- flagging for the correctness gate, not gating security on it.

Next: composing final verdict.

[MichaelTaylor3d]

SECURITY VERDICT: PASS

Head audited: e966e353353817656eb517c416253bfea1c3c904 (resolved via gh pr view 569 --json headRefOid, matches dispatch).

requires_restart -- explicit answer for the dig-app lane

requires_restart is true, unconditionally, and it is the HONEST answer. Verified by reading the code path, not assumed: server.rs:2768-2776 reads the operator's mirror-advertise override exactly ONCE, before the mirror-lifecycle's loop {}, for the life of that task. A control.config.setMirrorAdvertiseUrls call persists to config.json but the running process cannot observe it -- only the next process start re-reads it (advertise.rs:159 -> advertised_urls_effective_from -> read_mirror_advertise_urls_override_from). dig-app should render this as "saved, restart required" and must not claim it took effect live.

Per-item findings

1. Cross-family STUN rejection -- CLEAR. net.rs discards only when dig_ip::Family::of(&result) != dig_ip::Family::of(&server). Both directions covered by genuine revert-proof tests (a_cross_family_stun_answer_is_discarded_and_the_ladder_falls_through and its IPv6-bound reverse), each using an honest same-family control tier to prove the rejection -- not just "returns None". A legitimate same-family answer (incl. the §5.2-preferred IPv6->IPv6 case) is untouched by the predicate. This is tier-selection only, upstream of the existing (unmodified) reflexive_candidate/routability path.
2. No port-equality check -- CLEAR. Read the entire StunPlan::discover_reflexive body; no such check exists anywhere in the diff.
3. No host/cloud-range special-casing -- CLEAR. stun.<relay-host> is derived from parse_relay_host(relay_endpoint) -- the operator's own configured DIG_RELAY_URL -- never a literal stun.relay.dig.net. Doc comment (net.rs L34-42) states the GPL-2.0/self-host rationale explicitly.
4. Shared derivation -- CLEAR. stun_server_from_relay (single-endpoint, traversal-ladder + DHT transport) now delegates to stun_servers_from_relay (the per-family list reflexive discovery uses) instead of re-deriving its own host, exactly as the brief required -- the two consumers cannot diverge on which host to dial.
5. Well-formedness only, no routability check -- CLEAR, confirmed at the source in dig-node-control-interface v0.33.0 tag (src/params.rs:95-118): is_well_formed_advertise_url checks scheme+host only, with an explicit doc citing dig-node#562 and stating it is deliberately not a routability check. dig-node's own consumer (control.rs:231-281) adds nothing beyond .validated().
6. Some(vec![]) refused / None clears / Some(non-empty) sets -- CLEAR, verified end-to-end from the wire contract (params.rs validated()) through persistence (set_mirror_advertise_urls_override) through the CLI (entrypoint.rs/control_cli.rs, where an empty CLI arg list maps to None, making the ambiguous case structurally unreachable from that surface).
7. requires_restart honesty -- CLEAR, see above.

Also-items

• fmt string-continuation corruption -- checked: no added line ends in a bare string-continuation backslash, and no anomalous double-spaced string literals in any new operator-facing text. Not present in this diff.
• feat(peer)!: breaking marker -- version bumped PATCH only (0.254.83 -> 0.254.84), not major/minor, which is inconsistent with the marker if read literally. None of the four changes actually break a wire/API contract (item 1 is a runtime behavior tightening, item 3 is additive-if-adopted DNS preference, item 4 is a purely additive control method + field). This is a commit-labeling nit for the correctness gate, not a security defect -- NOT gating on it.

One non-gating defense-in-depth note

control.config.setMirrorAdvertiseUrls requires only the ORDINARY/paired control token (requires_master_token() == false, dig-node-control-interface v0.33.0 method.rs:449-458), and its persisted override survives pairing.revoke. This is a deliberate, documented tradeoff in the contract crate (precedented by cache.setCap/log.setLevel, cross-referenced to that repo's own open #40 discussion of setUpstream's identical shape) -- not an oversight. A compromised paired token could make an operator's own node mis-advertise its mirror URL until cleared, which is a self-griefing/availability risk to that operator's own mirror rewards, not a path to controlling or exfiltrating from other nodes -- verified-content-not-safe-content still gates every actual fetch. Recommend a follow-up ticket to revisit alongside #40 if setUpstream is ever promoted to master tier; not a gate on this PR.

What I verified by reading vs running

Everything above was verified by reading the diff, the full surrounding source at the PR head, and the referenced dig-node-control-interface v0.33.0 tag directly from its own repo (not memory). I did not build or run tests myself; I relied on the PR's own green "Test + coverage" CI job (confirmed via gh pr view --json statusCheckRollup) to confirm the new tests actually execute and pass, having independently confirmed those tests are genuine revert-proofs (not vacuous) by reading their assertions.

No LIVE vulnerability found. PASS.

[MichaelTaylor3d]

I removed the ! breaking marker from this PR's title before merging, and that was wrong. Recording it because the commit is now on main and the signal is gone.

What happened

The security gate reported the ! as a labelling nit — "none of the four changes actually break a wire/API contract" — and I retitled feat(peer)!: → feat(peer): at merge time, reasoning that a breaking marker beside a patch bump would make git-cliff emit a false breaking-change entry.

The lane's own reasoning, which I did not have in front of me, was better than mine. It re-verified the marker against the SPEC diff and kept it deliberately, on two real changed defaults:

1. Cross-family STUN answers that were previously silently accepted are now discarded — this can flip a node from advertising an address to NoPublicAddress.
2. STUN source selection goes from one fixed tier to a 3-tier precedence plan, which can change which address a node advertises.

CLAUDE.md §2.4 defines major as "removed/renamed API, a wire/format/schema break, or a changed default". Both of those are changed defaults. The lane applied the contract correctly; the gate applied a narrower test (wire/API only) and I acted on the narrower one.

Why the version number could not have carried it anyway — and why that makes the marker MORE important, not less

dig-node cannot express a breaking change in its version. Per #522, scripts/package-version.sh caps MSI ProductVersion at MAJOR ≤ 255, MINOR ≤ 255, PATCH ≤ 65535, and the repo had run out of minor slots at 0.255.x. The resolution was to move the ever-incrementing counter into the patch field — which is exactly why this PR is 0.254.84 and not 0.255.0.

So under the current scheme every release is a patch bump, and the version number is structurally incapable of signalling a breaking change. That makes the Conventional-Commit ! the only remaining signal — and I removed the only signal on the grounds that it disagreed with a version number that is no longer allowed to agree with it.

Correcting my own stale note while I am here: I have been repeating that "0.255.x is a one-way door" as a live constraint. It was true before #522 and is not the operative concern now — the counter moved to patch, which has 65535 slots. The live constraint is the opposite one: do not bump the minor, because that consumes the last slot and re-breaks packaging.

What I am NOT doing

Not rewriting main. The commit is merged and propagated; a history rewrite to restore a marker would cost more than the marker is worth (git-history-safety).

What must happen instead

The stable release notes for 0.254.84 must call out the behaviour change explicitly, since the changelog will not. Specifically: a node that was previously advertising an address obtained from a cross-family STUN answer may now report no public address — which is correct, because that address was not the node's, but it is a visible change an operator could otherwise mistake for a regression.

This is recorded on the epic's release step too, so it is not lost between here and the release.