Skip to content

Self update#271

Open
jai-deepsource wants to merge 9 commits intomasterfrom
self-update
Open

Self update#271
jai-deepsource wants to merge 9 commits intomasterfrom
self-update

Conversation

@jai-deepsource
Copy link
Contributor

No description provided.

- Fetch manifest from CDN on every invocation, download and replace
  binary if a newer version exists
- Verify SHA256 checksum before replacing, extract from tar.gz or zip
- Skip update in dev builds, CI environments, or when auto_update is
  false in config.toml
- Run update check in background goroutine, print notice to stderr
  after command completes
- Add AutoUpdate *bool field to CLIConfig for opt-out
- Phase 1 (CheckForUpdate) fetches manifest and writes state file if newer version exists, with a short 3s timeout to avoid slowing down CLI startup
- Phase 2 (ApplyUpdate) reads state file on next run and applies the update with a 30s timeout
- Removes background goroutine and channel-based approach in favor of synchronous two-phase model
- State file (update.json) is cleared before applying so broken updates don't retry forever
- Adds UpdateState struct and read/write/clear helpers for on-disk persistence
- Adds tests for state file lifecycle, version comparison, download + checksum verification, and no-op when state file is absent
- Extract hardcoded cli.deepsource.com URL into buildinfo.BaseURL so dev builds can point to cli.deepsource.one
- Only skip auto-update for local dev builds (version == "development"), not all dev-mode builds
- Update tests to reflect the new behavior
- Strip pre-release/build suffix (e.g. "44-e888cf0f" → "44") before parsing patch version
- Fixes strconv.Atoi failure when version contains a commit hash
@deepsource-io
Copy link

deepsource-io bot commented Mar 2, 2026

DeepSource Code Review

We reviewed changes in fccfbea...d7d18bc on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade  

Focus Area: Security
Security  

Reliability  

Complexity  

Hygiene  

Coverage  

Code Review Summary

Analyzer Status Updated (UTC) Details
Go Mar 4, 2026 8:50p.m. Review ↗
Secrets Mar 4, 2026 8:50p.m. Review ↗
Test coverage Mar 4, 2026 8:50p.m. Review ↗

Code Coverage Summary

Language Line Coverage (New Code) Line Coverage (Overall)
Aggregate
32.5%
20.8%
[▲ up 1.6% from master]
Go
32.5%
[⤫ below threshold]
20.8%
[▲ up 1.6% from master]
[✓ above threshold]

➟ Additional coverage metrics may have been reported. See full coverage report ↗

return fmt.Errorf("marshaling update state: %w", err)
}
p := updateStatePath()
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
Copy link

Choose a reason for hiding this comment

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

Expect directory permissions to be 0750 or less


Excessive permissions granted when creating a directory. This warning is
triggered whenever permission greater than 0750 is given.

checksum := sha256.Sum256(archive)
checksumHex := hex.EncodeToString(checksum[:])

srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Copy link

Choose a reason for hiding this comment

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

parameter 'r' seems to be unused, consider removing or renaming it as _


Unused parameters in functions or methods should be replaced with _
(underscore) or removed.


// updateStatePath returns the path to the update state file (~/.deepsource/update.json).
func updateStatePath() string {
home, _ := os.UserHomeDir()
Copy link

Choose a reason for hiding this comment

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

Ignored `os.UserHomeDir()` error can lead to insecure file creation


The error returned by os.UserHomeDir() is ignored. If this function fails, for example in a restricted environment where the home directory isn't available, home will be an empty string. This causes the update state file to be written to a relative path (.deepsource/update.json), potentially in the current working directory.

If the command is run from a world-writable directory like /tmp, an attacker could pre-create a malicious .deepsource/update.json file to control the update process, leading to arbitrary code execution. The error should be checked and handled to ensure the state file is always written to a secure, expected location.

Comment on lines +107 to +114
bakPath := fakeBin + ".bak"
if err := os.Rename(fakeBin, bakPath); err != nil {
t.Fatal(err)
}
if err := os.Rename(tmp.Name(), fakeBin); err != nil {
t.Fatal(err)
}
os.Remove(bakPath)
Copy link

Choose a reason for hiding this comment

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

`TestReplaceBinary` re-implements logic instead of calling the `replaceBinary` function


This block re-implements the logic for replacing a binary file, but the test never calls the production replaceBinary function it is meant to test. This provides a false sense of test coverage.

Refactor the test to call the actual replaceBinary function. This may require making replaceBinary more testable, such as by having it accept the target binary path as a parameter.

Comment on lines +32 to +51
parts := strings.SplitN(v, ".", 3)
if len(parts) != 3 {
return 0, 0, 0, fmt.Errorf("expected X.Y.Z, got %q", v)
}
major, err = strconv.Atoi(parts[0])
if err != nil {
return 0, 0, 0, err
}
minor, err = strconv.Atoi(parts[1])
if err != nil {
return 0, 0, 0, err
}
// Strip pre-release or build metadata suffix (e.g. "44-e888cf0f" → "44")
patchStr, _, _ := strings.Cut(parts[2], "-")
patch, err = strconv.Atoi(patchStr)
if err != nil {
return 0, 0, 0, err
}
return major, minor, patch, nil
}
Copy link

Choose a reason for hiding this comment

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

Custom semantic version parsing is incomplete and mishandles pre-release or build metadata


The custom implementation for parsing semantic versions in parseSemver is not fully compliant with the SemVer 2.0.0 standard. It incorrectly handles pre-release version precedence and will fail to parse versions containing build metadata (e.g., 1.2.3+456), causing update checks to fail or behave unexpectedly.

Using a battle-tested library is strongly recommended. Replace this custom logic with a standard library like golang.org/x/mod/semver to ensure correct and robust version parsing and comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants