feat(mobile): identify clients to Buzz servers - #2596
Conversation
Co-authored-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz> Signed-off-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz>
Co-authored-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz> Signed-off-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz>
|
@codex review |
|
@builderbot review |
|
To use Codex here, create a Codex account and connect to github. |
square-tomb
left a comment
There was a problem hiding this comment.
🤖 Independent review (round 1, agent load-bearing). I read the full diff against merge-base 4253688.
Overall this matches the design ground truth well: RFC 8941 structured Buzz-Client + coarse User-Agent, exactly the specified fields, none of the excluded ones, first-party origin gating on HTTP and WS upgrades, no changes to signed Nostr events, and lenient advisory parsing on the relay with good test coverage on both sides. The media-auth same-origin tightening is a genuine improvement and is tested.
Two issues I'd like addressed, plus two smaller notes as inline comments:
- Startup crash risk (mobile):
loadClientHeaders()runs inmain()beforerunAppand can throw on unexpected platform metadata; an advisory identification feature should never be able to prevent app launch. Details inline onmain.dart. - Unbounded metric label cardinality (relay):
app_versionis attacker-controlled free text and is used as a Prometheus label. Details inline onclient_info.rs.
Naming nit, no change requested: buzz_client_requests_total is incremented once per established WebSocket connection, not per request; buzz_client_connections_total would describe it better if you touch this file again.
Co-authored-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz> Signed-off-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz>
Co-authored-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz> Signed-off-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz>
square-tomb
left a comment
There was a problem hiding this comment.
🤖 Round 2 review (agent load-bearing) at head b5d0670. I re-read the full merge-base diff plus the b5d0670 fix commit. All four round-1 findings are properly addressed, and the User-Agent scope change is consistently implemented. Approving.
Verification of round-1 findings:
- Startup crash — fixed.
main()now loadsSharedPreferencesfirst, wrapsloadClientHeaders()in try/catch, and falls back toClientHeaders.emptywith a debug log. Launch can no longer fail on unexpected platform metadata. (The Settings version label goes blank on fallback sinceappVersionrides the same object; acceptable degradation, just noting the trade-off was implicitly accepted.) - Metric cardinality — fixed.
normalize_app_versionenforces numericmajor.minor[.patch]with components capped at 5 digits, buckets the label tomajor.minor, and shape violations count as parse failures. The mobile side enforces the matching_isValidAppVersioncontract at build time, and tests cover both12.34.56 → 12.34bucketing and rejection cases includingrandom-connection-valueand123456.2. - Pairing
relayUrlasymmetry — clarifying comments added at both pairing socket call sites explaining the payload origin is untrusted at that point. - Dead
OptionalFromRequestPartsimpl — removed;from_headersis now the sole entry point. The metric rename tobuzz_client_connections_totalalso resolves my naming nit.
Scope change (UA to all origins): clientHeadersForUrl now returns userAgentValue for valid non-first-party http/ws/https/wss URLs, full pairs for first-party, and nothing for malformed or non-network URLs. MediaGetAuthService.headersFor keeps Buzz-Client and Authorization strictly same-origin /media/ while allowing UA elsewhere; StateError on partially-initialized headers covers both views. Tests updated accordingly (Cloudflare third-party case, lookalike hosts, file:// rejection).
One non-blocking observation, no change requested: the normalized app_version label space is bounded but still large in theory (up to 5 digits per component). Connection rate limiting bounds the mint rate, and the parse contract bounds the shape, so I consider this acceptable for an advisory metric; if series churn ever shows up in Prometheus, tightening MAX_APP_VERSION_COMPONENT_LENGTH to 3 is the cheap knob.
Design conformance re-check at the new head: exact field set, excluded fields still absent, Buzz-Client first-party-only on HTTP + WS upgrades, coarse UA everywhere per the updated ground truth, signed Nostr events untouched, relay parsing advisory and lenient. No new findings.
square-tomb
left a comment
There was a problem hiding this comment.
🤖 Independent review (goblin) at head b5d0670 — APPROVE, no blocking findings.
What I verified against the design ground truth:
- Header contents/shape:
Buzz-Clientis a valid RFC 8941 dictionary (v=1, app=buzz-mobile, platform=…, app-version="…", app-build="…", os-version="…"[, os-api=N]) with correct string escaping (_structuredStringescapes"/\, rejects non-visible-ASCII); coarseUser-AgentisBuzz/<version> (<platform>; build <n>)with token/comment validation. Only the agreed fields are present; no device model, install IDs, timezone, network type, or bundle ID anywhere in the diff. - Gating:
clientHeadersForUrlsends the full pair only to the configured relay origin (strict same-origin with ws≡http/wss≡https scheme mapping and effective ports),pairing.buzz.xyz,*.communities.buzz.xyz(secure schemes only), or localhost in debug; everything else valid http(s)/ws(s) getsUser-Agentonly; malformed/non-network URLs get nothing. Tests cover the Cloudflare third-party case, thecommunities.buzz.xyz.evil.examplelookalike,ws://pairing.buzz.xyz(insecure scheme rejected), and port mismatch. - UA-on-all-origins coverage: traced every outbound network path in
mobile/lib— relay WS (RelaySocket→IOWebSocketChannelwith headers),/queryPOST, invite claim POST, media upload (incl. legacy-path retry, same request builder), media image fetches (MediaImageProvider→auth.headersFor), video playback (VideoPlayerController.networkUrlwithmediaGetHeadersForContext), pairing WS + credential probe. All now inject identification headers. Signed Nostr events are untouched (Event.frominputs unchanged). - Relay parsing: advisory and lenient — missing header returns
Nonewith no metric; invalid header incrementsbuzz_client_header_parse_failures_totaland can never reject the upgrade. Metric labels are bounded: fixedapp/platformvocabularies andapp_versionnormalized to validatedmajor.minor(≤5-digit components), enforced symmetrically client-side in_isValidAppVersion. Span/log fields are low-risk (Option<&str>s on the connection span). - Startup safety:
main()catches allloadClientHeaders()failures and falls back toClientHeaders.empty, whosevalues/userAgentValuereturn{}— no headers, no crash. Settings version label degrades to hidden in that state. - Ran the new tests locally at b5d0670:
cargo test -p buzz-relay client_info→ 4/4 pass;flutter testonclient_headers_test.dart,media_image_test.dart,pairing_socket_test.dart→ 33/33 pass.
Three non-blocking inline notes below (invite relayUrl trust, a malformed-baseUrl edge in media_auth, duplicate-header handling in the relay parser). None warrant holding the PR.
— submitted autonomously by agent goblin via the operator account
Co-authored-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz> Signed-off-by: npub1ux8n2yfs8qfvgd75s7kyhar2mztac355v6vmrz4juc9l3msw4pgstums9e <e18f3511303812c437d487ac4bf46ad897dc46946699b18ab2e60bf8ee0ea851@buzz.block.builderlab.xyz>
|
Closing as non-urgent, may revive someday. |
What changed?
Buzz mobile now identifies itself with two headers:
Buzz-Client(RFC 8941)v=1, app=buzz-mobile, platform=android, app-version="0.4.5", app-build="6", os-version="15", os-api=35User-AgentBuzz/0.4.5 (android; build 6)On the relay, a new advisory parser reads
Buzz-Clientleniently: a missing header is a normal state, an invalid one is counted and ignored, and parsing can never reject a request. Parsed fields are attached to the WebSocket connection span and log line and feed a low-cardinality per-client-version counter.Two small adjacent cleanups: the settings page version label now reads the cached client metadata instead of an async package-info lookup, and media auth's relay URL matching was tightened to strict same-origin (scheme, host, effective port).
Why?
Buzz-owned servers previously could not tell which app version or platform a connection came from, which made support triage, rollout monitoring, and deprecation decisions guesswork.
The header set is intentionally minimal for privacy: no device model, install IDs, timezone, network type, or raw bundle ID, and structured metadata is never sent to third parties or embedded in signed events.
How is it tested?
Build and run. Full mobile Flutter suite and full
buzz-relayRust suite. Mobile formatting and static analysis, plus Rust formatting and Clippy.Added tests:
client_headers_test.dartclient_infounit tests (relay parsing and metrics)Before/after images are not applicable because the change affects network metadata and relay observability without changing rendered UI.