feat: use nh3 for HTML sanitization by nouralmaa · Pull Request #10264 · ietf-tools/datatracker

nouralmaa · 2026-01-15T17:14:56Z

nouralmaa · 2026-01-15T17:20:19Z

ietf/utils/html.py

+# Allow the protocols/tags/attributes we specifically want, plus anything that nh3 declares
+# to be safe.
+
+acceptable_protocols = nh3.ALLOWED_URL_SCHEMES.union(


TODO check. this is more permissive than bleach's strict superset. however, unsure if this adds extra vulnerabilities since nh3 uses a better maintained parser https://github.com/rust-ammonia/ammonia

@jennifer-richards @rjsparks any thoughts?

I'm going to try to get a few other eyes on this

I think adding tel: and ftp: (really?) are fine from a security perspective.

We don't really need ftp, but tel might be useful.

codecov · 2026-01-15T17:45:19Z

Codecov Report

❌ Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.39%. Comparing base (c0115c9) to head (cbbd096).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
ietf/utils/markdown.py	89.36%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10264      +/-   ##
==========================================
- Coverage   88.57%   88.39%   -0.18%     
==========================================
  Files         317      325       +8     
  Lines       42544    43544    +1000     
==========================================
+ Hits        37682    38492     +810     
- Misses       4862     5052     +190

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

martinthomson · 2026-01-18T23:29:33Z

ietf/utils/html.py

-        "li", "ol", "p", "pre", "q", "s", "samp", "small", "span", "strike", "style",
+        "li", "ol", "p", "pre", "q", "s", "samp", "small", "span", "strike",
        "strong", "sub", "sup", "table", "title", "tbody", "td", "tfoot", "th", "thead",
        "tr", "tt", "u", "ul", "var"


xmp is also good

Suggested change

"tr", "tt", "u", "ul", "var"

"tr", "tt", "u", "ul", "var", "xmp"

martinthomson · 2026-01-18T23:31:32Z

ietf/utils/html.py

+# to be safe.
+
+acceptable_protocols = nh3.ALLOWED_URL_SCHEMES.union(
    {"http", "https", "mailto", "ftp", "xmpp"}


Suggested change

{"http", "https", "mailto", "ftp", "xmpp"}

{"ftp", "http", "https", "mailto", "tel", "xmpp"}

Sort. Add "tel".

(If performance is dictated by order, move "https" to the top.)

martinthomson · 2026-01-18T23:33:45Z

ietf/utils/html.py

-    protocols=acceptable_protocols,
-    strip=True,
+    url_schemes=acceptable_protocols,
+    link_rel=None


This doesn't seem right to me. The default value is safer. Are there cases where links opened will need window.opener? I can't imagine that being necessary for user-generated content.

Same below.

cool, changed it here as there were no rel attributes before

martinthomson · 2026-01-18T23:34:58Z

ietf/utils/html.py

-    protocols=acceptable_protocols,
-    strip=True,
+_liberal_nh3_cleaner = nh3.Cleaner(
+    tags=acceptable_tags.union({"mg", "figure", "figcaption"}),


typo?

Suggested change

tags=acceptable_tags.union({"mg", "figure", "figcaption"}),

tags=acceptable_tags.union({"img", "figure", "figcaption"}),

martinthomson · 2026-01-18T23:35:51Z

ietf/utils/html.py

    """Returns the given HTML sanitized, and with the given tags removed."""
    allowed = acceptable_tags - set(t.lower() for t in tags)
-    return bleach.clean(html, tags=allowed, strip=True)
+    return nh3.clean(html, tags=allowed)


You might want to audit invocations of this function.

rjsparks · 2026-01-30T18:57:50Z

Hi @nouralmaa - Thanks for continuing to work on this.

One point on something that's not obvious - commits bf72278 and 9736b74 touch an old data migration (see the filename) - those will never run again, and its better to not touch these migration files as they should show what happened when they actually ran on the production database. If it's easy, please revert those commits. If it's not, we'll do it when this gets towards final review.

When we have major changes to django (we may have one coming with django5) we tend to squash the migrations and that one would go away at that time.

nouralmaa · 2026-01-30T19:15:49Z

ietf/utils/templatetags/tests.py

            '<a href="https://www.ietf.org" rel="nofollow">https://www.ietf.org</a>',
        )
+        self.assertEqual(
+            linkify("https://mailman3.ietf.org/mailman3/lists/[email protected]/"),


fixes #10120

nouralmaa added 2 commits January 15, 2026 16:41

feat: use nh3 for html sanitizing

0330e40

chore: comments

806448b

nouralmaa commented Jan 15, 2026

View reviewed changes

chore: stray whitespace

91e3410

martinthomson reviewed Jan 18, 2026

View reviewed changes

nouralmaa added 4 commits January 19, 2026 15:52

chore: add tel protocol and xmp tag

bb4b6d3

chore: fix typo

2c7704b

tests: add tag removal tests

a0665ac

feat: rel noopener noreferrer

356cedb

This comment was marked as spam.

Sign in to view

nouralmaa added 11 commits January 26, 2026 21:27

feat: markdown linker extention

6042400

chore: refractor and add email validation

1aeb662

chore: add linkable protocols

31c4153

chore: remove bleach

9f0a249

fix: don't use default nh3 protocols

4136809

refractor: use django's urilizer inside templates

c39659b

fix: adjust template linker

f2de311

tests: remove bleach tests

153b742

fix: templat fixes

90c47a7

chore: code cleanup

fedd691

fix: adjust migration

bf72278

nouralmaa force-pushed the replace-bleach-nh3 branch from 821b2a9 to bf72278 Compare January 29, 2026 21:22

nouralmaa added 2 commits January 29, 2026 21:27

chore: iesg email bracket

9736b74

fix: adjust migration

cbbd096

tests: add mailman tests

9cc2773

nouralmaa commented Jan 30, 2026

View reviewed changes

revert: revert migration commit

426cbaa

nouralmaa marked this pull request as ready for review January 30, 2026 19:37

	"tr", "tt", "u", "ul", "var"
	"tr", "tt", "u", "ul", "var", "xmp"

	{"http", "https", "mailto", "ftp", "xmpp"}
	{"ftp", "http", "https", "mailto", "tel", "xmpp"}

	tags=acceptable_tags.union({"mg", "figure", "figcaption"}),
	tags=acceptable_tags.union({"img", "figure", "figcaption"}),

Conversation

nouralmaa commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

rjsparks commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nouralmaa commented Jan 15, 2026 •

edited

Loading

codecov bot commented Jan 15, 2026 •

edited

Loading

rjsparks commented Jan 30, 2026 •

edited

Loading