Skip to content

Commit 5fa424b

Browse files
Robert E. Leeruvnet
andcommitted
fix: Address remaining HIGH and MEDIUM security findings
- HIGH-01: Validate tmux socket path (reject traversal, non-absolute, oversized) - MED-01: Add 2s connect timeout to stale socket cleanup (prevents hanging) - MED-02: Validate socket path from env var (reject traversal, non-absolute) - MED-03: Replace raw &str status with SessionStatus enum in end_session() Co-Authored-By: claude-flow <ruv@ruv.net>
1 parent 2d1e344 commit 5fa424b

File tree

5 files changed

+56
-16
lines changed

5 files changed

+56
-16
lines changed

src/bridge.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::formatting;
55
use crate::injector::InputInjector;
66
use crate::session::SessionManager;
77
use crate::socket::SocketServer;
8-
use crate::types::{BridgeMessage, InlineButton, MessageType, SendOptions};
8+
use crate::types::{BridgeMessage, InlineButton, MessageType, SendOptions, SessionStatus};
99
use std::collections::{HashMap, HashSet};
1010
use std::sync::Arc;
1111
use tokio::sync::{broadcast, Mutex, RwLock};
@@ -387,7 +387,7 @@ impl BridgeShared {
387387
.remove(&msg.session_id);
388388

389389
let sessions = self.sessions.lock().await;
390-
sessions.end_session(&msg.session_id, "ended");
390+
sessions.end_session(&msg.session_id, SessionStatus::Ended);
391391
}
392392

393393
Ok(())
@@ -865,7 +865,7 @@ impl BridgeShared {
865865
sessions.resolve_approval(id, status);
866866

867867
if action == "abort" {
868-
sessions.end_session(&approval.session_id, "aborted");
868+
sessions.end_session(&approval.session_id, SessionStatus::Aborted);
869869
}
870870
}
871871

@@ -1084,7 +1084,7 @@ impl BridgeShared {
10841084
self.session_tmux_targets.write().await.remove(&session.id);
10851085

10861086
let sessions = self.sessions.lock().await;
1087-
sessions.end_session(&session.id, "ended");
1087+
sessions.end_session(&session.id, SessionStatus::Ended);
10881088
}
10891089
}
10901090

src/config.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,10 +177,22 @@ pub fn load_config(require_auth: bool) -> Result<Config> {
177177
}
178178
}
179179

180+
// MED-02: Validate socket path from env var to prevent path traversal
180181
let socket_path = env::var("TELEGRAM_BRIDGE_SOCKET")
181182
.ok()
182-
.map(PathBuf::from)
183-
.or_else(|| file.socket_path.map(PathBuf::from))
183+
.or(file.socket_path)
184+
.map(|s| {
185+
if s.contains("..") {
186+
tracing::warn!(path = %s, "Socket path contains '..', using default");
187+
return default_socket.clone();
188+
}
189+
let p = PathBuf::from(&s);
190+
if !p.is_absolute() {
191+
tracing::warn!(path = %s, "Socket path is not absolute, using default");
192+
return default_socket.clone();
193+
}
194+
p
195+
})
184196
.unwrap_or(default_socket);
185197

186198
Ok(Config {

src/injector.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,28 @@ impl InputInjector {
2121
}
2222

2323
/// Set the tmux target and optional socket path
24+
/// HIGH-01: Validate socket path to prevent arbitrary socket targeting
2425
pub fn set_target(&mut self, target: &str, socket: Option<&str>) {
2526
self.tmux_target = Some(target.to_string());
26-
self.tmux_socket = socket.map(|s| s.to_string());
27+
self.tmux_socket = socket.and_then(|s| {
28+
let path = std::path::Path::new(s);
29+
// Reject paths with traversal components
30+
if s.contains("..") {
31+
tracing::warn!(path = %s, "Rejecting tmux socket path with '..' traversal");
32+
return None;
33+
}
34+
// Must be an absolute path
35+
if !path.is_absolute() {
36+
tracing::warn!(path = %s, "Rejecting non-absolute tmux socket path");
37+
return None;
38+
}
39+
// Reasonable length limit
40+
if s.len() > 256 {
41+
tracing::warn!(len = s.len(), "Rejecting oversized tmux socket path");
42+
return None;
43+
}
44+
Some(s.to_string())
45+
});
2746
}
2847

2948
/// Get current tmux target

src/session.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,17 +227,19 @@ impl SessionManager {
227227
);
228228
}
229229

230-
pub fn end_session(&self, session_id: &str, status: &str) {
230+
/// MED-03: Accept SessionStatus enum instead of raw &str to prevent invalid states
231+
pub fn end_session(&self, session_id: &str, status: SessionStatus) {
231232
let now = Utc::now().to_rfc3339();
233+
let status_str = status.as_str();
232234
let _ = self.db.execute(
233235
"UPDATE sessions SET status = ?1, last_activity = ?2 WHERE id = ?3",
234-
params![status, now, session_id],
236+
params![status_str, now, session_id],
235237
);
236238
let _ = self.db.execute(
237239
"UPDATE pending_approvals SET status = 'expired' WHERE session_id = ?1 AND status = 'pending'",
238240
params![session_id],
239241
);
240-
tracing::info!(%session_id, %status, "Session ended");
242+
tracing::info!(%session_id, %status_str, "Session ended");
241243
}
242244

243245
pub fn reactivate_session(&self, session_id: &str) {
@@ -440,7 +442,7 @@ mod tests {
440442
.create_session(None, 12345, None, None, None, None)
441443
.unwrap();
442444

443-
mgr.end_session(&id, "ended");
445+
mgr.end_session(&id, SessionStatus::Ended);
444446
let session = mgr.get_session(&id).unwrap();
445447
assert_eq!(session.status, SessionStatus::Ended);
446448

src/socket.rs

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,21 +86,28 @@ impl SocketServer {
8686
}
8787

8888
/// Clean up stale socket file if no daemon is listening
89+
/// MED-01: Add connect timeout to prevent hanging on unresponsive sockets
8990
async fn cleanup_stale_socket(&self) -> Result<()> {
9091
if !self.socket_path.exists() {
9192
return Ok(());
9293
}
9394

94-
// Try connecting to check if socket is active
95-
match UnixStream::connect(&self.socket_path).await {
96-
Ok(_) => {
95+
// Try connecting with a timeout to check if socket is active
96+
let connect_result = tokio::time::timeout(
97+
std::time::Duration::from_secs(2),
98+
UnixStream::connect(&self.socket_path),
99+
)
100+
.await;
101+
102+
match connect_result {
103+
Ok(Ok(_)) => {
97104
return Err(AppError::Socket(format!(
98105
"Socket {} is already in use by another process",
99106
self.socket_path.display()
100107
)));
101108
}
102-
Err(_) => {
103-
// Connection failed = stale socket
109+
Ok(Err(_)) | Err(_) => {
110+
// Connection failed or timed out = stale socket
104111
fs::remove_file(&self.socket_path)?;
105112
tracing::info!(path = %self.socket_path.display(), "Removed stale socket file");
106113
}

0 commit comments

Comments
 (0)