askpass: Fix cli path when executed in a remote server #39475

marcocondrache · 2025-10-03T15:32:02Z

Closes #39469
Closes #39438
Closes #39458

I'm not able to test it, i would appreciate if somebody could do it. I think this bug was present also for SSH remote projects

Release Notes:

Fixed an issue where zed bin was not found in remote servers for askpass

…utable itself Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

notruri · 2025-10-03T16:06:19Z

lgtm

Zed: v0.208.0 (Zed Dev ed651ea3a0a9d2d7f298d7ea2d62ebc639e8711d) (Taylor's Version)
OS: Windows 10.0.26100
Architecture: x86_64

zed cli helper now works within WSL

maxbrunsfeld

@marcocondrache Thanks so much for tracking this down. Nice work.

I would love to solve this in a way that doesn't rely on checking the filename of the current exe.

Here's one way to fix it that I think is cleaner. Let me know if this makes sense to you:

We should put back the --askpass argument in the main editor on linux and macOS.
There should be a static variable in ask_pass.rs called ASKPASS_PROGRAM (a PathBuf) - the path to the program that should be invoked for askpass. By default, ASKPASS_PROGRAM should be initialized to the current_exe(). On mac and Linux, and in the remote server, it should just be left that way.
The askpass module should have a public setter function called set_askpass_program that can override the ASKPASS_PROGRAM. We should only call that function on Windows, in zed::main (passing in the path to the CLI).

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

marcocondrache · 2025-10-04T06:42:13Z

@maxbrunsfeld, thanks for the feedback! That makes perfect sense, and it's a cleaner approach. I will make the changes immediately

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

marcocondrache · 2025-10-04T08:51:51Z

@maxbrunsfeld I’ve updated the PR with the changes. I moved the logic for turning a path into a shell-safe string into the PathExt trait, since it felt generic enough to live there but if you think it makes more sense somewhere else, I’m happy to move it.

I’m also not too sure about the get_zed_cli_path function since it's called for set_askpass_program. Maybe it’d be simpler to inline that directly in zed::main, but I’ll let you guide what feels best here.

For testing, I can only try things out on macOS. I’ll give it a spin there, but unfortunately I won’t be much help on Linux or Windows 😞

osiewicz · 2025-10-04T11:02:33Z

crates/askpass/src/askpass.rs

    }
 }

