Merge pull request #22676 from netbox-community/22675-rss
#22675 Validate RSS feed entry link schemes to prevent javascript: XSS
This commit is contained in:
bctiemann
GitHub
commit
425b70275e
|
|
@ -2,7 +2,7 @@ import logging
|
|||
import uuid
|
||||
from functools import cached_property
|
||||
from hashlib import sha256
|
||||
from urllib.parse import urlencode
|
||||
from urllib.parse import urlencode, urlparse
|
||||
|
||||
import feedparser
|
||||
import requests
|
||||
|
|
@ -16,6 +16,8 @@ from django.utils.translation import gettext as _
|
|||
|
||||
from core.models import ObjectType
|
||||
from extras.choices import BookmarkOrderingChoices
|
||||
from netbox.config import get_config
|
||||
from utilities.html import clean_html
|
||||
from utilities.object_types import object_type_identifier, object_type_name
|
||||
from utilities.permissions import get_permission_for_model
|
||||
from utilities.proxy import resolve_proxies
|
||||
|
|
@ -356,7 +358,9 @@ class RSSFeedWidget(DashboardWidget):
|
|||
def cache_key(self):
|
||||
url = self.config['feed_url']
|
||||
url_checksum = sha256(url.encode('utf-8')).hexdigest()
|
||||
return f'dashboard_rss_{url_checksum}'
|
||||
# The version segment invalidates entries cached by a pre-sanitization release: such
|
||||
# entries live under the old key and are never read, so they can't be served unsanitized.
|
||||
return f'dashboard_rss_2_{url_checksum}'
|
||||
|
||||
def get_feed(self):
|
||||
if self.config.get('requires_internet') and settings.ISOLATED_DEPLOYMENT:
|
||||
|
|
@ -364,7 +368,8 @@ class RSSFeedWidget(DashboardWidget):
|
|||
'isolated_deployment': True,
|
||||
}
|
||||
|
||||
# Fetch RSS content from cache if available
|
||||
# Fetch RSS content from cache if available. Cached content is always sanitized before
|
||||
# it is written (see below), so no sanitization is needed on read.
|
||||
if feed_content := cache.get(self.cache_key):
|
||||
return {
|
||||
'feed': feedparser.FeedParserDict(feed_content),
|
||||
|
|
@ -390,6 +395,8 @@ class RSSFeedWidget(DashboardWidget):
|
|||
# Cap number of entries
|
||||
max_entries = self.config.get('max_entries')
|
||||
feed['entries'] = feed['entries'][:max_entries]
|
||||
# Sanitize feed-controlled content before caching/rendering
|
||||
self.sanitize_entries(feed['entries'])
|
||||
# Cache the feed content
|
||||
cache.set(self.cache_key, dict(feed), self.config.get('cache_timeout'))
|
||||
|
||||
|
|
@ -397,6 +404,27 @@ class RSSFeedWidget(DashboardWidget):
|
|||
'feed': feed,
|
||||
}
|
||||
|
||||
@staticmethod
|
||||
def sanitize_entries(entries):
|
||||
"""
|
||||
Sanitize feed-controlled entry content in place. The feed URL is untrusted external
|
||||
content, so we must guard against dangerous URL schemes (e.g. javascript:) in entry
|
||||
links and sanitize entry summaries as defense-in-depth.
|
||||
"""
|
||||
allowed_schemes = get_config().ALLOWED_URL_SCHEMES
|
||||
for entry in entries:
|
||||
# Blank any link whose scheme isn't permitted (blocks javascript:, data:, etc.).
|
||||
# This is the load-bearing control: the template renders entry.link into an href.
|
||||
if link := entry.get('link'):
|
||||
result = urlparse(link)
|
||||
if result.scheme and result.scheme.lower() not in allowed_schemes:
|
||||
entry['link'] = ''
|
||||
# Sanitize the summary HTML as defense-in-depth. The template renders entry.summary
|
||||
# with auto-escaping (not |safe), so this is not currently load-bearing; it guards
|
||||
# against a future change that renders the summary as markup.
|
||||
if summary := entry.get('summary'):
|
||||
entry['summary'] = clean_html(summary, allowed_schemes)
|
||||
|
||||
|
||||
@register_widget
|
||||
class BookmarksWidget(DashboardWidget):
|
||||
|
|
|
|||
|
|
@ -1,6 +1,9 @@
|
|||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from django.core.cache import cache
|
||||
from django.test import RequestFactory, TestCase, tag
|
||||
|
||||
from extras.dashboard.widgets import ObjectListWidget
|
||||
from extras.dashboard.widgets import ObjectListWidget, RSSFeedWidget
|
||||
from extras.templatetags.dashboard import render_widget
|
||||
|
||||
|
||||
|
|
@ -49,6 +52,81 @@ class ObjectListWidgetTestCase(TestCase):
|
|||
self.assertTrue('Unable to load content. Could not resolve list URL for:' in rendered)
|
||||
|
||||
|
||||
class RSSFeedWidgetSanitizationTestCase(TestCase):
|
||||
"""
|
||||
Feed entry content is externally controlled and untrusted. Links must be validated against
|
||||
ALLOWED_URL_SCHEMES so dangerous schemes (e.g. javascript:) cannot become clickable XSS sinks.
|
||||
"""
|
||||
|
||||
@tag('regression')
|
||||
def test_sanitize_entries_blanks_disallowed_schemes(self):
|
||||
entries = [
|
||||
{'link': 'javascript:alert(document.cookie)', 'title': 't1'},
|
||||
{'link': 'JavaScript:alert(1)', 'title': 't2'}, # case-insensitive
|
||||
{'link': 'data:text/html,<script>alert(1)</script>', 'title': 't3'},
|
||||
{'link': 'vbscript:msgbox(1)', 'title': 't4'},
|
||||
]
|
||||
RSSFeedWidget.sanitize_entries(entries)
|
||||
for entry in entries:
|
||||
self.assertEqual(entry['link'], '', msg=f"Failed to blank {entry['title']}")
|
||||
|
||||
@tag('regression')
|
||||
def test_sanitize_entries_preserves_allowed_links(self):
|
||||
entries = [
|
||||
{'link': 'https://example.com/post', 'title': 't1'},
|
||||
{'link': 'http://example.com/post', 'title': 't2'},
|
||||
{'link': 'mailto:user@example.com', 'title': 't3'},
|
||||
{'link': '/relative/path', 'title': 't4'}, # schemeless relative link
|
||||
]
|
||||
expected = [e['link'] for e in entries]
|
||||
RSSFeedWidget.sanitize_entries(entries)
|
||||
self.assertEqual([e['link'] for e in entries], expected)
|
||||
|
||||
@tag('regression')
|
||||
def test_sanitize_entries_cleans_summary_html(self):
|
||||
entries = [
|
||||
{'link': 'https://example.com', 'title': 't1', 'summary': '<b>ok</b><script>alert(1)</script>'},
|
||||
]
|
||||
RSSFeedWidget.sanitize_entries(entries)
|
||||
self.assertNotIn('<script>', entries[0]['summary'])
|
||||
self.assertIn('<b>ok</b>', entries[0]['summary'])
|
||||
|
||||
@tag('regression')
|
||||
def test_get_feed_sanitizes_before_caching(self):
|
||||
"""
|
||||
Fetched feed content must be sanitized before it is rendered or written to the cache,
|
||||
so a poisoned link is never stored or served.
|
||||
"""
|
||||
widget = RSSFeedWidget(config={
|
||||
'feed_url': 'https://example.com/feed.xml',
|
||||
'requires_internet': False,
|
||||
'max_entries': 10,
|
||||
'cache_timeout': 3600,
|
||||
})
|
||||
rss = (
|
||||
b'<?xml version="1.0"?>'
|
||||
b'<rss version="2.0"><channel><title>t</title>'
|
||||
b'<link>http://example.com</link><description>d</description>'
|
||||
b'<item><title>evil</title><link>javascript:alert(1)</link>'
|
||||
b'<description>d</description></item>'
|
||||
b'</channel></rss>'
|
||||
)
|
||||
mock_response = MagicMock()
|
||||
mock_response.content = rss
|
||||
|
||||
with (
|
||||
patch('extras.dashboard.widgets.requests.get', return_value=mock_response),
|
||||
patch('extras.dashboard.widgets.resolve_proxies', return_value={}),
|
||||
):
|
||||
result = widget.get_feed()
|
||||
|
||||
# The rendered feed is sanitized...
|
||||
self.assertEqual(result['feed']['entries'][0]['link'], '')
|
||||
# ...and the cached copy is sanitized too (never stored poisoned).
|
||||
cached = cache.get(widget.cache_key)
|
||||
self.assertEqual(cached['entries'][0]['link'], '')
|
||||
|
||||
|
||||
class RenderWidgetTemplateTagTestCase(TestCase):
|
||||
|
||||
def _make_context(self):
|
||||
|
|
|
|||
Loading…
Reference in New Issue