Skip to content

fix(security): correct the failure direction on five peer/network-facing input surfaces - #453

Draft
MichaelTaylor3d wants to merge 8 commits into
mainfrom
loop/batch-secpeer
Draft

fix(security): correct the failure direction on five peer/network-facing input surfaces#453
MichaelTaylor3d wants to merge 8 commits into
mainfrom
loop/batch-secpeer

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Five peer/network-facing security defects, all of the same shape — a stranger's input decides
something
— and every fix is a correction of the failure direction.

Failure direction, before and after

ticket surface before after
#349 dig.getPeers (peer wire) serves {"host":"::","port":0} to REMOTE peers as a dial candidate address withheld, peer kept (addresses: []); the check moved into a type
#285 relay endpoint parse any scheme accepted; unparsable port -> .unwrap_or(443), so it silently dials 443 at a host that may not be the one written fails closed — no dial when the endpoint cannot be read
#282 DIG_PEER_NETWORK matched three exact bytes, so OFF left the peer network RUNNING fails closed, one shared off-token across all three isolation knobs
#352 DIG_RELAY_URL= empty resolved to the compiled-in public relay fails closed — empty means no relay; bootstrap now logs which branch it took
#348 coin reservation an unexplained denial or a post-transmit transport failure reserved nothing, freeing coins of a bundle that may be in flight holds to the TTL unless the mempool stated its rejection

Blast radius checked

.gitnexus is stale for this repo (impact returns a false-safe impactedCount: 0), so the radius
was established by grep + direct read, and that is stated rather than implied.

  • parse_relay_host / relay_port / relay_socket_addr — all call sites enumerated: two in
    peer.rs (stun_servers_from_relay, relay_socket_addr at the NAT bring-up), the rest internal
    to net.rs. relay_port was private and is deleted.
  • is_usable_contact — five emitters found; the fifth (pool_peers) was the unguarded one. The
    other four keep working and now have a type to reach for.
  • push_signed_bundle — the reservation seam; PushOutcome gained no field.
  • DEFAULT_RELAY_URL is asserted to still parse under the stricter parser, so a fail-closed parse
    cannot take the relay tier down on a stock node.

No HIGH/CRITICAL-risk symbol was edited blind, but note two deliberate behaviour changes a
reviewer should weigh: parse_relay_host (pub) now refuses a scheme-less endpoint, and
DIG_RELAY_URL= now disables the relay. Both narrow what is accepted, and both are why this is a
minor bump.

Already-shipped check (§2.0) — it changed two of the five

Neither was rebuilt.

Tests — what each catches

test catches
a_relay_endpoint_that_cannot_be_read_yields_no_destination any relaxation toward the old parser; each malformed input is paired with the well-formed endpoint it is one character from, so the assertion is that the two are told apart
relay_port_comes_from_the_endpoint_or_its_scheme_never_a_post_failure_default reintroduction of unwrap_or(443). The ws:// rows are load-bearing: with wss:// only, "the scheme's default" and "hardcoded 443" are indistinguishable
get_peers_withholds_a_non_destination_address_but_keeps_the_peer the shipped defect and both over-corrections (dropping the row, blanking every row) — the fixture carries a truthful control peer beside each wildcard
a_wire_address_can_only_be_rendered_from_a_checked_contact a sixth emitter re-deriving {host, port}; loopback is asserted usable, pinning the guard against being tightened
the_three_isolation_knobs_share_one_off_vocabulary the divergence as it shipped. offline / no-thanks pin the boundary from the other side against a starts_with implementation
a_bundle_denied_without_a_reason_is_held_rather_than_freed the under-claim fail-open. Differs from its sibling control in exactly one field and demands the opposite outcome
a_transport_failure_holds_the_inputs_and_still_reports_the_failure the other half — and asserts the error still propagates, so a fix that swallowed the failure to reach the reserve call would fail

Revert-proof: each test was shown to fail when, and only when, its own fix was reverted
(3 named failures in dig-node-core, 2 in dig-wallet, with both pre-existing controls still
passing). Full output in the thread. Counts read from the test result: line, never an exit status.

SemVer — minor, 0.189.0 -> 0.190.0

Also dig-node-core 0.64.0 -> 0.65.0 and dig-wallet 0.43.0 -> 0.44.0 (both touched). Minor, not
patch
, because two behaviours narrow in ways a consumer can observe: parse_relay_host is pub
and now refuses input it used to accept, and DIG_RELAY_URL= changes meaning. Not major: this is a
0.x line and nothing was removed from the public surface except the private relay_port.

Cargo.lock carries the three version lines and no dependency churn.

Residual — stated, not hidden

#285's centralization is NOT done, and cannot be done from this repo. The ticket's designated
survivor dig_nat::relay::parse_relay_endpoint is private in the published dig-nat 0.21.0, so
dig-node cannot call it. The security half — the failure direction — is fixed here; the rival pair
still exists. The parser is transcribed rule-for-rule from dig-nat's own implementation, with its own
test vectors, so adopting the export is a deletion rather than a re-derivation. Making it pub is
an additive dig-nat release and needs a lane in that repo.

#348 does not make the accepted flag trustworthy. A hostile source can fabricate a rejection
string, and nothing here can verify one without an independent chain read. What the fix does is put
the cheapest lie and the accidental case on the safe side. Said plainly in the doc comment rather
than claimed as a closure.

Closes #349
Closes #352
Closes #348
Closes #285
Closes #282

Batch: #349 #352 #348 #285 #282 -- five defects where a stranger's input
decides something, each fixed by correcting the failure direction.

Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d and others added 2 commits August 31, 2026 06:55
…are one off-token

#285: relay endpoint parsing accepted any scheme and turned an unparsable port
into None, which relay_socket_addr resolved with .unwrap_or(443) -- so one
malformed string made the node silently dial 443 at a host that may not be the
one the operator wrote. Replaced with a fail-closed parse transcribed from
dig_nat::relay::parse_relay_endpoint (the designated survivor, still private in
the published 0.21.0), so adopting the export later is a deletion.