+pub fn set_askpass_program(path: std::path::PathBuf) {


nit: conventionally in Zed we use a fn init() function for initialization of globals related to a given module. That function also resides closer to the top of a file.

Suggested change

pub fn set_askpass_program(path: std::path::PathBuf) {

pub fn init(path: std::path::PathBuf) {

I think calling it init might be misleading in this case, since it only runs on Windows. Making it run unconditionally could also introduce security concerns - on Unix platforms, current_exe should be called right before use to avoid potential privilege escalation.

Fair, we can leave it as is then.

osiewicz · 2025-10-04T11:03:53Z

crates/askpass/src/askpass.rs


+pub fn set_askpass_program(path: std::path::PathBuf) {
+    if ASKPASS_PROGRAM.set(path).is_err() {
+        panic!("askpass program has already been set");


That could prolly be a debug_panic. We don't want to bring the whole app down, because - while multiple distinct values for ASKPASS_PROGRAM don't make sense - panicking turns a benign bug into a hard crash, even if you don't end up using any remoting functionalities.

Yep that's super right. I missed the debug_panic macro

osiewicz · 2025-10-04T11:04:49Z

crates/util/src/paths.rs

+
+    /// Try to make a shell-safe representation of the path.
+    ///
+    /// For Unix, the path is escaped to be safe for POSIX shells


This could be a doc comment on the trait method itself, not on it's implementation.

All the methods of this trait have the doc comment on the implementations. Should i move all of them?

Yeah, I think that'd be best.

I've moved the doc comments to the trait definition

osiewicz · 2025-10-04T11:05:38Z

crates/util/src/util.rs

+        .to_string())
 }

 /// Returns a shell escaped path for the zed cli executable, this function


Nit: is this doc comment outdated? The returned path is no longer shell-escaped.

Yep, i've missed that

crates/askpass/src/askpass.rs

crates/util/src/paths.rs

osiewicz · 2025-10-04T11:10:31Z

crates/util/src/paths.rs

+            // As of writing, this can only be fail if the path contains a null byte, which shouldn't be possible
+            // but shlex has annotated the error as #[non_exhaustive] so we can't make it a compile error if other
+            // errors are introduced in the future :(
+            Ok(shlex::try_quote(path_str)?.to_string())


Suggested change

Ok(shlex::try_quote(path_str)?.to_string())

Ok(shlex::try_quote(path_str)?.into())

I'm not sure if that'll compile from the get-go, but to_string can be slower than String::from. Not that it matters though, so feel free to disregard.

That's a good catch. The method returns Cow, so into() could indeed be faster. I've made the change

Ah, in that case you can even go the extra mile and use into_owned.

That's right. Done

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

osiewicz

LGTM! Will give it a go with other platforms before merging.

maxbrunsfeld · 2025-10-04T22:13:17Z

Thanks so much for the follow ups @marcocondrache

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

marcocondrache · 2025-10-05T07:57:18Z

@maxbrunsfeld @osiewicz thank you for the support!

osiewicz · 2025-10-06T22:39:49Z

I've tested this on Windows -> Linux and Mac -> Linux SSH scenarios. In both cases it fixed git usage.

The author has applied Max's feedback + I've manually tested the code.

osiewicz · 2025-10-08T16:23:13Z

/cherry-pick v0.207.x

Closes #39469 Closes #39438 Closes #39458 I'm not able to test it, i would appreciate if somebody could do it. I think this bug was present also for SSH remote projects Release Notes: - Fixed an issue where zed bin was not found in remote servers for askpass --------- Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

…s#39475) Closes zed-industries#39469 Closes zed-industries#39438 Closes zed-industries#39458 I'm not able to test it, i would appreciate if somebody could do it. I think this bug was present also for SSH remote projects Release Notes: - Fixed an issue where zed bin was not found in remote servers for askpass --------- Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

marcocondrache added 2 commits October 3, 2025 17:29

fix: when askpass is executed in remote server we should use the exec…

eb8858e

…utable itself Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: this should be unix safe

6601293

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 3, 2025

chore: this should make it platform agnostic

ed651ea

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

maxbrunsfeld requested review from JunkuiZhang and osiewicz October 3, 2025 23:51

maxbrunsfeld previously requested changes Oct 4, 2025

View reviewed changes

fix: restore askpass argument

034ace6

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

marcocondrache added 2 commits October 4, 2025 09:54

fix: use static variable and extract shell safe logic

4943644

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: retrieve cli path

7322f3d

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

osiewicz reviewed Oct 4, 2025

View reviewed changes

crates/askpass/src/askpass.rs Outdated Show resolved Hide resolved

osiewicz reviewed Oct 4, 2025

View reviewed changes

crates/askpass/src/askpass.rs Outdated Show resolved Hide resolved

osiewicz reviewed Oct 4, 2025

View reviewed changes

crates/util/src/paths.rs Outdated Show resolved Hide resolved

osiewicz reviewed Oct 4, 2025

View reviewed changes

marcocondrache added 5 commits October 4, 2025 14:00

fix: comments

19f9bd1

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: missing log crate

3be08ad

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: remove trim

ea769e2

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: clippy

1bdf7de

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

fix: machete

c7d6b6e

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

osiewicz approved these changes Oct 4, 2025

View reviewed changes

fix: comments

de87b18

Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>

SomeoneToIgnore assigned osiewicz Oct 5, 2025

Hiryxx mentioned this pull request Oct 6, 2025

git push from UI doesn't work on Linux development builds #39579

Closed

osiewicz enabled auto-merge (squash) October 6, 2025 22:31

osiewicz merged commit 4de13e0 into zed-industries:main Oct 6, 2025
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

askpass: Fix cli path when executed in a remote server #39475

askpass: Fix cli path when executed in a remote server #39475

Uh oh!

marcocondrache commented Oct 3, 2025

notruri commented Oct 3, 2025

maxbrunsfeld left a comment

marcocondrache commented Oct 4, 2025

marcocondrache commented Oct 4, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 4, 2025

osiewicz Oct 4, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 4, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 4, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 5, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 4, 2025

Uh oh!

Uh oh!

Uh oh!

osiewicz Oct 4, 2025

marcocondrache Oct 4, 2025

osiewicz Oct 4, 2025

marcocondrache Oct 5, 2025

osiewicz left a comment

maxbrunsfeld commented Oct 4, 2025

marcocondrache commented Oct 5, 2025

osiewicz commented Oct 6, 2025

Uh oh!

osiewicz commented Oct 8, 2025

Labels

4 participants

	pub fn set_askpass_program(path: std::path::PathBuf) {
	pub fn init(path: std::path::PathBuf) {

	Ok(shlex::try_quote(path_str)?.to_string())
	Ok(shlex::try_quote(path_str)?.into())

Uh oh!

askpass: Fix cli path when executed in a remote server #39475

askpass: Fix cli path when executed in a remote server #39475

Uh oh!

Conversation

marcocondrache commented Oct 3, 2025

notruri commented Oct 3, 2025

maxbrunsfeld left a comment

Choose a reason for hiding this comment

marcocondrache commented Oct 4, 2025

marcocondrache commented Oct 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osiewicz left a comment

Choose a reason for hiding this comment

maxbrunsfeld commented Oct 4, 2025

marcocondrache commented Oct 5, 2025

osiewicz commented Oct 6, 2025

Uh oh!

osiewicz commented Oct 8, 2025

Labels

4 participants