Skip to content

Harden generated install.sh/install.ps1 per shellcheck and PSScriptAnalyzer - #433

Merged
tpowell-progress merged 3 commits into
mainfrom
shellcheck-psscriptanalyzer-hardening
Aug 4, 2026
Merged

Harden generated install.sh/install.ps1 per shellcheck and PSScriptAnalyzer#433
tpowell-progress merged 3 commits into
mainfrom
shellcheck-psscriptanalyzer-hardening

Conversation

@Stromweld

@Stromweld Stromweld commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Description

Runs shellcheck -s sh against the generated Bourne (install.sh) script and PSScriptAnalyzer against the generated PowerShell (install.ps1) script, and fixes the real correctness/portability issues found in each generator template.

Bourne (sh) generator

Kept backticks (not $(...)) and expr (not $(( ))) throughout, since install.sh's own header requires it to run on true Bourne shell on Solaris 9 and AIX 6.x, where neither is available.

  • Quoted variable expansions vulnerable to word-splitting/globbing ($tmp_dir/stderr, $project, $download_dir, etc.)
  • Fixed reversed 2>&1 >/dev/null redirects in do_wget/do_curl/do_perl/do_python so stderr is actually captured/silenced as intended
  • Replaced tr '[A-Z]' '[a-z]' with tr 'A-Z' 'a-z' — POSIX tr treats bracketed classes literally, not as ranges
  • Replaced the bashism source /etc/os-release with .source is undefined in POSIX sh
  • Removed no-op self-assignments (e.g. https_proxy=$https_proxy)
  • Simplified a useless echo/backtick round-trip in do_download
  • Quoted the OPTIND shift substitution
  • Switched two debug-output blocks from echo "\n...\n$var" to printf for portable escape-sequence expansion

PowerShell generator

  • Real bug fix: removed a stray duplicate [string] type attribute on the $project parameter in Get-ProjectMetadata
  • Get-Win32OS now caches to $script:win32OS instead of $global:win32OS, since this all executes inside a new-module scriptblock and shouldn't leak into the caller's global session state
  • Swapped $null to the left side of -eq/-ne comparisons
  • Replaced gi and foreach aliases with full cmdlet names
  • Renamed Custom-GetFileHash/Is-FIPS to approved verbs (Get-CustomFileHash/Test-Fips)
  • Documented the intentionally-empty console-encoding catch block instead of leaving it silently empty
  • Rewrote the omnitruck text-response parser as an explicit ForEach-Object -Begin/-Process/-End pipeline instead of the positional-scriptblock foreach {}{}{} idiom

Explicitly left alone (verified as intentional / false positive, not fixed)

  • SC2006 (backticks vs $(...)) and SC2003 (expr) — required for Solaris 9 / AIX 6.x /bin/sh compatibility
  • PSAvoidUsingWriteHost — this is a one-shot console install script, not a reusable module; direct console output is correct here
  • PSAvoidUsingWMICmdlet (Get-WMIQuery) — already prefers Get-CimInstance and only falls back to Get-WmiObject for older/restricted environments
  • PSUseShouldProcessForStateChangingFunctions (New-Uri) — false positive, it only constructs a System.Uri object
  • PSUseSingularNouns (Get-ProjectMetadata, Test-Fips) — false positives on irregular/acronym nouns
  • PSUseDeclaredVarsMoreThanAssignments ($hash) — known analyzer limitation with ForEach-Object -Begin/-Process/-End variable sharing

Testing

  • sh -n install.sh — syntax OK
  • shellcheck -s sh -S style install.sh — all remaining findings are the intentionally-skipped categories above (verified none of the real bug categories remain)
  • PowerShell [System.Management.Automation.Language.Parser]::ParseFile — parses cleanly
  • Invoke-ScriptAnalyzer -Severity Warning,Error — all remaining findings are the intentionally-skipped/false-positive categories above
  • Also added install.sh/install.ps1 (local generator test output) to .gitignore

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • All commits have been signed-off for the DCO.

Bourne (sh) generator - shellcheck fixes, keeping backticks and expr
(not $(...) / $(( )) ) since the script must run on true Bourne shell
on Solaris 9 and AIX 6.x:
- Quote variable expansions that were vulnerable to word-splitting/
  globbing (tmp_dir/stderr paths, project, download_dir, etc.)
- Fix reversed '2>&1 >/dev/null' redirects in do_wget/do_curl/do_perl/
  do_python so stderr is actually silenced
- Replace 'tr '[A-Z]' '[a-z]'' with 'tr 'A-Z' 'a-z'' (bracketed classes
  are taken literally by POSIX tr)
- Replace bashism 'source /etc/os-release' with '.' (source is
  undefined in POSIX sh)
- Drop no-op self-assignments (e.g. https_proxy=$https_proxy)
- Simplify useless echo/backtick round-trip in do_download
- Quote the OPTIND shift substitution
- Use printf instead of echo for the two debug blocks that rely on
  backslash escape expansion

PowerShell generator - PSScriptAnalyzer fixes:
- Fix a real bug: stray duplicate [string] type attribute on the
  $project parameter in Get-ProjectMetadata
- Get-Win32OS caches to $script:win32OS instead of $global:win32OS
  since this all runs inside a new-module scriptblock
- Swap $null to the left of -eq/-ne comparisons
- Replace 'gi' and 'foreach' aliases with full cmdlet names
- Rename Custom-GetFileHash/Is-FIPS to approved verbs
  (Get-CustomFileHash/Test-Fips)
- Document the intentionally-empty console encoding catch block
- Rewrite the omnitruck text-response parser as an explicit
  ForEach-Object -Begin/-Process/-End pipeline

Ignore locally generated install.sh/install.ps1 test artifacts.

Signed-off-by: Corey Hemminger <hemminger@hotmail.com>
@Stromweld
Stromweld requested review from a team and jaymzh as code owners July 29, 2026 18:13
Signed-off-by: Corey Hemminger <hemminger@hotmail.com>
@Stromweld Stromweld self-assigned this Jul 30, 2026
@tpowell-progress
tpowell-progress merged commit b7796d9 into main Aug 4, 2026
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants