Skip to content

Fix escaping in JUnit and GitHub Actions output formats - #165

Open
DaveLiddament wants to merge 1 commit into
masterfrom
fix/output-formatter-escaping
Open

DaveLiddament wants to merge 1 commit into
masterfrom
fix/output-formatter-escaping

Conversation

@DaveLiddament

Copy link
Copy Markdown
Owner

JUnit

A violation message containing an XML-1.0-invalid character (e.g. \x08) produced a "report" consisting solely of <?xml version="1.0"?> — silent data loss. SimpleXML happily writes such characters into attributes, and DOMDocument::loadXML()'s failure was never checked.

  • Attribute values (testsuite/testcase names, failure messages) are sanitised: XML-1.0-invalid characters removed; if the value isn't valid UTF-8 at all, it's reduced to printable ASCII.
  • loadXML()'s return value is now checked (throws SarbException instead of silently continuing).

GitHub Actions

Escaping now follows the workflow-command rules:

  • Message: % → %25 and \r → %0D added (previously only \n was escaped). The Actions runner URL-decodes these sequences, so a literal %0A/%25 in a message was corrupted, and a bare CR truncated the annotation.
  • file= property: fully escaped — a path containing , or : previously terminated the property list.

Notes

  • ⚠️ Overlaps with Add support for PHPStan error identifiers #161, which adds a title= property to the github format with its own escapeProperty() — whichever merges second needs a small rebase (resolution: keep both, title uses escapeProperty()).
  • Full composer ci passes (100% coverage, PHPStan max, cs-fixer). New tests cover control characters, invalid UTF-8, and property/message escaping.

JUnit:
- Characters that are not valid in XML 1.0 (e.g. control characters in
  a violation message) are now removed. SimpleXML wrote them to the
  attribute unescaped and DOMDocument::loadXML() then failed, silently
  producing a report containing only an XML declaration.
- The return value of loadXML() is now checked.

GitHub Actions:
- Literal '%' and carriage returns in messages are now escaped (the
  Actions runner URL decodes %25/%0D/%0A, so unescaped '%' sequences
  corrupted annotations and bare CRs truncated them).
- The file property is now escaped as a workflow command property
  (a path containing ',' or ':' terminated the property list).
@DaveLiddament
DaveLiddament force-pushed the fix/output-formatter-escaping branch from 28efb5f to c7690a0 Compare November 6, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant