Skip to content

Commit 17effcb

Browse files
Sanitize render_markdown() output with nh3 library (#5133)
* Fix GHSA-v4xv-795h-rv4h. * Renumber change fragments * Address review feedback * Test fix * Ruff * Review feedback * Update nautobot/core/forms/fields.py
1 parent dbcc053 commit 17effcb

File tree

18 files changed

+221
-46
lines changed

18 files changed

+221
-46
lines changed

Diff for: changes/5133.added

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Enhanced Markdown-supporting fields (`comments`, `description`, Notes, Job log entries, etc.) to also permit the use of a limited subset of "safe" HTML tags and attributes.

Diff for: changes/5133.dependencies

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Added `nh3` HTML sanitization library as a dependency.

Diff for: changes/5133.security

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixed an XSS vulnerability ([GHSA-v4xv-795h-rv4h](https://github.com/nautobot/nautobot/security/advisories/GHSA-v4xv-795h-rv4h)) in the `render_markdown()` utility function used to render comments, notes, job log entries, etc.

Diff for: nautobot/core/constants.py

+33
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
from copy import deepcopy
2+
3+
import nh3
4+
15
SEARCH_MAX_RESULTS = 15
26

37
#
@@ -32,6 +36,35 @@
3236

3337
FILTER_NEGATION_LOOKUP_MAP = {"n": "exact"}
3438

39+
#
40+
# User input sanitization
41+
#
42+
43+
# Subset of the HTML tags allowed by default by ammonia:
44+
# https://github.com/rust-ammonia/ammonia/blob/master/src/lib.rs
45+
HTML_ALLOWED_TAGS = nh3.ALLOWED_TAGS - {
46+
# no image maps at present
47+
"area",
48+
"map",
49+
# no document-level markup at present
50+
"article",
51+
"aside",
52+
"footer",
53+
"header",
54+
"nav",
55+
# miscellaneous out-of-scope for now
56+
"data",
57+
"dfn",
58+
"figcaption",
59+
"figure",
60+
}
61+
62+
# Variant of the HTML attributes allowed by default by ammonia:
63+
# https://github.com/rust-ammonia/ammonia/blob/master/src/lib.rs
64+
# at present we just copy nh3.ALLOWED_ATTRIBUTES but we can modify this later as desired and appropriate
65+
HTML_ALLOWED_ATTRIBUTES = deepcopy(nh3.ALLOWED_ATTRIBUTES)
66+
67+
3568
#
3669
# Reserved Names
3770
#

Diff for: nautobot/core/forms/fields.py

+12-6
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist, ValidationError
1010
from django.db.models import Q
1111
from django.forms.fields import BoundField, InvalidJSONInput, JSONField as _JSONField
12+
from django.templatetags.static import static
1213
from django.urls import reverse
14+
from django.utils.html import format_html
1315
import django_filters
1416
from netaddr import EUI
1517
from netaddr.core import AddrFormatError
@@ -372,12 +374,16 @@ class CommentField(django_forms.CharField):
372374

373375
widget = django_forms.Textarea
374376
default_label = ""
375-
# TODO: Port Markdown cheat sheet to internal documentation
376-
default_helptext = (
377-
'<i class="mdi mdi-information-outline"></i> '
378-
'<a href="https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet" target="_blank">'
379-
"Markdown</a> syntax is supported"
380-
)
377+
378+
@property
379+
def default_helptext(self):
380+
# TODO: Port Markdown cheat sheet to internal documentation
381+
return format_html(
382+
'<i class="mdi mdi-information-outline"></i> '
383+
'<a href="https://www.markdownguide.org/cheat-sheet/#basic-syntax" rel="noopener noreferrer">Markdown</a> '
384+
'syntax is supported, as well as <a href="{}#render_markdown">a limited subset of HTML</a>.',
385+
static("docs/user-guide/platform-functionality/template-filters.html"),
386+
)
381387

382388
def __init__(self, *args, **kwargs):
383389
required = kwargs.pop("required", False)

Diff for: nautobot/core/settings.py

+2-1
Original file line numberDiff line numberDiff line change
@@ -682,7 +682,8 @@
682682
),
683683
"SUPPORT_MESSAGE": ConstanceConfigItem(
684684
default="",
685-
help_text="Help message to include on 4xx and 5xx error pages. Markdown is supported.\n"
685+
help_text="Help message to include on 4xx and 5xx error pages. "
686+
"Markdown is supported, as are some HTML tags and attributes.\n"
686687
"If unspecified, instructions to join Network to Code's Slack community will be provided.",
687688
),
688689
}

Diff for: nautobot/core/templatetags/helpers.py

+5-10
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
from django.contrib.staticfiles.finders import find
99
from django.templatetags.static import static, StaticNode
1010
from django.urls import NoReverseMatch, reverse
11-
from django.utils.html import format_html, strip_tags
11+
from django.utils.html import format_html
1212
from django.utils.safestring import mark_safe
1313
from django.utils.text import slugify as django_slugify
1414
from django_jinja import library
@@ -17,7 +17,7 @@
1717

1818
from nautobot.apps.config import get_app_settings_or_config
1919
from nautobot.core import forms
20-
from nautobot.core.utils import color, config, data, lookup
20+
from nautobot.core.utils import color, config, data, logging as nautobot_logging, lookup
2121
from nautobot.core.utils.requests import add_nautobot_version_query_param_to_url
2222

2323
# S308 is suspicious-mark-safe-usage, but these are all using static strings that we know to be safe
@@ -170,17 +170,12 @@ def render_markdown(value):
170170
Example:
171171
{{ text | render_markdown }}
172172
"""
173-
# Strip HTML tags
174-
value = strip_tags(value)
175-
176-
# Sanitize Markdown links
177-
schemes = "|".join(settings.ALLOWED_URL_SCHEMES)
178-
pattern = rf"\[(.+)\]\((?!({schemes})).*:(.+)\)"
179-
value = re.sub(pattern, "[\\1](\\3)", value, flags=re.IGNORECASE)
180-
181173
# Render Markdown
182174
html = markdown(value, extensions=["fenced_code", "tables"])
183175

176+
# Sanitize rendered HTML
177+
html = nautobot_logging.clean_html(html)
178+
184179
return mark_safe(html) # noqa: S308 # suspicious-mark-safe-usage, OK here since we sanitized the string earlier
185180

186181

Diff for: nautobot/core/tests/test_templatetags_helpers.py

+19-4
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,24 @@ def test_render_markdown(self):
7979
helpers.render_markdown("**bold and _italics_**"), "<p><strong>bold and <em>italics</em></strong></p>"
8080
)
8181
self.assertEqual(helpers.render_markdown("* list"), "<ul>\n<li>list</li>\n</ul>")
82-
self.assertEqual(
82+
self.assertHTMLEqual(
8383
helpers.render_markdown("[I am a link](https://www.example.com)"),
84-
'<p><a href="https://www.example.com">I am a link</a></p>',
84+
'<p><a href="https://www.example.com" rel="noopener noreferrer">I am a link</a></p>',
85+
)
86+
87+
def test_render_markdown_security(self):
88+
self.assertEqual(helpers.render_markdown('<script>alert("XSS")</script>'), "")
89+
self.assertHTMLEqual(
90+
helpers.render_markdown('[link](javascript:alert("XSS"))'),
91+
'<p><a title="XSS" rel="noopener noreferrer">link</a>)</p>', # the trailing ) seems weird to me, but...
92+
)
93+
self.assertHTMLEqual(
94+
helpers.render_markdown(
95+
"[link\nJS]"
96+
"(&#x6A&#x61&#x76&#x61&#x73&#x63&#x72&#x69&#x70&#x74&#x3A" # '(javascript:'
97+
"&#x61&#x6C&#x65&#x72&#x74&#x28&#x27&#x58&#x53&#x53&#x27&#x29)" # 'alert("XSS"))'
98+
),
99+
'<p><a rel="noopener noreferrer">link JS</a></p>',
85100
)
86101

87102
def test_render_json(self):
@@ -240,8 +255,8 @@ def test_support_message(self):
240255
self.assertHTMLEqual(
241256
helpers.support_message(),
242257
"<p>If further assistance is required, please join the <code>#nautobot</code> channel "
243-
'on <a href="https://slack.networktocode.com/">Network to Code\'s Slack community</a> '
244-
"and post your question.</p>",
258+
'on <a href="https://slack.networktocode.com/" rel="noopener noreferrer">Network to Code\'s '
259+
"Slack community</a> and post your question.</p>",
245260
)
246261

247262
with override_config(SUPPORT_MESSAGE="Reach out to your support team for assistance."):

Diff for: nautobot/core/tests/test_views.py

+7-7
Original file line numberDiff line numberDiff line change
@@ -366,9 +366,9 @@ def test_404_default_support_message(self):
366366
self.assertContains(response, "Network to Code", status_code=404)
367367
response_content = response.content.decode(response.charset)
368368
self.assertInHTML(
369-
"If further assistance is required, please join the <code>#nautobot</code> channel "
370-
'on <a href="https://slack.networktocode.com/">Network to Code\'s Slack community</a> '
371-
"and post your question.",
369+
"If further assistance is required, please join the <code>#nautobot</code> channel on "
370+
'<a href="https://slack.networktocode.com/" rel="noopener noreferrer">Network to Code\'s '
371+
"Slack community</a> and post your question.",
372372
response_content,
373373
)
374374

@@ -392,16 +392,16 @@ def test_500_default_support_message(self, mock_get):
392392
self.assertContains(response, "Network to Code", status_code=500)
393393
response_content = response.content.decode(response.charset)
394394
self.assertInHTML(
395-
"If further assistance is required, please join the <code>#nautobot</code> channel "
396-
'on <a href="https://slack.networktocode.com/">Network to Code\'s Slack community</a> '
397-
"and post your question.",
395+
"If further assistance is required, please join the <code>#nautobot</code> channel on "
396+
'<a href="https://slack.networktocode.com/" rel="noopener noreferrer">Network to Code\'s '
397+
"Slack community</a> and post your question.",
398398
response_content,
399399
)
400400

401401
@override_settings(DEBUG=False, SUPPORT_MESSAGE="Hello world!")
402402
@mock.patch("nautobot.core.views.HomeView.get", side_effect=Exception)
403403
def test_500_custom_support_message(self, mock_get):
404-
"""Nautobot's custom 500 page should be used and should include a default support message."""
404+
"""Nautobot's custom 500 page should be used and should include a custom support message if defined."""
405405
url = reverse("home")
406406
with self.assertTemplateUsed("500.html"):
407407
self.client.raise_request_exception = False

Diff for: nautobot/core/utils/logging.py

+14-1
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
1-
"""Utilities for working with log messages and similar features."""
1+
"""Utilities for working with log messages and similar user-authored data."""
22

33
import logging
44
import re
55

66
from django.conf import settings
7+
import nh3
8+
9+
from nautobot.core import constants
710

811
logger = logging.getLogger(__name__)
912

@@ -47,3 +50,13 @@ def sanitize(dirty, replacement="(redacted)"):
4750

4851
logger.warning("No sanitizer support for %s data", type(dirty))
4952
return dirty
53+
54+
55+
def clean_html(html):
56+
"""Use nh3/ammonia to strip out all HTML tags and attributes except those explicitly permitted."""
57+
return nh3.clean(
58+
html,
59+
tags=constants.HTML_ALLOWED_TAGS,
60+
attributes=constants.HTML_ALLOWED_ATTRIBUTES,
61+
url_schemes=set(settings.ALLOWED_URL_SCHEMES),
62+
)

Diff for: nautobot/docs/development/jobs/index.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ This is the human-friendly name of your job, as will be displayed in the Nautobo
113113
#### `description`
114114

115115
An optional human-friendly description of what this job does.
116-
This can accept either plain text or Markdown-formatted text. It can also be multiple lines:
116+
This can accept either plain text, Markdown-formatted text, or [a limited subset of HTML](../../user-guide/platform-functionality/template-filters.md#render_markdown). It can also be multiple lines:
117117

118118
```python
119119
class ExampleJob(Job):
@@ -512,7 +512,7 @@ To skip writing a log entry to the database, set the `skip_db_logging` key in th
512512
logger.info("This job is running!", extra={"skip_db_logging": True})
513513
```
514514

515-
Markdown rendering is supported for log messages.
515+
Markdown rendering is supported for log messages, as well as [a limited subset of HTML](../../user-guide/platform-functionality/template-filters.md#render_markdown).
516516

517517
+/- 1.3.4
518518
As a security measure, the `message` passed to any of these methods will be passed through the `nautobot.core.utils.logging.sanitize()` function in an attempt to strip out information such as usernames/passwords that should not be saved to the logs. This is of course best-effort only, and Job authors should take pains to ensure that such information is not passed to the logging APIs in the first place. The set of redaction rules used by the `sanitize()` function can be configured as [settings.SANITIZER_PATTERNS](../../user-guide/administration/configuration/optional-settings.md#sanitizer_patterns).

Diff for: nautobot/docs/user-guide/administration/configuration/optional-settings.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -964,7 +964,7 @@ If set to `False`, unknown/unrecognized filter parameters will be discarded and
964964

965965
Default: `""`
966966

967-
A message to include on error pages (status code 403, 404, 500, etc.) when an error occurs. You can configure this to direct users to the appropriate contact(s) within your organization that provide support for Nautobot. Markdown formatting is supported within this message (raw HTML is not).
967+
A message to include on error pages (status code 403, 404, 500, etc.) when an error occurs. You can configure this to direct users to the appropriate contact(s) within your organization that provide support for Nautobot. Markdown formatting is supported within this message, as well as [a limited subset of HTML](../../platform-functionality/template-filters.md#render_markdown).
968968

969969
If unset, the default message that will appear is `If further assistance is required, please join the #nautobot channel on [Network to Code's Slack community](https://slack.networktocode.com) and post your question.`
970970

Diff for: nautobot/docs/user-guide/platform-functionality/note.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,4 @@
44

55
Notes provide a place for you to store notes or general information on an object, such as a Device, that may not require a specific field for. This could be a note on a recent upgrade, a warning about a problematic device, or the reason the Rack was marked with the Status `Retired`.
66

7-
The note field supports [Markdown Basic Syntax](https://www.markdownguide.org/cheat-sheet/#basic-syntax).
7+
The note field supports [Markdown Basic Syntax](https://www.markdownguide.org/cheat-sheet/#basic-syntax) as well as [a limited subset of HTML](template-filters.md#render_markdown).

Diff for: nautobot/docs/user-guide/platform-functionality/template-filters.md

+75-1
Original file line numberDiff line numberDiff line change
@@ -216,12 +216,86 @@ Render a dictionary as formatted JSON.
216216

217217
### render_markdown
218218

219-
Render text as Markdown.
219+
Render and sanitize Markdown text into HTML. A limited subset of HTML tags and attributes are permitted in the text as well; non-permitted HTML will be stripped from the output for security.
220220

221221
```django
222222
{{ text | render_markdown }}
223223
```
224224

225+
#### Permitted HTML Tags and Attributes
226+
227+
+++ 2.1.2
228+
229+
The set of permitted HTML tags is defined in `nautobot.core.constants.HTML_ALLOWED_TAGS`, and their permitted attributes are defined in `nautobot.core.constants.HTML_ALLOWED_ATTRIBUTES`. As of Nautobot 2.1.2 the following are permitted:
230+
231+
??? info "Full list of HTML tags and attributes"
232+
| Tag | Attributes |
233+
| -------------- | -------------------------------------------------------------------- |
234+
| `<a>` | `href`, `hreflang` |
235+
| `<abbr>` | |
236+
| `<acronym>` | |
237+
| `<b>` | |
238+
| `<bdi>` | |
239+
| `<bdo>` | `dir` |
240+
| `<blockquote>` | `cite` |
241+
| `<br>` | |
242+
| `<caption>` | |
243+
| `<center>` | |
244+
| `<cite>` | |
245+
| `<code>` | |
246+
| `<col>` | `align`, `char`, `charoff`, `span` |
247+
| `<colgroup>` | `align`, `char`, `charoff`, `span` |
248+
| `<dd>` | |
249+
| `<del>` | `cite`, `datetime` |
250+
| `<details>` | |
251+
| `<div>` | |
252+
| `<dl>` | |
253+
| `<dt>` | |
254+
| `<em>` | |
255+
| `<h1>` | |
256+
| `<h2>` | |
257+
| `<h3>` | |
258+
| `<h4>` | |
259+
| `<h5>` | |
260+
| `<h6>` | |
261+
| `<hgroup>` | |
262+
| `<hr>` | `align`, `size`, `width` |
263+
| `<i>` | |
264+
| `<img>` | `align`, `alt`, `height`, `src`, `width` |
265+
| `<ins>` | `cite`, `datetime` |
266+
| `<kbd>` | |
267+
| `<li>` | |
268+
| `<mark>` | |
269+
| `<ol>` | `start` |
270+
| `<p>` | |
271+
| `<pre>` | |
272+
| `<q>` | `cite` |
273+
| `<rp>` | |
274+
| `<rt>` | |
275+
| `<rtc>` | |
276+
| `<ruby>` | |
277+
| `<s>` | |
278+
| `<samp>` | |
279+
| `<small>` | |
280+
| `<span>` | |
281+
| `<strike>` | |
282+
| `<strong>` | |
283+
| `<sub>` | |
284+
| `<summary>` | |
285+
| `<sup>` | |
286+
| `<table>` | `align`, `char`, `charoff`, `summary` |
287+
| `<tbody>` | `align`, `char`, `charoff` |
288+
| `<td>` | `align`, `char`, `charoff`, `colspan`, `headers`, `rowspan` |
289+
| `<th>` | `align`, `char`, `charoff`, `colspan`, `headers`, `rowspan`, `scope` |
290+
| `<thead>` | `align`, `char`, `charoff` |
291+
| `<time>` | |
292+
| `<tr>` | `align`, `char`, `charoff` |
293+
| `<tt>` | |
294+
| `<u>` | |
295+
| `<ul>` | |
296+
| `<var>` | |
297+
| `<wbr>` | |
298+
225299
### render_yaml
226300

227301
Render a dictionary as formatted YAML.

0 commit comments

Comments
 (0)