Skip to content

chore: Windows CI Node install and snyk-env.sh BOM#6764

Merged
PeterSchafer merged 1 commit intomainfrom
chore/CLI-1469-windows-ci-node-install
May 8, 2026
Merged

chore: Windows CI Node install and snyk-env.sh BOM#6764
PeterSchafer merged 1 commit intomainfrom
chore/CLI-1469-windows-ci-node-install

Conversation

@danskmt
Copy link
Copy Markdown
Contributor

@danskmt danskmt commented May 4, 2026

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages are release-note ready, emphasizing what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Hardens Windows CI for the Node toolchain used when building the CLI:

  • Node is installed only via nvm-windows (nvm install / nvm use) using the version pinned in .nvmrc. There is no MSI path or SHA256 check. If nvm is not on PATH, the script fails fast with a clear error.
  • Diagnostic logging: nvm binary path, NVM_HOME, NVM_SYMLINK, and [nvm-cache] HIT / [nvm-cache] MISS so logs show whether Node was downloaded this run or reused from the nvm store.
  • CircleCI cache: Windows tools cache bumped to windows-tools-cache-v4-{{ arch }}-{{ checksum ".nvmrc" }} so caches invalidate when Node changes; save_cache includes << windows_cache_dir >> and C:\ProgramData\nvm so the nvm install tree persists between jobs.

This addresses the class of failures where node.exe and npm’s own files came from different layouts (e.g. MSI vs NVM), by standardizing on one manager (nvm) on CI.

Note: scripts/windows/install-node.ps1 in this PR does not append Node to snyk-env.ps1 / snyk-env.sh; it only runs nvm and verifies C:\Program Files\nodejs\node.exe. Downstream steps rely on nvm use having switched the junction and on the image / default PATH including Program Files\nodejs as usual. If anything regresses, we can restore explicit PATH lines or document the assumption.

Changes

  • scripts/windows/install-node.ps1 — nvm-only install from .nvmrc; verification; logging; no MSI.
  • .circleci/config.ymlinstall-deps-windows-native-build: cache key v4 + {{ checksum ".nvmrc" }}; save_cache.paths adds C:\ProgramData\nvm.

Where should the reviewer start?

  1. scripts/windows/install-node.ps1
  2. .circleci/config.ymlinstall-deps-windows-native-build (restore/save cache)

How should this be manually tested?

  1. Run a Windows job that uses install-deps-windows-native-build (e.g. full Windows build).
  2. In Install Node.js (native), confirm nvm-windows:, NVM_HOME:, and [nvm-cache] HIT/MISS.
  3. Confirm legacy TS / npm steps still succeed (no Class extends value undefined from npm).
  4. On first green run, confirm NVM_HOME in logs matches C:\ProgramData\nvm; if not, adjust the save_cache path in .circleci/config.yml.

What's the product update that needs to be communicated to CLI users?

None. CI/build plumbing only.

Risk assessment (Low | Medium | High)?

Low — CI scripts and config only; no change to shipped CLI behaviour.

Any background context you want to provide?

CircleCI Windows images ship nvm-windows; a second MSI installer was redundant and risked inconsistent installs. {{ checksum ".nvmrc" }} ties the cache to the pinned Node version.

Related: CLI-1469. Prior discussion: #6758

What are the relevant tickets?

CLI-1469

Screenshots (if appropriate)

N/A

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 4, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from 69b0a5d to aa420c9 Compare May 4, 2026 16:22
@danskmt danskmt changed the title fix: Harden Windows CI Node.js install for nvm-managed images fix: fix Windows CI Node install and snyk-env.sh BOM issues May 4, 2026
@danskmt danskmt changed the title fix: fix Windows CI Node install and snyk-env.sh BOM issues fix: Windows CI Node install and snyk-env.sh BOM May 4, 2026
@danskmt danskmt marked this pull request as ready for review May 4, 2026 16:59
@danskmt danskmt requested review from a team as code owners May 4, 2026 16:59
@snyk-pr-review-bot

This comment has been minimized.

@danskmt
Copy link
Copy Markdown
Contributor Author

danskmt commented May 4, 2026

Integrity Verification Bypass 🟠 [major]
The new logic for nvm-windows detection bypasses the SHA256 checksum verification entirely. While the MSI fallback still checks the hash, the preferred nvm install path downloads and activates Node.js binaries without verifying their integrity. This is a regression compared to the previous version and other tool installation scripts in the same directory (e.g., install-gradle.ps1) that enforce checksums for all downloads.

