Open
Conversation
silv-io
requested review from
anisaoshafi and
carole-lavillonniere
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new interactive and non-interactive "update" CLI command; implements install-method detection (binary, Homebrew, npm); adds GitHub release fetching, asset download/extract/replace, npm/homebrew update handlers; introduces UI orchestration for interactive updates; and generalizes container log events to LogLineEvent with a Source field. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Checker as VersionChecker
participant Detector as InstallDetector
participant Updater
participant Sink
participant UI
User->>CLI: run "lstk update [--check] [--non-interactive]"
CLI->>Checker: Check(ctx, sink) / fetchLatestVersion
Checker->>Sink: Emit progress / Note
Checker-->>CLI: (latestVersion, available)
alt check only
CLI->>Sink: Emit availability note
Sink->>User: display
else perform update
CLI->>Detector: DetectInstallMethod()
Detector-->>CLI: InstallInfo
CLI->>Sink: Emit detected method
alt interactive
CLI->>UI: RunUpdate(parentCtx, checkOnly)
UI->>Updater: start Update(ctx, sink, checkOnly)
else non-interactive
CLI->>Updater: Update(ctx, sink, checkOnly)
end
Updater->>Sink: Emit LogLineEvent(source, line) and progress
Updater->>Sink: Emit success/failure
Sink->>User: display results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
internal/update/update.go (2)
97-120: Consider adding a timeout to HTTP requests.
http.DefaultClienthas no timeout, which could cause the update check to hang indefinitely on network issues. Consider using a client with a timeout or adding a context deadline.♻️ Proposed fix to add HTTP timeout
+var httpClient = &http.Client{ + Timeout: 30 * time.Second, +} + func fetchLatestVersion(ctx context.Context) (string, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, latestReleaseURL, nil) if err != nil { return "", err } req.Header.Set("Accept", "application/vnd.github+json") - resp, err := http.DefaultClient.Do(req) + resp, err := httpClient.Do(req) if err != nil { return "", err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 97 - 120, The fetchLatestVersion function uses http.DefaultClient (via http.DefaultClient.Do) which has no timeout; update it to enforce a deadline by either creating an http.Client with a sensible Timeout (e.g., 5–15s) and use that client to Do(req) or ensure the provided ctx has a deadline (wrap ctx with context.WithTimeout when nil) before calling http.NewRequestWithContext; keep the existing error handling and response decoding logic (references: fetchLatestVersion, latestReleaseURL, http.NewRequestWithContext, resp.Body.Close, json.NewDecoder.Decode) so the request cannot hang indefinitely.
236-277: Consider handling the case whereos.Renamesucceeds butos.Chmodfails.After a successful rename (line 271), if
os.Chmodfails (line 276), the binary is already replaced but may have incorrect permissions. The error is returned, but the update is effectively complete. This edge case is unlikely but could leave the binary in an unexpected state.💡 Proposed improvement
if err := os.Rename(newBinary, exePath); err != nil { // Cross-device rename: fall back to copy return copyFile(newBinary, exePath, info.Mode()) } - return os.Chmod(exePath, info.Mode()) + if err := os.Chmod(exePath, info.Mode()); err != nil { + // Rename succeeded but chmod failed; binary is updated but permissions may differ + return fmt.Errorf("update applied but failed to restore permissions: %w", err) + } + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 236 - 277, Move permission setting to the new binary before replacing the existing executable: in extractAndReplace, after obtaining info := os.Stat(exePath) and before calling os.Rename(newBinary, exePath), call os.Chmod(newBinary, info.Mode()) and handle any error; then perform the rename (and keep the existing fallback to copyFile which should use info.Mode()). Remove the final os.Chmod(exePath, info.Mode()) after the rename so we don't return an error for a completed replacement; reference symbols: extractAndReplace, newBinary, exePath, info.Mode(), os.Rename, os.Chmod, copyFile.internal/output/plain_format.go (1)
28-29: Consider including source prefix in plain output.The TUI renders log lines with a source prefix (
brew | ...), but plain output omits it. This could make non-interactive output harder to parse when mixing sources. Consider aligning the formats for consistency.💡 Proposed enhancement
case LogLineEvent: - return e.Line, true + if e.Source != "" { + return e.Source + " | " + e.Line, true + } + return e.Line, true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 28 - 29, The plain output branch for LogLineEvent currently returns only e.Line, omitting the source prefix; update the LogLineEvent handling in plain_format.go to prepend the source (e.Source or equivalent field on the event) to the returned string so plain output matches the TUI (e.g., "source | line"), ensuring you use the same separator/format used by the TUI and preserve existing behavior for events where source is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/update.go`:
- Around line 16-26: The new cobra command variable cmd for "update" lacks
PreRunE: initConfig so it bypasses the standard bootstrap; add PreRunE:
initConfig to the cmd definition (the same place where Use/Short/Long/RunE are
set) so the initConfig function runs before RunE; ensure you only wire
initConfig for non-side-effect-free commands (reference the cmd variable in
cmd/update.go and the initConfig function name).
In `@internal/output/events.go`:
- Around line 106-108: The plain output currently discards LogLineEvent.Source
because internal/output/plain_format.go prints e.Line directly; update
FormatEventLine(event any) to render the combined "source | line" string when
the event is a LogLineEvent (use e.Source and e.Line) and change plain_format.go
to call FormatEventLine instead of accessing e.Line, and ensure the TUI
rendering reuses FormatEventLine as well so both sinks use the same
source-prefixed formatting; reference LogLineEvent and FormatEventLine when
locating where to implement this change.
In `@internal/output/plain_sink_test.go`:
- Around line 108-114: The parity test was extended with
TestPlainSink_EmitsLogLineEvent but TestPlainSink_UsesFormatterParity still
omits LogLineEvent, so update the parity test to include LogLineEvent: add a
LogLineEvent case to the list exercised by TestPlainSink_UsesFormatterParity and
ensure the formatter function FormatEventLine and the sink implementation
PlainSink (and the Emit helper that dispatches LogLineEvent) are included in
that parity check so future changes to LogLineEvent formatting will be caught;
locate and modify the TestPlainSink_UsesFormatterParity test, the
FormatEventLine switch/handling for LogLineEvent, and confirm Emit/LogLineEvent
are present in the event union.
In `@internal/update/update.go`:
- Around line 151-183: logLineWriter is not concurrency-safe because cmd.Stdout
and cmd.Stderr share one instance so Write can be called concurrently and race
on w.buf; add a sync.Mutex (or sync.RWMutex) field to logLineWriter and
lock/unlock it in Write and Flush around accesses to w.buf (and the buffer
mutation/consumption) so concurrent writes and the final Flush are serialized;
update newLogLineWriter to initialize the struct with the mutex and ensure
methods still use the pointer receiver so the same mutex protects all
operations.
In `@test/integration/update_test.go`:
- Around line 193-200: The function npmPlatformPackage contains a no-op
conditional (if arch == "amd64" { arch = "amd64" }) — remove that redundant if
block and use runtime.GOARCH directly when building the package string in
npmPlatformPackage, or if a specific arch mapping was intended, replace the
no-op with the correct mapping (e.g., map "x86_64" to "amd64" or similar) and
then construct "lstk_"+os+"_"+arch.
- Around line 157-191: TestUpdateHomebrew currently overwrites the real
Homebrew-installed binary; modify the test to avoid destructive local runs by
gating it behind an explicit opt-in (e.g., check an env var like
LSTK_TEST_HOMEBREW == "1" at the start of TestUpdateHomebrew) or by calling
t.Skipf when the env var is not set; keep the existing requireHomebrew() and
homebrewLstkBinaryPath() checks but add the env-based guard so the test only
runs when explicitly enabled (or alternatively add a clear comment above
TestUpdateHomebrew documenting the destructive nature and the required opt-in).
---
Nitpick comments:
In `@internal/output/plain_format.go`:
- Around line 28-29: The plain output branch for LogLineEvent currently returns
only e.Line, omitting the source prefix; update the LogLineEvent handling in
plain_format.go to prepend the source (e.Source or equivalent field on the
event) to the returned string so plain output matches the TUI (e.g., "source |
line"), ensuring you use the same separator/format used by the TUI and preserve
existing behavior for events where source is empty.
In `@internal/update/update.go`:
- Around line 97-120: The fetchLatestVersion function uses http.DefaultClient
(via http.DefaultClient.Do) which has no timeout; update it to enforce a
deadline by either creating an http.Client with a sensible Timeout (e.g., 5–15s)
and use that client to Do(req) or ensure the provided ctx has a deadline (wrap
ctx with context.WithTimeout when nil) before calling
http.NewRequestWithContext; keep the existing error handling and response
decoding logic (references: fetchLatestVersion, latestReleaseURL,
http.NewRequestWithContext, resp.Body.Close, json.NewDecoder.Decode) so the
request cannot hang indefinitely.
- Around line 236-277: Move permission setting to the new binary before
replacing the existing executable: in extractAndReplace, after obtaining info :=
os.Stat(exePath) and before calling os.Rename(newBinary, exePath), call
os.Chmod(newBinary, info.Mode()) and handle any error; then perform the rename
(and keep the existing fallback to copyFile which should use info.Mode()).
Remove the final os.Chmod(exePath, info.Mode()) after the rename so we don't
return an error for a completed replacement; reference symbols:
extractAndReplace, newBinary, exePath, info.Mode(), os.Rename, os.Chmod,
copyFile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e7f5ae02-53d9-475e-9a38-bc55293cc9d7
📒 Files selected for processing (12)
cmd/root.gocmd/update.gointernal/container/logs.gointernal/output/events.gointernal/output/plain_format.gointernal/output/plain_sink_test.gointernal/ui/app.gointernal/ui/run_update.gointernal/update/install_method.gointernal/update/install_method_test.gointernal/update/update.gotest/integration/update_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/update_test.go (2)
98-139: Network-dependent test downloads real binaries from GitHub.This test makes live calls to GitHub's API and downloads actual release binaries:
- Line 126: The update command calls
fetchLatestVersion()which hitshttps://api.github.com/repos/localstack/lstk/releases/latest- It then downloads the binary from GitHub releases via
updateBinary()While this provides true end-to-end coverage, be aware:
- GitHub API has rate limits (60 requests/hour for unauthenticated)
- Download size/time could vary with release artifacts
- Test will fail if GitHub is unreachable
Consider adding a comment documenting these constraints, or setting the
GITHUB_TOKENenvironment variable in CI to increase rate limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/update_test.go` around lines 98 - 139, TestUpdateBinaryInPlace performs live network calls (via fetchLatestVersion() and updateBinary()) to GitHub which can hit API rate limits and be flaky; update the test file to add a short comment above TestUpdateBinaryInPlace documenting these constraints (rate limits, possible download timeout/failure, and suggestion to set GITHUB_TOKEN in CI) and optionally note that the test is network-dependent so CI should set GITHUB_TOKEN or run it conditionally.
40-96: Consider the flakiness risk from real network calls.This test performs real network operations:
- Line 57-60:
npm install@localstack/lstk`` fetches from npm registry- Line 87-92: The update command calls GitHub API and may download binaries
While this is valid for integration testing, these external dependencies can cause intermittent failures in CI due to rate limits or network issues. Consider:
- Adding retry logic or longer timeouts for network operations
- Documenting that this test requires network access
- Potentially skipping in environments where network is restricted
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/update_test.go` around lines 40 - 96, The TestUpdateNPMLocalInstall integration test performs real network operations (npmInstall created by exec.CommandContext in TestUpdateNPMLocalInstall and the nmBinaryPath execution via cmd exec.CommandContext which triggers the update flow), making CI flaky; update the test to either add robust retry/backoff and increased timeouts around the npm install and the update invocation, or gate/skip the test when network is unavailable (e.g., check a NETWORK_INTEGRATION env var or attempt a lightweight connectivity probe before running) and document the network requirement in the test comment or README so CI runners know to enable network-dependent tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/update_test.go`:
- Around line 98-139: TestUpdateBinaryInPlace performs live network calls (via
fetchLatestVersion() and updateBinary()) to GitHub which can hit API rate limits
and be flaky; update the test file to add a short comment above
TestUpdateBinaryInPlace documenting these constraints (rate limits, possible
download timeout/failure, and suggestion to set GITHUB_TOKEN in CI) and
optionally note that the test is network-dependent so CI should set GITHUB_TOKEN
or run it conditionally.
- Around line 40-96: The TestUpdateNPMLocalInstall integration test performs
real network operations (npmInstall created by exec.CommandContext in
TestUpdateNPMLocalInstall and the nmBinaryPath execution via cmd
exec.CommandContext which triggers the update flow), making CI flaky; update the
test to either add robust retry/backoff and increased timeouts around the npm
install and the update invocation, or gate/skip the test when network is
unavailable (e.g., check a NETWORK_INTEGRATION env var or attempt a lightweight
connectivity probe before running) and document the network requirement in the
test comment or README so CI runners know to enable network-dependent tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fc3f8741-4c36-4892-bb0e-ef97c960fa51
📒 Files selected for processing (1)
test/integration/update_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/update/update.go`:
- Around line 255-260: The "zip" branch in the switch (used by extractAndReplace
and located in the case "zip" block) must not call the external "unzip" binary;
replace it with Go's archive/zip-based extraction: open the zip with
zip.OpenReader, iterate files, create directories as needed, and extract each
file into the target dir (overwriting existing files and preserving file
permissions/exec bits where applicable), handling directories and symlinks
correctly and returning wrapped errors on failure; ensure you close the zip
reader and properly create parent directories before writing files so extraction
works cross-platform on Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6ea5d5c0-4741-4e32-a82b-d972ac35d0e9
📒 Files selected for processing (4)
cmd/update.gointernal/output/plain_sink_test.gointernal/update/update.gotest/integration/update_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/output/plain_sink_test.go
silv-io
commented
Mar 10, 2026
| ver := normalizeVersion(tag) | ||
| assetName := buildAssetName(ver, goruntime.GOOS, goruntime.GOARCH) | ||
|
|
||
| downloadURL := fmt.Sprintf("https://github.com/%s/releases/download/%s/%s", githubRepo, tag, assetName) |
Member
Author
There was a problem hiding this comment.
I think we should replace this with a location we control in the future
silv-io
force-pushed
the
silv-io/flc-413
branch
from
3968936 to
6b053bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 107-111: Remove GITHUB_TOKEN from the broad env block so the token
is not exported to every test subprocess; in the env section remove the
GITHUB_TOKEN entry and instead pass the token only to the specific step(s) or a
separate job that require GitHub-authenticated operations (e.g., the
npm/self-update or any GitHub API steps). Locate the env block that contains the
LSTK_TEST_HOMEBREW and CREATE_JUNIT_REPORT entries and update the workflow to
scope GITHUB_TOKEN narrowly (either by adding it to a single step's env or by
creating an isolated job for GitHub-authenticated checks) rather than keeping
the GITHUB_TOKEN variable globally available to make test-integration and its
child processes.
In `@internal/update/extract.go`:
- Around line 91-101: The code currently builds target := filepath.Join(destDir,
filepath.Clean(hdr.Name)) which still allows absolute paths or paths with .. to
escape destDir; update the extraction logic (the block that reads hdr, target,
destDir and handles tar.TypeDir/tar.TypeReg) to validate entries before creating
files: reject absolute hdr.Name (filepath.IsAbs), compute rel, err :=
filepath.Rel(destDir, filepath.Join(destDir, hdr.Name)) and if err != nil or
strings.HasPrefix(rel, ".."+string(os.PathSeparator)) or rel == ".." then return
an error and skip the entry; apply the same validation to the other extraction
block (the code at lines ~123-137) so no archive entry can resolve outside
destDir.
In `@internal/update/update.go`:
- Around line 40-72: Detect the install method before deciding update
availability: call DetectInstallMethod() at the top of Update and use that to
choose the update source (InstallHomebrew / InstallNPM branches) before relying
on Check()'s GitHub-based result; for package-manager branches (updateHomebrew,
updateNPM) after performing the update re-check the installed version (by
re-running Check or another version-detect routine) and only emit success
(output.EmitSuccess) when the installed version actually changed to the expected
latest—reference Update, Check, DetectInstallMethod, InstallHomebrew,
InstallNPM, updateHomebrew, updateNPM, updateBinary, and npmProjectDir when
locating where to make these changes.
- Around line 30-36: The code only checks equality via
normalizeVersion(current)==normalizeVersion(latest) and treats any mismatch as
an available update; instead parse/compare versions using proper semantic
ordering (e.g., a semver parse/Compare function) instead of just normalization:
in update.go around the block using normalizeVersion/current/latest and
output.EmitInfo/EmitNote, determine ordering (latest > current => emit "Update
available" and return available=true; latest == current => emit "Already up to
date" and return available=false; latest < current => emit a note like "Current
version is newer than latest" and return available=false). Use or add a
versionCompare/semver.Compare helper to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: efac9f83-8fe1-481d-82fb-2abd058692bd
📒 Files selected for processing (7)
.github/workflows/ci.ymlinternal/update/extract.gointernal/update/github.gointernal/update/homebrew.gointernal/update/npm.gointernal/update/update.gotest/integration/update_test.go
Comment on lines
+91
to
+101
| target := filepath.Join(destDir, filepath.Clean(hdr.Name)) | ||
| switch hdr.Typeflag { | ||
| case tar.TypeDir: | ||
| if err := os.MkdirAll(target, 0o755); err != nil { | ||
| return err | ||
| } | ||
| case tar.TypeReg: | ||
| if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { | ||
| return err | ||
| } | ||
| out, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, hdr.FileInfo().Mode()) |
There was a problem hiding this comment.
Reject archive entries that escape destDir.
filepath.Clean is not enough here: ../../... and absolute names can still resolve outside the extraction root. A malicious or compromised release archive can overwrite arbitrary files during self-update.
🔒 Suggested hardening
import (
"archive/tar"
"archive/zip"
"compress/gzip"
"fmt"
"io"
"os"
"path/filepath"
goruntime "runtime"
+ "strings"
)
+func safeExtractPath(destDir, name string) (string, error) {
+ cleanDest := filepath.Clean(destDir)
+ cleanName := filepath.Clean(name)
+ if filepath.IsAbs(cleanName) {
+ return "", fmt.Errorf("archive entry %q escapes extraction dir", name)
+ }
+
+ target := filepath.Join(cleanDest, cleanName)
+ rel, err := filepath.Rel(cleanDest, target)
+ if err != nil {
+ return "", err
+ }
+ if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+ return "", fmt.Errorf("archive entry %q escapes extraction dir", name)
+ }
+ return target, nil
+}
+
func extractTarGz(archivePath, destDir string) error {
@@
- target := filepath.Join(destDir, filepath.Clean(hdr.Name))
+ target, err := safeExtractPath(destDir, hdr.Name)
+ if err != nil {
+ return err
+ }
switch hdr.Typeflag {
@@
func extractZip(archivePath, destDir string) error {
@@
- target := filepath.Join(destDir, filepath.Clean(f.Name))
+ target, err := safeExtractPath(destDir, f.Name)
+ if err != nil {
+ return err
+ }
if f.FileInfo().IsDir() {Also applies to: 123-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/update/extract.go` around lines 91 - 101, The code currently builds
target := filepath.Join(destDir, filepath.Clean(hdr.Name)) which still allows
absolute paths or paths with .. to escape destDir; update the extraction logic
(the block that reads hdr, target, destDir and handles tar.TypeDir/tar.TypeReg)
to validate entries before creating files: reject absolute hdr.Name
(filepath.IsAbs), compute rel, err := filepath.Rel(destDir,
filepath.Join(destDir, hdr.Name)) and if err != nil or strings.HasPrefix(rel,
".."+string(os.PathSeparator)) or rel == ".." then return an error and skip the
entry; apply the same validation to the other extraction block (the code at
lines ~123-137) so no archive entry can resolve outside destDir.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/update/extract.go (1)
91-101:⚠️ Potential issue | 🔴 CriticalReject archive entries that escape
destDir.
filepath.Clean+filepath.Joinis still not enough here: absolute names and../segments can resolve outside the extraction root. A malicious or compromised release archive can overwrite arbitrary files during self-update. Please validate each tar/zip entry againstdestDirbefore creating directories or files.🔒 Hardening sketch
import ( "archive/tar" "archive/zip" "compress/gzip" "fmt" "io" "os" "path/filepath" goruntime "runtime" + "strings" ) + +func safeExtractPath(destDir, name string) (string, error) { + cleanDest := filepath.Clean(destDir) + cleanName := filepath.Clean(name) + if filepath.IsAbs(cleanName) { + return "", fmt.Errorf("archive entry %q escapes extraction dir", name) + } + + target := filepath.Join(cleanDest, cleanName) + rel, err := filepath.Rel(cleanDest, target) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", fmt.Errorf("archive entry %q escapes extraction dir", name) + } + return target, nil +} @@ - target := filepath.Join(destDir, filepath.Clean(hdr.Name)) + target, err := safeExtractPath(destDir, hdr.Name) + if err != nil { + return err + } @@ - target := filepath.Join(destDir, filepath.Clean(f.Name)) + target, err := safeExtractPath(destDir, f.Name) + if err != nil { + return err + }Also applies to: 123-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/extract.go` around lines 91 - 101, The extraction code constructs target from destDir and hdr.Name but does not prevent path traversal or absolute names; validate each entry before creating files/dirs by resolving both destDir and the joined target to absolute, using filepath.Clean/Abs, then ensure the resulting target is inside destDir (e.g., via filepath.Rel and rejecting when the rel path begins with ".." or when target is not under destDir). If validation fails for hdr.Name, return an error and skip creation; apply the same check for the zip extraction branch (the code around lines 123-137) to protect against ../ and absolute paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/app.go`:
- Around line 164-170: The buffered log path currently appends directly to
a.bufferedLines while a.spinner.PendingStop() is true, which leaves
bufferedLines unbounded; change the append to reuse the same bounded helper by
calling appendLine(a.bufferedLines, line) (or the appropriate signature) so the
buffer is capped the same way as a.lines; update the branch that checks
a.spinner.PendingStop() to use appendLine for a.bufferedLines instead of
a.bufferedLines = append(...).
In `@internal/update/github.go`:
- Around line 29-39: githubRequest currently uses http.DefaultClient (no
timeout) which can hang; replace it to use a dedicated *http.Client with a
bounded Timeout and optionally make the client configurable/injectable into the
updater. Update the githubRequest signature (or create an internal helper) to
accept or use a client configured with a sensible timeout (e.g., configurable
via env or constructor) and swap http.DefaultClient.Do(req) to client.Do(req);
also review updateBinary and ensure it either uses a longer timeout client for
large downloads or accepts a configurable client so callers can choose a longer
timeout for binary downloads.
In `@internal/update/install_method.go`:
- Around line 51-63: The classifyPath function misclassifies installs by
substring matching (e.g., "/caskroom/" and "node_modules") — change it to split
the cleaned path into segments and check for exact directory names instead: in
classifyPath, sanitize and lowercase the resolved path, split it by the OS path
separator into elements, and then test if any element equals "Caskroom" (or
"caskroom") to return InstallHomebrew or equals "node_modules" to return
InstallNPM; keep using InstallMethod/InstallHomebrew/InstallNPM symbols so the
matching is exact and avoids false positives like "node_modules-backup".
In `@test/integration/update_test.go`:
- Around line 175-200: The test overwrites the real Caskroom binary (caskBinary)
and never restores it; before running buildCmd (the fake 0.0.1 build) copy or
move the existing caskBinary to a temporary backup and register a t.Cleanup that
restores the backup (or removes the fake and moves the backup back) after the
test completes; alternatively set up and point caskBinary to a sandboxed
Homebrew prefix for buildCmd/updateCmd so the real system binary is untouched.
Ensure the cleanup handles cases where no original existed (remove fake) and
where a backup exists (restore it).
---
Duplicate comments:
In `@internal/update/extract.go`:
- Around line 91-101: The extraction code constructs target from destDir and
hdr.Name but does not prevent path traversal or absolute names; validate each
entry before creating files/dirs by resolving both destDir and the joined target
to absolute, using filepath.Clean/Abs, then ensure the resulting target is
inside destDir (e.g., via filepath.Rel and rejecting when the rel path begins
with ".." or when target is not under destDir). If validation fails for
hdr.Name, return an error and skip creation; apply the same check for the zip
extraction branch (the code around lines 123-137) to protect against ../ and
absolute paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2a0ac53e-93a5-4bcd-aba6-4cbfeb24a9da
📒 Files selected for processing (17)
.github/workflows/ci.ymlcmd/root.gocmd/update.gointernal/container/logs.gointernal/output/events.gointernal/output/plain_format.gointernal/output/plain_sink_test.gointernal/ui/app.gointernal/ui/run_update.gointernal/update/extract.gointernal/update/github.gointernal/update/homebrew.gointernal/update/install_method.gointernal/update/install_method_test.gointernal/update/npm.gointernal/update/update.gotest/integration/update_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/root.go
- internal/output/events.go
- internal/update/update.go
- internal/update/npm.go
- .github/workflows/ci.yml
- internal/update/install_method_test.go
- cmd/update.go
- internal/output/plain_format.go
- internal/update/homebrew.go
Comment on lines
+51
to
+63
| func classifyPath(resolved string) InstallMethod { | ||
| lower := strings.ToLower(resolved) | ||
|
|
||
| // Homebrew cask: symlink resolves into /Caskroom/ | ||
| if strings.Contains(lower, "/caskroom/") { | ||
| return InstallHomebrew | ||
| } | ||
|
|
||
| // npm install: the Go binary lives inside a node_modules directory | ||
| // e.g. <prefix>/lib/node_modules/@localstack/lstk_darwin_arm64/lstk | ||
| if strings.Contains(lower, "node_modules") { | ||
| return InstallNPM | ||
| } |
There was a problem hiding this comment.
Match install roots by path segment, not substring.
strings.Contains(lower, "node_modules") and strings.Contains(lower, "/caskroom/") can misclassify unrelated paths like /opt/node_modules-backup/lstk as managed installs. From there lstk update takes the wrong branch and can run npm install -g or brew upgrade against a standalone binary. Split the cleaned path into elements and match exact directory names instead.
🛠️ Safer path matching
func classifyPath(resolved string) InstallMethod {
- lower := strings.ToLower(resolved)
+ parts := strings.FieldsFunc(strings.ToLower(filepath.Clean(resolved)), func(r rune) bool {
+ return r == '/' || r == '\\'
+ })
+ hasPart := func(want string) bool {
+ for _, part := range parts {
+ if part == want {
+ return true
+ }
+ }
+ return false
+ }
- // Homebrew cask: symlink resolves into /Caskroom/
- if strings.Contains(lower, "/caskroom/") {
+ if hasPart("caskroom") {
return InstallHomebrew
}
- // npm install: the Go binary lives inside a node_modules directory
- // e.g. <prefix>/lib/node_modules/@localstack/lstk_darwin_arm64/lstk
- if strings.Contains(lower, "node_modules") {
+ if hasPart("node_modules") {
return InstallNPM
}
return InstallBinary
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/update/install_method.go` around lines 51 - 63, The classifyPath
function misclassifies installs by substring matching (e.g., "/caskroom/" and
"node_modules") — change it to split the cleaned path into segments and check
for exact directory names instead: in classifyPath, sanitize and lowercase the
resolved path, split it by the OS path separator into elements, and then test if
any element equals "Caskroom" (or "caskroom") to return InstallHomebrew or
equals "node_modules" to return InstallNPM; keep using
InstallMethod/InstallHomebrew/InstallNPM symbols so the matching is exact and
avoids false positives like "node_modules-backup".
Comment on lines
+175
to
+200
| // Build a fake old version into the Caskroom location | ||
| buildCmd := exec.CommandContext(ctx, "go", "build", | ||
| "-ldflags", "-X github.com/localstack/lstk/internal/version.version=0.0.1", | ||
| "-o", caskBinary, | ||
| ".", | ||
| ) | ||
| buildCmd.Dir = repoRoot | ||
| out, err := buildCmd.CombinedOutput() | ||
| require.NoError(t, err, "go build failed: %s", string(out)) | ||
|
|
||
| // Verify it reports the fake version | ||
| verCmd := exec.CommandContext(ctx, caskBinary, "version") | ||
| verOut, err := verCmd.CombinedOutput() | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(verOut), "0.0.1") | ||
|
|
||
| // Run update — should detect Homebrew and run brew upgrade | ||
| // Note: brew may consider lstk already up-to-date (its metadata tracks the | ||
| // cask version, not the actual binary content), so "Updated to" may or may | ||
| // not appear. We verify detection and that brew was invoked without error. | ||
| updateCmd := exec.CommandContext(ctx, caskBinary, "update", "--non-interactive") | ||
| updateOut, err := updateCmd.CombinedOutput() | ||
| updateStr := string(updateOut) | ||
| require.NoError(t, err, "lstk update failed: %s", updateStr) | ||
| assert.Contains(t, updateStr, "Homebrew", "should detect Homebrew install") | ||
| assert.Contains(t, updateStr, "brew upgrade", "should mention brew upgrade") |
There was a problem hiding this comment.
Restore the Homebrew binary in cleanup.
This test replaces the real Caskroom binary with a fake 0.0.1 build, but the assertions explicitly allow brew upgrade to do nothing. In that case the test still passes and leaves the machine with the fake binary installed. Please back up caskBinary before Line 175 and restore it with t.Cleanup, or run the test against a sandboxed Homebrew prefix instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/update_test.go` around lines 175 - 200, The test overwrites
the real Caskroom binary (caskBinary) and never restores it; before running
buildCmd (the fake 0.0.1 build) copy or move the existing caskBinary to a
temporary backup and register a t.Cleanup that restores the backup (or removes
the fake and moves the backup back) after the test completes; alternatively set
up and point caskBinary to a sandboxed Homebrew prefix for buildCmd/updateCmd so
the real system binary is untouched. Ensure the cleanup handles cases where no
original existed (remove fake) and where a backup exists (restore it).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
lstk updateto self-update the CLI, respecting the original installation method:brew upgrade localstack/tap/lstknode_modulespath, runsnpm install -g(global) ornpm installfrom project dir (local)--checkflag to only check for updates without applying -> will enable update notifications later :)Also generalizes
ContainerLogLineEvent→LogLineEventwith aSourcefield, so brew/npm subprocess output streams through the TUI with abrew | .../npm | ...prefix.Tests
classifyPath) and npm project dir resolutionFLC-413