Improve and test html sanitisation

src/openflexure_microscope_server/ui.py

@@ -3,6 +3,7 @@
 from html import escape
 from html.parser import HTMLParser
 from typing import Annotated, Any, Literal, Optional
+from urllib.parse import urlparse
 
 from pydantic import BaseModel, BeforeValidator, RootModel
 
@@ -23,6 +24,25 @@ ALLOWED_TAGS = {
     "a",
 }
 
+ALLOWED_SCHEMES = {"http", "https"}
+
+
+def strip_control_chars(input_string: str) -> str:
+    """Remove unprintable characters."""
+    return "".join(char for char in input_string if char.isprintable())
+
+
+def sanitize_http_url(url: str) -> Optional[str]:
+    """Santitise any url only allowing http and https without queries."""
+    url = strip_control_chars(url)
+    parsed = urlparse(url)
+    if parsed.scheme not in ALLOWED_SCHEMES:
+        return None
+    if not parsed.netloc:
+        return None
+    fragment_str = "#" + parsed.fragment if parsed.fragment else ""
+    return f"{parsed.scheme}://{parsed.netloc}{fragment_str}"
+
 
 class SafeHTMLParser(HTMLParser):
     """An HTMLParser that only allows a very limited subset of HTML."""
@@ -53,7 +73,8 @@ class SafeHTMLParser(HTMLParser):
             href = None
             for attr, value in attrs:
                 if attr == "href" and value is not None:
-                    href = value.strip()
+                    # If it is not a httplink this will stay as None
+                    href = sanitize_http_url(value.strip())
 
             if href:
                 safe_href = escape(href, quote=True)
@@ -92,7 +113,7 @@ class SafeHTMLParser(HTMLParser):
         self.output += f"&#{name};"
 
 
