feat: fsck incoming objects (#12695)

Weirdly, git doesn't verify the consistency of objects when receiving
new objects. Enable that git verifies this, so we don't allow a
repository to get in a weird or even corrupt state.

We've already dealt with a few cases of inconsistent objects, the most
notable one being mode of objects (forgejo/forgejo!9161). This can be
risky, as such ignore 3 consistency checks that are not harmful to
ignore and is battle tested by Gitlab.

bad timezone:
https://gitlab.com/gitlab-org/gitaly/-/commit/692a0d3476a5fe5832ec78df5a6d9d5e1d780364

missing space:
https://gitlab.com/gitlab-org/gitaly/-/commit/2da0b393998d394b743c70e7cf9cd0757a8f2733

non-zero padded filemode:
https://gitlab.com/gitlab-org/gitaly/-/commit/db8f2e8da5e7ff9cf84a99195481303016cd2138

Typically we set these settings in `modules/git/git.go`, but that means
a instance administrator wouldn't be able to override it. Given we don't
strictly require these settings to be set. A instance admin could
choose to disable the consistency checks or override our set of ignores
this would allow them to do so via the `[git.config]` section.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12695
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: elle <0xllx0@noreply.codeberg.org>

modules/setting/git.go

@@ -95,6 +95,17 @@ func loadGitFrom(rootCfg ConfigProvider) {
 	GitConfig.SetOption("core.logAllRefUpdates", "true")
 	GitConfig.SetOption("gc.reflogExpire", "90")
 
+	GitConfig.SetOption("transfer.fsckObjects", "true")
+	// To ignore specific warnings they have to be set for all of the three
+	// scenarios. Per git-config(1): "To uniformly configure the same fsck
+	// settings in different circumstances, all three of them must be set to the
+	// same values."
+	for _, prefix := range []string{"fsck.", "fetch.fsck.", "receive.fsck."} {
+		GitConfig.SetOption(prefix+"badTimezone", "ignore")
+		GitConfig.SetOption(prefix+"missingSpaceBeforeDate", "ignore")
+		GitConfig.SetOption(prefix+"zeroPaddedFilemode", "ignore")
+	}
+
 	for _, key := range secGitConfig.Keys() {
 		GitConfig.SetOption(key.Name(), key.String())
 	}

modules/setting/git_test.go

@@ -32,6 +32,32 @@ diff.algorithm = other
 	require.NoError(t, err)
 	loadGitFrom(cfg)
 	assert.Equal(t, "other", GitConfig.Options["diff.algorithm"])
+
+	t.Run("Fsck options", func(t *testing.T) {
+		cfg, err := NewConfigProviderFromData(`
+[git.config]
+receive.fsckObjects = false
+fetch.fsck.zeroPaddedFilemode = warn
+fsck.missingSpaceBeforeDate = error
+`)
+		require.NoError(t, err)
+		loadGitFrom(cfg)
+		assert.Equal(t, "false", GitConfig.Options["receive.fsckobjects"])
+		assert.Equal(t, "true", GitConfig.Options["transfer.fsckobjects"])
+		assert.NotContains(t, GitConfig.Options, "fetch.fsckobjects")
+
+		assert.Equal(t, "ignore", GitConfig.Options["fsck.badtimezone"])
+		assert.Equal(t, "error", GitConfig.Options["fsck.missingspacebeforedate"])
+		assert.Equal(t, "ignore", GitConfig.Options["fsck.zeropaddedfilemode"])
+
+		assert.Equal(t, "ignore", GitConfig.Options["receive.fsck.badtimezone"])
+		assert.Equal(t, "ignore", GitConfig.Options["receive.fsck.missingspacebeforedate"])
+		assert.Equal(t, "ignore", GitConfig.Options["receive.fsck.zeropaddedfilemode"])
+
+		assert.Equal(t, "ignore", GitConfig.Options["fetch.fsck.badtimezone"])
+		assert.Equal(t, "ignore", GitConfig.Options["fetch.fsck.missingspacebeforedate"])
+		assert.Equal(t, "warn", GitConfig.Options["fetch.fsck.zeropaddedfilemode"])
+	})
 }
 
 func TestGitReflog(t *testing.T) {

routers/web/repo/wiki_test.go

@@ -86,7 +86,7 @@ func TestWiki(t *testing.T) {
 	Wiki(ctx)
 	assert.Equal(t, http.StatusOK, ctx.Resp.Status())
 	assert.EqualValues(t, "Home", ctx.Data["Title"])
-	assertPagesMetas(t, []string{"Home", "Long Page", "Page With Image", "Page With Spaced Name", "Unescaped File", "XSS"}, ctx.Data["Pages"])
+	assertPagesMetas(t, []string{"Home", "Long Page", "Page With Image", "Page With Spaced Name", "Unescaped File"}, ctx.Data["Pages"])
 }
 
 func TestWikiPages(t *testing.T) {
@@ -96,7 +96,7 @@ func TestWikiPages(t *testing.T) {
 	contexttest.LoadRepo(t, ctx, 1)
 	WikiPages(ctx)
 	assert.Equal(t, http.StatusOK, ctx.Resp.Status())
-	assertPagesMetas(t, []string{"Home", "Long Page", "Page With Image", "Page With Spaced Name", "Unescaped File", "XSS"}, ctx.Data["Pages"])
+	assertPagesMetas(t, []string{"Home", "Long Page", "Page With Image", "Page With Spaced Name", "Unescaped File"}, ctx.Data["Pages"])
 }
 
 func TestNewWiki(t *testing.T) {

tests/gitea-repositories-meta/user2/repo1.wiki.git/description

@@ -1 +0,0 @@
-Unnamed repository; edit this file 'description' to name the repository.

tests/gitea-repositories-meta/user2/repo1.wiki.git/info/refs

@@ -0,0 +1 @@
+d49ac742d44063dcf69d4e0afe725813b777dd89	refs/heads/master

tests/gitea-repositories-meta/user2/repo1.wiki.git/objects/0c/f15c3f66ec8384480ed9c3cf87c9e97fbb0ec3

@@ -1,2 +0,0 @@
-x
­ŽŃmÄ DóMŰŔY¬Í�˘(ůJ
©`�5ÇÉś-›K*Ki,Hi!?ŁŃ<éiâVki0Z˙ÔXH“D(Z6ĨGňSb» 3“JDŢhµó!÷uB¬ĚDaJpˇ	íśŮčFôLĆą4+~´ëvŔ;‡ŁČ
-eýäžőç[Nx>KÝäÎü‡_sĺ˛q«/€]09MHpѤµękżÜä_dę-%¸í’‡Űž�ď	vÎ_Ą]ˇÔ^Ő/čI[t

tests/gitea-repositories-meta/user2/repo1.wiki.git/objects/71/911bf48766c7181518c1070911019fbb00b1fc

@@ -1 +0,0 @@
-x
ÆM‚@†á¯MÛ àºré›°6ñœ&&&¬ü9LežÅ›w½Ý×åt<#ÞñÃÍ¡ªmv-·•0w¬b¦¢jyÌ–†¤Ú—~Ý‹[žæÉçý=HÄ÷.¾"à‚íµÄçÇ�=

tests/gitea-repositories-meta/user2/repo1.wiki.git/objects/c1/0d10b7e655b3dab1f53176db57c8219a5488d6

@@ -1,2 +0,0 @@
-x
­ŽÝmÃ0ƒû¬)n�ú±t2íSèçÓÙ`ņ¥¶“e‚,VY¡/Hâ#È[)¹�EûÒ@NÈq¦è툎Ñr2«)DöÅ0âŒj§C®ìÑLÂ�ŸœaCÓÃ&š4B�v]$Eßí²ðIÓ‘e…¯¼þP×r¿I…sÍe“zªË³~_
-åõÄ[yã‡è¢v£�WíµV=í—›ü˘úH	vZ~s»@݉%Á•Š¨?TÊZH

tests/gitea-repositories-meta/user2/repo1.wiki.git/objects/d4/9ac742d44063dcf69d4e0afe725813b777dd89

@@ -1 +0,0 @@
-x
ťŹÍn›@…»ž§¸ű*ń†áGjŞŚmJ±‰C1–“ě~R0ŹÓ§i}’*ďU·•şĘ*gw>éHçËş¶­5�6}§Ą€3”)Sh˛´0—9ÍTƨáž;2i2Óĺ®-É^j§
óLň4wSžŰŞp™mSTČ­‚Ű\9V†Ô1Sép"Źşę¸ŐĽ|ůůëÇ“„ťî.Z>É법us™uíG 63�…Ąđ)"9ÓóG­Ţ¶.÷ĺˇ.áâO¦ž¬ ň#XţJ$›ŘűË	¨?o<1˝_Š)n�/±~ćá6ąëŐcS­űl<ÔXŽiŇ–b3{@ź÷B,g±řĎ	”Ż‘ŰËK|˝ĆEĐ[ß0\ď�ÉŹaWńݢRć|¤ři{şąžxd…ŤĂÉßô
9<ĐŮŇŹšg>»¨ŠKOvÉ"D§J„sEŕęŘśzňĎĆ[Í_s!2ϡév%ěe©Čo(˝”

tests/gitea-repositories-meta/user2/repo1.wiki.git/objects/info/packs

@@ -0,0 +1,2 @@
+P pack-dcd8abdfa327da7644d88ee8f092b02a7bb210d4.pack
+

tests/gitea-repositories-meta/user2/repo1.wiki.git/packed-refs

@@ -0,0 +1,2 @@
+# pack-refs with: peeled fully-peeled sorted 
+d49ac742d44063dcf69d4e0afe725813b777dd89 refs/heads/master

tests/gitea-repositories-meta/user2/repo1.wiki.git/refs/heads/master

@@ -1 +0,0 @@
-f54f5a6b7c4f83b606600e43186165854f189530

tests/integration/api_wiki_test.go

@@ -223,29 +223,6 @@ func TestAPIListWikiPages(t *testing.T) {
 				Message: "add unescaped file\n",
 			},
 		},
-		{
-			Title:   "XSS",
-			HTMLURL: meta[5].HTMLURL,
-			SubURL:  "XSS",
-			LastCommit: &api.WikiCommit{
-				ID: "f54f5a6b7c4f83b606600e43186165854f189530",
-				Author: &api.CommitUser{
-					Identity: api.Identity{
-						Name:  "Gusted<script class=\"evil\">alert('Oh no!');</script>",
-						Email: "valid@example.org",
-					},
-					Date: "2024-01-31T00:00:00Z",
-				},
-				Committer: &api.CommitUser{
-					Identity: api.Identity{
-						Name:  "Gusted<script class=\"evil\">alert('Oh no!');</script>",
-						Email: "valid@example.org",
-					},
-					Date: "2024-01-31T00:00:00Z",
-				},
-				Message: "Yay XSS",
-			},
-		},
 	}
 
 	assert.Equal(t, dummymeta, meta)

tests/integration/git_test.go

@@ -121,6 +121,7 @@ func testGit(t *testing.T, u *url.URL) {
 			t.Run("PushForkRemoteMessages", doTestForkPushMessages(httpContext, dstPath))
 			t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head"))
 			t.Run("InternalReferences", doInternalReferences(&httpContext, dstPath))
+			t.Run("FsckConsistencyChecks", doFsckConsistencyChecks(dstPath))
 			t.Run("BranchProtect", doBranchProtect(&httpContext, dstPath))
 			t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath))
 			t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge"))
