diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 71807176..0ba1340c 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -351,11 +351,12 @@ func oauthLoggedInProviders() map[string]bool { } // firstUsableProvider returns the saved provider best suited to run without -// onboarding: the first usable (inline credential present, or no-auth/local) -// non-local provider, else the first usable local one. It lets the CLI fall back -// to an already-configured login when the active provider happens to lack a -// credential, instead of re-running onboarding every launch. +// onboarding: the first usable (inline credential, stored OAuth login, or +// no-auth/local) non-local provider, else the first usable local one. It lets +// the CLI fall back to an already-configured login when the active provider +// happens to lack a credential, instead of re-running onboarding every launch. func firstUsableProvider(providers []config.ProviderProfile) (config.ProviderProfile, bool) { + logins := oauthLoggedInProviders() var localFallback config.ProviderProfile haveLocal := false for _, profile := range providers { @@ -371,7 +372,11 @@ func firstUsableProvider(providers []config.ProviderProfile) (config.ProviderPro continue } } - if _, missing := setupMissingCredentialEnv(profile); missing { + // A stored OAuth login (e.g. `zero auth login xai`) is a credential too, even + // when the profile has no inline key / env var — mirrors setupRequired and + // usableSavedProviders so this fallback doesn't force onboarding for a + // provider the user is already authenticated with. + if _, missing := setupMissingCredentialEnv(profile); missing && !providerHasOAuthLogin(profile, logins) { continue } if providerProfileIsLocal(profile) { diff --git a/internal/cli/setup_fallback_test.go b/internal/cli/setup_fallback_test.go index 7c87d138..692b9360 100644 --- a/internal/cli/setup_fallback_test.go +++ b/internal/cli/setup_fallback_test.go @@ -1,9 +1,12 @@ package cli import ( + "path/filepath" "testing" + "time" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/oauth" ) func TestFirstUsableProviderPrefersRemoteKeyed(t *testing.T) { @@ -66,6 +69,31 @@ func TestFirstUsableProviderSkipsUnresolvableCatalogWithoutBaseURL(t *testing.T) } } +// An OAuth-only provider (no inline key, no env var) must be selectable as a +// fallback, matching setupRequired/usableSavedProviders — otherwise a fully +// authenticated user gets forced back into onboarding when activeProvider +// goes stale. +func TestFirstUsableProviderRecognizesOAuthLogin(t *testing.T) { + path := filepath.Join(t.TempDir(), "tok.json") + t.Setenv("ZERO_OAUTH_STORAGE", "file") // an inherited "keyring" would ignore the temp path and hit the OS keychain + t.Setenv("ZERO_OAUTH_TOKENS_PATH", path) + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + if err := store.Save(oauth.ProviderKey("xai"), oauth.Token{AccessToken: "tok", ExpiresAt: time.Now().Add(time.Hour)}); err != nil { + t.Fatalf("seed token: %v", err) + } + + providers := []config.ProviderProfile{ + {Name: "xai", CatalogID: "xai", APIKeyEnv: "XAI_API_KEY"}, // no inline key/env, but logged in via OAuth + } + got, ok := firstUsableProvider(providers) + if !ok || got.Name != "xai" { + t.Fatalf("want OAuth-logged-in provider (xai), got %q ok=%v", got.Name, ok) + } +} + func TestProviderProfileIsLocal(t *testing.T) { cases := []struct { name string diff --git a/internal/oauth/encrypt.go b/internal/oauth/encrypt.go index d6a885c4..ad1a6dde 100644 --- a/internal/oauth/encrypt.go +++ b/internal/oauth/encrypt.go @@ -122,17 +122,17 @@ func createSecretFile(path string) ([]byte, error) { if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { return nil, fmt.Errorf("oauth: create token secret lock: %w", err) } + // Remember the lock-creation error itself rather than a subsequent + // "secret file doesn't exist yet" read error -- otherwise a real + // contention/ACL problem is masked by the expected-while-waiting + // ErrNotExist once the retries are exhausted. + lastErr = err if data, rerr := readSecretFileRetry(path); rerr == nil { return data, nil - } else { - lastErr = rerr } time.Sleep(secretRetryDelay) } - if lastErr != nil { - return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr) - } - return nil, fmt.Errorf("oauth: timed out waiting for token secret lock %s", lockPath) + return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr) } func writeNewSecretFile(path string) ([]byte, error) { diff --git a/internal/sandbox/grant_scope.go b/internal/sandbox/grant_scope.go index bf6441e8..747eb6f9 100644 --- a/internal/sandbox/grant_scope.go +++ b/internal/sandbox/grant_scope.go @@ -200,20 +200,23 @@ func normalizeHostScope(raw string) string { // directory itself or any descendant; a host grant matches its exact normalized // host. A narrower grant never covers a tool-wide request (reqScope == ""), so // such a request re-prompts (fail-safe). +// +// File and dir comparisons go through filepath.Rel (via scopePathEqual and the +// shared pathWithinRoot helper) rather than a plain string comparison, so they +// apply the same case-folding as the workspace-boundary check on Windows -- +// otherwise a persisted grant (including a deny) could be silently bypassed by +// spelling the same path with different case. func grantCovers(grant Grant, reqScope string) bool { switch grant.ScopeKind { case ScopeToolWide: return true case ScopeFile: - return reqScope != "" && reqScope == grant.Scope + return reqScope != "" && scopePathEqual(grant.Scope, reqScope) case ScopeDir: if reqScope == "" || grant.Scope == "" { return false } - if reqScope == grant.Scope { - return true - } - return strings.HasPrefix(reqScope, grant.Scope+string(filepath.Separator)) + return pathWithinRoot(grant.Scope, reqScope) case ScopeHost: return reqScope != "" && normalizeHostScope(reqScope) == normalizeHostScope(grant.Scope) default: @@ -221,6 +224,14 @@ func grantCovers(grant Grant, reqScope string) bool { } } +// scopePathEqual reports whether two absolute, cleaned scope paths refer to +// the same file, applying the same case-folding filepath.Rel already uses for +// directory-boundary checks (case-insensitive path components on Windows). +func scopePathEqual(a, b string) bool { + rel, err := filepath.Rel(a, b) + return err == nil && rel == "." +} + // scopeSpecificity ranks scope kinds so the most precise covering allow wins when // several grants match the same request. func scopeSpecificity(kind ScopeKind) int { diff --git a/internal/sandbox/grant_scope_test.go b/internal/sandbox/grant_scope_test.go index b3a60749..96896a91 100644 --- a/internal/sandbox/grant_scope_test.go +++ b/internal/sandbox/grant_scope_test.go @@ -2,6 +2,8 @@ package sandbox import ( "path/filepath" + "runtime" + "strings" "testing" ) @@ -234,3 +236,25 @@ func TestGrantCovers(t *testing.T) { }) } } + +// TestGrantCoversCaseInsensitiveOnWindows guards against a persisted grant +// (including a deny) being silently bypassed by spelling the same path with +// different case, since Windows path components are case-insensitive. +func TestGrantCoversCaseInsensitiveOnWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("path case-insensitivity is a Windows-specific filesystem property") + } + dir := filepath.Join(string(filepath.Separator)+"proj", "src") + file := filepath.Join(dir, "main.go") + descendant := filepath.Join(dir, "api", "z.go") + + fileGrant := Grant{Scope: file, ScopeKind: ScopeFile} + dirGrant := Grant{Scope: dir, ScopeKind: ScopeDir} + + if !grantCovers(fileGrant, strings.ToUpper(file)) { + t.Fatalf("file grant %q should cover differently-cased request %q on Windows", file, strings.ToUpper(file)) + } + if !grantCovers(dirGrant, strings.ToUpper(descendant)) { + t.Fatalf("dir grant %q should cover differently-cased descendant %q on Windows", dir, strings.ToUpper(descendant)) + } +} diff --git a/internal/securefile/securefile.go b/internal/securefile/securefile.go index a3a81f74..c4edb756 100644 --- a/internal/securefile/securefile.go +++ b/internal/securefile/securefile.go @@ -26,6 +26,8 @@ const ( secretRetryDelay = 2 * time.Millisecond ) +var openSecretLockFile = os.OpenFile + // Crypter encrypts a blob at rest with AES-256-GCM under a per-user random secret // persisted (0600) at secretPath. type Crypter struct { @@ -107,7 +109,7 @@ func createSecretFile(path string) ([]byte, error) { lockPath := path + ".lock" var lastErr error for attempt := 0; attempt < secretRetryAttempts; attempt++ { - lock, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + lock, err := openSecretLockFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err == nil { _ = lock.Close() defer os.Remove(lockPath) @@ -118,20 +120,25 @@ func createSecretFile(path string) ([]byte, error) { } return writeNewSecretFile(path) } - if !errors.Is(err, os.ErrExist) { + // On Windows a concurrent holder's os.Remove leaves the lock file in a + // "delete pending" state, so an O_EXCL create races it with + // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that + // as contention and retry too -- mirroring oauth's createSecretFile -- + // otherwise concurrent secret creation spuriously fails on Windows. + if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { return nil, fmt.Errorf("securefile: create secret lock: %w", err) } + // Remember the lock-creation error itself rather than a subsequent + // "secret file doesn't exist yet" read error -- otherwise a real + // contention/ACL problem is masked by the expected-while-waiting + // ErrNotExist once the retries are exhausted. + lastErr = err if data, rerr := readSecretFileRetry(path); rerr == nil { return data, nil - } else { - lastErr = rerr } time.Sleep(secretRetryDelay) } - if lastErr != nil { - return nil, fmt.Errorf("securefile: timed out waiting for secret %s: %w", path, lastErr) - } - return nil, fmt.Errorf("securefile: timed out waiting for secret lock %s", lockPath) + return nil, fmt.Errorf("securefile: timed out waiting for secret %s: %w", path, lastErr) } func writeNewSecretFile(path string) ([]byte, error) { diff --git a/internal/securefile/securefile_test.go b/internal/securefile/securefile_test.go index 7c88d720..f35da830 100644 --- a/internal/securefile/securefile_test.go +++ b/internal/securefile/securefile_test.go @@ -7,6 +7,33 @@ import ( "testing" ) +func TestCreateSecretFileRetriesOnPermissionContention(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + attempts := 0 + origOpen := openSecretLockFile + openSecretLockFile = func(path string, flag int, perm os.FileMode) (*os.File, error) { + attempts++ + if attempts == 1 { + return nil, os.ErrPermission + } + return os.OpenFile(path, flag, perm) + } + t.Cleanup(func() { + openSecretLockFile = origOpen + }) + + secret, err := createSecretFile(secretPath) + if err != nil { + t.Fatalf("createSecretFile should retry on permission contention: %v", err) + } + if len(secret) != secretBytes { + t.Fatalf("secret length = %d, want %d", len(secret), secretBytes) + } + if attempts < 2 { + t.Fatalf("expected at least 2 lock attempts, got %d", attempts) + } +} + func TestSealOpenRoundTrip(t *testing.T) { secret := filepath.Join(t.TempDir(), "k.secret") c := NewCrypter(secret) diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index d21cd662..44b3d6c1 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -5,6 +5,7 @@ import ( "fmt" "log" "strings" + "unicode/utf8" ) type ExecMode string @@ -221,11 +222,24 @@ func summarizePayload(payload any) string { text = string(data) } if len(text) > 500 { - return text[:500] + return truncateUTF8(text, 500) } return text } +// truncateUTF8 returns the longest prefix of s that is at most n bytes, +// backing off to the nearest rune boundary so a multi-byte character isn't +// split — a split rune here would embed invalid UTF-8 into the exec prompt. +func truncateUTF8(s string, n int) string { + if len(s) <= n { + return s + } + for n > 0 && !utf8.RuneStart(s[n]) { + n-- + } + return s[:n] +} + func extractText(value any) string { switch typed := value.(type) { case string: diff --git a/internal/sessions/store_test.go b/internal/sessions/store_test.go index 4786555b..acd4dd9c 100644 --- a/internal/sessions/store_test.go +++ b/internal/sessions/store_test.go @@ -11,6 +11,7 @@ import ( "sync" "testing" "time" + "unicode/utf8" ) func TestStoreCreatesAppendsListsAndReadsEvents(t *testing.T) { @@ -640,6 +641,23 @@ func TestFormatExecPromptTruncatesConversationMessagesAfterFilteringNoise(t *tes } } +// A truncation cut at a raw byte offset can land in the middle of a +// multi-byte UTF-8 rune (e.g. CJK text), embedding invalid UTF-8 into the +// exec prompt. summarizePayload must back off to a rune boundary instead. +func TestSummarizePayloadTruncatesOnRuneBoundary(t *testing.T) { + content := strings.Repeat("中文", 300) // 900 bytes of 3-byte runes + payload := json.RawMessage(fmt.Sprintf(`{"role":"user","content":%q}`, content)) + + got := summarizePayload(payload) + + if !utf8.ValidString(got) { + t.Fatalf("summarizePayload produced invalid UTF-8: %q", got) + } + if len(got) > 500 { + t.Fatalf("expected summary to be at most 500 bytes, got %d", len(got)) + } +} + func TestPrepareExecPersistsSpecialistMetadataForNewSession(t *testing.T) { store := NewStore(StoreOptions{RootDir: t.TempDir(), Now: fixedClock("2026-06-04T14:30:00Z")}) diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index 11073d5d..6ea7ba03 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -190,6 +190,33 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { } } +// Operator/utility-name text that only appears inside a quoted argument must +// not be mistaken for real shell syntax — e.g. `echo "foo; ls -la"` is a +// harmless echo, not a bash-style `ls` invocation, and quoted `head`/`grep` +// text isn't a piped POSIX utility. +func TestDetectShellCommandIssueIgnoresQuotedContent(t *testing.T) { + for _, command := range []string{ + `echo "foo; ls -la"`, + `echo "run head over the file"`, + `echo 'ls -la is a common bash command'`, + `echo "grep for the pattern"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected quoted content to be ignored for %q, got %#v", command, issue) + } + } + + // Sanity: the same utility names still get flagged when they're NOT quoted. + for _, command := range []string{ + `echo "foo" ; ls -la`, + `git log | head`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue == nil { + t.Fatalf("expected unquoted shell syntax to still be flagged for %q", command) + } + } +} + func TestDetectShellOutputIssueAddsWindowsSyntaxHint(t *testing.T) { issue := detectShellOutputIssue(`cd /d/tmp/zero-pr-158 && ls -la`, "The syntax of the command is incorrect.", "windows") if issue == nil { diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 9d5a0198..23c4a95b 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -54,15 +54,20 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } trimmed := strings.TrimSpace(command) - if windowsBashStyleCDPattern.MatchString(trimmed) || - windowsLSCommandPattern.MatchString(trimmed) { + // Match against a quote-masked copy so operator/utility-name text that only + // appears inside a quoted argument (e.g. `echo "foo; ls -la"`) isn't + // mistaken for real shell syntax and doesn't block an otherwise-harmless + // command. + masked := stripQuotedSpans(trimmed) + if windowsBashStyleCDPattern.MatchString(masked) || + windowsLSCommandPattern.MatchString(masked) { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Command looks like POSIX/Bash syntax, but Zero runs bash tool commands through Windows cmd.exe on this host.", Suggestion: "Use the cwd argument instead of cd, use Windows cmd.exe syntax, or use native tools such as list_directory, read_file, grep, and glob.", } } - if windowsPosixUtilityPattern.MatchString(trimmed) { + if windowsPosixUtilityPattern.MatchString(masked) { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Command uses a POSIX utility (head/tail/grep/wc/awk/sed/cut/xargs/tr) that Windows cmd.exe does not have.", @@ -72,6 +77,32 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } +// stripQuotedSpans blanks the interior of single- and double-quoted spans — +// replacing each byte with a space, preserving length and any operators or +// utility names outside quotes — so quoted argument text is never mistaken +// for shell syntax by the regexes above. +func stripQuotedSpans(s string) string { + var b strings.Builder + b.Grow(len(s)) + var quote byte + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case quote != 0: + b.WriteByte(' ') + if c == quote { + quote = 0 + } + case c == '\'' || c == '"': + quote = c + b.WriteByte(' ') + default: + b.WriteByte(c) + } + } + return b.String() +} + func detectShellOutputIssue(command string, output string, goos string) *shellIssue { if goos != "windows" { return nil diff --git a/internal/tui/binding_test.go b/internal/tui/binding_test.go index 52f1a039..2a8df447 100644 --- a/internal/tui/binding_test.go +++ b/internal/tui/binding_test.go @@ -289,3 +289,98 @@ func TestDispatchOptionO(t *testing.T) { t.Errorf("model.keyMatch should NOT match ctrl+o when option+o is configured") } } + +// A configured binding equal to a reserved hardcoded chord must be reverted +// to default rather than silently making the hardcoded action unreachable +// (e.g. configuring toggleDetailed to ctrl+f would otherwise swallow the +// /model picker's "favorite" shortcut). +func TestSanitizeKeyBindingsDropsReservedCollision(t *testing.T) { + cfg := config.KeyBindingsConfig{ + ToggleDetailed: "ctrl+f", // collides with the hardcoded favorite-model shortcut + ToggleMouse: "ctrl+e", // unaffected, should survive untouched + } + sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg)) + + if !sanitized.toggleDetailed.isZero() { + t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label()) + } + if sanitized.toggleMouse.isZero() || sanitized.toggleMouse.Label() != "Ctrl+E" { + t.Errorf("toggleMouse should be untouched, got %q", sanitized.toggleMouse.Label()) + } + if len(warnings) != 1 { + t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings) + } +} + +// Two configurable actions bound to the same chord must not both fire — +// sanitizeKeyBindings should keep the first and revert the rest to default. +func TestSanitizeKeyBindingsDropsMutualCollision(t *testing.T) { + cfg := config.KeyBindingsConfig{ + ToggleDetailed: "ctrl+o", + CycleReasoning: "ctrl+o", // collides with toggleDetailed above + } + sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg)) + + if sanitized.toggleDetailed.isZero() || sanitized.toggleDetailed.Label() != "Ctrl+O" { + t.Errorf("toggleDetailed (first claimant) should keep ctrl+o, got %q", sanitized.toggleDetailed.Label()) + } + if !sanitized.cycleReasoning.isZero() { + t.Errorf("cycleReasoning (second claimant) should be reverted to default, got %q", sanitized.cycleReasoning.Label()) + } + if len(warnings) != 1 { + t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings) + } +} + +// Non-colliding configured bindings must pass through unchanged with no +// warnings. +func TestSanitizeKeyBindingsNoCollisions(t *testing.T) { + cfg := config.KeyBindingsConfig{ + ToggleDetailed: "option+o", + ToggleSidebar: "option+b", + } + sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg)) + + if len(warnings) != 0 { + t.Fatalf("want no warnings, got %v", warnings) + } + if sanitized.toggleDetailed.Label() != "Alt+O" || sanitized.toggleSidebar.Label() != "Alt+B" { + t.Fatalf("bindings should be unchanged: toggleDetailed=%q toggleSidebar=%q", + sanitized.toggleDetailed.Label(), sanitized.toggleSidebar.Label()) + } +} + +// A configured binding must not shadow another action that is still on its +// built-in default chord. +func TestSanitizeKeyBindingsDropsCollisionWithOtherDefault(t *testing.T) { + cfg := config.KeyBindingsConfig{ + ToggleDetailed: "ctrl+t", // collides with cycleReasoning default + } + sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg)) + + if !sanitized.toggleDetailed.isZero() { + t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label()) + } + if !sanitized.cycleReasoning.isZero() { + t.Errorf("cycleReasoning should remain default (zero), got %q", sanitized.cycleReasoning.Label()) + } + if len(warnings) != 1 { + t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings) + } +} + +// Bare hardcoded keys handled directly in updateModel (e.g. Tab navigation) +// are reserved too and must not be shadowed by configurable bindings. +func TestSanitizeKeyBindingsDropsBareHardcodedCollision(t *testing.T) { + cfg := config.KeyBindingsConfig{ + ToggleDetailed: "tab", + } + sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg)) + + if !sanitized.toggleDetailed.isZero() { + t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label()) + } + if len(warnings) != 1 { + t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings) + } +} diff --git a/internal/tui/keybindings.go b/internal/tui/keybindings.go index 42f2b9ac..35364a58 100644 --- a/internal/tui/keybindings.go +++ b/internal/tui/keybindings.go @@ -1,6 +1,7 @@ package tui import ( + "fmt" "strings" "unicode/utf8" @@ -263,3 +264,96 @@ func (m model) keyMatch(b parsedBinding, msg tea.KeyMsg, defaultFn func(tea.KeyM } return defaultFn(msg) } + +// reservedBindings lists hardcoded (non-configurable) chords handled directly in +// model.go's key dispatch. If a configurable binding uses one of these chords, +// one of the actions becomes unreachable (depending on switch order), so +// sanitizeKeyBindings reverts the configurable binding back to its default. +var reservedBindings = []struct { + binding parsedBinding + description string +}{ + {parseBinding("ctrl+c"), "cancel / exit"}, + {parseBinding("esc"), "cancel / close"}, + {parseBinding("enter"), "submit"}, + {parseBinding("shift+tab"), "cycle permission mode"}, + {parseBinding("tab"), "navigation / completion"}, + {parseBinding("backspace"), "composer edit / attachment removal"}, + {parseBinding("up"), "history/navigation"}, + {parseBinding("down"), "history/navigation"}, + {parseBinding("pgup"), "transcript scroll"}, + {parseBinding("pgdown"), "transcript scroll"}, + {parseBinding("ctrl+f"), "favorite model (in the /model picker)"}, + {parseBinding("?"), "help overlay"}, +} + +// sanitizeKeyBindings drops (reverts to default) any configured binding that +// collides with a reserved hardcoded chord above, or with another +// configured binding, since either collision would silently make one of the +// two actions permanently unreachable. Returns the sanitized bindings plus a +// human-readable warning for each dropped binding, for the caller to surface +// as a startup notice. +func sanitizeKeyBindings(b keyBindings) (keyBindings, []string) { + entries := []struct { + name string + binding *parsedBinding + defaultBinding parsedBinding + }{ + {"toggleDetailed", &b.toggleDetailed, parseBinding("ctrl+o")}, + {"toggleMouse", &b.toggleMouse, parseBinding("ctrl+e")}, + {"cycleReasoning", &b.cycleReasoning, parseBinding("ctrl+t")}, + {"togglePlan", &b.togglePlan, parseBinding("ctrl+p")}, + {"toggleSidebar", &b.toggleSidebar, parseBinding("ctrl+b")}, + } + + var warnings []string + for _, e := range entries { + if e.binding.isZero() { + continue + } + for _, other := range entries { + if other.name == e.name || !other.binding.isZero() { + continue + } + if *e.binding == other.defaultBinding { + warnings = append(warnings, fmt.Sprintf( + "keybindings.%s (%s) conflicts with keybindings.%s default (%s); using the default instead.", + e.name, e.binding.Label(), other.name, other.defaultBinding.Label())) + *e.binding = parsedBinding{} + break + } + } + } + + for _, e := range entries { + if e.binding.isZero() { + continue + } + for _, reserved := range reservedBindings { + if *e.binding == reserved.binding { + warnings = append(warnings, fmt.Sprintf( + "keybindings.%s (%s) conflicts with the built-in %s shortcut; using the default instead.", + e.name, e.binding.Label(), reserved.description)) + *e.binding = parsedBinding{} + break + } + } + } + + claimedBy := map[parsedBinding]string{} + for _, e := range entries { + if e.binding.isZero() { + continue + } + if other, ok := claimedBy[*e.binding]; ok { + warnings = append(warnings, fmt.Sprintf( + "keybindings.%s (%s) conflicts with keybindings.%s; using the default instead.", + e.name, e.binding.Label(), other)) + *e.binding = parsedBinding{} + continue + } + claimedBy[*e.binding] = e.name + } + + return b, warnings +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 766f9ab9..308bfce8 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -692,6 +692,8 @@ func newModel(ctx context.Context, options Options) model { notify.MaybeAddWebhookSink(notifier, os.Getenv, nil) notifier.SetFocused(true) + resolvedKeyBindings, keyBindingWarnings := sanitizeKeyBindings(resolveKeyBindings(options.KeyBindings)) + m := model{ ctx: ctx, cwd: cwd, @@ -727,7 +729,7 @@ func newModel(ctx context.Context, options Options) model { permissionMode: permissionMode, reasoningEffort: options.ReasoningEffort, responseStyle: defaultedResponseStyle(options.ResponseStyle), - keyBindings: resolveKeyBindings(options.KeyBindings), + keyBindings: resolvedKeyBindings, themeMode: resolveThemeMode(options.Theme, os.Getenv("ZERO_THEME"), options.SavedTheme), hasDarkBg: true, userAgent: options.UserAgent, @@ -758,6 +760,9 @@ func newModel(ctx context.Context, options Options) model { // styleStreamingLine), so no accent glow and no per-line fade ticks. m.fadeDisabled = true m.refreshMCPViewState() + for _, warning := range keyBindingWarnings { + m = m.appendSystemNotice(warning) + } return m } diff --git a/internal/update/apply.go b/internal/update/apply.go index 3467dd3d..29736de1 100644 --- a/internal/update/apply.go +++ b/internal/update/apply.go @@ -148,8 +148,8 @@ func applyStandaloneUpdate(ctx context.Context, result Result, executablePath st if err := downloadFile(downloadCtx, asset.ChecksumURL, checksumPath); err != nil { return nil, fmt.Errorf("download release checksum: %w", err) } - if _, err := release.VerifySHA256Checksum(checksumPath); err != nil { - return nil, fmt.Errorf("verify release checksum: %w", err) + if err := verifyArchiveChecksum(checksumPath, asset.ArchiveName); err != nil { + return nil, err } extractDir := filepath.Join(tempDir, "extracted") @@ -200,6 +200,23 @@ func applyStandaloneUpdate(ctx context.Context, result Result, executablePath st return warnings, nil } +// verifyArchiveChecksum verifies the checksum file at checksumPath and +// cross-checks it names expectedArchiveName. VerifySHA256Checksum hashes +// whichever file the checksum text names, not necessarily the archive we +// downloaded — this mirrors the cross-check VerifyReleaseChecksums already +// does, so a checksum file that names a different file can't be used to +// verify (and thereby vouch for) the wrong bytes before extraction. +func verifyArchiveChecksum(checksumPath string, expectedArchiveName string) error { + verified, err := release.VerifySHA256Checksum(checksumPath) + if err != nil { + return fmt.Errorf("verify release checksum: %w", err) + } + if verified.ArchiveName != expectedArchiveName { + return fmt.Errorf("checksum file references %s, expected %s", verified.ArchiveName, expectedArchiveName) + } + return nil +} + // installBinary stages sourcePath next to targetPath (same directory, so the // final rename is atomic/same-filesystem) and then swaps it into place. func installBinary(sourcePath string, targetPath string) error { diff --git a/internal/update/apply_test.go b/internal/update/apply_test.go index 4d334319..37927349 100644 --- a/internal/update/apply_test.go +++ b/internal/update/apply_test.go @@ -300,3 +300,38 @@ func TestApplyStandaloneUpdateRejectsChecksumMismatch(t *testing.T) { t.Fatalf("executable should be untouched after checksum failure, got %q", data) } } + +// VerifySHA256Checksum hashes whichever file the checksum text names, which +// isn't necessarily the archive the caller asked to verify. A checksum file +// that correctly verifies against a DIFFERENT (but real, correctly-hashed) +// file must still be rejected, not treated as vouching for the requested +// archive. +func TestVerifyArchiveChecksumRejectsFilenameMismatch(t *testing.T) { + dir := t.TempDir() + + decoyName := "decoy.tar.gz" + decoyPath := filepath.Join(dir, decoyName) + if err := os.WriteFile(decoyPath, []byte("decoy contents"), 0o644); err != nil { + t.Fatalf("WriteFile decoy: %v", err) + } + decoyChecksum, err := release.SHA256File(decoyPath) + if err != nil { + t.Fatalf("SHA256File: %v", err) + } + checksumText, err := release.FormatSHA256Checksum(decoyChecksum, decoyName) + if err != nil { + t.Fatalf("FormatSHA256Checksum: %v", err) + } + checksumPath := filepath.Join(dir, "real.tar.gz.sha256") + if err := os.WriteFile(checksumPath, []byte(checksumText), 0o644); err != nil { + t.Fatalf("WriteFile checksum: %v", err) + } + + err = verifyArchiveChecksum(checksumPath, "real.tar.gz") + if err == nil { + t.Fatal("expected error when checksum file references a different archive") + } + if !strings.Contains(err.Error(), decoyName) || !strings.Contains(err.Error(), "real.tar.gz") { + t.Fatalf("expected error to name both the referenced file and the expected one, got %q", err) + } +} diff --git a/internal/update/extract.go b/internal/update/extract.go index c0d51563..ce0a5acb 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -87,6 +87,14 @@ func extractZip(archivePath string, destDir string) error { } continue } + // Release archives only ever contain regular files and directories; + // reject anything else (symlinks, devices) rather than silently write + // the link-target string (or other special content) out as an + // ordinary file — mirrors extractTarGz's rejection of non-regular tar + // entries. + if !entry.Mode().IsRegular() { + return fmt.Errorf("unsupported archive entry type for %s", entry.Name) + } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return err } diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index c5f8761b..f8aaa823 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -148,6 +148,46 @@ func TestExtractZipRejectsPathTraversal(t *testing.T) { } } +// A zip entry with the Unix symlink mode bit set must be rejected, not +// silently written out as an ordinary file containing the link-target +// string — mirroring extractTarGz's rejection of non-regular tar entries. +func TestExtractZipRejectsSymlinkEntry(t *testing.T) { + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.zip") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create archive: %v", err) + } + zipWriter := zip.NewWriter(file) + header := &zip.FileHeader{Name: "zero"} + header.SetMode(os.ModeSymlink | 0o777) + writer, err := zipWriter.CreateHeader(header) + if err != nil { + t.Fatalf("CreateHeader: %v", err) + } + if _, err := writer.Write([]byte("/some/other/path")); err != nil { + t.Fatalf("Write symlink target: %v", err) + } + if err := zipWriter.Close(); err != nil { + t.Fatalf("close zip writer: %v", err) + } + if err := file.Close(); err != nil { + t.Fatalf("close archive file: %v", err) + } + + destDir := filepath.Join(dir, "extracted") + if err := os.MkdirAll(destDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := extractArchive(archivePath, destDir); err == nil { + t.Fatal("expected extractArchive to reject a symlink entry") + } + if _, err := os.Lstat(filepath.Join(destDir, "zero")); err == nil { + t.Fatal("symlink entry should not have been written to the destination") + } +} + func TestFindByBasenameSearchesRecursively(t *testing.T) { dir := t.TempDir() nested := filepath.Join(dir, "helpers") diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index 073cbdcb..d15c29f1 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -5,6 +5,12 @@ package update import ( "fmt" "os" + "time" +) + +const ( + restoreRenameRetryAttempts = 10 + restoreRenameRetryDelay = 100 * time.Millisecond ) // replaceBinary installs newPath over targetPath. Windows will not let a @@ -18,7 +24,11 @@ func replaceBinary(targetPath string, newPath string) error { return fmt.Errorf("rename running binary aside: %w", err) } if err := os.Rename(newPath, targetPath); err != nil { - if restoreErr := os.Rename(oldPath, targetPath); restoreErr != nil { + // Retry the restore: a transient Windows file lock (antivirus/indexer + // scanning the just-renamed file, a lingering handle) can make a rename + // fail momentarily, and here failure means targetPath is left missing + // entirely rather than merely stale — worth a short retry to avoid that. + if restoreErr := renameWithRetry(oldPath, targetPath); restoreErr != nil { return fmt.Errorf("install new binary: %w; additionally failed to restore the original binary: %v (original preserved at %s)", err, restoreErr, oldPath) } return fmt.Errorf("install new binary: %w", err) @@ -26,6 +36,19 @@ func replaceBinary(targetPath string, newPath string) error { return nil } +func renameWithRetry(oldPath string, newPath string) error { + var lastErr error + for attempt := 0; attempt < restoreRenameRetryAttempts; attempt++ { + if err := os.Rename(oldPath, newPath); err == nil { + return nil + } else { + lastErr = err + } + time.Sleep(restoreRenameRetryDelay) + } + return lastErr +} + // CleanupStaleBinary best-effort removes a ".old" file left behind by a // previous replaceBinary call once the old process holding it has exited. // Callers should invoke this once at startup for the current executable. diff --git a/internal/update/replace_windows_test.go b/internal/update/replace_windows_test.go new file mode 100644 index 00000000..5649e3dc --- /dev/null +++ b/internal/update/replace_windows_test.go @@ -0,0 +1,65 @@ +//go:build windows + +package update + +import ( + "os" + "path/filepath" + "testing" +) + +func TestReplaceBinaryReplacesRunningBinary(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + newPath := filepath.Join(dir, "zero.exe.new") + + if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + if err := os.WriteFile(newPath, []byte("new-binary"), 0o755); err != nil { + t.Fatalf("WriteFile new: %v", err) + } + + if err := replaceBinary(targetPath, newPath); err != nil { + t.Fatalf("replaceBinary: %v", err) + } + + data, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile target: %v", err) + } + if string(data) != "new-binary" { + t.Fatalf("target content = %q, want %q", data, "new-binary") + } + if _, err := os.Stat(targetPath + ".old"); err != nil { + t.Fatalf("expected the original binary to be preserved at %s.old: %v", targetPath, err) + } +} + +func TestRenameWithRetrySucceedsImmediately(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + dst := filepath.Join(dir, "dst") + if err := os.WriteFile(src, []byte("data"), 0o644); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + + if err := renameWithRetry(src, dst); err != nil { + t.Fatalf("renameWithRetry: %v", err) + } + if _, err := os.Stat(dst); err != nil { + t.Fatalf("expected dst to exist after rename: %v", err) + } +} + +// A permanently-failing rename (source never appears) must exhaust its +// retries and surface the underlying error, rather than retrying forever. +func TestRenameWithRetryFailsAfterExhaustingAttempts(t *testing.T) { + dir := t.TempDir() + missing := filepath.Join(dir, "does-not-exist") + dst := filepath.Join(dir, "dst") + + if err := renameWithRetry(missing, dst); err == nil { + t.Fatal("expected renameWithRetry to fail for a source that never appears") + } +}