feat: fix path injection on macos 15 #841
Conversation
Library/EmacsBase.rb
Outdated
| # Create wrapper script | ||
| File.open(emacs_binary, "w") do |f| | ||
| f.write <<~EOS | ||
| #!/bin/bash |
There was a problem hiding this comment.
| #!/bin/bash | |
| #!/usr/bin/env bash |
I think this would be a better idea to allow newer bash versions as well.
There was a problem hiding this comment.
not sure there is difference; if Emacs is run via launchctl (finder, spotlight, etc), it will be built-in bash anyways
this actually makes me wonder if we need to provide an env variable to skip setting PATH here - for cases when you want run Emacs from terminal, but... oh maybe I also need to make emacs bin to run original Emacs executable instead of this modified version.
There was a problem hiding this comment.
oh fair point! I guess I just knee jerk look for that shebang lol. I agree having an opt out knob in the form of something like EMACS_PLUS_NO_PATH_INJECTION even if overly verbose is a good idea!
There was a problem hiding this comment.
cool, I am on a party right now, so not super focused
hence - all feedback and comments are appreciated even more than usually ❤️
p.s. pushed ability to dynamically disable path injection
There was a problem hiding this comment.
It would probably be more correct here to use #!/bin/sh . macOS does unfortunately depend on bash pretty closely, but /bin/sh is a slightly leaner and more standards-compliant build of bash which will make startup a little faster. Apple has been (extremely slowly, since it's such a core system component) moving away from bash by steering users to zsh as the default login shell, so we can expect it'll go away eventually.
There was a problem hiding this comment.
Nice! I really appreciate the work keeping this project alive for emacs on macOS. Thanks for all the time you spend on it! PR LGTM!
There was a problem hiding this comment.
I would suggest the shell-quoting issues be addressed before landing this, because those could potentially provoke very confusing crashes for users who happen to have odd characters in their $PATH.
This would also be a good opportunity to simply drop this functionality, as launchctl config user path (introduced in Yosemite; 2017) has existed for almost as long as emacs-plus itself (2016) for users who want a UNIX path that matches their Homebrew path for all applications.
For my part I just don't want a potentially buggy shell script intercepting the launch process, and it looks like the emacs-app cask has added all the stuff I wanted from this project by now, so this should be my last comment on the subject. Thanks for all the work over the years delivering Emacs features to Mac users earlier than upstream wants to, and I hope you work out a strategy that works well for all your users!
| app = "#{prefix}/Emacs.app" | ||
| plist = "#{app}/Contents/Info.plist" | ||
| emacs_binary = "#{app}/Contents/MacOS/Emacs" | ||
| emacs_real = "#{app}/Contents/MacOS/Emacs-real" |
There was a problem hiding this comment.
Computing the absolute path here makes the app bundle non-relocatable. No need to introduce more problems here.
There was a problem hiding this comment.
to clarify, what I mean is that the script can itself compute a relative path internally, with something like "$(dirname $0)/Emacs-real", which avoids the need for having emacs_real defined at all in Ruby.
Library/EmacsBase.rb
Outdated
| f.write <<~EOS | ||
| #!/bin/bash | ||
| if [ -z "$EMACS_PLUS_NO_PATH_INJECTION" ]; then | ||
| export PATH="#{path}" |
There was a problem hiding this comment.
This is a potential injection/quoting bug. It's possible to have path names with $ or other shell metacharacters in them, which would cause this path value to get interpreted as a double-quoted string would. You're also not defending against the possibility that there are double-quotes in the path itself.
I'd recommend adding shell-quoting logic here as well as switching to single rather than double quotes to avoid the possibility of a stray path character injection.
There was a problem hiding this comment.
In general, I agree with you. The PATH is captured at BUILD time, not from user input at runtime. This is trusted data. If someone's build-time PATH contains malicious characters, they have much bigger problems. This isn't really a "security vulnerability" - it's more of a "robustness against unusual paths" issue (like paths with $ or spaces).
So I guess the following should be enough (basically, repeating your words):
- export PATH="#{path}"
+ export PATH='#{path}'
There was a problem hiding this comment.
So I guess the following should be enough (basically, repeating your words):
Not quite. On the ruby side you need to quote any potential single-quotes first, in case somebody has something like "Jim's Shell Scripts" on their $PATH.
This whack-a-mole of silly, annoying edge cases is exactly why generating shell scripts is a very annoying implementation technique. But if you're using single-quoted strings, it's basically just this one; there's nothing else you need to quote, even newlines are covered.
Still, it might be worth considering something like a custom site-start.el rather than replacing the executable mainpoint? That still happens early enough in startup that I think it could address your concerns about being initialized early. You could put the path into ~/.emacs-plus-custom-path and not worry about quoting at all?
Library/EmacsBase.rb
Outdated
| if [ -z "$EMACS_PLUS_NO_PATH_INJECTION" ]; then | ||
| export PATH="#{path}" | ||
| fi | ||
| exec "#{emacs_real}" "$@" |
There was a problem hiding this comment.
If the users's homebrew path has a shell metacharacter in it you'll trigger the same kinds of bugs as above.
There was a problem hiding this comment.
Ah, disregard, it was about original exec "#{emacs_real}" "$@" and not the proposed code. I will update PR after I test that it works.
Not sure I understand you. Are you talking about a situation when someone installed Homebrew in a path like:
- /path with spaces/homebrew
- /foo$bar/homebrew
- /some/path
withbackticks/homebrew
If that's what you are talking about, then let's trace through what happens:
$0= path to the wrapper script (e.g.,/opt/homebrew/.../Emacs)dirname "$0"= directory containing the script$(dirname "$0")/Emacs-real= constructs the path toEmacs-real- The whole thing is wrapped in
"..."(double quotes)
The double quotes around the entire "$(dirname "$0")/Emacs-real" protect against spaces and most special characters. The inner "$0" also protects the path when passed to dirname.
So are you concerned about extremely exotic cases like:
- Paths with newlines
- Paths with $ that might expand inside the double quotes
?
Honestly, if someone's Homebrew prefix contains shell metacharacters, everything would break, not just Emacs. So this sounds more like a theoretical concern.
Or am I missing something?
Library/EmacsBase.rb
Outdated
| # Create wrapper script | ||
| File.open(emacs_binary, "w") do |f| | ||
| f.write <<~EOS | ||
| #!/bin/bash |
There was a problem hiding this comment.
It would probably be more correct here to use #!/bin/sh . macOS does unfortunately depend on bash pretty closely, but /bin/sh is a slightly leaner and more standards-compliant build of bash which will make startup a little faster. Apple has been (extremely slowly, since it's such a core system component) moving away from bash by steering users to zsh as the default login shell, so we can expect it'll go away eventually.
| To disable PATH injection, set EMACS_PLUS_NO_PATH_INJECTION before running Emacs: | ||
| export EMACS_PLUS_NO_PATH_INJECTION=1 |
There was a problem hiding this comment.
If users are primarily GUI users who don't want this, export won't do the job, because it will only apply to shells that source this value, and GUI users aren't launching emacs from the shell most of the time; you should document this as creating a launchd plist that will run launchctl setenv EMACS_PLUS_NO_PATH_INJECTION 1 to add this environment variable.
Or, if you're going to monkey with launchd configuration anyway, why not instruct the user to use launchctl config user path to actually set the path for all applications, not just Emacs, because that's what you're actually trying to do, and this functionality doesn't belong in Emacs?
There was a problem hiding this comment.
Hey @glyph,
Thanks for all your helpful comments. I'll respond to the others when I have more time, but wanted to address this specific point straight away.
I think we already had a good discussion on this topic in #578.
Or, if you're going to monkey with launchd configuration anyway, why not instruct the user to use launchctl config user path to actually set the path for all applications, not just Emacs, because that's what you're actually trying to do, and this functionality doesn't belong in Emacs?
The key difference is that other applications don't need this. Emacs with native compilation enabled by default requires specific environment setup to function properly. Whilst environment setup isn't strictly part of the Emacs distribution, the combination of how Emacs is designed and how macOS operates means we get an Emacs.app that doesn't work out-of-the-box without additional configuration.
launchctl config user path is system-wide configuration that affects all GUI applications and requires elevated privileges. That's far too broad a solution for what is specifically an Emacs requirement.
Power users who need different behaviour can disable PATH injection via EMACS_PLUS_NO_PATH_INJECTION, and as I mentioned in #578, exec-path-from-shell remains the most reliable solution for users with complex, dynamic PATH requirements. Everything else we're doing here is about making Emacs.app work out-of-the-box for the majority of users - which aligns with Emacs+'s goal of being a batteries-included distribution.
There was a problem hiding this comment.
I wonder if it should be disableable during installation, rather than when running. That way people could opt to not have a shell script (and the potentially extra startup time) involved.
No description provided.