Skip to content

Sanitize render_markdown() output with nh3 library #5133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 19, 2024

Conversation

glennmatthews
Copy link
Contributor

What's Changed

  • Add nh3 (Python wrapper around ammonia) as an HTML sanitization library and use it in render_markdown.
    • This provides a more robust defense against potential XSS and obfuscated script-injection attacks than Django's built-in strip_tags() that we were previously using.
    • As a bonus, we are no longer stripping all HTML tags before rendering Markdown to HTML - "safe" tags and attributes defined in an allow-list constant are now preserved, meaning that the various Markdown fields we have now permit explicit HTML tags and attributes so long as they comply with the allow-lists.
  • Update documentation
  • Add test coverage for a specific reported XSS example and several other examples.

Screenshots

Some notes that I authored in manual testing. The "Malicious link" would run JS code when rendered in develop and clicked, but now is just a no-op link to nowhere. The "Home" link, horizontal rule, table, and "Hello World" heading are all written directly as HTML rather than Markdown.

develop:
image

this branch:
image

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • n/a Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

Copy link
Contributor

@HanlinMiao HanlinMiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to why rel="noopener noreferrer" is present in some of the rendered markdown text.

@@ -216,12 +216,86 @@ Render a dictionary as formatted JSON.

### render_markdown

Render text as Markdown.
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Render and sanitize Markdown text into HTML. A limited subset of HTML tags and attributes are permitted in the text; non-permitted HTML will be stripped from the output for security.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads more accurately/completely to me as is - but I'm open to convincing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a nit of all nits. "As well" sounds like there was something similar stated previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking "as well as Markdown".

@glennmatthews
Copy link
Contributor Author

I'm a little confused as to why rel="noopener noreferrer" is present in some of the rendered markdown text.

It's added automatically by nh3/ammonia; my limited understanding is that it prevents external links, when clicked on, from being able to introspect information from the previous (i.e. Nautobot) page context.

@glennmatthews glennmatthews merged commit 17effcb into develop Jan 19, 2024
@glennmatthews glennmatthews deleted the GHSA-v4xv-795h-rv4h branch January 19, 2024 21:37
glennmatthews added a commit that referenced this pull request Jan 22, 2024
* Added global filtering to Job Result log table

* Fix integration test failure

* Apply suggestions from code review

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>

* use self.browser.is_text_not_present instead of time.sleep

* PR feedback

* fix migration

* fixes early return conditional in ensure_git_repository - again. (#5043)

The type returned by `git.Repo.rev_parse("HEAD")` was still off, i.e. not
a string. This commit fixes this by converting it to a string.

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Improve IPAddressEditView and IPAddressAssignView logic (#5054)

* Improve IPAddressEditView and IPAddressAssignView logic.

* Number change fragments

* Make ruff happy

---------

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

* bump version

* Process CSV import one row at a time to permit back-references (#4977)

* Process CSV import one row at a time to permit back-references to earlier rows

* Fix failing test

* Change fragment

---------

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

* fix: devcontainer (#4906)

* fix: devcontainer

* refactor(devcontainer): remove pylint and pre-commit leftover

* fix(devcontainer): arch detection

* feat(devcontainer): replace black, flake8 and isort with ruff

* refactor(devcontainer): remove prettier and eslint extensions

* refactor: remove eslint fixall

* refactor(devcontainer): move superuser information to dev.env

* fix(devcontainer): update comment with ruff instead of black

* update docs and add changelogs

* Apply suggestions from code review

* move changelogs to housekeeping category

---------

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* tt

* ruff fix

* Fixed bug with invoke cli and invoke nbshell. (#5079)

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Increase grid breakpoints by 240px (#5080)

* Update nautobot/extras/tests/integration/test_jobs.py

* bump packaging (#5077)

* bump packaging

* update lock file

* create change fragment

* update packaging dependency spec to '>=23.1'

* Update changes/5076.housekeeping

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

---------

Co-authored-by: Anthony House <anthony.house@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Job-related bug fixes (#5095)

* Job-related bug fixes

* Renumber change fragment

* [Snyk] Security upgrade jinja2 from 3.1.2 to 3.1.3 (#5086)

* Bump gitpython from 3.1.40 to 3.1.41 (#5083)

* A few performance updates (#5024)

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>

* Skip test

* Removes startplugin mgmt cmd. (#5082)

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Remove `files/get` endpoint which is not used from Nautobot 2.0 (#5115)

* remove `files/get` endpoint

* Update nautobot/core/tests/test_views.py

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* update based on review

---------

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* more flexible default sanitizer pattern (#4994)

* more flexible default sanitizer pattern

* Fix re.compile syntax

* add test for new patterns create changelog fragment

* fix test cases for new pattern

* update other santization references

* add examples of matching patterns

---------

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* More job fixes (#5102)

* More job fixes

* Renumber change fragment

* Revert redundant logging

* Enhance sanitize() to handle data other than strings, fix tests

* Add checks to runjob command

* Ruff

* Additional change fragments

* Address review feedback, add test coverage

* BugFix cf table entry rendering (#5081)

* BugFix cf table entry rendering

* Update nautobot/core/tables.py

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Create 5081.fixed

---------

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Fix #4075 - sortability of Device Bays list view by installed device status (#5110)

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

* Update PULL_REQUEST_TEMPLATE.md (#5118)

* Update PULL_REQUEST_TEMPLATE.md

* Create 5118.housekeeping

---------

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* Fixed Sync Git Repository requires non-matching permissions for UI vs API (#5128)

* Adds location & rack group to device bulk edit query params (#5113)

* 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

* Towncrier and version bump

* Fixes #5058: use location instead of location_id in vm forms (#5124)

* Bug: VM Interface vlan list in UI uses invalid option
Fixes #5058

* added changelog fragment

* grammer

* Update changes/5058.fixed

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

---------

Co-authored-by: TLCF <31373129+TLCF@users.noreply.github.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

* Update release note

* Add `TreeManager.max_depth` cachedproperty as an alternative to `max_tree_depth()` (#5131)

* Add TreeManager.max_depth cachedproperty as an alternative to max_tree_depth()

* Change fragment

* Add signal to clear max_depth cache when needed and test case to verify it

* Update release-note

---------

Co-authored-by: Timizuo <ebideritimizuo@gmail.com>
Co-authored-by: Timizuo <94907097+timizuoebideri1@users.noreply.github.com>
Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
Co-authored-by: Leo Kirchner <Kircheneer@users.noreply.github.com>
Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
Co-authored-by: Eric Jacob <erjac77@gmail.com>
Co-authored-by: housepbass <80693460+housepbass@users.noreply.github.com>
Co-authored-by: Anthony House <anthony.house@networktocode.com>
Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: John Anderson <lampwins@gmail.com>
Co-authored-by: Josh VanDeraa <jv@networktocode.com>
Co-authored-by: Gerasimos Tzakis <gertzakis@gmail.com>
Co-authored-by: Jeff Kala <48843785+jeffkala@users.noreply.github.com>
Co-authored-by: Jacob McGill <9847006+jmcgill298@users.noreply.github.com>
Co-authored-by: Joe Wesch <10467633+joewesch@users.noreply.github.com>
Co-authored-by: TL <9435779+tlourey@users.noreply.github.com>
Co-authored-by: TLCF <31373129+TLCF@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants