Harden InstallTarballPackage.sh: input validation, safe download, temp cleanup, and quoting#1
Merged
Conversation
Reviewer's GuideHardens the PowerShell tarball installation script by enabling stricter shell behavior, validating inputs, downloading via a secure temporary location, and consistently using variables and quoting for paths and URLs. Flow diagram for hardened PowerShell tarball installation scriptflowchart TD
A[Start InstallTarballPackage.sh] --> B[Read POWERSHELL_VERSION and POWERSHELL_PACKAGE]
B --> C{Validate POWERSHELL_VERSION}
C -->|invalid| X[usage and exit]
C -->|valid| D{Validate POWERSHELL_PACKAGE name}
D -->|invalid| X
D -->|valid| E{Check .tar.gz extension}
E -->|invalid| X
E -->|valid| F[Create TEMP_DIR with mktemp -d]
F --> G[Set trap cleanup for TEMP_DIR]
G --> H[Build DOWNLOAD_URL and PACKAGE_PATH]
H --> I[curl download to PACKAGE_PATH with strict TLS]
I --> J[Create INSTALL_DIR with mkdir -p]
J --> K[Extract tar to INSTALL_DIR]
K --> L[Create symlink POWERSHELL_LINKFILE -> INSTALL_DIR/pwsh]
L --> M{Update /etc/shells if needed}
M --> N[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughHarden the tarball installer: stricter shell options and parameter validation, temp-dir creation with cleanup, TLS‑restricted curl downloads plus SHA‑256 verification, versioned extraction and safer symlink handling, conditional shells file update, and a shell test suite that fakes curl/tar. ChangesShell Script Robustness and Security Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the cleanup trap, consider guarding the
rm -rfcall (e.g.,if [ -n "$TEMP_DIR" ] && [ -d "$TEMP_DIR" ]; then rm -rf -- "$TEMP_DIR"; fi) to avoid accidentally removing an unexpected path ifmktempever fails orTEMP_DIRis modified. - The strict validation patterns for
POWERSHELL_VERSIONandPOWERSHELL_PACKAGEare quite tight; it may be worth double-checking that all currently published and anticipated tag/package naming conventions are compatible, or relaxing the patterns slightly to avoid future breakage from new version naming schemes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the cleanup trap, consider guarding the `rm -rf` call (e.g., `if [ -n "$TEMP_DIR" ] && [ -d "$TEMP_DIR" ]; then rm -rf -- "$TEMP_DIR"; fi`) to avoid accidentally removing an unexpected path if `mktemp` ever fails or `TEMP_DIR` is modified.
- The strict validation patterns for `POWERSHELL_VERSION` and `POWERSHELL_PACKAGE` are quite tight; it may be worth double-checking that all currently published and anticipated tag/package naming conventions are compatible, or relaxing the patterns slightly to avoid future breakage from new version naming schemes.
## Individual Comments
### Comment 1
<location path="docker/InstallTarballPackage.sh" line_range="65-67" />
<code_context>
# Create the symbolic link that points to powershell
-ln -s /opt/microsoft/powershell/$POWERSHELL_VERSION/pwsh $POWERSHELL_LINKFILE
+ln -s "$INSTALL_DIR/pwsh" "$POWERSHELL_LINKFILE"
# Add the symbolic link path to /etc/shells
if [ ! -f /etc/shells ]; then
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `ln -s` will fail if the symlink or file already exists, which may break idempotent or upgrade scenarios.
With `set -e`, a pre-existing `/usr/bin/pwsh` (from a prior install/upgrade) will cause `ln -s` to fail and abort the script. To keep the script idempotent and upgrade-safe, either use a force option like `ln -sfn` or explicitly remove/replace the existing link first (e.g., `if [ -L "$POWERSHELL_LINKFILE" ]; then rm "$POWERSHELL_LINKFILE"; fi`).
```suggestion
# Create the symbolic link that points to powershell (overwrite any existing link/file to be idempotent)
ln -sfn "$INSTALL_DIR/pwsh" "$POWERSHELL_LINKFILE"
# Add the symbolic link path to /etc/shells
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…p cleanup, and quoting
### Motivation
- Improve robustness and safety of the tarball installation script by adding strict shell options and explicit input validation.
- Prevent unsafe filenames and malformed version strings from being used to construct download URLs or filesystem paths.
- Ensure temporary artifacts are cleaned up and downloads use secure, fail-fast curl options.
### Description
- Enabled strict error handling with `set -eu` and added parameter defaults using `${1:-}` and `${2:-}` for `POWERSHELL_VERSION` and `POWERSHELL_PACKAGE`.
- Added validation `case` checks to reject invalid `POWERSHELL_VERSION` and `POWERSHELL_PACKAGE` values and to enforce that the package ends with `.tar.gz`.
- Create a secure temporary directory via `mktemp -d`, register a `cleanup` function with `trap` to remove it, and download the package into the temp directory using `curl --fail --location --show-error --proto '=https' --tlsv1.2`.
- Use variables for `DOWNLOAD_URL`, `PACKAGE_PATH`, and `INSTALL_DIR`, update `tar` and `ln -s` invocations to use quoted variables, and ensure `/etc/shells` modifications are quoted and guarded with `grep`.
### Testing
- No automated tests were run on this change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/InstallTarballPackage.sh`:
- Around line 58-63: The script currently downloads the PowerShell tarball
(using DOWNLOAD_URL into PACKAGE_PATH) and extracts it into INSTALL_DIR without
verifying integrity; update the flow to first download the release checksum file
(e.g., hashes.sha256) for the same release, parse the SHA-256 entry that
corresponds to the downloaded filename, compute the SHA-256 of PACKAGE_PATH
(e.g., sha256sum or shasum -a 256) and compare it against the expected value,
and abort with a non-zero exit and error log if the checksums do not match; only
run tar zxf "$PACKAGE_PATH" -C "$INSTALL_DIR" after the checksum verification
succeeds.
- Line 66: Ensure creating the pwsh symlink is safe to rerun: before calling ln
-s use INSTALL_DIR and POWERSHELL_LINKFILE to check if POWERSHELL_LINKFILE
already exists; if it is a symlink and points to "$INSTALL_DIR/pwsh" do nothing,
if it is a symlink pointing elsewhere remove it and then create the symlink, and
if it exists but is not a symlink skip creation (or fail with a clear message)
to avoid clobbering a regular file; update the ln invocation accordingly so
rerunning the script under set -e is idempotent.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16158ff7-67b0-4d17-aa38-e282446e58ab
📒 Files selected for processing (1)
docker/InstallTarballPackage.sh
…p dir, and atomic symlink ### Motivation - Make the PowerShell tarball install script more robust and secure by adding strict error handling and input validation. - Avoid partial installs and leftover temporary files by using a temporary directory with cleanup on exit and making installations atomic. - Improve download reliability and harden handling of filenames and paths to prevent injection or malformed input issues. ### Description - Enable strict shell behavior with `set -eu` and validate `POWERSHELL_VERSION` and `POWERSHELL_PACKAGE` inputs with `case` patterns to reject unsafe values. - Require the package to end with `.tar.gz` and construct `DOWNLOAD_URL`, `PACKAGE_PATH`, and `INSTALL_DIR` variables to centralize paths and filenames. - Use `mktemp -d` to create a temporary directory and `trap` a `cleanup` function to remove it on `EXIT`, `HUP`, `INT`, and `TERM`. - Download with `curl --fail --location --show-error --proto '=https' --tlsv1.2` to ensure secure and reliable transfers and extract the package using the variable paths. - Create the symlink with `ln -sfn` for atomic updates and quote variables when writing or appending the path to `/etc/shells` while guarding duplicates with `grep -q`. ### Testing - No automated tests were added or executed for this change.
…ion, and safer download/cleanup
### Motivation
- Improve security and robustness of the PowerShell tarball installer by validating inputs and verifying release checksums.
- Ensure downloaded artifacts are retrieved over a hardened TLS configuration and any temporary files are cleaned up.
- Make the script fail fast on unset variables and provide clearer error messages for invalid parameters.
### Description
- Turn on strict shell options with `set -eu` and validate positional parameters using `POWERSHELL_VERSION=${1:-}` and `POWERSHELL_PACKAGE=${2:-}`.
- Add input validation for `POWERSHELL_VERSION` and `POWERSHELL_PACKAGE`, including allowed character checks and enforcing a `.tar.gz` package extension.
- Download the package and `hashes.sha256` using secure `curl` options and verify the package SHA-256 against the release `hashes.sha256` file; introduce `get_file_sha256` helper to support `sha256sum` or `shasum`.
- Use a `mktemp` temporary directory with a `cleanup` trap for safe removal, use `INSTALL_DIR` and `PACKAGE_PATH` variables, and extract to the target directory; create an atomic symlink with `ln -sfn` and quote variables when writing to `/etc/shells`.
### Testing
- Ran `shellcheck` on the modified script and fixed issues flagged by the linter, which passed without errors.
- Executed an automated smoke test in a container that runs the script with a known release (example `7.6.2` / `powershell-7.6.2-linux-x64.tar.gz`), which completed with exit code `0` and created the expected install directory and `/usr/bin/pwsh` symlink.
- Verified checksum mismatch handling by an automated negative test that supplies a tampered package, which correctly failed checksum validation with a non-zero exit code.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/docker/InstallTarballPackage.Tests.sh (1)
112-156: 💤 Low valueOptional: clean up per-test temp dirs. Each test creates
work_dir=$(mktemp -d)but never removes it, so every run leaks directories under/tmp(relevant in CI over time). Consider tracking and removing them, e.g. registering a trap-based cleanup orrm -rfat the end of each test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/docker/InstallTarballPackage.Tests.sh` around lines 112 - 156, Each test creates a temporary directory in the variable work_dir (see test_creates_symlink_when_missing, test_keeps_matching_symlink, test_replaces_different_symlink, test_refuses_regular_file) but never removes it; update each test to ensure the temp dir is cleaned up after the test completes (either add a per-test rm -rf "$work_dir" at the end or register a trap cleanup that removes work_dir on exit/failure) so CI does not leak /tmp directories.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/InstallTarballPackage.sh`:
- Around line 51-56: The trap currently calls cleanup for EXIT HUP INT TERM but
SIGINT/SIGHUP/SIGTERM handlers should also terminate the script; modify the
traps so that the cleanup function is invoked and then the shell exits for
HUP/INT/TERM (e.g. add a wrapper or separate trap commands that call cleanup and
then exit with a non-zero status) while keeping a plain EXIT trap for normal
cleanup; update the trap line referencing the cleanup function and the existing
trap invocation so HUP/INT/TERM do cleanup then stop execution.
- Around line 123-127: The current check uses grep -q "^${POWERSHELL_LINKFILE}$"
which treats POWERSHELL_LINKFILE as a regex and can miss literal lines
containing regex metacharacters; change the existence check to a literal,
whole-line match using grep -Fqx -- with the variables (search
POWERSHELL_LINKFILE in SHELLS_FILE), and when creating or appending the line use
printf '%s\n' "$POWERSHELL_LINKFILE" to write to SHELLS_FILE instead of echo to
ensure proper escaping and a trailing newline.
---
Nitpick comments:
In `@test/docker/InstallTarballPackage.Tests.sh`:
- Around line 112-156: Each test creates a temporary directory in the variable
work_dir (see test_creates_symlink_when_missing, test_keeps_matching_symlink,
test_replaces_different_symlink, test_refuses_regular_file) but never removes
it; update each test to ensure the temp dir is cleaned up after the test
completes (either add a per-test rm -rf "$work_dir" at the end or register a
trap cleanup that removes work_dir on exit/failure) so CI does not leak /tmp
directories.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fddacf22-adc1-4fe8-8358-d2b78fba1387
📒 Files selected for processing (2)
docker/InstallTarballPackage.shtest/docker/InstallTarballPackage.Tests.sh
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <[email protected]>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
set -euand added parameter defaults using${1:-}and${2:-}forPOWERSHELL_VERSIONandPOWERSHELL_PACKAGE.casechecks to reject invalidPOWERSHELL_VERSIONandPOWERSHELL_PACKAGEvalues and to enforce that the package ends with.tar.gz.mktemp -d, register acleanupfunction withtrapto remove it, and download the package into the temp directory usingcurl --fail --location --show-error --proto '=https' --tlsv1.2.DOWNLOAD_URL,PACKAGE_PATH, andINSTALL_DIR, updatetarandln -sinvocations to use quoted variables, and ensure/etc/shellsmodifications are quoted and guarded withgrep.Testing
Summary by Sourcery
Harden the PowerShell tarball installation script with stricter error handling, validated inputs, safer downloads, and improved path/quoting practices.
Bug Fixes:
Enhancements:
Summary by CodeRabbit