Skip to content

fix: harden cli installer bootstrap#601

Merged
shadowfax92 merged 2 commits intomainfrom
fix/powershell-installer
Mar 27, 2026
Merged

fix: harden cli installer bootstrap#601
shadowfax92 merged 2 commits intomainfrom
fix/powershell-installer

Conversation

@shadowfax92
Copy link
Copy Markdown
Contributor

@shadowfax92 shadowfax92 commented Mar 27, 2026

Summary

  • keep the public one-line install commands unchanged in the CLI README
  • restore the shell installer usage and checksum verification behavior
  • narrow the implementation change to the PowerShell installer by removing its same-release checksum verification only

Rework

The first pass changed the public bootstrap flow on both platforms. This rework restores the original one-line install UX and limits the code-level mitigation to install.ps1, matching the latest product direction.

Test plan

  • bash -n apps/cli/scripts/install.sh
  • git diff --check
  • bun run lint
  • bun run typecheck (fails in this workspace because apps/agent/.wxt/tsconfig.json is missing; unrelated)
  • bun run test

@github-actions github-actions bot added the fix label Mar 27, 2026
@shadowfax92
Copy link
Copy Markdown
Contributor Author

@claude review

@shadowfax92
Copy link
Copy Markdown
Contributor Author

@greptileai review

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @shadowfax92's task in 57s —— View job


Code Review

  • Read changed files
  • Analyze diff
  • Post review feedback

Overall this is a well-motivated, focused change. The move from pipe-to-shell to download-then-run is good practice. A few issues worth considering:

Security: Checksum Removal

The most significant concern is removing the checksum verification entirely rather than fixing it. The PR description correctly identifies that irm | iex is the AV trigger — but the checksums were protecting against a separate threat: a compromised CDN or GitHub release serving a tampered binary. These are independent risks.

The checksums were fetched from the same GitHub release, which the PR notes is "same-release checksum verification" — a valid criticism since an attacker who controls the release could swap both. However, even same-release checksums provide transport integrity (detecting corruption or CDN-level tampering mid-transit). Removing them entirely leaves no verification between download and execution.

