From e49b1cc76fe67c2b48e425420cd0bd66c5d7f92a Mon Sep 17 00:00:00 2001 From: Luis Pater Date: Mon, 27 Jul 2026 07:05:19 +0800 Subject: [PATCH] feat(store): add guard for watcher-originated auth file deletions - Implemented `guardWatcherAuthRemovalLocked` to prevent unauthorized watcher-triggered deletions of tracked auth files. - Updated `PersistAuthFiles` to handle redundant watcher events safely after intentional deletions. - Improved error handling for git tree and repository inspections during watcher removal validation. - Enhanced `commitAndPushLocked` logic to perform additional checks before committing changes. - Added comprehensive tests to validate watcher-originated deletion safeguards and correct behavior for explicit deletions. Closes: #4438 --- internal/store/gitstore.go | 105 +++++++++++++++++--- internal/store/gitstore_test.go | 166 ++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+), 16 deletions(-) diff --git a/internal/store/gitstore.go b/internal/store/gitstore.go index 4da6e5cb..14a3dfb6 100644 --- a/internal/store/gitstore.go +++ b/internal/store/gitstore.go @@ -16,6 +16,7 @@ import ( "github.com/go-git/go-git/v6/config" "github.com/go-git/go-git/v6/plumbing" "github.com/go-git/go-git/v6/plumbing/client" + gitindex "github.com/go-git/go-git/v6/plumbing/format/index" "github.com/go-git/go-git/v6/plumbing/object" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/plumbing/transport/http" @@ -406,15 +407,13 @@ func (s *GitTokenStore) Delete(_ context.Context, id string) error { if err = os.Remove(path); err != nil && !os.IsNotExist(err) { return fmt.Errorf("auth filestore: delete failed: %w", err) } - if err == nil { - rel, errRel := s.relativeToRepo(path) - if errRel != nil { - return errRel - } - messageID := id - if errCommit := s.commitAndPushLocked(fmt.Sprintf("Delete auth %s", messageID), rel); errCommit != nil { - return errCommit - } + rel, errRel := s.relativeToRepo(path) + if errRel != nil { + return errRel + } + messageID := id + if errCommit := s.commitAndPushLocked(fmt.Sprintf("Delete auth %s", messageID), rel); errCommit != nil { + return errCommit } return nil } @@ -451,9 +450,65 @@ func (s *GitTokenStore) PersistAuthFiles(_ context.Context, message string, path if strings.TrimSpace(message) == "" { message = "Sync watcher updates" } + if handled, errGuard := s.guardWatcherAuthRemovalLocked(message, filtered); handled || errGuard != nil { + return errGuard + } return s.commitAndPushLocked(message, filtered...) } +func (s *GitTokenStore) guardWatcherAuthRemovalLocked(message string, relPaths []string) (bool, error) { + if !strings.HasPrefix(strings.TrimSpace(message), "Remove auth ") { + return false, nil + } + repoDir := s.repoDirSnapshot() + if repoDir == "" { + return true, fmt.Errorf("git token store: repository path not configured") + } + repo, errOpen := git.PlainOpen(repoDir) + if errOpen != nil { + return true, fmt.Errorf("git token store: open repo for watcher removal guard: %w", errOpen) + } + head, errHead := repo.Head() + if errHead != nil { + if errors.Is(errHead, plumbing.ErrReferenceNotFound) { + return true, nil + } + return true, fmt.Errorf("git token store: inspect head for watcher removal guard: %w", errHead) + } + commit, errCommit := repo.CommitObject(head.Hash()) + if errCommit != nil { + return true, fmt.Errorf("git token store: inspect commit for watcher removal guard: %w", errCommit) + } + tree, errTree := commit.Tree() + if errTree != nil { + return true, fmt.Errorf("git token store: inspect tree for watcher removal guard: %w", errTree) + } + + hasExistingPath := false + for _, rel := range relPaths { + cleanRel := filepath.ToSlash(filepath.Clean(rel)) + worktreePath := filepath.Join(repoDir, filepath.FromSlash(cleanRel)) + if _, errStat := os.Stat(worktreePath); errStat == nil { + hasExistingPath = true + continue + } else if !errors.Is(errStat, fs.ErrNotExist) { + return true, fmt.Errorf("git token store: stat watcher removal path %s: %w", cleanRel, errStat) + } + + if _, errFile := tree.File(cleanRel); errFile == nil { + return true, fmt.Errorf("git token store: refusing watcher-originated removal of tracked auth %s; use an explicit delete", cleanRel) + } else if !errors.Is(errFile, object.ErrFileNotFound) { + return true, fmt.Errorf("git token store: inspect watcher removal path %s: %w", cleanRel, errFile) + } + } + if hasExistingPath { + return false, nil + } + // Explicit GitTokenStore.Delete already removed the path from HEAD. The + // subsequent filesystem watcher event is therefore redundant and safe to ignore. + return true, nil +} + func (s *GitTokenStore) resolveDeletePath(id string) (string, error) { if strings.ContainsRune(id, os.PathSeparator) || filepath.IsAbs(id) { return id, nil @@ -838,8 +893,14 @@ func (s *GitTokenStore) commitAndPushLocked(message string, relPaths ...string) continue } if _, err = worktree.Add(rel); err != nil { + if errors.Is(err, gitindex.ErrEntryNotFound) { + continue + } if errors.Is(err, os.ErrNotExist) { - if _, errRemove := worktree.Remove(rel); errRemove != nil && !errors.Is(errRemove, os.ErrNotExist) { + if _, errRemove := worktree.Remove(rel); errRemove != nil { + if errors.Is(errRemove, os.ErrNotExist) || errors.Is(errRemove, gitindex.ErrEntryNotFound) { + continue + } return fmt.Errorf("git token store: remove %s: %w", rel, errRemove) } } else { @@ -883,20 +944,32 @@ func (s *GitTokenStore) commitAndPushLocked(message string, relPaths ...string) } else if errRewrite := s.rewriteHeadAsSingleCommit(repo, headRef.Name(), commitHash, message, signature); errRewrite != nil { return errRewrite } + return s.pushRepositoryLocked(repo, repoDir) +} + +func (s *GitTokenStore) pushRepositoryLocked(repo *git.Repository, repoDir string) error { + if repo == nil { + return fmt.Errorf("git token store: repository is nil") + } + headRef, errHead := repo.Head() + if errHead != nil { + if errors.Is(errHead, plumbing.ErrReferenceNotFound) { + return nil + } + return fmt.Errorf("git token store: get head for push: %w", errHead) + } pushOpts := &git.PushOptions{ClientOptions: s.gitClientOptions(), Force: true} if s.branch != "" { pushOpts.RefSpecs = []config.RefSpec{config.RefSpec("refs/heads/" + s.branch + ":refs/heads/" + s.branch)} } else { // When branch is unset, pin push to the currently checked-out branch. - if headRef, err := repo.Head(); err == nil { - pushOpts.RefSpecs = []config.RefSpec{config.RefSpec(headRef.Name().String() + ":" + headRef.Name().String())} - } + pushOpts.RefSpecs = []config.RefSpec{config.RefSpec(headRef.Name().String() + ":" + headRef.Name().String())} } - if err = repo.Push(pushOpts); err != nil { - if errors.Is(err, git.NoErrAlreadyUpToDate) { + if errPush := repo.Push(pushOpts); errPush != nil { + if errors.Is(errPush, git.NoErrAlreadyUpToDate) { return nil } - return fmt.Errorf("git token store: push: %w", err) + return fmt.Errorf("git token store: push: %w", errPush) } s.maybeRunGC(repoDir) return nil diff --git a/internal/store/gitstore_test.go b/internal/store/gitstore_test.go index 9ec4c100..0c10c53c 100644 --- a/internal/store/gitstore_test.go +++ b/internal/store/gitstore_test.go @@ -1,10 +1,13 @@ package store import ( + "context" + "errors" "net/http" "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -12,6 +15,7 @@ import ( gitconfig "github.com/go-git/go-git/v6/config" "github.com/go-git/go-git/v6/plumbing" "github.com/go-git/go-git/v6/plumbing/object" + cliproxyauth "github.com/router-for-me/CLIProxyAPI/v7/sdk/cliproxy/auth" ) type testBranchSpec struct { @@ -239,6 +243,139 @@ func TestEnsureRepositoryResetsToRemoteDefaultWhenBranchUnset(t *testing.T) { assertRemoteBranchContents(t, remoteDir, "master", "local master update\n") } +func TestGitTokenStoreRefusesWatcherOriginatedAuthDeletion(t *testing.T) { + t.Parallel() + + root := t.TempDir() + remoteDir := setupGitRemoteRepository(t, root, "master", + testBranchSpec{name: "master", contents: "remote master branch\n"}, + ) + store := NewGitTokenStore(remoteDir, "", "", "") + baseDir := filepath.Join(root, "workspace", "auths") + store.SetBaseDir(baseDir) + if err := store.EnsureRepository(); err != nil { + t.Fatalf("EnsureRepository: %v", err) + } + + auth := &cliproxyauth.Auth{ + ID: "protected.json", + FileName: "protected.json", + Provider: "codex", + Metadata: map[string]any{"type": "codex", "access_token": "token"}, + } + path, err := store.Save(context.Background(), auth) + if err != nil { + t.Fatalf("Save: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/protected.json", true) + + if err := os.Remove(path); err != nil { + t.Fatalf("simulate unexpected local removal: %v", err) + } + err = store.PersistAuthFiles(context.Background(), "Remove auth protected.json", path) + if err == nil { + t.Fatal("PersistAuthFiles watcher removal error = nil, want fail-closed rejection") + } + if got := err.Error(); !strings.Contains(got, "refusing watcher-originated removal") { + t.Fatalf("PersistAuthFiles error = %q, want watcher-removal rejection", got) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/protected.json", true) +} + +func TestGitTokenStoreWatcherRemovalNoOpsAfterExplicitDelete(t *testing.T) { + t.Parallel() + + root := t.TempDir() + remoteDir := setupGitRemoteRepository(t, root, "master", + testBranchSpec{name: "master", contents: "remote master branch\n"}, + ) + store := NewGitTokenStore(remoteDir, "", "", "") + baseDir := filepath.Join(root, "workspace", "auths") + store.SetBaseDir(baseDir) + if err := store.EnsureRepository(); err != nil { + t.Fatalf("EnsureRepository: %v", err) + } + + auth := &cliproxyauth.Auth{ + ID: "explicit.json", + FileName: "explicit.json", + Provider: "codex", + Metadata: map[string]any{"type": "codex", "access_token": "token"}, + } + path, err := store.Save(context.Background(), auth) + if err != nil { + t.Fatalf("Save: %v", err) + } + // Management deletes unlink the file before invoking Store.Delete. + if err := os.Remove(path); err != nil { + t.Fatalf("pre-remove explicit auth: %v", err) + } + if err := store.Delete(context.Background(), path); err != nil { + t.Fatalf("Delete after pre-remove: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/explicit.json", false) + + if err := store.Delete(context.Background(), path); err != nil { + t.Fatalf("repeated Delete: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/explicit.json", false) + + if err := store.PersistAuthFiles(context.Background(), "Remove auth explicit.json", path); err != nil { + t.Fatalf("watcher removal after explicit delete: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/explicit.json", false) +} + +func TestGitTokenStoreRepeatedDeleteDoesNotOverwriteRemoteOnlyChanges(t *testing.T) { + t.Parallel() + + root := t.TempDir() + remoteDir := setupGitRemoteRepository(t, root, "master", + testBranchSpec{name: "master", contents: "remote master branch\n"}, + ) + storeA := NewGitTokenStore(remoteDir, "", "", "") + baseA := filepath.Join(root, "workspace-a", "auths") + storeA.SetBaseDir(baseA) + if err := storeA.EnsureRepository(); err != nil { + t.Fatalf("EnsureRepository A: %v", err) + } + authA := &cliproxyauth.Auth{ + ID: "a.json", + FileName: "a.json", + Provider: "codex", + Metadata: map[string]any{"type": "codex", "access_token": "a"}, + } + pathA, err := storeA.Save(context.Background(), authA) + if err != nil { + t.Fatalf("Save A: %v", err) + } + if err := storeA.Delete(context.Background(), pathA); err != nil { + t.Fatalf("Delete A: %v", err) + } + + storeB := NewGitTokenStore(remoteDir, "", "", "") + baseB := filepath.Join(root, "workspace-b", "auths") + storeB.SetBaseDir(baseB) + if err := storeB.EnsureRepository(); err != nil { + t.Fatalf("EnsureRepository B: %v", err) + } + authB := &cliproxyauth.Auth{ + ID: "b.json", + FileName: "b.json", + Provider: "codex", + Metadata: map[string]any{"type": "codex", "access_token": "b"}, + } + if _, err := storeB.Save(context.Background(), authB); err != nil { + t.Fatalf("Save B: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/b.json", true) + + if err := storeA.Delete(context.Background(), pathA); err != nil { + t.Fatalf("repeated Delete A: %v", err) + } + assertRemoteTreePath(t, remoteDir, "master", "auths/b.json", true) +} + func TestCommitAndPushLockedPushesBeforeRunningGC(t *testing.T) { root := t.TempDir() remoteDir := setupGitRemoteRepository(t, root, "master", @@ -497,6 +634,35 @@ func findBranchSpec(branches []testBranchSpec, name string) (testBranchSpec, boo return testBranchSpec{}, false } +func assertRemoteTreePath(t *testing.T, remoteDir, branch, path string, want bool) { + t.Helper() + + repo, err := git.PlainOpen(remoteDir) + if err != nil { + t.Fatalf("open remote repo: %v", err) + } + ref, err := repo.Reference(plumbing.NewBranchReferenceName(branch), true) + if err != nil { + t.Fatalf("read remote branch %s: %v", branch, err) + } + commit, err := repo.CommitObject(ref.Hash()) + if err != nil { + t.Fatalf("read remote commit: %v", err) + } + tree, err := commit.Tree() + if err != nil { + t.Fatalf("read remote tree: %v", err) + } + _, err = tree.File(filepath.ToSlash(path)) + got := err == nil + if err != nil && !errors.Is(err, object.ErrFileNotFound) { + t.Fatalf("inspect remote path %s: %v", path, err) + } + if got != want { + t.Fatalf("remote path %s exists = %v, want %v", path, got, want) + } +} + func assertRepositoryBranchAndContents(t *testing.T, repoDir, branch, wantContents string) { t.Helper() -- 2.51.2