installer: harden the Node provisioning path against its own inputs
ci/woodpecker/pr/ci Pipeline is pending approval
ci/woodpecker/pr/ci Pipeline is pending approval
Answers the review on #1228. Each item below was measured against the pre-change code, and where the review's stated consequence did not reproduce, that is recorded rather than repeated. BLOCKER -- `mapfile` is a Bash 4 builtin and macOS ships Bash 3.2, which this installer supports (node_platform names Darwin). newest_matching_file was therefore unavailable on macOS, and an empty answer is exactly what sends the uninstaller down its delete-the-destination branch. The lookup no longer renders candidates as text at all: the glob output is compared in-shell by mtime, via a stat helper that probes for GNU -c vs BSD -f once. That removes the Bash 4 dependency, the `ls | head` SIGPIPE failure, and the newline-splitting bug together, because all three came from turning filenames into lines. The function now distinguishes three outcomes instead of two: found, nothing matched, and could-not-tell. Callers act destructively on the answer, so the third case had to stop being indistinguishable from the second. The uninstaller leaves the file in place on an unanswerable lookup, and the manifest builder refuses to record a null backup it cannot vouch for. HIGH -- writing ~/.profile does not reach the shells that matter. A bash login shell reads the first of .bash_profile / .bash_login / .profile that exists and never looks at the rest, so on a host with either of the first two the entry was a silent no-op; a non-interactive remote zsh reads .zshenv and neither .zprofile nor .zshrc, which is what the previous version wrote; and a systemd --user unit reads no shell file at all, which is how a Mosaic agent seat starts. All four are now covered, with .bash_profile and .bash_login appended to only when they already exist -- creating one would itself start shadowing .profile. The systemd case is an environment.d drop-in. MEDIUM -- the checksum lookup interpolated the filename into a grep pattern. A Node tarball name is mostly dots, and a dot matches any character, so a manifest line for a different-but-regex-equivalent name was accepted as this file's checksum. Confirmed against the old function: it accepted the decoy. Filenames are now compared exactly, every line is read so a duplicate entry is refused rather than silently resolved, and the digest must look like a SHA-256. MEDIUM -- the PATH line is executed by every future shell that reads the file, and the directory was interpolated unescaped. A path containing shell syntax is now refused with a message instead of written. MEDIUM -- the idempotence check was an unanchored substring match, so a commented-out example of the same export made the installer skip the real entry. Reproduced against the old function, and now anchored with grep -Fqx. MEDIUM -- MOSAIC_NODE_DIST accepted any scheme. https:// and file:// only. The narrower point in the review stands and is not fixed by this: when the dist is overridden, the tarball and the checksum that vouches for it come from the same place, so the gate is integrity and not authenticity. HIGH, with a correction -- MOSAIC_NODE_VERSION is now validated before it becomes a path, but the review's specific consequence does not reproduce. `rm -rf` on a path ending in `..` is refused by rm itself, and a traversal version mangles the download URL so the run dies at curl long before the removal. Both were measured. The check is defence in depth and a clearer error, not a demonstrated hole being closed. Also removed a second `| head -1` in node_resolve_version, the same SIGPIPE shape as the one this PR already fixed, and the index result is validated before it becomes a path. Tests. The review was right that several existing cases passed on the unpatched code. The version-selection case now lists a higher major first and an older release of the right major after the right answer, so "first entry" and "last match" both fail it. The PATH case starts a real login shell and asks it to resolve node, rather than grepping for text the installer just wrote. The checksum-failure case asserts nothing survives, including the staging directory. New cases cover the empty manifest, the regex-equivalent decoy, the duplicate entry, the invalid version, the non-https dist, the shell-syntax path, the commented-out profile line, the .bash_profile shadow, and the environment.d drop-in. Each new case was run against the pre-change installer: the decoy, the commented-out line, the .bash_profile shadow and environment.d all go red there, which is the evidence that they test something. Bash 3.2 cannot be executed here, so the portability guard is a lint over install.sh for Bash 4 syntax. It is a weaker instrument than a run and is not claimed otherwise -- but every Bash 4 construct that has broken macOS in this file was added by someone who was not running it there either. test:installer passes.
This commit is contained in:
@@ -42,8 +42,10 @@ case "$(uname -m)" in
|
||||
esac
|
||||
PLATFORM="${TEST_OS}-${TEST_ARCH}"
|
||||
|
||||
VERSION=v22.99.0
|
||||
OLD_VERSION=v20.99.0
|
||||
VERSION=v22.99.0 # the one that must be chosen
|
||||
MID_VERSION=v22.50.0 # same major, older -- catches "take the last match"
|
||||
OLD_VERSION=v20.99.0 # wrong major
|
||||
NEWER_MAJOR=v24.99.0 # listed first -- catches "take the first entry"
|
||||
|
||||
# ─── fixtures ─────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -130,9 +132,14 @@ publish_release() {
|
||||
}
|
||||
|
||||
publish_release "$VERSION"
|
||||
publish_release "$MID_VERSION"
|
||||
publish_release "$OLD_VERSION"
|
||||
# Newest-first, as nodejs.org publishes it.
|
||||
printf '[{"version":"%s"},{"version":"%s"}]\n' "$VERSION" "$OLD_VERSION" > "$DIST/index.json"
|
||||
publish_release "$NEWER_MAJOR"
|
||||
# Newest-first, as nodejs.org publishes it. Every wrong entry is genuinely installable,
|
||||
# so a resolver that picks one fails on the assertion rather than on a 404 -- the
|
||||
# assertion is then about version selection and not about the fixture.
|
||||
printf '[{"version":"%s"},{"version":"%s"},{"version":"%s"},{"version":"%s"}]\n' \
|
||||
"$NEWER_MAJOR" "$VERSION" "$MID_VERSION" "$OLD_VERSION" > "$DIST/index.json"
|
||||
|
||||
# A PATH with the usual tools but no Node toolchain, so "a host with no Node" is
|
||||
# actually true on a developer machine and in CI, both of which have one installed.
|
||||
@@ -182,7 +189,12 @@ grep -qF -- "Installed Node ${VERSION}" <<<"$OUTPUT"
|
||||
[[ -x "${NODE_HOME}/${VERSION}/bin/node" ]]
|
||||
grep -qF -- "install -g @mosaicstack/[email protected]" "$LOG"
|
||||
|
||||
echo "[test] the newest matching major is chosen, not merely the first published"
|
||||
echo "[test] the newest release of the required major is chosen"
|
||||
# The index lists a higher major first and an older release of the right major after
|
||||
# the right answer, so "first entry" and "last match" both produce a wrong directory.
|
||||
[[ -d "${NODE_HOME}/${VERSION}" ]]
|
||||
[[ ! -d "${NODE_HOME}/${NEWER_MAJOR}" ]]
|
||||
[[ ! -d "${NODE_HOME}/${MID_VERSION}" ]]
|
||||
[[ ! -d "${NODE_HOME}/${OLD_VERSION}" ]]
|
||||
|
||||
echo "[test] future shells can find both Node and the CLI"
|
||||
@@ -193,11 +205,54 @@ grep -qF -- "export PATH=\"${PREFIX}/bin:\$PATH\"" "$HOME_DIR/.profile"
|
||||
grep -qF -- "export PATH=\"${NODE_HOME}/${VERSION}/bin:\$PATH\"" "$HOME_DIR/.bashrc"
|
||||
grep -qF -- "export PATH=\"${PREFIX}/bin:\$PATH\"" "$HOME_DIR/.bashrc"
|
||||
|
||||
echo "[test] a real login shell resolves node, not just the text of a profile line"
|
||||
# Grepping the file only proves the installer wrote something. This starts an actual
|
||||
# login shell against that HOME and asks it to find the binary.
|
||||
RESOLVED="$(env -i HOME="$HOME_DIR" PATH="$NONODE_BIN" TERM=dumb bash -lc 'command -v node')"
|
||||
[[ "$RESOLVED" == "${NODE_HOME}/${VERSION}/bin/node" ]] || {
|
||||
echo "a login shell resolved node to '${RESOLVED}'" >&2
|
||||
exit 1
|
||||
}
|
||||
|
||||
echo "[test] a systemd --user unit gets the same PATH, via environment.d"
|
||||
# Units read no shell file at all, which is how a Mosaic agent seat starts.
|
||||
ENVD="$HOME_DIR/.config/environment.d/50-mosaic-path.conf"
|
||||
[[ -f "$ENVD" ]] || { echo "no environment.d drop-in was written" >&2; exit 1; }
|
||||
grep -qF -- "PATH=${NODE_HOME}/${VERSION}/bin:\${PATH}" "$ENVD"
|
||||
grep -qF -- "PATH=${PREFIX}/bin:\${PATH}" "$ENVD"
|
||||
|
||||
echo "[test] re-running reuses the Node it installed and does not duplicate PATH lines"
|
||||
OUTPUT="$(run_bare --cli --next --yes --no-auto-launch 2>&1)"
|
||||
grep -qF -- "from ${NODE_HOME}" <<<"$OUTPUT"
|
||||
[[ "$(grep -c 'export PATH=' "$HOME_DIR/.profile")" -eq 2 ]]
|
||||
[[ "$(grep -c 'export PATH=' "$HOME_DIR/.bashrc")" -eq 2 ]]
|
||||
[[ "$(grep -c '^PATH=' "$ENVD")" -eq 2 ]]
|
||||
|
||||
reset_home
|
||||
echo "[test] a ~/.bash_profile does not silently swallow the PATH entry"
|
||||
# A bash login shell reads the first of .bash_profile / .bash_login / .profile that
|
||||
# exists and never looks at the rest. Writing only .profile is a no-op on such a host,
|
||||
# and the failure is invisible until something cannot find node.
|
||||
: > "$HOME_DIR/.bash_profile"
|
||||
run_bare --cli --next --yes --no-auto-launch >/dev/null 2>&1
|
||||
RESOLVED="$(env -i HOME="$HOME_DIR" PATH="$NONODE_BIN" TERM=dumb bash -lc 'command -v node')"
|
||||
[[ "$RESOLVED" == "${NODE_HOME}/${VERSION}/bin/node" ]] || {
|
||||
echo "with a .bash_profile present, a login shell resolved node to '${RESOLVED}'" >&2
|
||||
exit 1
|
||||
}
|
||||
|
||||
reset_home
|
||||
echo "[test] a commented-out example does not count as the PATH entry already existing"
|
||||
# The idempotence check used to be an unanchored substring match, so a line like this
|
||||
# in a user's profile made the installer skip the real entry.
|
||||
mkdir -p "$HOME_DIR"
|
||||
printf '# export PATH="%s/%s/bin:$PATH"\n' "$NODE_HOME" "$VERSION" > "$HOME_DIR/.profile"
|
||||
run_bare --cli --next --yes --no-auto-launch >/dev/null 2>&1
|
||||
[[ "$(grep -c '^export PATH=' "$HOME_DIR/.profile")" -eq 2 ]] || {
|
||||
echo "expected two real export lines, found:" >&2
|
||||
cat "$HOME_DIR/.profile" >&2
|
||||
exit 1
|
||||
}
|
||||
|
||||
reset_home
|
||||
echo "[test] --no-node-install refuses instead of installing"
|
||||
@@ -228,7 +283,13 @@ RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "failed checksum verification" <<<"$OUTPUT"
|
||||
# Not just "no usable node": nothing at all may survive. An unpack that ran before
|
||||
# verification, or a staging directory left behind, would still satisfy the weaker
|
||||
# check while leaving unverified bytes on disk for the next run to adopt.
|
||||
[[ ! -x "${NODE_HOME}/${VERSION}/bin/node" ]]
|
||||
[[ ! -e "${NODE_HOME}/${VERSION}" ]]
|
||||
[[ ! -e "${NODE_HOME}/${VERSION}.partial" ]]
|
||||
[[ ! -d "$NODE_HOME" ]] || [[ -z "$(ls -A "$NODE_HOME")" ]]
|
||||
publish_release "$VERSION"
|
||||
|
||||
reset_home
|
||||
@@ -272,4 +333,128 @@ OUTPUT="$(
|
||||
grep -qF -- "older than the required >= 22" <<<"$OUTPUT"
|
||||
[[ -x "${NODE_HOME}/${VERSION}/bin/node" ]]
|
||||
|
||||
# ─── refusals: untrusted input that reaches a path or an exec ─────────────────
|
||||
|
||||
reset_home
|
||||
echo "[test] an empty checksum manifest is refused, not read as an empty digest"
|
||||
: > "${DIST}/${VERSION}/SHASUMS256.txt"
|
||||
set +e
|
||||
OUTPUT="$(run_bare --cli --next --yes --no-auto-launch 2>&1)"
|
||||
RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "No checksum published" <<<"$OUTPUT"
|
||||
[[ ! -e "${NODE_HOME}/${VERSION}" ]]
|
||||
publish_release "$VERSION"
|
||||
|
||||
reset_home
|
||||
echo "[test] a manifest naming a regex-equivalent file does not vouch for this one"
|
||||
# The lookup used to interpolate the filename into a grep pattern. A Node tarball name
|
||||
# is mostly dots, and a dot matches any character, so this line -- which names a
|
||||
# different file -- was accepted as this file's checksum.
|
||||
DECOY="node-${VERSION}-${PLATFORM}Xtar.gz"
|
||||
printf '%s %s\n' "$(printf '0%.0s' $(seq 1 64))" "$DECOY" > "${DIST}/${VERSION}/SHASUMS256.txt"
|
||||
set +e
|
||||
OUTPUT="$(run_bare --cli --next --yes --no-auto-launch 2>&1)"
|
||||
RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "No checksum published" <<<"$OUTPUT"
|
||||
[[ ! -e "${NODE_HOME}/${VERSION}" ]]
|
||||
publish_release "$VERSION"
|
||||
|
||||
reset_home
|
||||
echo "[test] a manifest listing the same file twice is refused rather than guessed at"
|
||||
BASE="node-${VERSION}-${PLATFORM}.tar.gz"
|
||||
GOOD="$(awk '{print $1}' "${DIST}/${VERSION}/SHASUMS256.txt")"
|
||||
{
|
||||
printf '%s %s\n' "$GOOD" "$BASE"
|
||||
printf '%s %s\n' "$(printf '0%.0s' $(seq 1 64))" "$BASE"
|
||||
} > "${DIST}/${VERSION}/SHASUMS256.txt"
|
||||
set +e
|
||||
OUTPUT="$(run_bare --cli --next --yes --no-auto-launch 2>&1)"
|
||||
RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "refusing to guess" <<<"$OUTPUT"
|
||||
[[ ! -e "${NODE_HOME}/${VERSION}" ]]
|
||||
publish_release "$VERSION"
|
||||
|
||||
echo "[test] a version string is checked before it becomes a path"
|
||||
# MOSAIC_NODE_VERSION becomes a directory name under NODE_HOME, and that directory is
|
||||
# later handed to `rm -rf`. This is defence in depth, and the honest scope should be
|
||||
# recorded: the plain 'v..' case is separately refused by rm itself, and a traversal
|
||||
# value breaks the download URL before the removal is reached. Measured, not assumed.
|
||||
# What the check buys is that neither of those accidents is what is protecting us, and
|
||||
# that a typo is refused with its own name on it rather than a curl error.
|
||||
eval "$(sed -n '/^node_valid_version()/,/^}/p' "$ROOT/tools/install.sh")"
|
||||
for good in v22.99.0 v0.0.0 v22.11.0 v100.0.1; do
|
||||
node_valid_version "$good" || { echo "rejected a real version: ${good}" >&2; exit 1; }
|
||||
done
|
||||
for bad in 'v..' '..' 'v9.9.9/../../elsewhere' '/etc' 'v22' 'v22.1' '22.1.0' 'v22.1.0-rc1' '' 'v1.0.0 ' '$(id)'; do
|
||||
! node_valid_version "$bad" || { echo "accepted a bad version: '${bad}'" >&2; exit 1; }
|
||||
done
|
||||
|
||||
reset_home
|
||||
echo "[test] a bad MOSAIC_NODE_VERSION is refused by name, before any download"
|
||||
set +e
|
||||
OUTPUT="$(
|
||||
env -u npm_config_prefix \
|
||||
HOME="$HOME_DIR" MOSAIC_HOME="$MOSAIC_HOME_DIR" MOSAIC_PREFIX="$PREFIX" \
|
||||
MOSAIC_NO_COLOR=1 MOSAIC_NODE_HOME="$NODE_HOME" \
|
||||
MOSAIC_NODE_DIST="file://${DIST}" MOSAIC_NODE_VERSION="v9.9.9/../../elsewhere" \
|
||||
MOSAIC_TEST_REAL_NODE="$REAL_NODE" MOSAIC_TEST_NPM_LOG="$LOG" \
|
||||
MOSAIC_TEST_STATE="$STATE" PATH="$NONODE_BIN" \
|
||||
bash "$ROOT/tools/install.sh" --cli --next --yes --no-auto-launch 2>&1
|
||||
)"
|
||||
RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "MOSAIC_NODE_VERSION" <<<"$OUTPUT"
|
||||
grep -qF -- "Downloading Node" <<<"$OUTPUT" && {
|
||||
echo "the download started despite an invalid version" >&2
|
||||
exit 1
|
||||
}
|
||||
[[ ! -d "$NODE_HOME" ]]
|
||||
|
||||
reset_home
|
||||
echo "[test] a download location with no transport integrity is refused"
|
||||
set +e
|
||||
OUTPUT="$(
|
||||
env -u npm_config_prefix \
|
||||
HOME="$HOME_DIR" MOSAIC_HOME="$MOSAIC_HOME_DIR" MOSAIC_PREFIX="$PREFIX" \
|
||||
MOSAIC_NO_COLOR=1 MOSAIC_NODE_HOME="$NODE_HOME" \
|
||||
MOSAIC_NODE_DIST="http://example.invalid/dist" \
|
||||
MOSAIC_TEST_REAL_NODE="$REAL_NODE" MOSAIC_TEST_NPM_LOG="$LOG" \
|
||||
MOSAIC_TEST_STATE="$STATE" PATH="$NONODE_BIN" \
|
||||
bash "$ROOT/tools/install.sh" --cli --next --yes --no-auto-launch 2>&1
|
||||
)"
|
||||
RC=$?
|
||||
set -e
|
||||
[[ "$RC" -ne 0 ]]
|
||||
grep -qF -- "MOSAIC_NODE_DIST must be" <<<"$OUTPUT"
|
||||
[[ ! -d "$NODE_HOME" ]]
|
||||
|
||||
reset_home
|
||||
echo "[test] a path containing shell syntax is not written into a profile"
|
||||
# The PATH line is executed by every future shell that reads the file, so a directory
|
||||
# holding $() or a quote would run there as code.
|
||||
EVIL="$TMP/ev\$(touch $TMP/pwned)il"
|
||||
set +e
|
||||
env -u npm_config_prefix \
|
||||
HOME="$HOME_DIR" MOSAIC_HOME="$MOSAIC_HOME_DIR" MOSAIC_PREFIX="$EVIL" \
|
||||
MOSAIC_NO_COLOR=1 MOSAIC_NODE_HOME="$NODE_HOME" \
|
||||
MOSAIC_NODE_DIST="file://${DIST}" \
|
||||
MOSAIC_TEST_REAL_NODE="$REAL_NODE" MOSAIC_TEST_NPM_LOG="$LOG" \
|
||||
MOSAIC_TEST_STATE="$STATE" PATH="$NONODE_BIN" \
|
||||
bash "$ROOT/tools/install.sh" --cli --next --yes --no-auto-launch >/dev/null 2>&1
|
||||
set -e
|
||||
if [[ -f "$HOME_DIR/.profile" ]]; then
|
||||
grep -qF -- 'touch' "$HOME_DIR/.profile" && {
|
||||
echo "a command substitution was written into .profile" >&2
|
||||
exit 1
|
||||
}
|
||||
fi
|
||||
[[ ! -e "$TMP/pwned" ]] || { echo "the embedded command ran" >&2; exit 1; }
|
||||
|
||||
echo "[test] installer node provisioning tests passed"
|
||||
|
||||
Reference in New Issue
Block a user