Fix(security) sanitize attacker controlled session id in csv export and webhook notifications#95
Conversation
Add CSV injection mitigation for exported fields.
Implement sanitization for session_id in notifications to prevent markdown injection vulnerabilities across Slack, Discord, and Microsoft Teams alerts.
Added tests for CSV injection prevention in export functionality.
Added tests to neutralize malicious link and mention syntax in Slack, Discord, and Teams webhooks.
|
Warning Review limit reached
Next review available in: 56 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
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 |
What
Two related injection fixes, both rooted in the same untrusted input:
session_idis read straight off the/chatrequest body with novalidation (
middleware/interceptor.py) and flows unmodified into (1)the CSV export and (2) all chat-based webhook payloads.
Issue 1 — CSV Injection (CWE-1236) in
/admin/escalations/exportexport_escalations()wrote DB rows to CSV viacsv.DictWriterwith noescaping. A
session_idlike=cmd|'/c calc'!A1round-trips unmodifiedinto the export; an operator opening it in Excel/Sheets/LibreOffice gets
the formula executed on their machine.
Fix: new
_csv_safe()— prefixes any field value starting with=,+,-,@, tab, or CR with a leading', the standard OWASP CSVInjection mitigation. Applied to every exported field (not just
session_id) as defense-in-depth.Issue 2 — Notification injection in escalation webhooks
session_idwas interpolated unescaped into Slack, Discord, and — worstof all — Microsoft Teams webhook payloads. Teams' Adaptive Card FactSet
had no escaping at all, so a
session_idcontaining markdown linksyntax renders as a clickable masked link inside what looks like an
internal safety alert — a way to spear-phish the incident-response team
through your own alerting pipe. Slack/Discord wrap
session_idinbackticks, but an attacker's own backtick breaks out of that span.
Fix: new
_sanitize_for_notification(), applied at all threeinterpolation points (
send_slack,send_discord,send_teams).Substitutes fullwidth Unicode lookalikes for
` * _ ~ | < > [ ]—neutralizes Slack/Discord
<...>link/mention syntax,[text](url)masked links, and backtick code-span breakout — plus a zero-width space
inserted into Discord's literal
@everyone/@herekeywords, whicharen't bracket-based.
Backslash-escaping (
\`) was tried first and rejected: caught by atest proving Slack's mrkdwn dialect doesn't reliably honor backslash
escapes for these characters the way Discord/CommonMark do — an escaped
backtick could still terminate a Slack code span. Fullwidth substitution
sidesteps that entirely, since the substitute genuinely isn't the syntax
character on any of the three platforms, regardless of escape support.
PagerDuty and email were left untouched — PagerDuty's
custom_detailsisn't markdown-rendered the same way, and email is plain-text MIME. Noted
in passing:
send_email's Subject header interpolatessession_iddirectly, which is a related but distinct class (email header /CRLF
injection, CWE-93) — out of scope for this PR since it wasn't part of
what was reported; worth a follow-up issue if you want it filed
separately.
Testing
New
TestExportCSV(tests/test_admin_api.py): formula-prefixedsession_idneutralized, all six trigger characters covered(parametrized), normal session IDs unaffected, auth still required.
New
TestNotificationInjection(tests/test_webhooks.py): Slack link/mention/backtick-breakout neutralized, Discord link/mention/
@everyoneneutralized, Teams FactSet link neutralized (the sharpest case — no prior
escaping at all), normal session IDs byte-identical to before.
pytest -q→ 450 passed, 0 failed.ruff checkon all four changedfiles → clean (pre-existing, unrelated lint debt in
webhooks.pyandtest_admin_api.py— an unusedcolorvar and two unused test imports —confirmed present in the pristine file too, not introduced here; left
out of scope for this diff).