From 6812f23cf41d21514368d2451a4c41332958510b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 6 Jul 2026 10:37:50 +0200 Subject: [PATCH 1/2] fix: close security-relevant gaps found in codebase audit Split out of #481 per review feedback, grouping the security-sensitive findings from issue #480 (a multi-agent codebase audit) separately from the UX/robustness ones: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant (e.g. `deny --path C:\Users\me\project\Secrets`) be silently bypassed by spelling a Windows path with different case - securefile/oauth: securefile.go's createSecretFile lacked the ERROR_ACCESS_DENIED handling oauth/encrypt.go's already had, so concurrent zero processes on Windows could spuriously hard-fail credstore's encrypted API-key storage with "Access is denied"; both also discarded the real lock-creation error in favor of a subsequent unrelated ErrNotExist, masking genuine ACL failures behind a misleading "timed out: file does not exist" - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted, so a checksum file naming a different (but validly-hashed) archive could vouch for the wrong bytes before extraction; extractZip didn't reject symlink-mode entries like extractTarGz already did All three fixes ship with regression tests. Refs #480. --- internal/oauth/encrypt.go | 12 ++++---- internal/sandbox/grant_scope.go | 21 ++++++++++---- internal/sandbox/grant_scope_test.go | 24 ++++++++++++++++ internal/securefile/securefile.go | 23 +++++++++------ internal/securefile/securefile_test.go | 27 +++++++++++++++++ internal/update/apply.go | 21 ++++++++++++-- internal/update/apply_test.go | 35 ++++++++++++++++++++++ internal/update/extract.go | 8 ++++++ internal/update/extract_test.go | 40 ++++++++++++++++++++++++++ 9 files changed, 190 insertions(+), 21 deletions(-) diff --git a/internal/oauth/encrypt.go b/internal/oauth/encrypt.go index d6a885c4c..ad1a6ddee 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 bf6441e89..747eb6f98 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 b3a607496..96896a91d 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 a3a81f743..c4edb7568 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 7c88d7207..f35da8302 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/update/apply.go b/internal/update/apply.go index 3467dd3db..29736de1a 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 4d3343196..379273498 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 c0d51563b..ce0a5acbf 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 c5f8761b4..f8aaa8239 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") From 49c7505da63d33a22b24569bb7d99fddf019c0d0 Mon Sep 17 00:00:00 2001 From: PierrunoYT <95778421+PierrunoYT@users.noreply.github.com> Date: Mon, 6 Jul 2026 11:21:29 +0200 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/update/apply.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/update/apply.go b/internal/update/apply.go index 29736de1a..1ac183293 100644 --- a/internal/update/apply.go +++ b/internal/update/apply.go @@ -212,7 +212,7 @@ func verifyArchiveChecksum(checksumPath string, expectedArchiveName string) erro 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 fmt.Errorf("checksum file %s references %q, expected %q", filepath.Base(checksumPath), verified.ArchiveName, expectedArchiveName) } return nil }