From 752eff02d856703da0cd98e8f7da74d3abaa9016 Mon Sep 17 00:00:00 2001 From: bnewc <114115883+bnewc@users.noreply.github.com> Date: Tue, 11 Jun 2024 17:36:54 -0700 Subject: [PATCH] Prevent user from using illegal characters in output filename Add some checks in the Dangerzone GUI and CLI that will prevent a user from mistakenly adding illegal characters in the output filename. --- dangerzone/document.py | 18 ++++++++++++- dangerzone/errors.py | 7 +++++ dangerzone/gui/main_window.py | 50 ++++++++++++++++++++++++++--------- tests/test_document.py | 18 +++++++++++++ 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/dangerzone/document.py b/dangerzone/document.py index 6e3a057..0b3e3f4 100644 --- a/dangerzone/document.py +++ b/dangerzone/document.py @@ -1,8 +1,10 @@ import enum import logging import os +import platform +import re import secrets -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Optional from . import errors, util @@ -68,6 +70,20 @@ class Document: def validate_output_filename(filename: str) -> None: if not filename.endswith(".pdf"): raise errors.NonPDFOutputFileException() + + if platform.system() == "Windows": + final_filename = PureWindowsPath(filename).name + illegal_chars_regex = re.compile(r"[\"*/:<>?\\|]") + else: + final_filename = PurePosixPath(filename).name + illegal_chars_regex = re.compile(r"[\\]") + + if platform.system() in ("Windows", "Darwin"): + match = illegal_chars_regex.search(final_filename) + if match: + # filename contains illegal characters + raise errors.IllegalOutputFilenameException(match.group(0)) + if not os.access(Path(filename).parent, os.W_OK): # in unwriteable directory raise errors.UnwriteableOutputDirException() diff --git a/dangerzone/errors.py b/dangerzone/errors.py index dbbcb4b..a55f508 100644 --- a/dangerzone/errors.py +++ b/dangerzone/errors.py @@ -42,6 +42,13 @@ class NonPDFOutputFileException(DocumentFilenameException): super().__init__("Safe PDF filename must end in '.pdf'") +class IllegalOutputFilenameException(DocumentFilenameException): + """Exception for when the output file contains illegal characters.""" + + def __init__(self, char: str) -> None: + super().__init__(f"Illegal character: {char}") + + class UnwriteableOutputDirException(DocumentFilenameException): """Exception for when the output file is not writeable.""" diff --git a/dangerzone/gui/main_window.py b/dangerzone/gui/main_window.py index 7d4e3c6..43fb4c8 100644 --- a/dangerzone/gui/main_window.py +++ b/dangerzone/gui/main_window.py @@ -810,23 +810,29 @@ class SettingsWidget(QtWidgets.QWidget): self.safe_extension = QtWidgets.QLineEdit() self.safe_extension.setStyleSheet("margin-left: -6px;") # no left margin self.safe_extension.textChanged.connect(self.update_ui) - self.safe_extension_invalid = QtWidgets.QLabel("(must end in .pdf)") + self.safe_extension_invalid = QtWidgets.QLabel("") self.safe_extension_invalid.setStyleSheet("color: red") self.safe_extension_invalid.hide() self.safe_extension_name_layout = QtWidgets.QHBoxLayout() self.safe_extension_name_layout.setSpacing(0) self.safe_extension_name_layout.addWidget(self.safe_extension_filename) self.safe_extension_name_layout.addWidget(self.safe_extension) - # FIXME: Workaround for https://github.com/freedomofpress/dangerzone/issues/339. # We should drop this once we drop Ubuntu Focal support. if hasattr(QtGui, "QRegularExpressionValidator"): - dot_pdf_regex = QtCore.QRegularExpression(r".*\.[Pp][Dd][Ff]") - validator = QtGui.QRegularExpressionValidator(dot_pdf_regex) + QRegEx = QtCore.QRegularExpression + QRegExValidator = QtGui.QRegularExpressionValidator else: - dot_pdf_regex = QtCore.QRegExp(r".*\.[Pp][Dd][Ff]") # type: ignore [assignment] - validator = QtGui.QRegExpValidator(dot_pdf_regex) # type: ignore [call-overload] - self.safe_extension.setValidator(validator) + QRegEx = QtCore.QRegExp # type: ignore [assignment] + QRegExValidator = QtGui.QRegExpValidator # type: ignore [assignment] + self.dot_pdf_validator = QRegExValidator(QRegEx(r".*\.[Pp][Dd][Ff]")) + if platform.system() == "Linux": + illegal_chars_regex = r"[/]" + elif platform.system() == "Darwin": + illegal_chars_regex = r"[\\]" + else: + illegal_chars_regex = r"[\"*/:<>?\\|]" + self.illegal_chars_regex = QRegEx(illegal_chars_regex) self.safe_extension_layout = QtWidgets.QHBoxLayout() self.safe_extension_layout.addWidget(self.save_checkbox) self.safe_extension_layout.addWidget(self.safe_extension_label) @@ -969,14 +975,32 @@ class SettingsWidget(QtWidgets.QWidget): # ignore validity if not saving file self.safe_extension_invalid.hide() return True + return ( + self.check_safe_extension_illegal_chars() + and self.check_safe_extension_dot_pdf() + ) - if self.safe_extension.hasAcceptableInput(): - self.safe_extension_invalid.hide() - return True - else: - # prevent starting conversion until correct - self.safe_extension_invalid.show() + def check_safe_extension_illegal_chars(self) -> bool: + match = self.illegal_chars_regex.match(self.safe_extension.text()) + if match.hasMatch(): + self.set_safe_extension_invalid_label( + f"illegal character: {match.captured()}" + ) return False + self.safe_extension_invalid.hide() + return True + + def check_safe_extension_dot_pdf(self) -> bool: + self.safe_extension.setValidator(self.dot_pdf_validator) + if not self.safe_extension.hasAcceptableInput(): + self.set_safe_extension_invalid_label("must end in .pdf") + return False + self.safe_extension_invalid.hide() + return True + + def set_safe_extension_invalid_label(self, string: str) -> None: + self.safe_extension_invalid.setText(string) + self.safe_extension_invalid.show() def check_either_save_or_open(self) -> bool: return ( diff --git a/tests/test_document.py b/tests/test_document.py index 375ffaa..8715e2d 100644 --- a/tests/test_document.py +++ b/tests/test_document.py @@ -79,6 +79,24 @@ def test_output_file_not_pdf(tmp_path: Path) -> None: assert not os.path.exists(docx_file) +@pytest.mark.skipif(platform.system() != "Windows", reason="Windows-specific") +def test_illegal_output_filename_windows(tmp_path: Path) -> None: + d = Document() + + for char in '"*:<>?': + with pytest.raises(errors.IllegalOutputFilenameException): + d.output_filename = str(tmp_path / f"illegal{char}name.pdf") + + +@pytest.mark.skipif(platform.system() != "Darwin", reason="MacOS-specific") +def test_illegal_output_filename_macos(tmp_path: Path) -> None: + illegal_name = str(tmp_path / "illegal\\name.pdf") + d = Document() + + with pytest.raises(errors.IllegalOutputFilenameException): + d.output_filename = illegal_name + + @pytest.mark.skipif(platform.system() == "Windows", reason="Unix-specific") def test_archive_unwriteable_dir(tmp_path: Path) -> None: doc = tmp_path / "doc.pdf"