-
Notifications
You must be signed in to change notification settings - Fork 176
Add environment variable with installer file path #1151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
news/1151-installer-path-variable
Outdated
| @@ -0,0 +1,19 @@ | |||
| ### Enhancements | |||
|
|
|||
| * EXE: An environment variable `INSTALLER_PATH` is now defined for `post_install` scripts and set to the path of installer executable while the installer is running. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * EXE: An environment variable `INSTALLER_PATH` is now defined for `post_install` scripts and set to the path of installer executable while the installer is running. | |
| * EXE: An environment variable `INSTALLER_PATH` is now defined for `post_install` scripts and set to the path of the installer executable while the installer is running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marcoesters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding that variable to the SH and PKG installers, too, while we're at it.
constructor/nsis/main.nsi.tmpl
Outdated
| Push $R2 | ||
|
|
||
| {%- if post_install_exists %} | ||
| System::Call 'Kernel32::SetEnvironmentVariable(t, t) i("INSTALLER_PATH", "$EXEPATH")' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add this to where the other variables are defined in the Install section, and not make this a condition for post-install. It could also be used in pre-install scripts, maybe.
PLUGINSDIR may also be a good variable to add here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved it and added INSTALLER_PLUGINSDIR, see f8da863.
I also considered if we should have it as:
...
SetEnvironmentVariable INSTALLER_PLUGINSDIR...
{%- if pre_install_exists or post_install_exists %}
SetEnvironmentVariable INSTALLER_PATH...
{%- endif %}
Thoughts? It doesn't hurt having INSTALLER_PATH always defined, but I also wonder if it is unnecessary, is it always safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be unsafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any reason, but I am bringing it up just for consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an attack vector right now. The environment variable is not available outside the installation process. So, they can only be used through in scripts. Apart from the pre-install or post-install scripts of the installer, those are the pre-link or pre-unlink scripts of the packages.
However, even if someone were to place a malicious package into an installer, this doesn't give the attacker any more privileges that they'd already have. So, the limitation won't reduce the overall exposure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, good assessment!
constructor/header.sh
Outdated
| case "$0" in | ||
| */*) SCRIPT_PATH="$0" ;; | ||
| *) SCRIPT_PATH="$(command -v -- "$0" 2>/dev/null)" ;; | ||
| esac | ||
|
|
||
| [ -z "$SCRIPT_PATH" ] && { | ||
| echo "ERROR: Cannot determine installer path" | ||
| exit 1 | ||
| } | ||
| # Resolve the directory to an absolute path | ||
| SCRIPT_DIR=$(cd -- "$(dirname -- "$SCRIPT_PATH")" && pwd) | ||
| INSTALLER_PATH="$SCRIPT_DIR/$(basename -- "$SCRIPT_PATH")" | ||
| export INSTALLER_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For visibility, note that this is based on a proposed AI solution and is said to be robust for both linux/mac with a #!/bin/sh shebang. I simplified it a bit because I removed an assumption to follow symlinks, I don't think that is necessary in this case?
It also is a bit extra because it is trying to determine if the script was invoked using ., source or if the script was on $PATH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script location is already a local variable, so you can just reuse that:
constructor/constructor/header.sh
Line 89 in dccf7d6
| THIS_FILE=$(basename "$0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I noticed the entire path was already defined, I removed the earlier implementation and simply reuse the existing one: 8cf48d3
|
|
||
| # PACKAGE_PATH is poorly documented, and might not exist across all OS versions | ||
| if [ -n "${PACKAGE_PATH:-}" ]; then | ||
| export INSTALLER_PATH="$PACKAGE_PATH" # The path to the .pkg installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add a default value? What we are trying to achieve here doesn't seem to be well-supported by Apple for .pkg installers, however, I found this and it worked for me when I tested locally. However, it's a poorly documented variable and might not always work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, it's a poorly documented variable
That is, unfortunately, not unusual. This code may fail though with set -u. Here is how I solved it for another variable:
constructor/constructor/osx/run_user_script.sh
Lines 33 to 40 in dccf7d6
| # The value for COMMAND_LINE_INSTALL is not documented, | |
| # but it is unset for interactive installations. To be | |
| # safe, set the variable for non-interactive installation | |
| # in a roundabout way. | |
| export INSTALLER_UNATTENDED="${COMMAND_LINE_INSTALL:-0}" | |
| if [[ "${INSTALLER_UNATTENDED}" != "0" ]]; then | |
| INSTALLER_UNATTENDED="1" | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a default variable makes sense here. If the location of the installer cannot be detected because the variable is not defined, then it shouldn't be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the 0 is necessary, I used the :- syntax as well (if [ -n "${PACKAGE_PATH:-}" ]; then) but this situation evaluates to an empty string if unset.
news/1151-installer-path-variable
Outdated
| An environment variable `INSTALLER_PATH` is now defined for `post_install` scripts and set to the path of the installer executable while the installer is running. | ||
| * EXE: An environment variable `INSTALLER_PLUGINSDIR` is now also defined, it serves the same purpose as the NSIS variable `PLUGINSDIR`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| An environment variable `INSTALLER_PATH` is now defined for `post_install` scripts and set to the path of the installer executable while the installer is running. | |
| * EXE: An environment variable `INSTALLER_PLUGINSDIR` is now also defined, it serves the same purpose as the NSIS variable `PLUGINSDIR`. | |
| * An environment variable `INSTALLER_PATH` is now defined for pre-install and post-install scripts, and set to the path of the installer executable while the installer is running. (#1151) | |
| * EXE: An environment variable `INSTALLER_PLUGINSDIR` is now also defined, it serves the same purpose as the NSIS variable `$PLUGINSDIR`. (#1151) |
Please always add the PR number (and issue number, not applicable here) to the new items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I keep forgetting that. Thanks for the reminder!
Co-authored-by: Marco Esters <[email protected]>
Description
This PR adds an environment variable
INSTALLER_PATHfor installers that havepost_install: ...defined.Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?