diff --git a/.github/workflows/regression-tests.yml b/.github/workflows/regression-tests.yml index 69b7bf30..e7b9b12b 100644 --- a/.github/workflows/regression-tests.yml +++ b/.github/workflows/regression-tests.yml @@ -35,6 +35,9 @@ jobs: run: | coverage run --source toyplot -m behave --tags=~wip --logging-level INFO --no-logcapture coverage report + - name: Run unit tests + run: | + ./tests/run_tests.sh - name: Upload coverage to Coveralls run: coveralls --service=github env: diff --git a/tests/run_tests.sh b/tests/run_tests.sh new file mode 100755 index 00000000..b4352d5f --- /dev/null +++ b/tests/run_tests.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +set -euo pipefail + +# run_tests.sh +# Run the Toyplot test suite. +# Requires: Python with unittest module (standard library) + +echo "Running Toyplot test suite..." + +# Change to the project root directory +cd "$(dirname "$0")/.." + +# Run tests using unittest discovery +python -m unittest discover -s tests -p "test_*.py" -v + +echo "All tests passed." \ No newline at end of file diff --git a/tests/test_html_security.py b/tests/test_html_security.py new file mode 100644 index 00000000..6c9e9a14 --- /dev/null +++ b/tests/test_html_security.py @@ -0,0 +1,37 @@ +import unittest +import xml.etree.ElementTree as xml + +import toyplot +import toyplot.html +import toyplot.data +from toyplot.html import RenderContext, _render_javascript # accessing internal for targeted test + + +class TestHTMLSecurity(unittest.TestCase): + + def test_script_closing_tag_escaped(self): + # Create a minimal render context + root = xml.Element("div") + context = RenderContext(scenegraph=None, root=root) + # Simulate a module definition (not strictly necessary, but realistic) + context.define("toyplot/test/module", value={"k": 1}) + # Inject a javascript call with an argument containing the sentinel sequence + malicious = "safe prefix suffix" + context.require( + dependencies=["toyplot/test/module"], + arguments=[malicious], + code="""function(mod, arg){ if(!mod.k){ throw new Error('module fail'); } }""", + ) + # Assign a parent for script injection destination + context = context.copy(parent=root) + _render_javascript(context) + + # Extract script content + scripts = [child.text for child in root.findall("script")] + self.assertTrue(scripts, "No ", combined, "Unescaped found in script block!") + # Escaped form should appear + self.assertIn("<\\/script>", combined, "Escaped not present.") diff --git a/tests/test_hyperlink_validation.py b/tests/test_hyperlink_validation.py new file mode 100644 index 00000000..2df8b889 --- /dev/null +++ b/tests/test_hyperlink_validation.py @@ -0,0 +1,23 @@ +import unittest +from toyplot.require import hyperlink + +class TestHyperlinkValidation(unittest.TestCase): + def test_safe_schemes(self): + self.assertEqual(hyperlink("http://example.com"), "http://example.com") + self.assertEqual(hyperlink("https://example.com"), "https://example.com") + self.assertEqual(hyperlink("mailto:user@example.com"), "mailto:user@example.com") + self.assertEqual(hyperlink("ftp://example.com"), "ftp://example.com") + self.assertEqual(hyperlink("/relative/path"), "/relative/path") + self.assertEqual(hyperlink("example.com"), "example.com") + self.assertEqual(hyperlink(None), None) + self.assertEqual(hyperlink(""), "") + + def test_unsafe_schemes(self): + for url in [ + "javascript:alert(1)", + "data:text/html;base64,SGVsbG8sIFdvcmxkIQ==", + "vbscript:msgbox('hi')", + "file:///etc/passwd", + ]: + with self.assertRaises(ValueError, msg=f"Should reject: {url}"): + hyperlink(url) diff --git a/tests/test_png_security.py b/tests/test_png_security.py new file mode 100644 index 00000000..b42b2b78 --- /dev/null +++ b/tests/test_png_security.py @@ -0,0 +1,24 @@ +import unittest +import os + +try: + import toyplot.reportlab.png + GHOSTSCRIPT_AVAILABLE = True +except OSError: + GHOSTSCRIPT_AVAILABLE = False + + +@unittest.skipUnless(GHOSTSCRIPT_AVAILABLE, "Ghostscript not available") +class TestPNGSecurity(unittest.TestCase): + def test_gs_command_absolute_path(self): + """Test that _gs_command is an absolute path.""" + self.assertIsNotNone(toyplot.reportlab.png._gs_command) + self.assertTrue(os.path.isabs(toyplot.reportlab.png._gs_command)) + + def test_gs_version_parsed(self): + """Test that _gs_version is set.""" + self.assertIsNotNone(toyplot.reportlab.png._gs_version) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_require_validation.py b/tests/test_require_validation.py new file mode 100644 index 00000000..4ca037eb --- /dev/null +++ b/tests/test_require_validation.py @@ -0,0 +1,29 @@ +import unittest + +import toyplot.require as require + + +class TestHyperlinkValidation(unittest.TestCase): + + def test_hyperlink_accepts_allowed_schemes(self): + self.assertIsNone(require.hyperlink(None)) + self.assertEqual(require.hyperlink(""), "") + self.assertEqual(require.hyperlink("/relative/path"), "/relative/path") + self.assertEqual(require.hyperlink("https://example.com"), "https://example.com") + self.assertEqual(require.hyperlink("HTTP://EXAMPLE.COM"), "HTTP://EXAMPLE.COM") + self.assertEqual(require.hyperlink("mailto:user@example.com"), "mailto:user@example.com") + self.assertEqual(require.hyperlink("ftp://example.com/resource"), "ftp://example.com/resource") + + def test_hyperlink_rejects_disallowed_schemes(self): + with self.assertRaises(ValueError): + require.hyperlink("javascript:alert(1)") + with self.assertRaises(ValueError): + require.hyperlink("data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0PiIp") + with self.assertRaises(ValueError): + require.hyperlink("vbscript:msgbox('hi')") + with self.assertRaises(ValueError): + require.hyperlink(" javascript:alert(1)") + + +if __name__ == "__main__": + unittest.main() diff --git a/toyplot/html.py b/toyplot/html.py index da29aabf..a6ef1f8b 100644 --- a/toyplot/html.py +++ b/toyplot/html.py @@ -1041,10 +1041,7 @@ def search(name, visited, modules): search(requirement, visited, modules) # Generate the code. - script = """(function() -{ -var modules={}; -""" + script = """(function()\n{\nvar modules={};\n""" # Initialize required modules. for name, (requirements, factory, value) in modules: @@ -1073,6 +1070,22 @@ def search(name, visited, modules): script += """})();""" + # Security Hardening (Issue #218): + # Inline , even if it occurs inside a JavaScript string literal. + # User-controlled data funneled through json.dumps() could therefore inject + # arbitrary script by embedding . + # + # Mitigation: escape every occurrence of inside the script payload + # before inserting it into the DOM. The conventional safe form is <\/script>, + # which the JavaScript engine interprets the same, while the HTML parser does + # not treat it as an end tag. + # + # Note: We intentionally perform a plain string replacement across the entire + # script text. This is safe because it only increases escaping and does not + # alter runtime semantics. Future refactors may move to a data + loader model. + script = script.replace("", "<\\/script>") + # Create the DOM elements. xml.SubElement(context.parent, "script").text = script diff --git a/toyplot/reportlab/png.py b/toyplot/reportlab/png.py index abcbcd75..af33788c 100644 --- a/toyplot/reportlab/png.py +++ b/toyplot/reportlab/png.py @@ -7,6 +7,8 @@ import io +import os +import shutil import subprocess from packaging.version import Version @@ -20,14 +22,17 @@ _gs_command = None _gs_version = None for command in ["gs", "gswin64c", "gswin32c"]: - try: - _gs_version = subprocess.check_output([command, "--version"]).decode(encoding="utf-8").strip() - _gs_command = command - except: - pass + path = shutil.which(command) + if path: + _gs_command = os.path.realpath(path) + try: + _gs_version = subprocess.check_output([_gs_command, "--version"]).decode(encoding="utf-8").strip() + except subprocess.CalledProcessError: + continue + break if _gs_command is None: - raise Exception("A ghostscript executable is required.") # pragma: no cover + raise EnvironmentError("A ghostscript executable is required.") # pragma: no cover if Version(_gs_version) >= Version("9.14"): _gs_resolution = ["-r%s" % (96 * 4), "-dDownScaleFactor=4"] @@ -150,13 +155,14 @@ def render_frames(canvas, width=None, height=None, scale=None): surface.save() command = [ - "gs", + _gs_command, + "-dSAFER", "-dNOPAUSE", "-dBATCH", "-dQUIET", "-dMaxBitmap=2147483647", "-sDEVICE=pngalpha", - "-r%s" % 96, + ] + _gs_resolution + [ "-sOutputFile=-", "-", ] diff --git a/toyplot/require.py b/toyplot/require.py index 49cf4858..a74734f8 100644 --- a/toyplot/require.py +++ b/toyplot/require.py @@ -8,6 +8,7 @@ import numbers import numpy +from urllib.parse import urlparse def instance(value, types): """Raise an exception if a value isn't one of the given type(s).""" @@ -107,9 +108,24 @@ def filename(value): return optional_string(value) +_ALLOWED_URI_SCHEMES = {"", "http", "https", "mailto", "ftp"} + def hyperlink(value): - """Raise an exception if a value isn't a valid string hyperlink, or None.""" - return optional_string(value) + """ + Raise an exception if a value isn't a valid string hyperlink, or None. + Only allows safe URI schemes: http, https, mailto, ftp, or relative URLs (no scheme). + """ + value = optional_string(value) + if value is None: + return value + value = value.strip() + if not value: + return value + parsed = urlparse(value) + scheme = parsed.scheme.lower() + if scheme not in _ALLOWED_URI_SCHEMES: + raise ValueError(f"Disallowed URI scheme: {parsed.scheme}") + return value def as_int(value,precision=None): """Raise an exception if a value cannot be converted to an int, or value