From d38936cdc96aa3693aaa1beac85b1aa6ce7eba24 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Apr 2026 10:02:40 +0000 Subject: [PATCH] fix: normalize "." repo identifier to auto-detect in openGitRepo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user passes "." as a repo identifier (e.g., `aifr log .`, `aifr refs .`, `aifr stash-list .`), it means "this directory" — semantically identical to the no-argument auto-detect case. Previously, "." was treated as an explicit filesystem path and subjected to access control on the repo root, causing it to be denied when the auto-detect path (empty string) would succeed for the same repo. Fix by normalizing "." to "" at the top of openGitRepo, the single chokepoint for all git operations. This applies consistently to all git commands (log, refs, stash-list, rev-parse, git-config) without touching any individual command's arg parsing. ".." and other relative paths ("./sub", "../other") remain subject to access control since they may reference a different repository. https://claude.ai/code/session_016QBk2VFvP92AXspnSePeLz --- internal/engine/gitops.go | 8 ++ internal/engine/gitops_test.go | 129 +++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 internal/engine/gitops_test.go diff --git a/internal/engine/gitops.go b/internal/engine/gitops.go index ab602ed..e3b8259 100644 --- a/internal/engine/gitops.go +++ b/internal/engine/gitops.go @@ -17,7 +17,15 @@ import ( // openGitRepo opens a git repository and checks access control for // filesystem-path repos. Named repos and CWD auto-detect skip the // access check since they are admin-configured or implicitly allowed. +// +// Bare "." is normalized to "" (auto-detect) since it means "this +// directory" — semantically identical to the no-argument case. Other +// relative paths ("./sub", "..", "../other") remain subject to access +// control because they may reference a different repository. func (e *Engine) openGitRepo(repoIdentifier string) (*git.Repository, string, error) { + if repoIdentifier == "." { + repoIdentifier = "" + } repo, repoPath, err := e.gitProvider.OpenRepo(repoIdentifier) if err != nil { return nil, "", err diff --git a/internal/engine/gitops_test.go b/internal/engine/gitops_test.go new file mode 100644 index 0000000..0ef127e --- /dev/null +++ b/internal/engine/gitops_test.go @@ -0,0 +1,129 @@ +// Copyright 2026 — see LICENSE file for terms. +package engine + +import ( + "os" + "testing" + + "go.pennock.tech/aifr/internal/accessctl" + "go.pennock.tech/aifr/internal/config" +) + +// TestOpenGitRepoDotNormalization verifies that "." is treated as +// auto-detect (same as "") rather than as an explicit filesystem path. +// This matters because auto-detect skips access control (the user is +// already in the directory), while explicit paths are checked. +func TestOpenGitRepoDotNormalization(t *testing.T) { + dir := t.TempDir() + initTestGitRepo(t, dir, 1) + + // Create an engine that does NOT allow the repo dir. + // Use a non-existent allow path so nothing is permitted. + checker, err := accessctl.NewChecker(accessctl.CheckerParams{ + Allow: []string{"/nonexistent-allow-path/**"}, + }) + if err != nil { + t.Fatal(err) + } + eng, err := NewEngine(checker, config.DefaultConfig()) + if err != nil { + t.Fatal(err) + } + + // cd into the repo so auto-detect finds it. + origDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chdir(origDir) }) + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + + // Auto-detect (empty string) should succeed — no access check. + _, err = eng.Log("", "HEAD", LogParams{MaxCount: 1}) + if err != nil { + t.Errorf("Log with empty repo (auto-detect) failed: %v", err) + } + + // Bare "." should also succeed — normalized to auto-detect. + _, err = eng.Log(".", "HEAD", LogParams{MaxCount: 1}) + if err != nil { + t.Errorf("Log with \".\" repo failed: %v", err) + } + + // Explicit absolute path should be denied. + _, err = eng.Log(dir, "HEAD", LogParams{MaxCount: 1}) + if err == nil { + t.Error("Log with explicit absolute path should have been denied") + } + + // ".." should be denied (it's a different path, subject to access control). + // Create a subdir and cd into it so ".." resolves to the repo root. + subdir := dir + "/subdir" + os.Mkdir(subdir, 0o755) + if err := os.Chdir(subdir); err != nil { + t.Fatal(err) + } + _, err = eng.Log("..", "HEAD", LogParams{MaxCount: 1}) + if err == nil { + t.Error("Log with \"..\" should have been denied") + } +} + +// TestDotRepoConsistentAcrossCommands verifies that "." normalization +// works for all git commands, not just log. +func TestDotRepoConsistentAcrossCommands(t *testing.T) { + dir := t.TempDir() + initTestGitRepo(t, dir, 1) + + checker, err := accessctl.NewChecker(accessctl.CheckerParams{ + Allow: []string{"/nonexistent-allow-path/**"}, + }) + if err != nil { + t.Fatal(err) + } + eng, err := NewEngine(checker, config.DefaultConfig()) + if err != nil { + t.Fatal(err) + } + + origDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chdir(origDir) }) + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + + // All these should succeed with "." (normalized to auto-detect). + t.Run("Refs", func(t *testing.T) { + _, err := eng.Refs(".", true, true, true) + if err != nil { + t.Errorf("Refs with \".\" failed: %v", err) + } + }) + + t.Run("Log", func(t *testing.T) { + _, err := eng.Log(".", "HEAD", LogParams{MaxCount: 1}) + if err != nil { + t.Errorf("Log with \".\" failed: %v", err) + } + }) + + // All these should fail with explicit absolute path. + t.Run("Refs_absolute_denied", func(t *testing.T) { + _, err := eng.Refs(dir, true, true, true) + if err == nil { + t.Error("Refs with absolute path should have been denied") + } + }) + + t.Run("Log_absolute_denied", func(t *testing.T) { + _, err := eng.Log(dir, "HEAD", LogParams{MaxCount: 1}) + if err == nil { + t.Error("Log with absolute path should have been denied") + } + }) +}