Skip to content

fix(viewer): harden PDF report rendering - #1192

Merged
bearsyankees merged 2 commits into
usestrix:mainfrom
kusonooyasumi:fix/pdf-reportlab-hardening
Sep 1, 2026
Merged

fix(viewer): harden PDF report rendering#1192
bearsyankees merged 2 commits into
usestrix:mainfrom
kusonooyasumi:fix/pdf-reportlab-hardening

Conversation

@kusonooyasumi

@kusonooyasumi kusonooyasumi commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace sequential Markdown regexes and forgeable NUL sentinels with CommonMark tokenization that emits only fixed ReportLab formatting tags
  • disable raw HTML, links, images, and autolinks so external report content cannot inject markup or trigger resource loading
  • normalize unsafe controls and lone surrogates, canonicalize finding severities, and reject mixed-timezone duration values
  • fall back to escaped original Markdown if ReportLab rejects generated formatting
  • cover valid nested formatting, malformed emphasis, sentinel collisions, hostile run/finding fields, severity injection, and timestamp edge cases

This builds on and credits the root-cause analysis by @apetcu in #1176, while closing the remaining ReportLab trust-boundary paths found during the broader audit.

Fixes #1171.

Verification

  • uv run pytest tests/test_report_pdf.py tests/test_fenced_code.py tests/test_viewer.py - 73 passed
  • uv run ruff format --check strix/interface/viewer/report_pdf.py tests/test_report_pdf.py
  • uv run ruff check strix/interface/viewer/report_pdf.py tests/test_report_pdf.py
  • uv run mypy strix/interface/viewer/report_pdf.py tests/test_report_pdf.py
  • uv run bandit -r strix/interface/viewer/report_pdf.py -c pyproject.toml
  • uv lock --check
  • real pr-1176-repro run generated a valid PDF
  • full uv run pytest - 1221 passed, 1 unrelated failure in tests/test_pricing.py::test_resolves_common_bare_model_names because the live LiteLLM catalog no longer resolves MiniMax-M3
  • all four reviewer-provided crossed-emphasis regression cases render safely as valid ReportLab markup

Prepared with AI assistance; reviewed and tested locally.

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens PDF report generation against unsafe external content and malformed metadata.

  • Replaces regex-based inline Markdown conversion with CommonMark tokenization and fixed ReportLab formatting tags.
  • Normalizes unsafe text, severities, and timestamp error handling.
  • Adds focused tests for hostile content, malformed Markdown, severity values, and timezone edge cases.
  • Adds markdown-it-py as a direct dependency and updates the lockfile.

Confidence Score: 5/5

The PR appears safe to merge with no actionable changed-code defects identified.

The new parser emits only fixed formatting tags or escaped token content, unsafe characters are normalized before ReportLab processing, and the dependency lock remains stable.

Important Files Changed

Filename Overview
strix/interface/viewer/report_pdf.py Reworks Markdown rendering and normalizes untrusted report fields without introducing an actionable changed-code defect.
tests/test_report_pdf.py Adds broad regression coverage for safe inline formatting, hostile fields, malformed input, severity normalization, and mixed timezone values.
pyproject.toml Declares markdown-it-py as a direct runtime dependency.
uv.lock Adds the direct markdown-it-py dependency edge without changing resolved package versions.

Reviews (1): Last reviewed commit: "fix(viewer): harden PDF report rendering" | Re-trigger Greptile

@itzzdev09 itzzdev09 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this against #1171 by running both branches. It fixes the bug — but the mechanism isn't the one the issue describes, which seems worth recording since it changes what a regression test should assert.

The issue's repro doesn't actually reproduce on main. _inline_md calls html.escape() before the emphasis regexes run, so literal tags are already neutral:

>>> _inline_md("<b><i></b></i>")
'&lt;b&gt;&lt;i&gt;&lt;/b&gt;&lt;/i&gt;'

End-to-end on current main, the exact finding text from #1171 builds a PDF fine:

issue payload      -> PDF built, 4354 bytes

What actually breaks it is crossed markdown emphasis. Lines 421-423 rewrite **/* independently, so interleaved markers emit improperly nested tags:

'*a **b* c**'          -> '<i>a <b>b</i> c</b>'     ValueError
'**a *b** c*'          -> '<b>a <i>b</b> c</i>'     ValueError
'*outer **inner* end**' -> '<i>outer <b>inner</i> end</b>'  ValueError
'__a *b__ c*'          -> '<b>a <i>b</b> c</i>'     ValueError

That produces exactly the reported saw </b> instead of expected </i>. So the reporter's finding almost certainly contained emphasis markers alongside the literal tags, and the tags got the blame.

This PR fixes all of them. On 42655fb, every case above builds, and end-to-end:

issue payload      -> PDF built, 4354 bytes
crossed emphasis   -> PDF built, 4429 bytes   (ValueError on main)
normal markdown    -> PDF built, 4608 bytes

Formatting still renders rather than merely not crashing:

'**bold**'             -> '<b>bold</b>'
'`code`'               -> '<font face="Courier" color="#b31d28">code</font>'
'**bold with `code`**' -> '<b>bold with <font face="Courier" ...>code</font></b>'

Two observations:

Crossed input now resolves to well-formed but all-italic markup — *a **b* c** becomes <i>a <i><i>b</i> c</i></i>. CommonMark's own handling of that input is ambiguous, and well-formed-and-italic beats a 500, so this looks like the right trade. Just noting it in case anyone expects mixed bold/italic there.

Nice catch on _severity_badgeParagraph(severity.upper(), ...) was unescaped on main and this PR wraps it in _esc(). That's a second, separate hole closed.

If it helps, the four crossed-emphasis strings above are good regression cases, since a test built only around <b><i></b></i> would pass on main and prove nothing.


Disclosure: reviewed with AI assistance (Claude Code). Every output above is from running both branches, not inspection.

@bearsyankees
bearsyankees merged commit 1df67c5 into usestrix:main Sep 1, 2026
1 check passed
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.

[BUG]PDF report generation fails when finding contains malformed HTML-like text

3 participants