Fix markdown URL parsing (#17924)

Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: zeripath <[email protected]>

Author: 3 people

modules/markup/html.go

@@ -41,16 +41,21 @@ var (
 	// While fast, this is also incorrect and lead to false positives.
 	// TODO: fix invalid linking issue
 
+	// valid chars in encoded path and parameter: [-+~_%.a-zA-Z0-9/]
+
 	// sha1CurrentPattern matches string that represents a commit SHA, e.g. d8a994ef243349f321568f9e36d5c3f444b99cae
 	// Although SHA1 hashes are 40 chars long, the regex matches the hash from 7 to 40 chars in length
-	// so that abbreviated hash links can be used as well. This matches git and github useability.
+	// so that abbreviated hash links can be used as well. This matches git and GitHub usability.
 	sha1CurrentPattern = regexp.MustCompile(`(?:\s|^|\(|\[)([0-9a-f]{7,40})(?:\s|$|\)|\]|[.,](\s|$))`)
 
 	// shortLinkPattern matches short but difficult to parse [[name|link|arg=test]] syntax
 	shortLinkPattern = regexp.MustCompile(`\[\[(.*?)\]\](\w*)`)
 
-	// anySHA1Pattern allows to split url containing SHA into parts
-	anySHA1Pattern = regexp.MustCompile(`https?://(?:\S+/){4}([0-9a-f]{40})(/[^#\s]+)?(#\S+)?`)
+	// anySHA1Pattern splits url containing SHA into parts
+	anySHA1Pattern = regexp.MustCompile(`https?://(?:\S+/){4,5}([0-9a-f]{40})(/[-+~_%.a-zA-Z0-9/]+)?(#[-+~_%.a-zA-Z0-9]+)?`)
+
+	// comparePattern matches "http://domain/org/repo/compare/COMMIT1...COMMIT2#hash"
+	comparePattern = regexp.MustCompile(`https?://(?:\S+/){4,5}([0-9a-f]{40})(\.\.\.?)([0-9a-f]{40})?(#[-+~_%.a-zA-Z0-9]+)?`)
 
 	validLinksPattern = regexp.MustCompile(`^[a-z][\w-]+://`)
 
@@ -65,7 +70,7 @@ var (
 	blackfridayExtRegex = regexp.MustCompile(`[^:]*:user-content-`)
 
 	// EmojiShortCodeRegex find emoji by alias like :smile:
-	EmojiShortCodeRegex = regexp.MustCompile(`:[\w\+\-]+:`)
+	EmojiShortCodeRegex = regexp.MustCompile(`:[-+\w]+:`)
 )
 
 // CSS class for action keywords (e.g. "closes: #1")
@@ -152,6 +157,7 @@ type processor func(ctx *RenderContext, node *html.Node)
 
 var defaultProcessors = []processor{
 	fullIssuePatternProcessor,
+	comparePatternProcessor,
 	fullSha1PatternProcessor,
 	shortLinkProcessor,
 	linkProcessor,
@@ -178,6 +184,7 @@ func PostProcess(
 
 var commitMessageProcessors = []processor{
 	fullIssuePatternProcessor,
+	comparePatternProcessor,
 	fullSha1PatternProcessor,
 	linkProcessor,
 	mentionProcessor,
@@ -208,6 +215,7 @@ func RenderCommitMessage(
 
 var commitMessageSubjectProcessors = []processor{
 	fullIssuePatternProcessor,
+	comparePatternProcessor,
 	fullSha1PatternProcessor,
 	linkProcessor,
 	mentionProcessor,
@@ -920,12 +928,57 @@ func fullSha1PatternProcessor(ctx *RenderContext, node *html.Node) {
 		if hash != "" {
 			text += " (" + hash + ")"
 		}
-
 		replaceContent(node, start, end, createCodeLink(urlFull, text, "commit"))
 		node = node.NextSibling.NextSibling
 	}
 }
 
+func comparePatternProcessor(ctx *RenderContext, node *html.Node) {
+	if ctx.Metas == nil {
+		return
+	}
+
+	next := node.NextSibling
+	for node != nil && node != next {
+		m := comparePattern.FindStringSubmatchIndex(node.Data)
+		if m == nil {
+			return
+		}
+
+		urlFull := node.Data[m[0]:m[1]]
+		text1 := base.ShortSha(node.Data[m[2]:m[3]])
+		textDots := base.ShortSha(node.Data[m[4]:m[5]])
+		text2 := base.ShortSha(node.Data[m[6]:m[7]])
+
+		hash := ""
+		if m[9] > 0 {
+			hash = node.Data[m[8]:m[9]][1:]
+		}
+
+		start := m[0]
+		end := m[1]
+
+		// If url ends in '.', it's very likely that it is not part of the
+		// actual url but used to finish a sentence.
+		if strings.HasSuffix(urlFull, ".") {
+			end--
+			urlFull = urlFull[:len(urlFull)-1]
+			if hash != "" {
+				hash = hash[:len(hash)-1]
+			} else if text2 != "" {
+				text2 = text2[:len(text2)-1]
+			}
+		}
+
+		text := text1 + textDots + text2
+		if hash != "" {
+			text += " (" + hash + ")"
+		}
+		replaceContent(node, start, end, createCodeLink(urlFull, text, "compare"))
+		node = node.NextSibling.NextSibling
+	}
+}
+
 // emojiShortCodeProcessor for rendering text like :smile: into emoji
 func emojiShortCodeProcessor(ctx *RenderContext, node *html.Node) {
 	start := 0
@@ -1038,8 +1091,8 @@ func sha1CurrentPatternProcessor(ctx *RenderContext, node *html.Node) {
 			continue
 		}
 
-		replaceContent(node, m[2], m[3],
-			createCodeLink(util.URLJoin(setting.AppURL, ctx.Metas["user"], ctx.Metas["repo"], "commit", hash), base.ShortSha(hash), "commit"))
+		link := util.URLJoin(setting.AppURL, ctx.Metas["user"], ctx.Metas["repo"], "commit", hash)
+		replaceContent(node, m[2], m[3], createCodeLink(link, base.ShortSha(hash), "commit"))
 		start = 0
 		node = node.NextSibling.NextSibling
 	}

modules/markup/html_internal_test.go

@@ -15,9 +15,9 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-const AppURL = "http://localhost:3000/"
-const Repo = "gogits/gogs"
-const AppSubURL = AppURL + Repo + "/"
+const TestAppURL = "http://localhost:3000/"
+const TestOrgRepo = "gogits/gogs"
+const TestRepoURL = TestAppURL + TestOrgRepo + "/"
 
 // alphanumLink an HTML link to an alphanumeric-style issue
 func alphanumIssueLink(baseURL, class, name string) string {
@@ -52,7 +52,7 @@ var alphanumericMetas = map[string]string{
 	"style":  IssueNameStyleAlphanumeric,
 }
 
-// these values should match the Repo const above
+// these values should match the TestOrgRepo const above
 var localMetas = map[string]string{
 	"user": "gogits",
 	"repo": "gogs",
@@ -90,8 +90,7 @@ func TestRender_IssueIndexPattern(t *testing.T) {
 }
 
 func TestRender_IssueIndexPattern2(t *testing.T) {
-	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
+	setting.AppURL = TestAppURL
 
 	// numeric: render inputs with valid mentions
 	test := func(s, expectedFmt, marker string, indices ...int) {
@@ -108,7 +107,7 @@ func TestRender_IssueIndexPattern2(t *testing.T) {
 
 		links := make([]interface{}, len(indices))
 		for i, index := range indices {
-			links[i] = numericIssueLink(util.URLJoin(setting.AppSubURL, path), "ref-issue", index, marker)
+			links[i] = numericIssueLink(util.URLJoin(TestRepoURL, path), "ref-issue", index, marker)
 		}
 		expectedNil := fmt.Sprintf(expectedFmt, links...)
 		testRenderIssueIndexPattern(t, s, expectedNil, &RenderContext{Metas: localMetas})
@@ -152,8 +151,7 @@ func TestRender_IssueIndexPattern2(t *testing.T) {
 }
 
 func TestRender_IssueIndexPattern3(t *testing.T) {
-	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
+	setting.AppURL = TestAppURL
 
 	// alphanumeric: render inputs without valid mentions
 	test := func(s string) {
@@ -178,8 +176,7 @@ func TestRender_IssueIndexPattern3(t *testing.T) {
 }
 
 func TestRender_IssueIndexPattern4(t *testing.T) {
-	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
+	setting.AppURL = TestAppURL
 
 	// alphanumeric: render inputs with valid mentions
 	test := func(s, expectedFmt string, names ...string) {
@@ -197,7 +194,7 @@ func TestRender_IssueIndexPattern4(t *testing.T) {
 
 func testRenderIssueIndexPattern(t *testing.T, input, expected string, ctx *RenderContext) {
 	if ctx.URLPrefix == "" {
-		ctx.URLPrefix = AppSubURL
+		ctx.URLPrefix = TestAppURL
 	}
 
 	var buf strings.Builder
@@ -207,21 +204,20 @@ func testRenderIssueIndexPattern(t *testing.T, input, expected string, ctx *Rend
 }
 
 func TestRender_AutoLink(t *testing.T) {
-	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
+	setting.AppURL = TestAppURL
 
 	test := func(input, expected string) {
 		var buffer strings.Builder
 		err := PostProcess(&RenderContext{
-			URLPrefix: setting.AppSubURL,
+			URLPrefix: TestRepoURL,
 			Metas:     localMetas,
 		}, strings.NewReader(input), &buffer)
 		assert.Equal(t, err, nil)
 		assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(buffer.String()))
 
 		buffer.Reset()
 		err = PostProcess(&RenderContext{
-			URLPrefix: setting.AppSubURL,
+			URLPrefix: TestRepoURL,
 			Metas:     localMetas,
 			IsWiki:    true,
 		}, strings.NewReader(input), &buffer)
@@ -230,11 +226,11 @@ func TestRender_AutoLink(t *testing.T) {
 	}
 
 	// render valid issue URLs
-	test(util.URLJoin(setting.AppSubURL, "issues", "3333"),
-		numericIssueLink(util.URLJoin(setting.AppSubURL, "issues"), "ref-issue", 3333, "#"))
+	test(util.URLJoin(TestRepoURL, "issues", "3333"),
+		numericIssueLink(util.URLJoin(TestRepoURL, "issues"), "ref-issue", 3333, "#"))
 
 	// render valid commit URLs
-	tmp := util.URLJoin(AppSubURL, "commit", "d8a994ef243349f321568f9e36d5c3f444b99cae")
+	tmp := util.URLJoin(TestRepoURL, "commit", "d8a994ef243349f321568f9e36d5c3f444b99cae")
 	test(tmp, "<a href=\""+tmp+"\" class=\"commit\"><code class=\"nohighlight\">d8a994ef24</code></a>")
 	tmp += "#diff-2"
 	test(tmp, "<a href=\""+tmp+"\" class=\"commit\"><code class=\"nohighlight\">d8a994ef24 (diff-2)</code></a>")
@@ -245,13 +241,12 @@ func TestRender_AutoLink(t *testing.T) {
 }
 
 func TestRender_FullIssueURLs(t *testing.T) {
-	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
+	setting.AppURL = TestAppURL
 
 	test := func(input, expected string) {
 		var result strings.Builder
 		err := postProcess(&RenderContext{
-			URLPrefix: AppSubURL,
+			URLPrefix: TestRepoURL,
 			Metas:     localMetas,
 		}, []processor{fullIssuePatternProcessor}, strings.NewReader(input), &result)
 		assert.NoError(t, err)