diff --git a/matrix/src/main.rs b/matrix/src/main.rs index 5654584..e36d643 100644 --- a/matrix/src/main.rs +++ b/matrix/src/main.rs @@ -25,7 +25,7 @@ use anyhow::Result; #[cfg(not(test))] use clap::Parser; use tokio::time::Duration; -use tracing::{error, info}; +use tracing::{error, info, warn}; #[cfg(not(test))] use crate::cli::Cli; @@ -37,6 +37,14 @@ use crate::potatomesh::{FetchParams, PotatoClient, PotatoMessage, PotatoNode}; #[cfg(not(test))] use tokio::time::sleep; +/// Consecutive poll attempts a single message may fail before it is skipped +/// (advanced past, with a warning) so it cannot block every message queued +/// behind it forever. Transient failures (a brief homeserver/API hiccup) +/// recover well within this bound; a message still failing after this many +/// polls is treated as poison and dropped — a far better outcome than silently +/// losing everything queued behind it. +const MAX_FORWARD_ATTEMPTS: u32 = 5; + #[derive(Debug, serde::Serialize, serde::Deserialize, Default)] pub struct BridgeState { /// Highest message id processed by the bridge. @@ -50,6 +58,14 @@ pub struct BridgeState { /// Legacy checkpoint timestamp used before last_rx_time was added. #[serde(default, skip_serializing)] last_checked_at: Option, + /// Id of the message currently blocking the batch, and how many consecutive + /// polls it has failed to forward. In-memory only (never persisted — a + /// restart is itself a fresh attempt); used to skip a poison message after + /// [`MAX_FORWARD_ATTEMPTS`] instead of retrying it forever. + #[serde(skip)] + failing_msg_id: Option, + #[serde(skip)] + failing_msg_attempts: u32, } impl BridgeState { @@ -180,7 +196,46 @@ async fn poll_once( if let Err(e) = handle_message(potato, matrix, state, msg).await { error!("Error handling message {}: {:?}", msg.id, e); - continue; + // Track consecutive failures of THIS specific message across + // polls (the batch is refetched each poll while the + // watermark is stuck, so the same id reappears at the head). + if state.failing_msg_id == Some(msg.id) { + state.failing_msg_attempts += 1; + } else { + state.failing_msg_id = Some(msg.id); + state.failing_msg_attempts = 1; + } + + if state.failing_msg_attempts >= MAX_FORWARD_ATTEMPTS { + // Poison message: it has failed too many polls in a row, + // so skip it rather than block the whole batch behind it + // indefinitely. Advance the watermark past it (as if + // processed) and continue with the rest. Dropping this + // one message is the lesser evil versus stalling forever. + warn!( + "Skipping message {} after {} failed forward attempts; advancing past it", + msg.id, state.failing_msg_attempts + ); + state.failing_msg_id = None; + state.failing_msg_attempts = 0; + state.update_with(msg); + persist_state(state, state_path); + continue; + } + + // Below the skip threshold: stop the batch here. The failed + // message and everything after it stay uncommitted and are + // retried, in order, on the next poll — so a *transient* + // failure never loses, reorders, or duplicates anything. (The + // watermark advances only on success, so a later success can + // never jump past this failure — the silent-loss bug.) + break; + } + + // Success clears any failure tracking for this message. + if state.failing_msg_id == Some(msg.id) { + state.failing_msg_id = None; + state.failing_msg_attempts = 0; } // persist after each processed message @@ -217,7 +272,13 @@ async fn main() -> Result<()> { let cfg = config::load(cli.to_inputs())?; log_config(&cfg); - let http = reqwest::Client::builder().build()?; + // Bound every HTTP request so a hung homeserver or PotatoMesh API cannot + // stall the single-threaded poll loop indefinitely. `timeout` caps the + // whole request/response; `connect_timeout` caps TCP/TLS establishment. + let http = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .connect_timeout(Duration::from_secs(10)) + .build()?; let potato = PotatoClient::new(http.clone(), cfg.potatomesh.clone()); potato.health_check().await?; let matrix = MatrixAppserviceClient::new(http.clone(), cfg.matrix.clone()); @@ -462,6 +523,7 @@ mod tests { last_rx_time: None, last_rx_time_ids: vec![], last_checked_at: None, + ..Default::default() }; let older = sample_msg(9); let newer = sample_msg(11); @@ -506,6 +568,7 @@ mod tests { last_rx_time: Some(99), last_rx_time_ids: vec![123], last_checked_at: Some(77), + ..Default::default() }; state.save(path_str).unwrap(); @@ -568,6 +631,7 @@ mod tests { last_rx_time: Some(123), last_rx_time_ids: vec![], last_checked_at: None, + ..Default::default() }; let params = build_fetch_params(&state); @@ -582,6 +646,7 @@ mod tests { last_rx_time: Some(123), last_rx_time_ids: vec![], last_checked_at: None, + ..Default::default() }; let params = build_fetch_params(&state); @@ -596,6 +661,7 @@ mod tests { last_rx_time: None, last_rx_time_ids: vec![], last_checked_at: None, + ..Default::default() }; let params = build_fetch_params(&state); @@ -620,6 +686,7 @@ mod tests { last_rx_time: Some(123), last_rx_time_ids: vec![42], last_checked_at: None, + ..Default::default() }; persist_state(&state, path_str); @@ -690,6 +757,7 @@ mod tests { last_rx_time: Some(100), last_rx_time_ids: vec![1], last_checked_at: None, + ..Default::default() }; poll_once(&potato, &matrix, &mut state, state_str).await; @@ -746,6 +814,247 @@ mod tests { assert_eq!(loaded.last_rx_time_ids, vec![1]); } + /// Regression test for the watermark-advance bug: within a single batch, + /// an *earlier* message (lower `rx_time`) that fails to forward must not be + /// silently skipped forever just because a *later* message would succeed. + /// + /// The batch is `[A(rx_time=10, fails), B(rx_time=20, would succeed)]`. + /// `A` fails because its node lookup returns 500; `B`'s full forward chain + /// is mocked to succeed. With the fix, `poll_once` stops at the first + /// failure, so the watermark never advances past `A` and `A` stays eligible + /// for retry. Under the old code, `B` succeeded and pushed `last_rx_time` to + /// 20, permanently stranding `A` — which this test would catch. + #[tokio::test] + async fn poll_once_does_not_advance_watermark_past_failed_message() { + let tmp_dir = tempfile::tempdir().unwrap(); + let state_path = tmp_dir.path().join("state.json"); + let state_str = state_path.to_str().unwrap(); + + let mut server = mockito::Server::new_async().await; + + // Batch of two TEXT messages; poll_once sorts ascending by rx_time so A + // (rx_time=10) is processed before B (rx_time=20) regardless of order. + let mock_msgs = server + .mock("GET", "/api/messages") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"[ + {"id":1,"rx_time":10,"rx_iso":"2025-11-27T00:00:00Z","from_id":"!aaaaaaaa","to_id":"^all","channel":1,"portnum":"TEXT_MESSAGE_APP","text":"Ping","lora_freq":868,"modem_preset":"MediumFast","channel_name":"TEST","node_id":"!aaaaaaaa"}, + {"id":2,"rx_time":20,"rx_iso":"2025-11-27T00:00:00Z","from_id":"!bbbbbbbb","to_id":"^all","channel":1,"portnum":"TEXT_MESSAGE_APP","text":"Ping","lora_freq":868,"modem_preset":"MediumFast","channel_name":"TEST","node_id":"!bbbbbbbb"} + ]"#, + ) + .create(); + + // Earlier message A fails: its node lookup returns 500. + let mock_node_a = server + .mock("GET", "/api/nodes/aaaaaaaa") + .match_query(mockito::Matcher::Any) + .with_status(500) + .create(); + + // Later message B would fully succeed (node + register + join + + // displayname + send). These stay unused under the fix, but make the + // test a true regression: under the buggy code B forwards and advances + // the watermark to 20. + let _mock_node_b = server + .mock("GET", "/api/nodes/bbbbbbbb") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"node_id":"!bbbbbbbb","long_name":"Node B","short_name":"NB"}"#) + .create(); + let _mock_register = server + .mock("POST", "/_matrix/client/v3/register") + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_join = server + .mock( + "POST", + mockito::Matcher::Regex(r"/_matrix/client/v3/rooms/.+/join".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_display = server + .mock( + "PUT", + mockito::Matcher::Regex(r"/_matrix/client/v3/profile/.+/displayname".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_send = server + .mock( + "PUT", + mockito::Matcher::Regex(r"/_matrix/client/v3/rooms/.+/send/.+".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + + let http_client = reqwest::Client::new(); + let potatomesh_cfg = PotatomeshConfig { + base_url: server.url(), + poll_interval_secs: 1, + }; + let matrix_cfg = MatrixConfig { + homeserver: server.url(), + as_token: "AS_TOKEN".to_string(), + hs_token: "HS_TOKEN".to_string(), + server_name: "example.org".to_string(), + room_id: "!roomid:example.org".to_string(), + }; + + let potato = PotatoClient::new(http_client.clone(), potatomesh_cfg); + let matrix = MatrixAppserviceClient::new(http_client, matrix_cfg); + let mut state = BridgeState::default(); + + poll_once(&potato, &matrix, &mut state, state_str).await; + + // A's node lookup was attempted and failed. + mock_msgs.assert(); + mock_node_a.assert(); + + // The watermark must NOT have advanced past the failed message, so A + // remains eligible to be refetched and retried on the next poll. + let msg_a = PotatoMessage { + id: 1, + rx_time: 10, + node_id: "!aaaaaaaa".to_string(), + ..sample_msg(1) + }; + assert_eq!( + state.last_rx_time, None, + "watermark advanced despite a failed earlier message" + ); + assert_ne!( + state.last_rx_time, + Some(20), + "watermark jumped past the failed message to the later success" + ); + assert!( + state.should_forward(&msg_a), + "failed message must remain eligible for retry" + ); + } + + #[tokio::test] + async fn poll_once_skips_poison_message_after_max_attempts() { + // A permanently-failing message must not block the batch forever. A + // (rx_time=10) always fails (node lookup 500); B (rx_time=20) succeeds. + // After MAX_FORWARD_ATTEMPTS polls the bridge skips A (advances past it) + // and then forwards B, so the watermark reaches 20 — progress is made + // while earlier polls still refused to jump past the failure. + let tmp_dir = tempfile::tempdir().unwrap(); + let state_path = tmp_dir.path().join("state.json"); + let state_str = state_path.to_str().unwrap(); + + let mut server = mockito::Server::new_async().await; + + let _mock_msgs = server + .mock("GET", "/api/messages") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"[ + {"id":1,"rx_time":10,"rx_iso":"2025-11-27T00:00:00Z","from_id":"!aaaaaaaa","to_id":"^all","channel":1,"portnum":"TEXT_MESSAGE_APP","text":"Ping","lora_freq":868,"modem_preset":"MediumFast","channel_name":"TEST","node_id":"!aaaaaaaa"}, + {"id":2,"rx_time":20,"rx_iso":"2025-11-27T00:00:00Z","from_id":"!bbbbbbbb","to_id":"^all","channel":1,"portnum":"TEXT_MESSAGE_APP","text":"Ping","lora_freq":868,"modem_preset":"MediumFast","channel_name":"TEST","node_id":"!bbbbbbbb"} + ]"#, + ) + .create(); + + // A always fails; B's full forward chain always succeeds. + let _mock_node_a = server + .mock("GET", "/api/nodes/aaaaaaaa") + .match_query(mockito::Matcher::Any) + .with_status(500) + .create(); + let _mock_node_b = server + .mock("GET", "/api/nodes/bbbbbbbb") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"node_id":"!bbbbbbbb","long_name":"Node B","short_name":"NB"}"#) + .create(); + let _mock_register = server + .mock("POST", "/_matrix/client/v3/register") + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_join = server + .mock( + "POST", + mockito::Matcher::Regex(r"/_matrix/client/v3/rooms/.+/join".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_display = server + .mock( + "PUT", + mockito::Matcher::Regex(r"/_matrix/client/v3/profile/.+/displayname".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + let _mock_send = server + .mock( + "PUT", + mockito::Matcher::Regex(r"/_matrix/client/v3/rooms/.+/send/.+".to_string()), + ) + .match_query(mockito::Matcher::Any) + .with_status(200) + .create(); + + let http_client = reqwest::Client::new(); + let potato = PotatoClient::new( + http_client.clone(), + PotatomeshConfig { + base_url: server.url(), + poll_interval_secs: 1, + }, + ); + let matrix = MatrixAppserviceClient::new( + http_client, + MatrixConfig { + homeserver: server.url(), + as_token: "AS_TOKEN".to_string(), + hs_token: "HS_TOKEN".to_string(), + server_name: "example.org".to_string(), + room_id: "!roomid:example.org".to_string(), + }, + ); + let mut state = BridgeState::default(); + + // Poll up to (and including) the skip threshold. + for _ in 0..MAX_FORWARD_ATTEMPTS { + poll_once(&potato, &matrix, &mut state, state_str).await; + } + + // A was skipped on the final poll and B then forwarded, so the watermark + // advanced to B's rx_time. A is no longer eligible (it is behind the + // watermark), and the failure tracker was cleared. + assert_eq!( + state.last_rx_time, + Some(20), + "poison message should have been skipped and the later message forwarded" + ); + let msg_a = PotatoMessage { + id: 1, + rx_time: 10, + node_id: "!aaaaaaaa".to_string(), + ..sample_msg(1) + }; + assert!( + !state.should_forward(&msg_a), + "skipped message must not be reprocessed" + ); + } + /// Drive `handle_message` end-to-end against a mocked Matrix homeserver /// and PotatoMesh API, asserting that the bridged message body carries /// the expected protocol tag and preset abbreviation. Shared by the diff --git a/matrix/src/matrix.rs b/matrix/src/matrix.rs index c52716a..6ed2faa 100644 --- a/matrix/src/matrix.rs +++ b/matrix/src/matrix.rs @@ -95,8 +95,26 @@ impl MatrixAppserviceClient { if resp.status().is_success() { Ok(()) } else { - // If user already exists, Synapse / HS usually returns 400 M_USER_IN_USE. - // We'll just ignore non-success and hope it's that case. + // If the puppet already exists, Synapse / HS returns 400 M_USER_IN_USE, + // which is expected and safely ignored. Anything else (e.g. 401/403 from + // a misconfigured `as_token`) is surfaced with the status and the Matrix + // error body so the failure is diagnosable instead of being swallowed + // here and only manifesting as a downstream error. + let status = resp.status(); + let body_snip = resp.text().await.unwrap_or_default(); + // Only the specific M_USER_IN_USE errcode means "puppet already + // exists" -- keying off the bare 400 status would also swallow real + // failures returned as 400 (malformed request, config issues) and + // skip the diagnostic warning below. + let already_registered = body_snip.contains("M_USER_IN_USE"); + if !already_registered { + tracing::warn!( + "Unexpected response registering puppet user {}: status {}, body: {}", + localpart, + status, + body_snip + ); + } Ok(()) } } @@ -338,7 +356,55 @@ mod tests { .mock("POST", "/_matrix/client/v3/register") .match_query("kind=user") .match_header("authorization", "Bearer AS_TOKEN") - .with_status(400) // M_USER_IN_USE + .with_status(400) + .with_body(r#"{"errcode":"M_USER_IN_USE","error":"User ID already taken."}"#) + .create(); + + let mut cfg = dummy_cfg(); + cfg.homeserver = server.url(); + let client = MatrixAppserviceClient::new(reqwest::Client::new(), cfg); + let result = client.ensure_user_registered("testuser").await; + + mock.assert(); + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_ensure_user_registered_other_400_is_not_treated_as_in_use() { + // A 400 that is NOT M_USER_IN_USE (e.g. a malformed request) must reach + // the warn branch rather than being silently ignored as "already + // registered". The call still returns Ok(()) so registration is + // non-fatal, but the failure is surfaced for diagnosis. + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/_matrix/client/v3/register") + .match_query("kind=user") + .match_header("authorization", "Bearer AS_TOKEN") + .with_status(400) + .with_body(r#"{"errcode":"M_INVALID_PARAM","error":"bad request"}"#) + .create(); + + let mut cfg = dummy_cfg(); + cfg.homeserver = server.url(); + let client = MatrixAppserviceClient::new(reqwest::Client::new(), cfg); + let result = client.ensure_user_registered("testuser").await; + + mock.assert(); + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_ensure_user_registered_unexpected_status_logs_and_is_ok() { + // A non-400 failure (e.g. 403 from a misconfigured as_token) is NOT the + // expected "already registered" case, so it exercises the warn branch. + // The call still returns Ok(()) so registration remains non-fatal. + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/_matrix/client/v3/register") + .match_query("kind=user") + .match_header("authorization", "Bearer AS_TOKEN") + .with_status(403) + .with_body(r#"{"errcode":"M_FORBIDDEN","error":"bad token"}"#) .create(); let mut cfg = dummy_cfg(); diff --git a/web/lib/potato_mesh/application/federation/instance_fetcher.rb b/web/lib/potato_mesh/application/federation/instance_fetcher.rb index d188f63..82a9aba 100644 --- a/web/lib/potato_mesh/application/federation/instance_fetcher.rb +++ b/web/lib/potato_mesh/application/federation/instance_fetcher.rb @@ -78,13 +78,30 @@ module PotatoMesh Timeout.timeout(PotatoMesh::Config.remote_instance_request_timeout) do http.start do |connection| request = build_federation_http_request(Net::HTTP::Get, uri) - response = connection.request(request) - case response - when Net::HTTPSuccess - response.body - else - raise InstanceHttpResponseError, "unexpected response #{response.code}" + # Stream the response with the block form of +request+ so the size + # cap (mirroring the inbound +read_json_body+ ceiling) is enforced + # *incrementally*. The non-block form buffers the whole body into + # memory before we could inspect it, so a malicious/oversized peer + # could still cause a large allocation before we raise. Reading in + # chunks lets us abort as soon as the limit is exceeded. + max_bytes = PotatoMesh::Config.remote_instance_max_response_bytes + body = nil + connection.request(request) do |response| + unless response.is_a?(Net::HTTPSuccess) + raise InstanceHttpResponseError, "unexpected response #{response.code}" + end + + buffer = +"" + response.read_body do |chunk| + buffer << chunk + if buffer.bytesize > max_bytes + raise InstanceHttpResponseError, + "response exceeds maximum size of #{max_bytes} bytes" + end + end + body = buffer end + body end end rescue InstanceHttpResponseError diff --git a/web/lib/potato_mesh/application/routes/ingest.rb b/web/lib/potato_mesh/application/routes/ingest.rb index 5cb6201..adb7039 100644 --- a/web/lib/potato_mesh/application/routes/ingest.rb +++ b/web/lib/potato_mesh/application/routes/ingest.rb @@ -113,6 +113,13 @@ module PotatoMesh end app.post "/api/instances" do + # Reject federation registrations outright when federation is + # disabled (mirrors the GET /api/instances guard in api.rb) so a + # PRIVATE=1 or FEDERATION=0 deployment never performs outbound + # federation fetches or writes in response to an unsolicited, + # signed announcement. + halt 404 unless federation_enabled? + content_type :json begin payload = JSON.parse(read_json_body) diff --git a/web/lib/potato_mesh/config.rb b/web/lib/potato_mesh/config.rb index ca6c9d9..23f9ed4 100644 --- a/web/lib/potato_mesh/config.rb +++ b/web/lib/potato_mesh/config.rb @@ -73,6 +73,7 @@ module PotatoMesh DEFAULT_REMOTE_INSTANCE_CONNECT_TIMEOUT = 15 DEFAULT_REMOTE_INSTANCE_READ_TIMEOUT = 60 DEFAULT_REMOTE_INSTANCE_REQUEST_TIMEOUT = 30 + DEFAULT_REMOTE_INSTANCE_MAX_RESPONSE_BYTES = 8 * 1_048_576 DEFAULT_FEDERATION_MAX_INSTANCES_PER_RESPONSE = 64 DEFAULT_FEDERATION_MAX_DOMAINS_PER_CRAWL = 256 DEFAULT_FEDERATION_WORKER_POOL_SIZE = 4 @@ -567,6 +568,21 @@ module PotatoMesh ) end + # Maximum accepted size of a single federation HTTP response body. + # + # Mirrors {#max_json_body_bytes}, which caps inbound ingest payloads, so + # a malicious or misbehaving federation peer cannot force this process to + # buffer an unbounded response body in memory. Customisable via the + # REMOTE_INSTANCE_MAX_RESPONSE_BYTES environment variable. + # + # @return [Integer] byte ceiling for federation HTTP response bodies. + def remote_instance_max_response_bytes + fetch_positive_integer( + "REMOTE_INSTANCE_MAX_RESPONSE_BYTES", + DEFAULT_REMOTE_INSTANCE_MAX_RESPONSE_BYTES, + ) + end + # Limit the number of remote instances processed from a single response. # # @return [Integer] maximum entries processed per /api/instances payload. diff --git a/web/public/assets/js/app/__tests__/chat-format.test.js b/web/public/assets/js/app/__tests__/chat-format.test.js index 34427b6..2c013f8 100644 --- a/web/public/assets/js/app/__tests__/chat-format.test.js +++ b/web/public/assets/js/app/__tests__/chat-format.test.js @@ -127,6 +127,26 @@ test('formatChatChannelTag wraps channel names after the short name slot', () => ); }); +test('formatChatChannelTag escapes HTML metacharacters in the channel name (DOM XSS)', () => { + // Channel names originate from untrusted mesh traffic (any radio can set an + // arbitrary channel name) and the result of this function is inserted via + // innerHTML by node-page/messages.js, so an unescaped channel name would + // allow markup/script injection that executes for every viewer of the node + // detail page. Regression test for that DOM-XSS finding. + assert.equal( + formatChatChannelTag({ channelName: '' }), + '[<img src=x onerror=alert(1)>]' + ); + assert.equal( + formatChatChannelTag({ channelName: '' }), + '[</script><script>alert(1)</script>]' + ); + assert.equal( + formatChatChannelTag({ channelName: `"'&` }), + '["'&]' + ); +}); + test('formatChatPresetTag renders preset hints with placeholders', () => { assert.equal(formatChatPresetTag({ presetCode: 'MF' }), '[MF]'); assert.equal(formatChatPresetTag({ presetCode: null }), `[${PRESET_PLACEHOLDER}]`); diff --git a/web/public/assets/js/app/chat-format.js b/web/public/assets/js/app/chat-format.js index 03b3f13..650861f 100644 --- a/web/public/assets/js/app/chat-format.js +++ b/web/public/assets/js/app/chat-format.js @@ -73,12 +73,18 @@ export function formatChatMessagePrefix({ timestamp, frequency }) { * Empty channel names remain blank within the brackets, mirroring the original * UI behaviour that reserves the slot without introducing placeholder text. * - * @param {{ channelName: string|null }} params Normalised and escaped display strings. + * The channel name originates from untrusted, attacker-controllable data (a + * MeshCore/Meshtastic channel name can be set to arbitrary text by any radio + * on the mesh), so it is escaped here before being embedded in the returned + * HTML string. Callers must pass the *raw* channel name — pre-escaping it + * before calling this function would cause double-escaping. + * + * @param {{ channelName: string|null }} params Raw (unescaped) channel name. * @returns {string} Channel tag suitable for HTML insertion. */ export function formatChatChannelTag({ channelName }) { const channel = typeof channelName === 'string' ? channelName : channelName == null ? '' : String(channelName); - return `[${channel}]`; + return `[${escapeHtml(channel)}]`; } /** @@ -184,9 +190,10 @@ function firstNonNull(...candidates) { return null; } -// normalizeString is the canonical implementation in utils.js; imported here -// so callers of chat-format.js that use it directly continue to work. -import { normalizeString } from './utils.js'; +// normalizeString and escapeHtml are the canonical implementations in +// utils.js; imported here so callers of chat-format.js that use them +// directly continue to work. +import { normalizeString, escapeHtml } from './utils.js'; import { resolveMeshcorePresetDisplay } from './node-modem-metadata.js'; /** diff --git a/web/public/assets/js/app/main.js b/web/public/assets/js/app/main.js index acaee76..0c219d2 100644 --- a/web/public/assets/js/app/main.js +++ b/web/public/assets/js/app/main.js @@ -3750,14 +3750,14 @@ export function initializeApp(config) { const protocolIconCell = protocolIconPrefixHtml(n.protocol); tr.innerHTML = ` ${protocolIconCell} - ${n.node_id || ""} + ${escapeHtml(n.node_id || "")} ${renderShortHtml(n.short_name, n.role, n.long_name, n)} ${longNameHtml} ${loraFrequencyDisplay} ${modemPresetDisplay} ${timeAgo(n.last_heard, nowSec)} - ${n.role || "CLIENT"} - ${fmtHw(n.hw_model)} + ${escapeHtml(n.role || "CLIENT")} + ${escapeHtml(fmtHw(n.hw_model))} ${fmtAlt(n.battery_level, "%")} ${fmtAlt(n.voltage, "V")} ${timeHum(n.uptime_seconds)} diff --git a/web/spec/app_spec.rb b/web/spec/app_spec.rb index fd38172..1353282 100644 --- a/web/spec/app_spec.rb +++ b/web/spec/app_spec.rb @@ -2768,6 +2768,55 @@ RSpec.describe "Potato Mesh Sinatra app" do ], ) end + + context "when federation is disabled" do + around do |example| + original = ENV["FEDERATION"] + begin + ENV["FEDERATION"] = "0" + example.run + ensure + if original.nil? + ENV.delete("FEDERATION") + else + ENV["FEDERATION"] = original + end + end + end + + it "returns 404 and never attempts to process the registration" do + # A disabled instance must reject the announcement before touching the + # network or the database — otherwise an attacker could still force + # outbound federation fetches and DB writes against a PRIVATE=1 / + # FEDERATION=0 deployment simply by POSTing a signed announcement. + expect_any_instance_of(Sinatra::Application).not_to receive(:fetch_instance_json) + + post "/api/instances", instance_payload.to_json, { "CONTENT_TYPE" => "application/json" } + + expect(last_response.status).to eq(404) + + with_db(readonly: true) do |db| + count = db.get_first_value("SELECT COUNT(*) FROM instances WHERE id = ?", [instance_attributes[:id]]) + expect(count).to eq(0) + end + end + end + + context "when private mode is enabled" do + it "returns 404 regardless of the FEDERATION setting" do + # federation_enabled? returns false whenever private mode is on, + # independent of FEDERATION; the route guard must honour that too. Stub + # Config.private_mode_enabled? (the suite's convention, e.g. the node + # visibility specs) rather than toggling ENV["PRIVATE"] — the top-level + # `before` hook deletes ENV["PRIVATE"] before each example, and stubbing + # also avoids any cross-spec ENV leak. + allow(PotatoMesh::Config).to receive(:private_mode_enabled?).and_return(true) + + post "/api/instances", instance_payload.to_json, { "CONTENT_TYPE" => "application/json" } + + expect(last_response.status).to eq(404) + end + end end describe "GET /api/instances" do diff --git a/web/spec/federation_spec.rb b/web/spec/federation_spec.rb index 85f3e52..4f247de 100644 --- a/web/spec/federation_spec.rb +++ b/web/spec/federation_spec.rb @@ -949,15 +949,16 @@ RSpec.describe PotatoMesh::App::Federation do it "applies federation headers to instance fetch requests" do connection = instance_double(HTTP_CONNECTION_DOUBLE) success_response = Net::HTTPOK.new("1.1", "200", "OK") - allow(success_response).to receive(:body).and_return("{}") allow(success_response).to receive(:code).and_return("200") + allow(success_response).to receive(:read_body).and_yield("{}") captured_request = nil allow(http_client).to receive(:start) do |&block| block.call(connection) end - allow(connection).to receive(:request) do |request| + allow(connection).to receive(:request) do |request, &block| captured_request = request + block.call(success_response) success_response end @@ -978,7 +979,10 @@ RSpec.describe PotatoMesh::App::Federation do allow(http_client).to receive(:start) do |&block| block.call(connection) end - allow(connection).to receive(:request).and_return(failure_response) + allow(connection).to receive(:request) do |_request, &block| + block.call(failure_response) + failure_response + end expect do federation_helpers.send(:perform_single_http_request, uri) @@ -988,6 +992,31 @@ RSpec.describe PotatoMesh::App::Federation do ) end + it "rejects a response body larger than the configured maximum" do + connection = instance_double(HTTP_CONNECTION_DOUBLE) + success_response = Net::HTTPOK.new("1.1", "200", "OK") + allow(success_response).to receive(:code).and_return("200") + # Stream the body in chunks; the cap is exceeded partway through, so the + # read must be aborted before the whole body is buffered into memory. + allow(success_response).to receive(:read_body).and_yield("aaaa").and_yield("bbbb") + + allow(PotatoMesh::Config).to receive(:remote_instance_max_response_bytes).and_return(5) + allow(http_client).to receive(:start) do |&block| + block.call(connection) + end + allow(connection).to receive(:request) do |_request, &block| + block.call(success_response) + success_response + end + + expect do + federation_helpers.send(:perform_single_http_request, uri) + end.to raise_error( + PotatoMesh::App::InstanceFetchError, + a_string_including("exceeds maximum size of 5 bytes"), + ) + end + it "passes ip_address through to build_remote_http_client" do allow(http_client).to receive(:start).and_raise(StandardError.new("test")) diff --git a/web/views/layouts/app.erb b/web/views/layouts/app.erb index 38d2e82..0c1c64c 100644 --- a/web/views/layouts/app.erb +++ b/web/views/layouts/app.erb @@ -61,14 +61,20 @@ <% if (defined?(current_view_mode) ? current_view_mode : nil).to_s == "dashboard" %> - " inside a JSON string + # value still terminates the element in HTML parsing. %> + <% instance_ld_json = JSON.generate({ "@context" => "https://schema.org", "@type" => "WebSite", "name" => meta_name, "url" => canonical_base, "description" => meta_description, - }) %> + }).gsub("<", "\\u003c").gsub(">", "\\u003e").gsub("&", "\\u0026") %> + <% end %> <%# Import map versions the entire served JS module graph (SPEC AV3); must