Merge pull request #884 from koordinates/html-diff-xss-protection

Protect against XSS in diff-view by using application/json
koordinates · Jul 11, 2023 · d8823a0 · d8823a0
2 parents 8fb2bd6 + ae121ad
commit d8823a0
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@ _When adding new entries to the changelog, please include issue/PR numbers where
 ## 0.14.1 (UNRELEASED)
 
 - Fixes a bug where Git subprocesses (such as git clone) don't prompt the user for credentials or to resolve SSH issues on Windows. [#852](https://github.com/koordinates/kart/issues/852)
+- Better protection against XSS in the HTML diff viewer. [#884](https://github.com/koordinates/kart/pull/884)
 
 ## 0.14.0
 

diff --git a/kart/diff-view.html b/kart/diff-view.html
@@ -103,8 +103,9 @@
             top: 2px;
         }
     </style>
-    <script id="kart-data">const DATA=${geojson_data};</script>
+    <script id="kart-data" type="application/json">${geojson_data}</script>
     <script type="module">
+        const DATA = JSON.parse(document.getElementById('kart-data').textContent);
         const GEOM = '⭔'
 
         function buildMap() {

diff --git a/kart/html_diff_writer.py b/kart/html_diff_writer.py
@@ -1,3 +1,4 @@
+import html
 import json
 import string
 import sys
@@ -56,23 +57,28 @@ def write_diff(self, diff_format=DiffFormat.FULL):
         }
 
         fo = resolve_output_path(self.output_path)
-        fo.write(
-            template.substitute(
-                {
-                    "title": title,
-                    "geojson_data": json.dumps(
-                        all_datasets_geojson, cls=ExtendedJsonEncoder
-                    ),
-                }
-            )
-        )
+        fo.write(self.substitute_into_template(template, title, all_datasets_geojson))
 
         if fo != sys.stdout:
             fo.close()
             webbrowser.open_new(f"file://{self.output_path.resolve()}")
 
         self.write_warnings_footer()
 
+    @classmethod
+    def substitute_into_template(cls, template, title, all_datasets_geojson):
+        return template.substitute(
+            {
+                "title": html.escape(title),
+                "geojson_data": json.dumps(
+                    all_datasets_geojson, cls=ExtendedJsonEncoder
+                )
+                .replace("/", r"\x2f")
+                .replace("<", r"\x3c")
+                .replace(">", r"\x3e"),
+            }
+        )
+
 
 HtmlDiffWriter.filtered_dataset_deltas_as_geojson = (
     GeojsonDiffWriter.filtered_dataset_deltas_as_geojson

diff --git a/tests/test_diff.py b/tests/test_diff.py
@@ -1,17 +1,17 @@
 import functools
 import json
 import re
+import string
 import time
 from pathlib import Path
 
 import html5lib
 import pytest
 
 import kart
-from kart.base_diff_writer import BaseDiffWriter
 from kart.diff_format import DiffFormat
 from kart.diff_structs import Delta, DeltaDiff
-from kart.diff_util import get_file_diff
+from kart.html_diff_writer import HtmlDiffWriter
 from kart.json_diff_writers import JsonLinesDiffWriter
 from kart.geometry import hex_wkb_to_ogr
 from kart.repo import KartRepo
@@ -30,10 +30,10 @@ def _check_html_output(s):
     document = parser.parse(s)
     # find the <script> element containing data
     el = document.find("./head/script[@id='kart-data']")
-    # find the JSON
-    m = re.match(r"\s*const DATA=(.*);\s*$", el.text, flags=re.DOTALL)
+    # Make sure we're parsing it as JSON.
+    assert el.attrib == {"id": "kart-data", "type": "application/json"}
     # validate it
-    return json.loads(m.group(1))
+    return json.loads(el.text)
 
 
 @pytest.mark.parametrize("output_format", DIFF_OUTPUT_FORMATS)
@@ -2098,14 +2098,47 @@ def test_attached_files_patch(data_archive, cli_runner):
             },
         }
 
+
 def test_load_user_provided_html_template(data_archive, cli_runner):
     with data_archive("points") as repo_path:
         r = cli_runner.invoke(
             [
                 "diff",
                 f"--output-format=html",
-                f"--html-template=" + str(Path(__file__).absolute().parent.parent / "kart" / "diff-view.html"),
+                f"--html-template="
+                + str(
+                    Path(__file__).absolute().parent.parent / "kart" / "diff-view.html"
+                ),
                 "HEAD^...",
             ]
         )
         assert r.exit_code == 0, r.stderr
+
+
+def test_xss_protection():
+    TEMPLATE = """
+<html>
+  <head>
+    <title>Kart Diff: ${title}</title>
+    <script type="application/json">${geojson_data}</script>
+  </head>
+  <body>...</body>
+</html>
+""".lstrip()
+    html_xss = "<script>alert(1);</script>"
+    json_xss = {"key": "</script><script>alert(1);</script>"}
+    result = HtmlDiffWriter.substitute_into_template(
+        string.Template(TEMPLATE), html_xss, json_xss
+    )
+
+    EXPECTED_RESULT = """
+<html>
+  <head>
+    <title>Kart Diff: &lt;script&gt;alert(1);&lt;/script&gt;</title>
+    <script type="application/json">{"key": "\\x3c\\x2fscript\\x3e\\x3cscript\\x3ealert(1);\\x3c\\x2fscript\\x3e"}</script>
+  </head>
+  <body>...</body>
+</html>
+""".lstrip()
+
+    assert result == EXPECTED_RESULT