Remove XSS potential using format_html helpers (#2950)

Author: sethmlarson

apps/boxes/templatetags/boxes.py

@@ -15,7 +15,7 @@
 def box(label):
     """Render the content of a Box identified by its label slug."""
     try:
-        return mark_safe(Box.objects.only("content").get(label=label).content.rendered)
+        return mark_safe(Box.objects.only("content").get(label=label).content.rendered)  # noqa: S308
     except Box.DoesNotExist:
         log.warning("WARNING: box not found: label=%s", label)
         return ""

apps/companies/templatetags/companies.py

@@ -2,12 +2,12 @@
 
 from django import template
 from django.template.defaultfilters import stringfilter
-from django.utils.html import format_html
+from django.utils.html import format_html_join, mark_safe
 
 register = template.Library()
 
 
-@register.filter(is_safe=True)
+@register.filter()
 @stringfilter
 def render_email(value):
     """Render an email address with obfuscated dots and at-sign using spans."""
@@ -16,8 +16,8 @@ def render_email(value):
         mailbox_tokens = mailbox.split(".")
         domain_tokens = domain.split(".")
 
-        mailbox = "<span>.</span>".join(mailbox_tokens)
-        domain = "<span>.</span>".join(domain_tokens)
+        mailbox = format_html_join(mark_safe("<span>.</span>"), "{}", [(token,) for token in mailbox_tokens])
+        domain = format_html_join(mark_safe("<span>.</span>"), "{}", [(token,) for token in domain_tokens])
 
-        return format_html(f"{mailbox}<span>@</span>{domain}")
+        return mailbox + mark_safe("<span>@</span>") + domain
     return None

apps/downloads/templatetags/download_tags.py

@@ -6,8 +6,7 @@
 import requests
 from django import template
 from django.core.cache import cache
-from django.utils.html import format_html
-from django.utils.safestring import mark_safe
+from django.utils.html import format_html, format_html_join, mark_safe
 
 from apps.downloads.models import Release
 
@@ -111,11 +110,11 @@ def wbr_wrap(value: str | None) -> str:
 
     # Split into two halves, each half has internal <wbr> breaks
     midpoint = len(chunks) // 2
-    first_half = "<wbr>".join(chunks[:midpoint])
-    second_half = "<wbr>".join(chunks[midpoint:])
+    first_half = format_html_join(mark_safe("<wbr>"), "{}", chunks[:midpoint])
+    second_half = format_html_join(mark_safe("<wbr>"), "{}", chunks[midpoint:])
 
-    return mark_safe(
-        f'<span class="checksum-half">{first_half}</span><wbr><span class="checksum-half">{second_half}</span>'
+    return format_html(
+        '<span class="checksum-half">{}</span><wbr><span class="checksum-half">{}</span>', first_half, second_half
     )
 
 

apps/sponsors/admin.py

@@ -1,6 +1,7 @@
 """Django admin configuration for the sponsors app."""
 
 import contextlib
+from textwrap import dedent
 
 from django.contrib import admin
 from django.contrib.contenttypes.admin import GenericTabularInline
@@ -12,7 +13,7 @@
 from django.template import Context, Template
 from django.urls import path, reverse
 from django.utils.functional import cached_property
-from django.utils.html import mark_safe
+from django.utils.html import format_html, format_html_join, mark_safe
 from import_export import resources
 from import_export.admin import ImportExportActionModelAdmin
 from import_export.fields import Field
@@ -296,12 +297,12 @@ def get_benefit_split(self, obj: SponsorshipPackage) -> str:
         for i, (name, pct) in enumerate(split):
             pct_str = f"{pct:.0f}%"
             widths.append(pct_str)
-            spans.append(f"<span title='{name}' style='background-color:{colors[i]}'>{pct_str}</span>")
+            spans.append((name, colors[i], pct_str))
         # define a style that will show our span elements like a single horizontal stacked bar chart
         style = f"color:#fff;text-align:center;cursor:pointer;display:grid;grid-template-columns:{' '.join(widths)}"
+        split_bar = format_html_join("", "<span title='{}' style='background-color:{}'>{}</span>", spans)
         # wrap it all up and put a bow on it
-        html = f"<div style='{style}'>{''.join(spans)}</div>"
-        return mark_safe(html)
+        return format_html("<div style='{}'>{}</div>", style, split_bar)
 
 
 class SponsorContactInline(admin.TabularInline):
@@ -325,7 +326,7 @@ class SponsorshipsInline(admin.TabularInline):
     def link(self, obj):
         """Return a link to the sponsorship change page."""
         url = reverse("admin:sponsors_sponsorship_change", args=[obj.id])
-        return mark_safe(f"<a href={url}>{obj.id}</a>")
+        return format_html("<a href='{}'>{}</a>", url, obj.id)
 
 
 @admin.register(Sponsor)
@@ -652,27 +653,25 @@ def get_readonly_fields(self, request, obj):
     def sponsor_link(self, obj):
         """Return an HTML link to the sponsor's admin change page."""
         url = reverse("admin:sponsors_sponsor_change", args=[obj.sponsor.id])
-        return mark_safe(f"<a href={url}>{obj.sponsor.name}</a>")
+        return format_html("<a href='{}'>{}</a>", url, obj.sponsor.name)
 
     @admin.display(description="Estimated cost")
     def get_estimated_cost(self, obj):
         """Return the estimated cost HTML for customized sponsorships."""
-        cost = None
         html = "This sponsorship has not customizations so there's no estimated cost"
         if obj.for_modified_package:
             msg = "This sponsorship has customizations and this cost is a sum of all benefit's internal values from when this sponsorship was created"
             cost = intcomma(obj.estimated_cost)
-            html = f"{cost} USD <br/><b>Important: </b> {msg}"
-        return mark_safe(html)
+            html = format_html("{} USD <br/><b>Important: </b> {}", cost, msg)
+        return html
 
     @admin.display(description="Contract")
     def get_contract(self, obj):
         """Return an HTML link to the contract or a placeholder."""
         if not obj.contract:
             return "---"
         url = reverse("admin:sponsors_contract_change", args=[obj.contract.pk])
-        html = f"<a href='{url}' target='_blank'>{obj.contract}</a>"
-        return mark_safe(html)
+        return format_html("<a href='{}' target='_blank'>{}</a>", url, obj.contract)
 
     def get_urls(self):
         """Register custom admin URLs for sponsorship workflow actions."""
@@ -741,19 +740,20 @@ def get_sponsor_web_logo(self, obj):
         template = Template(html)
         context = Context({"sponsor": obj.sponsor})
         html = template.render(context)
-        return mark_safe(html)
+        return mark_safe(html)  # noqa: S308
 
     @admin.display(description="Print Logo")
     def get_sponsor_print_logo(self, obj):
         """Render and return the sponsor's print logo as a thumbnail image."""
         img = obj.sponsor.print_logo
-        html = ""
+        html = "---"
         if img:
-            html = "{% load thumbnail %}{% thumbnail img '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}"
-            template = Template(html)
+            template = Template(
+                "{% load thumbnail %}{% thumbnail img '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}"
+            )
             context = Context({"img": img})
-            html = template.render(context)
-        return mark_safe(html) if html else "---"
+            html = mark_safe(template.render(context))  # noqa: S308
+        return html
 
     @admin.display(description="Primary Phone")
     def get_sponsor_primary_phone(self, obj):
@@ -764,18 +764,24 @@ def get_sponsor_primary_phone(self, obj):
     def get_sponsor_mailing_address(self, obj):
         """Return the sponsor's formatted mailing address as HTML."""
         sponsor = obj.sponsor
-        city_row = f"{sponsor.city} - {sponsor.get_country_display()} ({sponsor.country})"
         if sponsor.state:
-            city_row = f"{sponsor.city} - {sponsor.state} - {sponsor.get_country_display()} ({sponsor.country})"
+            city_row_html = format_html(
+                "<p>{} - {} - {} ({})</p>", sponsor.city, sponsor.state, sponsor.get_country_display(), sponsor.country
+            )
+        else:
+            city_row_html = format_html(
+                "<p>{} - {} ({})</p>", sponsor.city, sponsor.get_country_display(), sponsor.country
+            )
 
-        mail_row = sponsor.mailing_address_line_1
         if sponsor.mailing_address_line_2:
-            mail_row += f" - {sponsor.mailing_address_line_2}"
+            mail_row_html = format_html(
+                "<p>{} - {}</p>", sponsor.mailing_address_line_1, sponsor.mailing_address_line_2
+            )
+        else:
+            mail_row_html = format_html("<p>{}</p>", sponsor.mailing_address_line_1)
 
-        html = f"<p>{city_row}</p>"
-        html += f"<p>{mail_row}</p>"
-        html += f"<p>{sponsor.postal_code}</p>"
-        return mark_safe(html)
+        postal_code_row = format_html("<p>{}</p>", sponsor.postal_code)
+        return city_row_html + mail_row_html + postal_code_row
 
     @admin.display(description="Contacts")
     def get_sponsor_contacts(self, obj):
@@ -785,14 +791,14 @@ def get_sponsor_contacts(self, obj):
         primary = [c for c in contacts if c.primary]
         not_primary = [c for c in contacts if not c.primary]
         if primary:
-            html = "<b>Primary contacts</b><ul>"
-            html += "".join([f"<li>{c.name}: {c.email} / {c.phone}</li>" for c in primary])
-            html += "</ul>"
+            html = mark_safe("<b>Primary contacts</b><ul>")
+            html += format_html_join("", "<li>{}: {} / {}</li>", [(c.name, c.email, c.phone) for c in primary])
+            html += mark_safe("</ul>")
         if not_primary:
-            html += "<b>Other contacts</b><ul>"
-            html += "".join([f"<li>{c.name}: {c.email} / {c.phone}</li>" for c in not_primary])
-            html += "</ul>"
-        return mark_safe(html)
+            html += mark_safe("<b>Other contacts</b><ul>")
+            html += format_html_join("", "<li>{}: {} / {}</li>", [(c.name, c.email, c.phone) for c in not_primary])
+            html += mark_safe("</ul>")
+        return html
 
     @admin.display(description="Added by User")
     def get_custom_benefits_added_by_user(self, obj):
@@ -801,8 +807,7 @@ def get_custom_benefits_added_by_user(self, obj):
         if not benefits:
             return "---"
 
-        html = "".join([f"<p>{b}</p>" for b in benefits])
-        return mark_safe(html)
+        return format_html_join("", "<p>{}</p>", benefits)
 
     @admin.display(description="Removed by User")
     def get_custom_benefits_removed_by_user(self, obj):
@@ -811,8 +816,7 @@ def get_custom_benefits_removed_by_user(self, obj):
         if not benefits:
             return "---"
 
-        html = "".join([f"<p>{b}</p>" for b in benefits])
-        return mark_safe(html)
+        return format_html_join("", "<p>{}</p>", benefits)
 
     def rollback_to_editing_view(self, request, pk):
         """Delegate to the rollback_to_editing admin view."""
@@ -878,18 +882,25 @@ def links(self, obj):
         benefits_url = reverse("admin:sponsors_sponsorshipbenefit_changelist")
         preview_label = "View sponsorship application"
         year = obj.year
-        html = "<ul>"
         preview_querystring = f"config_year={year}"
         preview_url = f"{application_url}?{preview_querystring}"
         filter_querystring = f"year={year}"
         year_benefits_url = f"{benefits_url}?{filter_querystring}"
         year_packages_url = f"{benefits_url}?{filter_querystring}"
 
-        html += f"<li><a target='_blank' href='{year_packages_url}'>List packages</a>"
-        html += f"<li><a target='_blank' href='{year_benefits_url}'>List benefits</a>"
-        html += f"<li><a target='_blank' href='{preview_url}'>{preview_label}</a>"
-        html += "</ul>"
-        return mark_safe(html)
+        return format_html(
+            dedent("""
+            <ul>
+                <li><a target='_blank' href='{year_packages_url}'>List packages</a>
+                <li><a target='_blank' href='{year_benefits_url}'>List benefits</a>
+                <li><a target='_blank' href='{preview_url}'>{preview_label}</a>
+            </ul>
+            """),
+            year_packages_url=year_packages_url,
+            year_benefits_url=year_benefits_url,
+            preview_url=preview_url,
+            preview_label=preview_label,
+        )
 
     @admin.display(description="Other configured years")
     def other_years(self, obj):
@@ -904,22 +915,32 @@ def other_years(self, obj):
         application_url = reverse("select_sponsorship_application_benefits")
         benefits_url = reverse("admin:sponsors_sponsorshipbenefit_changelist")
         preview_label = "View sponsorship application form for this year"
-        html = "<ul>"
+        html = mark_safe("<ul>")
         for year in configured_years:
             preview_querystring = f"config_year={year}"
             preview_url = f"{application_url}?{preview_querystring}"
             filter_querystring = f"year={year}"
             year_benefits_url = f"{benefits_url}?{filter_querystring}"
             year_packages_url = f"{benefits_url}?{filter_querystring}"
 
-            html += f"<li><b>{year}</b>:"
-            html += "<ul>"
-            html += f"<li><a target='_blank' href='{year_packages_url}'>List packages</a>"
-            html += f"<li><a target='_blank' href='{year_benefits_url}'>List benefits</a>"
-            html += f"<li><a target='_blank' href='{preview_url}'>{preview_label}</a>"
-            html += "</ul></li>"
-        html += "</ul>"
-        return mark_safe(html)
+            html += format_html(
+                dedent("""
+            <li><b>{year}</b>:"
+                <ul>
+                    <li><a target='_blank' href='{year_packages_url}'>List packages</a>
+                    <li><a target='_blank' href='{year_benefits_url}'>List benefits</a>
+                    <li><a target='_blank' href='{preview_url}'>{preview_label}</a>
+                </ul>
+            </li>
+            """),
+                year=year,
+                year_packages_url=year_packages_url,
+                year_benefits_url=year_benefits_url,
+                preview_url=preview_url,
+                preview_label=preview_label,
+            )
+        html += mark_safe("</ul>")
+        return html
 
     def clone_application_config(self, request):
         """Delegate to the clone_application_config admin view."""
@@ -1042,17 +1063,16 @@ def document_link(self, obj):
             msg = "Download Signed Contract"
 
         if url and msg:
-            html = f'<a href="{url}" target="_blank">{msg}</a>'
-        return mark_safe(html)
+            html = format_html('<a href="{}" target="_blank">{}</a>', url, msg)
+        return html
 
     @admin.display(description="Sponsorship")
     def get_sponsorship_url(self, obj):
         """Return an HTML link to the related sponsorship's admin page."""
         if not obj.sponsorship:
             return "---"
         url = reverse("admin:sponsors_sponsorship_change", args=[obj.sponsorship.pk])
-        html = f"<a href='{url}' target='_blank'>{obj.sponsorship}</a>"
-        return mark_safe(html)
+        return format_html("<a href='{}' target='_blank'>{}</a>", url, obj.sponsorship)
 
     def get_urls(self):
         """Register custom admin URLs for contract workflow actions."""
@@ -1257,8 +1277,8 @@ def get_value(self, obj):
         """Return the asset value, linking to the file URL if applicable."""
         html = obj.value
         if obj.value and getattr(obj.value, "url", None):
-            html = f"<a href='{obj.value.url}' target='_blank'>{obj.value}</a>"
-        return mark_safe(html)
+            html = format_html("<a href='{}' target='_blank'>{}</a>", (obj.value.url, obj.value))
+        return html
 
     @admin.display(description="Associated with")
     def get_related_object(self, obj):
@@ -1276,8 +1296,7 @@ def get_related_object(self, obj):
         if not content_object:  # safety belt
             return obj.content_object
 
-        html = f"<a href='{content_object.admin_url}' target='_blank'>{content_object}</a>"
-        return mark_safe(html)
+        return format_html("<a href='{}' target='_blank'>{}</a>", content_object.admin_url, content_object)
 
     @admin.action(description="Export selected")
     def export_assets_as_zipfile(self, request, queryset):

apps/sponsors/forms.py

@@ -10,7 +10,7 @@
 from django.db.models import Q
 from django.utils import timezone
 from django.utils.functional import cached_property
-from django.utils.safestring import mark_safe
+from django.utils.html import format_html
 from django.utils.text import slugify
 from django.utils.translation import gettext_lazy as _
 from django_countries.fields import CountryField
@@ -751,11 +751,11 @@ def __init__(self, *args, **kwargs):
             field = required_asset.as_form_field(required=required, initial=value)
 
             if required_asset.due_date and not bool(value):
-                field.label = mark_safe(
-                    f"<big><b>{field.label}</b></big><br><b>(Required by {required_asset.due_date})</b>"
+                field.label = format_html(
+                    "<big><b>{}</b></big><br><b>(Required by {})</b>", field.label, required_asset.due_date
                 )
             if bool(value):
-                field.label = mark_safe(f"<big><b>{field.label}</b></big><br><small>(Fulfilled, thank you!)</small>")
+                field.label = format_html("<big><b>{}</b></big><br><small>(Fulfilled, thank you!)</small>", field.label)
 
             fields[f_name] = field
 

apps/successstories/admin.py

@@ -35,4 +35,4 @@ def get_list_display(self, request):
     @admin.display(description="View on site")
     def show_link(self, obj):
         """Return a clickable link icon to the story's public page."""
-        return format_html(f'<a href="{obj.get_absolute_url()}">\U0001f517</a>')
+        return format_html('<a href="{}">\U0001f517</a>', obj.get_absolute_url())

pyproject.toml

@@ -141,8 +141,6 @@ ignore = [
     # Boolean args are idiomatic in Django models, forms, and views
     "FBT001",  # boolean-positional-arg-in-function-definition
     "FBT002",  # boolean-default-value-positional-argument
-    # mark_safe is required Django pattern for admin display
-    "S308",    # suspicious-mark-safe-usage
     # Circular imports are resolved with local imports in Django
     "PLC0415", # import-outside-top-level
     # TODO comment formatting is not worth enforcing