diff --git a/crates/buzz-acp/src/relay.rs b/crates/buzz-acp/src/relay.rs index c87e25b22..5c0a7dde7 100644 --- a/crates/buzz-acp/src/relay.rs +++ b/crates/buzz-acp/src/relay.rs @@ -3867,17 +3867,19 @@ async fn do_connect( .map_err(|e| RelayError::Http(format!("invalid relay URL: {e}")))?; // Advisory `Buzz-Client` identity so the relay can attribute this - // connection to the harness and its version. Best-effort: an unshipped - // platform or a destination we must not identify to simply connects - // without the header. + // connection to the harness. Best-effort: an unshipped platform or a + // destination we must not identify to simply connects without the header. + // No version is sent — buzz-acp inherits the workspace version, which is + // never bumped (see `RELEASING.md`), so reporting it would pin every + // harness connection to a fixed number forever. let request = { let uri = parsed .as_str() .parse() .map_err(|e| RelayError::Http(format!("invalid relay URL: {e}")))?; let builder = ClientRequestBuilder::new(uri); - match may_identify_to(parsed.as_str()) - .then(|| client_header_value_for_host(ClientApp::Acp, env!("CARGO_PKG_VERSION"))) + match may_identify_to(&parsed) + .then(|| client_header_value_for_host(ClientApp::Acp, None)) .flatten() { Some(value) => builder.with_header(CLIENT_HEADER, value), diff --git a/crates/buzz-core/src/client_identity.rs b/crates/buzz-core/src/client_identity.rs index f185d20a3..b50fd6c15 100644 --- a/crates/buzz-core/src/client_identity.rs +++ b/crates/buzz-core/src/client_identity.rs @@ -9,6 +9,17 @@ //! Buzz-Client: v=1, app=buzz-desktop, platform=macos, app-version="0.5.2" //! ``` //! +//! # Only a real release version may be sent +//! +//! `app_version` is [`Option`] because not every client has a version worth +//! publishing. Only the desktop app and the relay carry independently bumped +//! release versions (`RELEASING.md`, "Version Sources"); crates that inherit +//! `version.workspace = true` sit at a workspace version that has never been +//! bumped, so `env!("CARGO_PKG_VERSION")` would pin them to a fixed number +//! forever and a dashboard would read that as "nobody ever upgrades". Those +//! clients pass `None` and are reported with an `unknown` version instead, +//! which is accurate. A wrong version is worse than no version. +//! //! This module owns only the *sending* half: the canonical header name, the //! app/platform vocabularies, and the serializer. The relay parses the header //! with its own lenient reader (`buzz-relay`'s `client_info`) and treats it as @@ -25,6 +36,8 @@ use std::fmt::Write as _; +use url::{Host, Url}; + /// Canonical header name. Lowercase so it can be used directly as an HTTP/2 /// field name and compared against `http::HeaderName` without reallocating. pub const CLIENT_HEADER: &str = "buzz-client"; @@ -126,57 +139,68 @@ impl ClientPlatform { } } -/// Maximum serialized header length. A Buzz-generated value is far shorter; -/// this only bounds a pathological `app_version` before it reaches the wire. -const MAX_HEADER_LEN: usize = 256; +/// Longest `Buzz-Client` header value either side will handle, in bytes. +/// +/// One constant for both halves of the contract: the sender refuses to emit a +/// longer value and the relay refuses to parse one, so a value that fits on +/// the wire is always one the relay will read. A Buzz-generated header is well +/// under 128 bytes; this only bounds parser work on a hostile input. +pub const MAX_HEADER_LEN: usize = 256; -/// Longest accepted `app-version`, matching the relay's own bound. -const MAX_APP_VERSION_LEN: usize = 32; +/// Longest accepted `app-version`, in bytes. +/// +/// Shared with the sender so both halves of the contract agree on the bound. +pub const MAX_APP_VERSION_LEN: usize = 32; /// Build the `Buzz-Client` header value for this client. /// -/// Returns `None` when the platform is not one Buzz ships, or when -/// `app_version` is empty or not a plausible version string. A missing header -/// is a supported state on the relay, so refusing to send is always safe — -/// callers must never substitute a placeholder. +/// Pass `app_version` only when the client has a real, independently bumped +/// release version; pass `None` otherwise, and the `app-version` member is +/// omitted so the relay reports the version as unknown rather than as a +/// misleading constant. An `app_version` that is not a plausible version +/// string is treated the same as `None`. /// /// # Examples /// /// ``` /// use buzz_core::client_identity::{client_header_value, ClientApp, ClientPlatform}; /// -/// let value = client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, "0.5.2").unwrap(); +/// let value = client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, Some("0.5.2")); /// assert_eq!(value, r#"v=1, app=buzz-desktop, platform=macos, app-version="0.5.2""#); +/// +/// // A client with no real release version still reports app and platform. +/// let value = client_header_value(ClientApp::Cli, ClientPlatform::Linux, None); +/// assert_eq!(value, "v=1, app=buzz-cli, platform=linux"); /// ``` #[must_use] pub fn client_header_value( app: ClientApp, platform: ClientPlatform, - app_version: &str, -) -> Option { - // Only horizontal whitespace is trimmed. Trimming CR/LF would silently - // sanitize a header-injection attempt into an accepted value; those are - // rejected by `is_serializable_app_version` instead. - let app_version = app_version.trim_matches([' ', '\t']); - if !is_serializable_app_version(app_version) { - return None; - } - + app_version: Option<&str>, +) -> String { let mut value = String::with_capacity(64); // Field order matches the wire example and the relay's tests; RFC 8941 // dictionaries are order-independent, so this is presentation only. let _ = write!( value, - "v={CLIENT_HEADER_FORMAT_VERSION}, app={}, platform={}, app-version=\"{app_version}\"", + "v={CLIENT_HEADER_FORMAT_VERSION}, app={}, platform={}", app.as_str(), platform.as_str(), ); - // Unreachable for a validated version, but never emit an oversized header. - if value.len() > MAX_HEADER_LEN { - return None; + // Only horizontal whitespace is trimmed. Trimming CR/LF would silently + // sanitize a header-injection attempt into an accepted value; those are + // rejected by `is_serializable_app_version` instead. + if let Some(app_version) = app_version + .map(|raw| raw.trim_matches([' ', '\t'])) + .filter(|raw| is_serializable_app_version(raw)) + { + let _ = write!(value, ", app-version=\"{app_version}\""); } - Some(value) + + // Unreachable for a validated version, but never emit an oversized header. + debug_assert!(value.len() <= MAX_HEADER_LEN, "{value}"); + value } /// Build the header for the current compile target, or `None` on an unshipped @@ -185,8 +209,12 @@ pub fn client_header_value( /// This is the entry point clients should use; it removes the chance of a /// caller hardcoding the wrong platform for a build. #[must_use] -pub fn client_header_value_for_host(app: ClientApp, app_version: &str) -> Option { - client_header_value(app, ClientPlatform::current()?, app_version) +pub fn client_header_value_for_host(app: ClientApp, app_version: Option<&str>) -> Option { + Some(client_header_value( + app, + ClientPlatform::current()?, + app_version, + )) } /// Whether `url` is a destination this client may identify itself to. @@ -198,51 +226,35 @@ pub fn client_header_value_for_host(app: ClientApp, app_version: &str) -> Option /// origin would leak the header to any on-path observer. Loopback is exempted /// so local development still exercises the same code path. /// +/// Takes a parsed [`Url`] so host extraction — userinfo, IPv6 literals, ports, +/// percent-encoding — is the `url` crate's problem rather than this module's. +/// /// # Examples /// /// ``` /// use buzz_core::client_identity::may_identify_to; +/// use url::Url; /// -/// assert!(may_identify_to("wss://buzz.example.com/")); -/// assert!(may_identify_to("ws://127.0.0.1:8080/")); -/// assert!(!may_identify_to("ws://buzz.example.com/")); +/// assert!(may_identify_to(&Url::parse("wss://buzz.example.com/").unwrap())); +/// assert!(may_identify_to(&Url::parse("ws://127.0.0.1:8080/").unwrap())); +/// assert!(!may_identify_to(&Url::parse("ws://buzz.example.com/").unwrap())); /// ``` #[must_use] -pub fn may_identify_to(url: &str) -> bool { - let Some((scheme, rest)) = url.split_once("://") else { - return false; - }; - match scheme.to_ascii_lowercase().as_str() { +pub fn may_identify_to(url: &Url) -> bool { + match url.scheme() { // TLS: the header is only visible to the relay itself. "wss" | "https" => true, // Cleartext is acceptable only when it cannot leave the machine. - "ws" | "http" => is_loopback_authority(rest), + "ws" | "http" => match url.host() { + Some(Host::Domain(domain)) => domain.eq_ignore_ascii_case("localhost"), + Some(Host::Ipv4(ip)) => ip.is_loopback(), + Some(Host::Ipv6(ip)) => ip.is_loopback(), + None => false, + }, _ => false, } } -/// Whether an authority (`host[:port]`, possibly followed by a path) is -/// loopback. -fn is_loopback_authority(rest: &str) -> bool { - // Trim the path/query/fragment, then any `userinfo@` prefix. - let authority = rest.split(['/', '?', '#']).next().unwrap_or(""); - let authority = authority - .rsplit_once('@') - .map_or(authority, |(_userinfo, host)| host); - let host = match authority.strip_prefix('[') { - // IPv6 literal: `[::1]:port`. - Some(inner) => inner.split(']').next().unwrap_or(""), - None => authority.split(':').next().unwrap_or(""), - }; - if host.eq_ignore_ascii_case("localhost") { - return true; - } - // Parse as an address rather than prefix-matching "127.": a name like - // `127.0.0.1.example.com` shares the prefix but is a remote host. - host.parse::() - .is_ok_and(|ip| ip.is_loopback()) -} - /// Whether `app_version` is safe to place inside an RFC 8941 quoted string. /// /// RFC 8941 quoted strings admit only printable ASCII, and `"`/`\` would need @@ -269,14 +281,27 @@ mod tests { #[test] fn serializes_the_documented_wire_format() { - let value = - client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, "0.5.2").unwrap(); + let value = client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, Some("0.5.2")); assert_eq!( value, r#"v=1, app=buzz-desktop, platform=macos, app-version="0.5.2""# ); } + #[test] + fn omits_app_version_when_the_client_has_no_release_version() { + // buzz-cli and buzz-acp inherit the never-bumped workspace version, so + // they send no version at all rather than a misleading constant. + assert_eq!( + client_header_value(ClientApp::Cli, ClientPlatform::Linux, None), + "v=1, app=buzz-cli, platform=linux" + ); + assert_eq!( + client_header_value(ClientApp::Acp, ClientPlatform::MacOs, None), + "v=1, app=buzz-acp, platform=macos" + ); + } + #[test] fn app_and_platform_tokens_are_stable() { // These strings are Prometheus label values and a cross-repo wire @@ -321,17 +346,20 @@ mod tests { } #[test] - fn rejects_versions_that_are_not_versions() { + fn drops_versions_that_are_not_versions() { + // A placeholder must never be published as a version label; the header + // is still sent so app and platform are not lost. for bad in ["", " ", "unknown", "dev", "v1.2.3", "nightly"] { - assert!( - client_header_value(ClientApp::Cli, ClientPlatform::Linux, bad).is_none(), - "expected {bad:?} to be refused" + assert_eq!( + client_header_value(ClientApp::Cli, ClientPlatform::Linux, Some(bad)), + "v=1, app=buzz-cli, platform=linux", + "expected {bad:?} to be dropped" ); } } #[test] - fn rejects_versions_that_would_break_the_quoted_string() { + fn drops_versions_that_would_break_the_quoted_string() { // A quote or backslash would need RFC 8941 escaping; a newline would // allow header injection. All must be refused, never escaped. for bad in [ @@ -343,37 +371,51 @@ mod tests { "0.5.2\u{7f}", "0.5.2é", ] { - assert!( - client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, bad).is_none(), - "expected {bad:?} to be refused" + let value = client_header_value(ClientApp::Desktop, ClientPlatform::MacOs, Some(bad)); + assert_eq!( + value, "v=1, app=buzz-desktop, platform=macos", + "expected {bad:?} to be dropped" ); } } #[test] - fn rejects_an_absurdly_long_version() { + fn drops_an_absurdly_long_version() { let long = format!("0.{}", "9".repeat(MAX_APP_VERSION_LEN)); assert!(long.len() > MAX_APP_VERSION_LEN); - assert!(client_header_value(ClientApp::Cli, ClientPlatform::Linux, &long).is_none()); + assert_eq!( + client_header_value(ClientApp::Cli, ClientPlatform::Linux, Some(&long)), + "v=1, app=buzz-cli, platform=linux" + ); } #[test] fn accepts_prerelease_and_build_metadata() { - let value = client_header_value(ClientApp::Cli, ClientPlatform::Linux, "1.2.3-rc.1+build9") - .unwrap(); + let value = client_header_value( + ClientApp::Cli, + ClientPlatform::Linux, + Some("1.2.3-rc.1+build9"), + ); assert!(value.ends_with(r#"app-version="1.2.3-rc.1+build9""#)); } #[test] fn trims_surrounding_spaces_and_tabs_before_validating() { - let value = client_header_value(ClientApp::Desktop, ClientPlatform::Windows, " \t0.5.2\t ") - .unwrap(); + let value = client_header_value( + ClientApp::Desktop, + ClientPlatform::Windows, + Some(" \t0.5.2\t "), + ); assert_eq!( value, r#"v=1, app=buzz-desktop, platform=windows, app-version="0.5.2""# ); } + fn url(raw: &str) -> Url { + Url::parse(raw).unwrap_or_else(|e| panic!("{raw:?} is not a URL: {e}")) + } + #[test] fn identifies_only_to_tls_or_loopback_origins() { for allowed in [ @@ -383,11 +425,15 @@ mod tests { "https://buzz.example.com/", // Cleartext loopback cannot leave the machine. "ws://localhost:8080/", + "ws://LOCALHOST:8080/", "ws://127.0.0.1:8080/", "ws://127.3.2.1/", "ws://[::1]:8080/", ] { - assert!(may_identify_to(allowed), "expected {allowed:?} allowed"); + assert!( + may_identify_to(&url(allowed)), + "expected {allowed:?} allowed" + ); } } @@ -403,12 +449,14 @@ mod tests { "ws://127.0.0.1.example.com/", // Loopback in userinfo must not fool the host check. "ws://localhost@evil.example.com/", - // Non-WebSocket and malformed destinations. + // Non-WebSocket destinations. "file:///etc/passwd", - "buzz.example.com", - "", + "ftp://buzz.example.com/", ] { - assert!(!may_identify_to(refused), "expected {refused:?} refused"); + assert!( + !may_identify_to(&url(refused)), + "expected {refused:?} refused" + ); } } @@ -417,8 +465,12 @@ mod tests { // Tests only run on targets Buzz ships, so `current()` is `Some` here. let platform = ClientPlatform::current().expect("test host is a shipped platform"); assert_eq!( - client_header_value_for_host(ClientApp::Cli, "0.1.0"), - client_header_value(ClientApp::Cli, platform, "0.1.0") + client_header_value_for_host(ClientApp::Cli, Some("0.1.0")), + Some(client_header_value(ClientApp::Cli, platform, Some("0.1.0"))) + ); + assert_eq!( + client_header_value_for_host(ClientApp::Cli, None), + Some(client_header_value(ClientApp::Cli, platform, None)) ); } @@ -437,8 +489,10 @@ mod tests { ClientPlatform::Ios, ClientPlatform::Android, ] { - let value = client_header_value(app, platform, "10.20.30-rc.1").unwrap(); - assert!(value.len() <= MAX_HEADER_LEN, "{value}"); + for version in [None, Some("10.20.30-rc.1")] { + let value = client_header_value(app, platform, version); + assert!(value.len() <= MAX_HEADER_LEN, "{value}"); + } } } } diff --git a/crates/buzz-relay/src/client_info.rs b/crates/buzz-relay/src/client_info.rs index 3007e5d5e..7c382a659 100644 --- a/crates/buzz-relay/src/client_info.rs +++ b/crates/buzz-relay/src/client_info.rs @@ -17,14 +17,23 @@ //! is a normal, supported state; a malformed one is counted and discarded. No //! input on this path can ever cause a connection to be rejected. //! -//! # Cardinality is bounded by construction +//! # Cardinality //! -//! Label values are the guard against a forged header exploding Prometheus -//! series. `app` and `platform` are resolved to `&'static str` from closed -//! allowlists — an unrecognized token yields no label, never a passthrough of +//! `app` and `platform` are resolved to `&'static str` from closed allowlists, +//! so an unrecognized token yields no label rather than a passthrough of //! attacker-controlled bytes. `app_version` is narrowed to `MAJOR.MINOR` with -//! both components bounded in length, so the worst case is -//! `apps × platforms × plausible versions`. +//! each component capped at [`MAX_VERSION_COMPONENT_LEN`] digits, which still +//! leaves a large *reachable* label alphabet from a forgeable header. +//! +//! The bound that matters in practice is therefore not the alphabet but the +//! metric kind: this module emits **only a gauge**. The Prometheus recorder is +//! configured with an idle timeout for `MetricKindMask::GAUGE` +//! (`metrics::install`), so a label set that stops being emitted is dropped +//! from the registry. A burst of forged versions costs memory for one idle +//! timeout window and then self-cleans. A counter would have to be retained +//! for the process lifetime, so connection *rate* is deliberately left to the +//! existing `buzz_ws_connections_total` rather than duplicated here with a +//! forgeable label set. //! //! # Why a hand-written parser //! @@ -36,35 +45,27 @@ use axum::http::HeaderMap; use buzz_core::client_identity::{ - ClientApp, ClientPlatform, CLIENT_HEADER, CLIENT_HEADER_FORMAT_VERSION, + ClientApp, ClientPlatform, CLIENT_HEADER, CLIENT_HEADER_FORMAT_VERSION, MAX_APP_VERSION_LEN, + MAX_HEADER_LEN, }; -/// Label value for connections with no usable `Buzz-Client` header. +/// Label value used when a dimension is unknown. /// -/// Emitting an explicit bucket rather than omitting the series is what lets -/// `sum(buzz_client_connections_total)` reconcile with -/// `sum(buzz_ws_connections_total)`, and puts "unidentified" on a dashboard as -/// a visible line instead of a silent gap. +/// Emitting an explicit bucket rather than omitting the series puts +/// "unidentified" on a dashboard as a visible line instead of a silent gap, +/// and lets `sum(buzz_client_connections_active)` reconcile with +/// `buzz_ws_connections_active`. Used both for a wholly unidentified +/// connection and for a client that identified itself but has no real release +/// version to report (`buzz-cli` and `buzz-acp` inherit a workspace version +/// that is never bumped, so they deliberately send no `app-version`). pub const UNKNOWN_LABEL: &str = "unknown"; -/// Longest `Buzz-Client` header the relay will parse, in bytes. -/// -/// A Buzz-generated value is well under 128 bytes. This bounds parser work on -/// a hostile input before any allocation. -const MAX_HEADER_LEN: usize = 512; - /// Longest accepted `MAJOR` or `MINOR` component of `app-version`. /// /// Five digits admits every plausible version while capping the label /// alphabet at 10^5 values per component. const MAX_VERSION_COMPONENT_LEN: usize = 5; -/// Longest raw value retained for logging. -/// -/// Log fields are not Prometheus labels, so they need no allowlist — only a -/// length bound so a hostile header cannot bloat a log line. -const MAX_LOGGED_VALUE_LEN: usize = 32; - /// Why a present `Buzz-Client` header could not be used. /// /// Rendered as a bounded `reason` label on @@ -81,7 +82,7 @@ enum ParseFailure { UnknownApp, /// `platform` was absent or outside the allowlist. UnknownPlatform, - /// `app-version` was absent or not a bounded `MAJOR.MINOR[...]`. + /// `app-version` was present but not a bounded `MAJOR.MINOR[...]`. BadAppVersion, } @@ -109,10 +110,9 @@ pub struct ClientInfo { app: &'static str, /// Allowlisted platform token. platform: &'static str, - /// `MAJOR.MINOR`, safe as a metric label. - app_version: String, - /// Exact version as sent, for logs only — never a label. - app_version_detail: String, + /// `MAJOR.MINOR`, safe as a metric label, or `None` when the client sent + /// no version. + app_version: Option, } impl ClientInfo { @@ -195,14 +195,19 @@ impl ClientInfo { }) .ok_or(ParseFailure::UnknownPlatform)?; - let app_version_raw = app_version.ok_or(ParseFailure::BadAppVersion)?; - let app_version = major_minor(app_version_raw).ok_or(ParseFailure::BadAppVersion)?; + // An absent `app-version` is valid: clients without an independently + // bumped release version omit it rather than send a constant. A + // *present* one that is unusable is still a failure, so a genuinely + // broken version is visible rather than silently downgraded. + let app_version = match app_version { + Some(raw) => Some(major_minor(raw).ok_or(ParseFailure::BadAppVersion)?), + None => None, + }; Ok(Self { app, platform, app_version, - app_version_detail: truncate_for_log(app_version_raw), }) } @@ -218,71 +223,47 @@ impl ClientInfo { self.platform } - /// `MAJOR.MINOR` version, safe as a metric label. + /// `MAJOR.MINOR` version, or [`UNKNOWN_LABEL`] when the client sent none. #[must_use] pub fn app_version(&self) -> &str { - &self.app_version - } - - /// Exact version as sent, for logs only. - #[must_use] - pub fn app_version_detail(&self) -> &str { - &self.app_version_detail + self.app_version.as_deref().unwrap_or(UNKNOWN_LABEL) } } -/// The three label values for a connection, using [`UNKNOWN_LABEL`] when the -/// client did not identify itself. -fn labels(info: Option<&ClientInfo>) -> (&str, &str, &str) { - match info { - Some(info) => (info.app, info.platform, info.app_version.as_str()), - None => (UNKNOWN_LABEL, UNKNOWN_LABEL, UNKNOWN_LABEL), - } -} - -/// Count a newly established connection by client identity. -pub fn record_connection(info: Option<&ClientInfo>) { - let (app, platform, app_version) = labels(info); - metrics::counter!( - "buzz_client_connections_total", - "app" => app.to_owned(), - "platform" => platform.to_owned(), - "app_version" => app_version.to_owned() - ) - .increment(1); -} - -/// Add a live connection to the per-client active gauge. +/// The per-client live-connection gauge, labeled for `info`. /// /// This is a **separate** series from `buzz_ws_connections_active`, which is /// consumed by the HPA as an unlabeled `AverageValue` target. Labeling that /// gauge would shard the series the autoscaler reads; this parallel gauge /// gives the same breakdown without touching scaling behaviour. -pub fn increment_active(info: Option<&ClientInfo>) { - let (app, platform, app_version) = labels(info); +/// +/// A gauge is the only metric kind emitted here, deliberately: the recorder +/// idle-evicts gauge series, so a forged label set cannot accumulate for the +/// process lifetime the way a counter would. +fn active_gauge(info: Option<&ClientInfo>) -> metrics::Gauge { + let (app, platform, app_version) = match info { + Some(info) => (info.app, info.platform, info.app_version()), + None => (UNKNOWN_LABEL, UNKNOWN_LABEL, UNKNOWN_LABEL), + }; metrics::gauge!( "buzz_client_connections_active", "app" => app.to_owned(), "platform" => platform.to_owned(), "app_version" => app_version.to_owned() ) - .increment(1.0); +} + +/// Add a live connection to the per-client active gauge. +pub fn increment_active(info: Option<&ClientInfo>) { + active_gauge(info).increment(1.0); } /// Remove a closed connection from the per-client active gauge. /// /// Must be paired with exactly one [`increment_active`] call, or the gauge -/// drifts. Retired label sets are dropped by the recorder's configured gauge -/// idle timeout rather than going stale. +/// drifts. pub fn decrement_active(info: Option<&ClientInfo>) { - let (app, platform, app_version) = labels(info); - metrics::gauge!( - "buzz_client_connections_active", - "app" => app.to_owned(), - "platform" => platform.to_owned(), - "app_version" => app_version.to_owned() - ) - .decrement(1.0); + active_gauge(info).decrement(1.0); } /// A dictionary member's value, still in its wire form. @@ -408,10 +389,9 @@ fn is_token(candidate: &str) -> bool { /// Narrow a version to `MAJOR.MINOR`, or `None` if it is not one. /// /// Trailing components and pre-release/build metadata are discarded so patch -/// releases do not each create a new time series. The *whole* string is still -/// validated first: the discarded tail is retained for logging, so leaving it -/// unchecked would let a hostile client put arbitrary text in a log field even -/// though the metric label stayed bounded. +/// releases do not each create a new time series. The whole string is +/// validated first so a version that is malformed only in its discarded tail +/// is still reported as a failure rather than silently accepted. fn major_minor(version: &str) -> Option { if !is_plausible_version(version) { return None; @@ -425,10 +405,11 @@ fn major_minor(version: &str) -> Option { /// Whether the full version string looks like a version Buzz produced. /// /// Matches the sender-side rule in `buzz_core::client_identity`: dot-separated -/// alphanumerics with optional `-`/`+` pre-release and build metadata. +/// alphanumerics with optional `-`/`+` pre-release and build metadata, within +/// the shared length bound. fn is_plausible_version(version: &str) -> bool { !version.is_empty() - && version.len() <= MAX_LOGGED_VALUE_LEN + && version.len() <= MAX_APP_VERSION_LEN && version .bytes() .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b'+')) @@ -448,11 +429,6 @@ fn numeric_component(component: &str) -> Option<&str> { Some(component) } -/// Bound a raw value's length for inclusion in a log line. -fn truncate_for_log(value: &str) -> String { - value.chars().take(MAX_LOGGED_VALUE_LEN).collect() -} - #[cfg(test)] mod tests { use super::*; @@ -468,7 +444,17 @@ mod tests { assert_eq!(info.app(), "buzz-desktop"); assert_eq!(info.platform(), "macos"); assert_eq!(info.app_version(), "0.5"); - assert_eq!(info.app_version_detail(), "0.5.2"); + } + + #[test] + fn accepts_a_header_with_no_app_version() { + // buzz-cli and buzz-acp have no independently bumped release version, + // so they omit `app-version` rather than send a misleading constant. + // That must parse, not be counted as a failure. + let info = parse("v=1, app=buzz-cli, platform=linux").unwrap(); + assert_eq!(info.app(), "buzz-cli"); + assert_eq!(info.platform(), "linux"); + assert_eq!(info.app_version(), UNKNOWN_LABEL); } #[test] @@ -488,11 +474,13 @@ mod tests { ClientPlatform::Ios, ClientPlatform::Android, ] { - let raw = client_header_value(app, platform, "1.2.3").expect("builder emits"); - let info = parse(&raw).unwrap_or_else(|e| panic!("{raw:?} rejected as {e:?}")); - assert_eq!(info.app(), app.as_str()); - assert_eq!(info.platform(), platform.as_str()); - assert_eq!(info.app_version(), "1.2"); + for (version, expected) in [(Some("1.2.3"), "1.2"), (None, UNKNOWN_LABEL)] { + let raw = client_header_value(app, platform, version); + let info = parse(&raw).unwrap_or_else(|e| panic!("{raw:?} rejected as {e:?}")); + assert_eq!(info.app(), app.as_str()); + assert_eq!(info.platform(), platform.as_str()); + assert_eq!(info.app_version(), expected); + } } } } @@ -605,8 +593,11 @@ mod tests { parse(r#"v=1, app=buzz-desktop, app-version="1.0.0""#), Err(ParseFailure::UnknownPlatform) ); + // `app-version` is the one optional member — absent is valid, but a + // present-and-broken one is still a failure. + assert!(parse("v=1, app=buzz-desktop, platform=macos").is_ok()); assert_eq!( - parse(r#"v=1, app=buzz-desktop, platform=macos"#), + parse(r#"v=1, app=buzz-desktop, platform=macos, app-version="nope""#), Err(ParseFailure::BadAppVersion) ); } @@ -704,9 +695,20 @@ mod tests { #[test] fn unidentified_connections_get_the_unknown_bucket() { - assert_eq!(labels(None), (UNKNOWN_LABEL, UNKNOWN_LABEL, UNKNOWN_LABEL)); + // An unidentified connection must still produce a series, so + // "unidentified" is a visible dashboard line rather than a silent gap. let info = parse(r#"v=1, app=buzz-cli, platform=linux, app-version="1.0.0""#).unwrap(); - assert_eq!(labels(Some(&info)), ("buzz-cli", "linux", "1.0")); + assert_eq!( + (info.app(), info.platform(), info.app_version()), + ("buzz-cli", "linux", "1.0") + ); + // Gauge helpers accept `None` and label every dimension unknown; they + // are exercised here to prove the label path cannot panic without a + // recorder installed. + increment_active(None); + decrement_active(None); + increment_active(Some(&info)); + decrement_active(Some(&info)); } #[test] @@ -725,13 +727,16 @@ mod tests { } #[test] - fn logged_version_detail_is_length_bounded() { - // `app-version` is bounded by the builder, but the parser must not - // rely on a hostile client honouring that. - let long = "1.2".to_owned() + &".9".repeat(64); - assert_eq!( - truncate_for_log(&long).chars().count(), - MAX_LOGGED_VALUE_LEN + fn the_sender_cannot_emit_a_header_this_parser_would_reject_as_oversized() { + // Both halves share `MAX_HEADER_LEN`, so anything the builder produces + // must fit the parser's bound. A divergence here would silently count + // real clients as parse failures. + let longest = client_header_value( + ClientApp::Desktop, + ClientPlatform::Android, + Some("10000.10000.99999-rc.1+build"), ); + assert!(longest.len() <= MAX_HEADER_LEN, "{longest}"); + assert!(ClientInfo::parse_bytes(longest.as_bytes()).is_ok()); } } diff --git a/crates/buzz-relay/src/connection.rs b/crates/buzz-relay/src/connection.rs index cbe2600b9..27f96b0b5 100644 --- a/crates/buzz-relay/src/connection.rs +++ b/crates/buzz-relay/src/connection.rs @@ -209,7 +209,7 @@ async fn handle_active_connection( addr = %addr, client.app = client.as_ref().map(ClientInfo::app), client.platform = client.as_ref().map(ClientInfo::platform), - client.app_version = client.as_ref().map(ClientInfo::app_version_detail), + client.app_version = client.as_ref().map(ClientInfo::app_version), "WebSocket connection established" ); metrics::counter!( @@ -217,7 +217,6 @@ async fn handle_active_connection( "community" => conn.tenant.host().to_owned() ) .increment(1); - crate::client_info::record_connection(client.as_ref()); let challenge_msg = RelayMessage::auth_challenge(&challenge); if tx diff --git a/crates/buzz-ws-client/src/connection.rs b/crates/buzz-ws-client/src/connection.rs index 6a0f160f5..3928046f5 100644 --- a/crates/buzz-ws-client/src/connection.rs +++ b/crates/buzz-ws-client/src/connection.rs @@ -11,6 +11,7 @@ use tokio::time::timeout; use tokio_tungstenite::tungstenite::client::ClientRequestBuilder; use tokio_tungstenite::{connect_async, tungstenite::Message, MaybeTlsStream, WebSocketStream}; use tracing::debug; +use url::Url; use crate::error::WsClientError; use crate::message::{build_auth_event, parse_relay_message, OkResponse, RelayMessage}; @@ -21,18 +22,18 @@ type WsStream = WebSocketStream>; /// header when the destination permits it. /// /// Attaching the header is never allowed to fail a connection: an unshipped -/// platform, an unusable version, or a destination outside -/// [`may_identify_to`] simply yields a request without the header. +/// platform or a destination outside [`may_identify_to`] simply yields a +/// request without the header. fn client_request( - url: &str, + url: &Url, app: ClientApp, - app_version: &str, + app_version: Option<&str>, ) -> Result { - let uri = url - .parse() - .map_err(|e: tokio_tungstenite::tungstenite::http::uri::InvalidUri| { + let uri = url.as_str().parse().map_err( + |e: tokio_tungstenite::tungstenite::http::uri::InvalidUri| { WsClientError::Url(e.to_string()) - })?; + }, + )?; let builder = ClientRequestBuilder::new(uri); if !may_identify_to(url) { return Ok(builder); @@ -75,28 +76,38 @@ impl NostrWsConnection { } /// Connects to the relay at `url` without performing authentication. + /// + /// Identifies as [`ClientApp::Cli`], which covers the `buzz` CLI and the + /// test client. No version is sent: both inherit the workspace version, + /// which has never been bumped (see `RELEASING.md`), so reporting it would + /// pin every CLI connection to a fixed number forever. The relay records + /// the version as unknown, which is accurate. pub async fn connect(url: &str) -> Result { - Self::connect_as(url, ClientApp::Cli, env!("CARGO_PKG_VERSION")).await + Self::connect_as(url, ClientApp::Cli, None).await } - /// Connects to the relay at `url`, identifying as `app` version - /// `app_version` in the advisory `Buzz-Client` header. + /// Connects to the relay at `url`, identifying as `app` in the advisory + /// `Buzz-Client` header. + /// + /// Pass `app_version` only for a client with a real, independently bumped + /// release version; pass `None` otherwise so the relay reports the version + /// as unknown rather than as a misleading constant. /// /// The header lets the relay report which client versions and platforms /// its live connections come from. It is best-effort: if it cannot be - /// built (unshipped platform, unusable version) or the destination is not - /// one this client may identify itself to, the connection proceeds without - /// it and the relay counts it as unidentified. + /// built (unshipped platform) or the destination is not one this client may + /// identify itself to, the connection proceeds without it and the relay + /// counts it as unidentified. pub async fn connect_as( url: &str, app: ClientApp, - app_version: &str, + app_version: Option<&str>, ) -> Result { let parsed = url - .parse::() + .parse::() .map_err(|e| WsClientError::Url(e.to_string()))?; - let request = client_request(parsed.as_str(), app, app_version)?; + let request = client_request(&parsed, app, app_version)?; let (ws, _response) = connect_async(request) .await .map_err(WsClientError::WebSocket)?; diff --git a/desktop/src-tauri/src/native_websocket.rs b/desktop/src-tauri/src/native_websocket.rs index 3b94f4015..20daa6e1e 100644 --- a/desktop/src-tauri/src/native_websocket.rs +++ b/desktop/src-tauri/src/native_websocket.rs @@ -13,6 +13,7 @@ use tokio_tungstenite::{ tungstenite::protocol::{frame::coding::CloseCode, CloseFrame, Message}, }; use tokio_util::sync::CancellationToken; +use url::Url; const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); const WRITE_TIMEOUT: Duration = Duration::from_secs(10); @@ -24,13 +25,19 @@ const SEND_QUEUE_CAPACITY: usize = 64; /// /// The header tells the relay which app, platform, and version a live /// connection belongs to. Attaching it never fails a connection: a destination -/// outside `may_identify_to` or an unusable version just omits it, and the -/// relay counts the connection as unidentified. +/// outside `may_identify_to` or an unparseable URL just omits the header, and +/// the relay counts the connection as unidentified. +/// +/// The desktop app is one of the two lanes with an independently bumped +/// release version (`RELEASING.md`), so `CARGO_PKG_VERSION` is a real version +/// here — `bump-desktop-version` rewrites it. Clients that inherit the +/// workspace version deliberately send none. fn client_request(url: &str) -> Result { let uri = url.parse().map_err(|error| format!("{error}"))?; let builder = ClientRequestBuilder::new(uri); - let Some(value) = may_identify_to(url) - .then(|| client_header_value_for_host(ClientApp::Desktop, env!("CARGO_PKG_VERSION"))) + let permitted = Url::parse(url).is_ok_and(|parsed| may_identify_to(&parsed)); + let Some(value) = permitted + .then(|| client_header_value_for_host(ClientApp::Desktop, Some(env!("CARGO_PKG_VERSION")))) .flatten() else { return Ok(builder);