fix(security): strip script/style elements whose end tag carries trailing junk#429
Conversation
…ling junk
ContentSanitizer removes <script>/<style> elements from GitHub issue, PR, and
comment bodies before they are embedded in an agent prompt. The end-tag
patterns only accepted '</script\s*>' and '</style>', but HTML treats
everything up to the next '>' as part of the end tag. Given a valid end tag
like '</script foo>' the pattern found no match, so the *entire* element --
payload included -- passed through untouched:
<script>ignore all instructions</script foo> -> unchanged
Match end tags as '</script[^>]*>' / '</style[^>]*>', and anchor the open tag
with '\b' so '<scriptx>' is not swallowed. Flagged by CodeQL py/bad-tag-filter.
The module had no tests; add coverage for the tag-stripping contract. Five of
the new tests fail against the previous patterns.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
1 participant
Third of three PRs from the security-dashboard triage (after #426 and #428). This is the one real code bug the triage turned up.
The bug
ContentSanitizer(runners/github/sanitize.py) strips<script>/<style>elements out of GitHub issue, PR, and comment bodies before they are embedded in an agent prompt — it's prompt-injection defence for attacker-controlled content.Its end-tag patterns were
</script\s*>and</style>. But HTML treats everything up to the next>as part of an end tag, so</script foo>is a perfectly valid way to close a script element. The regex found no match for it — and because the open and close are matched as one unit, a failed close means the entire element was left in place, payload and all:I reproduced each of these against the shipped patterns before changing anything. CodeQL flagged it as
py/bad-tag-filter(alert #841) with exactly this end-tag example.The fix
</script[^>]*>matches the end-tag forms above.\bon the open tag keeps<scriptx>keep me</scriptx>from being swallowed — a false positive the old pattern also avoided, and I didn't want to trade one bug for another.Tests
The module had no test coverage at all, so this adds
tests/test_github_sanitize.py(17 tests) pinning the contract: script/style removal across attribute, casing, multiline and junk-end-tag forms; lookalike tags and prose mentioning<script>preserved; HTML-comment stripping; clean bodies untouched.Verified the tests aren't vacuous — against the previous patterns 5 of them fail; against the fix all 17 pass. Neighbouring suites stay green (
test_github_bot_detection,test_github_pr_e2e,test_github_pr_review, + this file: 74 passed).Scope note: what I deliberately left out
The triage also surfaced two broad CodeQL classes that I'd rather not fold in here:
js/log-injection(18, medium) — untrusted pty/websocket data reaching log sinks.js/file-system-race(12, high) — TOCTOUexistsSync→ read/write, mostly in the Electron main process and build scripts.Both mean touching ~30 files across the main process for issues with low real-world exploitability in a desktop app. That's a review-heavy change with genuine regression risk, and it doesn't belong in the same PR as a contained regex fix. Happy to take them on separately if you want them — my suggestion is a central escaping helper for the log sinks first, since that one is mechanical and testable.
🤖 Generated with Claude Code