Harden HTTP, dry-run, and command guardrails

internal/archtest/baseline/dryrun.txt

@@ -1,28 +1,33 @@
 # Baseline for archtest rule "dryrun".
-# Each line is <file>:<line> of a known existing violation.
+# Each line is <file>:<line> # 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

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

internal/archtest/baseline/no-raw-http.txt

@@ -1,16 +1,5 @@
 # Baseline for archtest rule "no-raw-http".
-# Each line is <file>:<line> of a known existing violation.
+# Each line is <file>:<line> # 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

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
+}

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
 	}

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")

internal/cli/snapshot_import.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"net/http"
@@ -15,7 +16,7 @@ import (
 	"github.com/openbootdotdev/openboot/internal/ui/tui"
 )
 
-func runSnapshotImport(importPath string, dryRun bool) error {
+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 +65,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) {

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)
 	}

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)
 	}

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

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))