fix: close security-relevant gaps found in codebase audit

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

internal/sandbox/grant_scope.go

@@ -200,27 +200,38 @@ 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:
 		return false
 	}
 }
 
+// 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 {

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

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

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)

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 %s references %q, expected %q", filepath.Base(checksumPath), 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 {

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

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
 		}

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