Merge pull request from GHSA-hpcg-xjq5-g666

* Add memory consumption limits for memfs

* Clean up mix of per-file and total size code

* Use memboxfs for object storage

* Wire in git fetch limits, still no tests

* improve error handling

* cover git clone failure case

* delete unused file

* more tests

* remove comment

---------

Co-authored-by: Evan Anderson <[email protected]>

cmd/dev/app/image/cmd_verify.go

@@ -112,6 +112,7 @@ func runCmdVerify(cmd *cobra.Command, _ []string) error {
 func buildGitHubClient(token string) (provifv1.GitHub, error) {
  return clients.NewRestClient(
  &minderv1.GitHubProviderConfig{},
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential(token),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

cmd/dev/app/rule_type/rttst.go

@@ -302,7 +302,9 @@ func getProvider(pstr string, token string, providerConfigFile string) (provifv1
  case "github":
  client, err := clients.NewGitHubAppProvider(
  &minderv1.GitHubAppProviderConfig{},
- &serverconfig.GitHubAppConfig{AppName: "test"},
+ &serverconfig.ProviderConfig{
+ GitHubApp: &serverconfig.GitHubAppConfig{AppName: "test"},
+ },
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential(token),
  nil,

internal/config/server/provider.go

@@ -19,4 +19,10 @@ package server
 type ProviderConfig struct {
  GitHubApp *GitHubAppConfig `mapstructure:"github-app"`
  GitHub *GitHubConfig `mapstructure:"github"`
+ Git GitConfig `mapstructure:"git"`
+}
+
+type GitConfig struct {
+ MaxFiles int64 `mapstructure:"max_files" default:"10000"`
+ MaxBytes int64 `mapstructure:"max_bytes" default:"100_000_000"`
 }

internal/config/utils.go

@@ -258,7 +258,7 @@ func getDefaultValueForInt64(value string) (any, error) {
  var defaultValue any
  var err error
 
- defaultValue, err = strconv.Atoi(value)
+ defaultValue, err = strconv.ParseInt(value, 0, 0)
  if err == nil {
  return defaultValue, nil
  }
@@ -285,7 +285,7 @@ func getDefaultValue(field reflect.StructField, value string, valueName string)
  defaultValue, err = getDefaultValueForInt64(value)
  case reflect.Int32, reflect.Int16, reflect.Int8, reflect.Int,
  reflect.Uint64, reflect.Uint32, reflect.Uint16, reflect.Uint8, reflect.Uint:
- defaultValue, err = strconv.Atoi(value)
+ defaultValue, err = strconv.ParseInt(value, 0, 0)
  case reflect.Float64:
  defaultValue, err = strconv.ParseFloat(value, 64)
  case reflect.Bool:

internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect_test.go

@@ -56,6 +56,7 @@ func testGithubProvider(baseURL string) (provifv1.GitHub, error) {
  &pb.GitHubProviderConfig{
  Endpoint: baseURL,
  },
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/engine/actions/remediate/pull_request/pull_request_test.go

@@ -93,6 +93,7 @@ func testGithubProvider() (provifv1.GitHub, error) {
  &pb.GitHubProviderConfig{
  Endpoint: ghApiUrl + "/",
  },
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/engine/actions/remediate/rest/rest_test.go

@@ -56,6 +56,7 @@ func testGithubProvider(baseURL string) (provifv1.REST, error) {
  &pb.GitHubProviderConfig{
  Endpoint: baseURL,
  },
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/engine/ingester/artifact/artifact_test.go

@@ -48,6 +48,7 @@ func testGithubProvider() (provinfv1.GitHub, error) {
  &pb.GitHubProviderConfig{
  Endpoint: baseURL,
  },
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/engine/ingester/git/git_test.go

@@ -17,6 +17,8 @@ package git_test
 import (
  "bytes"
  "context"
+ "github.com/stacklok/minder/internal/config/server"
+ v1 "github.com/stacklok/minder/pkg/providers/v1"
  "testing"
 
  "github.com/stretchr/testify/require"
@@ -31,9 +33,18 @@ import (
 func TestGitIngestWithCloneURLFromRepo(t *testing.T) {
  t.Parallel()
 
+ cfg := server.GitConfig{
+ MaxFiles: 100,
+ MaxBytes: 1_000_000,
+ }
+ gitClient := gitclient.NewGit(
+ credentials.NewEmptyCredential(),
+ gitclient.WithConfig(cfg),
+ )
+
  gi, err := gitengine.NewGitIngester(
  &pb.GitType{Branch: "master"},
- gitclient.NewGit(credentials.NewEmptyCredential()),
+ gitClient,
  )
  require.NoError(t, err, "expected no error")
 
@@ -193,3 +204,59 @@ func TestGitIngestFailsBecauseOfUnexistentCloneUrl(t *testing.T) {
  require.Error(t, err, "expected error")
  require.Nil(t, got, "expected nil result")
 }
+
+func TestGitIngestFailsWhenRepoTooLarge(t *testing.T) {
+ t.Parallel()
+
+ // set size limit to 1 byte
+ cfg := server.GitConfig{
+ MaxFiles: 100,
+ MaxBytes: 1,
+ }
+ gitClient := gitclient.NewGit(
+ credentials.NewEmptyCredential(),
+ gitclient.WithConfig(cfg),
+ )
+
+ gi, err := gitengine.NewGitIngester(
+ &pb.GitType{Branch: "master"},
+ gitClient,
+ )
+
+ require.NoError(t, err, "expected no error")
+
+ got, err := gi.Ingest(context.Background(), &pb.Artifact{}, map[string]any{
+ "clone_url": "https://github.com/octocat/Hello-World.git",
+ })
+ require.Error(t, err, "expected error")
+ require.ErrorIs(t, err, v1.ErrRepositoryTooLarge, "expected ErrRepositoryTooLarge")
+ require.Nil(t, got, "expected non-nil result")
+}
+
+func TestGitIngestFailsWhenRepoHasTooManyFiles(t *testing.T) {
+ t.Parallel()
+
+ // will fail because of files in .git
+ cfg := server.GitConfig{
+ MaxFiles: 1,
+ MaxBytes: 1_000_000,
+ }
+ gitClient := gitclient.NewGit(
+ credentials.NewEmptyCredential(),
+ gitclient.WithConfig(cfg),
+ )
+
+ gi, err := gitengine.NewGitIngester(
+ &pb.GitType{Branch: "master"},
+ gitClient,
+ )
+
+ require.NoError(t, err, "expected no error")
+
+ got, err := gi.Ingest(context.Background(), &pb.Artifact{}, map[string]any{
+ "clone_url": "https://github.com/octocat/Hello-World.git",
+ })
+ require.Error(t, err, "expected error")
+ require.ErrorIs(t, err, v1.ErrRepositoryTooLarge, "expected ErrRepositoryTooLarge")
+ require.Nil(t, got, "expected non-nil result")
+}

internal/engine/ingester/ingester_test.go

@@ -160,6 +160,7 @@ func TestNewRuleDataIngest(t *testing.T) {
 
  client, err := clients.NewRestClient(
  &pb.GitHubProviderConfig{},
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/engine/ingester/rest/rest_test.go

@@ -114,6 +114,7 @@ func testGithubProviderBuilder(baseURL string) (provifv1.REST, error) {
  &pb.GitHubProviderConfig{
  Endpoint: baseURL,
  },
+ nil,
  &ratecache.NoopRestClientCache{},
  credentials.NewGitHubTokenCredential("token"),
  clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),

internal/providers/git/git.go

@@ -19,30 +19,51 @@ import (
  "context"
  "errors"
  "fmt"
+ "io/fs"
 
  "github.com/go-git/go-billy/v5/memfs"
+ "github.com/go-git/go-git/v5/plumbing/cache"
+ "github.com/go-git/go-git/v5/storage/filesystem"
+
  "github.com/go-git/go-git/v5"
  "github.com/go-git/go-git/v5/plumbing"
  "github.com/go-git/go-git/v5/plumbing/transport"
- "github.com/go-git/go-git/v5/storage/memory"
-
+ "github.com/stacklok/minder/internal/config/server"
+ "github.com/stacklok/minder/internal/providers/git/memboxfs"
  minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
  provifv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
 // Git is the struct that contains the GitHub REST API client
 type Git struct {
  credential provifv1.GitCredential
+ maxFiles int64
+ maxBytes int64
 }
 
+const maxCachedObjectSize = 100 * 1024 // 100KiB
+
 // Ensure that the Git client implements the Git interface
 var _ provifv1.Git = (*Git)(nil)
 
+type Options func(*Git)
+
 // NewGit creates a new GitHub client
-func NewGit(token provifv1.GitCredential) *Git {
- return &Git{
+func NewGit(token provifv1.GitCredential, opts ...Options) *Git {
+ ret := &Git{
  credential: token,
  }
+ for _, opt := range opts {
+ opt(ret)
+ }
+ return ret
+}
+
+func WithConfig(cfg server.GitConfig) Options {
+ return func(g *Git) {
+ g.maxFiles = cfg.MaxFiles
+ g.maxBytes = cfg.MaxBytes
+ }
 }
 
 // CanImplement returns true/false depending on whether the Provider
@@ -67,20 +88,32 @@ func (g *Git) Clone(ctx context.Context, url, branch string) (*git.Repository, e
  return nil, fmt.Errorf("invalid clone options: %w", err)
  }
 
- storer := memory.NewStorage()
- fs := memfs.New()
+ // TODO(#3582): Switch this to use a tmpfs backed clone
+ memFS := memfs.New()
+ if g.maxFiles != 0 && g.maxBytes != 0 {
+ memFS = &memboxfs.LimitedFs{
+ Fs: memFS,
+ MaxFiles: g.maxFiles,
+ TotalFileSize: g.maxBytes,
+ }
+ }
+ storerCache := cache.NewObjectLRU(maxCachedObjectSize)
+ // reuse same FS for storage and cloned files
+ storer := filesystem.NewStorage(memFS, storerCache)
 
  // We clone to the memfs go-billy filesystem driver, which doesn't
  // allow for direct access to the underlying filesystem. This is
  // because we want to be able to run this in a sandboxed environment
  // where we don't have access to the underlying filesystem.
- r, err := git.CloneContext(ctx, storer, fs, opts)
+ r, err := git.CloneContext(ctx, storer, memFS, opts)
  if err != nil {
  var refspecerr git.NoMatchingRefSpecError
  if errors.Is(err, git.ErrBranchNotFound) || refspecerr.Is(err) {
  return nil, provifv1.ErrProviderGitBranchNotFound
  } else if errors.Is(err, transport.ErrEmptyRemoteRepository) {
  return nil, provifv1.ErrRepositoryEmpty
+ } else if errors.Is(err, fs.ErrPermission) {
+ return nil, provifv1.ErrRepositoryTooLarge
  }
  return nil, fmt.Errorf("could not clone repo: %w", err)
  }