Tighten install script version guard to full semver#225
Conversation
The regex only checked major.minor, so a partial version like "0.2" would pass and produce a guaranteed-to-fail download URL. Require all three components to fail fast with a clear error message.
There was a problem hiding this comment.
Pull request overview
This PR tightens the “latest version” guard in the installer and ensure script to require a full MAJOR.MINOR.PATCH semver, preventing partial versions (e.g., 0.2) from being accepted and generating invalid release download URLs.
Changes:
- Update version validation regex in
scripts/install.shto requireX.Y.Z. - Apply the same regex tightening in
scripts/ensure-basecamp.sh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/install.sh | Require full semver when extracting the latest release version from the GitHub redirect URL. |
| scripts/ensure-basecamp.sh | Require full semver when determining the latest version during auto-install. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward regex tightening, two identical one-line changes.
Extended reasoning...
Overview
This PR changes a single regex pattern in two shell scripts (scripts/install.sh and scripts/ensure-basecamp.sh), tightening the version validation from ^[0-9]+\.[0-9]+ to ^[0-9]+\.[0-9]+\.[0-9]+ to require full semver (X.Y.Z) instead of accepting partial versions (X.Y).
Security risks
None. The change strictly tightens input validation in install scripts. No new code paths, no relaxed checks, no changes to authentication, permissions, or data handling.
Level of scrutiny
Minimal scrutiny warranted. This is a two-line mechanical change following established patterns, with clear intent documented in the PR description as a follow-up to #223. The regex modification is correct and easy to verify by inspection.
Other factors
The bug report identifies a pre-existing gap where BASECAMP_VERSION env var bypasses validation entirely. This is explicitly a pre-existing issue not introduced by this PR — the old code also had no validation on that path. While it would be nice to fix in this PR, its absence does not block approval of the current change. The PR has a test plan and make check passes.
The env var path bypassed get_latest_version() and its semver guard, allowing partial versions like 0.2 to produce broken download URLs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/install.sh">
<violation number="1" location="scripts/install.sh:310">
P2: Regex is missing a `$` end-anchor, so values like `1.2.3garbage` pass validation and produce a broken download URL. Consider anchoring with `$` (or `(-|$)` if you want to allow pre-release suffixes like `1.2.3-rc.1`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Values like 1.2.3garbage would pass the unanchored regex and produce broken download URLs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
LGTM — regex tightening is correct and the env var validation gap is now closed.
Extended reasoning...
Overview
This PR makes a small, targeted change to two shell scripts (scripts/install.sh and scripts/ensure-basecamp.sh), tightening a version regex from ^[0-9]+\.[0-9]+ to ^[0-9]+\.[0-9]+\.[0-9]+ so that partial version strings like 0.2 are rejected early instead of producing broken download URLs. A third hunk adds the same validation to the BASECAMP_VERSION env var code path in main(), addressing feedback from my previous review.
Security risks
None. The change strictly narrows input validation — it rejects more inputs, not fewer. There is no auth, crypto, or permissions code involved.
Level of scrutiny
Low scrutiny is appropriate. This is a three-line regex tightening in install/setup scripts, following an established pattern already present in the codebase. The intent is clear from the PR description and the diff is trivially verifiable.
Other factors
The author addressed the pre-existing gap I flagged in my previous review (the BASECAMP_VERSION env var bypass). The inline comment thread is resolved. No bugs were found by the automated bug hunting system. The change is a direct follow-up to PR #223 and the commit history confirms the progression.
- Include resolved tag/URL in error messages for debuggability - Accept prerelease suffixes (e.g. 1.2.3-rc.1) in BASECAMP_VERSION env var - Keep strict X.Y.Z for get_latest_version (GitHub latest is always stable)
Summary
scripts/install.shandscripts/ensure-basecamp.shfrom^[0-9]+\.[0-9]+to^[0-9]+\.[0-9]+\.[0-9]+0.2would have passed the old guard and produced a guaranteed-to-fail download URLFollow-up to #223 — addresses post-merge review feedback.
Test plan
make checkpassescurl -fsSL -o /dev/null -w '%{url_effective}' https://github.com/basecamp/basecamp-cli/releases/lateststill resolvesv0.2.3correctlySummary by cubic
Tightened version checks in the install scripts to require full semver (X.Y.Z) with anchored regex and clearer diagnostics.
get_latest_versionremains strict (stable tags only), while env-supplied versions can include prereleases like 1.2.3-rc.1.^[0-9]+\.[0-9]+\.[0-9]+$inscripts/install.shandscripts/ensure-basecamp.sh; error messages now include the resolved tag/URL.VERSION/BASECAMP_VERSIONwith^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$to reject partials/trailing junk and allow prereleases.v0.2.3.Written for commit d283885. Summary will update on new commits.