Consider: if cdn.browseros.com is ever compromised or if GitHub release assets can be tampered with (they can't easily, but CDN caching layers can), there's no detection. The fix could instead fetch the checksum from a different origin (e.g., your CDN vs. GitHub), which would defend against both the original criticism and the attack vector.

That said, if this is an intentional product decision to simplify the installer (trading off integrity check for simplicity), it's understandable — just worth documenting why.

install.sh: /tmp path not cleaned up

packages/browseros-agent/apps/cli/README.md and the inline usage comment in install.sh both use a fixed path /tmp/browseros-cli-install.sh. This file is never cleaned up and could persist across reboots or be read by other users on a shared system. Minor, but worth noting.

The install.ps1 equivalent uses $env:TEMP which is user-scoped and cleaner. The shell version could use $(mktemp) similarly.

Fix this →

install.ps1: env var fallback removed cleanly

packages/browseros-agent/apps/cli/scripts/install.ps1:24 — the $env:BROWSEROS_VERSION / $env:BROWSEROS_DIR fallback was correctly removed since it only existed to support the irm | iex pattern where param() was ignored. Clean removal.

README: explanatory note is good

The added note at packages/browseros-agent/apps/cli/README.md explaining why the pattern was changed is appreciated — users who wonder why it's two commands instead of one will find the answer inline.

Minor: install.sh comment inconsistency

packages/browseros-agent/apps/cli/scripts/install.sh:6-11 — the usage comment shows the two-step pattern correctly, but note that unlike the PowerShell script which saves to $env:TEMP (user-specific), the shell script saves to /tmp (world-readable on Linux). Worth being consistent.


Summary: The pipe-to-shell removal is the right call. The main tradeoff to weigh is whether removing checksum verification entirely is acceptable given it does provide transport integrity independent of the delivery mechanism concern. If intentional, worth a short comment in the scripts. Everything else is clean.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR narrows the Windows PowerShell installer change to a single, intentional simplification: it removes the same-release checksum verification block from install.ps1. The bash installer and public one-liner README commands are left untouched, restoring the intended UX after the first-pass rework.

Key points:

  • Checksum removal is intentional and low-risk: The removed checksums.txt was hosted on the same GitHub release as the binary itself, so it provided no meaningful protection against a compromised release — an attacker who can swap the zip can equally swap the checksum file. TLS 1.2 is already enforced and is the primary integrity guarantee.
  • Env-var fallbacks preserved: $env:BROWSEROS_VERSION (line 27) and $env:BROWSEROS_DIR (line 28) are still present, keeping CI/non-interactive pipelines working — the concern raised in the prior thread no longer applies.
  • Cleanup is correct: The finally block still removes the temp directory unconditionally, and $ErrorActionPreference = "Stop" is unchanged.
  • No new logic is introduced; the only diff is deletion of ~31 lines.

Confidence Score: 5/5

Safe to merge — the only change is removing a same-release checksum that offered negligible security over HTTPS, with no regressions introduced.

All prior P1 concerns (env-var fallbacks being dropped) are confirmed resolved — the fallbacks remain on lines 27–28. The checksum removal is explicit product intent, is low-risk given TLS 1.2 enforcement, and no new logic is added. No P0 or P1 findings remain.

No files require special attention.

Important Files Changed

Filename Overview
packages/browseros-agent/apps/cli/scripts/install.ps1 Removes same-release checksum download and verification block (~31 lines); download → extract → install flow is otherwise unchanged. Env-var fallbacks for $env:BROWSEROS_VERSION and $env:BROWSEROS_DIR are preserved.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant PS as install.ps1
    participant GH as GitHub API
    participant CDN as GitHub Releases CDN

    U->>PS: Run (param or irm|iex)
    PS->>GH: GET /releases (if no version given)
    GH-->>PS: latest tag
    PS->>CDN: GET {binary}_{version}_windows_{arch}.zip
    CDN-->>PS: zip file (HTTPS/TLS 1.2)
    Note over PS: ~~Download checksums.txt~~ (removed)
    Note over PS: ~~Verify SHA-256~~ (removed)
    PS->>PS: Expand-Archive → move exe → update PATH
    PS-->>U: "Run browseros-cli --help"
Loading

Reviews (2): Last reviewed commit: "refactor: rework 0327-harden_cli_install..." | Re-trigger Greptile

@shadowfax92
Copy link
Copy Markdown
Contributor Author

@greptileai review

@shadowfax92
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @shadowfax92's task in 1m 39s —— View job


Code Review

  • Read changed files
  • Analyze diff
  • Post review feedback

Summary

After the rework commit (7b607e4), this PR makes exactly one change: remove the checksum verification block from install.ps1 (~32 lines). The README is unchanged (still irm | iex), and install.sh is unchanged (checksum verification retained). This is worth calling out clearly because the earlier greptile and Claude reviews were written against the pre-rework commit, and much of that feedback no longer applies to the current diff.


Issues

1. PR title no longer matches the change

fix: harden cli installer bootstrap implies a security improvement. The actual change removes a security control (checksum verification). The rework stripped the hardening (download-then-run) and left only the weakening. The title should reflect what the final PR actually does.

2. No rationale documented for the asymmetry

install.sh retains checksum verification; install.ps1 no longer has it. There's nothing in the script or commit message explaining why. The PR description mentions "same-release checksum verification" as the concern (valid point — checksums from the same release don't guard against a compromised release), but that reasoning would apply equally to install.sh. Future maintainers hitting this will wonder why the two installers behave differently.

A one-line comment above the download block in install.ps1 would be sufficient:

# Checksum omitted — checksums.txt is hosted in the same release, so same-source
# verification doesn't defend against a compromised release asset.

3. Stale inline comment at line 26

# When piped via irm | iex, param() is ignored — fall back to env vars

The env-var fallbacks are still present and the irm | iex usage is still documented in the header comment and the README, so this is consistent and not a bug. But it reads oddly alongside a script that no longer has checksums specifically because of concerns with the irm | iex flow. Minor.


What's No Longer Relevant (Pre-Rework Feedback)

  • Greptile's P1 Unblock-File concern was about a download-then-run README pattern that no longer exists on this branch. The README still shows irm | iex, so no zone-marking issue applies.
  • The $env:BROWSEROS_VERSION env-var removal concern is moot — those fallbacks were restored in the rework.
  • The /tmp cleanup concern is moot — the README install snippet wasn't changed.

Verdict

The change is safe to merge as-is. The only blocking concern I'd flag is updating the PR title to avoid misleading the git history. The missing rationale comment is worth adding but not a blocker.

@shadowfax92 shadowfax92 merged commit 1c5ffdf into main Mar 27, 2026
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant