Web UI: Refactor file upload code to make it safer

python/common/src/piscsi/file_cmds.py

@@ -30,6 +30,8 @@
 FILE_READ_ERROR = "Unhandled exception when reading file: %s"
 FILE_WRITE_ERROR = "Unhandled exception when writing to file: %s"
 URL_SAFE = "/:?&"
+# Common file sharing protocol meta data dirs to filter out from target upload dirs
+EXCLUDED_DIRS = ["Network Trash Folder", "Temporary Items", "TheVolumeSettingsFolder"]
 
 
 class FileCmds:
@@ -60,15 +62,14 @@ def list_subdirs(self, directory):
         Returns a (list) of (str) subdir_list.
         """
         subdir_list = []
-        # Filter out file sharing meta data dirs
-        excluded_dirs = ("Network Trash Folder", "Temporary Items", "TheVolumeSettingsFolder")
         for root, dirs, _files in walk(directory, topdown=True):
             # Strip out dirs that begin with .
             dirs[:] = [d for d in dirs if d[0] != "."]
             for dir in dirs:
-                if dir not in excluded_dirs:
+                if dir not in EXCLUDED_DIRS:
                     dirpath = path.join(root, dir)
-                    subdir_list.append(dirpath.replace(directory, "", 1))
+                    # Remove the section of the path up until the first subdir
+                    subdir_list.append(dirpath.replace(directory + "/", "", 1))
 
         subdir_list.sort()
         return subdir_list

python/web/src/templates/index.html

@@ -401,7 +401,7 @@
     {% for dir in images_subdirs %}
     <option value="{{dir}}">{{dir}}</option>
     {% endfor %}
-    <option value="/" selected>/</option>
+    <option value="" selected>/</option>
     </select>
     {% if file_server_dir_exists %}
     <input type="radio" name="destination" id="shared_files" value="shared_files">
@@ -411,7 +411,7 @@
     {% for dir in shared_subdirs %}
     <option value="{{dir}}">{{dir}}</option>
     {% endfor %}
-    <option value="/" selected>/</option>
+    <option value="" selected>/</option>
     </select>
     {% endif %}
     <input type="submit" value="{{ _("Download") }}" onclick="processNotify('{{ _("Downloading File...") }}')">

python/web/src/web.py

@@ -58,6 +58,7 @@
     auth_active,
     is_bridge_configured,
     is_safe_path,
+    validate_target_dir,
     browser_supports_modern_themes,
 )
 from settings import (
@@ -992,19 +993,15 @@ def download_file():
     images_subdir = request.form.get("images_subdir")
     shared_subdir = request.form.get("shared_subdir")
     if destination == "disk_images":
-        safe_path = is_safe_path(Path("." + images_subdir))
-        if not safe_path["status"]:
-            return make_response(safe_path["msg"], 403)
         server_info = piscsi_cmd.get_server_info()
-        destination_dir = server_info["image_dir"] + images_subdir
+        destination_dir = Path(server_info["image_dir"]) / images_subdir
     elif destination == "shared_files":
-        safe_path = is_safe_path(Path("." + shared_subdir))
-        if not safe_path["status"]:
-            return make_response(safe_path["msg"], 403)
-        destination_dir = FILE_SERVER_DIR + shared_subdir
+        destination_dir = Path(FILE_SERVER_DIR) / shared_subdir
     else:
         return response(error=True, message=_("Unknown destination"))
 
+    validate_target_dir(destination_dir)
+
     process = file_cmd.download_to_dir(url, Path(destination_dir) / Path(url).name)
     process = ReturnCodeMapper.add_msg(process)
     if process["status"]:
@@ -1034,21 +1031,17 @@ def upload_file():
     images_subdir = request.form.get("images_subdir")
     shared_subdir = request.form.get("shared_subdir")
     if destination == "disk_images":
-        safe_path = is_safe_path(Path("." + images_subdir))
-        if not safe_path["status"]:
-            return make_response(safe_path["msg"], 403)
         server_info = piscsi_cmd.get_server_info()
         destination_dir = Path(server_info["image_dir"]) / images_subdir
     elif destination == "shared_files":
-        safe_path = is_safe_path(Path("." + shared_subdir))
-        if not safe_path["status"]:
-            return make_response(safe_path["msg"], 403)
         destination_dir = Path(FILE_SERVER_DIR) / shared_subdir
     elif destination == "piscsi_config":
         destination_dir = Path(CFG_DIR)
     else:
         return make_response(_("Unknown destination"), 403)
 
+    validate_target_dir(destination_dir)
+
     log = logging.getLogger("pydrop")
     file_object = request.files["file"]
     file_name = secure_filename(file_object.filename)

python/web/src/web_utils.py

@@ -8,7 +8,7 @@
 from ua_parser import user_agent_parser
 from re import findall
 
-from flask import request, abort
+from flask import request, abort, make_response
 from flask_babel import _
 from werkzeug.utils import secure_filename
 
@@ -324,6 +324,16 @@ def is_safe_path(file_name):
     return {"status": True, "msg": ""}
 
 
+def validate_target_dir(target_dir):
+    """
+    Takes (Path) target_dir on the host and validates that it is a valid dir for uploading.
+    """
+    safe_path = is_safe_path(Path(".") / target_dir)
+    if not safe_path["status"]:
+        return make_response(safe_path["msg"], 403)
+    return True
+
+
 def browser_supports_modern_themes():
     """
     Determines if the browser supports the HTML/CSS/JS features used in non-legacy themes.

python/web/tests/api/test_files.py

@@ -208,7 +208,7 @@ def test_extract_file(
         "/files/download_url",
         data={
             "destination": "disk_images",
-            "images_subdir": "/",
+            "images_subdir": "",
             "url": url,
         },
     )
@@ -335,7 +335,7 @@ def test_download_properties(http_client, list_files, delete_file):
 def test_download_url_to_dir(env, httpserver, http_client, list_files, delete_file):
     file_name = str(uuid.uuid4())
     http_path = f"/images/{file_name}"
-    subdir = "/"
+    subdir = ""
     url = httpserver.url_for(http_path)
 
     with open("tests/assets/test_image.hds", mode="rb") as file:
@@ -362,7 +362,7 @@ def test_download_url_to_dir(env, httpserver, http_client, list_files, delete_fi
     assert file_name in list_files()
     assert (
         response_data["messages"][0]["message"]
-        == f"Downloaded file to {env['images_dir']}{subdir}{file_name}"
+        == f"Downloaded file to {env['images_dir']}/{subdir}{file_name}"
     )
 
     # Cleanup