Skip to content

fix(cli): make uipro update actually upgrade the CLI via npm (supersedes #326)#389

Merged
mrgoonie merged 1 commit into
nextlevelbuilder:mainfrom
ia-abatista:fix/uipro-update-via-npm
Jun 25, 2026
Merged

fix(cli): make uipro update actually upgrade the CLI via npm (supersedes #326)#389
mrgoonie merged 1 commit into
nextlevelbuilder:mainfrom
ia-abatista:fix/uipro-update-via-npm

Conversation

@ia-abatista

Copy link
Copy Markdown
Contributor

What & why

uipro update checked the latest release but, when the installed CLI was behind, only printed npm install -g uipro-cli@… and returned — it never actually upgraded. Users stayed on stale assets (the root of #353 for npm consumers). This makes uipro update perform the upgrade itself.

Change

When the installed version is behind the latest release, updateCommand now runs the global install:

execFileSync(
  isWindows ? 'npm.cmd' : 'npm',
  ['install', '-g', `uipro-cli@${latestVersion}`],
  { stdio: 'inherit', shell: isWindows }
);

Addressing the review on #326:

  • execFileSync with an explicit args array instead of execSync('npm install -g …') — no shell string is built or expanded on the common path.
  • Windows: npm is npm.cmd, which Node refuses to spawn without a shell since CVE-2024-27980, so Windows uses npm.cmd + shell: true; POSIX runs with no shell at all.
  • Injection-safe: latestVersion is validated as semver (/^\d+\.\d+\.\d+([.-][0-9A-Za-z.-]+)?$/) before it can reach the Windows shell — e.g. a tag like 2.7.0; rm -rf / is rejected and falls back to manual instructions.
  • Failure handling: if the install fails (e.g. needs admin), it prints the manual command and exits non-zero.
  • Post-update guidance: on success it tells the user to rerun uipro init --ai <platform> --force (the maintainer's suggestion). README updated to match.

Verification

  • npm run typecheck passes.
  • Version-guard unit check: accepts 2.7.0, 2.8.0-beta.1; rejects 2.7.0; rm -rf /, latest && evil, empty.

(Can't exercise a real global install in CI without side effects; the install path is execFileSync with a validated static arg array.)

Supersedes #326 (resolves the execSync/shell review finding + Windows .cmd handling + version validation). Credit to @alfredo-petri for the original fix.

`uipro update` only printed manual instructions when a newer release
existed; it never upgraded the package, so users stayed on stale assets.

It now runs the global install itself when behind:
- execFileSync with an explicit args array (no shell string to expand),
  per the review on nextlevelbuilder#326 — not execSync.
- Windows uses npm.cmd with shell:true (Node won't spawn a .cmd without a
  shell post-CVE-2024-27980); POSIX runs npm with no shell at all.
- The release version is validated as semver before use, so nothing
  unexpected can reach the Windows shell.
- On failure it falls back to manual instructions; on success it tells
  the user to rerun `uipro init --ai <platform> --force`.

README updated to describe the new behavior.

Supersedes nextlevelbuilder#326. Credit to @alfredo-petri for the original fix.

Co-authored-by: alfredo-petri <146951566+alfredo-petri@users.noreply.github.com>
@ia-abatista

Copy link
Copy Markdown
Contributor Author

@mrgoonie more clean up duty

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: approved. This PR fixes the stale updater path with the safer shape requested on #326: POSIX uses execFileSync with an explicit argv array and no shell, Windows is limited to npm.cmd with shell:true, and the release tag is normalized/semver-gated before it can reach that Windows shell path. The README now clearly says update the global CLI first, then rerun init with --force.nnEvidence checked: reviewed cli/src/commands/update.ts and cli/README.md diff; compared against the prior #326 review; searched related PRs/issues (#326, #353, #362); verified the current code keeps the already-current case refreshing local bundled assets.nnLocal verification note: npm run typecheck could not run in this cron workspace because node_modules/tsc is not installed, so I did not treat that as a code failure.nnVerdict: APPROVE.

@mrgoonie mrgoonie merged commit 4ab7038 into nextlevelbuilder:main Jun 25, 2026
@ia-abatista ia-abatista deleted the fix/uipro-update-via-npm branch June 25, 2026 09:23
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