Escape untrusted values rendered into issue/PR bodies

[schenksj]

Escape untrusted values rendered into issue/PR bodies

Neutralizes the places where finding- or scan-derived text gets rendered into GitHub issue/PR markdown without escaping. These came from the cross-model /vulnhunt evaluation (eval_rep/), plus the review feedback on this PR. Developed test-first, keeping the existing suites green.

The branch is up to date with main via a merge commit (0c917f4). The earlier config.py prerequisite is gone: the identical fix landed on main via #13, and the now-redundant TestModuleImportRegression test is removed in 835e20f.

CANON-21: Constrain marker values in issue-body HTML comments (CWE-79)

Commit 2615c32. vulnhunter-agent/agent/issues_render.py substitutes VULN_ID and RESULTS_DIR_NAME verbatim into HTML-comment markers such as <!-- vulnhunt-finding-id: {VULN_ID} -->. They stay un-escaped on purpose so the markers remain machine-parseable, but a value containing --> would close the comment early.

• Change: _sanitize_marker_value() restricts marker values to [A-Za-z0-9._-], which is exactly the charset the downstream parsers accept. It applies to VULN_ID, RESULTS_DIR_NAME and (defensively) IDEMPOTENCY_KEY.
• Follow-up (71256ad): when stripping changes a value, a warning is now logged naming the marker and the before/after values. Previously this was silent, and stripping can turn a results-dir name into a different one. The consumer side (vulnhunter-fix/scripts/issue_intake.py) already enforces the same [A-Za-z0-9._-] charset and rejects ..///\ since Port results-dir validation from verify_extract to issue_intake #26, so it needs no change.
• Tests: test_issues_render.py::TestMarkerCommentBreakout, ::TestMarkerSanitizeWarns.

CANON-44: Escape residual-risk entries in the delivery body (CWE-79/116)

Commits 4949b3a, 49b29e2, fea53f1. vulnhunter-fix/vulnhunter_fix/delivery.py rendered each residual_vectors entry verbatim into the ## Residual Risk bullets.

