From 89845f67283f5c767daa86aa0dd970d92cbdf717 Mon Sep 17 00:00:00 2001 From: Roy Marples Date: Wed, 12 Jul 2017 03:26:54 +0100 Subject: [PATCH] Fix SHA1 hash linking (#2143) This changes the regex to look for a hash from 7 to 40 characters, to match the use of abbreviated hash lookups in both git and github. The restriction of not being a pure number is also removed because 1234567 is now considered a valid abbreviated hash, as is deadbeef. A note has been added to the top of the code to state that the literal regex match is fine, but no extra validation is currently performed so some false positives are expected. A future change could ensure that the hash exists in the repository before rendering it as a link, although this might incur a slight performance penalty. Reverts part of commit 4a46613 and fixes #2053. --- modules/markdown/markdown.go | 19 +++++++++++++------ modules/markdown/markdown_test.go | 2 ++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/modules/markdown/markdown.go b/modules/markdown/markdown.go index 9773e8c2f..bebf454ff 100644 --- a/modules/markdown/markdown.go +++ b/modules/markdown/markdown.go @@ -43,6 +43,10 @@ func IsMarkdownFile(name string) bool { } var ( + // NOTE: All below regex matching do not perform any extra validation. + // Thus a link is produced even if the user does not exist, the issue does not exist, the commit does not exist, etc. + // While fast, this is also incorrect and lead to false positives. + // MentionPattern matches string that mentions someone, e.g. @Unknwon MentionPattern = regexp.MustCompile(`(\s|^|\W)@[0-9a-zA-Z-_\.]+`) @@ -55,9 +59,9 @@ var ( CrossReferenceIssueNumericPattern = regexp.MustCompile(`( |^)[0-9a-zA-Z]+/[0-9a-zA-Z]+#[0-9]+\b`) // Sha1CurrentPattern matches string that represents a commit SHA, e.g. d8a994ef243349f321568f9e36d5c3f444b99cae - // FIXME: this pattern matches pure numbers as well, right now we do a hack to check in renderSha1CurrentPattern - // by converting string to a number. - Sha1CurrentPattern = regexp.MustCompile(`(?:^|\s|\()([0-9a-f]{40})\b`) + // 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. + Sha1CurrentPattern = regexp.MustCompile(`(?:^|\s|\()([0-9a-f]{7,40})\b`) // ShortLinkPattern matches short but difficult to parse [[name|link|arg=test]] syntax ShortLinkPattern = regexp.MustCompile(`(\[\[.*\]\]\w*)`) @@ -525,9 +529,12 @@ func renderSha1CurrentPattern(rawBytes []byte, urlPrefix string) []byte { ms := Sha1CurrentPattern.FindAllSubmatch(rawBytes, -1) for _, m := range ms { hash := m[1] - if com.StrTo(hash).MustInt() > 0 { - continue - } + // The regex does not lie, it matches the hash pattern. + // However, a regex cannot know if a hash actually exists or not. + // We could assume that a SHA1 hash should probably contain alphas AND numerics + // but that is not always the case. + // Although unlikely, deadbeef and 1234567 are valid short forms of SHA1 hash + // as used by git and github for linking and thus we have to do similar. rawBytes = bytes.Replace(rawBytes, hash, []byte(fmt.Sprintf( `%s`, URLJoin(urlPrefix, "commit", string(hash)), base.ShortSha(string(hash)))), -1) } diff --git a/modules/markdown/markdown_test.go b/modules/markdown/markdown_test.go index c622009e8..2943ffde9 100644 --- a/modules/markdown/markdown_test.go +++ b/modules/markdown/markdown_test.go @@ -319,6 +319,8 @@ func TestRender_Commits(t *testing.T) { var src = strings.Replace(subtree, "/commit/", "/src/", -1) test(sha, `

b6dd6210ea

`) + test(sha[:7], `

b6dd621

`) + test(sha[:39], `

b6dd6210ea

`) test(commit, `

b6dd6210ea

`) test(tree, `

b6dd6210ea/src

`) test("commit "+sha, `

commit b6dd6210ea

`)