Fix for Bash env script injection on RPM family distros #1189
Conversation
|
Oh wow, I super appreciate the research you did into this! |
| exit1=$? | ||
| add_install_dir_to_path "$_install_dir_expr" "$_env_script_path" "$_env_script_path_expr" ".bash_profile .bash_login .bashrc" "sh" | ||
| shotgun_install_dir_to_path "$_install_dir_expr" "$_env_script_path" "$_env_script_path_expr" ".bash_profile .bash_login .bashrc" "sh" |
There was a problem hiding this comment.
I believe this slightly changes behaviour here. Currently we will create .bash_profile if none of these files exist, and with shotgun we no long detect this case. Is that intentional?
There was a problem hiding this comment.
No, it's not - good catch, thanks! I'll fix that.
| add_install_dir_to_path "$_install_dir_expr" "$_env_script_path" "$_env_script_path_expr" "$_rcfile_relative" "$_shell" | ||
| fi | ||
| done | ||
|
|
||
| if [ "$_found" = false ]; then | ||
| add_install_dir_to_path "$_install_dir_expr" "$_env_script_path" "$_env_script_path_expr" "$(echo "$_rcfiles" | awk '{ print $1 }')" "$_shell" |
There was a problem hiding this comment.
What does this extra awk accomplish?
There was a problem hiding this comment.
Oops - nothing, that was left over from the check I pasted from add_install_dir_to_path before I checked and found that var=bool is dash/POSIX compliant.
|
Hmm interesting it looks like rustup currently in fact doesn't create a bash file if they're all missing, so maybe your original commit should be desired behaviour? (It does unconditionally create .profile) |
|
I can definitely see an argument for that. Should we make the other shells behave similarly, in that case? I did notice that the Atuin installer was creating a |
|
Alternately - I was going to ask what you thought about making |
|
This is really interesting, it seems like rustup has tightened up how hard it shotguns since last i checked. It means that someone who installs zsh later won't see cargo on path, hmmm... |
|
(Gonna consult with coworkers on what we wanna do here) |
|
Thanks for this PR! It's a big help.
macOS's terminal uses login shells by default, so we do want to plan around it. (I don't even have a I think, reading up on what rustup does, that their logic makes some sense. Plus, if we stop creating a file by default, we can completely dodge the "login shell or not login shell" question. The "should we create |
|
Interesting, I didn't know that about macOS. With respect to |
|
Sounds good! In that case, how about we keep what you've written except we change the order so the "write to the first in the list" is |
|
Sure! Do you think we need to alter the behavior for the other shells at all? I'd need to double check, but I think ZSH etc. all match |
|
Double checked, and the existing logic for POSIX/ |
commit f322c7cb6f10d52bb6df0259de82b827bdd6df3b
Author: James Haywood <jameshaywood@fastmail.com>
Date: Sat Jul 20 13:38:18 2024 +0000
> squash commits
> CI failure jumpscare
many such cases
commit 64e187b5c413bb72f115c7ed2d98151981c9d9b1
Merge: 65c6c517 f18f683
Author: James Haywood <jameshaywood@fastmail.com>
Date: Sat Jul 20 13:35:24 2024 +0000
Merge branch 'main' into bash-fix
commit 65c6c51734b1db552688d761e2c93226523dade8
Author: James Haywood <jameshaywood@fastmail.com>
Date: Mon Jul 8 10:23:27 2024 +0800
Fix compatibility issues with Bash on certain distributions (Fedora, EL)
Ensure a file is created if none are found
Remove spurious awk usage
Make .profile the fallback option for Bash
Silence CI, mv $file (basename $file .new) is speaking
|
Thank you :D |
Resolves #1187 - this is a quick fix I hacked up that piggybacks on the existing
add_install_dir_to_pathfunction.It works well in a Fedora container, but shell scripting is not my strong suit - I'll gladly cook up something different if there's a footgun I overlooked.