#282/#352: the three network-reaching isolation knobs each carried a private
off-token predicate reading a different vocabulary, so DIG_PEER_NETWORK=OFF left
the peer network running while DIG_RELAY_URL=OFF disabled the relay. One shared
is_off_token now serves all three, and DIG_RELAY_URL= means no relay rather than
the compiled-in public one. Bootstrap resolution announces which branch it took.

Co-Authored-By: Claude <noreply@anthropic.com>
…ssibly-in-flight bundle

#349: dig.getPeers served the wildcard address to REMOTE peers as a dial
candidate, because is_usable_contact had been adopted call site by call site and
pool_peers was the fifth site nobody looked at. The check now lives in a
ContactAddr type whose construction IS the question and whose address_json is the
only renderer of the wire entry. The ADDRESS is withheld, never the peer: the row
survives with an empty addresses array, the shape both consumers already handle.

#348: reservation was gated on an untrusted 'accepted' alone, so an unexplained
denial or a post-transmit transport failure freed coins of a bundle that may be in
flight. Anything short of a STATED mempool rejection now holds to the TTL.

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Progress — branch loop/batch-secpeer @ 918909e

#285 relay endpoint parse — DONE, green (42 passed).
Before: any scheme accepted; unparsable port -> None -> .unwrap_or(443), so a malformed
endpoint silently dialled 443 at a host that may not be the one written (userinfo and #frag
survived the parse). After: fails closed — no scheme, an unknown scheme, an unparsable port, or
a malformed IPv6 authority all yield no destination and no dial.
Residual: the ticket's survivor dig_nat::relay::parse_relay_endpoint is private in the
published dig-nat 0.21.0
, so dig-node cannot call it. Transcribed rule-for-rule with dig-nat's own
test vectors, so adopting the export later is a deletion. Centralization needs a dig-nat release —
outside this worktree.

#282 backfill-on-miss — ALREADY SHIPPED on main; the third switch was real.
resolve_backfill_on_miss already fails closed and its doc cites #282 by number. The ticket's
"check whether a third exists" found one: is_peer_network_enabled matched three exact byte
strings, so DIG_PEER_NETWORK=OFF left the peer network RUNNING while the identically-spelled
DIG_RELAY_URL=OFF disabled the relay. Three private off-token predicates, three vocabularies.
After: one shared is_off_token, trimmed and case-insensitive, used by all three knobs.

#352 DIG_BOOTSTRAP_PEERS — mostly shipped in #312; two real gaps closed.
The set-but-empty fix and the Windows platform note are already on main. Gaps: (a) the sibling
knob DIG_RELAY_URL= resolved to the compiled-in public relay — same defect one knob over —
now means no relay; (b) bootstrap resolution was silent about which of its three branches it took,
so a fleet that had dialled production looked identical to one that had not. Now info-logged.

#349 dig.getPeers wildcard — DONE, green (5 passed).
Before: a relay-reached peer was served to remote peers as {"host":"::","port":0} — a
non-destination advertised as a dial candidate. After: the check moved into a ContactAddr type
whose construction is the question and whose address_json is the only renderer, so a sixth
emitter has nothing to reach for. Address withheld, peer kept (addresses: []).
Consumer check the ticket asked for, done: parse_forwarded_providers already drops an
addressless provider by design and by test, and dig.announce validates only is_array — so an
empty array is a shape both consumers already handle, while omitting the field would break the
first and fail the second.

#348 reservation — implemented, tests building.
Before: gated on an untrusted accepted alone. An unexplained denial, or a transport failure after
transmit, reserved nothing — coins returned to the selectable set while a bundle carrying them
was in flight. After: anything short of a stated mempool rejection holds to the TTL. TTL
deliberately unchanged (shortening it trades a double-select for the worse lockout failure).
Stated honestly in the doc comment: a hostile source can still fabricate a rejection string; this
makes the cheapest lie and the accidental case land on the safe side, it does not verify the flag.

…t reservation rule

Both behaviours are now normative rather than incidental: a peer row with no
dialable destination keeps the row and empties the addresses ARRAY (#349), and a
push reserves its inputs unless the mempool DEFINITIVELY refused it (#348).

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Revert-proof — each test fails when, and only when, its own fix is reverted

Work was committed first, so the reverts were applied to the working tree and undone with
git checkout -- <path> (never git stash, which is repo-global and crosses sibling worktrees).

Three fixes reverted together in dig-node-core, each a distinct code path, so attribution is 1:1:

revert test result
unknown scheme defaults to 443 again (#285) a_relay_endpoint_that_cannot_be_read_yields_no_destination FAILED
pool_peer_row emits the address unconditionally (#349) get_peers_withholds_a_non_destination_address_but_keeps_the_peer FAILED
is_peer_network_enabled back to exact-byte matching (#282) the_three_isolation_knobs_share_one_off_vocabulary FAILED
running 4 tests
test result: FAILED. 1 passed; 3 failed; 0 ignored; 0 measured; 1022 filtered out

parse_relay_host_strips_scheme_port_and_path correctly passed under that revert — the revert
defaulted an unknown scheme rather than removing the scheme requirement, so the scheme-less row it
asserts was unaffected. It is reported rather than hidden: it shows the reverts were narrow, and it
is exactly why the discriminating test exists alongside it.

Counts read from the test result: line, not from an exit status, and every run matched a non-zero
number of tests.

Green runs (fixes in place):

  • dig-node-core --lib relay42 passed; 0 failed
  • dig-node-core --lib (the five new/changed tests) — 5 passed; 0 failed
  • dig-wallet --lib (all four reservation tests) — 4 passed; 0 failed

…rvably

Root 0.189.0 -> 0.190.0, dig-node-core 0.64.0 -> 0.65.0, dig-wallet 0.43.0 ->
0.44.0 (both touched). Minor rather than patch because two behaviours narrow in
ways a consumer can observe: parse_relay_host is pub and now refuses a
scheme-less endpoint it used to accept, and DIG_RELAY_URL= changes meaning from
'the compiled-in public relay' to 'no relay'. Not major -- a 0.x line, and
nothing left the public surface but the private relay_port.

Cargo.lock carries the three version lines and no dependency churn.

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Final state — 09095c9, DRAFT

cargo check -p dig-node-core -p dig-wallet --libRC=0, no errors, no unused warnings.
Version bump + Cargo.lock verified: the lock diff is exactly three version lines and no
dependency churn
.

One thing I could NOT verify, stated rather than glossed: the full dig-node-core --lib run
reached 1009 passed / 0 failed and then stalled on
tests::serve_content_plaintext_reaches_pool_holder_at_peer_rpc_port, with no advancement across
several minutes (... ok count flat at 1009). That test binds a real peer-RPC port and six lanes
share this machine, so port contention is the likely cause; nothing in this diff touches the serve
path. I stopped the run rather than hold the shared build lock further. Treat the full-suite green
as UNCONFIRMED locally
— CI runs it on a clean box, and that is the gate.

What IS confirmed locally, all with counts read from the test result: line:

  • dig-node-core --lib relay — 42 passed / 0 failed
  • dig-node-core --lib (the five new/changed tests) — 5 passed / 0 failed
  • dig-wallet --lib (all four reservation tests) — 4 passed / 0 failed
  • revert-proof: 3 named failures in dig-node-core, 2 in dig-wallet, both pre-existing controls
    still passing

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Correction to the comment above: the suite was NOT stalled

I reported the full dig-node-core --lib run as stalled at 1009. That was wrong, and the error was
mine.
It has since advanced to 1020 passed / 0 failed and is still going.

What I got wrong, because it is the reusable part: I inferred "stalled" from a flat ... ok
count across two samples ~45s apart. The tail of that suite is the serve_content_plaintext_*
group, which binds real ports and runs sequentially with real timeouts — several minutes per
test
is its normal speed. So a flat count over one short window is exactly what a slow test and a
hung test both look like, and I read the ambiguous signal as the worse one.

That is the same shape as the traps this repo already documents: elapsed time measured from the
observer describes the observer.
Two samples of a counter are not a rate.

Standing state: 1020 passed, 0 failed, still running. No test has failed at any point. I will
post the terminal test result: line when it lands rather than leave either claim standing.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Full suite: GREEN. The earlier "stalled" report is retracted.

test result: ok. 1026 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 768.16s
RC=0

cargo test -p dig-node-core --libthe whole crate, 0 filtered out, so no assertion was
skipped by a filter. It took 12m48s; it was never hung, only slow, and the earlier report of a
stall was my misreading of a flat counter over a short window.

Local verification is now complete:

run result
dig-node-core --lib (full, unfiltered) 1026 passed / 0 failed
dig-wallet --lib (four reservation tests) 4 passed / 0 failed
cargo check -p dig-node-core -p dig-wallet --lib RC=0, no errors, no unused warnings
revert-proof 3 named failures in dig-node-core, 2 in dig-wallet; both pre-existing controls passing

Notably the full run confirms the two deliberate behaviour changes — parse_relay_host refusing a
scheme-less endpoint, and DIG_RELAY_URL= meaning "no relay" — broke no other test in the crate,
which the targeted runs could not have shown.

dig-wallet --lib was exercised only under a filter; CI covers it unfiltered.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — IN PROGRESS, not the verdict

Auditing head 09095c95acc6e083f2664149ce656df5e7822d66 (resolved from gh pr view --json headRefOid), base 3e480dd. Own worktree at /c/tmp/worktrees/secgate-453; no shared checkout touched.

Interim 1 — the off-token vocabulary sweep (brief's priority check 1)

Shared: confirmed. is_off_token is one pub(crate) fn at crates/dig-node-core/src/peer.rs:495, reached by all three knobs — is_relay_enabled (peer.rs:457), is_peer_network_enabled (peer.rs:473), bootstrap.rs:135 is_disabled. Not three copies. Trimmed, eq_ignore_ascii_case, equality not starts_with — and the discriminating negatives the lane claimed do exist and would fail a prefix implementation: "offline" and "no-thanks" are asserted !is_off_token at peer.rs:4502.

But the claim is broader than the fix. peer.rs:481 says the token is "shared by every network-reaching DIG_* knob." It is not. Four other default-ON, disable-token-driven knobs still read a different vocabulary — none accepts disabled, none accepts empty:

knob file:line OFF set disabled ignored?
DIG_WALLET_ENABLE_CHAIN_SYNC crates/dig-node-service/src/config.rs:419 0 false no off yes
DIG_NODE_DIGLOCAL crates/dig-node-service/src/config.rs:362 0 false no off yes
DIG_HOLDINGS_INGEST crates/dig-node-core/src/seams/dig_peer/holdings.rs:1069 0 false off no yes
DIG_NODE_STORE_MELT crates/dig-node-core/src/seams/dig_peer/store_melted.rs:551 off 0 false no yes

The remaining switches are all truthy-only opt-ins (DIG_NODE_ALLOW_REMOTE config.rs:445, DIG_WALLET_ENABLE_LIVE_BROADCAST config.rs:376, DIG_NODE_ADVERTISE_LOOPBACK net.rs:280, DIG_NODE_INBOUND_DEMAND_CACHE download.rs:281, DIG_NODE_BACKFILL_ON_MISS download.rs:242, DIG_NODE_FORWARD_ON_MISS via dig_sex::discovery::parse_enabled) — those fail toward not reaching the network and are not in this class.

And the lane's second claim does not survive its own code. The brief says parse_chain_sync_flag was left alone because it is a "default-ON knob for non-network-reaching behaviour." config.rs:150-155, the field doc for enable_chain_sync, says the opposite in its own words:

"a supervisor dials the network: it probes 127.0.0.1:8444 and then the Chia DNS introducers. A test harness must be able to build state without a shared CI runner making unrequested outbound connections"

So DIG_WALLET_ENABLE_CHAIN_SYNC=disabled leaves a node dialling the Chia DNS introducers while the identically-spelled DIG_RELAY_URL=disabled isolates the relay — the exact shape of #282, in a knob this PR declares covered. DIG_HOLDINGS_INGEST is the sharper operational case: it exists specifically so an operator facing an announcement flood can shed inbound work without downgrading (holdings.rs:400), and =disabled silently does nothing during that flood.

Ranking and gate/no-gate deferred to the verdict. Continuing on: #285 transcription faithfulness against published dig-nat 0.21.0, #349's other peer-address render paths, #348 reachability of the wider hold.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — IN PROGRESS, not the verdict (2 of 3)

Head 09095c95acc6e083f2664149ce656df5e7822d66.

Interim 2 — #348: the guard is unreachable from the only production pusher, and SPEC.md states a property the code does not have

This is my highest-severity finding and I want it durable before I finish.

is_definitive_rejection (crates/dig-wallet/src/sage/rpc.rs:2151) frees the inputs when !accepted && rejection.is_some(), and holds when rejection is None. The only production SignedBundlePusher cannot ever emit rejection: None on a refusal. ChainTransport::push (crates/dig-wallet/src/sage/chain.rs:512-532):

Ok(if status.inclusion.is_admitted() {
    PushOutcome { accepted: true,  transaction_id: Some(..), rejection: None }
} else {
    PushOutcome { accepted: false, transaction_id: None,
        rejection: Some(match status.error.as_deref().map(str::trim) {
            Some(reason) if !reason.is_empty() => format!("{}: {reason}", status.status),
            _ => status.status.clone(),          // <-- ALWAYS Some
        }) }
})

The _ arm manufactures the reason locally from the status label when the node sent none, so rejection is Some(..) on every non-admitted answer. ChainTransport is the only impl (chain.rs:537; the trait doc at chain.rs:164 says so itself) and the only production wiring is crates/dig-wallet/src/sage/service.rs:290 .with_pusher(chain.clone()). Every other with_pusher in the tree is a FakePusher in #[cfg(test)].

Consequence — the attack the fix names is still fully open. Walk it:

  1. A hostile / faulty chia peer receives the bundle and does relay it.
  2. It answers with a non-admitted status (FAILED, PENDING, anything not is_admitted()).
  3. ChainTransport::push fills rejection: Some("PENDING") from the label.
  4. is_definitive_rejectiontrue → inputs freed.
  5. A second send inside the confirmation window reselects the same inputs. That is exactly the double-select window sec(reservation): gated on an untrusted 'accepted' — the under-claim direction fails OPEN into the double-select window #348 exists to close.

The doc comment at rpc.rs:2126-2148 says "a source that wants a coin reselected only has to say 'not accepted'" and offers the guard as the answer to it. On the shipped path the source still only has to say "not accepted" — dig-node writes the reason on its behalf. The doc is honest that a hostile source can fabricate a rejection string; what it does not say is that fabrication is not required, because no reachable code path produces the None the guard keys on.

And SPEC.md:5356-5366 states this normatively, born false:

"a source that denies a relay it performed MUST NOT thereby return the coins to selection"

A source that denies a relay it performed does return the coins to selection, today, through the only pusher that exists. A normative MUST that the implementation in the same commit does not satisfy is worse than no clause.

What the fix DOES change on the production path is one thing only: push() returning Err now reserves the inputs before propagating PushError::Unreachable. That half is real, live, and correct — the transport-failure ambiguity is genuine and holding is the right side of it. I am not disputing it. My objection is that the headline half — the untrusted-source denial, the half both the doc and the SPEC lead with — is guarded by a branch nothing in production can take.

Severity: MEDIUM as a vulnerability (the double-select window is unchanged from before this PR, so nothing regresses) and HIGH as a claim defect — SPEC.md now asserts a property the node does not have, and the next reader will believe #348 closed a window that is open. Under the brief's own rule, a fix that overstates its own guarantee is a finding.

Two ways out, either acceptable to me: keep the guard and correct both the doc and the SPEC clause to say the untrusted-source half is not closed and why (cheapest); or key "definitive" on the mempool's own status.error presence rather than on the locally-manufactured label, so a bare status with no stated reason is genuinely held.

Interim 2b — #349: a second renderer of the same wire shape does not go through ContactAddr

ContactAddr (net.rs:129) and address_json (net.rs:145) do what the ticket asked for dig.getPeers: the wildcard cannot be minted, so it cannot be rendered. That part I confirm.

But net.rs:117 claims "ContactAddr::address_json is the only way to render the shipped {host, port, kind} entry" and "no second address encoding enters the ecosystem." There is a second one. provider_json (crates/dig-node-core/src/download.rs:3019) serialises p.addresses straight into the providers array of the dig.getAvailability miss answer and the -32008 redirect — both peer-facing — with no destination check. Those CandidateAddrs are built from remote peer input by parse_candidate_addr (crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs:521), which validates only that host is a string and port fits a u16. 0 is a valid u16 and "::" is a valid string.

Attacker path: a peer answering this node's forwarded ask returns {"peer_id": <64-hex>, "addresses":[{"host":"::","port":0}]}. It survives parse_candidate_addr, survives the addresses.is_empty() drop at forwarded_ask.rs:507 because the array is non-empty, becomes a ProviderRecord, and is then re-served to the next asking node through provider_json. So the wildcard #349 removed from dig.getPeers can still be injected deliberately by a stranger and relayed onward — worse than the accidental relay-circuit case that motivated the ticket, since it costs the attacker one JSON object.

Not introduced by this PR — pre-existing — so I do not gate on the guard. I do gate on the sentence: net.rs:117's "only way" claim is false as written and will stop the next reader looking here.

Continuing: #285 transcription divergences (two found, both minor), then the verdict.

Pure reflow of lines the type-boundary change made over-width. No behaviour
change; `git diff -w` against the prior head is empty.

Refs #349
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security: CHANGES-REQUIRED

Head audited: 09095c95acc6e083f2664149ce656df5e7822d66 (resolved from gh pr view 453 --json headRefOid, not from the dispatch brief). Base 3e480dd302ae0a8518ce535e5b1bc4a563b960ff. Own detached worktree /c/tmp/worktrees/secgate-453; no shared checkout and no sibling worktree touched, no mutating git command outside my own tree.

I gate on three claims the diff makes that its own code does not support — one of them a normative MUST NOT in the money path. Every remedy is text-only and cheap. I am NOT gating on any underlying guard and I am not asking for new behaviour. The five behaviour changes are, on balance, an improvement and I say so below.


GATING

G1 — HIGH (claim) / MEDIUM (residual vuln). #348's headline guard is unreachable from the only production pusher, and SPEC.md states a MUST NOT the code permits

is_definitive_rejection (crates/dig-wallet/src/sage/rpc.rs:2151) frees inputs on !accepted && rejection.is_some(); it holds only when rejection is None. No production path can produce that None. ChainTransport::push (crates/dig-wallet/src/sage/chain.rs:512-532) manufactures the reason locally when the node sent none — the _ arm falls back to status.status.clone(), so rejection is Some(..) on every non-admitted answer.

ChainTransport is the sole impl (chain.rs:537; the trait doc at chain.rs:164 says so itself) and crates/dig-wallet/src/sage/service.rs:290 .with_pusher(chain.clone()) is the sole production wiring. Every other with_pusher in the tree is a #[cfg(test)] FakePusher.

Exploit, unchanged by this PR: a hostile or faulty chia peer relays the bundle, then answers with any non-admitted status (PENDING, FAILED). ChainTransport fills rejection: Some("PENDING"). is_definitive_rejection returns true, the inputs are freed, and a second send inside the confirmation window reselects them. That is precisely the window #348 exists to close, and the guard does not touch it.

The doc at rpc.rs:2131 says "A source that wants a coin reselected only has to say 'not accepted'." On the shipped path that sentence is still true after the fix, because dig-node writes the reason on the lying source's behalf. The doc is honest that a rejection string can be fabricated; it does not say fabrication is not required.

SPEC.md:5356-5366 is worse, because it is normative and internally inconsistent. It defines definitive as "the mempool stated its reason" and then asserts "a source that denies a relay it performed MUST NOT thereby return the coins to selection". Given ChainTransport always states a reason, those two sentences cannot both hold, and the code implements the weaker one. A future implementer building to this SPEC builds something else; a future gate reads the clause as satisfied. A born-false MUST NOT in the reservation rules is worse than no clause.

Required — either discharges the gate:

  • (a) correct rpc.rs:2119-2148 and SPEC.md:5356-5366 to state that the untrusted-source half is not closed, and why (the only pusher synthesises the reason); or
  • (b) key "definitive" on the mempool's own status.error being present, so a bare status label is held.

I have no preference. (a) is a text change.

G2 — MEDIUM. peer.rs:478 claims the off-token is "shared by every network-reaching DIG_* knob". Four are not

The unification itself is real and I verified it (see PASSED). The sentence is not. Four default-ON, disable-token-driven knobs read a different vocabulary — none accepts disabled, none accepts empty:

knob file:line OFF set
DIG_WALLET_ENABLE_CHAIN_SYNC crates/dig-node-service/src/config.rs:419 0 false no off
DIG_NODE_DIGLOCAL crates/dig-node-service/src/config.rs:362 0 false no off
DIG_HOLDINGS_INGEST crates/dig-node-core/src/seams/dig_peer/holdings.rs:1069 0 false off no
DIG_NODE_STORE_MELT crates/dig-node-core/src/seams/dig_peer/store_melted.rs:551 off 0 false no

Everything else in the repo is a truthy-only opt-in that fails toward not reaching the network — DIG_NODE_ALLOW_REMOTE (config.rs:445), DIG_WALLET_ENABLE_LIVE_BROADCAST (config.rs:376), DIG_NODE_ADVERTISE_LOOPBACK (net.rs:280), DIG_NODE_INBOUND_DEMAND_CACHE (download.rs:281), DIG_NODE_BACKFILL_ON_MISS (download.rs:242), DIG_NODE_FORWARD_ON_MISS (via dig_sex::discovery::parse_enabled). Those are not in this class.

On the carve-out and #459: I accept parse_dig_local_flag (a local listener). I do not accept the stated reason for parse_chain_sync_flag. The brief calls both "non-network-reaching"; config.rs:150-155, the field's own doc, says the opposite in its own words — "a supervisor dials the network: it probes 127.0.0.1:8444 and then the Chia DNS introducers." So DIG_WALLET_ENABLE_CHAIN_SYNC=disabled leaves a node dialling public introducers while the identically-spelled DIG_RELAY_URL=disabled isolates. DIG_HOLDINGS_INGEST is the sharper case: it exists specifically to let an operator shed inbound work under an announcement flood without downgrading (holdings.rs:400), and =disabled silently does nothing during exactly that flood.

Required in THIS PR: scope the peer.rs:478 sentence to the three knobs it actually covers. Not required here: fixing the knobs — that is #459's job. #459 should be widened to name DIG_HOLDINGS_INGEST and DIG_NODE_STORE_MELT, and to drop the "non-network-reaching" rationale for chain sync.

G3 — MEDIUM. net.rs:122 claims address_json is "the only way to render the shipped {host, port, kind} entry". There is a second renderer, and it carries attacker input to remote callers

ContactAddr does what #349 asked for on dig.getPeers — confirmed below. But provider_json (crates/dig-node-core/src/download.rs:3019) serialises p.addresses straight into the providers array of the dig.getAvailability miss answer and the -32008 redirect — both peer-facing — with no destination check. Those CandidateAddr values come from remote peer input via parse_candidate_addr (crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs:521), which validates only that host is a string and port fits a u16. 0 is a valid u16; "::" is a valid string.

Exploit (pre-existing, not introduced here): a peer answering this node's forwarded ask returns a provider whose one address is {"host":"::","port":0}. It passes parse_candidate_addr, survives the addresses.is_empty() drop at forwarded_ask.rs:507 because the array is non-empty, becomes a ProviderRecord, and is re-served to the next asking node by provider_json. The wildcard #349 removed from dig.getPeers can therefore still be injected on purpose by a stranger and relayed onward, for the cost of one JSON object — strictly worse than the accidental relay-circuit case that motivated the ticket, whose harm the diff itself states: "a peer that dials it wastes one of its few dial slots, and a peer that caches it caches a hole."

Required in THIS PR: correct the net.rs:117-122 claim. Not required here: the guard on parse_candidate_addr / provider_json — file it; it is pre-existing and outside this diff.


NON-GATING

  • LOW — the new "empty means off" is silent on two of the three knobs. sec(config): on Windows DIG_BOOTSTRAP_PEERS= is DELETED not empty — an isolated node fell back to mainnet and peered with the production gateway #352's own remedy was to announce which branch was taken, "because the three branches are indistinguishable from the outside and one of them reaches the public network" (bootstrap.rs:45). That log went to bootstrap only. crates/dig-node-service/src/server.rs:2157 is if peer_network_enabled() { spawn_peer_network(..) } with no else and no log, so DIG_PEER_NETWORK= (empty — e.g. a template rendering an unset variable) now silently disables the entire peer network with zero output. Same for DIG_RELAY_URL=. The direction is fail-closed so it is not a hole; it is the diagnosability defect the PR names and then half-fixes.
  • LOW — the Relay endpoint parsing accepts any scheme and silently dials 443 on a malformed port #285 transcription diverges from dig-nat 0.21.0 in two places. Both harmless today, but together they falsify "adopting the export later is a deletion rather than a re-derivation" (net.rs:381):
    1. dig-node trims the whole endpoint (net.rs:391); dig-nat does not (relay.rs:1165). Neutralised in practice — resolve_relay_url (peer.rs:437) trims before any caller.
    2. wss://[2001:db8::1]junk — dig-node refuses it (net.rs:404, the None => return None arm); dig-nat accepts it at the default port (relay.rs:1187, an unconditional None => default_port). dig-node is stricter, so the direction is safe, but a future swap to dig-nat's export would silently loosen this case. One sentence naming the two deltas stops that being a surprise.
  • CI is RED and blocks merge regardless (§2.4a). Rustfmt fails — I reproduced it locally, cargo fmt --all -- --check RC=1, purely cosmetic wrapping at peer.rs:1363 (the pool_peer_row signature) and peer.rs:4483/4505 (the new test's arrays). Lint commit messages also fails. Test + coverage was still pending when I read it. Neither is a security defect; both are merge preconditions.

PASSED — what I checked and found clean

  • Off-token genuinely shared, and genuinely not a prefix. One pub(crate) fn is_off_token at peer.rs:498, reached by is_relay_enabled (peer.rs:458), is_peer_network_enabled (peer.rs:473) and bootstrap.rs:135. Not three copies. eq_ignore_ascii_case on a trimmed value — equality, so a starts_with implementation is excluded, and the discriminating negatives the lane claimed do exist: offline and no-thanks are asserted NOT off-tokens at peer.rs:4508.
  • The old test WAS vacuous, and the new one is not. peer_network_enabled_default_on_off_only_for_opt_out listed off/0/false and on; the old impl (!matches!(env, Some("off")|Some("0")|Some("false"))) and the new one agree on every one of those rows, so it passed under both and could never have caught Backfill-on-miss config fails OPEN on a malformed value while the sibling switch fails CLOSED #282. The replacement asserts OFF, off, Off, disabled, DISABLED, False, No, empty and whitespace against both knobs — the old peer-network impl returns enabled for all nine, so the test fails against it. Non-vacuous, and it pins the boundary from both sides.
  • Both directions of Backfill-on-miss config fails OPEN on a malformed value while the sibling switch fails CLOSED #282/sec(config): on Windows DIG_BOOTSTRAP_PEERS= is DELETED not empty — an isolated node fell back to mainnet and peered with the production gateway #352 hold. Unset leaves the defaults intact on all three (is_relay_enabled(None) and is_peer_network_enabled(None) asserted at peer.rs:4485-4486; bootstrap_targets_from_env's None arm takes the compiled-in anchors). No over-correction into "empty means isolated everywhere" — is_off_token is applied to those three knobs and nothing else. I checked specifically that an operator who sets nothing still gets a normal node.
  • sec(peers): dig.getPeers serves the wildcard address to REMOTE peers as a dial candidate — move is_usable_contact to the type boundary #349's type boundary is real for dig.getPeers. ContactAddr::new (net.rs:133) is the only constructor and gates on is_usable_contact; address_json (net.rs:145) is an inherent method, so the entry cannot be minted for a non-destination. The peer row survives with an empty addresses array, the shape parse_forwarded_providers already drops by design and dig.announce accepts. The test fixture carries a truthful control beside each wildcard, so "emit everything" and "emit nothing" both fail. Loopback is deliberately kept dialable (peer.rs:4455), correctly preventing a tightening regression on single-host runs. The other guarded emitter, connected_peers_json (peer.rs:339), is the local control.peerStatus surface and emits a plain addr.to_string(), not this shape — not a counterexample.
  • Relay endpoint parsing accepts any scheme and silently dials 443 on a malformed port #285 centralization is genuinely blocked, and the transcription is faithful in substance. parse_relay_endpoint is fn, not pub fn, at dig-nat-0.21.0/src/relay.rs:1164, and struct RelayEndpoint at :1156 is private too — read from the published crate in the cargo registry, not the workspace path; Cargo.lock pins dig-nat 0.21.0 from registry+crates.io. All four rules match: scheme required and ws/wss only (:1168), an unparsable port is an error (:1186/:1193), userinfo stripped (:1175), path+query+fragment dropped (:1174). The test vectors really are dig-nat's own assertions — the no-scheme and wrong-scheme and :notaport rows (relay.rs:1670-1672), the userinfo case (:1663), the ws-to-80 default (:1648), the bracketed IPv6 pair (:1653-1658).
  • Relay endpoint parsing accepts any scheme and silently dials 443 on a malformed port #285 fails closed without taking the relay tier down. parse_relay_host and relay_socket_addr both project through parse_relay_endpoint, and a test pins that DEFAULT_RELAY_URL (dig_constants::DIG_RELAY_URL, peer.rs:77) still parses — so the stricter parser cannot refuse the compiled-in relay. The one behaviour an existing operator loses is the scheme-less endpoint, and dig-nat already refused those (relay.rs:1670), so such a node was deriving a STUN server for a relay it could never connect to. Aligning is the right direction, not a regression.
  • sec(reservation): gated on an untrusted 'accepted' — the under-claim direction fails OPEN into the double-select window #348's transport-failure half is real, live and correct. push() returning Err previously propagated with ? before any reservation; it now reserves and still returns PushError::Unreachable, and the test asserts both at once, so a fix that swallowed the error to reach the reserve call would fail. The 10-minute RESERVATION_TTL_MS (rpc.rs:527) bounds the hold and self-heals, and the refusing-control test pins the guard from the other side so "reserve unconditionally" cannot pass. I considered whether the wider hold is a new denial primitive: a source that can silently deny can already deny service by not relaying at all, so the marginal denial is a bounded 10-minute coin hold on top of an already-failing send. Not a finding.
  • Secrets / custody / privilege. No key, token, credential or PAT introduced, logged or committed. The new tracing::info! in bootstrap.rs logs the DIG_BOOTSTRAP_PEERS value — peer ids and addresses, not a secret. Section 908 intact: nothing in the diff signs, and push_signed_bundle still refuses a node-custodied spend before reaching the pusher (rpc.rs:2095). No new endpoint, RPC method, permission, FFI, install root, service registration or file-permission change. No dependency added, removed or loosened — Cargo.lock's only delta is the three version bumps.
  • SemVer verified from the files. Workspace 0.189.0 to 0.190.0, dig-node-core 0.64.0 to 0.65.0, dig-wallet 0.43.0 to 0.44.0. Minor is right: parse_relay_host is pub (net.rs:428) and now refuses input it accepted; parse_relay_endpoint, RelayEndpoint and ContactAddr are new pub items; relay_port was private, so its removal is not a surface change.

What I could not reach

  • I did not compile or run the suite. Seven sibling lanes share this box, and the coordinator confirmed the lane re-ran at this head: cargo test -p dig-node-core --lib unfiltered, 1026 passed / 0 failed / 0 filtered out, RC=0, plus dig-wallet --lib 4/4. I judged a competing ten-minute build not worth the contention, and none of my findings depends on execution — G1 and G3 are each established by reading two code paths. The one thing I did run is cargo fmt --all -- --check (no compile), which reproduced the CI failure. CI's Test + coverage remains the authoritative gate and was still pending when I read it.
  • .gitnexus is stale, so impact would return a false-safe zero. Every call graph above is grep plus direct read, stated as such.
  • I did not audit dig-sex, dig-gossip or dig-nat themselves beyond reading dig-nat 0.21.0's parse_relay_endpoint and dig-sex 0.4.0's parse_enabled for the two comparisons above.

Re-gate scope: all three gating items are text-only. If the fix stays inside doc comments and SPEC.md, this leg alone re-runs — no need for the full triple.

…ld can fire

`is_definitive_rejection` (`sage/rpc.rs`) frees a bundle's inputs back into
selection only for a refusal the mempool STATED, and holds them to the TTL
otherwise -- because a peer that relayed the bundle and then answered with a bare
verdict may still have put it in flight.

`ChainTransport::push` manufactured a reason from `status.status`, so `rejection`
was `Some(..)` on EVERY non-admitted answer, the guard was true every time, and
the hold could never fire. The fix read as shipped and was vacuous; the
double-select window #348 exists to close was untouched.

SPEC.md §18.7 was already correct -- "a bare denial carrying no reason ... MUST be
treated as POSSIBLY IN FLIGHT" -- so this is the code being brought to the spec,
not a spec change.

The decision is extracted as `stated_rejection` and asserted directly, because an
end-to-end assertion on a held reservation passes for many reasons and only one of
them is this mapping being right. Revert-proved: restoring the manufactured reason
fails it on `left: Some("PENDING"), right: None`.

Also corrects two comments that claimed more than the code does: the off-token is
shared by the three ISOLATION knobs, not by every network-reaching `DIG_*` knob
(four others read a narrower vocabulary -- #459); and `ContactAddr` closes the
`dig.getPeers` emitter, not the class, since `provider_json` still relays
remote-supplied candidates unchecked.

Refs #348 #349 #459

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security scoped re-gate — IN PROGRESS, not the verdict

Head audited: 3cb731f6209645c183156b2db8204e2c92ef0fb3 (resolved myself via gh pr view 453 --json headRefOid).

Subject: content delta 09095c95..3cb731f. Note the branch was rebased09095c95 is NOT an
ancestor of 3cb731f (git merge-base = 3e480dd), so this is a tree comparison, not a commit
range. Three files: crates/dig-node-core/src/peer.rs, crates/dig-node-core/src/seams/dig_peer/net.rs,
crates/dig-wallet/src/sage/chain.rs. SPEC.md is not in the delta.

Read entirely from git objects (git show <sha>:<path>) — no shared checkout mutated.
.gitnexus is stale, so this is grep + direct read throughout.


G1 — the guard chain now fires. Verified in both directions.

1. Does the hold actually fire? Yes. Traced the whole chain, not just the mapping:

  • chain.rs:513 stated_rejection reads status.error, trims, drops empty -> None for a bare verdict.
  • chain.rs:542-559 is the ONLY non-admitted PushOutcome construction; rejection: Self::stated_rejection(&status).
  • rpc.rs:2151-2153 is_definitive_rejection = !accepted && rejection.is_some().
  • rpc.rs:2105-2113 reserves unless that guard holds.

So a bare PENDING -> error: None -> rejection: None -> guard false -> reservation fires, inputs held.
An Err (transport failure) fails the Ok(o) pattern -> also held. Both match SPEC 18.7.

I checked the vacuity had not merely RELOCATED into chia-query, which is where it would hide.
It has not. In chia-query 0.20.0 (lock pin ca14bd03...) TxStatus.error is Option<String> with
#[serde(default)] (types/response.rs:102), and both translators pass the wire value through
verbatim and manufacture nothing: peer/translate.rs:109-122 and coinset/mod.rs:33-46. Their only
callers source it from the wire — peer/mod.rs:1079 (ack.error) and coinset/mod.rs:475
(json["error"].as_str()). Nothing anywhere fills error from status. The distinction
stated_rejection branches on is real.

2. The other direction — does a stated rejection still FREE? Yes, and there is no lockout.
FAILED + BAD_AGGREGATE_SIGNATURE -> Some("FAILED: BAD_AGGREGATE_SIGNATURE") -> guard true ->
no reservation -> coins stay selectable. rpc.rs:527 RESERVATION_TTL_MS = 600_000 (10 min), so even
the widened hold is bounded at ten minutes. No CRITICAL lockout finding.

3. Is stated_rejection the only producer? Yes, in production. git grep "PushOutcome {" at head
returns exactly two production constructions, both branches of ChainTransport::push
(chain.rs:536, :542), and exactly one production impl SignedBundlePusher (chain.rs:566).
Every other producer is FakePusher in rpc.rs tests.

4. Operator visibility. The stated reason survives verbatim, verdict-prefixed, to
control.rs:2549. Issue 48's BAD_AGGREGATE_SIGNATURE is not dropped. The bare verdict is now
dropped — Finding 3 below, LOW.

5. SPEC 18.7 — the code was changed to match the spec, NOT the reverse. SPEC.md does not appear
in git diff 09095c95 3cb731f at all. For completeness I checked provenance against main: the 18.7
paragraph does not exist in 3e480dd, it was added inside this PR by d9be753. So the honest
sequence is: at 09095c9 that paragraph was a born-false normative claim (asserting a MUST the
vacuous guard did not satisfy); 3cb731f makes it true. Right direction, and I found no edit
softening the spec toward the code.

Why the pre-existing tests did not catch this — for the gate-rot log. The three 348 tests
(rpc.rs:10723, :10783, :10827) drive a FakePusher that constructs PushOutcome by hand,
so a_bundle_denied_without_a_reason_is_held_rather_than_freed asserts on rejection: None — a shape
ChainTransport::push could not produce at 09095c9. The double was wrong in exactly one
direction and made the failure unreachable: the tests were green while the guard was vacuous. The new
a_bare_verdict_is_not_a_stated_rejection tests the real producer directly, which is the right place
to close it.


Finding 1 — MEDIUM: the fix swallowed ChainTransport::push's doc comment and moved a 908 custody claim onto the wrong function

crates/dig-wallet/src/sage/chain.rs:490-513. The helper was inserted between push's doc block
and push
, so the block now reads:

/// Push an ALREADY-SIGNED bundle.
///
/// The node never signs and is never given anything it could sign with (908): this takes a
/// complete bundle and relays it. A mempool refusal comes back as `Ok(PushOutcome)` with
/// `accepted: false`; an unreachable network is an `Err`.
/// The mempool's OWN stated reason for refusing a bundle, or `None` when it stated none.
...
fn stated_rejection(status: &chia_query::TxStatus) -> Option<String> {

Two consequences:

  1. pub async fn push — a public API on the custody path — now carries no doc comment at all (6.2).
  2. The 908 statement now documents a private pure function that holds no key, pushes nothing, and
    takes a &TxStatus. "this takes a complete bundle and relays it" is false of stated_rejection.
    A custody invariant recorded against the wrong symbol is the drift class 4.2 exists to prevent:
    the next reader looking for where 908 is enforced on the push path finds it on a string formatter.

Not exploitable. One-line fix: move the four stated_rejection doc lines above the helper and restore
push's block to push.

Finding 2 — MEDIUM, defence-in-depth, PRE-EXISTING, do NOT gate: a stated reason meaning "I already have it" still frees the coins

rpc.rs:2151-2153. The guard premise is "a refusal the mempool STATED is definitive". That is sound
for BAD_AGGREGATE_SIGNATURE. It is not sound for DOUBLE_SPEND, MEMPOOL_CONFLICT or
ALREADY_INCLUDING_TRANSACTION — stated reasons meaning the bundle (or a conflicting one) is in
flight
, which is exactly when freeing the inputs is most dangerous.

chia-query makes this reachable rather than theoretical. router.rs:793 push_tx calls
peer_then_coinset(peer, peer_retry, coinset), and router.rs:139-167 transmits the bundle to up
to three destinations
:

peer A admits the bundle to its mempool, but the ack times out (peer/mod.rs:1074, "request timed
out") -> peer A is ejected and the bundle is re-transmitted to peer B -> peer B has already seen it
gossiped and answers with a STATED reason -> is_definitive_rejection is true ->
reserve_pushed_bundle is skipped -> the inputs of a bundle sitting in the public mempool return to
the selectable set -> a second send inside the 10-minute window reselects them.

This is not a regression and must not gate. At 09095c9 rejection was Some(..) on every
non-admitted answer, so this path freed the coins along with all the others; 3cb731f strictly
narrows it. But it is now the only remaining free path, which is what makes it worth a ticket: the
guard should arguably key on the class of stated reason, not merely on its presence. Not filed.

Finding 3 — LOW: a bare verdict is no longer visible anywhere

A non-admitted answer with no error text now yields {accepted:false, transaction_id:null, rejection:null} at control.rs:2544-2551, and ChainTransport::push logs nothing. The operator
loses the PENDING/UNKNOWN label entirely. This satisfies the brief bar (a stated reason is never
dropped) and the response does not lie — "not accepted, no reason given" is true — so it is not a
money-honesty stop. But an operator debugging a stuck broadcast now gets strictly less than before.
Cheap remedy: a tracing line carrying status.status, or a separate verdict field.

Related, while someone is in there: control.rs:2529 still reads "A mempool that examined the bundle
and refused it answers {accepted:false, rejection}"
, which now describes a shape that no longer
always holds.


Still to do: cut a private worktree and re-run the revert-proof for
a_bare_verdict_is_not_a_stated_rejection (checking the test COUNT, not merely the exit status); G2
knob-list completeness; G3 accuracy in net.rs.

Narrowing `rejection` to a STATED reason is what lets #348's hold fire, but taken
alone it left an operator debugging a bare `PENDING` with `accepted:false`,
`transaction_id:null`, `rejection:null` and no label at all. The response did not
lie -- "not accepted, no reason given" is true -- but it said strictly less than
before, which is its own regression on a money path.

`PushOutcome` gains `verdict`, always present. The two fields answer different
questions and only one of them may drive the hold, which is why they are separate
rather than one string: folding the label back into `rejection` would re-break the
guard, and dropping it re-blinds the operator.

`control.wallet.broadcast` surfaces it, and the doc above it no longer claims a
refusal always answers `{accepted:false, rejection}` -- a shape that stopped
always holding when `rejection` narrowed.

Caught by the security re-gate as a LOW observability finding on my own fix.

Refs #348

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment