Sdkman-cli: candidate paths are prepended to PATH

[blueglyph]

We require a conversation to take place in the appropriate Gitter Room before raising a new Github Issue. Please note that issues will be closed immediately if prior discussion did not take place. We need to do this to help manage the quality and validity of Issues raised on this project.

Please tick one:

• [ ] this is a new Feature Request, a conversation was started in the Gitter CLI Development Room.
• [ ] this is a new Bug Report or Issue, a conversation was started in Gitter User Issues Room.

Please explain the Issue / Feature Request here:

(no discussion required, the bug is obvious)

Version SDKMAN 5.7.3+337

The path of the installed candidates are put in front of the shell PATH variable, which is bad practice.

The problem is due to the __sdkman_prepend_candidate_to_path function in skdman-path-helpers.sh, line 73:

echo "$PATH" | grep -q "$candidate_dir" || PATH="${candidate_bin_dir}:${PATH}"

It can be fixed by adding the path to the end of the list to give the expected behaviour, like so:

echo "$PATH" | grep -q "$candidate_dir" || PATH="${PATH}:${candidate_bin_dir}"

Please don't put application's path first as it may disrupt the normal functioning of the shell!

[marc0der]

Really? A bug? Could you provide a proof / link as citation that it is _bad practice_ as you say?

Please follow the guidelines stated above as everyone else does.

[blueglyph]

If it's not a bug, it's a mistake (the line between the two can indeed be very blurry). Prepending paths introduces a vulnerability as someone could add fake executables in the new directories, it is also a risk of shadowing the legit OS commands.

Check for example here:
https://unix.stackexchange.com/questions/26047/how-to-correctly-add-a-path-to-path

Please note also that it is best to add this in .profile and not .bashrc. For an explanation, please check for example here:
https://superuser.com/questions/183870/difference-between-bashrc-and-bash-profile/183980

[marc0der]

Nope, no mistake, it is by design. We purposefully want our path entries to have precedence over existing ones in user space. Take JAVA_HOME for instance. Many systems export JAVA_HOME, as part of the PATH which would mean that sdkman's path entry would never be picked up. Now _that_ would be a bug.

As for your argument about vulnerabilities, a fake executable could make it's way _anywhere_ on your system, even in the system directories. Heck, they could even make their way into your ~/bin folder which usually always has precedence over the rest of the path entries.

Lastly, we don't distribute any binaries that could shadow system binaries (except say java and javac which we purposefully _want_ to shadow).

For an explanation why we don't use .profile (which is typically used for login shells) and instead use .bashrc and .zshrc, see the feature specification.

[eladchen]

This is definitely wrong - If during an installation phase a certain env variable / binary exists it should be noted as a warning to the user, and presumably prefixed (HomeBrew style)

Most system don't have 99% of the SDK's, sure Java is common but that's just about it.
Just as HomeBrew doesn't prepend anything to the $PATH and is still widely used, so should this tool, but seeing as you're the author it is entirely up to you.

BTW Here's an example: Currently there is no Java candidate for Oracle's 8 JDK
Even when I installed it manually, my system kept looking for JAVA_HOME set by this tool.
If the binaries path were appended instead, there would be no problem.

[marc0der]

@eladchen a simple solution for you would be to do a local install of this external version, making sdkman aware of it and thus also managing its home variable.

Also, there is another issue open about making sdkman aware of system installed versions. It just hasn't been implemented yet.

[eladchen]

Thanks for the tip.

I think it will be nice if there was a reference to that issue in this one.

[marc0der]

Sure, the issue in question is #673.