diff --git a/Cargo.lock b/Cargo.lock index 55764dd..c7f498c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2266,7 +2266,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.65.0" +version = "0.65.1" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 5937d8e..4254996 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.65.0" +version = "0.65.1" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over diff --git a/DEVELOPMENT_LOG.md b/DEVELOPMENT_LOG.md index 6334c14..e994257 100644 --- a/DEVELOPMENT_LOG.md +++ b/DEVELOPMENT_LOG.md @@ -839,3 +839,34 @@ TRANSITIVE LOCK entry for exactly this reason, and a caret dep looking correct p array summing to `total_length`, so a partial layout is unusable rather than partially useful. That is why a paged prologue must complete before a stream ends — including on a one-byte request, where the remaining pages ride zero-payload frames — and why `complete` is withheld until the last page is out. + +## An address is a TYPE, not a string — and a fixed guard's allowlist takes its detectors with it (#1682, #1722) + +`Config::bind_addr` rendered `DIG_NODE_HOST` and the port with `format!("{host}:{port}")`. For an +IPv6 host that yields `::1:9778`, which is not a socket address in the grammar — and since a bind +failure on that listener is documented FATAL, configuring the address family §5.2 PREFERS took the +node down. The lesson is not "remember to bracket": it is that a function returning `String` cannot +stop the next caller, so the accessors now return `SocketAddr` and the authority can only be rendered +by `SocketAddr`'s own `Display`. Typing it found two further live defects nobody had ticketed — the +control client's `http://{addr}/` JSON-RPC URL and the `extension_host` operator log line — because +both consumed the same broken string. + +Two things worth carrying beyond this fix: + +**A guard's allowlist can be load-bearing for the guard itself.** The #1593 source scanner proved it +had escaped its own crate by asserting `waived == KNOWN_VIOLATIONS.len()`: both tracked violations +lived in the sibling crate `dig-node-service`, so reaching them proved the scan's reach. Fixing both +defects emptied the list and turned that assertion into `0 == 0` — the check evaporated at exactly +the moment it succeeded, and nothing would have reported it. When you pay off a tracked-violation +list, look for what the list was incidentally proving; here the reach detector was replaced with a +direct assertion that the scan read files from the sibling crate. Same property, no dependence on +debt existing. + +**A ticket saying "the margin is zero" may be measuring a different relation than the one it names.** +#1722 reported dig-node's `ADVERTISED_TTL_SECS` as having zero margin against dig-dht's +`provider_ttl`. It has an hour of margin against `provider_ttl` (1h vs 2h) and zero against +`republish_interval` (1h vs 1h) — two distinct relations with opposite failure modes: exceeding +`provider_ttl` gets the claim silently clamped, while a `republish_interval` above the advertised TTL +makes records expire before the holder re-announces. Pinning only the named relation would have left +the real one unguarded. No single value satisfies both bounds with margin, so closing the zero margin +is a wire-behaviour decision, not a test fix. diff --git a/SPEC.md b/SPEC.md index 84c40cf..c464867 100644 --- a/SPEC.md +++ b/SPEC.md @@ -320,6 +320,15 @@ The server opens UP TO THREE listeners for the SAME router: address — this listener is then skipped, not added to. This bind is **best-effort**: on failure (IPv6 loopback unavailable/disabled) the node MUST log a structured warning to stderr and continue IPv4-only — it MUST NOT abort. +**Authority construction (§5.2, #1682).** Every socket authority the node binds, reports, or embeds +in a URL MUST be built from a typed address and port — never by concatenating the two as text. A +literal IPv6 address therefore always appears BRACKETED (`[::1]:9778`, `[2001:db8::1]:9778`), which +is what the socket-address and URL grammars require. `DIG_NODE_HOST` set to any IPv6 literal MUST +bind successfully; because a failure on listener 1 is FATAL, rendering that authority as unbracketed +text would make configuring the address family this ecosystem PREFERS a self-inflicted outage. This +governs the `/health` `addr` field, the `status` output, the control-client's JSON-RPC URL, and the +`open` command's browser-navigable candidate URLs equally. + 3. **`127.0.0.2:80`** — the bare-`http://dig.local` listener (constants `DIG_LOCAL_IP` = `127.0.0.2`, `DIG_LOCAL_PORT` = `80`, `DIG_LOCAL_HOST` = `dig.local`). This bind is **best-effort**: on failure (no privilege, port in use, missing macOS `127.0.0.2` loopback diff --git a/crates/dig-node-core/src/seams/dig_peer/holdings.rs b/crates/dig-node-core/src/seams/dig_peer/holdings.rs index 4058a31..b07ee1c 100644 --- a/crates/dig-node-core/src/seams/dig_peer/holdings.rs +++ b/crates/dig-node-core/src/seams/dig_peer/holdings.rs @@ -54,9 +54,17 @@ use dig_gossip::{ /// still serving. One hour sits inside every current provider TTL, so the clamp is a no-op and the /// advertised lifetime is the one the announcer actually meant. /// -/// COUPLING: this value MUST track [`dig_dht::DhtConfig::provider_ttl`] (default 2 hours) and stay -/// `<=` the smallest one any receiving node configures. Reducing that field below one hour, without a -/// matching reduction here, silently truncates every advertisement this node makes. +/// COUPLING — ENFORCED, not merely described (#1722). Two relations bind this value to dig-dht, and +/// `tests/holdings_ttl_coupling.rs` fails if either breaks: +/// - `ADVERTISED_TTL_SECS <= `[`dig_dht::DhtConfig::provider_ttl`] (default 2h) — exceeding it makes +/// every claim silently clamped, so the announcer's intent is lost. +/// - [`dig_dht::DhtConfig::republish_interval`]` <= ADVERTISED_TTL_SECS` (default 1h) — a record that +/// expires before the holder re-announces drops the holder out of discovery until it does. +/// +/// The second currently holds EXACTLY, with zero margin: dig-dht republishes hourly and this claims +/// one hour. That is measured, not designed; the test records it so neither side can drift silently. +/// It also stays `<=` the smallest `provider_ttl` any receiving node configures — which is why the +/// value is not simply raised to 2h to buy refresh margin. pub const ADVERTISED_TTL_SECS: u64 = 3_600; /// Announcements one **provider** may have applied per [`RATE_WINDOW_SECS`]. diff --git a/crates/dig-node-core/tests/banned_address_patterns.rs b/crates/dig-node-core/tests/banned_address_patterns.rs index a612bf6..9fecf13 100644 --- a/crates/dig-node-core/tests/banned_address_patterns.rs +++ b/crates/dig-node-core/tests/banned_address_patterns.rs @@ -22,39 +22,33 @@ use std::path::{Path, PathBuf}; /// Violations that already existed when this guard was strengthened — TRACKED, not waived. /// -/// Each entry is a real instance of the banned construct that a weaker version of this scanner (one that -/// matched three literal spellings) never saw. They are recorded here rather than fixed in the same -/// change because fixing them alters production bind/URL behaviour, which belongs in its own reviewed -/// change; they are recorded rather than hidden because weakening the matcher to make the guard green -/// would trade a real defect for a comfortable test. +/// **Currently EMPTY**, and that is the intended steady state: both original entries were the +/// #1682 defects and both are now fixed at the source. It is kept rather than deleted because the +/// mechanism — an exception matched on its distinguishing SNIPPET and its FILE, never on a line +/// number or a filename alone — is what makes a future tracked violation recordable without going +/// blind, and [`a_tracked_violation_excuses_only_its_own_call_not_its_whole_file`] keeps that +/// property honest against an empty list too. /// -/// Both are live IPv6 defects, not stylistic: -/// - `config.rs` renders `DIG_NODE_HOST` + port by text, so an operator setting a v6 host gets the -/// unbracketed `::1:9333`, which then fails to parse at bind time — and that bind failure is -/// documented as FATAL. -/// - `open.rs` builds a URL authority the same way, so a v6 host yields an unparseable URL. -/// -/// An entry is matched on its distinguishing snippet, not a line number, so unrelated edits to these -/// files do not silently move the exception onto different code. **Delete the entry when the site is -/// fixed**, which also drops it from the waived-count assertion in -/// [`no_source_file_builds_a_socket_address_from_concatenated_text`] — the two maintain each other. -/// -/// Tracked as **#1682**. -const KNOWN_VIOLATIONS: &[(&str, &str)] = &[ - // #1682 — `Config::bind_addr` renders host + port by text, so `DIG_NODE_HOST=::1` yields the - // unbracketed `::1:9333` and the bind then fails — a failure documented as FATAL. - ( - "dig-node-service/src/config.rs", - "self.host.unwrap_or(IpAddr::V4(Ipv4Addr::LOCALHOST))", - ), - // #1682 — `serve_url` builds a URL authority the same way, so a v6 host yields an unparseable URL. - ("dig-node-service/src/open.rs", r#""{host}:{p}""#), -]; +/// Adding an entry is a debt, not a waiver: it needs a ticket, and the reason fixing it does not +/// belong in the same change. Weakening the matcher instead is never the answer — that trades a +/// real defect for a comfortable test, which is precisely how these two defects stayed invisible. +const KNOWN_VIOLATIONS: &[(&str, &str)] = &[]; + +/// A relative path with platform separators normalized to `/`, so every path comparison in this +/// scanner reads the same on Windows and Unix. +fn normalize(relative_path: &str) -> String { + relative_path.replace('\\', "/") +} -/// Is this call one of the [`KNOWN_VIOLATIONS`], in the file that entry names? -fn is_known_violation(relative_path: &str, call: &str) -> bool { - let normalized = relative_path.replace('\\', "/"); - KNOWN_VIOLATIONS +/// Is this call excused by `tracked`, in the file that entry names? +/// +/// Takes the list as a PARAMETER rather than reading [`KNOWN_VIOLATIONS`] directly so the matching +/// rule stays provable while the real list is empty. A self-test that could only feed it real +/// entries would go vacuous the moment the debt was paid — and the rule it proves (an entry excuses +/// its own call, never its whole file) is exactly what must not rot before the NEXT entry is added. +fn is_known_violation(tracked: &[(&str, &str)], relative_path: &str, call: &str) -> bool { + let normalized = normalize(relative_path); + tracked .iter() .any(|(file, snippet)| normalized.ends_with(file) && call.contains(snippet)) } @@ -317,6 +311,14 @@ fn without_comment_lines(source: &str) -> String { const MINIMUM_FILES_SCANNED: usize = 100; const MINIMUM_FORMAT_CALLS_SEEN: usize = 600; +/// How many `dig-node-service` files the scan must read — the SIBLING-crate reach detector. +/// +/// One would prove the point; the floor sits higher so a partial read is caught too, and well under +/// the real count (measured 20 at introduction) to survive ordinary churn. Any value above zero +/// gives the property the waived count used to give and no longer can: proof that the scan looked +/// OUTSIDE the crate it lives in. +const MINIMUM_SIBLING_CRATE_FILES: usize = 5; + /// **Proves:** no source file in the workspace builds a socket address by string concatenation (#1593) — /// AND that the scan actually reached the workspace to find that out. /// @@ -334,10 +336,15 @@ const MINIMUM_FORMAT_CALLS_SEEN: usize = 600; /// `crates/00-foundation/dig-node-core`, [`crates_root`] resolves to `crates/00-foundation` — `read_dir` /// SUCCEEDS, the self-test stays green, and every sibling level goes silently unscanned. /// -/// The waived-count assertion is what makes that detectable rather than merely unlikely: both tracked -/// sites live in the SIBLING crate `dig-node-service`, so reaching them proves the scan escaped its own -/// crate. It also self-maintains — fixing a site deletes its entry and its share of this assertion -/// together, and a stale entry turns the scan RED rather than green. +/// The SIBLING-CRATE assertion is what makes that detectable rather than merely unlikely: the scan +/// must have read files from `dig-node-service`, which lives beside this crate and not under it, so a +/// [`crates_root`] that collapsed to one crate's directory fails LOUDLY instead of passing clean. +/// +/// That detector replaced the waived-count one (#1682). The original relied on both tracked +/// violations living in `dig-node-service`: reaching them proved the scan escaped its own crate. But +/// that check was only ever as strong as the debt was long — fixing both defects took the waived +/// count to `0 == 0`, which is exactly the vacuous shape the file floor alone already was. Pinning +/// the sibling crate directly says the same thing and keeps saying it with an empty allowlist. #[test] fn no_source_file_builds_a_socket_address_from_concatenated_text() { let root = crates_root(); @@ -345,6 +352,7 @@ fn no_source_file_builds_a_socket_address_from_concatenated_text() { let mut files_scanned = 0usize; let mut format_calls_seen = 0usize; let mut waived = 0usize; + let mut sibling_crate_files = 0usize; for file in rust_sources(&root) { // This file necessarily CONTAINS the banned pattern — as the strings it matches on, and as the // fixtures that prove the matcher works. Scanning itself would make the ban permanently red. @@ -363,13 +371,16 @@ fn no_source_file_builds_a_socket_address_from_concatenated_text() { .unwrap_or(&file) .display() .to_string(); + if normalize(&relative).starts_with("dig-node-service/") { + sibling_crate_files += 1; + } for (line, call) in format_calls(&without_comment_lines(&contents)) { format_calls_seen += 1; let collapsed = call.split_whitespace().collect::>().join(" "); if !builds_an_address_from_text(&call) { continue; } - if is_known_violation(&relative, &collapsed) { + if is_known_violation(KNOWN_VIOLATIONS, &relative, &collapsed) { waived += 1; } else { offenders.push(format!("{relative} line {line}: format!({collapsed})")); @@ -399,11 +410,13 @@ fn no_source_file_builds_a_socket_address_from_concatenated_text() { assert_eq!( waived, KNOWN_VIOLATIONS.len(), - "expected to reach all {} tracked violations and reached {waived}. Both live in the SIBLING \ - crate `dig-node-service`, so this failing means either the scan no longer escapes its own \ - crate, or a tracked site was fixed — in which case delete its `KNOWN_VIOLATIONS` entry (#1682).", + "expected to reach all {} tracked violations and reached {waived}. A stale entry — one whose site was fixed — turns this RED rather than green: delete it (#1682).", KNOWN_VIOLATIONS.len() ); + assert!( + sibling_crate_files >= MINIMUM_SIBLING_CRATE_FILES, + "the scan read only {sibling_crate_files} files from the SIBLING crate `dig-node-service` (expected at least {MINIMUM_SIBLING_CRATE_FILES}), so it never escaped its own crate and a clean result proves nothing about the workspace. Check `crates_root` — a move into an Appendix-B level directory does exactly this while leaving every other assertion green." + ); } /// **Proves:** the tracked-violation list excuses ONLY the exact sites it names — a different offending @@ -414,22 +427,32 @@ fn no_source_file_builds_a_socket_address_from_concatenated_text() { /// filename alone would pass this file and fail this test. #[test] fn a_tracked_violation_excuses_only_its_own_call_not_its_whole_file() { - let (tracked_file, tracked_snippet) = KNOWN_VIOLATIONS[0]; + // A SYNTHETIC list, so this holds while `KNOWN_VIOLATIONS` is empty. The snippet and file are + // shaped like a real entry; nothing here depends on the real one existing. + const TRACKED: &[(&str, &str)] = &[("dig-node-service/src/config.rs", "the_tracked_call()")]; + let (tracked_file, tracked_snippet) = TRACKED[0]; assert!( - is_known_violation(tracked_file, tracked_snippet), + is_known_violation(TRACKED, tracked_file, tracked_snippet), "the tracked site must be recognised in its own file" ); assert!( - !is_known_violation(tracked_file, r#""{ip}:{port}""#), + !is_known_violation(TRACKED, tracked_file, r#""{ip}:{port}""#), "a DIFFERENT offending call in a tracked file must still be an offender" ); assert!( !is_known_violation( + TRACKED, "dig-node-core/src/seams/dig_peer/union_locator.rs", tracked_snippet ), "a tracked snippet must not be excused in a file the entry does not name" ); + // An EMPTY list excuses nothing — the state the list is actually in, asserted rather than + // assumed, so a bug that made an empty list match everything could not pass unnoticed. + assert!( + !is_known_violation(&[], tracked_file, tracked_snippet), + "an empty tracked list must excuse nothing" + ); } /// Runs a source snippet through the SAME pipeline the real scan uses, so the self-test exercises the diff --git a/crates/dig-node-core/tests/holdings_ttl_coupling.rs b/crates/dig-node-core/tests/holdings_ttl_coupling.rs new file mode 100644 index 0000000..224f642 --- /dev/null +++ b/crates/dig-node-core/tests/holdings_ttl_coupling.rs @@ -0,0 +1,152 @@ +//! The advertised-TTL ↔ provider-record-lifetime coupling, enforced instead of commented (#1722). +//! +//! [`ADVERTISED_TTL_SECS`] is the `expires_at` this node claims on every opcode-222 `Add`. Whether +//! that claim is honoured depends on TWO dig-dht values it has no compile-time link to, and getting +//! either relation wrong fails SILENTLY — discovery goes intermittently lossy rather than visibly +//! broken, which is the worst shape a bug can take in a replication path. +//! +//! Until now the coupling was a doc-comment naming the symbol. A comment cannot fail, so a change on +//! either side would have shipped. These tests fail instead. +//! +//! # The two relations, which are NOT the same relation +//! +//! 1. **`advertised <= provider_ttl`** — the TRUNCATION bound. dig-dht clamps an ingested record to +//! `min(record.expires_at, now + provider_ttl)`, so claiming longer than the receiver's TTL is +//! silently cut back and the announcer's intent is lost. Current margin: 1h claimed against a 2h +//! default — comfortable. +//! +//! 2. **`republish_interval <= advertised`** — the REFRESH bound. The holder re-announces every +//! `republish_interval`; if a record expires before that fires, the holder vanishes from discovery +//! until it does. Current margin: **ZERO** — dig-dht republishes hourly and this node advertises +//! exactly one hour, so the record expires at the same instant the refresh is due. +//! +//! #1722 described "the margin" as zero, which is true of (2) and not of (1). Pinning only the +//! relation named in the ticket would have left the other unguarded, so both are pinned here. +//! +//! The zero margin in (2) is deliberately NOT closed by this change — see +//! [`the_refresh_bound_holds_but_only_exactly`] for why that is a wire-behaviour decision rather +//! than a test fix. + +use std::time::Duration; + +use dig_dht::DhtConfig; +use dig_node_core::seams::dig_peer::holdings::ADVERTISED_TTL_SECS; + +/// Why an advertised TTL is unsound against a given pair of dig-dht intervals, or `Ok`. +/// +/// A PURE predicate over all three values, rather than three inline assertions on the real +/// constants, so the rule can be exercised at and past each bound. Asserting only the real values +/// would confirm today's numbers without proving the check can fail at all — and a bound tested from +/// one side only can never distinguish a real guard from a tautology. +fn advertised_ttl_is_sound( + advertised: Duration, + provider_ttl: Duration, + republish_interval: Duration, +) -> Result<(), String> { + if advertised > provider_ttl { + return Err(format!( + "advertised {advertised:?} exceeds provider_ttl {provider_ttl:?}: every claim would be \ + silently clamped to the shorter lifetime" + )); + } + if republish_interval > advertised { + return Err(format!( + "republish_interval {republish_interval:?} exceeds advertised {advertised:?}: records \ + expire before the holder re-announces, so discovery loses the holder periodically" + )); + } + Ok(()) +} + +/// The advertised TTL as a [`Duration`], so it compares directly against dig-dht's fields. +fn advertised() -> Duration { + Duration::from_secs(ADVERTISED_TTL_SECS) +} + +/// **Proves:** the REAL `ADVERTISED_TTL_SECS` is sound against the REAL `DhtConfig::default()` — +/// both relations at once, read from dig-dht itself rather than from a number copied into a comment. +/// +/// **Catches:** a change on EITHER side of a coupling that has no compile-time link. Lowering +/// dig-dht's `provider_ttl` below one hour, raising its `republish_interval` above one hour, or +/// moving `ADVERTISED_TTL_SECS` in either direction all fail here. That is the whole point: this is +/// the only thing in the tree that would notice. +#[test] +fn the_advertised_ttl_is_sound_against_dig_dhts_real_defaults() { + let config = DhtConfig::default(); + assert_eq!( + advertised_ttl_is_sound(advertised(), config.provider_ttl, config.republish_interval), + Ok(()), + "advertised={:?} provider_ttl={:?} republish_interval={:?}", + advertised(), + config.provider_ttl, + config.republish_interval + ); +} + +/// **Proves:** the truncation bound is enforced from BOTH sides — at the bound it passes, one second +/// over it fails. +/// +/// **Catches:** the predicate degrading into something that cannot fail (a `>=` flipped, a +/// comparison dropped). A guard asserted only on values that satisfy it proves nothing about the +/// guard. +/// +/// The one-second step is chosen over a round number on purpose: the bound is `<=`, so equality must +/// PASS and the very next representable value must FAIL. A test that overshot by an hour would still +/// pass against a predicate with an off-by-one in it. +#[test] +fn the_truncation_bound_is_enforced_from_both_sides() { + let provider_ttl = Duration::from_secs(7_200); + let republish = Duration::from_secs(3_600); + + assert_eq!( + advertised_ttl_is_sound(provider_ttl, provider_ttl, republish), + Ok(()), + "advertising EXACTLY provider_ttl is sound — the clamp is a no-op at equality" + ); + + let one_over = provider_ttl + Duration::from_secs(1); + let err = advertised_ttl_is_sound(one_over, provider_ttl, republish) + .expect_err("advertising one second beyond provider_ttl must be rejected"); + assert!( + err.contains("silently clamped"), + "expected the truncation diagnosis, got: {err}" + ); +} + +/// **Proves:** the refresh bound holds for the real values, and holds ONLY EXACTLY — one second more +/// republish interval fails. +/// +/// **Catches:** dig-dht raising `republish_interval` above an hour, which would put this node's +/// records in a permanent expire-then-refresh flap. It also records the finding in an executable +/// form: the assertion below states that the current values sit exactly ON the bound, so if anyone +/// later adds margin on either side, this test fails and forces them to update the recorded +/// relationship rather than leaving a stale claim behind. +/// +/// **Deliberately NOT fixed here.** Closing the margin means either advertising longer (up to +/// dig-dht's 2h `provider_ttl`, which gains an hour of refresh margin but is then clamped for any +/// receiver configured BELOW 2h — and `provider_ttl` is a config field, not a constant, so some +/// receiver being lower is a real possibility) or dig-dht republishing sooner (a change in another +/// repo, release-first). Both change on-wire behaviour, and they trade clamp-safety against +/// refresh-margin in opposite directions with no value satisfying both. That is a decision, not a +/// test fix, so #1722's mechanical pin lands here and the value choice is reported separately. +#[test] +fn the_refresh_bound_holds_but_only_exactly() { + let config = DhtConfig::default(); + assert_eq!( + config.republish_interval, + advertised(), + "the refresh margin is currently ZERO by measurement, not by design — dig-dht republishes \ + every {:?} and this node advertises {:?}. If either moved, update this recorded \ + relationship (#1722) rather than relaxing the assertion.", + config.republish_interval, + advertised() + ); + + let one_over = advertised() + Duration::from_secs(1); + let err = advertised_ttl_is_sound(advertised(), config.provider_ttl, one_over) + .expect_err("a republish interval one second past the advertised TTL must be rejected"); + assert!( + err.contains("expire before the holder re-announces"), + "expected the refresh diagnosis, got: {err}" + ); +} diff --git a/crates/dig-node-service/src/config.rs b/crates/dig-node-service/src/config.rs index 9a52637..1c07ba3 100644 --- a/crates/dig-node-service/src/config.rs +++ b/crates/dig-node-service/src/config.rs @@ -33,7 +33,7 @@ //! installs) — and set the SAME value for both the service and the browser launch //! so they keep sharing it. -use std::net::{IpAddr, Ipv4Addr}; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; /// Default loopback bind port — an UNCOMMON high port, deliberately clear of the /// collision-prone common-dev ports (80/443/3000/5000/8000/8080/8888/9000) that a @@ -227,17 +227,22 @@ impl Config { } } - /// The `host:port` socket string for the always-on localhost listener - /// (binding / logging): the explicit `DIG_NODE_HOST` override, or the default - /// `127.0.0.1` when unset. A bind failure on THIS address is fatal (see - /// `server::serve_with_shutdown`) — every consumer (CLI `status`/`pair`, the - /// installed-service summary, `/health`'s `addr` field) treats it as THE - /// address, so its shape never changes based on the dual-stack default below. - pub fn bind_addr(&self) -> String { - format!( - "{}:{}", + /// The socket address for the always-on localhost listener (binding / logging): the + /// explicit `DIG_NODE_HOST` override, or the default `127.0.0.1` when unset. A bind + /// failure on THIS address is fatal (see `server::serve_with_shutdown`) — every + /// consumer (CLI `status`/`pair`, the installed-service summary, `/health`'s `addr` + /// field) treats it as THE address, so its shape never changes based on the + /// dual-stack default below. + /// + /// Returns a [`SocketAddr`] rather than a string so the authority can only ever be + /// rendered by [`SocketAddr`]'s own `Display`, which brackets an IPv6 literal. The + /// previous text-concatenated form (#1682) produced the unbracketed `::1:9778` for an + /// IPv6 host, which is not a socket address at all — and the FATAL bind failure that + /// followed meant configuring the family §5.2 prefers took the node down. + pub fn bind_addr(&self) -> SocketAddr { + SocketAddr::new( self.host.unwrap_or(IpAddr::V4(Ipv4Addr::LOCALHOST)), - self.port + self.port, ) } @@ -250,28 +255,30 @@ impl Config { /// adding to it). This listener is BEST-EFFORT at bind time (see `serve`): an /// IPv6-loopback-unavailable system falls back to IPv4-only, mirroring the /// existing [`Config::dig_local_addr`] best-effort pattern. - pub fn bind_addr_v6(&self) -> Option { - self.host.is_none().then(|| format!("[::1]:{}", self.port)) + pub fn bind_addr_v6(&self) -> Option { + self.host + .is_none() + .then(|| SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), self.port)) } - /// The `host:port` socket string for the BEST-EFFORT bare-`http://dig.local` - /// listener (`127.0.0.2:80`), or `None` when `dig_local` is disabled (#91). + /// The socket address for the BEST-EFFORT bare-`http://dig.local` listener + /// (`127.0.0.2:80`), or `None` when `dig_local` is disabled (#91). /// `serve` tries to bind this in ADDITION to [`bind_addr`]; a failure is /// logged and ignored (localhost keeps serving). - pub fn dig_local_addr(&self) -> Option { + pub fn dig_local_addr(&self) -> Option { self.dig_local - .then(|| format!("{DIG_LOCAL_IP}:{DIG_LOCAL_PORT}")) + .then(|| SocketAddr::new(IpAddr::V4(DIG_LOCAL_IP), DIG_LOCAL_PORT)) } - /// The `host:port` for the BEST-EFFORT local HTTPS listener serving + /// The socket address for the BEST-EFFORT local HTTPS listener serving /// `https://dig.local` (`127.0.0.2:443`, #624), or `None` when `dig_local` is /// disabled. Shares the `dig_local` toggle with the plaintext `:80` listener — /// both are the "bare dig.local" surface, one plaintext, one TLS. The TLS listener /// is additionally gated on a dig-cert leaf being present (`crate::tls`); with no /// leaf yet only plaintext serves. - pub fn dig_local_https_addr(&self) -> Option { + pub fn dig_local_https_addr(&self) -> Option { self.dig_local - .then(|| format!("{DIG_LOCAL_IP}:{DIG_LOCAL_HTTPS_PORT}")) + .then(|| SocketAddr::new(IpAddr::V4(DIG_LOCAL_IP), DIG_LOCAL_HTTPS_PORT)) } /// The IPv6-loopback HTTPS bind (`[::1]:443`) to open BESIDE @@ -281,9 +288,9 @@ impl Config { /// so an IPv6 loopback client (e.g. `https://localhost` where `localhost` resolves /// to `::1` first) reaches the identical surface. BEST-EFFORT: a bind failure is /// logged and the node continues on the IPv4 listener (see `server`). - pub fn dig_local_https_addr_v6(&self) -> Option { + pub fn dig_local_https_addr_v6(&self) -> Option { self.dig_local - .then(|| format!("[::1]:{DIG_LOCAL_HTTPS_PORT}")) + .then(|| SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), DIG_LOCAL_HTTPS_PORT)) } } @@ -507,7 +514,10 @@ mod tests { // #132: the default localhost port is the uncommon high port 9778 (the // dig-wallet 9777 sibling), NOT the collision-prone 8080. assert_eq!(DEFAULT_PORT, 9778); - assert_eq!(c.bind_addr(), "127.0.0.1:9778"); + assert_eq!( + c.bind_addr(), + SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9778) + ); assert_eq!(c.upstream, DEFAULT_UPSTREAM); } @@ -519,8 +529,14 @@ mod tests { // IPv4 loopback AND the additional (best-effort) IPv6 loopback, same port. let c = Config::default(); assert_eq!(c.host, None); - assert_eq!(c.bind_addr(), "127.0.0.1:9778"); - assert_eq!(c.bind_addr_v6().as_deref(), Some("[::1]:9778")); + assert_eq!( + c.bind_addr(), + SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9778) + ); + assert_eq!( + c.bind_addr_v6(), + Some(SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 9778)) + ); } #[test] @@ -531,10 +547,101 @@ mod tests { host: Some(std::net::Ipv4Addr::new(10, 0, 0, 5).into()), ..Config::default() }; - assert_eq!(c.bind_addr(), "10.0.0.5:9778"); + assert_eq!( + c.bind_addr(), + SocketAddr::new(Ipv4Addr::new(10, 0, 0, 5).into(), 9778) + ); assert_eq!(c.bind_addr_v6(), None); } + // ----- #1682: an IPv6 authority is never rendered by text concatenation ----- + + /// Every bind address this config can produce, as the string the node actually binds, + /// paired with a label naming the accessor it came from. + /// + /// Collected through `to_string()` deliberately: that is the exact rendering the bind + /// path and every operator-facing URL consume, so a test that goes through it cannot + /// pass by inspecting typed parts the render then discards. + fn rendered_bind_addresses(c: &Config) -> Vec<(&'static str, String)> { + let mut out = vec![("bind_addr", c.bind_addr().to_string())]; + if let Some(v6) = c.bind_addr_v6() { + out.push(("bind_addr_v6", v6.to_string())); + } + if let Some(dl) = c.dig_local_addr() { + out.push(("dig_local_addr", dl.to_string())); + } + if let Some(dl) = c.dig_local_https_addr() { + out.push(("dig_local_https_addr", dl.to_string())); + } + if let Some(dl) = c.dig_local_https_addr_v6() { + out.push(("dig_local_https_addr_v6", dl.to_string())); + } + out + } + + /// **Proves:** a literal IPv6 `DIG_NODE_HOST` produces a bind address that PARSES — + /// asserted against the exact [`SocketAddr`] expected, not against a substring. + /// + /// **Catches:** the #1682 defect exactly. `bind_addr` rendered host and port by text, so + /// `DIG_NODE_HOST=::1` yielded the unbracketed `::1:9778`, which is not a socket address in + /// the grammar — and the bind failure on THIS address is documented FATAL. So configuring + /// the family §5.2 makes preferred took the node down. + /// + /// Two hosts, both real: the loopback an operator on Windows actually reaches the node by, + /// and a full-form global-unicast address whose own embedded colons are what make the + /// missing brackets ambiguous rather than merely ugly. + #[test] + fn an_ipv6_host_override_binds_a_parseable_address() { + for (host, expected) in [ + ( + IpAddr::V6(std::net::Ipv6Addr::LOCALHOST), + "[::1]:9778".parse::().expect("v6 loopback"), + ), + ( + "2001:db8::1".parse::().expect("v6 global literal"), + "[2001:db8::1]:9778" + .parse::() + .expect("v6 global"), + ), + ] { + let c = Config { + host: Some(host), + ..Config::default() + }; + let rendered = c.bind_addr().to_string(); + let parsed = rendered.parse::().unwrap_or_else(|e| { + panic!("DIG_NODE_HOST={host} renders {rendered:?}, which does not parse: {e}") + }); + assert_eq!(parsed, expected); + } + } + + /// **Proves:** EVERY address accessor renders a parseable authority under an IPv6 host — + /// not only the one accessor #1682 named. + /// + /// **Catches:** a fix applied at the single site that was reported while a sibling accessor + /// keeps concatenating. The set is enumerated from the config rather than listed per test, + /// so a NEW accessor is covered the moment it is added to `rendered_bind_addresses`. + #[test] + fn every_bind_accessor_renders_a_parseable_authority_for_an_ipv6_host() { + for host in [ + None, + Some(IpAddr::V6(std::net::Ipv6Addr::LOCALHOST)), + Some(IpAddr::V4(Ipv4Addr::LOCALHOST)), + ] { + let c = Config { + host, + ..Config::default() + }; + for (label, rendered) in rendered_bind_addresses(&c) { + assert!( + rendered.parse::().is_ok(), + "host={host:?}: {label} rendered {rendered:?}, which is not a socket address" + ); + } + } + } + #[test] fn parse_host_override_parses_a_valid_ip_literal() { assert_eq!( @@ -605,7 +712,10 @@ mod tests { // bare-dig.local listener, addressed 127.0.0.2:80. let c = Config::default(); assert!(c.dig_local); - assert_eq!(c.dig_local_addr().as_deref(), Some("127.0.0.2:80")); + assert_eq!( + c.dig_local_addr(), + Some(SocketAddr::new(DIG_LOCAL_IP.into(), 80)) + ); } #[test] @@ -622,8 +732,14 @@ mod tests { // The bare `https://dig.local` surface (#624) shares the dig_local toggle and // binds 127.0.0.2:443, with the IPv6 loopback sibling on [::1]:443 (§5.2). let c = Config::default(); - assert_eq!(c.dig_local_https_addr().as_deref(), Some("127.0.0.2:443")); - assert_eq!(c.dig_local_https_addr_v6().as_deref(), Some("[::1]:443")); + assert_eq!( + c.dig_local_https_addr(), + Some(SocketAddr::new(DIG_LOCAL_IP.into(), 443)) + ); + assert_eq!( + c.dig_local_https_addr_v6(), + Some(SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 443)) + ); } #[test] diff --git a/crates/dig-node-service/src/control_client.rs b/crates/dig-node-service/src/control_client.rs index 74d3580..e4e4eed 100644 --- a/crates/dig-node-service/src/control_client.rs +++ b/crates/dig-node-service/src/control_client.rs @@ -47,14 +47,14 @@ pub fn call_control(config: &Config, method: &str, params: Value) -> std::io::Re let rt = tokio::runtime::Builder::new_current_thread() .enable_all() .build()?; - rt.block_on(call_async(&addr, &token, method, params)) + rt.block_on(call_async(addr, &token, method, params)) } /// POST one JSON-RPC control method with the master token; return its `result` (or `{}` when /// the node omits one). A transport failure = the node isn't running; a JSON-RPC `error` = /// the node rejected the call (e.g. a method it does not implement → METHOD_NOT_FOUND). async fn call_async( - addr: &str, + addr: std::net::SocketAddr, token: &str, method: &str, params: Value, diff --git a/crates/dig-node-service/src/open.rs b/crates/dig-node-service/src/open.rs index e6c8b5a..94a7a6e 100644 --- a/crates/dig-node-service/src/open.rs +++ b/crates/dig-node-service/src/open.rs @@ -61,7 +61,7 @@ //! "open" facility as a SINGLE non-shell argv entry. use std::io::{Read, Write}; -use std::net::{TcpListener, TcpStream}; +use std::net::{SocketAddr, TcpListener, TcpStream}; use std::time::{Duration, Instant}; use dig_urn_resolver::images::{self, ErrorImage}; @@ -69,7 +69,7 @@ use dig_urn_resolver::{ResolveError, ResolveOutcome, ResolvedData}; use serde_json::json; use crate::cli::Outcome; -use crate::config::Config; +use crate::config::{Config, DIG_LOCAL_HOST}; /// Shell metacharacters (and quoting/grouping characters) rejected anywhere in the link. The /// launch path never uses a shell, so this is defense-in-depth against the untrusted OS argument @@ -455,19 +455,55 @@ fn candidate_urls(config: &Config, link: &DigLink) -> Vec { if link.root.is_none() { urls.push(format!("http://{}.dig/{}", link.store_id, path)); } - urls.push(serve_url("dig.local", None, &store_ref, path)); - let host = browser_host(config); - urls.push(serve_url(&host, Some(config.port), &store_ref, path)); + urls.push(serve_url( + &UrlAuthority::Named { + name: DIG_LOCAL_HOST, + port: None, + }, + &store_ref, + path, + )); + urls.push(serve_url(&browser_authority(config), &store_ref, path)); urls } -/// Build a node `/s//` serve URL for `host` (with an optional explicit `port`). A -/// store-root (empty path) keeps a trailing slash (the #289 route contract). -fn serve_url(host: &str, port: Option, store_ref: &str, path: &str) -> String { - let authority = match port { - Some(p) => format!("{host}:{p}"), - None => host.to_string(), - }; +/// A URL authority — a `host` or `host:port` — that can only be assembled from TYPED parts. +/// +/// The point of the type is what it makes IMPOSSIBLE: an IPv6 literal can only reach a URL +/// through [`UrlAuthority::Socket`], whose rendering is [`SocketAddr`]'s own `Display`, and that +/// always brackets. Concatenating an `IpAddr` and a port by text (#1682, §5.2) produces +/// `::1:9778` — which is not an authority in the URL grammar — and no amount of care at a call +/// site keeps that out of a signature that accepts `&str`. Here there is no such signature. +enum UrlAuthority { + /// A DNS name with an optional port. A name never needs bracketing, so this arm carries + /// `&'static str`: only names this binary itself knows (`dig.local`, `localhost`) belong + /// here — never operator- or peer-supplied text. + Named { + /// The registered name, e.g. `dig.local`. + name: &'static str, + /// The port, or `None` for the scheme default. + port: Option, + }, + /// An IP address and port. Rendered by [`SocketAddr`], so an IPv6 literal is bracketed. + Socket(SocketAddr), +} + +impl std::fmt::Display for UrlAuthority { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + UrlAuthority::Named { + name, + port: Some(port), + } => write!(f, "{name}:{port}"), + UrlAuthority::Named { name, port: None } => write!(f, "{name}"), + UrlAuthority::Socket(addr) => write!(f, "{addr}"), + } + } +} + +/// Build a node `/s//` serve URL for `authority`. A store-root (empty path) keeps a +/// trailing slash (the #289 route contract). +fn serve_url(authority: &UrlAuthority, store_ref: &str, path: &str) -> String { if path.is_empty() { format!("http://{authority}/s/{store_ref}/") } else { @@ -475,13 +511,16 @@ fn serve_url(host: &str, port: Option, store_ref: &str, path: &str) -> Stri } } -/// The host for the `localhost` tier URL: the operator's explicit `DIG_NODE_HOST` when set -/// (bracketed for IPv6), else `localhost` — friendlier than `127.0.0.1` and equally reachable. -fn browser_host(config: &Config) -> String { +/// The authority for the `localhost` tier URL: the operator's explicit `DIG_NODE_HOST` when set, +/// else the name `localhost` — friendlier than `127.0.0.1` and equally reachable, and the form +/// that follows the resolver to whichever loopback family answers (§5.2). +fn browser_authority(config: &Config) -> UrlAuthority { match config.host { - Some(ip) if ip.is_ipv6() => format!("[{ip}]"), - Some(ip) => ip.to_string(), - None => "localhost".to_string(), + Some(ip) => UrlAuthority::Socket(SocketAddr::new(ip, config.port)), + None => UrlAuthority::Named { + name: "localhost", + port: Some(config.port), + }, } } @@ -600,6 +639,74 @@ mod tests { use std::collections::HashSet; use std::sync::{Arc, Mutex}; + /// **Proves:** the `localhost`-tier candidate URL carries a BRACKETED authority for an IPv6 + /// `DIG_NODE_HOST`, asserted as the exact full URL. + /// + /// **Catches:** the [`UrlAuthority`] typing being unwound back to text concatenation (#1682, + /// §5.2) — an unbracketed `http://::1:9778/s/…` is not a URL a browser can navigate. + /// + /// Honest about what this is: unlike the `bind_addr` case, this was NEVER live. The former + /// `browser_host` already bracketed, so no fixture could have made the old code fail here. It + /// is a REGRESSION PIN on the typed construction, and its load-bearing-ness was proven by + /// deleting the bracketing and watching it fail, not by a red-first run. + /// + /// The global-unicast address is chosen over `::1` deliberately: its own interior `::` is what + /// makes a missing bracket ambiguous rather than merely unusual, and a full-form address also + /// exercises `SocketAddr`'s zero-compression rendering. + #[test] + fn the_localhost_tier_url_brackets_an_ipv6_host() { + let link = DigLink { + store_id: store(), + root: None, + path: "index.html".to_string(), + }; + for (host, expected_authority) in [ + ( + "2001:db8::1".parse().expect("v6 global literal"), + "[2001:db8::1]:9778", + ), + ("::1".parse().expect("v6 loopback literal"), "[::1]:9778"), + ("10.0.0.5".parse().expect("v4 literal"), "10.0.0.5:9778"), + ] { + let config = Config { + host: Some(host), + port: 9778, + ..Config::default() + }; + let urls = candidate_urls(&config, &link); + let expected = format!("http://{expected_authority}/s/{}/index.html", store()); + assert!( + urls.contains(&expected), + "host={host}: expected the exact URL {expected:?} among {urls:?}" + ); + } + } + + /// **Proves:** with no `DIG_NODE_HOST` override the `localhost`-tier URL uses the NAME + /// `localhost`, not a literal address. + /// + /// **Catches:** the default collapsing to `127.0.0.1`, which would pin the URL to one loopback + /// family and lose the §5.2 property that the resolver picks whichever family answers — the + /// exact failure #288 exists to prevent, in URL form. + #[test] + fn the_default_localhost_tier_url_uses_the_name_not_a_literal_address() { + let link = DigLink { + store_id: store(), + root: None, + path: String::new(), + }; + let config = Config { + host: None, + port: 9778, + ..Config::default() + }; + let urls = candidate_urls(&config, &link); + assert!( + urls.contains(&format!("http://localhost:9778/s/{}/", store())), + "expected the localhost NAME form among {urls:?}" + ); + } + fn store() -> String { "a".repeat(64) } diff --git a/crates/dig-node-service/src/server.rs b/crates/dig-node-service/src/server.rs index b6f6169..c0a7e91 100644 --- a/crates/dig-node-service/src/server.rs +++ b/crates/dig-node-service/src/server.rs @@ -402,7 +402,7 @@ pub async fn build_state(config: &Config) -> AppState { .user_agent(concat!("dig-node/", env!("CARGO_PKG_VERSION"))) .build() .expect("dig-node: build http client"), - addr: config.bind_addr(), + addr: config.bind_addr().to_string(), config_path, state_dir, control_token, @@ -1566,7 +1566,7 @@ where Some(v6_addr) => match tokio::net::TcpListener::bind(&v6_addr).await { Ok(l) => Some((v6_addr, l)), Err(e) => { - warn_ipv6_bind_failed(&v6_addr, &e); + warn_ipv6_bind_failed(v6_addr, &e); None } }, @@ -1582,7 +1582,7 @@ where Some((dl_addr, l)) } Err(e) => { - warn_dig_local_bind_failed(&dl_addr, &e); + warn_dig_local_bind_failed(dl_addr, &e); None } }, @@ -1680,7 +1680,7 @@ where /// loopback bind failure is uncommon (most OSes always provide `::1`) but not /// impossible: IPv6 disabled at the kernel/network-stack level, or a sandboxed/ /// restricted environment without it. -fn warn_ipv6_bind_failed(v6_addr: &str, e: &std::io::Error) { +fn warn_ipv6_bind_failed(v6_addr: SocketAddr, e: &std::io::Error) { tracing::warn!( addr = %v6_addr, error = %e, @@ -1695,7 +1695,7 @@ fn warn_ipv6_bind_failed(v6_addr: &str, e: &std::io::Error) { /// continue, never abort") is obvious at the call site. The hint is platform-aware: /// `:80` is privileged on Linux (root / CAP_NET_BIND_SERVICE) and on macOS also /// needs the `127.0.0.2` loopback alias. -fn warn_dig_local_bind_failed(dl_addr: &str, e: &std::io::Error) { +fn warn_dig_local_bind_failed(dl_addr: SocketAddr, e: &std::io::Error) { tracing::warn!( addr = %dl_addr, error = %e, @@ -1748,7 +1748,7 @@ fn bring_up_local_https(config: &Config, app: &Router, shutdown_notify: &Arc, @@ -1773,7 +1773,6 @@ fn spawn_https_listener( Ok(listener) => { tracing::info!(addr = %addr, "HTTPS (https://dig.local) listening"); let shutdown = shutdown_notify.clone(); - let addr = addr.to_string(); tokio::spawn(async move { if let Err(e) = serve_https(listener, rustls_config, app, shutdown).await { tracing::warn!(addr = %addr, error = %e, "HTTPS listener exited"); diff --git a/crates/dig-node-service/src/service.rs b/crates/dig-node-service/src/service.rs index 90bcca1..2ecfc30 100644 --- a/crates/dig-node-service/src/service.rs +++ b/crates/dig-node-service/src/service.rs @@ -57,6 +57,7 @@ use std::cell::Cell; use std::ffi::OsString; use std::io; +use std::net::SocketAddr; use std::path::PathBuf; use std::str::FromStr; use std::time::Duration; @@ -916,7 +917,7 @@ pub fn status(config: &Config) -> io::Result { // A tiny blocking probe with a std TcpStream + manual HTTP keeps `status` free // of an async runtime and an HTTP client dependency in the binary path. A // 2-second connect/read timeout is plenty for loopback. - let (serving, summary) = match probe_health(&addr) { + let (serving, summary) = match probe_health(addr) { Ok(true) => (true, format!("dig-node: SERVING on http://{addr} ({url})")), Ok(false) => ( false, @@ -932,14 +933,15 @@ pub fn status(config: &Config) -> io::Result { }; Ok(Outcome::new( summary, - json!({ "serving": serving, "addr": addr, "health_url": url }), + json!({ "serving": serving, "addr": addr.to_string(), "health_url": url }), )) } /// Minimal blocking HTTP/1.0 `GET /health` probe over loopback. Returns whether /// the response status line is `2xx`. Avoids pulling an async HTTP client into the -/// status path. `addr` is `host:port`. -fn probe_health(addr: &str) -> io::Result { +/// status path. Takes a typed [`SocketAddr`] so an IPv6 target is connected to — and +/// rendered into the probe URL — without the authority ever being spelled by hand (#1682). +fn probe_health(addr: SocketAddr) -> io::Result { use std::io::{Read, Write}; use std::net::TcpStream; @@ -1297,7 +1299,7 @@ SERVICE_NAME: net.dignetwork.dig-node #[test] fn probe_health_false_on_refused_connection() { // 127.0.0.1:1 has nothing listening in the test environment. - assert!(!probe_health("127.0.0.1:1").unwrap()); + assert!(!probe_health(SocketAddr::new(std::net::Ipv4Addr::LOCALHOST.into(), 1)).unwrap()); } #[test]