if ($nvmExe) {
  Write-Host "nvm-windows detected; using nvm to install and activate Node.js v$nodeVersion..."

  $nvmList = & nvm list 2>&1 | Out-String
  if ($nvmList -notmatch [regex]::Escape($nodeVersion)) {
    Write-Host "Installing Node.js v$nodeVersion via nvm..."
    & nvm install $nodeVersion
  } else {
    Write-Host "Node.js v$nodeVersion already installed in nvm."
  }

  Write-Host "Activating Node.js v$nodeVersion via nvm..."
  & nvm use $nodeVersion
} else {

Maintenance Landmine 🟠 [major]
The hardcoded $expectedSha256 for the MSI fallback is no longer guarded by an explicit version check. In the previous code, a mismatch between .nvmrc and $expectedNodeVersion would trigger a clear error. Now, updating .nvmrc without manually updating $expectedSha256 will cause any CI runner using the MSI fallback to fail with an opaque checksum verification error during Node.js download.

# MSI fallback only: SHA256 of node-v{version}-x64.msi for the version in .nvmrc (nodejs.org). Update when bumping Node in .nvmrc.
$expectedSha256 = 'c54f5f7e2416e826fd84e878f28e3b53363ae9c3f60a140af4434b2453b5ae89'

# Resolve repo root from script location (script is in scripts/windows)
$repoRoot = Resolve-Path (Join-Path $PSScriptRoot '..\..')
$nvmrcPath = Join-Path $repoRoot '.nvmrc'

if (-not (Test-Path $nvmrcPath)) {
  throw ".nvmrc not found at $nvmrcPath"
}

$nodeVersion = (Get-Content $nvmrcPath -Raw).Trim().TrimStart('v')

Loose Version Match 🟡 [minor]
The nvm list check uses a loose regex match against the version string. If $nodeVersion is '20.11.1' and nvm list contains '20.11.10', the -notmatch check will evaluate to false (a match is found), causing the script to skip installing the specific required version. It should use word boundaries or an exact match against the listed versions.

  1. NVM manages its own downloads over HTTPS from nodejs.org; SHA256 verification is for the manual MSI fallback only.
  2. That was the same behavior before, requiring to update both. Here we'll use .nvmrc as the source of node version.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from aa420c9 to 9d00bc2 Compare May 4, 2026 17:34
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt changed the title fix: Windows CI Node install and snyk-env.sh BOM chore: Windows CI Node install and snyk-env.sh BOM May 5, 2026
@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from 9d00bc2 to abb1374 Compare May 5, 2026 07:56
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch 3 times, most recently from b599041 to aedda9a Compare May 5, 2026 13:34
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch 2 times, most recently from 6b4fc27 to 2de598c Compare May 5, 2026 13:40
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from 2de598c to 21b32c7 Compare May 5, 2026 13:43
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

Comment thread scripts/windows/install-node.ps1 Outdated
Comment thread scripts/windows/install-node.ps1
Comment thread scripts/windows/install-node.ps1 Outdated
Comment thread .circleci/config.yml Outdated
@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from 21b32c7 to f8e6bc9 Compare May 8, 2026 11:20
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from f8e6bc9 to 921f1ba Compare May 8, 2026 12:11
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from 921f1ba to de1628b Compare May 8, 2026 12:35
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1469-windows-ci-node-install branch from de1628b to 5eb116f Compare May 8, 2026 12:43
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Performance Regression 🟠 [major]

The cache key for windows-tools-cache now includes checksum ".nvmrc". This cache is shared by Maven, Gradle, .NET SDK, Python, and GNU Make (as seen in the install-deps-windows-native-build steps). Adding the Node version checksum to the global key means any Node.js upgrade will invalidate the entire tools cache, forcing a re-download and re-installation of every other toolchain, which will significantly increase CI turnaround time for those PRs.

- windows-tools-cache-v4-{{ arch }}-{{ checksum ".nvmrc" }}
Incomplete BOM Fix 🟡 [minor]

The PR aims to fix Byte Order Mark (BOM) issues in snyk-env.sh for Git Bash compatibility. While the BOM-inducing Out-File -Encoding UTF8 calls were removed from this script, the cross-file context shows that identical logic remains in other tool installation scripts (e.g., install-maven.ps1, install-dotnet-sdk.ps1) and the install-go command. Since these all write to the same shared environment file, snyk-env.sh will still be generated with a BOM if any other tool is installed, failing the stated objective.

    throw "node.exe not found at expected path '$nodeExe' after installation."
  }

  $reportedVersion = & $nodeExe -v
  if ($reportedVersion.Trim() -ne "v$nodeVersion") {
    throw "Installed Node.js version '$reportedVersion' does not match expected 'v$nodeVersion'."
  }

  Write-Host "Node.js $reportedVersion installed and active at $nodeExe"

}
catch {
  Write-Error "Failed to install Node.js: $($_.Exception.Message)"
  exit 1
}
📚 Repository Context Analyzed

This review considered 8 relevant code sections from 8 files (average relevance: 0.72)

@danskmt danskmt requested a review from PeterSchafer May 8, 2026 13:13
@PeterSchafer PeterSchafer enabled auto-merge May 8, 2026 13:58
@PeterSchafer PeterSchafer merged commit e7fd9d2 into main May 8, 2026
12 checks passed
@PeterSchafer PeterSchafer deleted the chore/CLI-1469-windows-ci-node-install branch May 8, 2026 14:11
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