fix(reports): escape test titles in the HTML report

CHANGELOG.md

@@ -23,6 +23,7 @@
 - Performance: `--coverage` is roughly 5x faster and `--coverage-report-html` roughly 19x — a run over this repo went from 16.2s to 2.9s, and a 128-file HTML report from 58.7s to 3.1s. The report phase emits each format in one awk invocation per run instead of Bash loops and forks per file and per row, and the capture path writes records straight to disk, normalizes a path with one fork instead of four, and reads each cache once (#1092, #1096, #1098, #1099, #1102, #1104, #1110, #1117)
 
 ### Fixed
+- The HTML report escapes test titles instead of writing them into the markup. A title is user text, so `bashunit::set_test_title "<script>…"` corrupted the table and ran in whoever opened the report — a CI artifact is read in a browser — while a bare `&` was invalid entity syntax. Rows were also joined on `|`, so a title containing one shifted every column and produced a `class` that matches no rule (#1249)
 - `--coverage-paths` accepts a path containing a space (or an apostrophe, or a glob character). The paths are baked into the DEBUG trap's `case` as syntax, and the literal segments were unquoted, so `my src/` produced a trap that does not parse: 75 syntax errors for a one-test run, coverage reported 0% because nothing was recorded, and the test itself was marked failed although its assertion passed (#1245)
 - `bashunit --output junit` produces valid XML. A blank line printed between a file's tests and the next landed in front of the document, so the declaration was not first and no parser accepted it — and since parallel is not the default, the plain documented command was the broken one. `--output json` gained the same leading blank line, unnoticed because JSON tolerates leading whitespace (#1243)
 - `--parallel --stop-on-failure` no longer corrupts a machine `--output` stream. `Stop on failure enabled...` is human output but was printed with no regard for the stream, so it preceded the document under `--output json` (which then did not parse) and displaced the XML declaration under `--output junit`. It also carried a leading carriage return, which snapshots strip and therefore never caught

src/reports/html.sh

@@ -17,6 +17,13 @@ function bashunit::reports::generate_report_html() {
   local temp_file
   temp_file=$(mktemp "${TMPDIR:-/tmp}/bashunit-report.XXXXXX")
 
+  # Fields are separated by US (0x1f), not `|`. A test name is user text --
+  # bashunit::set_test_title takes anything and a data provider interpolates
+  # values into it -- so a title containing the delimiter shifted every column
+  # and turned `class="$status"` into a class that does not exist (#1249).
+  local _us
+  _us=$(printf '\037')
+
   # Collect test cases by file
   : >"$temp_file" # Clear temp file if it exists
   local i
@@ -25,11 +32,32 @@ function bashunit::reports::generate_report_html() {
     local name="${_BASHUNIT_REPORTS_TEST_NAMES[$i]:-}"
     local status="${_BASHUNIT_REPORTS_TEST_STATUSES[$i]:-}"
     local test_time="${_BASHUNIT_REPORTS_TEST_DURATIONS[$i]:-}"
-    local test_case="$file|$name|$status|$test_time"
+    local test_case="$file$_us$name$_us$status$_us$test_time"
 
     echo "$test_case" >>"$temp_file"
   done
 
+  # Escape every field for HTML in one pass. The same user text was written
+  # straight into the markup, so a title containing `<script>` ran in whoever
+  # opened the report -- a CI artifact is read in a browser.
+  #
+  # awk, not ${var//&/&amp;}: a bare `&` in a bash replacement means "the
+  # matched text" from 5.2 on while staying literal on 3.2, and there is no
+  # spelling that is right across the supported range (#1096). In awk the same
+  # rule applies to gsub, hence the escaped \\& below. The separator is passed
+  # in as a byte rather than written as \x1f, which is not POSIX awk (#1098).
+  local escaped_file
+  escaped_file=$(mktemp "${TMPDIR:-/tmp}/bashunit-report-esc.XXXXXX")
+  awk -v FS="$_us" -v OFS="$_us" '{
+    for (i = 1; i <= NF; i++) {
+      gsub(/&/, "\\&amp;", $i)
+      gsub(/</, "\\&lt;", $i)
+      gsub(/>/, "\\&gt;", $i)
+      gsub(/"/, "\\&quot;", $i)
+    }
+    print
+  }' "$temp_file" >"$escaped_file" && mv "$escaped_file" "$temp_file"
+
   {
     echo "<!DOCTYPE html>"
     echo "<html lang=\"en\">"
@@ -82,7 +110,7 @@ function bashunit::reports::generate_report_html() {
     # Read the temporary file and group by file
     local current_file=""
     local file name status test_time
-    while IFS='|' read -r file name status test_time; do
+    while IFS="$_us" read -r file name status test_time; do
       if [ "$file" != "$current_file" ]; then
         if [ -n "$current_file" ]; then
           echo "    </tbody>"

tests/acceptance/bashunit_report_html_escaping_test.sh

@@ -0,0 +1,78 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+# The HTML report wrote test names straight into the markup, and a test title
+# is user text -- `bashunit::set_test_title` takes anything, and a data
+# provider interpolates values into it. So a title containing `<` corrupted the
+# table, and one containing `<script>` was executed by whoever opened the
+# report, which for a CI artifact is a browser (#1249).
+#
+# Rows were also joined with `|` into a temp file and split back on it, so a
+# title containing a pipe shifted every column: the name truncated, the status
+# cell showed a fragment of the title, and `class="$status"` became a CSS class
+# that does not exist, losing the row's colour.
+
+function set_up_before_script() {
+  BASHUNIT_BIN="$(pwd)/bashunit"
+}
+
+function set_up() {
+  WORKDIR="$(bashunit::temp_dir)"
+}
+
+function _report_with_title() { # $1 = title
+  {
+    printf '%s\n' '#!/usr/bin/env bash'
+    printf '%s\n' 'function test_titled() {'
+    printf '  bashunit::set_test_title "%s"\n' "$1"
+    printf '%s\n' '  assert_same 1 1'
+    printf '%s\n' '}'
+  } >"$WORKDIR/t_test.sh"
+
+  (cd "$WORKDIR" && "$BASHUNIT_BIN" --no-parallel --report-html rep.html t_test.sh >/dev/null 2>&1) || true
+  cat "$WORKDIR/rep.html"
+}
+
+function test_a_title_with_markup_is_escaped() {
+  local html
+  html="$(_report_with_title '<script>alert(1)</script>')"
+
+  assert_not_contains "<script>alert(1)</script>" "$html"
+  assert_contains "&lt;script&gt;" "$html"
+}
+
+function test_a_title_with_an_ampersand_is_escaped() {
+  local html
+  html="$(_report_with_title 'a & b')"
+
+  assert_contains "a &amp; b" "$html"
+}
+
+# The row is stored and split on a delimiter, so a title carrying it must not
+# shift the columns: the status cell has to hold a status, not a fragment.
+function test_a_title_with_a_pipe_keeps_the_columns_aligned() {
+  local html
+  html="$(_report_with_title 'before|after')"
+
+  assert_contains 'class="passed"' "$html"
+  assert_not_contains 'class="after"' "$html"
+  assert_contains "<td>passed</td>" "$html"
+}
+
+# And it still has to render as the title the user asked for. A pipe needs no
+# entity once it is not the delimiter -- it is ordinary text in HTML.
+function test_a_title_with_a_pipe_still_reads_as_written() {
+  local html
+  html="$(_report_with_title 'before|after')"
+
+  assert_contains "<td>before|after</td>" "$html"
+}
+
+# An ordinary title must come through untouched, or the escaping is too eager.
+function test_a_plain_title_is_unchanged() {
+  local html
+  html="$(_report_with_title 'plain title')"
+
+  assert_contains "<td>plain title</td>" "$html"
+  assert_contains 'class="passed"' "$html"
+}