feat(update): propose self-update when a newer release is available - #167
Conversation
Startup proposal after the banner: 'Update available: X (you have Y) / Update now? [y/N]'. Accepting downloads the platform archive from the public releases repo (qmax-code-releases), extracts the binary, and atomically replaces the running executable via write-tmp + rename (cross-device fallback; windows shifts the running exe aside). - checks hit the GitHub API at most once per 24h (~/.qmax-code/update_check.json) - a declined version is recorded and never re-offered - dev builds and QMAX_NO_UPDATE_CHECK=1 skip the check; network failures are silent - /update command checks on demand and installs when found - pre-release versions sort below their release (1.24.0-rc1 < 1.24.0)
Sigilix is reviewing this pull request. A structured overview will replace this note when retrieval and specialist review complete. |
|
QualityMax ReviewVerdict: COMMENT · Confidence: evidence-backed scan Files eligible: 3 · Files reviewed: 1 · Files with findings: 1 · Findings: 2 · Inline cards: 2 Priority findings
Review gates
Important files
Change diagram — Flowgraph TD
A[Start REPL] --> B{Check for Update}
B -->|Available| C[Prompt User]
C -->|Yes| D[Apply Update]
C -->|No| E[Mark Skipped]
D --> F[Overwrite Binary]
F --> G[Restart Required]
Review lifecycleUse the inline cards to inspect evidence and suggested remediation. Re-run the QualityMax review after pushing a fix; unchanged cards are identified by their stable finding marker. Dismiss with a reason through the existing QualityMax/GitHub review feedback flow. 0 prior card(s) are stale/resolved on this head. Proof legend: VERIFIED independently judged patch · REPRODUCED verified finding · GROUNDED deterministic evidence · MODEL-ONLY model judgment. QualityMax project results are available in the configured project. Receipt · commit
|
The repo's TestNoEgressOutsideHTTPX guard forbids raw net/http clients and requests outside internal/httpx; fetchLatest and Download now use httpx.NewClient + httpx.NewRequest so update traffic is receipt-recorded.
|
|
| Gate | Result |
|---|---|
| 🔍 AI diff review | gemini-3.1-flash-lite · completed · 3 eligible / 1 reviewed · gemini-3.1-flash-lite |
| 🔍 SAST | completed · 5 eligible / 5 reviewed · gemini-3.1-pro-preview |
| 🔍 Canonical PR review delivery | failed · 0 eligible / 0 reviewed · review=http_4xx/HTTP 422;retryable=false, comment=delivered#5367242772 |
| 🧪 Repo Tests | ✅ 607/607 passed (go) |
| 🤖 AI Tests |
Powered by QualityMax — AI-Powered Test Automation
|
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/update/update.go:357 | file-scope | Missing signature verification of downloaded binary allows supply-chain attack |
| } | ||
| if len(bin) < 1<<10 { | ||
| return fmt.Errorf("extracted binary suspiciously small (%d bytes)", len(bin)) | ||
| } | ||
|
|
||
| // Windows cannot overwrite a running executable in place; shift it aside | ||
| // and drop the new one where it was. The .old file is cleaned by the | ||
| // installer on future runs. | ||
| if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
Non-atomic fallback in Apply can corrupt the executable on rename failure
The Apply method's fallback path for a failed rename writes directly to the executable path with os.WriteFile, which truncates the existing binary before writing. If the write fails (e.g., disk full), the executable is left corrupted and the original binary is lost. The method is advertised as an atomic replacement, but this fallback is not atomic and can result in a broken installation.
Write the new binary to a temp file in the same directory as the executable (already done). If rename fails, copy the new binary to the destination using a write+sync+rename strategy on the same device, or abort the update without destroying the existing binary.
Example:
input: rename from .new to the executable path fails (e.g., cross-device install)
actual: fallback calls os.WriteFile(exePath, bin, 0o755) — opens with O_TRUNC, destroying the existing binary, then writes the new content. If the write fails partway, the file is truncated and the new binary is incomplete.
expected: the old binary is preserved; the update is either fully applied or the system is left in the original state.
Why this wasn't caught: No test for the fallback when rename fails; the existing TestApplySwapsBinary does not simulate a rename failure or verify the old binary is preserved.
Current:
}
if len(bin) < 1<<10 {
return fmt.Errorf("extracted binary suspiciously small (%d bytes)", len(bin))
}
// Windows cannot overwrite a running executable in place; shift it aside
// and drop the new one where it was. The .old file is cleaned by the
// installer on future runs.
if runtime.GOOS == "windows" {Proposed:
if err := os.Rename(tmp, exePath); err != nil {
_ = os.Remove(tmp)
return fmt.Errorf("cannot replace binary (rename failed): %w", err)
}| } | |
| if len(bin) < 1<<10 { | |
| return fmt.Errorf("extracted binary suspiciously small (%d bytes)", len(bin)) | |
| } | |
| // Windows cannot overwrite a running executable in place; shift it aside | |
| // and drop the new one where it was. The .old file is cleaned by the | |
| // installer on future runs. | |
| if runtime.GOOS == "windows" { | |
| if err := os.Rename(tmp, exePath); err != nil { | |
| _ = os.Remove(tmp) | |
| return fmt.Errorf("cannot replace binary (rename failed): %w", err) | |
| } |
More Info
- Threat model: A user on a system where the executable resides on a different mount point or where the rename fails for any reason. The update attempt silently destroys the existing binary, leaving the user without a working executable.
- Specific code citations:
internal/update/update.go:340-345— theos.Renamefailure branch that callsos.WriteFiledirectly. - Existing protections: The rename is unlikely to fail because the temp file is in the same directory as the target, but the fallback exists for cross-device or permission edge cases. No backup of the original binary is taken.
- Proposed mitigation: Replace the direct
os.WriteFilewith a two-step atomic write on the same device (write to a temp file, then rename) or, if the rename fails, leave the existing binary intact and report the error so the user can intervene. - Alternative mitigations considered: Log a warning and abort the update without overwriting the existing binary, leaving the .new file for the user to handle.
- Severity calibration: The condition is rare (rename failure), but the impact is severe: the user's binary is destroyed and a manual reinstall is required.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/update/update.go
Line: 332-340
Comment:
**Non-atomic fallback in Apply can corrupt the executable on rename failure**
The `Apply` method's fallback path for a failed rename writes directly to the executable path with `os.WriteFile`, which truncates the existing binary before writing. If the write fails (e.g., disk full), the executable is left corrupted and the original binary is lost. The method is advertised as an atomic replacement, but this fallback is not atomic and can result in a broken installation.
Write the new binary to a temp file in the same directory as the executable (already done). If rename fails, copy the new binary to the destination using a write+sync+rename strategy on the same device, or abort the update without destroying the existing binary.
Example:
input: rename from .new to the executable path fails (e.g., cross-device install)
actual: fallback calls os.WriteFile(exePath, bin, 0o755) — opens with O_TRUNC, destroying the existing binary, then writes the new content. If the write fails partway, the file is truncated and the new binary is incomplete.
expected: the old binary is preserved; the update is either fully applied or the system is left in the original state.
Threat model:
A user on a system where the executable resides on a different mount point or where the rename fails for any reason. The update attempt silently destroys the existing binary, leaving the user without a working executable.
Specific code citations:
`internal/update/update.go:340-345` — the `os.Rename` failure branch that calls `os.WriteFile` directly.
Existing protections:
The rename is unlikely to fail because the temp file is in the same directory as the target, but the fallback exists for cross-device or permission edge cases. No backup of the original binary is taken.
Proposed mitigation:
Replace the direct `os.WriteFile` with a two-step atomic write on the same device (write to a temp file, then rename) or, if the rename fails, leave the existing binary intact and report the error so the user can intervene.
Alternative mitigations considered:
Log a warning and abort the update without overwriting the existing binary, leaving the .new file for the user to handle.
Severity calibration:
The condition is rare (rename failure), but the impact is severe: the user's binary is destroyed and a manual reinstall is required.
Suggested fix shape:
if err := os.Rename(tmp, exePath); err != nil {
_ = os.Remove(tmp)
return fmt.Errorf("cannot replace binary (rename failed): %w", err)
}
Why this wasn't caught:
No test for the fallback when rename fails; the existing TestApplySwapsBinary does not simulate a rename failure or verify the old binary is preserved.
How can I resolve this? If you propose a fix, please make it concise.
| return os.WriteFile(exePath, bin, 0o755) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing integrity check on extracted binary size may allow truncated/malformed binary
The Apply method checks if len(bin) < 1<<10 to reject suspiciously small binaries, but this is insufficient. A malicious or corrupted archive could produce a binary that passes the size check but is malformed (e.g., wrong architecture, truncated, or corrupted), causing the program to crash or behave unexpectedly after restart.
Add a more robust integrity check, such as verifying the binary's Go build ID, checking a stored SHA-256 hash from the release metadata, or at least ensuring the binary is executable (via elf or mach-o parsing).
Example:
A network glitch truncates the downloaded archive, resulting in a 500KB binary that is missing the final segment. The size check passes, but the binary crashes on startup.
Suggested fix:
// After extraction:
wantHash := fetchChecksum(r.Asset + ".sha256")
if sha256.Sum256(bin) != wantHash {
return fmt.Errorf("checksum mismatch")
}More Info
- Threat model: A network corruption or malicious CDN could serve a partially truncated binary that is still >1KB but invalid. The user's qmax-code would fail to start after restart, causing denial of service.
- Specific code citations:
internal/update/update.go:357—ApplycallsextractBinaryand then checkslen(bin) < 1<<10. - Existing protections: Only the 1KB size check; no hash comparison or format validation.
- Proposed mitigation: Fetch a SHA-256 checksum file from the release assets and compare it against the extracted binary. Alternatively, parse the binary's header to confirm it's a valid executable for the current platform.
- Alternative mitigations considered: Use Go's
debug/elfordebug/machoto read the binary's architecture and ensure it matchesruntime.GOARCH. This adds a dependency but is lightweight. - Severity calibration: Score 3 because exploitation likely requires an active attacker on the network path (MITM) or a compromised CDN, and the impact is denial of service rather than code execution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/update/update.go
Line: 357
Comment:
**Missing integrity check on extracted binary size may allow truncated/malformed binary**
The `Apply` method checks `if len(bin) < 1<<10` to reject suspiciously small binaries, but this is insufficient. A malicious or corrupted archive could produce a binary that passes the size check but is malformed (e.g., wrong architecture, truncated, or corrupted), causing the program to crash or behave unexpectedly after restart.
Add a more robust integrity check, such as verifying the binary's Go build ID, checking a stored SHA-256 hash from the release metadata, or at least ensuring the binary is executable (via `elf` or `mach-o` parsing).
Example:
A network glitch truncates the downloaded archive, resulting in a 500KB binary that is missing the final segment. The size check passes, but the binary crashes on startup.
Threat model:
A network corruption or malicious CDN could serve a partially truncated binary that is still >1KB but invalid. The user's qmax-code would fail to start after restart, causing denial of service.
Specific code citations:
`internal/update/update.go:357` — `Apply` calls `extractBinary` and then checks `len(bin) < 1<<10`.
Existing protections:
Only the 1KB size check; no hash comparison or format validation.
Proposed mitigation:
Fetch a SHA-256 checksum file from the release assets and compare it against the extracted binary. Alternatively, parse the binary's header to confirm it's a valid executable for the current platform.
Alternative mitigations considered:
Use Go's `debug/elf` or `debug/macho` to read the binary's architecture and ensure it matches `runtime.GOARCH`. This adds a dependency but is lightweight.
Severity calibration:
Score 3 because exploitation likely requires an active attacker on the network path (MITM) or a compromised CDN, and the impact is denial of service rather than code execution.
Suggested fix shape:
// After extraction:
wantHash := fetchChecksum(r.Asset + ".sha256")
if sha256.Sum256(bin) != wantHash {
return fmt.Errorf("checksum mismatch")
}
How can I resolve this? If you propose a fix, please make it concise.
| return os.WriteFile(exePath, bin, 0o755) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Atomic rename cross-device fallback may leave temporary .new file on some systems
When os.Rename fails (e.g., across filesystem boundaries), the code falls back to os.WriteFile(exePath, bin, 0o755). This overwrites the running executable in-place, which on some platforms (Linux, macOS) may fail because the file is locked by the running process, leaving the .new temporary file behind.
The fallback should delete the .new file after a successful write, and on failure clean up the .new file to avoid leaving garbage.
Example:
Binary is on a different mount, rename fails, fallback write fails due to file lock, `.new` file remains.
Suggested fix:
// In the fallback:
defer os.Remove(tmp)
if err := os.WriteFile(exePath, bin, 0o755); err != nil {
return err
}More Info
- Threat model: On a cross-device install (binary on a different mount), the rename fails, the fallback write may succeed or fail depending on OS locking. If it fails, the
.newfile remains in the directory, potentially causing confusion or being executed accidentally. - Specific code citations:
internal/update/update.go:357—if err := os.Rename(tmp, exePath); err != nil { _ = os.Remove(tmp); return os.WriteFile(exePath, bin, 0o755) } - Existing protections:
os.Remove(tmp)is called before the fallback, but if the fallback fails the.newfile is not cleaned up. - Proposed mitigation: Ensure the fallback path also removes the
.newfile on any error, and perhaps log a warning about cross-device rename. - Alternative mitigations considered: Use
io.Copyto a temporary file on the same filesystem as the target, then rename. - Severity calibration: Score 2 because it's a minor hardening issue; the leftover file is not a security vulnerability but could cause user confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/update/update.go
Line: 357
Comment:
**Atomic rename cross-device fallback may leave temporary .new file on some systems**
When `os.Rename` fails (e.g., across filesystem boundaries), the code falls back to `os.WriteFile(exePath, bin, 0o755)`. This overwrites the running executable in-place, which on some platforms (Linux, macOS) may fail because the file is locked by the running process, leaving the `.new` temporary file behind.
The fallback should delete the `.new` file after a successful write, and on failure clean up the `.new` file to avoid leaving garbage.
Example:
Binary is on a different mount, rename fails, fallback write fails due to file lock, `.new` file remains.
Threat model:
On a cross-device install (binary on a different mount), the rename fails, the fallback write may succeed or fail depending on OS locking. If it fails, the `.new` file remains in the directory, potentially causing confusion or being executed accidentally.
Specific code citations:
`internal/update/update.go:357` — `if err := os.Rename(tmp, exePath); err != nil { _ = os.Remove(tmp); return os.WriteFile(exePath, bin, 0o755) }`
Existing protections:
`os.Remove(tmp)` is called before the fallback, but if the fallback fails the `.new` file is not cleaned up.
Proposed mitigation:
Ensure the fallback path also removes the `.new` file on any error, and perhaps log a warning about cross-device rename.
Alternative mitigations considered:
Use `io.Copy` to a temporary file on the same filesystem as the target, then rename.
Severity calibration:
Score 2 because it's a minor hardening issue; the leftover file is not a security vulnerability but could cause user confusion.
Suggested fix shape:
// In the fallback:
defer os.Remove(tmp)
if err := os.WriteFile(exePath, bin, 0o755); err != nil {
return err
}
How can I resolve this? If you propose a fix, please make it concise.
Minor bump: first release shipping the self-update proposal (#167). Installs on 1.24.0 will receive their first update prompt once this version is published to qmax-code-releases.
1 participant
What
qmax-code now proposes updating itself when a newer release is out.
At startup (after the banner, once per 24h max):
Quality-Max/qmax-code-releases, extracts the binary, atomically replaces the running executable.✓ Updated to 1.25.0 — restart qmax-code to use itSkipped — /update checks again anytime.and that exact version is never re-offered/update→ on-demand check + install;QMAX_NO_UPDATE_CHECK=1→ check fully disabledHow
internal/update/update.go—MaybeCheckCheck(bypasses cache, honors opt-out + dev build)Release.Apply— download (128 MiB cap) → extract from tar.gz/zip → write.new→ atomic rename (cross-device fallback; Windows shifts the running exe aside)repl.go—maybeProposeUpdateafter the banner, viaterm.ReadConsent/update+docs/COMMANDS.mdrowSafety properties: network failures are silent (check can never break startup); semver comparison treats pre-releases as older than their release; dev builds (
""/dev) never check; state in~/.qmax-code/update_check.json.Validation
go build ./...,go vet ./...cleango test ./internal/update/— 8 tests: version compare (incl. pre-release), asset naming, cache/skip/opt-out, silent network failure, tar.gz + zip extraction, atomic swapgo test ./internal/repl/— all passCheck("1.23.0")→ found1.24.0via real GitHub API → downloaded real darwin-arm64 asset → extracted → swapped a target binary (18,141,376 bytes verified)Changes at a glance (5 files, +635)
internal/update/update.go+299 (new)internal/update/update_test.go+280 (new)internal/repl/repl.go+52/updatecommanddocs/COMMANDS.md+1,CHANGELOG.md+13flowchart TD S["startup (non-quiet)"] --> MC{"MaybeCheck(version)<br/>24h cache · skip marker · dev-build · QMAX_NO_UPDATE_CHECK"} MC -->|no newer / cached / skipped| OFF["no prompt"] MC -->|newer release| P["Update available: X — [y/N]"] P -->|N| SKIP["MarkSkipped(X) — never re-offer X"] P -->|y| DL["Download platform archive<br/>qmax-code-releases"] DL --> EX["Extract binary (tar.gz / zip)"] EX --> SWAP["write .new → atomic rename<br/>windows: shift running exe aside"] SWAP --> OK["✓ restart to use it"] CMD["/update"] --> CHK{"Check(version)<br/>bypasses cache"} CHK -->|newer| DL CHK -->|up to date| UTD["✓ up to date"]