Fix XSS vulnerabilities (#29336)

- The Wiki page did not sanitize author name
- the reviewer name on a "dismiss review" comment is also affected
- the migration page has some spots

---------

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-authored-by: jolheiser <john.olheiser@gmail.com>

diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl
index 4c1b428aaa1fde501b159f114d7e540de7d6a937..4433d86fcc52dbdce307d8582bf5319304c073bb 100644 (file)
--- a/templates/repo/issue/view_content/comments.tmpl
+++ b/templates/repo/issue/view_content/comments.tmpl
@@ -615,7 +615,7 @@
                                                {{else}}
                                                        {{$reviewerName = .Review.OriginalAuthor}}
                                                {{end}}
-                                               {{ctx.Locale.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}}
+                                               <span class="dismissed-message">{{ctx.Locale.Tr "repo.issues.review.dismissed" ($reviewerName | Escape) $createdStr | Safe}}</span>
                                        </span>
                                </div>
                                {{if .Content}}

diff --git a/templates/repo/migrate/migrating.tmpl b/templates/repo/migrate/migrating.tmpl
index 48411e2da233f93c515beda0185cfabb6e9f4c11..7871cd71292ce970f50f1b3d6e4519394ba2c994 100644 (file)
--- a/templates/repo/migrate/migrating.tmpl
+++ b/templates/repo/migrate/migrating.tmpl
@@ -21,12 +21,12 @@
                                        <div class="ui stackable middle very relaxed page grid">
                                                <div class="sixteen wide center aligned centered column">
                                                        <div id="repo_migrating_progress">
-                                                               <p>{{ctx.Locale.Tr "repo.migrate.migrating" .CloneAddr | Safe}}</p>
+                                                               <p>{{ctx.Locale.Tr "repo.migrate.migrating" (.CloneAddr | Escape) | Safe}}</p>
                                                                <p id="repo_migrating_progress_message"></p>
                                                        </div>
                                                        <div id="repo_migrating_failed" class="gt-hidden">
                                                                {{if .CloneAddr}}
-                                                                       <p>{{ctx.Locale.Tr "repo.migrate.migrating_failed" .CloneAddr | Safe}}</p>
+                                                                       <p>{{ctx.Locale.Tr "repo.migrate.migrating_failed" (.CloneAddr | Escape) | Safe}}</p>
                                                                {{else}}
                                                                        <p>{{ctx.Locale.Tr "repo.migrate.migrating_failed_no_addr" | Safe}}</p>
                                                                {{end}}
@@ -58,7 +58,7 @@
        <div class="content">
                <div class="ui warning message">
                        {{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}<br>
-                       {{ctx.Locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}}
+                       {{ctx.Locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}}
                        {{if .Repository.NumForks}}<br>
                        {{ctx.Locale.Tr "repo.settings.delete_notices_fork_1"}}
                        {{end}}

diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl
index 8456bb409b358fff67f8597aefdeea0b482035ed..6e7bd4ce4d82306142f6b29f71f8c9d0e858b90c 100644 (file)
--- a/templates/repo/settings/options.tmpl
+++ b/templates/repo/settings/options.tmpl
@@ -906,7 +906,7 @@
                <div class="content">
                        <div class="ui warning message">
                                {{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}<br>
-                               {{ctx.Locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}}
+                               {{ctx.Locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}}
                                {{if .Repository.NumForks}}<br>
                                {{ctx.Locale.Tr "repo.settings.delete_notices_fork_1"}}
                                {{end}}
@@ -941,7 +941,7 @@
                <div class="content">
                        <div class="ui warning message">
                                {{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}<br>
-                               {{ctx.Locale.Tr "repo.settings.wiki_delete_notices_1" .Repository.Name | Safe}}
+                               {{ctx.Locale.Tr "repo.settings.wiki_delete_notices_1" (.Repository.Name | Escape) | Safe}}
                        </div>
                        <form class="ui form" action="{{.Link}}" method="post">
                                {{.CsrfTokenHtml}}

diff --git a/templates/repo/wiki/revision.tmpl b/templates/repo/wiki/revision.tmpl
index 95b3cd0920333f98bacdce3ce662dbbd19e24b3c..afd9c94c3140161d286d45d13b95c686b0d8acfd 100644 (file)
--- a/templates/repo/wiki/revision.tmpl
+++ b/templates/repo/wiki/revision.tmpl
@@ -10,7 +10,7 @@
                                        {{$title}}
                                        <div class="ui sub header gt-word-break">
                                                {{$timeSince := TimeSince .Author.When ctx.Locale}}
-                                               {{ctx.Locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}}
+                                               {{ctx.Locale.Tr "repo.wiki.last_commit_info" (.Author.Name | Escape) $timeSince | Safe}}
                                        </div>
                                </div>
                        </div>

diff --git a/templates/repo/wiki/view.tmpl b/templates/repo/wiki/view.tmpl
index 039ff3f179ce65d4a886b5c2eea4a1b018f9ef6e..c6e47ee63e3d430dbc74f73451f83bc7bd92e56d 100644 (file)
--- a/templates/repo/wiki/view.tmpl
+++ b/templates/repo/wiki/view.tmpl
@@ -40,7 +40,7 @@
                                        {{$title}}
                                        <div class="ui sub header">
                                                {{$timeSince := TimeSince .Author.When ctx.Locale}}
-                                               {{ctx.Locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}}
+                                               {{ctx.Locale.Tr "repo.wiki.last_commit_info" (.Author.Name | Escape) $timeSince | Safe}}
                                        </div>
                                </div>
                                <div class="eight wide right aligned column">

diff --git a/tests/integration/xss_test.go b/tests/integration/xss_test.go
index e575ed3990cc3f73abd9440881c54aa549eea113..9ed4d35605444c8a177213fb9f4ee00fff5d94dc 100644 (file)
--- a/tests/integration/xss_test.go
+++ b/tests/integration/xss_test.go
@@ -4,13 +4,23 @@
 package integration
 
 import (
+       "context"
+       "fmt"
        "net/http"
+       "net/url"
+       "os"
+       "path/filepath"
+       "strings"
        "testing"
+       "time"
 
        "code.gitea.io/gitea/models/unittest"
        user_model "code.gitea.io/gitea/models/user"
+       "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/tests"
 
+       gogit "github.com/go-git/go-git/v5"
+       "github.com/go-git/go-git/v5/plumbing/object"
        "github.com/stretchr/testify/assert"
 )
 
@@ -37,3 +47,78 @@ func TestXSSUserFullName(t *testing.T) {
                htmlDoc.doc.Find("div.content").Find(".header.text.center").Text(),
        )
 }
+
+func TestXSSWikiLastCommitInfo(t *testing.T) {
+       onGiteaRun(t, func(t *testing.T, u *url.URL) {
+               // Prepare the environment.
+               dstPath := t.TempDir()
+               r := fmt.Sprintf("%suser2/repo1.wiki.git", u.String())
+               u, err := url.Parse(r)
+               assert.NoError(t, err)
+               u.User = url.UserPassword("user2", userPassword)
+               assert.NoError(t, git.CloneWithArgs(context.Background(), git.AllowLFSFiltersArgs(), u.String(), dstPath, git.CloneRepoOptions{}))
+
+               // Use go-git here, because using git wouldn't work, it has code to remove
+               // `<`, `>` and `\n` in user names. Even though this is permitted and
+               // wouldn't result in a error by a Git server.
+               gitRepo, err := gogit.PlainOpen(dstPath)
+               if err != nil {
+                       panic(err)
+               }
+
+               w, err := gitRepo.Worktree()
+               if err != nil {
+                       panic(err)
+               }
+
+               filename := filepath.Join(dstPath, "Home.md")
+               err = os.WriteFile(filename, []byte("Oh, a XSS attack?"), 0o644)
+               if !assert.NoError(t, err) {
+                       t.FailNow()
+               }
+
+               _, err = w.Add("Home.md")
+               if !assert.NoError(t, err) {
+                       t.FailNow()
+               }
+
+               _, err = w.Commit("Yay XSS", &gogit.CommitOptions{
+                       Author: &object.Signature{
+                               Name:  `Gusted <script class="evil">alert('Oh no!');</script>`,
+                               Email: "valid@example.org",
+                               When:  time.Date(2024, time.January, 31, 0, 0, 0, 0, time.UTC),
+                       },
+               })
+               if !assert.NoError(t, err) {
+                       t.FailNow()
+               }
+
+               // Push.
+               _, _, err = git.NewCommand(git.DefaultContext, "push").AddArguments(git.ToTrustedCmdArgs([]string{"origin", "master"})...).RunStdString(&git.RunOpts{Dir: dstPath})
+               assert.NoError(t, err)
+
+               // Check on page view.
+               t.Run("Page view", func(t *testing.T) {
+                       defer tests.PrintCurrentTest(t)()
+
+                       req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/Home")
+                       resp := MakeRequest(t, req, http.StatusOK)
+                       htmlDoc := NewHTMLParser(t, resp.Body)
+
+                       htmlDoc.AssertElement(t, "script.evil", false)
+                       assert.EqualValues(t, `Gusted edited this page 0001-01-01 00:00:00 +00:00`, strings.TrimSpace(htmlDoc.Find(".ui.sub.header").Text()))
+               })
+
+               // Check on revisions page.
+               t.Run("Revision page", func(t *testing.T) {
+                       defer tests.PrintCurrentTest(t)()
+
+                       req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/Home?action=_revision")
+                       resp := MakeRequest(t, req, http.StatusOK)
+                       htmlDoc := NewHTMLParser(t, resp.Body)
+
+                       htmlDoc.AssertElement(t, "script.evil", false)
+                       assert.EqualValues(t, `Gusted edited this page 0001-01-01 00:00:00 +00:00`, strings.TrimSpace(htmlDoc.Find(".ui.sub.header").Text()))
+               })
+       })
+}