Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions libshpool/src/attach.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like it'd be useful to have slashes for grouping session names. e.g. something like fixing-bugs/1 fixing-bugs/2 etc. I guess we're doing this because the session name gets passed directly into some file operations. Have you considered URL encoding the names for filesystem purposes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda feel like there are lots of other potential seperators people can use, and I would rather not make the rules about which files go where overly complicated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take out period, white space and slash there's not really any seperators left (I mean I guess dash?) If you use URL encoding you can get rid of all these other checks you have to do too, so the code would probably become simpler if anything. The only downside is that the filepath that gets used will be ugly, but people can just not use special characters in their session name if they care about that.

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;
Expand Down
19 changes: 18 additions & 1 deletion libshpool/src/daemon/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")?;

Expand Down
37 changes: 37 additions & 0 deletions shpool/tests/attach.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
13 changes: 10 additions & 3 deletions shpool/tests/support/daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading