Start nh3 sanitization and fixes

Matistjati · Matistjati · commit 5810c318caf5 · 2025-04-05T15:57:58.000+02:00
diff --git a/problemtools/md2html.py b/problemtools/md2html.py
@@ -1,8 +1,10 @@
 #! /usr/bin/env python3
 # -*- coding: utf-8 -*-
+import nh3
 import os.path
-import string
 import argparse
+import html
+import string
 import subprocess
 import re
 import tempfile
@@ -35,13 +37,12 @@ def convert(problem: str, options: argparse.Namespace) -> bool:
     statement_common.foreach_image(statement_path,
                  lambda img_name: copy_image(problem, img_name))
     
-    command = ["pandoc", statement_path, "-t" , "html", "-f", "markdown-raw_html", "--mathjax"]
+    command = ["pandoc", statement_path, "-t" , "html", "--mathjax"]
     statement_html = subprocess.run(command, capture_output=True, text=True,
         shell=False, check=True).stdout
 
 
     templatepaths = [os.path.join(os.path.dirname(__file__), 'templates/markdown_html'),
-                     os.path.join(os.path.dirname(__file__), '../templates/markdown_html'),
                      '/usr/lib/problemtools/templates/markdown_html']
     templatepath = next((p for p in templatepaths
                               if os.path.isdir(p) and os.path.isfile(os.path.join(p, "default-layout.html"))),
@@ -50,30 +51,51 @@ def convert(problem: str, options: argparse.Namespace) -> bool:
     if templatepath is None:
         raise Exception('Could not find directory with markdown templates')
 
-    problem_name = statement_common.get_problem_name(problem, options.language)
+    problem_name = statement_common.get_yaml_problem_name(problem, options.language)
 
-    html_template = _substitute_template(templatepath, "default-layout.html",
+    if problem_name:
+        problem_name = html.escape(problem_name)
+    
+    statement_html = _substitute_template(templatepath, "default-layout.html",
            statement_html=statement_html,
            language=options.language,
            title=problem_name or "Missing problem name",
-           problemid=problembase)
+           problemid=problembase) # No need to escape problem shortname, the spec has tight restrictions directory names
 
     samples = statement_common.format_samples(problem, to_pdf=False)
 
     # Insert samples at {{nextsample}} and {{remainingsamples}}
-    html_template, remaining_samples = statement_common.inject_samples(html_template, samples, "")
+    statement_html, remaining_samples = statement_common.inject_samples(statement_html, samples, "")
 
     # Insert the remaining samples at the bottom
-    if FOOTNOTES_STRING in html_template:
-        pos = html_template.find(FOOTNOTES_STRING)
+    if FOOTNOTES_STRING in statement_html:
+        pos = statement_html.find(FOOTNOTES_STRING)
     else:
-        pos = html_template.find("</body>")
-    html_template = html_template[:pos] + "".join(remaining_samples) + html_template[pos:]
-
-    html_template = replace_hr_in_footnotes(html_template)
+        pos = statement_html.find("</body>")
+    statement_html = statement_html[:pos] + "".join(remaining_samples) + statement_html[pos:]
+
+    statement_html = replace_hr_in_footnotes(statement_html)
+    html_body = statement_html[statement_html.find("<body>"):]
+    statement_html = statement_html[:statement_html.find("<body>")]
+
+    allowed_classes = ("sample", "problemheader", "problembody", "sampleinteractionwrite", "sampleinteractionread",)
+    def attribute_filter(tag, attribute, value):
+        if attribute == "class" and value in allowed_classes:
+            return value
+        if tag == "img" and attribute == "src":
+            return value
+        return None
+    
+    html_body = nh3.clean(html_body,
+        link_rel="noopener nofollow noreferrer",
+        attribute_filter=attribute_filter,
+        tags=nh3.ALLOWED_TAGS | {"img"},
+        attributes={"table": {"class"}, "div": {"class"}, "img": {"src"}},
+    )
+    statement_html += html_body
 
     with open(destfile, "w", encoding="utf-8", errors="xmlcharrefreplace") as output_file:
-        output_file.write(html_template)
+        output_file.write(statement_html)
 
     if options.css:
         with open("problem.css", "w") as output_file:
diff --git a/problemtools/problem2pdf.py b/problemtools/problem2pdf.py
@@ -32,7 +32,6 @@ def md2pdf(options: argparse.Namespace) -> bool:
     statement_common.assert_images_are_valid_md(statement_path)
 
     templatepaths = [os.path.join(os.path.dirname(__file__), 'templates/markdown_pdf'),
-                    os.path.join(os.path.dirname(__file__), '../templates/markdown_pdf'),
                     '/usr/lib/problemtools/templates/markdown_pdf']
     templatepath = next((p for p in templatepaths
                             if os.path.isdir(p) and os.path.isfile(os.path.join(p, "fix_tables.md"))),
@@ -48,7 +47,7 @@ def md2pdf(options: argparse.Namespace) -> bool:
     with open(statement_path, "r") as file:
         statement_md = file.read()
     
-    problem_name = statement_common.get_problem_name(problem_root, options.language)
+    problem_name = statement_common.get_yaml_problem_name(problem_root, options.language)
 
     # Add problem name and id to the top
     problem_id = os.path.basename(problem_root)
@@ -66,7 +65,7 @@ def md2pdf(options: argparse.Namespace) -> bool:
     with tempfile.NamedTemporaryFile(mode='w', suffix=".md") as temp_file:
         temp_file.write(statement_md)
         temp_file.flush()
-        command = ["pandoc", temp_file.name, "-o", destfile, f"--resource-path={statement_dir}", "-f", "markdown-raw_html"]
+        command = ["pandoc", temp_file.name, "-o", destfile, f"--resource-path={statement_dir}"]
         try:
             return subprocess.run(command, capture_output=True, text=True, shell=False, check=True)
         except subprocess.CalledProcessError as e:
diff --git a/problemtools/statement_common.py b/problemtools/statement_common.py
@@ -6,6 +6,7 @@
 import subprocess
 import re
 import json
+import yaml
 from pathlib import Path
 =======
 from typing import Optional
@@ -67,7 +68,7 @@ def json_dfs(data, callback) -> None:
 
 def foreach_image(statement_path, callback):
     # Find all images in the statement and call callback for each one
-    command = ["pandoc", statement_path, "-t" , "json", "-f", "markdown-raw_html"]
+    command = ["pandoc", statement_path, "-t" , "json"]
     statement_json = subprocess.run(command, capture_output=True,
                                     text=True, shell=False, check=True).stdout
     json_dfs(json.loads(statement_json), callback)
@@ -92,19 +93,32 @@ def assert_images_are_valid_md(statement_path: str) -> None:
     foreach_image(statement_path,
                 lambda img_name: assert_image_is_valid(problem_root, img_name))
 
-def get_problem_name(problem: str, language: Optional[str]) -> Optional[str]:
-    """Load problem.yaml to get problem name"""
-    if language is None:
-        language = "en"
-    with verifyproblem.Problem(problem) as prob:
-        config = verifyproblem.ProblemConfig(prob)
-    if not config.check(None):
-        raise Exception("Invalid problem.yaml")
+def get_yaml_problem_name(problem: str, language: Optional[str]) -> Optional[str]:
+
+    # TODO: getting this should be done using verifyproblem
+    # Wait until new config parsing system is in place
+    config_file = Path(problem) / 'problem.yaml'
+    if not config_file.is_file():
+        raise FileNotFoundError(f"No problem.yaml found")
+
+    try:
+        with open(config_file) as f:
+            config = yaml.safe_load(f)
+        if config is None:
+            config = {}
+    except Exception as e:
+        raise Exception(f"Invalid problem.yaml: {e}")
+
+    if 'name' in config and not isinstance(config['name'], dict):
+        config['name'] = {'': config['name']}
+
     names = config.get("name")
     # If there is only one language, per the spec that is the one we want
     if len(names) == 1:
         return next(iter(names.values()))
 
+    if language is None:
+        language = "en"
     if language not in names:
         raise Exception(f"No problem name defined for language {language or 'en'}")
     return names[language]
diff --git a/problemtools/tests/problems/<script>alert("Hello world!");</<script>alert("Hello world!");</script>/problem.yaml b/problemtools/tests/problems/<script>alert("Hello world!");</<script>alert("Hello world!");</script>/problem.yaml
@@ -0,0 +1 @@
+name: Problem ID xss
diff --git a/problemtools/tests/problems/<script>alert("Hello world!");</<script>alert("Hello world!");</script>/problem_statement/problem.md b/problemtools/tests/problems/<script>alert("Hello world!");</<script>alert("Hello world!");</script>/problem_statement/problem.md
@@ -0,0 +1 @@
+
diff --git a/problemtools/tests/problems/problemnamexss/problem.yaml b/problemtools/tests/problems/problemnamexss/problem.yaml
@@ -0,0 +1 @@
+name: <script>alert("Hello world!");</script>
diff --git a/problemtools/tests/problems/problemnamexss/problem_statement/problem.md b/problemtools/tests/problems/problemnamexss/problem_statement/problem.md
@@ -0,0 +1 @@
+XSS injection via problem name.
diff --git a/problemtools/tests/problems/samplexss/data/sample/1.ans b/problemtools/tests/problems/samplexss/data/sample/1.ans
@@ -0,0 +1 @@
+PWNED
diff --git a/problemtools/tests/problems/samplexss/data/sample/1.in b/problemtools/tests/problems/samplexss/data/sample/1.in
@@ -0,0 +1,3 @@
+<script>
+   alert("Hello world!");
+</script>
diff --git a/problemtools/tests/problems/samplexss/data/sample/testdata.yaml b/problemtools/tests/problems/samplexss/data/sample/testdata.yaml
@@ -0,0 +1,5 @@
+on_reject: continue
+range: 0 0
+accept_score: 0
+grader_flags: first_error
+input_validator_flags: nFive=0
diff --git a/problemtools/tests/problems/samplexss/data/testdata.yaml b/problemtools/tests/problems/samplexss/data/testdata.yaml
@@ -0,0 +1,3 @@
+on_reject: continue
+range: 0 2
+grader_flags: ignore_sample
diff --git a/problemtools/tests/problems/samplexss/problem.yaml b/problemtools/tests/problems/samplexss/problem.yaml
@@ -0,0 +1 @@
+name: Sample XSS
diff --git a/problemtools/tests/problems/samplexss/problem_statement/problem.md b/problemtools/tests/problems/samplexss/problem_statement/problem.md
@@ -0,0 +1,27 @@
+Various XSS methods. Hopefully the sanitizer doesn't let any of them through.
+
+
+<script>
+   alert("Hello world!");
+</script>
+
+<img src=x onerror=alert('XSS')>
+
+<a href="#" onclick="alert('XSS')">Click me</a>
+
+<svg onload=alert('XSS')></svg>
+
+<a href="javascript:alert('XSS')">Click me</a>
+
+<input type="text" value="<script>alert('XSS')</script>">
+
+<script>eval('\x61\x6c\x65\x72\x74\x28\x27\x58\x53\x53\x27\x29')</script>
+
+<svg><script>alert('XSS')</script></svg>
+
+<iframe src="javascript:alert('XSS')"></iframe>
+
+<math><mtext><script>alert('XSS')</script></mtext></math>
+
+<div style="background:url(javascript:alert('XSS'))">
+
diff --git a/problemtools/tests/problems/specialcharacterssample/data/sample/1.ans b/problemtools/tests/problems/specialcharacterssample/data/sample/1.ans
@@ -0,0 +1 @@
+Nice!
diff --git a/problemtools/tests/problems/specialcharacterssample/data/sample/1.in b/problemtools/tests/problems/specialcharacterssample/data/sample/1.in
@@ -0,0 +1 @@
+0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~
diff --git a/problemtools/tests/problems/specialcharacterssample/data/sample/testdata.yaml b/problemtools/tests/problems/specialcharacterssample/data/sample/testdata.yaml
@@ -0,0 +1,5 @@
+on_reject: continue
+range: 0 0
+accept_score: 0
+grader_flags: first_error
+input_validator_flags: nFive=0
diff --git a/problemtools/tests/problems/specialcharacterssample/data/testdata.yaml b/problemtools/tests/problems/specialcharacterssample/data/testdata.yaml
@@ -0,0 +1,3 @@
+on_reject: continue
+range: 0 2
+grader_flags: ignore_sample
diff --git a/problemtools/tests/problems/specialcharacterssample/problem.yaml b/problemtools/tests/problems/specialcharacterssample/problem.yaml
@@ -0,0 +1 @@
+name: Special Characters Sample
diff --git a/problemtools/tests/problems/specialcharacterssample/problem_statement/problem.md b/problemtools/tests/problems/specialcharacterssample/problem_statement/problem.md
@@ -0,0 +1 @@
+All printable ASCII characters in sample
diff --git a/problemtools/tests/problems/statementxss/problem.yaml b/problemtools/tests/problems/statementxss/problem.yaml
@@ -0,0 +1 @@
+name: XSS
diff --git a/problemtools/tests/problems/statementxss/problem_statement/problem.md b/problemtools/tests/problems/statementxss/problem_statement/problem.md
@@ -0,0 +1,27 @@
+Various XSS methods. Hopefully the sanitizer doesn't let any of them through.
+
+
+<script>
+   alert("Hello world!");
+</script>
+
+<img src=x onerror=alert('XSS')>
+
+<a href="#" onclick="alert('XSS')">Click me</a>
+
+<svg onload=alert('XSS')></svg>
+
+<a href="javascript:alert('XSS')">Click me</a>
+
+<input type="text" value="<script>alert('XSS')</script>">
+
+<script>eval('\x61\x6c\x65\x72\x74\x28\x27\x58\x53\x53\x27\x29')</script>
+
+<svg><script>alert('XSS')</script></svg>
+
+<iframe src="javascript:alert('XSS')"></iframe>
+
+<math><mtext><script>alert('XSS')</script></mtext></math>
+
+<div style="background:url(javascript:alert('XSS'))">
+
diff --git a/problemtools/tests/test_markdown.py b/problemtools/tests/test_markdown.py
@@ -0,0 +1,8 @@
+from pathlib import Path
+from problemtools.tests.test_xss import render
+
+def test_sample_escaping():
+    problem_path = Path(__file__).parent / "problems" / "specialcharacterssample"
+    html = render(problem_path)
+    all_printable = r"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~"
+    assert all_printable in html
diff --git a/problemtools/tests/test_xss.py b/problemtools/tests/test_xss.py
@@ -0,0 +1,33 @@
+import os
+from pathlib import Path
+from problemtools.problem2html import convert, get_parser
+import tempfile
+
+def render(problem_path):
+    with tempfile.TemporaryDirectory() as temp_dir:
+        args, _unknown = get_parser().parse_known_args(['--problem', str(problem_path.resolve()), '--dest-dir', str(temp_dir)])
+        convert(args)
+        with open(f"{temp_dir}/index.html", "r") as f:
+            html = f.read()
+            return html
+
+def test_no_xss_statement():
+    problem_path = Path(__file__).parent / "problems" / "statementxss"
+    html = render(problem_path)
+    assert "alert" not in html
+    
+def test_no_xss_problemname():
+    problem_path = Path(__file__).parent / "problems" / "problemnamexss"
+    html = render(problem_path)
+    assert "<script>" not in html
+   
+def test_no_xss_sample():
+    problem_path = Path(__file__).parent / "problems" / "samplexss"
+    html = render(problem_path)
+    assert "<script>" not in html
+
+def test_no_xss_problem_id():
+    problem_path = Path(__file__).parent / "problems" / '<script>alert("Hello world!");</script>'
+    html = render(problem_path)
+    assert "<script>" not in html
+

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+name: <script>alert("Hello world!");</script>`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+<script>`
	`2`	`+ alert("Hello world!");`
	`3`	`+</script>`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+on_reject: continue`
	`2`	`+range: 0 2`
	`3`	`+grader_flags: ignore_sample`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{\|}~
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+All printable ASCII characters in sample`