@@ -170,6 +171,7 @@ func testGit(t *testing.T, u *url.URL) {
 				t.Run("PushForkRemoteMessages", doTestForkPushMessages(sshContext, dstPath))
 				t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2"))
 				t.Run("InternalReferences", doInternalReferences(&sshContext, dstPath))
+				t.Run("FsckConsistencyChecks", doFsckConsistencyChecks(dstPath))
 				t.Run("BranchProtect", doBranchProtect(&sshContext, dstPath))
 				t.Run("MergeFork", func(t *testing.T) {
 					defer tests.PrintCurrentTest(t)()
@@ -1337,3 +1339,78 @@ func TestCloneAccessTokenResources(t *testing.T) {
 		})
 	})
 }
+
+func doFsckConsistencyChecks(dstPath string) func(t *testing.T) {
+	return func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		tree, _, err := git.NewCommand(git.DefaultContext, "rev-parse", "HEAD^{tree}").RunStdString(&git.RunOpts{Dir: dstPath})
+		require.NoError(t, err)
+		head, _, err := git.NewCommand(git.DefaultContext, "rev-parse", "HEAD^{commit}").RunStdString(&git.RunOpts{Dir: dstPath})
+		require.NoError(t, err)
+
+		// Is checked for and should error.
+		t.Run("badName", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			var buf bytes.Buffer
+			buf.WriteString("tree ")
+			buf.WriteString(tree)
+			buf.WriteString("parent ")
+			buf.WriteString(head)
+			buf.WriteString("author  @>\n")
+			buf.WriteString("committer  @>\n")
+			buf.WriteString("\n")
+
+			commitID, _, err := git.NewCommand(git.DefaultContext, "hash-object", "-t", "commit", "--literally", "--stdin", "-w").RunStdString(&git.RunOpts{Dir: dstPath, Stdin: &buf})
+			require.NoError(t, err)
+			commitID = strings.TrimSpace(commitID)
+
+			_, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(commitID + ":refs/heads/consistency-check").RunStdString(&git.RunOpts{Dir: dstPath})
+			require.Error(t, gitErr)
+			assert.Contains(t, stdErr, "badName: invalid author/committer line - bad name")
+		})
+
+		t.Run("missingSpaceBeforeEmail", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			var buf bytes.Buffer
+			buf.WriteString("tree ")
+			buf.WriteString(tree)
+			buf.WriteString("parent ")
+			buf.WriteString(head)
+			buf.WriteString(`author Gusted<script class="evil">alert('Oh no!');</script> <valid@example.org> 1706659200 +0000`)
+			buf.WriteRune('\n')
+			buf.WriteString(`committer Gusted<script class="evil">alert('Oh no!');</script> <valid@example.org> 1706659200 +0000`)
+			buf.WriteString("\n\n")
+
+			commitID, _, err := git.NewCommand(git.DefaultContext, "hash-object", "-t", "commit", "--literally", "--stdin", "-w").RunStdString(&git.RunOpts{Dir: dstPath, Stdin: &buf})
+			require.NoError(t, err)
+			commitID = strings.TrimSpace(commitID)
+
+			_, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(commitID + ":refs/heads/consistency-check").RunStdString(&git.RunOpts{Dir: dstPath})
+			require.Error(t, gitErr)
+			assert.Contains(t, stdErr, "missingSpaceBeforeEmail: invalid author/committer line - missing space before email")
+		})
+
+		// Is set to be ignored.
+		t.Run("badTimezone", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			var buf bytes.Buffer
+			buf.WriteString("tree ")
+			buf.WriteString(tree)
+			buf.WriteString("parent ")
+			buf.WriteString(head)
+			buf.WriteString("author name <email> 1234 +\n")
+			buf.WriteString("committer name <email> 1234 +\n")
+			buf.WriteString("\n")
+
+			commitID, _, err := git.NewCommand(git.DefaultContext, "hash-object", "-t", "commit", "--literally", "--stdin", "-w").RunStdString(&git.RunOpts{Dir: dstPath, Stdin: &buf})
+			require.NoError(t, err)
+			commitID = strings.TrimSpace(commitID)
+
+			require.NoError(t, git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(commitID+":refs/heads/consistency-check").Run(&git.RunOpts{Dir: dstPath}))
+		})
+	}
+}

tests/integration/xss_test.go

@@ -39,33 +39,6 @@ func TestXSSUserFullName(t *testing.T) {
 	)
 }
 
-func TestXSSWikiLastCommitInfo(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	// Check on page view.
-	t.Run("Page view", func(t *testing.T) {
-		defer tests.PrintCurrentTest(t)()
-
-		req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/XSS")
-		resp := MakeRequest(t, req, http.StatusOK)
-		htmlDoc := NewHTMLParser(t, resp.Body)
-
-		htmlDoc.AssertElement(t, "script.evil", false)
-		assert.Contains(t, htmlDoc.Find(".ui.sub.header").Text(), `Gusted<script class="evil">alert('Oh no!');</script> edited this page 2024-01-31`)
-	})
-
-	// Check on revisions page.
-	t.Run("Revision page", func(t *testing.T) {
-		defer tests.PrintCurrentTest(t)()
-
-		req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/XSS?action=_revision")
-		resp := MakeRequest(t, req, http.StatusOK)
-		htmlDoc := NewHTMLParser(t, resp.Body)
-
-		htmlDoc.AssertElement(t, "script.evil", false)
-		assert.Contains(t, htmlDoc.Find(".ui.sub.header").Text(), `Gusted<script class="evil">alert('Oh no!');</script> edited this page 2024-01-31`)
-	})
-}
-
 func TestXSSReviewDismissed(t *testing.T) {
 	defer unittest.OverrideFixtures("tests/integration/fixtures/TestXSSReviewDismissed")()
 	defer tests.PrepareTestEnv(t)()