From 7bfa52afb7c8369be1f6e8885debc096acdb85a1 Mon Sep 17 00:00:00 2001 From: Andrew Haines Date: Wed, 30 Aug 2023 10:11:32 +0100 Subject: [PATCH] fix: Normalize Git store subdirectory config to handle leading `./` correctly (#1774) Signed-off-by: Andrew Haines --- internal/storage/git/conf.go | 22 ++++--- internal/storage/git/store.go | 14 ++-- internal/storage/git/store_test.go | 102 +++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 18 deletions(-) diff --git a/internal/storage/git/conf.go b/internal/storage/git/conf.go index 0fcf10121..abe7d28a5 100644 --- a/internal/storage/git/conf.go +++ b/internal/storage/git/conf.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/go-git/go-git/v5/plumbing/transport" @@ -94,33 +95,36 @@ func (conf *Conf) Key() string { return confKey } -func (conf *Conf) Validate() error { - var errs []error - +func (conf *Conf) Validate() (errs error) { switch conf.Protocol { case "ssh", "http", "https", "file": default: - errs = append(errs, fmt.Errorf("unknown git protocol: %s", conf.Protocol)) + errs = multierr.Append(errs, fmt.Errorf("unknown git protocol: %s", conf.Protocol)) } if conf.URL == "" { - errs = append(errs, errors.New("git URL is required")) + errs = multierr.Append(errs, errors.New("git URL is required")) } if conf.CheckoutDir == "" { cacheDir, err := os.UserCacheDir() if err != nil { - errs = append(errs, fmt.Errorf("checkoutDir unspecified and failed to determine user cache dir: %w", err)) + errs = multierr.Append(errs, fmt.Errorf("checkoutDir unspecified and failed to determine user cache dir: %w", err)) } else { conf.CheckoutDir = filepath.Join(cacheDir, util.AppName, "git") } } - if len(errs) > 0 { - return multierr.Combine(errs...) + subDir := conf.getSubDir() + if filepath.IsAbs(subDir) || strings.HasPrefix(subDir, "../") || subDir == ".." { + errs = multierr.Append(errs, errors.New("subDir must be a relative path within the repository")) } - return nil + return errs +} + +func (conf *Conf) getSubDir() string { + return filepath.ToSlash(filepath.Clean(conf.SubDir)) } func (conf *Conf) getBranch() string { diff --git a/internal/storage/git/store.go b/internal/storage/git/store.go index 31f84233e..c81dac33a 100644 --- a/internal/storage/git/store.go +++ b/internal/storage/git/store.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "os" - "path/filepath" "strings" "time" @@ -56,12 +55,14 @@ type Store struct { repo *git.Repository sf singleflight.Group *storage.SubscriptionManager + subDir string } func NewStore(ctx context.Context, conf *Conf) (*Store, error) { s := &Store{ log: zap.S().Named("git.store").With("dir", conf.CheckoutDir), conf: conf, + subDir: conf.getSubDir(), SubscriptionManager: storage.NewSubscriptionManager(ctx), } @@ -227,12 +228,7 @@ func (s *Store) cloneRepo(ctx context.Context) error { } func (s *Store) loadAll(ctx context.Context) error { - policyDir := "." - if s.conf.SubDir != "" { - policyDir = s.conf.SubDir - } - - idx, err := index.Build(ctx, os.DirFS(s.conf.CheckoutDir), index.WithRootDir(policyDir)) + idx, err := index.Build(ctx, os.DirFS(s.conf.CheckoutDir), index.WithRootDir(s.subDir)) if err != nil { return err } @@ -448,8 +444,8 @@ func (s *Store) normalizePath(path string) (string, util.IndexedFileType) { return path, util.FileTypeNotIndexed } - if s.conf.SubDir != "" { - relativePath := strings.TrimPrefix(path, strings.TrimSuffix(filepath.ToSlash(s.conf.SubDir), "/")+"/") + if s.subDir != "." { + relativePath := strings.TrimPrefix(path, s.subDir+"/") if path == relativePath { // not in policies directory return path, util.FileTypeNotIndexed } diff --git a/internal/storage/git/store_test.go b/internal/storage/git/store_test.go index c3620cbfe..4234e2e92 100644 --- a/internal/storage/git/store_test.go +++ b/internal/storage/git/store_test.go @@ -18,6 +18,7 @@ import ( "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -31,6 +32,7 @@ import ( "github.com/cerbos/cerbos/internal/storage/internal" "github.com/cerbos/cerbos/internal/test" "github.com/cerbos/cerbos/internal/test/mocks" + "github.com/cerbos/cerbos/internal/util" ) const ( @@ -579,6 +581,106 @@ func TestReloadable(t *testing.T) { internal.TestSuiteReloadable(store, mkInitFn(t, sourceGitDir), mkAddFn(t, sourceGitDir, ps), mkDeleteFn(t, sourceGitDir))(t) } +func TestNormalizePath(t *testing.T) { + testCases := []struct { + subDir string + path string + wantPath string + wantFileType util.IndexedFileType + }{ + { + subDir: ".", + path: "foo.yaml", + wantPath: "foo.yaml", + wantFileType: util.FileTypePolicy, + }, + { + subDir: ".", + path: "foo.json", + wantPath: "foo.json", + wantFileType: util.FileTypePolicy, + }, + { + subDir: ".", + path: "foo/bar.yaml", + wantPath: "foo/bar.yaml", + wantFileType: util.FileTypePolicy, + }, + { + subDir: "foo", + path: "foo/bar.yaml", + wantPath: "bar.yaml", + wantFileType: util.FileTypePolicy, + }, + { + subDir: "foo", + path: "foo/bar/baz.yaml", + wantPath: "bar/baz.yaml", + wantFileType: util.FileTypePolicy, + }, + { + subDir: ".", + path: "_schemas/foo.json", + wantPath: "foo.json", + wantFileType: util.FileTypeSchema, + }, + { + subDir: ".", + path: "_schemas/foo/bar.json", + wantPath: "foo/bar.json", + wantFileType: util.FileTypeSchema, + }, + { + subDir: "foo", + path: "foo/_schemas/bar.json", + wantPath: "bar.json", + wantFileType: util.FileTypeSchema, + }, + { + subDir: "foo", + path: "foo/_schemas/bar/baz.json", + wantPath: "bar/baz.json", + wantFileType: util.FileTypeSchema, + }, + { + subDir: ".", + path: "", + wantPath: "", + wantFileType: util.FileTypeNotIndexed, + }, + { + subDir: ".", + path: "foo.txt", + wantPath: "foo.txt", + wantFileType: util.FileTypeNotIndexed, + }, + { + subDir: ".", + path: "_schemas/foo.yaml", + wantPath: "_schemas/foo.yaml", + wantFileType: util.FileTypeNotIndexed, + }, + { + subDir: "foo", + path: "bar.yaml", + wantPath: "bar.yaml", + wantFileType: util.FileTypeNotIndexed, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(fmt.Sprintf("subDir=%s,path=%s", tc.subDir, tc.path), func(t *testing.T) { + store := &Store{subDir: tc.subDir} + + havePath, haveFileType := store.normalizePath(tc.path) + + assert.Equal(t, tc.wantPath, havePath, "Unexpected path") + assert.Equal(t, tc.wantFileType, haveFileType, "Unexpected file type") + }) + } +} + func mkInitFn(t *testing.T, sourceGitDir string) internal.MutateStoreFn { t.Helper()