Safe naming and authN checks

compose.yaml

@@ -5,7 +5,7 @@
 
 services:
   redis:
-    image: bitnami/redis:7.2
+    image: bitnamilegacy/redis:7.2
     environment:
       - ALLOW_EMPTY_PASSWORD=yes
     ports:

crates/sshx-core/src/lib.rs

@@ -8,9 +8,13 @@ use std::sync::atomic::{AtomicU32, Ordering};
 
 use serde::{Deserialize, Serialize};
 
+pub mod name;
+
+pub use name::{InvalidName, SafeName};
+
 /// Protocol buffer and gRPC definitions, automatically generated by Tonic.
 #[allow(missing_docs, non_snake_case)]
-#[allow(clippy::derive_partial_eq_without_eq)]
+#[allow(clippy::derive_partial_eq_without_eq, clippy::result_large_err)]
 pub mod proto {
     tonic::include_proto!("sshx");
 

crates/sshx-core/src/name.rs

@@ -0,0 +1,190 @@
+//! Validated identifiers that are safe to embed in a URL path.
+
+use std::fmt;
+use std::ops::Deref;
+use std::str::FromStr;
+
+use serde::{de::Visitor, Deserialize, Deserializer, Serialize};
+
+/// A name that is safe to interpolate into a URL path without escaping.
+///
+/// Session names are generated with [`rand_alphanumeric`], so this is a
+/// whitelist rather than a blacklist: it rejects anything that could change
+/// the meaning of a URL path, such as `/`, `.`, or `%`. Interpolating a
+/// `SafeName` into a path is therefore always correct, so callers never need
+/// to escape it.
+///
+/// [`rand_alphanumeric`]: crate::rand_alphanumeric
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct SafeName(String);
+
+impl SafeName {
+    /// Maximum accepted length, in bytes.
+    ///
+    /// This is far above the length of any generated name, and exists only to
+    /// bound the work done on attacker-controlled input.
+    pub const MAX_LEN: usize = 64;
+
+    /// Returns the name as a string slice.
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+
+impl fmt::Display for SafeName {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+impl Deref for SafeName {
+    type Target = str;
+
+    fn deref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl FromStr for SafeName {
+    type Err = InvalidName;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        if s.is_empty() {
+            return Err(InvalidName::Empty);
+        }
+        if s.len() > Self::MAX_LEN {
+            return Err(InvalidName::TooLong);
+        }
+        if !s.bytes().all(|b| b.is_ascii_alphanumeric()) {
+            return Err(InvalidName::NotAlphanumeric);
+        }
+        Ok(SafeName(s.to_string()))
+    }
+}
+
+impl Serialize for SafeName {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        serializer.serialize_str(&self.0)
+    }
+}
+
+/// Deserializes by way of [`SafeName::from_str`], so the whitelist is enforced
+/// by every deserializer and not just by explicit `parse` calls.
+///
+/// This is what lets `axum::extract::Path<SafeName>` reject an invalid name on
+/// its own: axum deserializes path parameters, so a bad name fails extraction
+/// with `400 Bad Request` before the handler (and any peer proxying) runs.
+impl<'de> Deserialize<'de> for SafeName {
+    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+        struct SafeNameVisitor;
+
+        impl Visitor<'_> for SafeNameVisitor {
+            type Value = SafeName;
+
+            fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                f.write_str("a non-empty alphanumeric name")
+            }
+
+            fn visit_str<E: serde::de::Error>(self, v: &str) -> Result<SafeName, E> {
+                v.parse().map_err(E::custom)
+            }
+        }
+
+        deserializer.deserialize_str(SafeNameVisitor)
+    }
+}
+
+/// The reason a string was rejected as a [`SafeName`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum InvalidName {
+    /// The name was the empty string.
+    Empty,
+
+    /// The name exceeded [`SafeName::MAX_LEN`].
+    TooLong,
+
+    /// The name contained a byte outside `A-Z`, `a-z`, and `0-9`.
+    NotAlphanumeric,
+}
+
+impl fmt::Display for InvalidName {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let msg = match self {
+            Self::Empty => "name is empty",
+            Self::TooLong => "name is too long",
+            Self::NotAlphanumeric => "name must be purely alphanumeric",
+        };
+        f.write_str(msg)
+    }
+}
+
+impl std::error::Error for InvalidName {}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn accepts_alphanumeric() {
+        for name in ["aB3xY9zQ1mK", "0123456789", "a", "Z"] {
+            assert_eq!(name.parse::<SafeName>().unwrap().as_str(), name);
+        }
+    }
+
+    #[test]
+    fn rejects_path_metacharacters() {
+        // Each of these would change the meaning of an interpolated URL path.
+        for name in [
+            "",
+            "..",
+            "a/b",
+            "a%2Fb",
+            "../../admin",
+            "a b",
+            "a\tb",
+            "a\nb",
+            "a?b",
+            "a#b",
+            "a\\b",
+            "ünïcode",
+            "a\0b",
+        ] {
+            assert!(
+                name.parse::<SafeName>().is_err(),
+                "expected {name:?} to be rejected"
+            );
+        }
+    }
+
+    #[test]
+    fn rejects_overlong() {
+        assert!("a".repeat(SafeName::MAX_LEN).parse::<SafeName>().is_ok());
+        assert!("a"
+            .repeat(SafeName::MAX_LEN + 1)
+            .parse::<SafeName>()
+            .is_err());
+    }
+
+    #[test]
+    fn roundtrips_through_display() {
+        let name: SafeName = "abcDEF1234".parse().unwrap();
+        assert_eq!(name.to_string(), "abcDEF1234");
+        assert_eq!(name.as_str(), "abcDEF1234");
+    }
+
+    #[test]
+    fn generated_names_are_always_safe() {
+        // Guards the invariant that makes interpolating a session name into a
+        // URL path safe. If the generator's alphabet ever widens past
+        // alphanumeric, this fails rather than silently opening a hole.
+        for len in [1, 10, 14, 22] {
+            for _ in 0..64 {
+                let name = crate::rand_alphanumeric(len);
+                assert!(
+                    name.parse::<SafeName>().is_ok(),
+                    "generated name {name:?} (len {len}) is not a SafeName"
+                );
+            }
+        }
+    }
+}

crates/sshx-server/src/web/socket.rs

@@ -10,7 +10,7 @@ use axum::response::IntoResponse;
 use bytes::Bytes;
 use futures_util::SinkExt;
 use sshx_core::proto::{server_update::ServerMessage, NewShell, TerminalInput, TerminalSize};
-use sshx_core::Sid;
+use sshx_core::{SafeName, Sid};
 use subtle::ConstantTimeEq;
 use tokio::sync::mpsc;
 use tokio_stream::StreamExt;
@@ -21,7 +21,7 @@ use crate::web::protocol::{WsClient, WsServer};
 use crate::ServerState;
 
 pub async fn get_session_ws(
-    Path(name): Path<String>,
+    Path(name): Path<SafeName>,
     ws: WebSocketUpgrade,
     State(state): State<Arc<ServerState>>,
 ) -> impl IntoResponse {
@@ -93,7 +93,6 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc<Session>) -> Result<
 
     let metadata = session.metadata();
     let user_id = session.counter().next_uid();
-    session.sync_now();
     send(socket, WsServer::Hello(user_id, metadata.name.clone())).await?;
 
     let can_write = match recv(socket).await? {
@@ -128,6 +127,10 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc<Session>) -> Result<
     };
 
     let _user_guard = session.user_scope(user_id, can_write)?;
+    // Persist the allocated ID counter, but only once the client is actually
+    // authenticated, so that unauthenticated connections cannot force the
+    // session to be snapshotted and written to storage.
+    session.sync_now();
 
     let update_tx = session.update_tx(); // start listening for updates before any state reads
     let mut broadcast_stream = session.subscribe_broadcast();

crates/sshx-server/tests/simple.rs

@@ -32,3 +32,53 @@ async fn test_web_get() -> Result<()> {
 
     Ok(())
 }
+
+/// A session name is interpolated into an upstream URL when proxying between
+/// mesh peers, so the `{name}` path parameter must be restricted to characters
+/// that cannot alter the meaning of that URL.
+///
+/// Each case performs a real WebSocket handshake, because that is the only
+/// unambiguous discriminator: with validation in place the upgrade is refused
+/// with `400`, whereas without it the upgrade succeeds and the handler runs.
+/// A plain `GET` cannot distinguish the two, since `WebSocketUpgrade` also
+/// rejects a non-upgrade request with `400`.
+#[tokio::test]
+async fn test_session_name_rejects_path_traversal() -> Result<()> {
+    let server = TestServer::new().await;
+
+    // Control: a legitimate name must still be allowed to connect.
+    tokio_tungstenite::connect_async(server.ws_endpoint("aB3xY9zQ1mK")).await?;
+
+    // These stay percent-encoded on the wire, since the `http` crate does not
+    // normalize paths. The server decodes them into `/` and `.` before
+    // validating, which is exactly what has to be rejected.
+    let rejected = [
+        "a%2Fb",              // decodes to "a/b"
+        "a%2F..%2F..%2Fevil", // decodes to "a/../../evil"
+        "%2E%2E%2Fadmin",     // decodes to "../admin"
+        "%2E%2E",             // decodes to ".."
+        "a%20b",              // decodes to "a b"
+        "a+b",
+    ];
+
+    for name in rejected {
+        let uri = format!("ws://{}/api/s/{}", server.local_addr(), name);
+        let err = tokio_tungstenite::connect_async(&uri)
+            .await
+            .err()
+            .unwrap_or_else(|| panic!("expected {name:?} to be refused"));
+
+        match err {
+            tokio_tungstenite::tungstenite::Error::Http(resp) => {
+                assert_eq!(
+                    resp.status(),
+                    http::StatusCode::BAD_REQUEST,
+                    "expected {name:?} to be refused with 400"
+                );
+            }
+            other => panic!("expected an HTTP rejection for {name:?}, got {other}"),
+        }
+    }
+
+    Ok(())
+}

package.json

@@ -35,6 +35,7 @@
     "@sveltejs/adapter-static": "^2.0.3",
     "@sveltejs/kit": "^1.24.1",
     "@types/lodash-es": "^4.17.9",
+    "@types/node": "^20.6.0",
     "@typescript-eslint/eslint-plugin": "^6.7.0",
     "@typescript-eslint/parser": "^6.7.0",
     "autoprefixer": "^10.4.15",

rustfmt.toml

@@ -3,4 +3,5 @@ group_imports = "StdExternalCrate"
 wrap_comments = true
 format_strings = true
 normalize_comments = true
+comment_width = 100
 reorder_impl_items = true