Browse Source

fix(security): Use format_html for avatar related functions (#58319)

Continuation of https://github.com/getsentry/sentry/pull/58227

According to Django docs:

https://docs.djangoproject.com/en/4.2/ref/utils/#django.utils.html.format_html

> For the case of building up small HTML fragments, this function is to
be preferred over string interpolation using `%` or `str.format()`
directly, because it applies escaping to all arguments - just like the
template system applies escaping by default.
> So, instead of writing:
> 
> ```
> mark_safe(
>     "%s <b>%s</b> %s"
>     % (
>         some_html,
>         escape(some_text),
>         escape(some_other_text),
>     )
> )
> ```
> You should instead use:
> 
> ```
> format_html(
>     "{} <b>{}</b> {}",
>     mark_safe(some_html),
>     some_text,
>     some_other_text,
> )
> ```
Alexander Tarasov 1 year ago
parent
commit
df7a1bec19

+ 2 - 2
src/sentry/notifications/notifications/activity/base.py

@@ -6,7 +6,7 @@ from typing import TYPE_CHECKING, Any, Mapping, MutableMapping
 from urllib.parse import urlparse, urlunparse
 
 from django.utils.html import format_html
-from django.utils.safestring import SafeString, mark_safe
+from django.utils.safestring import SafeString
 
 from sentry.db.models import Model
 from sentry.notifications.helpers import get_reason_context
@@ -186,7 +186,7 @@ class GroupActivityNotification(ActivityNotification, abc.ABC):
 
         fmt = '<span class="avatar-container">{}</span> <strong>{}</strong>'
 
-        author = format_html(fmt, mark_safe(avatar_as_html(self.user)), name)
+        author = format_html(fmt, avatar_as_html(self.user), name)
 
         issue_name = self.group.qualified_short_id or "an issue"
         an_issue = format_html('<a href="{}">{}</a>', self.get_group_link(), issue_name)

+ 6 - 5
src/sentry/notifications/utils/avatar.py

@@ -1,7 +1,8 @@
 from __future__ import annotations
 
 from django.urls import reverse
-from django.utils.html import escape
+from django.utils.html import format_html
+from django.utils.safestring import SafeString
 
 from sentry.models.avatars.user_avatar import UserAvatar
 from sentry.models.user import User
@@ -37,14 +38,14 @@ def get_sentry_avatar_url() -> str:
     return str(absolute_uri(get_asset_url("sentry", url)))
 
 
-def avatar_as_html(user: User | RpcUser) -> str:
+def avatar_as_html(user: User | RpcUser) -> SafeString:
     if not user:
-        return '<img class="avatar" src="{}" width="20px" height="20px" />'.format(
-            escape(get_sentry_avatar_url())
+        return format_html(
+            '<img class="avatar" src="{}" width="20px" height="20px" />', get_sentry_avatar_url()
         )
     avatar_type = user.get_avatar_type()
     if avatar_type == "upload":
-        return f'<img class="avatar" src="{escape(get_user_avatar_url(user))}" />'
+        return format_html('<img class="avatar" src="{}" />', get_user_avatar_url(user))
     elif avatar_type == "letter_avatar":
         return get_email_avatar(user.get_display_name(), user.get_label(), 20, False)
     else:

+ 2 - 3
src/sentry/templatetags/sentry_avatars.py

@@ -2,7 +2,6 @@ from urllib.parse import urlencode
 
 from django import template
 from django.urls import reverse
-from django.utils.safestring import mark_safe
 
 from sentry.models.avatars.user_avatar import UserAvatar
 from sentry.models.user import User
@@ -23,7 +22,7 @@ def gravatar_url(context, email, size, default="mm"):
 
 @register.simple_tag(takes_context=True)
 def letter_avatar_svg(context, display_name, identifier, size=None):
-    return mark_safe(get_letter_avatar(display_name, identifier, size=size))
+    return get_letter_avatar(display_name, identifier, size=size)
 
 
 @register.simple_tag(takes_context=True)
@@ -42,7 +41,7 @@ def profile_photo_url(context, user_id, size=None):
 # than 1-2 avatars. It will make a request for every user!
 @register.simple_tag(takes_context=True)
 def email_avatar(context, display_name, identifier, size=None, try_gravatar=True):
-    return mark_safe(get_email_avatar(display_name, identifier, size, try_gravatar))
+    return get_email_avatar(display_name, identifier, size, try_gravatar)
 
 
 @register.inclusion_tag("sentry/partial/avatar.html")

+ 8 - 11
src/sentry/templatetags/sentry_platforms.py

@@ -1,5 +1,4 @@
 from django import template
-from django.utils.safestring import mark_safe
 
 from sentry.utils.avatar import get_letter_avatar, get_platform_avatar
 
@@ -8,18 +7,16 @@ register = template.Library()
 
 @register.simple_tag(takes_context=True)
 def letter_platform_svg(context, display_name, identifier, size=None, rounded=False):
-    return mark_safe(
-        get_letter_avatar(
-            display_name,
-            identifier,
-            size,
-            use_svg=False,
-            initials=display_name[0:2] if display_name else None,
-            rounded=rounded,
-        )
+    return get_letter_avatar(
+        display_name,
+        identifier,
+        size,
+        use_svg=False,
+        initials=display_name[0:2] if display_name else None,
+        rounded=rounded,
     )
 
 
 @register.simple_tag(takes_context=True)
 def platform_avatar(context, display_name, size=36):
-    return mark_safe(get_platform_avatar(display_name, size))
+    return get_platform_avatar(display_name, size)

+ 19 - 12
src/sentry/utils/avatar.py

@@ -11,7 +11,8 @@ from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.core.validators import validate_email
 from django.utils.encoding import force_str
-from django.utils.html import escape
+from django.utils.html import escape, format_html
+from django.utils.safestring import SafeString
 from PIL import Image
 
 from sentry.http import safe_urlopen
@@ -74,7 +75,7 @@ def get_letter_avatar(
     use_svg: Optional[bool] = True,
     initials: Optional[str] = None,
     rounded: Optional[bool] = False,
-) -> str:
+) -> SafeString:
     display_name = (display_name or "").strip() or "?"
     names = display_name.split(" ")
     initials = initials or "{}{}".format(names[0][0], names[-1][0] if len(names) > 1 else "")
@@ -82,24 +83,26 @@ def get_letter_avatar(
     color = get_letter_avatar_color(identifier)
     if use_svg:
         size_attrs = f'height="{size}" width="{size}"' if size else ""
-        return (
+        return format_html(
             '<svg viewBox="0 0 120 120" xmlns="http://www.w3.org/2000/svg" {size_attrs}>'
             '<rect x="0" y="0" width="120" height="120" rx="15" ry="15" fill={color}></rect>'
             '<text x="50%" y="50%" font-size="65" dominant-baseline="central" text-anchor="middle" fill="#FFFFFF">'
             "{initials}"
             "</text>"
-            "</svg>"
-        ).format(color=color, initials=initials, size_attrs=size_attrs)
+            "</svg>",
+            color=color,
+            initials=initials,
+            size_attrs=size_attrs,
+        )
     else:
         size_attrs = f"height:{size}px;width:{size}px;" if size else ""
         font_size = "font-size:%spx;" % (size / 2) if size else ""
         line_height = "line-height:%spx;" % size if size else ""
         span_class = " rounded" if rounded else ""
-        return (
+        return format_html(
             '<span class="html-avatar{span_class}" '
             'style="background-color:{color};{size_attrs}{font_size}{line_height}">'
-            "{initials}</span>"
-        ).format(
+            "{initials}</span>",
             color=color,
             initials=initials,
             size_attrs=size_attrs,
@@ -114,7 +117,7 @@ def get_email_avatar(
     identifier: str,
     size: Optional[int] = None,
     try_gravatar: Optional[bool] = True,
-) -> str:
+) -> SafeString:
     if try_gravatar:
         try:
             validate_email(identifier)
@@ -129,16 +132,20 @@ def get_email_avatar(
                 if resp.status_code == 200:
                     # default to mm if including in emails
                     gravatar_url = get_gravatar_url(identifier, size=size)
-                    return f'<img class="avatar" src="{gravatar_url}">'
+                    return format_html('<img class="avatar" src="{}">', gravatar_url)
     return get_letter_avatar(display_name, identifier, size, use_svg=False)
 
 
 def get_platform_avatar(
     display_name: Optional[str],
     size: Optional[int] = None,
-) -> str:
+) -> SafeString:
     # TODO: @taylangocmen add platformicons from package when available
-    return f'<img class="avatar" src="https://raw.githubusercontent.com/getsentry/platformicons/master/svg/{display_name}.svg" height={size}>'
+    return format_html(
+        '<img class="avatar" src="https://raw.githubusercontent.com/getsentry/platformicons/master/svg/{display_name}.svg" height={size}>',
+        display_name=display_name,
+        size=size,
+    )
 
 
 def is_black_alpha_only(data: IO[bytes]) -> bool: