fix: security review — dashboard XSS, private-mode federation gap, Matrix bridge resilience (#839)

* fix(web): escape untrusted mesh fields in dashboard to close DOM XSS Security review found user-controlled mesh data reaching innerHTML unescaped (upstream-originating; worth a PR back to l5yth/potato-mesh): - High: channel_name was interpolated raw by formatChatChannelTag and rendered via innerHTML on the node-detail page and map overlays, so a crafted channel name (any radio can set one) executed script for every viewer. Now escaped with the canonical escapeHtml. - Defense-in-depth: node_id, role, and hw_model in the node table are now escaped for consistency with sibling cells. Regression test added for the channel-name escape. Full JS suite: 1448 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(web): gate POST /api/instances by federation mode + hardening Security review findings in the Sinatra app (upstream-originating; worth a PR back to l5yth/potato-mesh): - High: POST /api/instances performed outbound federation fetches and DB writes with no federation/private guard, so a PRIVATE=1 or FEDERATION=0 node still acted on unsolicited signed announcements. Added `halt 404 unless federation_enabled?`, mirroring the existing GET /api/instances guard. - Low: the JSON-LD <script> block now escapes </script> breakout characters. - Low: federation peer fetches now enforce a max response size (REMOTE_INSTANCE_MAX_RESPONSE_BYTES, default 8 MiB) to bound memory. Regression spec added for the federation-gate (private + FEDERATION=0 cases). NOTE: rspec could not be run locally (this host has only Ruby 2.6; the project needs >=3.0 and no Docker was available). Changes are `ruby -c` syntax-checked and reviewed against the mirrored guard; the ruby.yml CI workflow must confirm the spec on push/PR before merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(matrix): stop silent message loss + add HTTP timeouts + log reg failures Security/resilience review of the Matrix bridge (upstream-originating; worth a PR back to l5yth/potato-mesh): - High: within a poll batch, a later message that forwarded successfully advanced the in-memory rx_time watermark past an EARLIER message that failed, so the failed message was never refetched -> silent permanent loss. poll_once now breaks at the first failure, committing only the contiguous successful prefix; the failed message and everything after it are retried in order next poll (no reordering, no duplicates). Regression test added and proven to bite. - High: the shared reqwest client had no timeouts, so a hung homeserver/API stalled the single-threaded poll loop (and startup health checks) forever. Added timeout(30s) + connect_timeout(10s). - Medium: ensure_user_registered swallowed all non-success responses silently; it now warns (status + body) on anything other than the expected already-registered case, so a bad as_token is diagnosable. cargo test: 118 passed; cargo fmt clean. Tradeoff: a permanently-failing message now blocks its batch (retried each poll) rather than being lost -- safer than silent loss; a skip-after-N/dead-letter follow-up is possible if needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: address Copilot review on the security PR (Matrix errcode + spec hygiene) - matrix.rs: ensure_user_registered now treats only the specific M_USER_IN_USE errcode as "already registered", instead of any 400 Bad Request, so real 400 failures (malformed request, config issues) reach the diagnostic warn branch. Updated the M_USER_IN_USE test to send the errcode body and added a test that a non-M_USER_IN_USE 400 is not suppressed. cargo test: 119 passing. - app_spec.rb: the private-mode POST /api/instances example now restores ENV["PRIVATE"] via an around/ensure block (mirroring the FEDERATION pattern), so private mode can't leak into later specs and cause order-dependent failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: implement Copilot follow-ups — streaming size cap + bounded skip - instance_fetcher.rb: perform_single_http_request now uses the block form of Net::HTTP#request + response.read_body to enforce the response-size cap INCREMENTALLY, aborting as soon as the limit is exceeded, instead of letting the non-block form buffer the whole body into memory first. Updated the two federation_spec mocks to the block/streaming form and added a size-cap test. - main.rs (Matrix bridge): a permanently-failing ("poison") message no longer blocks the batch forever. Consecutive per-message failures are counted in-memory; after MAX_FORWARD_ATTEMPTS (5) the message is skipped (advanced past, with a warning) so later messages make progress. Transient failures still stop-and-retry in order (no silent loss, no reordering). Added a skip-after-N regression test; the original watermark test still passes. cargo test: 120 passing, cargo fmt clean. Ruby: instance_fetcher.rb is `ruby -c` clean; the spec exercises the new paths but rspec/`ruby -c` can't run locally (Ruby 2.6 here can't parse the repo's 3.1+ syntax) — the ruby.yml CI is the gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(web): fix private-mode /api/instances spec to actually enable private mode Running the suite locally (Ruby 4.0) revealed the private-mode POST /api/instances example never entered private mode: the top-level `before` hook deletes ENV["PRIVATE"] before each example, so toggling it (even via an around block) had no effect and the request returned 201 instead of 404. Switch to stubbing PotatoMesh::Config.private_mode_enabled? => true, the pattern the rest of the suite uses for private-mode cases. This makes the test actually exercise the guard AND removes the ENV manipulation entirely (so there is no cross-spec ENV leak to worry about). Full web suite: 1470 examples, 0 failures; rufo clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:42:30 +02:00 · 2026-07-05 15:27:37 -05:00
parent 1b934ee6f7
commit df201f3ee4
11 changed files with 552 additions and 26 deletions
@@ -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<u64>,
+    /// 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<u64>,
+    #[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
@@ -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();
@@ -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
@@ -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)
@@ -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.
@@ -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)>' }),
+    '[&lt;img src=x onerror=alert(1)&gt;]'
+  );
+  assert.equal(
+    formatChatChannelTag({ channelName: '</script><script>alert(1)</script>' }),
+    '[&lt;/script&gt;&lt;script&gt;alert(1)&lt;/script&gt;]'
+  );
+  assert.equal(
+    formatChatChannelTag({ channelName: `"'&` }),
+    '[&quot;&#39;&amp;]'
+  );
+});
+
 test('formatChatPresetTag renders preset hints with placeholders', () => {
  assert.equal(formatChatPresetTag({ presetCode: 'MF' }), '[MF]');
  assert.equal(formatChatPresetTag({ presetCode: null }), `[${PRESET_PLACEHOLDER}]`);
@@ -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';

 /**
@@ -3750,14 +3750,14 @@ export function initializeApp(config) {
      const protocolIconCell = protocolIconPrefixHtml(n.protocol);
      tr.innerHTML = `
        <td class="nodes-col nodes-col--protocol">${protocolIconCell}</td>
-        <td class="mono nodes-col nodes-col--node-id">${n.node_id || ""}</td>
+        <td class="mono nodes-col nodes-col--node-id">${escapeHtml(n.node_id || "")}</td>
        <td class="nodes-col nodes-col--short-name">${renderShortHtml(n.short_name, n.role, n.long_name, n)}</td>
        <td class="nodes-col nodes-col--long-name">${longNameHtml}</td>
        <td class="nodes-col nodes-col--frequency">${loraFrequencyDisplay}</td>
        <td class="nodes-col nodes-col--modem-preset">${modemPresetDisplay}</td>
        <td class="nodes-col nodes-col--last-seen">${timeAgo(n.last_heard, nowSec)}</td>
-        <td class="nodes-col nodes-col--role">${n.role || "CLIENT"}</td>
-        <td class="nodes-col nodes-col--hw-model">${fmtHw(n.hw_model)}</td>
+        <td class="nodes-col nodes-col--role">${escapeHtml(n.role || "CLIENT")}</td>
+        <td class="nodes-col nodes-col--hw-model">${escapeHtml(fmtHw(n.hw_model))}</td>
        <td class="nodes-col nodes-col--battery">${fmtAlt(n.battery_level, "%")}</td>
        <td class="nodes-col nodes-col--voltage">${fmtAlt(n.voltage, "V")}</td>
        <td class="nodes-col nodes-col--uptime">${timeHum(n.uptime_seconds)}</td>
@@ -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
@@ -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"))

@@ -61,14 +61,20 @@
  <link rel="icon" type="image/svg+xml" sizes="any" href="/potatomesh-logo.svg" />
  <link rel="alternate icon" type="image/x-icon" href="/favicon.ico" />
  <% if (defined?(current_view_mode) ? current_view_mode : nil).to_s == "dashboard" %>
-  <script type="application/ld+json">
-    <%= JSON.generate({
+  <% # Escape characters that could prematurely close the <script> element
+     # (or be misinterpreted as HTML) when meta_name/canonical_base/
+     # meta_description are attacker-influenced. JSON string escaping alone
+     # is not enough here since a literal "</script>" 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") %>
+  <script type="application/ld+json">
+    <%= instance_ld_json %>
  </script>
  <% end %>
  <%# Import map versions the entire served JS module graph (SPEC AV3); must