From 926921950c7d889a588dfc68d1c7324a017f26a9 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Mon, 1 Jun 2026 20:39:19 +0800 Subject: [PATCH 1/2] Harden HTTP, dry-run, and command guardrails --- internal/archtest/baseline/dryrun.txt | 57 ++++++++++--------- internal/archtest/baseline/no-direct-exec.txt | 6 +- internal/archtest/baseline/no-raw-http.txt | 17 +----- internal/archtest/http_test.go | 54 +++++++++++++++--- internal/cli/install.go | 2 +- internal/cli/snapshot.go | 2 +- internal/cli/snapshot_import.go | 7 ++- internal/config/packages_remote.go | 4 +- internal/config/remote.go | 5 +- internal/httputil/ratelimit.go | 20 ++++++- internal/httputil/ratelimit_test.go | 42 ++++++++++++++ internal/installer/installer.go | 27 +++++++-- internal/installer/step_packages.go | 47 +++++++++++++-- internal/npm/npm.go | 45 ++++++++++++--- internal/npm/runner.go | 30 ++++++++++ internal/shell/shell.go | 10 +++- internal/system/system.go | 20 ++++++- internal/system/system_test.go | 29 ++++++++++ internal/updater/updater.go | 27 +++++++-- 19 files changed, 361 insertions(+), 90 deletions(-) diff --git a/internal/archtest/baseline/dryrun.txt b/internal/archtest/baseline/dryrun.txt index c510863..2a2bc53 100644 --- a/internal/archtest/baseline/dryrun.txt +++ b/internal/archtest/baseline/dryrun.txt @@ -1,28 +1,33 @@ # Baseline for archtest rule "dryrun". -# Each line is : of a known existing violation. +# Each line is : # reason for a known existing violation. # Regenerate: ARCHTEST_UPDATE_BASELINE=1 go test ./internal/archtest/... -internal/doctor/doctor.go:50 -internal/doctor/doctor.go:54 -internal/dotfiles/dotfiles.go:260 -internal/dotfiles/dotfiles.go:270 -internal/dotfiles/dotfiles.go:327 -internal/shell/shell.go:242 -internal/shell/shell.go:245 -internal/shell/shell.go:246 -internal/snapshot/capture.go:200 -internal/snapshot/capture.go:239 -internal/snapshot/capture.go:269 -internal/snapshot/capture.go:303 -internal/snapshot/capture.go:307 -internal/snapshot/capture.go:335 -internal/snapshot/capture.go:361 -internal/snapshot/local.go:22 -internal/snapshot/local.go:32 -internal/snapshot/local.go:35 -internal/snapshot/local.go:36 -internal/sync/diff.go:275 -internal/sync/source.go:63 -internal/sync/source.go:73 -internal/sync/source.go:76 -internal/sync/source.go:77 -internal/sync/source.go:91 + +# Audited exemptions: read-only probes or helpers already gated by callers. +internal/doctor/doctor.go:50 # read-only diagnostic runner; doctor does not mutate user state +internal/doctor/doctor.go:54 # read-only diagnostic runner; doctor does not mutate user state +internal/dotfiles/dotfiles.go:260 # helper writes conflict backups only from linkWithStow after Link dry-run gate +internal/dotfiles/dotfiles.go:270 # helper restores conflict backups only after a non-dry-run stow failure +internal/dotfiles/dotfiles.go:327 # helper removes conflict files only from linkWithStow after Link dry-run gate +internal/shell/shell.go:248 # helper patches .zshrc only after RestoreFromSnapshot dry-run gate +internal/shell/shell.go:251 # helper patches .zshrc only after RestoreFromSnapshot dry-run gate +internal/shell/shell.go:252 # helper cleanup only after RestoreFromSnapshot dry-run gate +internal/snapshot/capture.go:200 # read-only package inventory probe +internal/snapshot/capture.go:239 # read-only package inventory probe +internal/snapshot/capture.go:269 # read-only macOS defaults probe +internal/snapshot/capture.go:303 # read-only git config probe +internal/snapshot/capture.go:307 # read-only git config probe +internal/snapshot/capture.go:335 # read-only developer tool version probe +internal/snapshot/capture.go:361 # read-only dotfiles remote probe +internal/sync/diff.go:275 # read-only dotfiles remote probe for diff display + +# Debt: state persistence is intentionally outside user config mutation, but +# should move to explicit dry-run-aware APIs instead of baseline-only reasoning. +internal/snapshot/local.go:22 # state persistence: creates ~/.openboot for saved snapshot +internal/snapshot/local.go:32 # state persistence: atomic snapshot temp write +internal/snapshot/local.go:35 # state persistence: atomic snapshot rename +internal/snapshot/local.go:36 # state persistence: temp-file cleanup after failed rename +internal/sync/source.go:63 # state persistence: creates ~/.openboot for sync source +internal/sync/source.go:73 # state persistence: atomic sync source temp write +internal/sync/source.go:76 # state persistence: atomic sync source rename +internal/sync/source.go:77 # state persistence: temp-file cleanup after failed rename +internal/sync/source.go:91 # state persistence: removes sync source metadata diff --git a/internal/archtest/baseline/no-direct-exec.txt b/internal/archtest/baseline/no-direct-exec.txt index 35c899b..7bda143 100644 --- a/internal/archtest/baseline/no-direct-exec.txt +++ b/internal/archtest/baseline/no-direct-exec.txt @@ -12,9 +12,9 @@ internal/dotfiles/dotfiles.go:66 internal/dotfiles/dotfiles.go:284 internal/dotfiles/dotfiles.go:379 internal/installer/step_system.go:85 -internal/npm/npm.go:21 +internal/npm/npm.go:22 internal/permissions/screen_recording_cgo.go:21 -internal/shell/shell.go:171 -internal/updater/updater.go:198 +internal/shell/shell.go:177 internal/updater/updater.go:205 internal/updater/updater.go:212 +internal/updater/updater.go:219 diff --git a/internal/archtest/baseline/no-raw-http.txt b/internal/archtest/baseline/no-raw-http.txt index 07212a5..581de79 100644 --- a/internal/archtest/baseline/no-raw-http.txt +++ b/internal/archtest/baseline/no-raw-http.txt @@ -1,16 +1,5 @@ # Baseline for archtest rule "no-raw-http". -# Each line is : of a known existing violation. +# Each line is : # reason for a known existing violation. # Regenerate: ARCHTEST_UPDATE_BASELINE=1 go test ./internal/archtest/... -internal/auth/login.go:106 -internal/auth/login.go:164 -internal/cli/snapshot_import.go:71 -internal/cli/snapshot_publish.go:142 -internal/config/packages_remote.go:82 -internal/config/packages_remote.go:87 -internal/config/remote.go:56 -internal/config/remote.go:75 -internal/config/remote.go:233 -internal/search/search.go:46 -internal/shell/shell.go:29 -internal/shell/shell.go:63 -internal/updater/updater.go:792 +internal/config/packages_remote.go:84 # injects the default transport into a test-swappable client; round-trip uses httputil.Do +internal/config/remote.go:57 # injects the default transport into a version-header transport; round-trip uses httputil.Do diff --git a/internal/archtest/http_test.go b/internal/archtest/http_test.go index 3ed1484..f77e9b2 100644 --- a/internal/archtest/http_test.go +++ b/internal/archtest/http_test.go @@ -1,10 +1,14 @@ package archtest -import "testing" +import ( + "go/ast" + "testing" +) -// httpAllowedPaths is the set of packages allowed to construct raw HTTP -// requests or reference net/http globals. Everything else must go through -// internal/httputil.Do, which handles 429 + Retry-After uniformly. +// httpAllowedPaths is the set of packages allowed to perform raw HTTP +// round-trips or reference net/http globals. Everything else must route +// requests through internal/httputil.Do, which handles 429 + Retry-After +// uniformly. Request construction may remain local to call sites. var httpAllowedPaths = []string{ "internal/httputil", // owns the wrapper } @@ -15,17 +19,53 @@ var httpAllowedPaths = []string{ func TestNoRawHTTPNewRequest(t *testing.T) { r := rule{ name: "no-raw-http", - fix: "Use internal/httputil.Do for HTTP calls — it handles 429 + Retry-After and per-call rate limiting. Keep request construction local; only the round-trip goes through httputil.", + fix: "Use internal/httputil.Do for HTTP round-trips — it handles 429 + Retry-After and per-call rate limiting. Keep request construction local; only the round-trip goes through httputil.", } var violations []callSite for _, gf := range productionFiles(t) { if inAllowedPath(gf.path, httpAllowedPaths) { continue } - violations = append(violations, findCall(gf, "net/http", "NewRequest")...) - violations = append(violations, findCall(gf, "net/http", "NewRequestWithContext")...) + violations = append(violations, findRawHTTPDoCalls(gf)...) violations = append(violations, findRef(gf, "net/http", "DefaultClient")...) violations = append(violations, findRef(gf, "net/http", "DefaultTransport")...) } enforce(t, r, violations) } + +func findRawHTTPDoCalls(gf goFile) []callSite { + if importedAs(gf.file, "net/http") == "" { + return nil + } + + httputilLocal := importedAs(gf.file, "github.com/openbootdotdev/openboot/internal/httputil") + var out []callSite + ast.Inspect(gf.file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Do" { + return true + } + if len(call.Args) != 1 { + return true + } + argIdent, ok := call.Args[0].(*ast.Ident) + if !ok || argIdent.Name != "req" { + return true + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + if httputilLocal != "" && ident.Name == httputilLocal { + return true + } + p := gf.fset.Position(call.Pos()) + out = append(out, callSite{file: gf.path, line: p.Line}) + return true + }) + return out +} diff --git a/internal/cli/install.go b/internal/cli/install.go index 9f5a328..56afc71 100644 --- a/internal/cli/install.go +++ b/internal/cli/install.go @@ -141,7 +141,7 @@ func runInstallCmd(cmd *cobra.Command, args []string) error { return fmt.Errorf("--pick requires a remote config; use the preset selector instead") } - err := installer.Run(installCfg) + err := installer.RunContext(cmd.Context(), installCfg) if errors.Is(err, installer.ErrUserCancelled) { return nil } diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index d78cbf6..08a31e0 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -62,7 +62,7 @@ func runSnapshot(cmd *cobra.Command) error { importFile, _ := cmd.Flags().GetString("import") if importFile != "" { dryRun, _ := cmd.Flags().GetBool("dry-run") - return runSnapshotImport(importFile, dryRun) + return runSnapshotImportContext(cmd.Context(), importFile, dryRun) } localFlag, _ := cmd.Flags().GetBool("local") diff --git a/internal/cli/snapshot_import.go b/internal/cli/snapshot_import.go index 7ee1321..0392f64 100644 --- a/internal/cli/snapshot_import.go +++ b/internal/cli/snapshot_import.go @@ -1,6 +1,7 @@ package cli import ( + "context" "fmt" "io" "net/http" @@ -16,6 +17,10 @@ import ( ) func runSnapshotImport(importPath string, dryRun bool) error { + return runSnapshotImportContext(context.Background(), importPath, dryRun) +} + +func runSnapshotImportContext(ctx context.Context, importPath string, dryRun bool) error { snap, err := loadSnapshot(importPath) if err != nil { return fmt.Errorf("load snapshot: %w", err) @@ -64,7 +69,7 @@ func runSnapshotImport(importPath string, dryRun bool) error { return nil } - return installer.RunFromSnapshot(buildImportConfig(edited, dryRun)) + return installer.RunFromSnapshotContext(ctx, buildImportConfig(edited, dryRun)) } func downloadSnapshotBytes(url string, client *http.Client) ([]byte, error) { diff --git a/internal/config/packages_remote.go b/internal/config/packages_remote.go index a53eeb9..b01f0e5 100644 --- a/internal/config/packages_remote.go +++ b/internal/config/packages_remote.go @@ -8,6 +8,8 @@ import ( "os" "path/filepath" "time" + + "github.com/openbootdotdev/openboot/internal/httputil" ) // remotePackage matches the JSON returned by GET /api/packages. @@ -93,7 +95,7 @@ func fetchRemotePackages() ([]remotePackage, error) { Timeout: 8 * time.Second, Transport: &versionTransport{base: packagesHTTPTransport}, } - resp, err := client.Do(req) + resp, err := httputil.Do(client, req) if err != nil { return nil, fmt.Errorf("fetch packages: %w", err) } diff --git a/internal/config/remote.go b/internal/config/remote.go index 4a94a2e..89e107a 100644 --- a/internal/config/remote.go +++ b/internal/config/remote.go @@ -15,6 +15,7 @@ import ( "gopkg.in/yaml.v3" + "github.com/openbootdotdev/openboot/internal/httputil" "github.com/openbootdotdev/openboot/internal/system" ) @@ -81,7 +82,7 @@ func fetchConfigBySlug(apiBase, username, slug, token string) (*http.Response, e req.Header.Set("Authorization", "Bearer "+token) } - return remoteHTTPClient.Do(req) + return httputil.Do(remoteHTTPClient, req) } func parseConfigResponse(resp *http.Response, username, slug, token string) (*RemoteConfig, error) { @@ -239,7 +240,7 @@ func fetchConfigByAlias(apiBase, alias, token string) (*RemoteConfig, error) { req.Header.Set("Authorization", "Bearer "+token) } - resp, err := remoteHTTPClient.Do(req) + resp, err := httputil.Do(remoteHTTPClient, req) if err != nil { return nil, fmt.Errorf("fetch alias: %w", err) } diff --git a/internal/httputil/ratelimit.go b/internal/httputil/ratelimit.go index 9b95416..58c1e8b 100644 --- a/internal/httputil/ratelimit.go +++ b/internal/httputil/ratelimit.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "time" ) @@ -20,15 +21,28 @@ func (e *RateLimitError) Error() string { return fmt.Sprintf("Rate limited. Please wait %d seconds and try again.", e.RetryAfterSeconds) } -// parseRetryAfter reads the Retry-After header (in seconds) from a response. +// parseRetryAfter reads the Retry-After header from a response. +// It supports both RFC 9110 formats: delay-seconds and HTTP-date. // Returns 0 if the header is missing or unparseable. func parseRetryAfter(resp *http.Response) int { - val := resp.Header.Get("Retry-After") + val := strings.TrimSpace(resp.Header.Get("Retry-After")) if val == "" { return 0 } seconds, err := strconv.Atoi(val) - if err != nil || seconds < 0 { + if err == nil { + if seconds < 0 { + return 0 + } + return seconds + } + + retryAt, err := http.ParseTime(val) + if err != nil { + return 0 + } + seconds = int(time.Until(retryAt).Seconds()) + if seconds < 0 { return 0 } return seconds diff --git a/internal/httputil/ratelimit_test.go b/internal/httputil/ratelimit_test.go index e1a2ae6..659f675 100644 --- a/internal/httputil/ratelimit_test.go +++ b/internal/httputil/ratelimit_test.go @@ -159,6 +159,36 @@ func TestDo_MissingRetryAfterDefaultsToOneSecond(t *testing.T) { assert.Equal(t, 1*time.Second, sleptDuration) } +func TestDo_RetryAfterHTTPDate(t *testing.T) { + var calls atomic.Int32 + client := mockClient(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + if n == 1 { + w.Header().Set("Retry-After", time.Now().Add(2*time.Second).UTC().Format(http.TimeFormat)) + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + })) + + var sleptDuration time.Duration + originalSleep := sleepFunc + sleepFunc = func(d time.Duration) { sleptDuration = d } + defer func() { sleepFunc = originalSleep }() + + req, err := http.NewRequest("GET", fakeURL, nil) + require.NoError(t, err) + + resp, err := Do(client, req) + require.NoError(t, err) + defer resp.Body.Close() //nolint:errcheck + + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.GreaterOrEqual(t, sleptDuration, time.Second) + assert.LessOrEqual(t, sleptDuration, 2*time.Second) + assert.Equal(t, int32(2), calls.Load()) +} + func TestDo_WithRequestBody(t *testing.T) { var calls atomic.Int32 var receivedBodies []string @@ -247,6 +277,18 @@ func TestParseRetryAfter(t *testing.T) { } } +func TestParseRetryAfterHTTPDate(t *testing.T) { + resp := &http.Response{Header: http.Header{}} + resp.Header.Set("Retry-After", time.Now().Add(3*time.Second).UTC().Format(http.TimeFormat)) + + seconds := parseRetryAfter(resp) + assert.GreaterOrEqual(t, seconds, 1) + assert.LessOrEqual(t, seconds, 3) + + resp.Header.Set("Retry-After", time.Now().Add(-time.Second).UTC().Format(http.TimeFormat)) + assert.Equal(t, 0, parseRetryAfter(resp)) +} + func TestClampDuration(t *testing.T) { assert.Equal(t, 5*time.Second, clampDuration(5*time.Second, 60*time.Second)) assert.Equal(t, 60*time.Second, clampDuration(120*time.Second, 60*time.Second)) diff --git a/internal/installer/installer.go b/internal/installer/installer.go index 3f72585..1968051 100644 --- a/internal/installer/installer.go +++ b/internal/installer/installer.go @@ -1,6 +1,7 @@ package installer import ( + "context" "errors" "fmt" "log/slog" @@ -23,6 +24,10 @@ const ( ) func Run(cfg *config.Config) error { + return RunContext(context.Background(), cfg) +} + +func RunContext(ctx context.Context, cfg *config.Config) error { opts := cfg.ToInstallOptions() st := cfg.ToInstallState() @@ -30,7 +35,7 @@ func Run(cfg *config.Config) error { if opts.Update { err = runUpdate(opts, st) } else { - err = runInstall(opts, st) + err = runInstallContext(ctx, opts, st) } cfg.ApplyState(st) @@ -38,6 +43,10 @@ func Run(cfg *config.Config) error { } func runInstall(opts *config.InstallOptions, st *config.InstallState) error { + return runInstallContext(context.Background(), opts, st) +} + +func runInstallContext(ctx context.Context, opts *config.InstallOptions, st *config.InstallState) error { slog.Info("install_started", "version", opts.Version, "preset", opts.Preset, @@ -72,12 +81,16 @@ func runInstall(opts *config.InstallOptions, st *config.InstallState) error { st.OnlinePkgs = plan.OnlinePkgs } - return Apply(plan, ConsoleReporter{}) + return ApplyContext(ctx, plan, ConsoleReporter{}) } // Apply executes a resolved InstallPlan, reporting progress via r. // All user interaction has already happened in Plan(); this function only performs actions. func Apply(plan InstallPlan, r Reporter) error { + return ApplyContext(context.Background(), plan, r) +} + +func ApplyContext(ctx context.Context, plan InstallPlan, r Reporter) error { if !plan.PackagesOnly && !plan.SkipGit { if err := applyGitConfig(plan, r); err != nil { return fmt.Errorf("apply git config: %w", err) @@ -86,11 +99,11 @@ func Apply(plan InstallPlan, r Reporter) error { var softErrs []error - if err := applyPackages(plan, r); err != nil { + if err := applyPackages(ctx, plan, r); err != nil { softErrs = append(softErrs, fmt.Errorf("brew: %w", err)) } - if err := applyNpm(plan, r); err != nil { + if err := applyNpm(ctx, plan, r); err != nil { r.Error(fmt.Sprintf("npm package installation failed: %v", err)) softErrs = append(softErrs, fmt.Errorf("npm: %w", err)) } @@ -208,6 +221,10 @@ func checkDependencies(opts *config.InstallOptions, st *config.InstallState) err } func RunFromSnapshot(cfg *config.Config) error { + return RunFromSnapshotContext(context.Background(), cfg) +} + +func RunFromSnapshotContext(ctx context.Context, cfg *config.Config) error { opts := cfg.ToInstallOptions() st := cfg.ToInstallState() @@ -221,7 +238,7 @@ func RunFromSnapshot(cfg *config.Config) error { } plan := PlanFromSnapshot(opts, st) - err := Apply(plan, ConsoleReporter{}) + err := ApplyContext(ctx, plan, ConsoleReporter{}) cfg.ApplyState(st) return err } diff --git a/internal/installer/step_packages.go b/internal/installer/step_packages.go index 5de3d99..0682574 100644 --- a/internal/installer/step_packages.go +++ b/internal/installer/step_packages.go @@ -18,6 +18,15 @@ type categorizedPackages struct { npm []string } +const ( + brewInstallBaseTimeout = 30 * time.Minute + brewInstallTimeoutPerPackage = 20 * time.Minute + brewInstallMaxTimeout = 6 * time.Hour + npmInstallBaseTimeout = 10 * time.Minute + npmInstallTimeoutPerPackage = 10 * time.Minute + npmInstallMaxTimeout = 2 * time.Hour +) + func categorizeSelectedPackages(opts *config.InstallOptions, st *config.InstallState) categorizedPackages { var result categorizedPackages @@ -72,7 +81,7 @@ func categorizeSelectedPackages(opts *config.InstallOptions, st *config.InstallS return result } -func applyPackages(plan InstallPlan, r Reporter) error { //nolint:gocyclo // orchestrates multiple package categories; splitting would obscure the install sequence +func applyPackages(ctx context.Context, plan InstallPlan, r Reporter) error { //nolint:gocyclo // orchestrates multiple package categories; splitting would obscure the install sequence r.Header("Step 4: Installation") ui.Println() @@ -138,7 +147,9 @@ func applyPackages(plan InstallPlan, r Reporter) error { //nolint:gocyclo // orc r.Info(fmt.Sprintf("Installing %d packages (%d CLI, %d GUI)...", len(cliPkgs)+len(caskPkgs), len(cliPkgs), len(caskPkgs))) ui.Println() - installedCli, installedCask, brewErr := brew.InstallWithProgress(context.Background(), cliPkgs, caskPkgs, plan.DryRun) + brewCtx, cancel := packageInstallContext(ctx, len(cliPkgs)+len(caskPkgs)) + defer cancel() + installedCli, installedCask, brewErr := brew.InstallWithProgress(brewCtx, cliPkgs, caskPkgs, plan.DryRun) if brewErr != nil { r.Error(fmt.Sprintf("Some packages failed: %v", brewErr)) } @@ -162,7 +173,15 @@ func applyPackages(plan InstallPlan, r Reporter) error { //nolint:gocyclo // orc return brewErr } -func applyNpm(plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles npm batch + sequential fallback with per-package error tracking +func packageInstallContext(parent context.Context, totalPackages int) (context.Context, context.CancelFunc) { + timeout := brewInstallBaseTimeout + time.Duration(totalPackages)*brewInstallTimeoutPerPackage + if timeout > brewInstallMaxTimeout { + timeout = brewInstallMaxTimeout + } + return context.WithTimeout(parent, timeout) +} + +func applyNpm(ctx context.Context, plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles npm batch + sequential fallback with per-package error tracking npmPkgs := plan.Npm if len(npmPkgs) == 0 { return nil @@ -175,7 +194,7 @@ func applyNpm(plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles var newNpm []string if !plan.DryRun { - actualNpm, npmCheckErr := npm.GetInstalledPackages() + actualNpm, npmCheckErr := npm.GetInstalledPackagesContext(ctx) if npmCheckErr != nil { r.Warn(fmt.Sprintf("Failed to check installed npm packages: %v", npmCheckErr)) } else { @@ -212,8 +231,10 @@ func applyNpm(plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles maxAttempts := 3 var lastErr error + npmCtx, cancel := npmInstallContext(ctx, len(npmPkgs)) + defer cancel() for attempt := 1; attempt <= maxAttempts; attempt++ { - lastErr = npm.Install(npmPkgs, plan.DryRun) + lastErr = npm.InstallContext(npmCtx, npmPkgs, plan.DryRun) if lastErr == nil { break } @@ -223,7 +244,13 @@ func applyNpm(plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles } if plan.Silent || !system.HasTTY() { r.Warn(fmt.Sprintf("npm installation failed (attempt %d/%d), retrying...", attempt, maxAttempts)) - time.Sleep(time.Duration(attempt) * 2 * time.Second) + timer := time.NewTimer(time.Duration(attempt) * 2 * time.Second) + select { + case <-npmCtx.Done(): + timer.Stop() + return npmCtx.Err() + case <-timer.C: + } continue } ui.Println() @@ -243,3 +270,11 @@ func applyNpm(plan InstallPlan, r Reporter) error { //nolint:gocyclo // handles } return lastErr } + +func npmInstallContext(parent context.Context, totalPackages int) (context.Context, context.CancelFunc) { + timeout := npmInstallBaseTimeout + time.Duration(totalPackages)*npmInstallTimeoutPerPackage + if timeout > npmInstallMaxTimeout { + timeout = npmInstallMaxTimeout + } + return context.WithTimeout(parent, timeout) +} diff --git a/internal/npm/npm.go b/internal/npm/npm.go index d305a77..3d40296 100644 --- a/internal/npm/npm.go +++ b/internal/npm/npm.go @@ -1,6 +1,7 @@ package npm import ( + "context" "fmt" "os/exec" "strconv" @@ -43,9 +44,13 @@ func getNodeVersion() (int, error) { } func GetInstalledPackages() (map[string]bool, error) { + return GetInstalledPackagesContext(context.Background()) +} + +func GetInstalledPackagesContext(ctx context.Context) (map[string]bool, error) { packages := make(map[string]bool) - output, err := currentRunner().Output("list", "-g", "--depth=0", "--parseable") + output, err := runnerOutputContext(ctx, "list", "-g", "--depth=0", "--parseable") if err != nil { if len(output) > 0 { // npm list exits non-zero when packages have issues, but still @@ -78,6 +83,10 @@ func GetInstalledPackages() (map[string]bool, error) { } func Install(packages []string, dryRun bool) error { + return InstallContext(context.Background(), packages, dryRun) +} + +func InstallContext(ctx context.Context, packages []string, dryRun bool) error { if len(packages) == 0 { return nil } @@ -94,7 +103,7 @@ func Install(packages []string, dryRun bool) error { return nil } - installed, err := GetInstalledPackages() + installed, err := GetInstalledPackagesContext(ctx) if err != nil { return fmt.Errorf("list installed packages: %w", err) } @@ -119,7 +128,7 @@ func Install(packages []string, dryRun bool) error { ui.Info(fmt.Sprintf("Installing %d npm packages...", len(toInstall))) - failed, err := installBatch(toInstall) + failed, err := installBatchContext(ctx, toInstall) if err != nil { return fmt.Errorf("install npm packages: %w", err) } @@ -169,8 +178,12 @@ func warnIfNodeVersionTooLow(packages []string) { // fails it falls back to sequential per-package installs. Returns the list of // package names that could not be installed and any fatal error. func installBatch(toInstall []string) (failed []string, err error) { + return installBatchContext(context.Background(), toInstall) +} + +func installBatchContext(ctx context.Context, toInstall []string) (failed []string, err error) { args := append([]string{"install", "-g"}, toInstall...) - batchOutput, batchErr := currentRunner().CombinedOutput(args...) + batchOutput, batchErr := runnerCombinedOutputContext(ctx, args...) if batchErr == nil { ui.Success(fmt.Sprintf(" ✔ %d npm packages installed", len(toInstall))) @@ -181,13 +194,17 @@ func installBatch(toInstall []string) (failed []string, err error) { ui.Warn(fmt.Sprintf("Batch install failed (%s), falling back to sequential...", batchError)) ui.Println() - return installSequential(toInstall) + return installSequentialContext(ctx, toInstall) } // installSequential installs each package individually, skipping those that // were already picked up by a partial batch install. Returns failed package names. func installSequential(toInstall []string) (failed []string, err error) { - nowInstalled, err := GetInstalledPackages() + return installSequentialContext(context.Background(), toInstall) +} + +func installSequentialContext(ctx context.Context, toInstall []string) (failed []string, err error) { + nowInstalled, err := GetInstalledPackagesContext(ctx) if err != nil { return nil, fmt.Errorf("list packages after batch: %w", err) } @@ -209,7 +226,7 @@ func installSequential(toInstall []string) (failed []string, err error) { for _, pkg := range remaining { progress.SetCurrent(pkg) - errMsg := installNpmPackageWithRetry(pkg) + errMsg := installNpmPackageWithRetryContext(ctx, pkg) if errMsg != "" { progress.PrintLine(" ✗ %s (%s)", pkg, errMsg) failed = append(failed, pkg) @@ -260,9 +277,13 @@ func Uninstall(packages []string, dryRun bool) error { var retryBackoff = 2 * time.Second func installNpmPackageWithRetry(pkg string) string { + return installNpmPackageWithRetryContext(context.Background(), pkg) +} + +func installNpmPackageWithRetryContext(ctx context.Context, pkg string) string { maxAttempts := 3 for attempt := 1; attempt <= maxAttempts; attempt++ { - output, err := currentRunner().CombinedOutput("install", "-g", pkg) + output, err := runnerCombinedOutputContext(ctx, "install", "-g", pkg) if err == nil { return "" } @@ -270,7 +291,13 @@ func installNpmPackageWithRetry(pkg string) string { errMsg := parseNpmError(string(output)) if attempt < maxAttempts && isNpmRetryableError(errMsg) { delay := time.Duration(attempt) * retryBackoff - time.Sleep(delay) + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + timer.Stop() + return ctx.Err().Error() + case <-timer.C: + } continue } diff --git a/internal/npm/runner.go b/internal/npm/runner.go index a4003ab..cbf0323 100644 --- a/internal/npm/runner.go +++ b/internal/npm/runner.go @@ -1,6 +1,7 @@ package npm import ( + "context" "os/exec" "sync" ) @@ -27,6 +28,35 @@ func (execRunner) CombinedOutput(args ...string) ([]byte, error) { return exec.Command("npm", args...).CombinedOutput() //nolint:gosec // "npm" is a hardcoded binary; args are package names from validated config } +func (execRunner) OutputContext(ctx context.Context, args ...string) ([]byte, error) { + return exec.CommandContext(ctx, "npm", args...).Output() //nolint:gosec // "npm" is a hardcoded binary; args are package names from validated config +} + +func (execRunner) CombinedOutputContext(ctx context.Context, args ...string) ([]byte, error) { + return exec.CommandContext(ctx, "npm", args...).CombinedOutput() //nolint:gosec // "npm" is a hardcoded binary; args are package names from validated config +} + +type contextRunner interface { + OutputContext(ctx context.Context, args ...string) ([]byte, error) + CombinedOutputContext(ctx context.Context, args ...string) ([]byte, error) +} + +func runnerOutputContext(ctx context.Context, args ...string) ([]byte, error) { + r := currentRunner() + if cr, ok := r.(contextRunner); ok { + return cr.OutputContext(ctx, args...) + } + return r.Output(args...) +} + +func runnerCombinedOutputContext(ctx context.Context, args ...string) ([]byte, error) { + r := currentRunner() + if cr, ok := r.(contextRunner); ok { + return cr.CombinedOutputContext(ctx, args...) + } + return r.CombinedOutput(args...) +} + var ( runnerMu sync.RWMutex runner Runner = execRunner{} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 3bb5912..e94cf3a 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -1,6 +1,7 @@ package shell import ( + "context" "crypto/sha256" "encoding/hex" "fmt" @@ -11,6 +12,7 @@ import ( "path/filepath" "regexp" "strings" + "time" "github.com/openbootdotdev/openboot/internal/httputil" "github.com/openbootdotdev/openboot/internal/system" @@ -22,11 +24,13 @@ import ( // constant whenever the installer script changes upstream. const knownOMZInstallHash = "21043aec5b791ce4835479dc33ba2f92155946aeafd54604a8c83522627cc803" +const omzInstallTimeout = 10 * time.Minute + // omzInstallURL is a var so tests can redirect it without a real server. var omzInstallURL = "https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh" // omzHTTPClient is a var so tests can inject a mock transport. -var omzHTTPClient *http.Client = http.DefaultClient +var omzHTTPClient = &http.Client{Timeout: 30 * time.Second} var shellIdentifierRe = regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`) @@ -105,7 +109,9 @@ func InstallOhMyZsh(dryRun bool) error { return fmt.Errorf("chmod omz install script: %w", err) } - return system.RunCommand(tmpFile.Name(), "--unattended") + ctx, cancel := context.WithTimeout(context.Background(), omzInstallTimeout) + defer cancel() + return system.RunCommandContext(ctx, tmpFile.Name(), "--unattended") } const brewShellenvLine = `eval "$(/opt/homebrew/bin/brew shellenv)"` diff --git a/internal/system/system.go b/internal/system/system.go index 2fb3737..36abe98 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -1,6 +1,7 @@ package system import ( + "context" "fmt" "os" "os/exec" @@ -16,7 +17,11 @@ func HomeDir() (string, error) { } func RunCommand(name string, args ...string) error { - cmd := exec.Command(name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args + return RunCommandContext(context.Background(), name, args...) +} + +func RunCommandContext(ctx context.Context, name string, args ...string) error { + cmd := exec.CommandContext(ctx, name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin @@ -24,7 +29,11 @@ func RunCommand(name string, args ...string) error { } func RunCommandSilent(name string, args ...string) (string, error) { - cmd := exec.Command(name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args + return RunCommandSilentContext(context.Background(), name, args...) +} + +func RunCommandSilentContext(ctx context.Context, name string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args output, err := cmd.CombinedOutput() return strings.TrimSpace(string(output)), err } @@ -32,7 +41,12 @@ func RunCommandSilent(name string, args ...string) (string, error) { // RunCommandOutput runs name with args and returns stdout only (not stderr). // Use when stderr output must not contaminate parsed stdout (e.g. version probes, list commands). func RunCommandOutput(name string, args ...string) (string, error) { - cmd := exec.Command(name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args + return RunCommandOutputContext(context.Background(), name, args...) +} + +// RunCommandOutputContext runs name with args and returns stdout only (not stderr). +func RunCommandOutputContext(ctx context.Context, name string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args output, err := cmd.Output() return strings.TrimSpace(string(output)), err } diff --git a/internal/system/system_test.go b/internal/system/system_test.go index 7d5d7bf..b8e5f7e 100644 --- a/internal/system/system_test.go +++ b/internal/system/system_test.go @@ -1,9 +1,11 @@ package system import ( + "context" "os" "os/exec" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -340,6 +342,33 @@ func TestRunCommandOutput_CommandNotFound(t *testing.T) { assert.Error(t, err) } +func TestRunCommandContext_Cancelled(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + err := RunCommandContext(ctx, "sh", "-c", "sleep 1") + require.Error(t, err) + assert.ErrorIs(t, ctx.Err(), context.DeadlineExceeded) +} + +func TestRunCommandSilentContext_Cancelled(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + _, err := RunCommandSilentContext(ctx, "sh", "-c", "sleep 1") + require.Error(t, err) + assert.ErrorIs(t, ctx.Err(), context.DeadlineExceeded) +} + +func TestRunCommandOutputContext_Cancelled(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + _, err := RunCommandOutputContext(ctx, "sh", "-c", "sleep 1") + require.Error(t, err) + assert.ErrorIs(t, ctx.Err(), context.DeadlineExceeded) +} + // --------------------------------------------------------------------------- // HasTTY — ensure all branches are exercised // --------------------------------------------------------------------------- diff --git a/internal/updater/updater.go b/internal/updater/updater.go index 3ceef8e..eadf9f4 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -2,6 +2,7 @@ package updater import ( "bufio" + "context" "crypto/sha256" "encoding/hex" "encoding/json" @@ -21,11 +22,14 @@ import ( "syscall" "time" + "github.com/openbootdotdev/openboot/internal/httputil" "github.com/openbootdotdev/openboot/internal/ui" ) const CheckInterval = 24 * time.Hour +const brewUpgradeTimeout = 30 * time.Minute + var ( httpClient *http.Client httpClientOnce sync.Once @@ -194,22 +198,25 @@ const brewFormula = brewTap + "/openboot" // execBrewUpgrade is a package-level variable to allow test injection. var execBrewUpgrade = func(formula string) error { + ctx, cancel := context.WithTimeout(context.Background(), brewUpgradeTimeout) + defer cancel() + // Step 1: resolve the tap repository path. - repoOut, err := exec.Command("brew", "--repo", brewTap).Output() + repoOut, err := exec.CommandContext(ctx, "brew", "--repo", brewTap).Output() if err != nil { return fmt.Errorf("brew --repo %s: %w", brewTap, err) } repoPath := strings.TrimSpace(string(repoOut)) // Step 2: fast-forward the tap to pick up the new formula revision. - gitCmd := exec.Command("git", "-C", repoPath, "pull", "--ff-only") //nolint:gosec // "git" is hardcoded; repoPath is derived from brew --repository + gitCmd := exec.CommandContext(ctx, "git", "-C", repoPath, "pull", "--ff-only") //nolint:gosec // "git" is hardcoded; repoPath is derived from brew --repository gitCmd.Env = append(os.Environ(), "HOMEBREW_NO_AUTO_UPDATE=1") if err := gitCmd.Run(); err != nil { return fmt.Errorf("git pull tap: %w", err) } // Step 3: upgrade the formula. - upgradeCmd := exec.Command("brew", "upgrade", formula) //nolint:gosec // "brew" is hardcoded; formula is validated before this call + upgradeCmd := exec.CommandContext(ctx, "brew", "upgrade", formula) //nolint:gosec // "brew" is hardcoded; formula is validated before this call upgradeCmd.Env = append(os.Environ(), "HOMEBREW_NO_AUTO_UPDATE=1") if err := upgradeCmd.Run(); err != nil { return fmt.Errorf("brew upgrade %s: %w", formula, err) @@ -343,7 +350,11 @@ func verifyChecksum(path, filename string, checksums map[string]string) error { // /releases/download/v/ is used. func fetchChecksums(client *http.Client, version string) (map[string]string, error) { url := checksumsURL(version) - resp, err := client.Get(url) + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("create checksums request: %w", err) + } + resp, err := httputil.Do(client, req) if err != nil { return nil, fmt.Errorf("download checksums: %w", err) } @@ -407,7 +418,11 @@ func DownloadAndReplace(targetVersion, currentVersion string) error { url := binaryURL(targetVersion, filename) - resp, err := client.Get(url) + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return fmt.Errorf("create download request: %w", err) + } + resp, err := httputil.Do(client, req) if err != nil { return fmt.Errorf("download failed: %w", err) } @@ -796,7 +811,7 @@ func GetLatestVersion() (string, error) { if token := os.Getenv("GITHUB_TOKEN"); token != "" { req.Header.Set("Authorization", "Bearer "+token) } - resp, err := client.Do(req) + resp, err := httputil.Do(client, req) if err != nil { return "", fmt.Errorf("fetch latest version: %w", err) } From 04a957109da72add03d951312bc7f70dd6724848 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Mon, 1 Jun 2026 20:45:18 +0800 Subject: [PATCH 2/2] fix: remove unused backward-compat wrappers flagged by lint --- internal/cli/snapshot_import.go | 4 ---- internal/npm/npm.go | 12 ++---------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/internal/cli/snapshot_import.go b/internal/cli/snapshot_import.go index 0392f64..24491f5 100644 --- a/internal/cli/snapshot_import.go +++ b/internal/cli/snapshot_import.go @@ -16,10 +16,6 @@ import ( "github.com/openbootdotdev/openboot/internal/ui/tui" ) -func runSnapshotImport(importPath string, dryRun bool) error { - return runSnapshotImportContext(context.Background(), importPath, dryRun) -} - func runSnapshotImportContext(ctx context.Context, importPath string, dryRun bool) error { snap, err := loadSnapshot(importPath) if err != nil { diff --git a/internal/npm/npm.go b/internal/npm/npm.go index 3d40296..f9bd246 100644 --- a/internal/npm/npm.go +++ b/internal/npm/npm.go @@ -174,13 +174,9 @@ func warnIfNodeVersionTooLow(packages []string) { } } -// installBatch attempts a single batch install of all packages. If the batch +// installBatchContext attempts a single batch install of all packages. If the batch // fails it falls back to sequential per-package installs. Returns the list of // package names that could not be installed and any fatal error. -func installBatch(toInstall []string) (failed []string, err error) { - return installBatchContext(context.Background(), toInstall) -} - func installBatchContext(ctx context.Context, toInstall []string) (failed []string, err error) { args := append([]string{"install", "-g"}, toInstall...) batchOutput, batchErr := runnerCombinedOutputContext(ctx, args...) @@ -197,12 +193,8 @@ func installBatchContext(ctx context.Context, toInstall []string) (failed []stri return installSequentialContext(ctx, toInstall) } -// installSequential installs each package individually, skipping those that +// installSequentialContext installs each package individually, skipping those that // were already picked up by a partial batch install. Returns failed package names. -func installSequential(toInstall []string) (failed []string, err error) { - return installSequentialContext(context.Background(), toInstall) -} - func installSequentialContext(ctx context.Context, toInstall []string) (failed []string, err error) { nowInstalled, err := GetInstalledPackagesContext(ctx) if err != nil {