-def sanitize_html(html: str) -> str:
+def sanitise_html(html: str) -> str:
     """Santitise HTML to only have a small list of allowed tags.
 
     Tags allowed without attrs: ``<p>, <i>, <b>, <h1>, <h2>, <h3>, <h4>, <h5>, <h6>,``
@@ -113,7 +134,7 @@ def sanitize_html(html: str) -> str:
     return parser.output
 
 
-HtmlFragment = Annotated[str, BeforeValidator(sanitize_html)]
+HtmlFragment = Annotated[str, BeforeValidator(sanitise_html)]
 
 
 class HTMLBlock(BaseModel):

tests/shared_utils/md_test_cases.py

@@ -0,0 +1,109 @@
+"""Read multi-line test cases from markdown files.
+
+When checking if text coercion is working multiline Python can be very difficult
+to read and maintain. This file allows the input and expected results to be written
+into markdown codeblocks.
+
+Each test is a second level heading.
+"""
+
+import re
+
+# Level 2 markdown heading. Group 1 is the text
+CASE_REGEX = re.compile(r"^##\s(.*)", re.MULTILINE)
+# Level 3 markdown heading. Group 1 is the text
+LEVEL3_REGEX = re.compile(r"^###\s(.*)", re.MULTILINE)
+# Markdown code block.
+# Group 1 is the language specification after the back ticks
+# Group 2 is the code
+CODE_REGEX = re.compile(
+    r"^```([a-zA-Z]+)?\s?\n(?:(.*?)\n)?```", re.MULTILINE | re.DOTALL
+)
+
+
+class MdTestCase:
+    """A class to represent a test case read from a markdown file.
+
+    Useful attributes:
+
+    * title - The title of the test case
+    * input_code - the input code for the test case
+    * result_code - the expected result code for the test case
+    """
+
+    def __init__(self, title: str, case_text: str, block_type: str) -> None:
+        """Initialise the test case.
+
+        :param title: The title of the test case.
+        :param case_text: The full text of the test case.
+        :param block_type: The type of code block expected (the label after the triple
+            backticks).
+        """
+        self.title = title
+        self.block_type = block_type
+        self.input_code, self.result_code = self._parse(case_text)
+
+    def __repr__(self):
+        """Return the string representation of the test case."""
+        return f"<MdTestCase: {self.title}>"
+
+    def _parse(self, case_text: str) -> tuple[str, str]:
+        """Parse the case text and return the input and expected return code."""
+        h3_matches = list(LEVEL3_REGEX.finditer(case_text))
+        n_h3_matches = len(h3_matches)
+        if n_h3_matches != 2:
+            raise ValueError(
+                f"Case {self.title} has {n_h3_matches} level 3 headings. 2 expected."
+            )
+        if h3_matches[0].group(1) != "Input" or h3_matches[1].group(1) != "Result":
+            raise ValueError(
+                f"Case {self.title} does not have `Input` and `Result` headings."
+            )
+
+        input_section = case_text[h3_matches[0].end() : h3_matches[1].start()]
+        result_section = case_text[h3_matches[1].end() : None]
+
+        input_code_blocks = list(CODE_REGEX.finditer(input_section))
+        result_code_blocks = list(CODE_REGEX.finditer(result_section))
+        if len(input_code_blocks) != 1 or len(result_code_blocks) != 1:
+            raise ValueError(
+                f"Case {self.title} must have exactly 1 code block per section."
+            )
+
+        input_code_type = input_code_blocks[0].group(1)
+        input_code = input_code_blocks[0].group(2)
+        result_code_type = result_code_blocks[0].group(1)
+        result_code = result_code_blocks[0].group(2)
+        if input_code_type != self.block_type or result_code_type != self.block_type:
+            raise ValueError(
+                f"Case {self.title}: each code block must be labeled as "
+                f"{self.block_type}"
+            )
+
+        # If the code block is empty Regex returns None. Coerce to empty string.
+        input_code = "" if input_code is None else input_code.strip()
+        result_code = "" if result_code is None else result_code.strip()
+        return input_code, result_code
+
+
+def find_test_cases(path: str, block_type: str) -> list[MdTestCase]:
+    """Return all test cases in the markdown file.
+
+    :param path: Path to the markdown file containing cases.
+    :param block_type: The label for each triple backtick code block.
+
+    :return: A list of MdTestCase objects for each case.
+    """
+    with open(path, "r", encoding="utf-8") as file_obj:
+        text = file_obj.read()
+    test_case_headings = list(CASE_REGEX.finditer(text))
+    test_cases = []
+    n_headings = len(test_case_headings)
+    for i, heading_match in enumerate(test_case_headings):
+        title = heading_match.group(1)
+        start = heading_match.end()
+        end = test_case_headings[i + 1].start() if i + 1 < n_headings else None
+
+        case_text = text[start:end]
+        test_cases.append(MdTestCase(title, case_text, block_type))
+    return test_cases

tests/unit_tests/data/html_sanitisation_test_cases.md

@@ -0,0 +1,84 @@
+# HTML Sanitisation Test Cases
+
+This file is in markdown not python for readability.
+
+Each test is a level 2 heading. The input and expected values are level 3 headings.
+
+The HTML to test is in triple backtick code blocks.
+
+Anything not in code blocks is a comment.
+
+## Test scripts removed
+
+The script is removed but the contents remain.
+
+### Input
+```html
+<script>alert("x")</script>
+<p>Hello</p>
+```
+### Result
+```html
+alert(&quot;x&quot;)
+<p>Hello</p>
+```
+## Test script in bold tag
+
+### Input
+```html
+<b onmouseover=alert('Wufff!')>click me!</b>
+```
+### Result
+```html
+<b>click me!</b>
+```
+
+## Test script in disallowed tag
+### Input
+```html
+<img src="http://url.to.file.which/not.exist" onerror=alert(document.cookie);>
+```
+### Result
+```html
+```
+
+## Test cookie grabber script is escaped
+The script should become escaped and have no tags
+### Input
+```html
+<SCRIPT type="text/javascript">
+var adr = '../evil.php?cakemonster=' + escape(document.cookie);
+</SCRIPT>
+```
+### Result
+```html
+var adr = &#x27;../evil.php?cakemonster=&#x27; + escape(document.cookie);
+```
+
+## Test with non-http links
+Remove the javascript from the links.
+### Input
+```html
+<a href="javascript:alert(1)">link</a>
+<a href="javas&#99;ript:alert(1)">encoded</a>
+<a href="mailto:spammy@spam.com">Email</a>
+<a href="/a/relateive/link">Relative</a>
+```
+### Result
+```html
+<a>link</a>
+<a>encoded</a>
+<a>Email</a>
+<a>Relative</a>
+```
+
+## Test http links become external
+Remove the javascript from the links.
+### Input
+```html
+<a href="https://openflexure.org">OpenFlexure</a>
+```
+### Result
+```html
+<a href="https://openflexure.org" target="_blank" rel="noopener noreferrer">OpenFlexure</a>
+```

tests/unit_tests/test_ui.py

@@ -0,0 +1,24 @@
+"""Testing the server specified user interface."""
+
+import os
+
+import pytest
+
+from openflexure_microscope_server import ui
+
+from ..shared_utils.md_test_cases import find_test_cases
+
+THIS_DIR = os.path.dirname(os.path.abspath(__file__))
+HTML_SANITISATION_TEST_FILE = os.path.join(
+    THIS_DIR, "data", "html_sanitisation_test_cases.md"
+)
+HTML_SANITISATION_TEST_CASES = find_test_cases(HTML_SANITISATION_TEST_FILE, "html")
+
+
+@pytest.mark.parametrize("test_case", HTML_SANITISATION_TEST_CASES)
+def test_html_block_sanitises(test_case):
+    """Check each case in the test case file sanitises as expected."""
+    sanitised = ui.sanitise_html(test_case.input_code).strip()
+    assert sanitised == test_case.result_code, (
+        f"Test case: '{test_case.title}': Input code doesn't sanitise as expected."
+    )