From 41b52d24295e3bb3ab9df77335b81f5b9436a18d Mon Sep 17 00:00:00 2001
From: Ethan Pailes <pailes@google.com>
Date: Thu, 7 May 2026 20:54:25 +0000
Subject: [PATCH] fix!: prevent bad chars in session names

This patch adds a guard against bad chars in session names.
It also adds a check for bad session names into the daemon just
for extra defensiveness, though the attach proc is probably good
enough.

Closes #362
---
 libshpool/src/attach.rs        |  9 +++++++++
 libshpool/src/daemon/server.rs | 19 ++++++++++++++++-
 shpool/tests/attach.rs         | 37 ++++++++++++++++++++++++++++++++++
 shpool/tests/support/daemon.rs | 13 +++++++++---
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/libshpool/src/attach.rs b/libshpool/src/attach.rs
index a5c614d8..3f15d22d 100644
--- a/libshpool/src/attach.rs
+++ b/libshpool/src/attach.rs
@@ -103,6 +103,7 @@ impl Attach {
 
         info!("looping on attach_with_name");
         loop {
+            info!("attaching to '{}'", resolved_name);
             match self.attach_with_name(resolved_name) {
                 Ok(AttachResult::Done) => return Ok(()),
                 Ok(AttachResult::Switch(s)) => maybe_switch = s,
@@ -130,6 +131,14 @@ impl Attach {
             eprintln!("session name '{}' may not have whitespace", resolved_name);
             return Ok(AttachResult::Done);
         }
+        if resolved_name.chars().any(|c| '/' == c) {
+            eprintln!("session names may not contain slashes");
+            return Ok(AttachResult::Done);
+        }
+        if resolved_name == "." || resolved_name == ".." {
+            eprintln!("session names may not be special directory names");
+            return Ok(AttachResult::Done);
+        }
 
         let mut detached = false;
         let mut tries = 0;
diff --git a/libshpool/src/daemon/server.rs b/libshpool/src/daemon/server.rs
index 475cec2a..5c8d9cfc 100644
--- a/libshpool/src/daemon/server.rs
+++ b/libshpool/src/daemon/server.rs
@@ -221,10 +221,27 @@ impl Server {
     #[instrument(skip_all)]
     fn handle_attach(
         &self,
-        stream: UnixStream,
+        mut stream: UnixStream,
         conn_id: usize,
         header: AttachHeader,
     ) -> anyhow::Result<()> {
+        if header.name.chars().any(|c| '/' == c || c.is_whitespace())
+            || header.name == "."
+            || header.name == ".."
+            || header.name.is_empty()
+        {
+            write_reply(
+                &mut stream,
+                AttachReplyHeader {
+                    status: AttachStatus::UnexpectedError(format!(
+                        "invalid session name '{}'",
+                        header.name
+                    )),
+                },
+            )?;
+            return Ok(());
+        }
+
         let user_info = user::info().context("resolving user info")?;
         let shell_env = self.build_shell_env(&user_info, &header).context("building shell env")?;
 
diff --git a/shpool/tests/attach.rs b/shpool/tests/attach.rs
index a1805e63..302d6c78 100644
--- a/shpool/tests/attach.rs
+++ b/shpool/tests/attach.rs
@@ -602,6 +602,43 @@ fn whitespace_session_not_allowed() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[test]
+#[timeout(30000)]
+fn slash_session_not_allowed() -> anyhow::Result<()> {
+    let mut daemon_proc = support::daemon::Proc::new(
+        "norc.toml",
+        DaemonArgs { listen_events: false, ..DaemonArgs::default() },
+    )
+    .context("starting daemon proc")?;
+
+    let mut tty1 =
+        daemon_proc.attach("this/bad", Default::default()).context("attaching from tty1")?;
+    let mut line_matcher1 = tty1.stderr_line_matcher()?;
+    line_matcher1.scan_until_re("may not contain slashes")?;
+
+    Ok(())
+}
+
+#[test]
+#[timeout(30000)]
+fn dot_session_not_allowed() -> anyhow::Result<()> {
+    let mut daemon_proc = support::daemon::Proc::new(
+        "norc.toml",
+        DaemonArgs { listen_events: false, ..DaemonArgs::default() },
+    )
+    .context("starting daemon proc")?;
+
+    let mut tty1 = daemon_proc.attach(".", Default::default()).context("attaching from tty1")?;
+    let mut line_matcher1 = tty1.stderr_line_matcher()?;
+    line_matcher1.scan_until_re("may not be special directory names")?;
+
+    let mut tty2 = daemon_proc.attach("..", Default::default()).context("attaching from tty2")?;
+    let mut line_matcher2 = tty2.stderr_line_matcher()?;
+    line_matcher2.scan_until_re("may not be special directory names")?;
+
+    Ok(())
+}
+
 #[test]
 #[timeout(30000)]
 fn daemon_hangup() -> anyhow::Result<()> {
diff --git a/shpool/tests/support/daemon.rs b/shpool/tests/support/daemon.rs
index 34883179..102761b2 100644
--- a/shpool/tests/support/daemon.rs
+++ b/shpool/tests/support/daemon.rs
@@ -285,10 +285,17 @@ impl Proc {
     }
 
     pub fn attach(&mut self, name: &str, args: AttachArgs) -> anyhow::Result<attach::Proc> {
-        let log_file =
-            self.tmp_dir.path().join(format!("attach_{}_{}.log", name, self.subproc_counter));
+        let stripped_name: String = name
+            .replace(".", "dot")
+            .chars()
+            .filter(|c| !(c.is_whitespace() || "./".contains(*c)))
+            .collect();
+        let log_file = self
+            .tmp_dir
+            .path()
+            .join(format!("attach_{}_{}.log", stripped_name, self.subproc_counter));
         let test_hook_socket_path =
-            self.tmp_dir.path().join(format!("ah{}_{}.sock", name, self.subproc_counter));
+            self.tmp_dir.path().join(format!("ah{}_{}.sock", stripped_name, self.subproc_counter));
         eprintln!("spawning attach proc with log {:?}", log_file);
         self.subproc_counter += 1;