Skip to content

Security-sensitive fixes from the codebase audit (#480): sandbox path-casing, credential lock handling, update checksum/symlink #545

Description

@PierrunoYT

Summary

Split out of #480 / #481 per review feedback on #481: grouping the security-sensitive findings from the multi-agent codebase audit separately from the UX/robustness ones so each can be reviewed and landed independently.

This covers 3 of the 9 original findings, plus the securefile/oauth Windows lock-contention pair (not explicitly named in the split request, but grouped here since securefile.go backs credstore's encrypted API-key storage and the fix mirrors an existing oauth/encrypt.go fix one-for-one):

  1. Sandbox deny-grants are case-sensitive on Windowsinternal/sandbox/grant_scope.go. Persistent per-path grants (ScopeFile/ScopeDir) compared paths with plain ==/HasPrefix, unlike the workspace-boundary check which uses case-insensitive filepath.Rel. A user-authored deny --path C:\Users\me\project\Secrets silently failed to match a request spelled c:\users\me\project\secrets — same file on a case-insensitive filesystem, but the deny didn't apply.
  2. Windows lock-contention fix applied inconsistentlyinternal/securefile/securefile.go had the pre-fix(oauth): retry secret-lock on Windows ERROR_ACCESS_DENIED (delete-pending race) #445 bug that internal/oauth/encrypt.go already fixed: it only treated os.ErrExist as lock contention, not ERROR_ACCESS_DENIED. Since securefile.go backs credstore's encrypted API-key storage, concurrent zero processes on Windows could spuriously hard-fail with "Access is denied". Both files 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".
  3. Update checksum didn't cross-check the archive filenameinternal/update/apply.go. VerifySHA256Checksum hashes whichever file the checksum text names, not necessarily the archive that was downloaded; a checksum file naming a different (but validly-hashed) file could vouch for the wrong bytes before extraction.
  4. extractZip didn't reject symlink-mode entriesinternal/update/extract.go, unlike extractTarGz which already rejects non-regular tar entries.

Fix

PR: (to be linked)

All four fixes ship with regression tests. go build ./... and go test ./internal/sandbox/... ./internal/securefile/... ./internal/oauth/... ./internal/update/... pass.

Scope note

Originally reported and fixed together in #481, which is being split into smaller, independently-reviewable PRs per maintainer request.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions