Multiple Escaping Improvements (#17551)

There are multiple places where Gitea does not properly escape URLs that it is building and there are multiple places where it builds urls when there is already a simpler function available to use this.
    
This is an extensive PR attempting to fix these issues.

1. The first commit in this PR looks through all href, src and links in the Gitea codebase and has attempted to catch all the places where there is potentially incomplete escaping.
2. Whilst doing this we will prefer to use functions that create URLs over recreating them by hand.
3. All uses of strings should be directly escaped - even if they are not currently expected to contain escaping characters. The main benefit to doing this will be that we can consider relaxing the constraints on user names and reponames in future. 
4. The next commit looks at escaping in the wiki and re-considers the urls that are used there. Using the improved escaping here wiki files containing '/'. (This implementation will currently still place all of the wiki files the root directory of the repo but this would not be difficult to change.)
5. The title generation in feeds is now properly escaped.
6. EscapePound is no longer needed - urls should be PathEscaped / QueryEscaped as necessary but then re-escaped with Escape when creating html with locales Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>

5 files changed, 27 insertions, 20 deletions

diff --git a/services/webhook/dingtalk.go b/services/webhook/dingtalk.go
index 88e4078922..a949b073a5 100644
--- a/services/webhook/dingtalk.go
+++ b/services/webhook/dingtalk.go
@@ -13,6 +13,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/json"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 
 	dingtalk "github.com/lunny/dingtalk_webhook"
 )
@@ -41,7 +42,7 @@ func (d *DingtalkPayload) Create(p *api.CreatePayload) (api.Payloader, error) {
 	refName := git.RefEndName(p.Ref)
 	title := fmt.Sprintf("[%s] %s %s created", p.Repo.FullName, p.RefType, refName)
 
-	return createDingtalkPayload(title, title, fmt.Sprintf("view ref %s", refName), p.Repo.HTMLURL+"/src/"+refName), nil
+	return createDingtalkPayload(title, title, fmt.Sprintf("view ref %s", refName), p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName)), nil
 }
 
 // Delete implements PayloadConvertor Delete method
@@ -50,7 +51,7 @@ func (d *DingtalkPayload) Delete(p *api.DeletePayload) (api.Payloader, error) {
 	refName := git.RefEndName(p.Ref)
 	title := fmt.Sprintf("[%s] %s %s deleted", p.Repo.FullName, p.RefType, refName)
 
-	return createDingtalkPayload(title, title, fmt.Sprintf("view ref %s", refName), p.Repo.HTMLURL+"/src/"+refName), nil
+	return createDingtalkPayload(title, title, fmt.Sprintf("view ref %s", refName), p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName)), nil
 }
 
 // Fork implements PayloadConvertor Fork method
@@ -78,7 +79,7 @@ func (d *DingtalkPayload) Push(p *api.PushPayload) (api.Payloader, error) {
 		linkText = fmt.Sprintf("view commit %s...%s", p.Commits[0].ID[:7], p.Commits[len(p.Commits)-1].ID[:7])
 	}
 	if titleLink == "" {
-		titleLink = p.Repo.HTMLURL + "/src/" + branchName
+		titleLink = p.Repo.HTMLURL + "/src/" + util.PathEscapeSegments(branchName)
 	}
 
 	title := fmt.Sprintf("[%s:%s] %s", p.Repo.FullName, branchName, commitDesc)
diff --git a/services/webhook/discord.go b/services/webhook/discord.go
index 3de50a8a2f..587d2098eb 100644
--- a/services/webhook/discord.go
+++ b/services/webhook/discord.go
@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 )
 
 type (
@@ -115,7 +116,7 @@ func (d *DiscordPayload) Create(p *api.CreatePayload) (api.Payloader, error) {
 	refName := git.RefEndName(p.Ref)
 	title := fmt.Sprintf("[%s] %s %s created", p.Repo.FullName, p.RefType, refName)
 
-	return d.createPayload(p.Sender, title, "", p.Repo.HTMLURL+"/src/"+refName, greenColor), nil
+	return d.createPayload(p.Sender, title, "", p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName), greenColor), nil
 }
 
 // Delete implements PayloadConvertor Delete method
@@ -124,7 +125,7 @@ func (d *DiscordPayload) Delete(p *api.DeletePayload) (api.Payloader, error) {
 	refName := git.RefEndName(p.Ref)
 	title := fmt.Sprintf("[%s] %s %s deleted", p.Repo.FullName, p.RefType, refName)
 
-	return d.createPayload(p.Sender, title, "", p.Repo.HTMLURL+"/src/"+refName, redColor), nil
+	return d.createPayload(p.Sender, title, "", p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName), redColor), nil
 }
 
 // Fork implements PayloadConvertor Fork method
@@ -150,7 +151,7 @@ func (d *DiscordPayload) Push(p *api.PushPayload) (api.Payloader, error) {
 		titleLink = p.CompareURL
 	}
 	if titleLink == "" {
-		titleLink = p.Repo.HTMLURL + "/src/" + branchName
+		titleLink = p.Repo.HTMLURL + "/src/" + util.PathEscapeSegments(branchName)
 	}
 
 	title := fmt.Sprintf("[%s:%s] %s", p.Repo.FullName, branchName, commitDesc)
diff --git a/services/webhook/general.go b/services/webhook/general.go
index 777ae086b5..32a79c0783 100644
--- a/services/webhook/general.go
+++ b/services/webhook/general.go
@@ -7,10 +7,12 @@ package webhook
 import (
 	"fmt"
 	"html"
+	"net/url"
 	"strings"
 
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 )
 
 type linkFormatter = func(string, string) string
@@ -22,7 +24,7 @@ func noneLinkFormatter(url string, text string) string {
 
 // htmlLinkFormatter creates a HTML link
 func htmlLinkFormatter(url string, text string) string {
-	return fmt.Sprintf(`<a href="%s">%s</a>`, url, html.EscapeString(text))
+	return fmt.Sprintf(`<a href="%s">%s</a>`, html.EscapeString(url), html.EscapeString(text))
 }
 
 func getIssuesPayloadInfo(p *api.IssuePayload, linkFormatter linkFormatter, withSender bool) (string, string, string, int) {
@@ -46,7 +48,7 @@ func getIssuesPayloadInfo(p *api.IssuePayload, linkFormatter linkFormatter, with
 	case api.HookIssueAssigned:
 		list := make([]string, len(p.Issue.Assignees))
 		for i, user := range p.Issue.Assignees {
-			list[i] = linkFormatter(setting.AppURL+user.UserName, user.UserName)
+			list[i] = linkFormatter(setting.AppURL+url.PathEscape(user.UserName), user.UserName)
 		}
 		text = fmt.Sprintf("[%s] Issue assigned to %s: %s", repoLink, strings.Join(list, ", "), titleLink)
 		color = greenColor
@@ -66,7 +68,7 @@ func getIssuesPayloadInfo(p *api.IssuePayload, linkFormatter linkFormatter, with
 		text = fmt.Sprintf("[%s] Issue milestone cleared: %s", repoLink, titleLink)
 	}
 	if withSender {
-		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+p.Sender.UserName, p.Sender.UserName))
+		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+url.PathEscape(p.Sender.UserName), p.Sender.UserName))
 	}
 
 	var attachmentText string
@@ -139,7 +141,7 @@ func getPullRequestPayloadInfo(p *api.PullRequestPayload, linkFormatter linkForm
 
 func getReleasePayloadInfo(p *api.ReleasePayload, linkFormatter linkFormatter, withSender bool) (text string, color int) {
 	repoLink := linkFormatter(p.Repository.HTMLURL, p.Repository.FullName)
-	refLink := linkFormatter(p.Repository.HTMLURL+"/src/"+p.Release.TagName, p.Release.TagName)
+	refLink := linkFormatter(p.Repository.HTMLURL+"/src/"+util.PathEscapeSegments(p.Release.TagName), p.Release.TagName)
 
 	switch p.Action {
 	case api.HookReleasePublished:
@@ -153,7 +155,7 @@ func getReleasePayloadInfo(p *api.ReleasePayload, linkFormatter linkFormatter, w
 		color = redColor
 	}
 	if withSender {
-		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+p.Sender.UserName, p.Sender.UserName))
+		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+url.PathEscape(p.Sender.UserName), p.Sender.UserName))
 	}
 
 	return text, color
@@ -189,7 +191,7 @@ func getIssueCommentPayloadInfo(p *api.IssueCommentPayload, linkFormatter linkFo
 		color = redColor
 	}
 	if withSender {
-		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+p.Sender.UserName, p.Sender.UserName))
+		text += fmt.Sprintf(" by %s", linkFormatter(setting.AppURL+url.PathEscape(p.Sender.UserName), p.Sender.UserName))
 	}
 
 	return text, issueTitle, color
