From 565e3312385d533f96c359979a3ae7cc14eba671 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 17 Jan 2024 16:16:46 +0100 Subject: [PATCH 1/2] [SECURITY] Test XSS in wiki last commit information On the wiki and revisions page, information is shown about the last commit that modified that wiki page. This includes the time it was last edited and by whom. Verify it is sanitized. --- tests/integration/xss_test.go | 75 +++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/integration/xss_test.go b/tests/integration/xss_test.go index e575ed3990..42ce35150c 100644 --- a/tests/integration/xss_test.go +++ b/tests/integration/xss_test.go @@ -4,14 +4,24 @@ package integration import ( + "context" + "fmt" "net/http" + "net/url" + "os" + "path/filepath" "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" + "github.com/stretchr/testify/require" ) func TestXSSUserFullName(t *testing.T) { @@ -37,3 +47,68 @@ 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) + require.NoError(t, err) + + w, err := gitRepo.Worktree() + require.NoError(t, err) + + filename := filepath.Join(dstPath, "Home.md") + err = os.WriteFile(filename, []byte("Oh, a XSS attack?"), 0o644) + require.NoError(t, err) + + _, err = w.Add("Home.md") + require.NoError(t, err) + + _, err = w.Commit("Yay XSS", &gogit.CommitOptions{ + Author: &object.Signature{ + Name: `Gusted`, + Email: "valid@example.org", + When: time.Date(2024, time.January, 31, 0, 0, 0, 0, time.UTC), + }, + }) + require.NoError(t, err) + + // Push. + _, _, err = git.NewCommand(git.DefaultContext, "push").AddArguments(git.ToTrustedCmdArgs([]string{"origin", "master"})...).RunStdString(&git.RunOpts{Dir: dstPath}) + require.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.Contains(t, htmlDoc.Find(".ui.sub.header").Text(), `Gusted 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/Home?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 edited this page 2024-01-31`) + }) + }) +} From ca798e4cc2a8c6e3d1f2cfed01f47d8b3da9361f Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 18 Jan 2024 00:18:39 +0100 Subject: [PATCH 2/2] [SECURITY] Test XSS in dismissed review It's possible for reviews to not be assiocated with users, when they were migrated from another forge instance. In the migration code, there's no sanitization check for author names, so they could contain HTML tags and thus needs to be properely escaped. --- templates/repo/issue/view_content/comments.tmpl | 2 +- .../fixtures/TestXSSReviewDismissed/comment.yml | 9 +++++++++ .../fixtures/TestXSSReviewDismissed/review.yml | 8 ++++++++ tests/integration/xss_test.go | 15 +++++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 tests/integration/fixtures/TestXSSReviewDismissed/comment.yml create mode 100644 tests/integration/fixtures/TestXSSReviewDismissed/review.yml diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 9e50ee4d94..a4fd97297f 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -619,7 +619,7 @@ {{else}} {{$reviewerName = .Review.OriginalAuthor}} {{end}} - {{ctx.Locale.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}} + {{ctx.Locale.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}} {{if .Content}} diff --git a/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml b/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml new file mode 100644 index 0000000000..50162a4e7e --- /dev/null +++ b/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml @@ -0,0 +1,9 @@ +- + id: 1000 + type: 32 # dismiss review + poster_id: 2 + issue_id: 2 # in repo_id 1 + content: "XSS time!" + review_id: 1000 + created_unix: 1700000000 + updated_unix: 1700000000 diff --git a/tests/integration/fixtures/TestXSSReviewDismissed/review.yml b/tests/integration/fixtures/TestXSSReviewDismissed/review.yml new file mode 100644 index 0000000000..56bc08d35f --- /dev/null +++ b/tests/integration/fixtures/TestXSSReviewDismissed/review.yml @@ -0,0 +1,8 @@ +- + id: 1000 + type: 1 + issue_id: 2 + original_author: "Otto " + content: "XSS time!" + updated_unix: 1700000000 + created_unix: 1700000000 diff --git a/tests/integration/xss_test.go b/tests/integration/xss_test.go index 42ce35150c..acd716c7c7 100644 --- a/tests/integration/xss_test.go +++ b/tests/integration/xss_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" @@ -112,3 +113,17 @@ func TestXSSWikiLastCommitInfo(t *testing.T) { }) }) } + +func TestXSSReviewDismissed(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestXSSReviewDismissed/")() + defer tests.PrepareTestEnv(t)() + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1000}) + + req := NewRequest(t, http.MethodGet, fmt.Sprintf("/user2/repo1/pulls/%d", +review.IssueID)) + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, "script.evil", false) + assert.Contains(t, htmlDoc.Find("#issuecomment-1000 .dismissed-message").Text(), `dismissed Otto ’s review`) +}