diff options
author | mrsdizzie <info@mrsdizzie.com> | 2020-10-21 18:14:44 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-10-21 18:14:44 -0400 |
commit | f6ee7ce9b6da6abcb32f3d7d90f34dbe3b01bb41 (patch) | |
tree | 7f89960c7a334fbf830c3d1f6e38e5e3e1b4a3c7 /services/gitdiff | |
parent | 83106c166d8708f6ce5fd5c65ce4a4ad6943c48d (diff) | |
download | gitea-f6ee7ce9b6da6abcb32f3d7d90f34dbe3b01bb41.tar.gz gitea-f6ee7ce9b6da6abcb32f3d7d90f34dbe3b01bb41.zip |
Add better error checking for inline html diff code (#13239)
* Add better error checking for inline html diff code
A better fix for #13191 which cleans up this code a bit and adds basic checking which should avoid writing broken HTML in future situations.
* Update gitdiff_test.go
* better regex
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Diffstat (limited to 'services/gitdiff')
-rw-r--r-- | services/gitdiff/gitdiff.go | 88 | ||||
-rw-r--r-- | services/gitdiff/gitdiff_test.go | 4 |
2 files changed, 44 insertions, 48 deletions
diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 3aacc830f0..164e1c0ca5 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -181,64 +181,60 @@ var ( removedCodePrefix = []byte(`<span class="removed-code">`) codeTagSuffix = []byte(`</span>`) ) -var addSpanRegex = regexp.MustCompile(`<span\s*[a-z="]*$`) +var trailingSpanRegex = regexp.MustCompile(`<span\s*[[:alpha:]="]*?[>]?$`) + +// shouldWriteInline represents combinations where we manually write inline changes +func shouldWriteInline(diff diffmatchpatch.Diff, lineType DiffLineType) bool { + if true && + diff.Type == diffmatchpatch.DiffEqual || + diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd || + diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel { + return true + } + return false +} func diffToHTML(fileName string, diffs []diffmatchpatch.Diff, lineType DiffLineType) template.HTML { buf := bytes.NewBuffer(nil) - var addSpan string - for i := range diffs { - switch { - case diffs[i].Type == diffmatchpatch.DiffEqual: - // Looking for the case where our 3rd party diff library previously detected a string difference - // in the middle of a span class because we highlight them first. This happens when added/deleted code - // also changes the chroma class name, either partially or fully. If found, just move the openining span code forward into the next section - // see TestDiffToHTML for examples - if len(addSpan) > 0 { - diffs[i].Text = addSpan + diffs[i].Text - addSpan = "" + match := "" + + for _, diff := range diffs { + if shouldWriteInline(diff, lineType) { + if len(match) > 0 { + diff.Text = match + diff.Text + match = "" } - m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text) + // Chroma HTML syntax highlighting is done before diffing individual lines in order to maintain consistency. + // Since inline changes might split in the middle of a chroma span tag, make we manually put it back together + // before writing so we don't try insert added/removed code spans in the middle of an existing chroma span + // and create broken HTML. + m := trailingSpanRegex.FindStringSubmatchIndex(diff.Text) if m != nil { - addSpan = diffs[i].Text[m[0]:m[1]] - buf.WriteString(strings.TrimSuffix(diffs[i].Text, addSpan)) - } else { - addSpan = "" - buf.WriteString(getLineContent(diffs[i].Text)) - } - case diffs[i].Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd: - if len(addSpan) > 0 { - diffs[i].Text = addSpan + diffs[i].Text - addSpan = "" + match = diff.Text[m[0]:m[1]] + diff.Text = strings.TrimSuffix(diff.Text, match) } - // Print existing closing span first before opening added-code span so it doesn't unintentionally close it - if strings.HasPrefix(diffs[i].Text, "</span>") { + // Print an existing closing span first before opening added/remove-code span so it doesn't unintentionally close it + if strings.HasPrefix(diff.Text, "</span>") { buf.WriteString("</span>") - diffs[i].Text = strings.TrimPrefix(diffs[i].Text, "</span>") + diff.Text = strings.TrimPrefix(diff.Text, "</span>") } - m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text) - if m != nil { - addSpan = diffs[i].Text[m[0]:m[1]] - diffs[i].Text = strings.TrimSuffix(diffs[i].Text, addSpan) + // If we weren't able to fix it then this should avoid broken HTML by not inserting more spans below + // The previous/next diff section will contain the rest of the tag that is missing here + if strings.Count(diff.Text, "<") != strings.Count(diff.Text, ">") { + buf.WriteString(diff.Text) + continue } + } + switch { + case diff.Type == diffmatchpatch.DiffEqual: + buf.WriteString(diff.Text) + case diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd: buf.Write(addedCodePrefix) - buf.WriteString(diffs[i].Text) + buf.WriteString(diff.Text) buf.Write(codeTagSuffix) - case diffs[i].Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel: - if len(addSpan) > 0 { - diffs[i].Text = addSpan + diffs[i].Text - addSpan = "" - } - if strings.HasPrefix(diffs[i].Text, "</span>") { - buf.WriteString("</span>") - diffs[i].Text = strings.TrimPrefix(diffs[i].Text, "</span>") - } - m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text) - if m != nil { - addSpan = diffs[i].Text[m[0]:m[1]] - diffs[i].Text = strings.TrimSuffix(diffs[i].Text, addSpan) - } + case diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel: buf.Write(removedCodePrefix) - buf.WriteString(diffs[i].Text) + buf.WriteString(diff.Text) buf.Write(codeTagSuffix) } } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index bb7cb40313..e7eeca7004 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -50,7 +50,7 @@ func TestDiffToHTML(t *testing.T) { {Type: dmp.DiffEqual, Text: "</span> <span class=\"p\">{</span>"}, }, DiffLineAdd)) - assertEqual(t, "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"removed-code\"><span class=\"nx\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">"## [%s](%s/%s/%s/%s?q=&type=all&state=closed&milestone=%d) - %s"</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\"</span></span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">BaseURL</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Owner</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Repo</span><span class=\"p\">,</span> <span class=\"nx\"><span class=\"removed-code\">from</span><span class=\"p\">,</span> <span class=\"nx\">milestoneID</span><span class=\"p\">,</span> <span class=\"nx\">time</span><span class=\"p\">.</span><span class=\"nf\">Now</span><span class=\"p\">(</span><span class=\"p\">)</span><span class=\"p\">.</span><span class=\"nf\">Format</span><span class=\"p\">(</span><span class=\"s\">"2006-01-02"</span><span class=\"p\">)</span></span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{ + assertEqual(t, "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"removed-code\"><span class=\"nx\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">"## [%s](%s/%s/%s/%s?q=&type=all&state=closed&milestone=%d) - %s"</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\"</span></span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">BaseURL</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Owner</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Repo</span><span class=\"p\">,</span> <span class=\"removed-code\"><span class=\"nx\">from</span><span class=\"p\">,</span> <span class=\"nx\">milestoneID</span><span class=\"p\">,</span> <span class=\"nx\">time</span><span class=\"p\">.</span><span class=\"nf\">Now</span><span class=\"p\">(</span><span class=\"p\">)</span><span class=\"p\">.</span><span class=\"nf\">Format</span><span class=\"p\">(</span><span class=\"s\">"2006-01-02"</span><span class=\"p\">)</span></span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{ {Type: dmp.DiffEqual, Text: "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"n"}, {Type: dmp.DiffDelete, Text: "x\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">"## [%s](%s/%s/%s/%s?q=&type=all&state=closed&milestone=%d) - %s"</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\""}, {Type: dmp.DiffInsert, Text: "f\">getGiteaTagURL</span><span class=\"p\">(</span><span class=\"nx\">client"}, @@ -60,7 +60,7 @@ func TestDiffToHTML(t *testing.T) { {Type: dmp.DiffEqual, Text: "</span><span class=\"p\">)</span>"}, }, DiffLineDel)) - assertEqual(t, "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"nx\"><span class=\"removed-code\">language</span></span><span class=\"removed-code\"><span class=\"p\">,</span> <span class=\"kc\">true</span><span class=\"p\">,</span> <span class=\"nx\">attrs</span></span><span class=\"p\">,</span> <span class=\"kc\">false</span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{ + assertEqual(t, "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"removed-code\"><span class=\"nx\">language</span></span><span class=\"removed-code\"><span class=\"p\">,</span> <span class=\"kc\">true</span><span class=\"p\">,</span> <span class=\"nx\">attrs</span></span><span class=\"p\">,</span> <span class=\"kc\">false</span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{ {Type: dmp.DiffEqual, Text: "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"nx\">"}, {Type: dmp.DiffDelete, Text: "language</span><span "}, {Type: dmp.DiffEqual, Text: "c"}, |