Health scanner can't recognize a fixed finding: no-inner-html still reports 65 after the escaping remediation merged#206
○Openccantynz opened this issue 24d ago
ccantynzAuthorcommented 24d ago
Live case (2026-08-08): PR #5450 on verom-ai-immigration-compliance remediated all 65 no-inner-html findings the correct way — interpolated values wrapped in escapeHtml(), static templates left as-is — and merged. The health page still reports "65 findings" and still offers the same +5 improvement, because SECURITY_PATTERNS matches the regex innerHTML\s*= with no notion of whether the assigned value is escaped or static.
Consequence: the flagship loop (diagnose → fix → PR → merge → score rises) breaks on its own top recommendation. The user merges a real security fix and the score doesn't move — which reads as "the fix didn't work".
Options (can combine):
Escape-aware heuristic: don't flag an innerHTML assignment when the statement (or its template literal) contains escapeHtml(, or when the RHS is a string literal with no ${} interpolation (static template). Cheap, catches the two safe classes the remediation produced.
Suppression comment: a // safe-html: <reason> line marker the scanner honors per-line (precedent: the existing secrets-ok skip in analyzeSecurityScore), so intentional residuals can be acknowledged with justification.
Downgrade rather than drop: escaped-innerHTML sites could report severity info instead of medium, keeping visibility without the score penalty.
Note: scoreSecurityIssues' per-rule cap (20 pts for medium) means the score damage is bounded, but the improvement list is what promises the +5 gain — it must stop promising points that the merge already earned.
Verified state after merge: score 62 → 64, but the +2 came from Activity freshness (last push 0d), not Security.
c comment · e edit title · x close/reopen · ? shortcuts
Live case (2026-08-08): PR #5450 on verom-ai-immigration-compliance remediated all 65
no-inner-htmlfindings the correct way — interpolated values wrapped inescapeHtml(), static templates left as-is — and merged. The health page still reports "65 findings" and still offers the same +5 improvement, becauseSECURITY_PATTERNSmatches the regexinnerHTML\s*=with no notion of whether the assigned value is escaped or static.Consequence: the flagship loop (diagnose → fix → PR → merge → score rises) breaks on its own top recommendation. The user merges a real security fix and the score doesn't move — which reads as "the fix didn't work".
Options (can combine):
escapeHtml(, or when the RHS is a string literal with no${}interpolation (static template). Cheap, catches the two safe classes the remediation produced.// safe-html: <reason>line marker the scanner honors per-line (precedent: the existingsecrets-okskip inanalyzeSecurityScore), so intentional residuals can be acknowledged with justification.infoinstead ofmedium, keeping visibility without the score penalty.Note:
scoreSecurityIssues' per-rule cap (20 pts for medium) means the score damage is bounded, but the improvement list is what promises the +5 gain — it must stop promising points that the merge already earned.Verified state after merge: score 62 → 64, but the +2 came from Activity freshness (last push 0d), not Security.