diff --git a/lab/plugins/forgecomments/README.md b/lab/plugins/forgecomments/README.md index 314c123..3572ca4 100644 --- a/lab/plugins/forgecomments/README.md +++ b/lab/plugins/forgecomments/README.md @@ -101,15 +101,33 @@ To triage the findings, propose a fix round and push it to the same change reque leading markdown markers, or as the whole content of `**…**`. - `severity: high`, `medium`, `low` or `info` anywhere in a line. - A line that starts with `[high]`, `[medium]`, `[low]` or `[info]`, after leading - markdown markers, unless the tag starts a markdown link, as in `[High](…)`. - - Within a line, the marker that starts first wins, whatever its form. A marker right after - the word `not` or `no` doesn't count. Lines inside a fenced code block (```` ``` ```` or - `~~~`) are never read. As in CommonMark, only a line holding a run of the opening - character at least as long as the opener, and nothing else, closes the fence, and a fence - left open hides the rest of the five lines. A line is a blockquote line, and never read, - when a `>` appears anywhere in its leading markdown markers, list bullets and spaces, as - in `> x` or `- > x`. A lone CR, U+0085, U+2028 and U+2029 each end a line, as LF does, and + markdown markers, unless the tag starts a markdown link, as in `[High](…)`, a + reference link, as in `[high][ref]` or `[high][]`, or a link reference definition, as + in `[high]: http://x`. + + The severity word must end the word: one followed by a Unicode letter, mark or digit, as + in `severity: highé` or `High Severityé`, is not a marker, and neither is `severity` + right after a letter. Within a line, the marker that starts first wins, whatever its + form. A marker doesn't count when the text before it ends in the word `not` or `no`, or + in a word ending in `n't` or `n’t`, as in `isn't`, with only markdown markers, + punctuation and spaces between them, as in `not: severity: high` or `not, severity: + high`. + + Text inside an inline code span, as in `` `High Severity` `` or ``` ``severity: high`` ```, + never counts. As in CommonMark, a run of backticks opens a span that the next run of + exactly the same length closes, even on a later line of the same paragraph, and a run + with no such closer is literal. A backslash does not escape a backtick here. Lines inside + a fenced code block (```` ``` ```` or `~~~`) are never read. As in CommonMark, only a line + holding a run of the opening character at least as long as the opener, and nothing + else, closes the fence, and a fence left open hides the rest of the five lines. Outside + a list, a line indented by four or more columns is an indented code line and is never + read; a tab reaches the next multiple of four columns. Such a line never opens or closes + a fence, so a marker on the line after an indented ```` ``` ```` is still read. A list + item starts a list, and an indented line in a list is list content and is read. Any + other line indented by less than four columns ends the list, even a lazy continuation, + so an indented line after it is code. A line is a blockquote line, and never read, when + a `>` appears anywhere in its leading markdown markers, list bullets and spaces, as in + `> x` or `- > x`. A lone CR, U+0085, U+2028 and U+2029 each end a line, as LF does, and CRLF is one break. Without a marker, `defaultSeverity` applies. - **Order and bounds.** Findings are ordered highest severity first, and latest comment first within a severity. At most 500 are kept, so the lowest-severity, oldest ones are diff --git a/lab/plugins/forgecomments/severity.go b/lab/plugins/forgecomments/severity.go index ba18a7d..af33ab2 100644 --- a/lab/plugins/forgecomments/severity.go +++ b/lab/plugins/forgecomments/severity.go @@ -50,36 +50,214 @@ func severityRank(s string) int { // severityOf reads the first explicit marker in the first five lines of body, // case-insensitively, and returns fallback when there is none. Format // characters, such as bidi controls, are dropped first, so they cannot hide a -// marker. Lines inside a fenced code block, fence lines and blockquote lines -// are never read, and a fence left open hides the rest of those lines. Only -// those lines are split off, so a long body costs no more than its first lines. +// marker. Lines inside a fenced code block, fence lines, blockquote lines and +// indented code lines are never read, and a fence left open hides the rest of +// those lines. Text inside an inline code span never counts. Only those lines +// are split off, so a long body costs no more than its first lines. func severityOf(body, fallback string) string { + for _, l := range readLines(body) { + if s, ok := maskedSeverity(l.text, l.masked); ok { + return s + } + } + return fallback +} + +// codeIndent is the indent, in columns, at which a line outside a list is an +// indented code line. +const codeIndent = 4 + +// readLine is a line that may carry a marker, with format characters dropped, +// and the same line with its inline code spans masked byte for byte, so an +// index into one is an index into the other. +type readLine struct { + text, masked string +} + +// readLines is the lines of the first five that are read for a marker, in +// order. It drops fenced code, fence lines, blockquote lines, blank lines and, +// outside a list, lines indented by four or more columns, which CommonMark +// reads as indented code. A paragraph is a run of read lines that no blank or +// dropped line, heading or list item breaks, and its code spans are masked +// across its lines, as a code span may continue onto the next line. +func readLines(body string) []readLine { + var out []readLine var fenceChar byte - fenceLen := 0 - for _, line := range firstLines(body, markerLines) { - line = strings.Map(visible, line) + fenceLen, para := 0, 0 + inList := false + flush := func() { + maskParagraph(out[para:]) + para = len(out) + } + for _, raw := range firstLines(body, markerLines) { + indent := indentOf(raw) + line := strings.Map(visible, raw) lead := strings.TrimLeftFunc(line, unicode.IsSpace) c, n := fenceRun(lead) if fenceLen > 0 { // As in CommonMark, only a run of the opener's character, at least as - // long, with nothing after it closes the fence. - if c == fenceChar && n >= fenceLen && strings.TrimSpace(lead[n:]) == "" { + // long, with nothing after it and indented by less than four columns + // outside a list, closes the fence. + if c == fenceChar && n >= fenceLen && strings.TrimSpace(lead[n:]) == "" && (indent < codeIndent || inList) { fenceLen = 0 } continue } - if n > 0 { + blank := strings.TrimSpace(line) == "" + code := indent >= codeIndent && !inList && !blank + item := listItem(lead) + // A list item starts or continues a list. Any other line indented by less + // than four columns ends it, even a lazy continuation, so a later indented + // line is read as code: refusing a marker is the safe side. + if !blank && !code { + if item { + inList = true + } else if indent < codeIndent { + inList = false + } + } + switch { + case code, blank: + // An indented code line, or a run of ` or ~ on one, never opens a fence. + flush() + continue + case n > 0: + flush() fenceChar, fenceLen = c, n continue + case quoted(line): + flush() + continue + } + head := heading(lead) + if item || head { + flush() + } + out = append(out, readLine{text: line}) + if head { + flush() + } + } + flush() + return out +} + +// indentOf is the leading indent of raw in columns, as CommonMark counts it: a +// space is one column and a tab reaches the next multiple of four. Format +// characters are skipped, as they are dropped before a line is read. It stops +// counting at four columns, so its work is bounded. +func indentOf(raw string) int { + col := 0 + for _, r := range raw { + switch { + case r == ' ': + col++ + case r == '\t': + col += 4 - col%4 + case unicode.Is(unicode.Cf, r): + default: + return col + } + if col >= codeIndent { + return col + } + } + return col +} + +// listItem reports whether lead, a line without its indent, starts a list +// item: a `-`, `+` or `*` bullet, or one to nine digits and `.` or `)`, +// followed by a space or the end of the line. +func listItem(lead string) bool { + i := 0 + for i < len(lead) && i < 9 && lead[i] >= '0' && lead[i] <= '9' { + i++ + } + switch { + case i == 0 && lead != "" && strings.IndexByte("-+*", lead[0]) >= 0: + i = 1 + case i > 0 && i < len(lead) && (lead[i] == '.' || lead[i] == ')'): + i++ + default: + return false + } + return i == len(lead) || lead[i] == ' ' +} + +// heading reports whether lead, a line without its indent, is an ATX heading: +// one to six `#` followed by a space or the end of the line. +func heading(lead string) bool { + n := 0 + for n < len(lead) && n < 7 && lead[n] == '#' { + n++ + } + return n >= 1 && n <= 6 && (n == len(lead) || lead[n] == ' ') +} + +// maskParagraph masks the code spans of one paragraph, read as its lines +// joined by line breaks. +func maskParagraph(lines []readLine) { + if len(lines) == 0 { + return + } + texts := make([]string, len(lines)) + for i, l := range lines { + texts[i] = l.text + } + for i, m := range strings.Split(maskCodeSpans(strings.Join(texts, "\n")), "\n") { + lines[i].masked = m + } +} + +// maskCodeSpans replaces every byte of each inline code span in s, backticks +// included, with NUL, keeping line breaks, so no marker can match inside one +// and every index is unchanged. As in CommonMark, a run of backticks opens a +// span that the next run of exactly the same length closes, and a run with no +// such closer is literal. A backslash does not escape a backtick here, so a +// span is masked whenever it could be one. It runs in linear time: each run +// finds its closer through a table built in one pass from the end. +func maskCodeSpans(s string) string { + type run struct{ at, n int } + var runs []run + for i := 0; i < len(s); { + if s[i] != '`' { + i++ + continue } - if quoted(line) { + j := i + for j < len(s) && s[j] == '`' { + j++ + } + runs = append(runs, run{i, j - i}) + i = j + } + if len(runs) < 2 { + return s + } + closer := make([]int, len(runs)) + nextOfLen := map[int]int{} + for k := len(runs) - 1; k >= 0; k-- { + closer[k] = -1 + if m, ok := nextOfLen[runs[k].n]; ok { + closer[k] = m + } + nextOfLen[runs[k].n] = k + } + b := []byte(s) + for k := 0; k < len(runs); { + m := closer[k] + if m < 0 { + k++ continue } - if s, ok := lineSeverity(line); ok { - return s + for i := runs[k].at; i < runs[m].at+runs[m].n; i++ { + if b[i] != '\n' { + b[i] = 0 + } } + k = m + 1 } - return fallback + return string(b) } // firstLines splits off at most n leading lines of body. A lone CR, NEL, @@ -132,25 +310,39 @@ func quoted(line string) bool { } // lineSeverity is the severity of the marker with the smallest index in line, -// whatever its form. A marker that `not` or `no` precedes does not count. +// whatever its form, with the line's own code spans masked. A marker that +// `not`, `no` or a word ending in `n't` precedes does not count. func lineSeverity(line string) (string, bool) { + return maskedSeverity(line, maskCodeSpans(line)) +} + +// maskedSeverity is lineSeverity for a line whose code spans are already +// masked in masked. Markers are matched in masked, so none inside a code span +// counts, and negation is read in line, so a masked span never hides a `not`. +// A severity word that runs on into a Unicode letter, mark or digit, as in +// `highé`, is not a marker, and a tag that starts a link, a reference link or +// a link reference definition does not count. +func maskedSeverity(line, masked string) (string, bool) { best, level := -1, "" consider := func(at int, l string) { if (best < 0 || at < best) && !negated(line[:at]) { best, level = at, l } } - rest := stripMarkers(line) - start := len(line) - len(rest) - if m := namedMarker.FindStringSubmatch(rest); m != nil { + rest := stripMarkers(masked) + start := len(masked) - len(rest) + if m := namedMarker.FindStringSubmatch(rest); m != nil && !wordRuneAt(rest, len(m[0])) { consider(start, m[1]) } - if m := tagMarker.FindStringSubmatch(rest); m != nil && !strings.HasPrefix(rest[len(m[0]):], "(") { + if m := tagMarker.FindStringSubmatch(rest); m != nil && !startsLink(rest[len(m[0]):]) { consider(start, m[1]) } - for _, re := range []*regexp.Regexp{boldMarker, fieldMarker} { - for _, m := range re.FindAllStringSubmatchIndex(line, -1) { - consider(m[0], line[m[2]:m[3]]) + for _, m := range boldMarker.FindAllStringSubmatchIndex(masked, -1) { + consider(m[0], masked[m[2]:m[3]]) + } + for _, m := range fieldMarker.FindAllStringSubmatchIndex(masked, -1) { + if !wordRuneBefore(masked, m[0]) && !wordRuneAt(masked, m[3]) { + consider(m[0], masked[m[2]:m[3]]) } } if best < 0 { @@ -159,12 +351,52 @@ func lineSeverity(line string) (string, bool) { return strings.ToLower(level), true } +// startsLink reports whether the text after a `[level]` tag makes it a link, +// `[x](…)`, a reference link, `[x][ref]` or `[x][]`, or a link reference +// definition, `[x]: url`. +func startsLink(after string) bool { + return after != "" && strings.IndexByte("([:", after[0]) >= 0 +} + +// wordRune reports whether r continues a word: a Unicode letter, mark or +// digit, or an underscore. +func wordRune(r rune) bool { + return unicode.IsLetter(r) || unicode.IsMark(r) || unicode.IsDigit(r) || r == '_' +} + +// wordRuneAt reports whether s has a word rune at byte index at. +func wordRuneAt(s string, at int) bool { + if at >= len(s) { + return false + } + r, _ := utf8.DecodeRuneInString(s[at:]) + return wordRune(r) +} + +// wordRuneBefore reports whether s has a word rune just before byte index at. +func wordRuneBefore(s string, at int) bool { + if at <= 0 { + return false + } + r, _ := utf8.DecodeLastRuneInString(s[:at]) + return wordRune(r) +} + // negated reports whether the text before a marker ends in the word `not` or -// `no`, any case, with markdown markers and spaces allowed after it. +// `no`, any case, or in a word ending in `n't` or `n’t`, as in `isn't`, with +// markdown markers, punctuation and spaces allowed after it, as in `not:`. func negated(before string) bool { - before = strings.TrimRightFunc(before, markerOrSpace) + before = strings.TrimRightFunc(before, func(r rune) bool { return markerOrSpace(r) || unicode.IsPunct(r) }) word := before[strings.LastIndexFunc(before, func(r rune) bool { return !unicode.IsLetter(r) })+1:] - return strings.EqualFold(word, "not") || strings.EqualFold(word, "no") + if strings.EqualFold(word, "not") || strings.EqualFold(word, "no") { + return true + } + for _, suffix := range []string{"n't", "n’t"} { + if len(before) >= len(suffix) && strings.EqualFold(before[len(before)-len(suffix):], suffix) { + return true + } + } + return false } // candidate is one finding with the comment's creation time and its position diff --git a/lab/plugins/forgecomments/severity_hardening_test.go b/lab/plugins/forgecomments/severity_hardening_test.go new file mode 100644 index 0000000..a93a322 --- /dev/null +++ b/lab/plugins/forgecomments/severity_hardening_test.go @@ -0,0 +1,234 @@ +package main + +import ( + "strings" + "testing" + + "github.com/DanielNetzer/forgecell/lab/internal/plugin/contract" +) + +// severityCases checks each body against the severity it should yield. +func severityCases(t *testing.T, fallback string, cases []struct{ body, want string }) { + t.Helper() + for _, c := range cases { + if got := severityOf(c.body, fallback); got != c.want { + t.Fatalf("severityOf(%q) = %q, want %q", c.body, got, c.want) + } + } +} + +func TestInlineCodeSpansAreNeverRead(t *testing.T) { + const fallback = "info" + severityCases(t, fallback, []struct{ body, want string }{ + // A marker inside a code span, of any run length, does not count. + {"`High Severity`", fallback}, + {"``High Severity``", fallback}, + {"`severity: high`", fallback}, + {"see `severity: high` here", fallback}, + {"**`High Severity`**", fallback}, + {"`[high]` x", fallback}, + {"``a ` severity: high ` b``", fallback}, + // A code span may continue onto the next line of its paragraph. + {"see `code\nHigh Severity` here", fallback}, + {"see `code\nseverity: high`", fallback}, + // A marker outside a code span on the same line still counts. + {"`x` severity: high", "high"}, + {"severity: low `severity: high`", "low"}, + {"`severity: high` then severity: low", "low"}, + {"`a` `b` **Medium Severity**", "medium"}, + // A run with no closer of the same length is literal. + {"`` severity: high `", "high"}, + {"` severity: high", "high"}, + {"``High Severity", "high"}, + // A blank line, a heading or a list item ends the paragraph, so a span + // does not continue past it. + {"see `code\n\nHigh Severity `x`", "high"}, + {"# Title `\nHigh Severity `x`", "high"}, + {"- a `x\n- severity: high `y", "high"}, + // A code span does not hide a negation before it. + {"not `x` severity: high", "high"}, + {"`not` severity: high", fallback}, + }) + // The masking keeps every index and line break. + if got := maskCodeSpans("a `b\nc` d"); got != "a \x00\x00\n\x00\x00 d" { + t.Fatalf("masked %q", got) + } + if got := maskCodeSpans("``x` y"); got != "``x` y" { + t.Fatalf("unmatched runs %q", got) + } +} + +// TestCodeSpanMaskingIsLinear drives many backtick runs, each with no closer, +// through the masking; a quadratic scan would make this case very slow, while +// the result must still be exact. +func TestCodeSpanMaskingIsLinear(t *testing.T) { + var b strings.Builder + for n := 1; n <= 300; n++ { + b.WriteString(strings.Repeat("`", n)) + b.WriteString(" x ") + } + line := b.String() + " severity: high" + if got := maskCodeSpans(line); got != line { + t.Fatal("runs of distinct lengths must stay literal") + } + if got := severityOf(line, "low"); got != "high" { + t.Fatalf("severity %q", got) + } + many := strings.Repeat("`a` ", 50000) + "severity: high" + if got := severityOf(many, "low"); got != "high" { + t.Fatalf("severity %q", got) + } + if got := severityOf("x "+strings.Repeat("`", 200000)+" severity: high", "low"); got != "high" { + t.Fatalf("one long run %q", got) + } +} + +func TestReferenceStyleLinkTagsAreRefused(t *testing.T) { + const fallback = "info" + severityCases(t, fallback, []struct{ body, want string }{ + {"[high][ref]", fallback}, + {"[high][]", fallback}, + {"[high]: http://x", fallback}, + {"## [low][anchor] x", fallback}, + {"**[medium]:** x", fallback}, + {"[High](https://example.invalid)", fallback}, + // A tag that does not start a link still counts. + {"[high] x", "high"}, + {"[high] (not a link)", "high"}, + {"[high] [ref]", "high"}, + {"[high] : x", "high"}, + {"[LOW]", "low"}, + // A refused tag leaves a later marker readable. + {"[high][ref] severity: low", "low"}, + {"[high]: http://x\nMedium Severity", "medium"}, + }) +} + +func TestMoreNegationFormsRefuseTheMatch(t *testing.T) { + const fallback = "info" + severityCases(t, fallback, []struct{ body, want string }{ + {"isn't severity: high", fallback}, + {"ISN'T severity: high", fallback}, + {"this isn’t severity: high", fallback}, + {"don't **High Severity**", fallback}, + {"not: severity: high", fallback}, + {"not, severity: high", fallback}, + {"No. severity: high", fallback}, + {"**not**: severity: high", fallback}, + {"(not) severity: high", fallback}, + {"not — severity: high", fallback}, + // Words that only look like a negation still allow the marker. + {"note severity: high", "high"}, + {"know severity: low", "low"}, + {"nothing: severity: high", "high"}, + {"t severity: high", "high"}, + {"n't severity: high", fallback}, + // A negated marker leaves a later one readable. + {"not severity: high, severity: low", "low"}, + {"isn't severity: high, severity: low", "low"}, + {"Low Severity, not severity: high", "low"}, + }) + for before, want := range map[string]bool{"": false, "not": true, "no ": true, "isn't, ": true, "won’t ": true, "nota": false, "nt": false, "x't": false} { + if got := negated(before); got != want { + t.Fatalf("negated(%q) = %v, want %v", before, got, want) + } + } +} + +func TestASeverityWordRunningOnIntoALetterIsNotAMarker(t *testing.T) { + const fallback = "info" + severityCases(t, fallback, []struct{ body, want string }{ + {"severity: highé", fallback}, + {"severity: high́", fallback}, + {"severity: high٣", fallback}, + {"severity: lowя", fallback}, + {"High Severityé", fallback}, + {"**High Severityé**", fallback}, + {"## Low Severityж x", fallback}, + {"éseverity: high", fallback}, + {"日severity: high", fallback}, + // A format character between them is dropped first, so it does not help. + {"severity: high‍é", fallback}, + // Spaces, punctuation and symbols still end the word. + {"severity: high", "high"}, + {"severity:HIGH", "high"}, + {"severity: high.", "high"}, + {"severity: high—", "high"}, + {"severity: high →", "high"}, + {"LOW SEVERITY: cosmetic", "low"}, + {"Severity : Medium", "medium"}, + {"High Severity — race", "high"}, + {"**High Severity**é", "high"}, + {"é severity: high", "high"}, + // A refused match leaves a later one readable. + {"severity: highé severity: low", "low"}, + }) +} + +func TestIndentedCodeLinesAreNotRead(t *testing.T) { + const fallback = "info" + severityCases(t, fallback, []struct{ body, want string }{ + {" severity: high", fallback}, + {" High Severity", fallback}, + {"\tseverity: high", fallback}, + {" \t[high] x", fallback}, + {" ​ severity: high", fallback}, + {"para\n severity: high", fallback}, + {" - severity: high", fallback}, + {" severity: high\n[low] x", "low"}, + // An indented run of ` or ~ does not open a fence, so a marker on the next + // line is still read. + {" ```\nseverity: high", "high"}, + {" ~~~\n[high] x", "high"}, + {"\t```\nHigh Severity", "high"}, + // Inside a fence, an indented run does not close it. + {"```\n ```\nseverity: high", fallback}, + // Fences indented by up to three spaces behave as before. + {" ```\nseverity: high\n ```\n[low] x", "low"}, + {" ```\nseverity: high", fallback}, + {" severity: high", "high"}, + // Inside a list, an indented line is list content, not code. + {"- a\n severity: high", "high"}, + {"1. a\n - severity: high", "high"}, + {"- a\n\n severity: high", "high"}, + {"* a\n ```\n severity: low\n ```\n[high] x", "high"}, + // Any other line indented by less than four columns ends the list. + {"- a\nb\n severity: high", fallback}, + {"- a\n\nPara\n\n severity: high", fallback}, + // Blockquotes and the five-line bound are unchanged. + {"- > severity: high", fallback}, + {"1\n2\n3\n4\n x\nHigh Severity", fallback}, + {" x\n2\n3\n4\nHigh Severity", "high"}, + }) + for raw, want := range map[string]int{"": 0, "x": 0, " x": 3, "\tx": 4, " \tx": 4, " \tx": 4, "​​ x": 2, "  x": 0, strings.Repeat(" ", 1000) + "x": 4} { + if got := indentOf(raw); got != want { + t.Fatalf("indentOf(%q) = %d, want %d", raw, got, want) + } + } + for lead, want := range map[string]bool{"- x": true, "-": true, "1. x": true, "123456789) x": true, "1234567890. x": false, "-x": false, "**x**": false, "1.x": false, "---": false, "": false} { + if got := listItem(lead); got != want { + t.Fatalf("listItem(%q) = %v, want %v", lead, got, want) + } + } +} + +// TestHardenedMarkersEndToEnd drives the new rules through the findings tool. +func TestHardenedMarkersEndToEnd(t *testing.T) { + comments := []map[string]any{ + botComment("span", "`High Severity` in a code span", 1), + botComment("ref", "[high][ref] reference link", 2), + botComment("negated", "isn't severity: high", 3), + botComment("letter", "severity: highé", 4), + botComment("indented", " severity: high", 5), + botComment("real", "severity: high", 0), + } + got, _ := decoded[contract.ReviewerFindings](t, "reviewer_findings", findingsArgs(map[string]any{"defaultSeverity": "low"}, comments)) + if len(got.Findings) != 6 || got.Findings[0].ID != "real" || got.Findings[0].Severity != "high" { + t.Fatalf("findings %+v", got.Findings) + } + for _, f := range got.Findings[1:] { + if f.Severity != "low" { + t.Fatalf("finding %s has severity %q, want the default", f.ID, f.Severity) + } + } +}