fix: internal server error on a large .gitmodules (#10744)

Fix #10714 (introduced in #8438) by silently ignoring large .gitmodules files.

Additionally:
- the limit was bumped from 10KB to 64KB (https://github.com/boostorg/boost/blob/master/.gitmodules has 20KB)
- a warning is shown on the .gitmodules view page if this limit is exceeded

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10744
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: oliverpool <git@olivier.pfad.fr>
Co-committed-by: oliverpool <git@olivier.pfad.fr>

.deadcode-out

@@ -238,6 +238,7 @@ forgejo.org/services/repository
 
 forgejo.org/services/repository/files
 	ContentType.String
+	RepoFileOptionMode
 
 forgejo.org/services/repository/gitgraph
 	Parser.Reset

modules/git/blob.go

@@ -141,6 +141,28 @@ func (b *Blob) Name() string {
 	return b.name
 }
 
+// NewReader return a blob-reader which fails immediately with [BlobTooLargeError] if the file is bigger than the limit
+func (b *Blob) NewReader(limit int64) (rc io.ReadCloser, actualSize int64, err error) {
+	actualSize = b.Size()
+	if actualSize > limit {
+		return nil, actualSize, BlobTooLargeError{
+			Size:  actualSize,
+			Limit: limit,
+		}
+	}
+	r, _, cancel, err := b.newReader()
+	if err != nil {
+		return nil, actualSize, err
+	}
+
+	return &blobReader{
+		rd:                r,
+		n:                 actualSize,
+		additionalDiscard: 0,
+		cancel:            cancel,
+	}, actualSize, nil
+}
+
 // NewTruncatedReader return a blob-reader which silently truncates when the limit is reached (io.EOF will be returned)
 func (b *Blob) NewTruncatedReader(limit int64) (rc io.ReadCloser, fullSize int64, err error) {
 	r, fullSize, cancel, err := b.newReader()
@@ -168,14 +190,7 @@ func (b BlobTooLargeError) Error() string {
 // GetContentBase64 Reads the content of the blob and returns it as base64 encoded string.
 // Returns [BlobTooLargeError] if the (unencoded) content is larger than the limit.
 func (b *Blob) GetContentBase64(limit int64) (string, error) {
-	if b.Size() > limit {
-		return "", BlobTooLargeError{
-			Size:  b.Size(),
-			Limit: limit,
-		}
-	}
-
-	rc, size, err := b.NewTruncatedReader(limit)
+	rc, size, err := b.NewReader(limit)
 	if err != nil {
 		return "", err
 	}

modules/git/submodule.go

@@ -5,6 +5,7 @@
 package git
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -19,6 +20,8 @@ import (
 	"gopkg.in/ini.v1" //nolint:depguard // used to read .gitmodules
 )
 
+const MaxGitmodulesFileSize = 64 * 1024
+
 // GetSubmodule returns the Submodule of a given path
 func (c *Commit) GetSubmodule(path string, entry *TreeEntry) (Submodule, error) {
 	err := c.readSubmodules()
@@ -55,8 +58,12 @@ func (c *Commit) readSubmodules() error {
 		return err
 	}
 
-	rc, _, err := entry.Blob().NewTruncatedReader(10 * 1024)
+	rc, _, err := entry.Blob().NewReader(MaxGitmodulesFileSize)
 	if err != nil {
+		if errors.As(err, &BlobTooLargeError{}) {
+			c.submodules = make(map[string]Submodule)
+			return nil
+		}
 		return err
 	}
 	defer rc.Close()

options/locale_next/locale_en-US.json

@@ -88,6 +88,7 @@
 	"repo.pulls.maintainers_can_edit": "Maintainers can edit this pull request.",
 	"repo.pulls.maintainers_cannot_edit": "Maintainers cannot edit this pull request.",
 	"repo.form.cannot_create": "All spaces in which you can create repositories have reached the limit of repositories.",
+	"repo.view.gitmodules_too_large": "The .gitmodules file is too large and will be ignored (on API calls for instance)",
 	"migrate.form.error.url_credentials": "The URL contains credentials, put them in the username and password fields respectively",
 	"migrate.github.description": "Migrate data from github.com or GitHub Enterprise server.",
 	"migrate.git.description": "Migrate a repository only from any Git service.",

routers/web/repo/view.go

@@ -445,6 +445,10 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry) {
 				ctx.Data["FileWarning"] = strings.Join(warnings, "\n")
 			}
 		}
+	} else if ctx.Repo.TreePath == ".gitmodules" {
+		if fInfo.fileSize > git.MaxGitmodulesFileSize {
+			ctx.Data["FileWarning"] = ctx.Locale.Tr("repo.view.gitmodules_too_large")
+		}
 	}
 
 	isDisplayingSource := ctx.FormString("display") == "source"

services/repository/files/update.go

@@ -43,7 +43,6 @@ type ChangeRepoFile struct {
 	ContentReader io.ReadSeeker
 	SHA           string
 	Options       *RepoFileOptions
-	Symlink       bool
 }
 
 // ChangeRepoFilesOptions holds the repository files update options
@@ -62,8 +61,13 @@ type ChangeRepoFilesOptions struct {
 type RepoFileOptions struct {
 	treePath     string
 	fromTreePath string
-	executable   bool
-	symlink      bool
+	entryMode    git.EntryMode
+}
+
+func RepoFileOptionMode(entryMode git.EntryMode) *RepoFileOptions {
+	return &RepoFileOptions{
+		entryMode: entryMode,
+	}
 }
 
 // ChangeRepoFiles adds, updates or removes multiple files in the given repository
@@ -114,11 +118,14 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 			}
 		}
 
+		mode := git.EntryModeBlob
+		if file.Options != nil && file.Options.entryMode != 0 {
+			mode = file.Options.entryMode
+		}
 		file.Options = &RepoFileOptions{
 			treePath:     treePath,
 			fromTreePath: fromTreePath,
-			executable:   false,
-			symlink:      file.Symlink,
+			entryMode:    mode,
 		}
 		treePaths = append(treePaths, treePath)
 	}
@@ -320,7 +327,7 @@ func handleCheckErrors(file *ChangeRepoFile, actualBaseCommit *git.Commit, lastK
 			// haven't been made. We throw an error if one wasn't provided.
 			return models.ErrSHAOrCommitIDNotProvided{}
 		}
-		file.Options.executable = fromEntry.IsExecutable()
+		file.Options.entryMode = fromEntry.Mode()
 	}
 	if file.Operation == "create" || file.Operation == "update" {
 		// For the path where this file will be created/updated, we need to make
@@ -430,13 +437,7 @@ func CreateOrUpdateFile(ctx context.Context, t *TemporaryUploadRepository, file
 	}
 
 	// Add the object to the index
-	mode := "100644" // regular file
-	if file.Options.executable {
-		mode = "100755"
-	} else if file.Options.symlink {
-		mode = "120644"
-	}
-	if err := t.AddObjectToIndex(mode, objectHash, file.Options.treePath); err != nil {
+	if err := t.AddObjectToIndex(file.Options.entryMode.String(), objectHash, file.Options.treePath); err != nil {
 		return err
 	}
 

tests/integration/repo_generate_test.go

@@ -343,7 +343,7 @@ func TestRepoGenerateTemplatingSymlink(t *testing.T) {
 							Operation:     "create",
 							TreePath:      "problem/Readme.md",
 							ContentReader: strings.NewReader(tc.symlinkTarget),
-							Symlink:       true,
+							Options:       files_service.RepoFileOptionMode(git.EntryModeSymlink),
 						},
 					}),
 				})
@@ -420,7 +420,7 @@ func TestRepoGenerateTemplatingSymlinkGlobFile(t *testing.T) {
 					Operation:     "create",
 					TreePath:      ".forgejo/template",
 					ContentReader: strings.NewReader("/etc/passwd"),
-					Symlink:       true,
+					Options:       files_service.RepoFileOptionMode(git.EntryModeSymlink),
 				},
 			}),
 		})

tests/integration/repo_test.go

@@ -1551,42 +1551,130 @@ func TestRepoIssueFilterLinks(t *testing.T) {
 func TestRepoSubmoduleView(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-		repo, _, f := tests.CreateDeclarativeRepo(t, user2, "", []unit_model.Type{unit_model.TypeCode}, nil, nil)
-		defer f()
+		t.Run("FromGit", func(t *testing.T) {
+			repo, _, f := tests.CreateDeclarativeRepo(t, user2, "", []unit_model.Type{unit_model.TypeCode}, nil, nil)
+			defer f()
 
-		// Clone the repository, add a submodule and push it.
-		dstPath := t.TempDir()
+			// Clone the repository, add a submodule and push it.
+			dstPath := t.TempDir()
 
-		uClone := *u
-		uClone.Path = repo.FullName()
-		uClone.User = url.UserPassword(user2.Name, userPassword)
+			uClone := *u
+			uClone.Path = repo.FullName()
+			uClone.User = url.UserPassword(user2.Name, userPassword)
 
-		t.Run("Clone", doGitClone(dstPath, &uClone))
+			t.Run("Clone", doGitClone(dstPath, &uClone))
 
-		_, _, err := git.NewCommand(git.DefaultContext, "submodule", "add").AddDynamicArguments(u.JoinPath("/user2/repo1").String()).RunStdString(&git.RunOpts{Dir: dstPath})
-		require.NoError(t, err)
+			_, _, err := git.NewCommand(git.DefaultContext, "submodule", "add").AddDynamicArguments(u.JoinPath("/user2/repo1").String()).RunStdString(&git.RunOpts{Dir: dstPath})
+			require.NoError(t, err)
 
-		_, _, err = git.NewCommand(git.DefaultContext, "add", "repo1", ".gitmodules").RunStdString(&git.RunOpts{Dir: dstPath})
-		require.NoError(t, err)
+			_, _, err = git.NewCommand(git.DefaultContext, "add", "repo1", ".gitmodules").RunStdString(&git.RunOpts{Dir: dstPath})
+			require.NoError(t, err)
 
-		_, _, err = git.NewCommand(git.DefaultContext, "commit", "-m", "add submodule").RunStdString(&git.RunOpts{Dir: dstPath})
-		require.NoError(t, err)
+			_, _, err = git.NewCommand(git.DefaultContext, "commit", "-m", "add submodule").RunStdString(&git.RunOpts{Dir: dstPath})
+			require.NoError(t, err)
 
-		_, _, err = git.NewCommand(git.DefaultContext, "push").RunStdString(&git.RunOpts{Dir: dstPath})
-		require.NoError(t, err)
+			_, _, err = git.NewCommand(git.DefaultContext, "push").RunStdString(&git.RunOpts{Dir: dstPath})
+			require.NoError(t, err)
 
-		// Check that the submodule entry exist and the link is correct.
-		req := NewRequest(t, "GET", "/"+repo.FullName())
-		resp := MakeRequest(t, req, http.StatusOK)
+			// Check that the submodule entry exist and the link is correct.
+			req := NewRequest(t, "GET", "/"+repo.FullName())
+			resp := MakeRequest(t, req, http.StatusOK)
 
-		htmlDoc := NewHTMLParser(t, resp.Body)
-		htmlDoc.AssertElement(t, fmt.Sprintf(`tr[data-entryname="repo1"] a[href="%s"]`, u.JoinPath("/user2/repo1").String()), true)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			htmlDoc.AssertElement(t, fmt.Sprintf(`tr[data-entryname="repo1"] a[href="%s"]`, u.JoinPath("/user2/repo1").String()), true)
 
-		// Check that a link to the submodule returns a redirect and that the redirect link is correct.
-		req = NewRequest(t, "GET", "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/repo1")
-		resp = MakeRequest(t, req, http.StatusSeeOther)
+			// Check that a link to the submodule returns a redirect and that the redirect link is correct.
+			req = NewRequest(t, "GET", "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/repo1")
+			resp = MakeRequest(t, req, http.StatusSeeOther)
 
-		assert.Equal(t, u.JoinPath("/user2/repo1").String(), resp.Header().Get("Location"))
+			assert.Equal(t, u.JoinPath("/user2/repo1").String(), resp.Header().Get("Location"))
+		})
+
+		t.Run("Declarative", func(t *testing.T) {
+			repo, _, f := tests.CreateDeclarativeRepo(t, user2, "", []unit_model.Type{unit_model.TypeCode}, nil, []*files_service.ChangeRepoFile{
+				{
+					Operation: "create",
+					TreePath:  ".gitmodules",
+					ContentReader: strings.NewReader(`[submodule "relative-module"]
+  path = relative-module
+  url = https://git.example.org/submodule.git
+`),
+				}, {
+					Operation:     "create",
+					TreePath:      "relative-module",
+					FromTreePath:  "",
+					ContentReader: nil,
+					SHA:           "95601d16476a",
+					Options:       files_service.RepoFileOptionMode(git.EntryModeCommit),
+				},
+			})
+			defer f()
+
+			// Check that the submodule entry exist and the link is correct.
+			req := NewRequest(t, "GET", "/"+repo.FullName())
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			expectedDst := "https://git.example.org/submodule"
+			htmlDoc := NewHTMLParser(t, resp.Body)
+
+			href, ok := htmlDoc.Find(`tr[data-entryname="relative-module"] a`).Attr("href")
+			assert.True(t, ok, "could not find entry 'relative-module' in file list")
+			assert.Equal(t, expectedDst, href)
+
+			// Check that a link to the submodule returns a redirect and that the redirect link is correct.
+			req = NewRequest(t, "GET", "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/relative-module")
+			resp = MakeRequest(t, req, http.StatusSeeOther)
+
+			assert.Equal(t, expectedDst, resp.Header().Get("Location"))
+		})
+
+		t.Run("SubmodulesFileTooBig", func(t *testing.T) {
+			repo, _, f := tests.CreateDeclarativeRepo(t, user2, "", []unit_model.Type{unit_model.TypeCode}, nil, []*files_service.ChangeRepoFile{
+				{
+					Operation: "create",
+					TreePath:  ".gitmodules",
+					ContentReader: strings.NewReader(strings.Repeat("#", git.MaxGitmodulesFileSize-5) + // ensure that the partial read is invalid
+						`
+[submodule "relative-module"]
+  path = relative-module
+  url = https://git.example.org/submodule.git
+`),
+				}, {
+					Operation:     "create",
+					TreePath:      "relative-module",
+					FromTreePath:  "",
+					ContentReader: nil,
+					SHA:           "95601d16476a",
+					Options:       files_service.RepoFileOptionMode(git.EntryModeCommit),
+				},
+			})
+			defer f()
+
+			// Check that the submodule entry exist and the link is correct.
+			req := NewRequest(t, "GET", "/"+repo.FullName())
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+
+			_, ok := htmlDoc.Find(`tr[data-entryname="relative-module"] td.name a`).Attr("href")
+			assert.False(t, ok, "should not find a link to 'relative-module' in file list")
+
+			// Check that a link to the submodule returns a redirect and that the redirect link is correct.
+			req = NewRequest(t, "GET", "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/relative-module")
+			resp = MakeRequest(t, req, http.StatusSeeOther)
+
+			assert.Equal(t, "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/", resp.Header().Get("Location"))
+
+			// Check that a warning is present
+			req = NewRequest(t, "GET", "/"+repo.FullName()+"/src/branch/"+repo.DefaultBranch+"/.gitmodules")
+			resp = MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc = NewHTMLParser(t, resp.Body)
+
+			warn, err := htmlDoc.Find(`.non-diff-file-content .warning`).Html()
+			require.NoError(t, err)
+			assert.NotEmpty(t, warn)
+		})
 	})
 }