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
10 changes: 0 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 19 additions & 7 deletions crates/openjd-cli/tests/cli_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,14 @@ fn python_shim_dir() -> Option<PathBuf> {
// If the interpreter is literally named `python` we don't need a shim —
// its parent directory is already on `PATH` (that's how `which` found
// it, transitively). Detect this by comparing the file-name component.
if target.file_name().and_then(|n| n.to_str()) == Some("python") {
let fname = target.file_name().and_then(|n| n.to_str()).unwrap_or("");
// On Windows, `python.exe` is the canonical name.
let is_canonical = if cfg!(windows) {
fname.eq_ignore_ascii_case("python.exe")
} else {
fname == "python"
};
if is_canonical {
return None;
}
let shim_dir =
Expand All @@ -65,7 +72,7 @@ fn python_shim_dir() -> Option<PathBuf> {
std::fs::create_dir_all(&shim_dir)
.unwrap_or_else(|e| panic!("Failed to create shim_dir {shim_dir:?}: {e}"));
let shim_path = shim_dir.join(if cfg!(windows) {
"python.cmd"
"python.exe"
} else {
"python"
});
Expand All @@ -89,11 +96,16 @@ fn python_shim_dir() -> Option<PathBuf> {
}
#[cfg(windows)]
{
// On Windows a .cmd wrapper works for anything spawned via
// CreateProcess with a bare name lookup.
let script = format!("@echo off\r\n{} %*\r\n", target.display());
std::fs::write(&shim_path, script)
.unwrap_or_else(|e| panic!("Failed to write python shim {shim_path:?}: {e}"));
// On Windows, hard-link the real interpreter as `python.exe` so
// it resolves correctly under PATHEXT semantics without the
// argument-mangling issues of a .cmd/.bat wrapper. Hard link
// avoids a ~5 MB copy and works as long as source and dest are on
// the same volume (both are under %TEMP% / %LOCALAPPDATA%).
Comment on lines +99 to +103

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the PR description, it sounded like this change is supposed to fix resolution issues so we don't need stuff like this? I might be misinterpreting something though.

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'm of a mind to remove this python proxy thing entirely, but I haven't looked closely at what it's doing.

std::fs::hard_link(&target, &shim_path).unwrap_or_else(|_| {
std::fs::copy(&target, &shim_path).unwrap_or_else(|e| {
panic!("Failed to copy python interpreter to {shim_path:?}: {e}")
});
});
}
Some(shim_dir)
});
Expand Down
1 change: 0 additions & 1 deletion crates/openjd-sessions/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ futures-util = "0.3.32"
nix = { version = "0.31", features = ["signal", "process", "user", "fs"] }

[target.'cfg(windows)'.dependencies]
which = "8.0.2"
windows = { version = "0.62", features = [
"Win32_Foundation",
"Win32_Security",
Expand Down
17 changes: 13 additions & 4 deletions crates/openjd-sessions/src/cross_user_helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,15 @@ pub(crate) async fn run_via_helper(
message_tx: tokio::sync::mpsc::UnboundedSender<ActionMessage>,
cancel_writer: Option<&std::fs::File>,
) -> Result<crate::subprocess::SubprocessResult, SessionError> {
// Windows note: executable resolution happens inside the helper
// (helper/src/runner_win.rs::locate_executable), which runs as the
// target user — it can resolve executables in directories only that
// user can read, and its PATH fallback is the target user's own
// environment (matching what the workload actually runs under).
// Resolution failures come back over the protocol as an error response
// and are mapped to SessionError::SubprocessStart below.
let args = &config.args;

// Build the env map (only set values; unsets are excluded).
let env: serde_json::Map<String, serde_json::Value> = config
.env_vars
Expand All @@ -572,8 +581,8 @@ pub(crate) async fn run_via_helper(
.collect();

let cmd = serde_json::json!({
"command": config.args[0],
"args": &config.args[1..],
"command": args[0],
"args": &args[1..],
"env": env,
"cwd": config.working_dir,
});
Expand All @@ -586,7 +595,7 @@ pub(crate) async fn run_via_helper(
session_id,
LogContent::FILE_PATH | LogContent::PROCESS_CONTROL,
"Running command {}",
crate::subprocess::format_command_for_log(&config.args)
crate::subprocess::format_command_for_log(args)
);

// Timeout as async future instead of OS thread
Expand Down Expand Up @@ -682,7 +691,7 @@ pub(crate) async fn run_via_helper(

if let Some(msg) = resp.get("error").and_then(|v| v.as_str()) {
return Err(SessionError::SubprocessStart {
command: config.args[0].clone(),
command: args[0].clone(),
source: std::io::Error::other(msg.to_string()),
});
}
Expand Down
2 changes: 2 additions & 0 deletions crates/openjd-sessions/src/helper/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ mod protocol;
mod runner;
#[cfg(windows)]
mod runner_win;
#[cfg(windows)]
mod win32_which;

use protocol::{constant_time_eq, send, Command, Response, AUTH_TOKEN_LEN};
use std::io::{BufRead, Read};
Expand Down
50 changes: 49 additions & 1 deletion crates/openjd-sessions/src/helper/src/runner_win.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ pub fn run_command(
use windows::Win32::Foundation::HANDLE;
const CREATE_NEW_PROCESS_GROUP: u32 = 0x00000200;

let mut child = Command::new(&cmd.command)
let command = locate_executable(&cmd.command, &cmd.env, &cmd.cwd)?;

let mut child = Command::new(&command)
.args(&cmd.args)
.envs(&cmd.env)
.current_dir(&cmd.cwd)
Expand Down Expand Up @@ -184,6 +186,52 @@ fn handle_cancel(child_pid: u32, method: &CancelMethod) -> Option<std::time::Ins
}
}

/// Resolve `command` to an absolute path with canonical Windows search
/// semantics (PATHEXT-aware, earliest PATH directory wins), searching the
/// working directory first: `{cwd};{PATH}`.
///
/// Runs in the helper — i.e. **as the target user** — so it can resolve
/// executables in directories only the target user can read.
///
/// PATH and PATHEXT are each taken from exactly one source, chosen before
/// searching: a key in the action's env vars (case-insensitive) is used
/// exclusively — the helper's own value is never consulted then, even if
/// the search finds nothing. Only when the env map defines no such key at
/// all is the helper's own environment used (the target user's
/// environment block, inherited from `CreateEnvironmentBlock` at spawn).
/// This matches the `.envs()` merge at spawn — an action-supplied value
/// overwrites the inherited one — so resolution always searches with the
/// values the workload actually sees, never a union of the two.
///
/// Absolute paths pass through unchanged (the OS resolves the extension).
/// A not-found result is a hard error with the same message the Python
/// implementation raises — resolving here, before `Command::new`, prevents
/// `CreateProcessW`'s legacy fallback search (application directory, system
/// directories, the helper's own PATH lookup for `.exe` only).
fn locate_executable(
command: &str,
env: &std::collections::HashMap<String, String>,
cwd: &str,
) -> Result<String, String> {
if std::path::Path::new(command).is_absolute() {
return Ok(command.to_string());
}
let env_var = |name: &str| -> Option<String> {
env.iter()
.find(|(k, _)| k.eq_ignore_ascii_case(name))
.map(|(_, v)| v.clone())
};
let path_var = env_var("PATH").unwrap_or_else(|| std::env::var("PATH").unwrap_or_default());
let pathext =
env_var("PATHEXT").unwrap_or_else(|| std::env::var("PATHEXT").unwrap_or_default());
let search_path = format!("{cwd};{path_var}");
match crate::win32_which::locate_in(command, &search_path, &pathext, std::path::Path::new(cwd))
{
Some(found) => Ok(found.to_string_lossy().into_owned()),
None => Err(format!("Could not find executable file: {command}")),
}
}

/// Send CTRL_BREAK_EVENT to a process.
fn send_ctrl_break(pid: u32) -> bool {
use windows::Win32::System::Console::{GenerateConsoleCtrlEvent, CTRL_BREAK_EVENT};
Expand Down
Loading
Loading