diff --git a/app.py b/app.py index bbe01657..ee8881b1 100644 --- a/app.py +++ b/app.py @@ -5,17 +5,12 @@ from os import path from typing import Dict, Optional -from re import fullmatch -from werkzeug.security import safe_join -from werkzeug.utils import secure_filename from flask import Flask, g, redirect, request, Response, send_from_directory, url_for from flask_session import Session from flask_wtf.csrf import CSRFProtect from jinja2 import ChoiceLoader, FileSystemLoader -from util import get_validated_static_path - from api import api_bp from datarequest.datarequest import datarequest_bp from deposit.deposit import deposit_bp @@ -29,6 +24,7 @@ from user.user import user_bp from vault.vault import vault_bp +from util import get_validated_static_path app = Flask(__name__, static_folder='assets') app.json.sort_keys = False @@ -127,11 +123,16 @@ def static_loader() -> Optional[Response]: :returns: Static file """ - result = get_validated_static_path(request.full_path, request.path, app.config.get('YODA_THEME_PATH'), app.config.get('YODA_THEME')) + result = get_validated_static_path( + request.full_path, + request.path, + app.config.get('YODA_THEME_PATH'), + app.config.get('YODA_THEME') + ) if result is not None: static_dir, asset_name = result return send_from_directory(static_dir, asset_name) - + @app.before_request def protect_pages() -> Optional[Response]: diff --git a/unit-tests/test_util.py b/unit-tests/test_util.py index a4becef1..36bc3d14 100644 --- a/unit-tests/test_util.py +++ b/unit-tests/test_util.py @@ -2,21 +2,20 @@ """Unit tests for portal utility functions. """ -__copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__copyright__ = "Copyright (c) 2023, Utrecht University" +__license__ = "GPLv3, see LICENSE" import sys from unittest import TestCase from unittest.mock import patch -sys.path.append('..') +sys.path.append("..") -from util import is_email_in_domains from util import get_validated_static_path +from util import is_email_in_domains class UtilTest(TestCase): - def test_is_email_in_domains(self): self.assertEqual(is_email_in_domains("peter", ["uu.nl"]), False) self.assertEqual(is_email_in_domains("peter@uu.nl", ["uu.nl"]), True) @@ -29,65 +28,132 @@ def test_is_email_in_domains(self): self.assertEqual(is_email_in_domains("peter@cs.uu.nl", ["*.uu.nl"]), True) self.assertEqual(is_email_in_domains("peter@ai.cs.uu.nl", ["*.cs.uu.nl"]), True) self.assertEqual(is_email_in_domains("peter@ai.hum.uu.nl", ["*.cs.uu.nl"]), False) - - - # If path contains theme and uu, path does not exist - # if path contains anything else, path does exist - def exists_return_value(self, pathname): - return not ('theme' in pathname and 'uu' in pathname) + def exists_return_value(self, pathname): + """ Mock path.exists function. True if path does not contain "theme" and "uu" """ + return not ("theme" in pathname and "uu" in pathname) - @patch('os.path.exists') + @patch("os.path.exists") def test_static_loader_valid_path(self, mock_exists): mock_exists.side_effect = self.exists_return_value # uu theme - static_dir, asset_name = get_validated_static_path('/assets/img/logo.svg?wekr', '/assets/img/logo.svg', "/var/www/yoda/themes", "uu") - self.assertEqual(static_dir, '/var/www/yoda/static/img') - self.assertEqual(asset_name, 'logo.svg') + static_dir, asset_name = get_validated_static_path( + "/assets/img/logo.svg?wekr", + "/assets/img/logo.svg", + "/var/www/yoda/themes", + "uu", + ) + self.assertEqual(static_dir, "/var/www/yoda/static/img") + self.assertEqual(asset_name, "logo.svg") # other theme - static_dir, asset_name = get_validated_static_path('/assets/img/logo.svg?wekr', '/assets/img/logo.svg', "/var/www/yoda/themes", "wur") - self.assertEqual(static_dir, '/var/www/yoda/themes/wur/static/img') - self.assertEqual(asset_name, 'logo.svg') - + static_dir, asset_name = get_validated_static_path( + "/assets/img/logo.svg?wekr", + "/assets/img/logo.svg", + "/var/www/yoda/themes", + "wur", + ) + self.assertEqual(static_dir, "/var/www/yoda/themes/wur/static/img") + self.assertEqual(asset_name, "logo.svg") - @patch('os.path.exists') + @patch("os.path.exists") def test_static_loader_invalid_path(self, mock_exists): mock_exists.side_effect = self.exists_return_value # Too short - self.assertIsNone(get_validated_static_path("/?sawerw", "/", "/var/www/yoda/themes", "uu")) + self.assertIsNone( + get_validated_static_path( + "/?sawerw", "/", "/var/www/yoda/themes", "uu" + ) + ) # Path traversal attack - self.assertIsNone(get_validated_static_path("/assets/../../../../etc/passwd?werwrwr", "/assets/../../../../etc/passwd", "/var/www/yoda/themes", "uu")) + self.assertIsNone( + get_validated_static_path( + "/assets/../../../../etc/passwd?werwrwr", + "/assets/../../../../etc/passwd", + "/var/www/yoda/themes", + "uu", + ) + ) # non-printable characters full_path = "/assets/" + chr(13) + "img/logo.svg?werwer" path = "/assets/" + chr(13) + "img/logo.svg" - self.assertIsNone(get_validated_static_path(full_path, path, "/var/www/yoda/themes", "uu")) - self.assertIsNone(get_validated_static_path(full_path, path, "/var/www/yoda/themes", "wur")) + self.assertIsNone( + get_validated_static_path( + full_path, path, "/var/www/yoda/themes", "uu" + ) + ) + self.assertIsNone( + get_validated_static_path( + full_path, path, "/var/www/yoda/themes", "wur" + ) + ) # non-printable characters in asset name full_path = "/assets/img/l" + chr(13) + "ogo.svg?werwer" path = "/assets/img/l" + chr(13) + "ogo.svg" - self.assertIsNone(get_validated_static_path(full_path, path, "/var/www/yoda/themes", "uu")) - self.assertIsNone(get_validated_static_path(full_path, path, "/var/www/yoda/themes", "wur")) + self.assertIsNone( + get_validated_static_path( + full_path, path, "/var/www/yoda/themes", "uu" + ) + ) + self.assertIsNone( + get_validated_static_path( + full_path, path, "/var/www/yoda/themes", "wur" + ) + ) # .. in file name - self.assertIsNone(get_validated_static_path("/assets/img/lo..go.svg?sklaerw", "/assets/img/lo..go.svg?sklaerw", "/var/www/yoda/themes", "uu")) + self.assertIsNone( + get_validated_static_path( + "/assets/img/lo..go.svg?sklaerw", + "/assets/img/lo..go.svg?sklaerw", + "/var/www/yoda/themes", + "uu", + ) + ) - - @patch('os.path.exists') + @patch("os.path.exists") def test_static_loader_module_valid_path(self, mock_exists): mock_exists.side_effect = self.exists_return_value # uu theme - static_dir, asset_name = get_validated_static_path('/group_manager/assets/js/group_manager.js?wekr', '/group_manager/assets/js/group_manager.js', "/var/www/yoda/themes", "uu") - self.assertEqual(static_dir, '/var/www/yoda/group_manager/static/group_manager/js') - self.assertEqual(asset_name, 'group_manager.js') + static_dir, asset_name = get_validated_static_path( + "/group_manager/assets/js/group_manager.js?wekr", + "/group_manager/assets/js/group_manager.js", + "/var/www/yoda/themes", + "uu", + ) + self.assertEqual( + static_dir, "/var/www/yoda/group_manager/static/group_manager/js" + ) + self.assertEqual(asset_name, "group_manager.js") # other theme - static_dir, asset_name = get_validated_static_path('/group_manager/assets/lib/select2-bootstrap-5-theme/select2-bootstrap-5-theme.css?wekr', '/group_manager/assets/lib/select2-bootstrap-5-theme/select2-bootstrap-5-theme.css', "/var/www/yoda/themes", "wur") - self.assertEqual(static_dir, '/var/www/yoda/themes/wur/group_manager/static/group_manager/lib/select2-bootstrap-5-theme') - self.assertEqual(asset_name, 'select2-bootstrap-5-theme.css') - + static_dir, asset_name = get_validated_static_path( + "/group_manager/assets/lib/select2-bootstrap-5-theme/select2-bootstrap-5-theme.css?wekr", + "/group_manager/assets/lib/select2-bootstrap-5-theme/select2-bootstrap-5-theme.css", + "/var/www/yoda/themes", + "wur", + ) + self.assertEqual( + static_dir, + "/var/www/yoda/themes/wur/group_manager/static/group_manager/lib/select2-bootstrap-5-theme", + ) + self.assertEqual(asset_name, "select2-bootstrap-5-theme.css") - @patch('os.path.exists') + @patch("os.path.exists") def test_static_loader_module_invalid_path(self, mock_exists): mock_exists.side_effect = self.exists_return_value # Invalid module name - self.assertIsNone(get_validated_static_path("/../assets/../research/static/research/css/research.css?sklwrawe", "/../assets/../research/static/research/css/research.css", "/var/www/yoda/themes", "uu")) + self.assertIsNone( + get_validated_static_path( + "/../assets/../research/static/research/css/research.css?sklwrawe", + "/../assets/../research/static/research/css/research.css", + "/var/www/yoda/themes", + "uu", + ) + ) # Path traversal attack - self.assertIsNone(get_validated_static_path("/group_manager/assets/../../../../../../etc/passwd?werwrwr", "/group_manager/assets/../../../../../../etc/passwd", "/var/www/yoda/themes", "uu")) \ No newline at end of file + self.assertIsNone( + get_validated_static_path( + "/group_manager/assets/../../../../../../etc/passwd?werwrwr", + "/group_manager/assets/../../../../../../etc/passwd", + "/var/www/yoda/themes", + "uu", + ) + ) diff --git a/util.py b/util.py index 1244487d..013e6bca 100644 --- a/util.py +++ b/util.py @@ -1,24 +1,24 @@ #!/usr/bin/env python3 -__copyright__ = 'Copyright (c) 2021-2022, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__copyright__ = "Copyright (c) 2021-2022, Utrecht University" +__license__ = "GPLv3, see LICENSE" import sys import traceback -from typing import List, Optional, Tuple - from os import path from re import fullmatch +from typing import List, Optional, Tuple + from werkzeug.security import safe_join from werkzeug.utils import secure_filename def log_error(message: str, print_exception: bool = False) -> None: """Writes an error message, and optionally an exception trace to the - web server error log. + web server error log. - :param message: Error message to print - :param print_exception: Boolean, whether to print an exception trace + :param message: Error message to print + :param print_exception: Boolean, whether to print an exception trace """ print(message, file=sys.stderr) if print_exception: @@ -28,13 +28,13 @@ def log_error(message: str, print_exception: bool = False) -> None: def is_email_in_domains(email: str, domain_list: List[str]) -> bool: """Determines if an email address is in a list of domains - :param email: email address of a user - :param domain_list: list of domains, which can also include wildcard domains that - match a domain and any of its subdomains - (e.g. "*.uu.nl" matches both user@uu.nl, user@subdomain.uu.nl) + :param email: email address of a user + :param domain_list: list of domains, which can also include wildcard domains that + match a domain and any of its subdomains + (e.g. "*.uu.nl" matches both user@uu.nl, user@subdomain.uu.nl) - :returns: boolean value that indicates whether this email address is in one of the - domains + :returns: boolean value that indicates whether this email address is in one of the + domains """ for domain in domain_list: if domain.startswith("*."): @@ -47,18 +47,25 @@ def is_email_in_domains(email: str, domain_list: List[str]) -> bool: return False -def get_validated_static_path(full_path, request_path, yoda_theme_path, yoda_theme) -> Optional[Tuple[str, str]]: +def get_validated_static_path( + full_path, request_path, yoda_theme_path, yoda_theme +) -> Optional[Tuple[str, str]]: """ Static files handling - recognisable through '/assets/' Confirms that input path is valid and return corresponding static path - + + :param full_path: Full path of request + :param request_path: Short path of request + :param yoda_theme_path: Path to the yoda themes + :param yoda_theme: Name of the chosen theme + :returns: Tuple of static directory and filename for correct path, None for incorrect path """ # Only allow printable ascii - if fullmatch('[ -~]*', full_path) is not None and '/assets/' in full_path: + if fullmatch("[ -~]*", full_path) is not None and "/assets/" in full_path: user_static_area = path.join(yoda_theme_path, yoda_theme) - parts = full_path.split('/') - + parts = full_path.split("/") + # Trim empty string and file name from path parts = parts[1:-1] _, asset_name = path.split(request_path) @@ -66,14 +73,14 @@ def get_validated_static_path(full_path, request_path, yoda_theme_path, yoda_the if asset_name != secure_filename(asset_name): return - if parts[0] == 'assets': + if parts[0] == "assets": # Main assets - static_dir = safe_join(user_static_area + '/static', *parts[1:]) + static_dir = safe_join(user_static_area + "/static", *parts[1:]) if not static_dir: return user_static_filename = path.join(static_dir, asset_name) if not path.exists(user_static_filename): - static_dir = safe_join('/var/www/yoda/static', *parts[1:]) + static_dir = safe_join("/var/www/yoda/static", *parts[1:]) else: # Module specific assets module = parts[0] @@ -81,15 +88,17 @@ def get_validated_static_path(full_path, request_path, yoda_theme_path, yoda_the if module != secure_filename(module): return - module_static_area = path.join(module, 'static', module) - user_static_filename = safe_join(path.join(user_static_area, module_static_area), *parts[2:], asset_name) + module_static_area = path.join(module, "static", module) + user_static_filename = safe_join( + path.join(user_static_area, module_static_area), *parts[2:], asset_name + ) if not user_static_filename: return if path.exists(user_static_filename): static_dir = path.join(user_static_area, module_static_area, *parts[2:]) else: - static_dir = path.join('/var/www/yoda/', module_static_area, *parts[2:]) + static_dir = path.join("/var/www/yoda/", module_static_area, *parts[2:]) full_path = path.join(static_dir, asset_name) # Check that path is correct