fix(install): Validate ULOOP_VERSION shape before use#1675
Conversation
…nstall.ps1 Both installer scripts interpolate ULOOP_VERSION into the release download URL without normalizing dot segments, so a value like "../../evil/repo/releases/download/v1" would break out of the intended release path. Fail-close on any value that is not one of the two well-known channel selectors or a semver-shaped tag (bare, dispatcher-v-prefixed, uloop-project-runner-v-prefixed, or v-prefixed) so downstream URL builders and ULOOP_VERSION passthrough only ever see a value they were designed for. Existing dispatcher self-update flows already emit values that pass this check via NormalizeTargetVersion + DispatcherReleaseTag; the guard is defensive coverage for the ambient-env attacker.
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe installer validates ChangesVersion validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
grep -Eq matches per line, so a value like `printf '../evil\n3.0.0'` was passing the ERE on its second line while smuggling a newline-poisoned first line into the URL builder downstream. Add a whole-string `case` guard before the grep so any character outside the semver tag alphabet (digits, letters, `.`, `+`, `-`) is rejected at the string level. install.ps1 already anchors ^...$ at string level so this bypass never existed there. Add two regression cases with embedded newlines to test-install-version-format.sh so the whole-string guard cannot regress.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test-install-version-format.sh (1)
11-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a guard for missing function extraction.
The
extract_functionawk pattern (^validate_uloop_version\(\)) assumes the function definition starts at column 0. If the definition is ever indented, extraction silently yields nothing,evaldefines no function, and allexpect_failtests pass vacuously (command-not-found returns non-zero). Theexpect_passtests would catch this, but only with a less helpful error message.A one-line guard after the
evalwould make the failure mode explicit:eval "$(extract_function validate_uloop_version)" +if ! command -v validate_uloop_version >/dev/null 2>&1; then + echo "FAIL: could not extract validate_uloop_version from install.sh" >&2 + exit 1 +fiThe
evalpattern itself is acceptable here since the source is a trusted file in the same repository. The static analysis hints (SC1007 for theCDPATH=idiom, SC2034 forLATEST_VERSION/LATEST_BETA_VERSIONused by the eval'd function) are false positives in this context.🤖 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 `@scripts/test-install-version-format.sh` around lines 11 - 22, Add a guard immediately after eval "$(extract_function validate_uloop_version)" to verify that validate_uloop_version was successfully extracted and defined, failing with a clear diagnostic if it is missing. Use the existing extract_function and validate_uloop_version symbols; leave the trusted-file eval and noted static-analysis suppressions unchanged.
🤖 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.
Nitpick comments:
In `@scripts/test-install-version-format.sh`:
- Around line 11-22: Add a guard immediately after eval "$(extract_function
validate_uloop_version)" to verify that validate_uloop_version was successfully
extracted and defined, failing with a clear diagnostic if it is missing. Use the
existing extract_function and validate_uloop_version symbols; leave the
trusted-file eval and noted static-analysis suppressions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac742e5c-bfa4-4f3c-abcd-52b16dd1eab5
⛔ Files ignored due to path filters (1)
scripts/install.ps1is excluded by none and included by none
📒 Files selected for processing (3)
cli/dispatcher/shared-inputs-stamp.jsonscripts/install.shscripts/test-install-version-format.sh
…guard commit 900663a re-edited scripts/install.sh (added the case-based whole-string guard) without re-running scripts/stamp-release-inputs.sh, leaving the stamp's sharedInputsHash stale for the current install.sh contents. Restamp so the file matches the input set.
Summary
install.shandinstall.ps1reject values ofULOOP_VERSIONthat could break out of the intended release path (e.g.../../evil/repo/releases/download/v1), so a hostile ambient env cannot redirect the installer to a different release.User Impact
latest,latest-beta, bare semver (3.0.0-beta.5), and prefixed release tags (dispatcher-v3.0.0-beta.5,uloop-project-runner-v3.0.0-beta.43,v3.0.0).Changes
scripts/install.sh: addvalidate_uloop_versionPOSIX-sh guard, called immediately after$VERSIONis derived.scripts/install.ps1: addTest-UloopVersionFormatwith the same regex.scripts/test-install-version-format.sh: exercise 15 accepted and rejected shapes against the extractedvalidate_uloop_versionimplementation.Verification
sh scripts/test-install-version-format.sh— passsh scripts/test-install-release-filter.sh— pass (no regression on the existing release-selection flow)sh scripts/test-install-archive-manifest.sh— pass (manifest verification still works)