Markdown Sanitization - XSS#1046
Closed
AndyVale wants to merge 7 commits into
Closed
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a bleach-based server-side markdown sanitizer and registers it in settings; adds a client-side JS helper that posts editor content to the markdownify endpoint for preview; updates EasyMDE instances in several templates to use the server-backed preview and switches one template to use the sanitized markdown filter. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant EasyMDE
participant Server
participant Bleach
User->>Browser: Edit markdown in editor
Browser->>EasyMDE: Editor content change
EasyMDE->>Browser: previewRender(plainText) invoked
Browser->>Browser: getCookie('csrftoken')
Browser->>Server: POST /markdownx/markdownify/ (content + CSRF)
Server->>Server: markdownify(content) -> raw HTML
Server->>Bleach: clean(raw HTML) using allowlists
Bleach->>Server: sanitized HTML
Server->>Browser: 200 OK + sanitized HTML
Browser->>EasyMDE: Render sanitized HTML in preview pane
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/templates/courses/update.html (1)
88-102:⚠️ Potential issue | 🟡 MinorRemove this dead code—
getCookieis no longer used.After refactoring to use
sanitizedPreviewRenderfrommarkdown_preview.js, this localgetCookiefunction is no longer called anywhere in this template. Themarkdown_preview.jsfile includes its owngetCookieimplementation.Leaving dead code can cause confusion for future maintainers and slightly increases the page size.
🧹 Suggested removal of dead code
}); - // Helper function to get CSRF token from cookies - function getCookie(name) { - let cookieValue = null; - if (document.cookie && document.cookie !== '') { - const cookies = document.cookie.split(';'); - for (let i = 0; i < cookies.length; i++) { - const cookie = cookies[i].trim(); - if (cookie.substring(0, name.length + 1) === (name + '=')) { - cookieValue = decodeURIComponent(cookie.substring(name.length + 1)); - break; - } - } - } - return cookieValue; - } - // Fix form submission for hidden required textareas🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/update.html` around lines 88 - 102, Remove the unused local helper function getCookie from the template: locate the function definition named getCookie in the update.html template and delete it; ensure no remaining references to getCookie exist in update.html (the sanitizedPreviewRender logic now comes from markdown_preview.js which provides its own getCookie), so simply remove the dead getCookie function to avoid duplication and reduce page size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/success_stories/detail.html`:
- Around line 57-59: Move the template tag `{% load markdown_filters %}` from
its current location near the content block to the top of the template
(immediately after the `{% extends ... %}` line) and remove the duplicate `{%
load markdown_filters %}` at line 57 so the filter usage `{{
success_story.content|markdown }}` continues to work but the `{% load %}`
follows Django convention and improves readability.
In `@web/templatetags/markdown_filters.py`:
- Around line 28-47: Update markdownify_sanitized to include type hints and
eliminate the unnecessary intermediate variable: annotate the parameter as
Optional[str] (or str) and the return type as str, and replace the two-step
assignment with a single return that calls bleach.clean(markdownify(content),
tags=SAFE_TAGS, attributes=SAFE_ATTRIBUTES, strip=True). Ensure you import
typing.Optional if using Optional and keep references to markdownify,
bleach.clean, SAFE_TAGS, and SAFE_ATTRIBUTES unchanged.
- Around line 20-26: The SAFE_ATTRIBUTES list is fine but you should add an
explicit protocol allowlist for defense-in-depth: define a small set like
ALLOWED_PROTOCOLS = {"http", "https", "mailto"} and pass it into the
bleach.clean (or bleach.Cleaner) invocation that uses SAFE_ATTRIBUTES so
href/src are only allowed those protocols; reference SAFE_ATTRIBUTES and the
bleach.clean / bleach.Cleaner call in your patch so protocol filtering is
explicit rather than relying on bleach defaults.
---
Outside diff comments:
In `@web/templates/courses/update.html`:
- Around line 88-102: Remove the unused local helper function getCookie from the
template: locate the function definition named getCookie in the update.html
template and delete it; ensure no remaining references to getCookie exist in
update.html (the sanitizedPreviewRender logic now comes from markdown_preview.js
which provides its own getCookie), so simply remove the dead getCookie function
to avoid duplication and reduce page size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7143131e-c9af-42bc-9e91-452175c1c155
📒 Files selected for processing (8)
web/settings.pyweb/static/js/markdown_preview.jsweb/templates/courses/create.htmlweb/templates/courses/update.htmlweb/templates/success_stories/create.htmlweb/templates/success_stories/detail.htmlweb/templates/teach.htmlweb/templatetags/markdown_filters.py
Contributor
👀 Peer Review RequiredHi @AndyVale! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templatetags/markdown_filters.py`:
- Around line 48-52: Add type hints to the markdown filter: update the function
signature of markdown to accept text: str and return a django SafeString (or
typing.Any/str if you prefer) and eliminate the intermediate variable by
returning mark_safe(markdownify_sanitized(text)) directly; reference the
markdown function, markdownify_sanitized call, and mark_safe to locate and
update the code accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 161aeff7-7524-40b0-ac1f-bdc20f261e20
📒 Files selected for processing (3)
web/templates/courses/update.htmlweb/templates/success_stories/detail.htmlweb/templatetags/markdown_filters.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/templatetags/markdown_filters.py (1)
49-51:⚠️ Potential issue | 🟡 MinorReturn a concrete safe-string type instead of
Any.Line 49 still advertises
Any, which leaves the ANN401 warning unresolved and hides the actual contract here. Since this filter returnsmark_safe(...), annotate it asSafeStringand considertext: Optional[str]to matchmarkdownify_sanitized.✨ Suggested typing cleanup
-import bleach -from typing import Any, Optional +from typing import Optional + +import bleach from django import template -from django.utils.safestring import mark_safe +from django.utils.safestring import SafeString, mark_safe @@ `@register.filter` -def markdown(text: str) -> Any: +def markdown(text: Optional[str]) -> SafeString: """Convert markdown text to sanitized HTML.""" return mark_safe(markdownify_sanitized(text))As per coding guidelines "Use type hints in Python where appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templatetags/markdown_filters.py` around lines 49 - 51, The function signature for markdown should use a concrete safe-string return type and accept optional input: change the annotation of def markdown(text: str) -> Any to def markdown(text: Optional[str]) -> SafeString, importing Optional from typing and SafeString from django.utils.safestring, and keep the implementation returning mark_safe(markdownify_sanitized(text)) to match markdownify_sanitized's nullable input and to resolve the ANN401 warning while making the contract explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templatetags/markdown_filters.py`:
- Around line 31-45: Add regression tests covering the sanitizer boundary around
the markdownify_sanitized function: create tests that call markdownify_sanitized
with payloads that must be stripped (e.g., "<script>" tags, attributes like
onerror/onload, and javascript: links) and assert those are removed, and also
tests that ensure legitimate Markdown output is preserved (links, tables, fenced
code blocks, images) to prevent regressions when
SAFE_TAGS/SAFE_ATTRIBUTES/ALLOWED_PROTOCOLS change; include one test for the
markdownx preview flow if applicable and name tests to map to
markdownify_sanitized so future reviewers can find coverage easily.
---
Duplicate comments:
In `@web/templatetags/markdown_filters.py`:
- Around line 49-51: The function signature for markdown should use a concrete
safe-string return type and accept optional input: change the annotation of def
markdown(text: str) -> Any to def markdown(text: Optional[str]) -> SafeString,
importing Optional from typing and SafeString from django.utils.safestring, and
keep the implementation returning mark_safe(markdownify_sanitized(text)) to
match markdownify_sanitized's nullable input and to resolve the ANN401 warning
while making the contract explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 232edf39-24e7-4c9e-a42e-8ec9331247e3
📒 Files selected for processing (1)
web/templatetags/markdown_filters.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
dismissed
their stale review
All review conversations have been resolved.
Contributor
🚚 This Repository Is MovingHi @AndyVale, thank you for your contribution! We are in the process of migrating most of the logic from this repository to our new repository: alphaonelabs/learn. What this means for your PRPlease do not merge or continue work here. Instead:
This PR has been automatically closed. Once you have opened your PR in the new repository, feel free to reference it here. Thank you for your understanding and continued support! 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Markdown Sanitization - XSS
Abstract
This PR remediates Stored and Reflected XSS vulnerabilities in the markdown rendering pipeline by centralizing sanitization using
bleach.Current Problem
The application was vulnerable to script injection in two main areas:
|safewithout server-side sanitization, allowing malicious tags to execute for all users.XSS.mp4
Fixes
A defense-in-depth approach has been implemented:
1. Centralized Sanitization
markdownify_sanitizedfunction inweb/templatetags/markdown_filters.pyusingbleachto strip malicious tags and attributes while preserving markdown formatting.settings.pyto use this function globally for alldjango-markdownxAJAX previews.2. Template Security
|safeinstances with a|markdownfilter that converts and sanitizes content on the server.3. Hardened Editor Previews
static/js/markdown_preview.jsacross (I think) all editors (Course, Teach, and Success Stories) to enforce server-side sanitized previews, eliminating the reflected XSS vector.Verification
Confirmed that payloads such as
<script>tags andonerrorattributes are properly stripped while preserving valid markdown content.maybeNOT_XSS.mp4
Markdown Sanitization XSS Fixes
This PR centralizes server-side sanitization for markdown rendering to remediate stored and reflected XSS vectors.
Purpose
Remove stored XSS (templates that rendered raw HTML with |safe) and reflected/self-XSS in EasyMDE previews by routing all markdown rendering through a bleach-based sanitizer on the server.
Key Changes
Tests
Impact