Static analysis fixes for Python codebase (#1277)

* Don't log names of internal symbols that can be used for attacks * Add integrity check to dropzone.js library * Better a11y of web form labels * Safer handling of file download paths * Don't invert boolean check * Make backend auth check a flask abort * Clean up indentation to remove unwanted tabs * Run workflow either on PR events, or branch pushes, not both
2023-11-01 22:28:53 +09:00 · 2023-11-01 22:28:53 +09:00 · 063e8ed32b
parent 37b9110c99
commit 063e8ed32b
12 changed files with 79 additions and 61 deletions
--- a/.github/workflows/web.yml
+++ b/.github/workflows/web.yml
@ -3,11 +3,9 @@ name: Web Tests/Analysis
 on:
  workflow_dispatch:
  push:
-    paths:
-      - 'python/web/**'
-      - 'python/common/**'
-      - '.github/workflows/web.yml'
-      - 'easyinstall.sh'
+    branches:
+      - 'develop'
+      - 'main'
  pull_request:
    paths:
      - 'python/web/**'
@ -19,9 +17,6 @@ on:
      - opened
      - synchronize
      - reopened
-    branches:
-      - 'develop'
-      - 'main'

 jobs:
  backend_checks:
--- a/python/common/src/piscsi/file_cmds.py
+++ b/python/common/src/piscsi/file_cmds.py
@ -64,7 +64,7 @@ class FileCmds:
        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 not d[0] == "."]
+            dirs[:] = [d for d in dirs if d[0] != "."]
            for dir in dirs:
                if dir not in excluded_dirs:
                    dirpath = path.join(root, dir)
@ -488,12 +488,12 @@ class FileCmds:
        iso_filename = Path(server_info["image_dir"]) / f"{file_name}.iso"

        with TemporaryDirectory() as tmp_dir:
-            req_proc = self.download_to_dir(quote(url, safe=URL_SAFE), tmp_dir, file_name)
+            tmp_full_path = Path(tmp_dir) / file_name
+            req_proc = self.download_to_dir(quote(url, safe=URL_SAFE), tmp_full_path)
            logging.info("Downloaded %s to %s", file_name, tmp_dir)
            if not req_proc["status"]:
                return {"status": False, "msg": req_proc["msg"]}

-            tmp_full_path = Path(tmp_dir) / file_name
            if is_zipfile(tmp_full_path):
                if "XtraStuf.mac" in str(ZipFile(str(tmp_full_path)).namelist()):
                    logging.info(
@ -565,9 +565,9 @@ class FileCmds:
        }

    # noinspection PyMethodMayBeStatic
-    def download_to_dir(self, url, save_dir, file_name):
+    def download_to_dir(self, url, target_path):
        """
-        Takes (str) url, (str) save_dir, (str) file_name
+        Takes (str) url, (Path) target_path
        Returns (dict) with (bool) status and (str) msg
        """
        logging.info("Making a request to download %s", url)
@ -580,7 +580,7 @@ class FileCmds:
            ) as req:
                req.raise_for_status()
                try:
-                    with open(f"{save_dir}/{file_name}", "wb") as download:
+                    with open(str(target_path), "wb") as download:
                        for chunk in req.iter_content(chunk_size=8192):
                            download.write(chunk)
                except FileNotFoundError as error:
@ -593,7 +593,7 @@ class FileCmds:
        logging.info("Response content-type: %s", req.headers["content-type"])
        logging.info("Response status code: %s", req.status_code)

-        parameters = {"file_name": file_name, "save_dir": save_dir}
+        parameters = {"target_path": str(target_path)}
        return {
            "status": True,
            "return_code": ReturnCodes.DOWNLOADTODIR_SUCCESS,
--- a/python/ctrlboard/src/ctrlboard_event_handler/ctrlboard_menu_update_event_handler.py
+++ b/python/ctrlboard/src/ctrlboard_event_handler/ctrlboard_menu_update_event_handler.py
@ -107,8 +107,7 @@ class CtrlBoardMenuUpdateEventHandler(Observer):
        except AttributeError:
            log = logging.getLogger(__name__)
            log.error(
-                "Handler function [%s] not found or returned an error. Skipping.",
-                str(handler_function_name),
+                "Handler function not found or returned an error. Skipping.",
            )

    # noinspection PyUnusedLocal
--- a/python/ctrlboard/src/ctrlboard_menu_builder.py
+++ b/python/ctrlboard/src/ctrlboard_menu_builder.py
@ -8,7 +8,7 @@ from piscsi.piscsi_cmds import PiscsiCmds


 class CtrlBoardMenuBuilder(MenuBuilder):
-    """Class fgor building the control board UI specific menus"""
+    """Class for building the control board UI specific menus"""

    SCSI_ID_MENU = "scsi_id_menu"
    ACTION_MENU = "action_menu"
@ -48,7 +48,7 @@ class CtrlBoardMenuBuilder(MenuBuilder):
            return self.create_device_info_menu(context_object)

        log = logging.getLogger(__name__)
-        log.warning("Provided menu name [%s] cannot be built!", name)
+        log.error("Provided menu name cannot be built!")

        return self.create_scsi_id_list_menu(context_object)

--- a/python/web/src/return_code_mapper.py
+++ b/python/web/src/return_code_mapper.py
@ -23,7 +23,7 @@ class ReturnCodeMapper:
        ReturnCodes.DOWNLOADFILETOISO_SUCCESS:
            _("Created CD-ROM ISO image with arguments \"%(value)s\""),
        ReturnCodes.DOWNLOADTODIR_SUCCESS:
-            _("%(file_name)s downloaded to %(save_dir)s"),
+            _("Downloaded file to %(target_path)s"),
        ReturnCodes.WRITEFILE_SUCCESS:
            _("File created: %(target_path)s"),
        ReturnCodes.WRITEFILE_COULD_NOT_WRITE:
--- a/python/web/src/static/themes/classic/style.css
+++ b/python/web/src/static/themes/classic/style.css
@ -203,3 +203,7 @@ div.throttle-notice > div a {
 div.throttle-notice > div a:hover {
  text-decoration: underline;
 }
+
+label.hidden {
+  display: none;
+}
--- a/python/web/src/static/themes/modern/style.css
+++ b/python/web/src/static/themes/modern/style.css
@ -62,6 +62,10 @@ div.notice {
  color: #fff;
 }

+label.hidden {
+  display: none;
+}
+
 /*
 ------------------------------------------------------------------------------
 Tables
--- a/python/web/src/templates/base.html
+++ b/python/web/src/templates/base.html
@ -113,46 +113,46 @@
        </div>

        <div>
-            {% if env["netatalk_configured"] %}
-		{{ _("Mac AFP file sharing is enabled.") }}
-		{% if env["webmin_configured"] %}
-		    <a href="https://{{ env["ip_addr"] }}:10000/netatalk2/" target=\"_blank\">
-		    {{ ("Server administration") }}
-		    </a>
-		{% endif %}
-	    {% else %}
-		{{ _("Mac AFP file sharing is disabled.") }}
-            {% endif %}
+        {% if env["netatalk_configured"] %}
+        {{ _("Mac AFP file sharing is enabled.") }}
+        {% if env["webmin_configured"] %}
+            <a href="https://{{ env["ip_addr"] }}:10000/netatalk2/" target=\"_blank\">
+            {{ ("Server administration") }}
+            </a>
+        {% endif %}
+        {% else %}
+            {{ _("Mac AFP file sharing is disabled.") }}
+        {% endif %}
        </div>
        <div>
-            {% if env["samba_configured"] %}
-		{{ _("Windows SMB file sharing is enabled.") }}
-		{% if env["webmin_configured"] %}
-		    <a href="https://{{ env["ip_addr"] }}:10000/samba/" target=\"_blank\">
-		    {{ ("Server administration") }}
-		    </a>
-		{% endif %}
-	    {% else %}
-		{{ _("Windows SMB file sharing is disabled.") }}
-            {% endif %}
+        {% if env["samba_configured"] %}
+            {{ _("Windows SMB file sharing is enabled.") }}
+        {% if env["webmin_configured"] %}
+            <a href="https://{{ env["ip_addr"] }}:10000/samba/" target=\"_blank\">
+            {{ ("Server administration") }}
+            </a>
+        {% endif %}
+        {% else %}
+            {{ _("Windows SMB file sharing is disabled.") }}
+        {% endif %}
        </div>
        <div>
-            {% if env["ftp_configured"] %}
-		{{ _("FTP file sharing is enabled.") }}
-	    {% else %}
-		{{ _("FTP file sharing is disabled.") }}
-            {% endif %}
+        {% if env["ftp_configured"] %}
+            {{ _("FTP file sharing is enabled.") }}
+        {% else %}
+            {{ _("FTP file sharing is disabled.") }}
+        {% endif %}
        </div>
        <div>
-            {% if env["macproxy_configured"] %}
-		{{ _("Macproxy is running at %(ip_addr)s (default port 5000)", ip_addr=env['ip_addr']) }}
-	    {% else %}
-		{{ _("Macproxy is disabled.") }}
-            {% endif %}
+        {% if env["macproxy_configured"] %}
+            {{ _("Macproxy is running at %(ip_addr)s (default port 5000)", ip_addr=env['ip_addr']) }}
+        {% else %}
+            {{ _("Macproxy is disabled.") }}
+        {% endif %}
        </div>

        <div>
-	    {{ _("PiSCSI software version:") }} <b>{{ env["version"] }}</b>
+            {{ _("PiSCSI software version:") }} <b>{{ env["version"] }}</b>
        </div>
        <div>
            {{ _("Hardware and OS:") }} {{ env["running_env"]["env"] }}
--- a/python/web/src/templates/index.html
+++ b/python/web/src/templates/index.html
@ -396,6 +396,7 @@
    <input name="url" id="download_url" required="" type="url">
    <input type="radio" name="destination" id="disk_images" value="disk_images" checked="checked">
    <label for="disk_images">{{ _("Disk Images") }}</label>
+    <label for="images_subdir" class="hidden">{{ _("Directory") }}</label>
    <select name="images_subdir" id="images_subdir">
    {% for dir in images_subdirs %}
    <option value="{{dir}}">{{dir}}</option>
@ -405,6 +406,7 @@
    {% if file_server_dir_exists %}
    <input type="radio" name="destination" id="shared_files" value="shared_files">
    <label for="shared_files">{{ _("Shared Files") }}</label>
+    <label for="shared_subdir" class="hidden">{{ _("Directory") }}</label>
    <select name="shared_subdir" id="shared_subdir">
    {% for dir in shared_subdirs %}
    <option value="{{dir}}">{{dir}}</option>
--- a/python/web/src/templates/upload.html
+++ b/python/web/src/templates/upload.html
@ -11,10 +11,12 @@
    <li>{{ _("PiSCSI Config") }} = {{ CFG_DIR }}</li>
 </ul>

-<h3>{{ _("Destination") }}</h3>
 <form name="dropper" action="/files/upload" method="post" class="dropzone dz-clickable" enctype="multipart/form-data" id="dropper">
+    <fieldset>
+    <legend>{{ _("Destination") }}</legend>
    <input type="radio" name="destination" id="disk_images" value="disk_images" checked="checked">
    <label for="disk_images">{{ _("Disk Images") }}</label>
+    <label for="images_subdir" class="hidden">{{ _("Directory") }}</label>
    <select name="images_subdir" id="images_subdir">
    {% for dir in images_subdirs %}
    <option value="{{dir}}">{{dir}}</option>
@ -24,6 +26,7 @@
    {% if file_server_dir_exists %}
    <input type="radio" name="destination" id="shared_files" value="shared_files">
    <label for="shared_files">{{ _("Shared Files") }}</label>
+    <label for="shared_subdir" class="hidden">{{ _("Directory") }}</label>
    <select name="shared_subdir" id="shared_subdir">
    {% for dir in shared_subdirs %}
    <option value="{{dir}}">{{dir}}</option>
@ -33,9 +36,15 @@
    {% endif %}
    <input type="radio" name="destination" id="piscsi_config" value="piscsi_config">
    <label for="piscsi_config">{{ _("PiSCSI Config") }}</label>
+    </fieldset>
 </form>

-<script type="application/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/dropzone/5.9.3/min/dropzone.min.js"></script>
+<script
+    type="application/javascript"
+    src="https://cdnjs.cloudflare.com/ajax/libs/dropzone/5.9.3/min/dropzone.min.js"
+    integrity="sha384-PwiT+fWTPpIySx6DrH1FKraKo+LvVpOClsjx0TSdMYTKi7BR1hR149f4VHLUUnfA"
+    crossorigin="anonymous"
+></script>

 <script type="application/javascript">
    Dropzone.options.dropper = {
--- a/python/web/src/web.py
+++ b/python/web/src/web.py
@ -27,6 +27,7 @@ from flask import (
    make_response,
    session,
    jsonify,
+    abort,
 )

 from piscsi.piscsi_cmds import PiscsiCmds
@ -991,7 +992,7 @@ def download_file():
    else:
        return response(error=True, message=_("Unknown destination"))

-    process = file_cmd.download_to_dir(url, destination_dir, Path(url).name)
+    process = file_cmd.download_to_dir(url, Path(destination_dir) / Path(url).name)
    process = ReturnCodeMapper.add_msg(process)
    if process["status"]:
        return response(message=process["msg"])
@ -1447,6 +1448,16 @@ def log_http_request():
        logging.debug(message)


+@APP.before_request
+def check_backend_auth():
+    if not piscsi_cmd.is_token_auth()["status"] and not APP.config["PISCSI_TOKEN"]:
+        abort(
+            403,
+            "PiSCSI is password protected. "
+            "Start the Web Interface with the --password parameter.",
+        )
+
+
 if __name__ == "__main__":
    APP.secret_key = "piscsi_is_awesome_insecure_secret_key"
    APP.config["SESSION_TYPE"] = "filesystem"
@ -1524,12 +1535,6 @@ if __name__ == "__main__":
    file_cmd = FileCmds(piscsi=piscsi_cmd)
    sys_cmd = SysCmds()

-    if not piscsi_cmd.is_token_auth()["status"] and not APP.config["PISCSI_TOKEN"]:
-        raise Exception(
-            "PiSCSI is password protected. "
-            "Start the Web Interface with the --password parameter."
-        )
-
    if Path(f"{CFG_DIR}/{DEFAULT_CONFIG}").is_file():
        file_cmd.read_config(DEFAULT_CONFIG)
    if Path(f"{DRIVE_PROPERTIES_FILE}").is_file():
--- a/python/web/tests/api/test_files.py
+++ b/python/web/tests/api/test_files.py
@ -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"{file_name} downloaded to {env['images_dir']}{subdir}"
+        == f"Downloaded file to {env['images_dir']}{subdir}{file_name}"
    )

    # Cleanup