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 <evan@stacklok.com>

Author: dmjb

Diff for: 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()),

Diff for: 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,

Diff for: 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"`
 }

Diff for: 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:

Diff for: 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()),

Diff for: 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()),

Diff for: 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()),

Diff for: 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()),

Diff for: 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")
+}

Diff for: 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()),

Diff for: 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()),

Diff for: 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)
 	}