diff --git a/services/webhook/matrix.go b/services/webhook/matrix.go
index 08adaef6fd..4fd78ff5bb 100644
--- a/services/webhook/matrix.go
+++ b/services/webhook/matrix.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"html"
 	"net/http"
+	"net/url"
 	"regexp"
 	"strings"
 
@@ -19,6 +20,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 )
 
 const matrixPayloadSizeLimit = 1024 * 64
@@ -94,11 +96,11 @@ func MatrixLinkToRef(repoURL, ref string) string {
 	refName := git.RefEndName(ref)
 	switch {
 	case strings.HasPrefix(ref, git.BranchPrefix):
-		return MatrixLinkFormatter(repoURL+"/src/branch/"+refName, refName)
+		return MatrixLinkFormatter(repoURL+"/src/branch/"+util.PathEscapeSegments(refName), refName)
 	case strings.HasPrefix(ref, git.TagPrefix):
-		return MatrixLinkFormatter(repoURL+"/src/tag/"+refName, refName)
+		return MatrixLinkFormatter(repoURL+"/src/tag/"+util.PathEscapeSegments(refName), refName)
 	default:
-		return MatrixLinkFormatter(repoURL+"/src/commit/"+refName, refName)
+		return MatrixLinkFormatter(repoURL+"/src/commit/"+util.PathEscapeSegments(refName), refName)
 	}
 }
 
@@ -186,7 +188,7 @@ func (m *MatrixPayloadUnsafe) PullRequest(p *api.PullRequestPayload) (api.Payloa
 
 // Review implements PayloadConvertor Review method
 func (m *MatrixPayloadUnsafe) Review(p *api.PullRequestPayload, event webhook_model.HookEventType) (api.Payloader, error) {
-	senderLink := MatrixLinkFormatter(setting.AppURL+p.Sender.UserName, p.Sender.UserName)
+	senderLink := MatrixLinkFormatter(setting.AppURL+url.PathEscape(p.Sender.UserName), p.Sender.UserName)
 	title := fmt.Sprintf("#%d %s", p.Index, p.PullRequest.Title)
 	titleLink := fmt.Sprintf("%s/pulls/%d", p.Repository.HTMLURL, p.Index)
 	repoLink := MatrixLinkFormatter(p.Repository.HTMLURL, p.Repository.FullName)
@@ -281,7 +283,7 @@ func getMatrixHookRequest(w *webhook_model.Webhook, t *webhook_model.HookTask) (
 		return nil, fmt.Errorf("getMatrixHookRequest: unable to hash payload: %+v", err)
 	}
 
-	url := fmt.Sprintf("%s/%s", w.URL, txnID)
+	url := fmt.Sprintf("%s/%s", w.URL, url.PathEscape(txnID))
 
 	req, err := http.NewRequest(w.HTTPMethod, url, strings.NewReader(string(payload)))
 	if err != nil {
diff --git a/services/webhook/msteams.go b/services/webhook/msteams.go
index 2b88bb23ff..ae5af8d9b6 100644
--- a/services/webhook/msteams.go
+++ b/services/webhook/msteams.go
@@ -12,6 +12,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/json"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 )
 
 type (
@@ -79,7 +80,7 @@ func (m *MSTeamsPayload) Create(p *api.CreatePayload) (api.Payloader, error) {
 		p.Sender,
 		title,
 		"",
-		p.Repo.HTMLURL+"/src/"+refName,
+		p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName),
 		greenColor,
 		&MSTeamsFact{fmt.Sprintf("%s:", p.RefType), refName},
 	), nil
@@ -96,7 +97,7 @@ func (m *MSTeamsPayload) Delete(p *api.DeletePayload) (api.Payloader, error) {
 		p.Sender,
 		title,
 		"",
-		p.Repo.HTMLURL+"/src/"+refName,
+		p.Repo.HTMLURL+"/src/"+util.PathEscapeSegments(refName),
 		yellowColor,
 		&MSTeamsFact{fmt.Sprintf("%s:", p.RefType), refName},
 	), nil
@@ -133,7 +134,7 @@ func (m *MSTeamsPayload) Push(p *api.PushPayload) (api.Payloader, error) {
 		titleLink = p.CompareURL
 	}
 	if titleLink == "" {
-		titleLink = p.Repo.HTMLURL + "/src/" + branchName
+		titleLink = p.Repo.HTMLURL + "/src/" + util.PathEscapeSegments(branchName)
 	}
 
 	title := fmt.Sprintf("[%s:%s] %s", p.Repo.FullName, branchName, commitDesc)