• Change: _escape_residual_entry() html-escapes the entry, backslash-escapes ` [ ], and collapses CR/LF so an entry can't break out of its bullet. It now also defuses @user/@org/team mentions and bare https:// / www. autolinks with a zero-width-space reference (​, the Dependabot convention). Ordinary prose is unchanged.
• Tests: vulnhunter-fix/tests/test_hand_wave_guard.py.

Review: verification-table cell injection (reported by AmirF194)

Commit b81bee4. _column7_cell interpolated graph_callers / routed_callers (file:symbol from the scanned repo) unescaped into the 9-column verification table. A caller like x.py:a) | yes | FULL | <!-- added cells and forged the Verdict column GitHub displays, and a newline broke out of the table.

• Change: one helper, _escape_table_cell(), does the html/backtick/bracket escaping, escapes | as \| and collapses CR/LF. It is applied to every cell of render_verification_row: index, VULN id, the six data cells (including the computed column 7) and the Verdict. The Verdict is derived from the escaped cells. yes (file.py:42) citations come out byte-for-byte unchanged.
• Tests: test_sweep_and_verification.py::test_verification_row_*. The pipe-forging and newline cases each render exactly 9 cells and the real verdict (WORKAROUND). Every data field is checked individually.

Gate bypass in validate-verification.py (found while reproducing the above)

Commit d135de4. _parse_table silently dropped any row whose cell count differed from the header. A forged row was therefore never validated, and Gate 6 exited 0.

• Change: rows are split only on unescaped pipes (GFM semantics). Cells are decoded back from the renderer's escapes, so escaped citations and callers validate against their literal text. Any row with the wrong cell count raises MalformedTableError, naming the row, line and cell count, and the gate exits 1. Rows run to the first blank line, as in GFM (a leading pipe is optional), so the continuation of a newline-broken row is checked too. references/verification-table-rules.md documents the rule.
• Tests: these run the gate script as a subprocess. The forged-pipe row exited 0 before the fix and exits 1 after (expected 9 cells, got 12). The newline-broken row also exits 1. A renderer-escaped row (a\|b caller, src/\[id\].py citation) parses into 9 cells, validates (exit 0) and is genuinely checked: a bad citation in it now fails.

Review: smaller items

• Tier whitelist (3480e7b): render_residual_risk_section interpolated **{tier}** raw. A tier outside the result-schema enum (FULL/MITIGATION/WORKAROUND) now raises UnknownTierError, a HonestyGuardError like the other guard failures. The old "not fully closed" fallback is gone.
• Issue-body table cells (0b2ee90): the ### Finding table fields (CWE, CWE name, severity, location, root cause, data flow) did not escape | or newlines. A new _sanitize_inline() handles every single-line site: the table cells, the H2 title and the Priority line. The multi-line free-form sections (exploit description/impact, fix strategy, severity rationale) keep their newlines.
• Single-pass placeholder substitution (0b2ee90): render_body and the clean-scan renderer replaced placeholders field by field, so a value containing {IDEMPOTENCY_KEY} was expanded by a later pass. A single regex pass with a dict lookup (_substitute) fixes this. Unfilled-placeholder warnings now come from the template, not from user values.

Verification

Every change was written test-first (RED, then GREEN). The CI-equivalent pytest --cov runs:

• vulnhunter-agent (py3.12): 1162 passed, 1 skipped
• vulnhunter-fix (py3.11 and py3.12): 586 passed, 10 skipped each; coverage 86.75%
• Test delta vs main: test_issues_render.py +23, test_hand_wave_guard.py +15, test_sweep_and_verification.py +16 (54 total)

🤖 Generated with Claude Code

[schenksj]

Adversarial review + fix applied (pushed 49b29e2).

CANON-21 (marker breakout) reviewed solid. CANON-44 review found a gap: _escape_residual_entry escaped angle brackets/backticks/brackets but not newlines, so a multi-line residual_vectors entry broke out of the - {entry} bullet and injected block-level markdown (a forged ## Verification Complete heading + --- rule).

Fix: collapse [\r\n]+ → space in _escape_residual_entry (in addition to existing escaping), so an entry can't span multiple markdown lines. New test_residual_entry_newlines_cannot_inject_block_markdown (RED before, GREEN after). vulnhunter-fix suite: 549 passed, 9 skipped.

[AmirF194]

Ran this against 49b29e2 in a clean python:3.12-slim container.

The PR's own stated scope is escaping attacker-influenced values before they land in a posted PR/issue body, and _column7_cell (delivery.py:222,228) is the one interpolation site in this same file the diff leaves untouched. graph_callers/routed_callers are file:symbol strings straight from the scanned repo (query.py:169's callers_of, or the grep fallback's _enclosing_symbol), so a file literally named evil|report.py in the target repo is enough:

rows = [{..., "graph_callers": ["evil|report.py:handler", "clean.py:normal_caller"], ...}]
render_verification_table(rows)
# | 1 | VULN-001 | yes some_file.py:42 | yes | yes | n/a | yes (clean.py:normal_caller) (evil|report.py:handler) | yes | FULL |
# 9 columns expected, 10 parsed

A pipe in either half of the caller string splits into an extra cell, and a longer one can fabricate yes | yes | yes | FULL in place of the real verdict, which is exactly what a reviewer reads before approving an auto-generated security fix. Same _escape_residual_entry this PR adds for residual_vectors (or stripping |/newlines) would close this too.

[schenksj]

Thanks @AmirF194 for the careful review and the concrete repro — confirmed. _column7_cell interpolated scan-derived callers unescaped, so a | in a filename could forge the Verdict column and a newline could break out of the table.

Reproducing it surfaced something worse in the gate: scripts/validate-verification.py's _parse_table silently dropped any row whose cell count didn't match the header, so a forged row was never validated at all (same fake citations: exit 1 with a clean caller, exit 0 with the pipe). Both are fixed:

• b81bee4 — a single _escape_table_cell() (| → \|, CR/LF collapsed, plus the existing html/markdown escaping) applied to every cell of render_verification_row, not just column 7. Ordinary file:line citations render unchanged.
• d135de4 — the validator splits only on unescaped pipes and fails closed on a malformed row, naming it.

Following your scoping argument, I also swept the rest of the rendered bodies:

• 0b2ee90 — issue-body table fields (_sanitize_inline) escape |/newlines; placeholders are substituted in a single pass.
• 3480e7b — residual-risk tier is whitelisted before rendering.
• fea53f1 — @mentions and bare autolinks in residual entries are defused.
• 71256ad — marker sanitization logs a warning when it changes a value.

All with RED→GREEN tests. vulnhunter-agent: 1162 passed; vulnhunter-fix (py3.11 & 3.12): 586 passed.

[emeth-]

Approved - this is a clear improvement over main.

The escaping is correct where it's applied, _escape_table_cell and the validator's split/decode agree, and it was a good catch that the validator silently dropped malformed rows. Single-pass substitution and the tier whitelist are good too.

Some gaps that already exist on main may be worth follow-ups on, perhaps in a separate PR. They're mostly in Gate 6, which is the real control because the LLM can write the fix-side table directly:

1. The Verdict column isn't enforced anywhere. verification-table-rules.md says NEEDS_REWORK halts delivery, but no gate reads the Verdict. Against validate-verification.py with --sidecars-dir/--result:

   • A correctly derived NEEDS_REWORK row exits 0.
   • | 1 | VULN-001 | no | no | no | no | n/a | n/a | FULL | exits 0. That's the other half of Amir's forged-FULL concern.
   • A FULL row with any resolving citations and column 7 n/a exits 0, even when the sidecar lists callers the fix never routed through and result.json says MITIGATION.

   Suggestions:

   • Derive the verdict in the validator and fail on a mismatch or NEEDS_REWORK.
   • Fail when the Verdict is stronger than completeness_tier.
   • Reject column 7 n/a when the sidecar lists callers.

   Separately, the column-8 values sweep-root-causes.py emits (yes, yes (n/a)) fail the citation rule today, so rendered tables are rejected when a sweep ran.

2. _parse_table uses the first header-looking line anywhere, ignoring HTML comments, code fences and the delimiter row. It has two failure modes, both of which exit 0:

   • A decoy table above the real one (inside a fence or a comment) gets validated instead.
   • Text above the table with a lookalike-header table plus an unclosed <!-- hides the real table and ## Residual Risk on GitHub, up to the vulnfix-key marker's -->. Gate 6 still validates the hidden table, so reviewers only see the forged one. Gate 2 also counts the hidden headings as present.

   This matters because the fix side reads finding text straight from the scan README, and deliver.md gives no escaping guidance. Suggest stripping comments (an unclosed one runs to the end) and fences before parsing, refusing <!-- outside the known markers, and requiring exactly one table under ## Verification Table.