From bfc83d8ff9a5825a27eb44e43d9faf4d1f438915 Mon Sep 17 00:00:00 2001
From: David D Lowe <daviddlowe.flimm@gmail.com>
Date: Tue, 27 Aug 2024 14:22:50 +0100
Subject: [PATCH] Documented risk of XSS vulnerability when using Postgres
 headlines.

Because the default start and stop parameters are <b> and </b>
respectively, it is tempting to pass the headline value to the `safe`
template filter, to render the highlighted section of the headline in
bold. This is dangerous.

Also, tested the sanitation behavior of Postgres. If the undocumented
behavior of Postgres changes in this regard, we want to ensure that
Django's code and documentation is updated appropriately.
---
 docs/ref/contrib/postgres/search.txt | 13 ++++++++++
 tests/postgres_tests/test_search.py  | 37 ++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/docs/ref/contrib/postgres/search.txt b/docs/ref/contrib/postgres/search.txt
index 220ab4c591..e1459847f1 100644
--- a/docs/ref/contrib/postgres/search.txt
+++ b/docs/ref/contrib/postgres/search.txt
@@ -218,6 +218,19 @@ Usage example:
 See :ref:`postgresql-fts-search-configuration` for an explanation of the
 ``config`` parameter.
 
+.. warning::
+
+    In order to prevent XSS attacks, the string value of this field is not
+    marked as safe. Although the ``start_sel`` and ``stop_sel`` parameters
+    default to ``<b>`` and ``</b>``, it is dangerous to mark the headline as
+    safe or to pass it to the :tfilter:`safe` template filter, especially if
+    the value is from an untrusted source.
+
+    Tested versions of PostgreSQL will not remove or escape HTML tags from the
+    headline if ``highlight_all`` is ``True``. If ``highlight_all`` is
+    ``False``, then HTML tags will be removed from the headline. This behavior
+    is undocumented, and must not be relied upon.
+
 .. _highlighting search results: https://www.postgresql.org/docs/current/textsearch-controls.html#TEXTSEARCH-HEADLINE
 
 .. _postgresql-fts-search-configuration:
diff --git a/tests/postgres_tests/test_search.py b/tests/postgres_tests/test_search.py
index 472dca6c7b..2ff2ab5c4a 100644
--- a/tests/postgres_tests/test_search.py
+++ b/tests/postgres_tests/test_search.py
@@ -7,6 +7,7 @@ transcript.
 """
 
 from django.db.models import F, Value
+from django.utils.safestring import SafeString
 
 from . import PostgreSQLSimpleTestCase, PostgreSQLTestCase
 from .models import Character, Line, LineSavedSearch, Scene
@@ -732,6 +733,42 @@ class SearchHeadlineTests(GrailTestData, PostgreSQLTestCase):
             searched.headline,
         )
 
+    def test_headline_sanitize_html(self):
+        dangerous_line = Line.objects.create(
+            scene=self.robin,
+            character=self.minstrel,
+            dialogue='Foobar <script>console.log("danger");</script>',
+        )
+        dangerous_line.full_clean()
+
+        searched = Line.objects.annotate(
+            headline=SearchHeadline(
+                "dialogue",
+                SearchQuery("Foobar", config="english"),
+                highlight_all=False,
+            ),
+        ).get(pk=dangerous_line.pk)
+        self.assertNotIsInstance(searched.headline, SafeString)
+        self.assertEqual(
+            searched.headline,
+            '<b>Foobar</b>  console.log("danger"); ',
+            "When hightlight_all is False, PostgreSQL removes existing HTML tags",
+        )
+
+        searched = Line.objects.annotate(
+            headline=SearchHeadline(
+                "dialogue",
+                SearchQuery("Foobar", config="english"),
+                highlight_all=True,
+            ),
+        ).get(pk=dangerous_line.pk)
+        self.assertNotIsInstance(searched.headline, SafeString)
+        self.assertEqual(
+            searched.headline,
+            '<b>Foobar</b> <script>console.log("danger");</script>',
+            "When highlight_all is True, PostgreSQL riskily keeps existing HTML tags",
+        )
+
     def test_headline_short_word_option(self):
         self.check_default_text_search_config()
         searched = Line.objects.annotate(