fix(error)!: hold relay-supplied text as SafeText, bump dig-nat to 0.15 - #51
Merged
Merged
Conversation
The untrusted relay's `RelayMessage::Error { message }` was interpolated raw
into `GossipError::RelayError`, so a newline the relay chose forged a whole
second line in any log rendering the error. A second live instance sat in
`RelayClient::handle_register_ack`, which formatted a relay-supplied RegisterAck
`message` the same way.
Rather than sanitize at each render site — the approach this defect already
defeated once per variant — the variants whose text comes from a remote party
now hold `dig_nat::SafeText` (new in dig-nat 0.15.0), whose only doors are
`from_untrusted` and `from_static(&'static str)`. A runtime `String` cannot pass
through either, so raw peer text is unrepresentable rather than guarded, and
`SafeText`'s `Debug` renders like its `Display` because `{:?}` reaches a log at
least as often as `{}` does. Relay failures in `nat::discovery` funnel through
one `relay_failure` helper so there is a single place to read to know this holds.
Two deliberate non-changes, both recorded at the enum:
* `PeerNotConnected` / `PeerBanned` / `DuplicateConnection` were reported as
`String` but hold a `PeerId` — a fixed 32-byte `Bytes32` rendered as hex. A
peer cannot put text of its choosing there, so they are already
unrepresentable, and widening them to a text type would invite a future caller
to interpolate a formatted string into a field that currently cannot hold one.
* `IoError` / `InvalidConfig` / `AddressManagerStore` / `IntroducerError` keep
`String`: their text comes from the OS, the local config, or this crate's own
literals, never from a stranger.
`tests/sec_1883_peer_text_tests.rs` drives a hostile `message` — a real newline,
a bidi override and a NUL — through a real loopback WebSocket relay and asserts
on the error the moment `relay_get_peers` returns it, before anything logs it,
so a fix placed at a logging site instead of the wire boundary fails it. Each
assertion covers `Display` and `Debug`, and each test carries a control
demanding the error still name the subsystem, the relay's status code, and the
escaped reason — verified load-bearing by confirming that a variant which drops
the detail entirely fails those controls.
The dig-nat re-pin is mandatory rather than cosmetic: a 0.x MINOR is
semver-incompatible, so `"0.14"` can never resolve 0.15.0, and `SafeText` now
crosses dig-dht's and dig-peer's public error surfaces, making a tree with two
dig-nat versions an outright E0308 on those seams.
BREAKING CHANGE: `GossipError::RelayError`, `ConnectionFiltered` and `NatError`
carry `dig_nat::SafeText` instead of `String`; match arms binding the payload
need `.as_str()`. The `dig-nat` re-pin also rotates the identity of the dig-nat
types this crate re-exports from `src/nat`.
Refs DIG-Network/dig_ecosystem#1883, DIG-Network/dig_ecosystem#1674
Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
force-pushed
the
harden/1883-safetext-peer-error-text
branch
from
July 31, 2026 15:33
2e20b3e to
f0ff72d
Compare
MichaelTaylor3d
marked this pull request as ready for review
July 31, 2026 15:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes DIG-Network/dig_ecosystem#1883. Part of the dig-nat 0.15 release-first cascade (DIG-Network/dig_ecosystem#1674 / #1675).
The defect, and where it actually was
The ticket named five
GossipErrorvariants asString. Three of them are not:PeerNotConnected,PeerBannedandDuplicateConnectioncarry aPeerId, which is achia_protocol::Bytes32— a fixed 32 bytes rendered as hex. A peer cannot put text of its choosing there, so no forged peer id containing a newline exists to inject and those three are already unrepresentable. They are left asPeerIdon purpose: widening them to a text type would invite a future caller to interpolate a formatted string into a field that currently cannot hold one.The two String variants were real, and the type change surfaced a live instance the ticket had not found:
nat/discovery.rs—RelayMessage::Error { code, message }interpolatedmessageraw. That field is aStringdeserialized straight out of an explicitly untrusted relay's JSON, so a hostile relay chose its bytes freely. Proven live over a real loopback WebSocket before the fix:relay/relay_client.rs—handle_register_ackformatted a relay-supplied RegisterAckmessagethe same way.The fix
The variants whose text comes from a remote party (
RelayError,ConnectionFiltered,NatError) holddig_nat::SafeText, new in dig-nat 0.15.0. Its only doors arefrom_untrusted(escapes control + bidi characters, bounds the length) andfrom_static(&'static str), which a runtimeStringcannot pass through — so raw peer text becomes unrepresentable rather than guarded. That is the point: this defect was reopened once per variant by callers individually forgetting a convention, so a sanitiser you must remember to call is the wrong shape of fix.SafeText'sDebugrenders like itsDisplay, which matters here because several call sites use{:?}and a fix wired only toDisplaywould regress them silently. Relay failures innat::discoverynow funnel through onerelay_failurehelper, so there is a single place to read to know no relay-chosen byte reaches a rendered error.IoError/InvalidConfig/AddressManagerStore/IntroducerErrorkeepString— their text comes from the OS, the local config, or this crate's own literals. Recorded at the enum with the reasoning, so the next reader does not have to re-derive which variants are in the class.How it was verified
tests/sec_1883_peer_text_tests.rs(3 tests, new):messagecarrying a real newline, an RLO bidi override (categoryCf, whichchar::is_controlmisses) and a NUL travels a real loopback WebSocket relay speaking the RLY-005 wire. The assertion is on the error value the momentrelay_get_peersreturns it, before anything logs it — so a fix applied at a logging site rather than at the wire boundary leaves that value dirty and fails.PeerIdvariants, pinning why they stay a fixed-width id.Every assertion checks
DisplayandDebug. Every test carries a control so the fix cannot be satisfied by dropping detail: the error must still name the subsystem, carry the relay's status code, and show the escaped reason. Both directions were confirmed rather than assumed:SafeText::from_static("relay query failed")) fails the control (the error must still carry the relay's status code), even though it satisfies every newline assertion.Gates:
cargo fmt --checkclean,cargo clippy --all-targets --locked -- -D warningsclean,cargo test --lockedgreen across the suite.Pre-existing failures, NOT from this change (baseline-verified):
tests/con_1691_reconnect_tests.rsfails 4 of 5 on a pristineorigin/mainworktree as well — peer-map newest-wins supersede semantics, untouched here. Separately, the trackedCargo.lockonorigin/mainrecordsdig-gossip 0.17.19whileCargo.tomlsays0.17.21, socargo test --lockedfails onmainoutright; this branch's lock is re-resolved and correct. Both are being reported for their own tickets.Blast radius checked
Per the standing override, blast radius via ripgrep + a direct read of every construction and match site rather than gitnexus.
GossipError::{RelayError,ConnectionFiltered,NatError}has 30 references acrosssrc/error.rs,src/nat/discovery.rs,src/relay/relay_client.rs,src/service/gossip_handle.rsand 5 test files; the compiler enumerated all 14 production/test breakages after the type change and each was migrated (.as_str()at match sites that bound the payload).PeerIdwas traced tochia_protocol::Bytes32to establish that the other three variants needed no change. Risk: breaking for consumers — the payload type changes and the dig-nat re-pin rotates the identity of the dig-nat types re-exported fromsrc/nat, hence the MINOR on 0.x.Dependency + version
dig-nat = "0.14"→"0.15". The tracked lock resolvesdig-nat 0.15.0anddig-tls 0.3.0fromregistry+https://github.com/rust-lang/crates.io-index, one copy of each. The pre-existing[patch.crates-io]in this manifest covers only the vendoredchia-protocol/chia-sdk-client/native-tls; it does not touch the cascade crates, and it was not modified here.0.17.21→0.18.0(MINOR = breaking on 0.x): the variant payload types and the re-exported dig-nat type identities are both public-API changes.Docs kept coherent in the same unit
docs/resources/SPEC.md§4 anddocs/requirements/domains/crate_api/specs/API-004.mdboth declaredRelayError(String)normatively, including an acceptance checkbox asserting it "contains aString". Both now stateSafeTextwith the reason, and the API-004 variant table and itstest_relay_error_displayrow are updated to match.Consumers must pin
dig-gossip is not on crates.io (DIG-Network/dig_ecosystem#900 / #1872), so this version bump publishes nothing — consumers take it by git rev. A consumer moving to this commit must simultaneously be on dig-nat 0.15, since this crate re-exports dig-nat types from
src/natand a second dig-nat in the tree is now anE0308. Note that a git-rev consumer does not inherit this manifest's[patch.crates-io].