From 42dc3c60327335afb2c4236b1ab4a2e073348369 Mon Sep 17 00:00:00 2001 From: deeplow Date: Tue, 2 Jan 2024 10:44:09 +0000 Subject: [PATCH] FIXUP: Make dummy into unsafe conversion We previously had used the dummy conversion as a way to test dangerzone on systems that did not support nested virtualization. However, we now support a pseudo on-host conversion. This means that we can now do this type on conversion on those system. FIXUP: remove "ProcessBased" subclasses --- .github/workflows/ci.yml | 4 +- dangerzone/cli.py | 10 +- dangerzone/gui/__init__.py | 12 +- dangerzone/gui/main_window.py | 6 +- dangerzone/isolation_provider/base.py | 147 +++++++++------------ dangerzone/isolation_provider/container.py | 4 +- dangerzone/isolation_provider/dummy.py | 72 ---------- dangerzone/isolation_provider/qubes.py | 8 +- tests/gui/__init__.py | 6 +- tests/gui/test_main_window.py | 4 +- tests/isolation_provider/base.py | 2 +- tests/test_cli.py | 16 ++- 12 files changed, 97 insertions(+), 194 deletions(-) delete mode 100644 dangerzone/isolation_provider/dummy.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ac8c0488..92f20463b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,7 @@ jobs: windows: runs-on: windows-latest env: - DUMMY_CONVERSION: True + UNSAFE_CONVERSION: True steps: - uses: actions/checkout@v3 - uses: actions/setup-python@v4 @@ -31,7 +31,7 @@ jobs: macOS: runs-on: macos-latest env: - DUMMY_CONVERSION: True + UNSAFE_CONVERSION: True steps: - uses: actions/checkout@v3 - uses: actions/setup-python@v4 diff --git a/dangerzone/cli.py b/dangerzone/cli.py index 2361399f6..2a50c85d8 100644 --- a/dangerzone/cli.py +++ b/dangerzone/cli.py @@ -8,7 +8,7 @@ from . import args, errors from .document import ARCHIVE_SUBDIR, SAFE_EXTENSION from .isolation_provider.container import Container -from .isolation_provider.dummy import Dummy +from .isolation_provider.unsafe import UnsafeConverter from .isolation_provider.qubes import Qubes, is_qubes_native_conversion from .logic import DangerzoneCore from .util import get_version @@ -35,7 +35,7 @@ def print_header(s: str) -> None: help=f"Archives the unsafe version in a subdirectory named '{ARCHIVE_SUBDIR}'", ) @click.option( - "--unsafe-dummy-conversion", "dummy_conversion", flag_value=True, hidden=True + "--unsafe-conversion", "unsafe_conversion", flag_value=True, hidden=True ) @click.option( "--enable-timeouts / --disable-timeouts", @@ -58,12 +58,12 @@ def cli_main( enable_timeouts: bool, filenames: List[str], archive: bool, - dummy_conversion: bool, + unsafe_conversion: bool, ) -> None: setup_logging() - if getattr(sys, "dangerzone_dev", False) and dummy_conversion: - dangerzone = DangerzoneCore(Dummy()) + if getattr(sys, "dangerzone_dev", False) and unsafe_conversion: + dangerzone = DangerzoneCore(UnsafeConverter()) elif is_qubes_native_conversion(): dangerzone = DangerzoneCore(Qubes()) else: diff --git a/dangerzone/gui/__init__.py b/dangerzone/gui/__init__.py index 655b13b38..043a803f8 100644 --- a/dangerzone/gui/__init__.py +++ b/dangerzone/gui/__init__.py @@ -24,7 +24,7 @@ from .. import args, errors from ..document import Document from ..isolation_provider.container import Container -from ..isolation_provider.dummy import Dummy +from ..isolation_provider.unsafe import UnsafeConverter from ..isolation_provider.qubes import Qubes, is_qubes_native_conversion from ..util import get_resource_path, get_version from .logic import DangerzoneGui @@ -94,7 +94,7 @@ def infer_os_color_mode(self) -> OSColorMode: @click.command() @click.option( - "--unsafe-dummy-conversion", "dummy_conversion", flag_value=True, hidden=True + "--unsafe-conversion", "unsafe_conversion", flag_value=True, hidden=True ) @click.option( "--enable-timeouts / --disable-timeouts", @@ -112,7 +112,7 @@ def infer_os_color_mode(self) -> OSColorMode: @click.version_option(version=get_version(), message="%(version)s") @errors.handle_document_errors def gui_main( - dummy_conversion: bool, filenames: Optional[List[str]], enable_timeouts: bool + unsafe_conversion: bool, filenames: Optional[List[str]], enable_timeouts: bool ) -> bool: setup_logging() @@ -131,9 +131,9 @@ def gui_main( app = Application() # Common objects - if getattr(sys, "dangerzone_dev", False) and dummy_conversion: - dummy = Dummy() - dangerzone = DangerzoneGui(app, isolation_provider=dummy) + if getattr(sys, "dangerzone_dev", False) and unsafe_conversion: + unsafe_converter = UnsafeConverter() + dangerzone = DangerzoneGui(app, isolation_provider=unsafe_converter) elif is_qubes_native_conversion(): qubes = Qubes() dangerzone = DangerzoneGui(app, isolation_provider=qubes) diff --git a/dangerzone/gui/main_window.py b/dangerzone/gui/main_window.py index 7b398a791..73bdd10ec 100644 --- a/dangerzone/gui/main_window.py +++ b/dangerzone/gui/main_window.py @@ -23,7 +23,7 @@ from .. import errors from ..document import SAFE_EXTENSION, Document from ..isolation_provider.container import Container, NoContainerTechException -from ..isolation_provider.dummy import Dummy +from ..isolation_provider.unsafe import UnsafeConverter from ..isolation_provider.qubes import Qubes, is_qubes_native_conversion from ..util import get_resource_path, get_subprocess_startupinfo, get_version from .logic import Alert, CollapsibleBox, DangerzoneGui, UpdateDialog @@ -134,10 +134,10 @@ def __init__(self, dangerzone: DangerzoneGui) -> None: self.waiting_widget: WaitingWidget = WaitingWidgetContainer(self.dangerzone) self.waiting_widget.finished.connect(self.waiting_finished) - elif isinstance(self.dangerzone.isolation_provider, Dummy) or isinstance( + elif isinstance(self.dangerzone.isolation_provider, UnsafeConverter) or isinstance( self.dangerzone.isolation_provider, Qubes ): - # Don't wait with dummy converter and on Qubes. + # Don't wait with unsafe converter and on Qubes. self.waiting_widget = WaitingWidget() self.dangerzone.is_waiting_finished = True diff --git a/dangerzone/isolation_provider/base.py b/dangerzone/isolation_provider/base.py index 884ef6440..5a19168e7 100644 --- a/dangerzone/isolation_provider/base.py +++ b/dangerzone/isolation_provider/base.py @@ -49,8 +49,11 @@ class IsolationProvider(ABC): Abstracts an isolation provider """ + STARTUP_TIME_SECONDS = 0 # The maximum time it takes a the provider to start up. + def __init__(self) -> None: self.percentage = 0.0 + self.proc: Optional[subprocess.Popen] = None @abstractmethod def install(self) -> bool: @@ -64,102 +67,30 @@ def convert( ) -> None: self.progress_callback = progress_callback document.mark_as_converting() - try: - success = self._convert(document, ocr_lang) - except errors.ConversionException as e: - success = False - self.print_progress_trusted(document, True, str(e), 0) - except Exception as e: - success = False - log.exception( - f"An exception occurred while converting document '{document.id}'" - ) - self.print_progress_trusted(document, True, str(e), 0) - if success: - document.mark_as_safe() - if document.archive_after_conversion: - document.archive() - else: - document.mark_as_failed() - - @abstractmethod - def _convert( - self, - document: Document, - ocr_lang: Optional[str], - ) -> bool: - pass - - def _print_progress( - self, document: Document, error: bool, text: str, percentage: float - ) -> None: - s = Style.BRIGHT + Fore.YELLOW + f"[doc {document.id}] " - s += Fore.CYAN + f"{percentage}% " + Style.RESET_ALL - if error: - s += Fore.RED + text + Style.RESET_ALL - log.error(s) - else: - s += text - log.info(s) - - if self.progress_callback: - self.progress_callback(error, text, percentage) - - def print_progress_trusted( - self, document: Document, error: bool, text: str, percentage: float - ) -> None: - return self._print_progress(document, error, text, int(percentage)) - - def print_progress( - self, document: Document, error: bool, untrusted_text: str, percentage: float - ) -> None: - text = replace_control_chars(untrusted_text) - return self.print_progress_trusted( - document, error, "UNTRUSTED> " + text, percentage - ) - - @abstractmethod - def get_max_parallel_conversions(self) -> int: - pass - - def sanitize_conversion_str(self, untrusted_conversion_str: str) -> str: - conversion_string = replace_control_chars(untrusted_conversion_str) - - # Add armor (gpg-style) - armor_start = f"{DOC_TO_PIXELS_LOG_START}\n" - armor_end = DOC_TO_PIXELS_LOG_END - return armor_start + conversion_string + armor_end - - -class ProcessBasedIsolationProvider(IsolationProvider): - # The maximum time it takes a the provider to start up. - STARTUP_TIME_SECONDS = 0 - - def __init__(self) -> None: - self.proc: Optional[subprocess.Popen] = None - super().__init__() - - @abstractmethod - def start_doc_to_pixels_proc(self) -> subprocess.Popen: - pass - - def _convert( - self, - document: Document, - ocr_lang: Optional[str] = None, - ) -> bool: try: with tempfile.TemporaryDirectory() as t: Path(f"{t}/pixels").mkdir() self.doc_to_pixels(document, t) # TODO: validate convert to pixels output self.pixels_to_pdf(document, t, ocr_lang) - return True + document.mark_as_safe() + if document.archive_after_conversion: + document.archive() except errors.InterruptedConversion: assert self.proc is not None error_code = self.proc.wait(3) # XXX Reconstruct exception from error code - raise errors.exception_from_error_code(error_code) # type: ignore [misc] + exception = errors.exception_from_error_code(error_code) + document.mark_as_failed() + except errors.ConversionException as e: + self.print_progress_trusted(document, True, str(e), 0) + document.mark_as_failed() + except Exception as e: + log.exception( + f"An exception occurred while converting document '{document.id}'" + ) + self.print_progress_trusted(document, True, str(e), 0) + document.mark_as_failed() def doc_to_pixels(self, document: Document, tempdir: str) -> None: with open(document.input_filename, "rb") as f: @@ -237,6 +168,50 @@ def pixels_to_pdf( ) -> None: pass + def _print_progress( + self, document: Document, error: bool, text: str, percentage: float + ) -> None: + s = Style.BRIGHT + Fore.YELLOW + f"[doc {document.id}] " + s += Fore.CYAN + f"{percentage}% " + Style.RESET_ALL + if error: + s += Fore.RED + text + Style.RESET_ALL + log.error(s) + else: + s += text + log.info(s) + + if self.progress_callback: + self.progress_callback(error, text, percentage) + + def print_progress_trusted( + self, document: Document, error: bool, text: str, percentage: float + ) -> None: + return self._print_progress(document, error, text, int(percentage)) + + def print_progress( + self, document: Document, error: bool, untrusted_text: str, percentage: float + ) -> None: + text = replace_control_chars(untrusted_text) + return self.print_progress_trusted( + document, error, "UNTRUSTED> " + text, percentage + ) + + @abstractmethod + def get_max_parallel_conversions(self) -> int: + pass + + def sanitize_conversion_str(self, untrusted_conversion_str: str) -> str: + conversion_string = replace_control_chars(untrusted_conversion_str) + + # Add armor (gpg-style) + armor_start = f"{DOC_TO_PIXELS_LOG_START}\n" + armor_end = DOC_TO_PIXELS_LOG_END + return armor_start + conversion_string + armor_end + + @abstractmethod + def start_doc_to_pixels_proc(self) -> subprocess.Popen: + pass + # From global_common: diff --git a/dangerzone/isolation_provider/container.py b/dangerzone/isolation_provider/container.py index 58520493a..7deaf9a3a 100644 --- a/dangerzone/isolation_provider/container.py +++ b/dangerzone/isolation_provider/container.py @@ -17,7 +17,7 @@ get_tmp_dir, replace_control_chars, ) -from .base import ProcessBasedIsolationProvider +from .base import IsolationProvider # Define startupinfo for subprocesses if platform.system() == "Windows": @@ -35,7 +35,7 @@ def __init__(self, container_tech: str) -> None: super().__init__(f"{container_tech} is not installed") -class Container(ProcessBasedIsolationProvider): +class Container(IsolationProvider): # Name of the dangerzone container CONTAINER_NAME = "dangerzone.rocks/dangerzone" STARTUP_TIME_SECONDS = 5 diff --git a/dangerzone/isolation_provider/dummy.py b/dangerzone/isolation_provider/dummy.py deleted file mode 100644 index 3528656a0..000000000 --- a/dangerzone/isolation_provider/dummy.py +++ /dev/null @@ -1,72 +0,0 @@ -import logging -import os -import shutil -import sys -import time -from typing import Callable, Optional - -from ..document import Document -from ..util import get_resource_path -from .base import IsolationProvider - -log = logging.getLogger(__name__) - - -class Dummy(IsolationProvider): - """Dummy Isolation Provider (FOR TESTING ONLY) - - "Do-nothing" converter - the sanitized files are the same as the input files. - Useful for testing without the need to use docker. - """ - - def __init__(self) -> None: - # Sanity check - if not getattr(sys, "dangerzone_dev", False): - raise Exception( - "Dummy isolation provider is UNSAFE and should never be " - + "called in a non-testing system." - ) - - def install(self) -> bool: - return True - - def _convert( - self, - document: Document, - ocr_lang: Optional[str], - ) -> bool: - log.debug("Dummy converter started:") - log.debug( - f" - document: {os.path.basename(document.input_filename)} ({document.id})" - ) - log.debug(f" - ocr : {ocr_lang}") - log.debug("\n(simulating conversion)") - - success = True - - progress = [ - [False, "Converting to PDF using GraphicsMagick", 0.0], - [False, "Separating document into pages", 3.0], - [False, "Converting page 1/1 to pixels", 5.0], - [False, "Converted document to pixels", 50.0], - [False, "Converting page 1/1 from pixels to PDF", 50.0], - [False, "Merging 1 pages into a single PDF", 95.0], - [False, "Compressing PDF", 97.0], - [False, "Safe PDF created", 100.0], - ] - - for error, text, percentage in progress: - self.print_progress(document, error, text, percentage) # type: ignore [arg-type] - if error: - success = False - time.sleep(0.2) - - if success: - shutil.copy( - get_resource_path("dummy_document.pdf"), document.output_filename - ) - - return success - - def get_max_parallel_conversions(self) -> int: - return 1 diff --git a/dangerzone/isolation_provider/qubes.py b/dangerzone/isolation_provider/qubes.py index 1ac9aaa40..b64da4399 100644 --- a/dangerzone/isolation_provider/qubes.py +++ b/dangerzone/isolation_provider/qubes.py @@ -15,16 +15,12 @@ from ..conversion.pixels_to_pdf import PixelsToPDF from ..document import Document from ..util import get_resource_path -from .base import ( - PIXELS_TO_PDF_LOG_END, - PIXELS_TO_PDF_LOG_START, - ProcessBasedIsolationProvider, -) +from .base import PIXELS_TO_PDF_LOG_END, PIXELS_TO_PDF_LOG_START, IsolationProvider log = logging.getLogger(__name__) -class Qubes(ProcessBasedIsolationProvider): +class Qubes(IsolationProvider): """Uses a disposable qube for performing the conversion""" STARTUP_TIME_SECONDS = 5 * 60 # 5 minutes diff --git a/tests/gui/__init__.py b/tests/gui/__init__.py index 061bf4a4f..fcab3b40c 100644 --- a/tests/gui/__init__.py +++ b/tests/gui/__init__.py @@ -10,7 +10,7 @@ from dangerzone.gui import Application from dangerzone.gui.logic import DangerzoneGui from dangerzone.gui.updater import UpdaterThread -from dangerzone.isolation_provider.dummy import Dummy +from dangerzone.isolation_provider.unsafe import UnsafeConverter def get_qt_app() -> Application: @@ -31,14 +31,14 @@ def generate_isolated_updater( else: app = get_qt_app() - dummy = Dummy() + unsafe_converter = UnsafeConverter() # XXX: We can monkey-patch global state without wrapping it in a context manager, or # worrying that it will leak between tests, for two reasons: # # 1. Parallel tests in PyTest take place in different processes. # 2. The monkeypatch fixture tears down the monkey-patch after each test ends. monkeypatch.setattr(util, "get_config_dir", lambda: tmp_path) - dangerzone = DangerzoneGui(app, isolation_provider=dummy) + dangerzone = DangerzoneGui(app, isolation_provider=unsafe_converter) updater = UpdaterThread(dangerzone) return updater diff --git a/tests/gui/test_main_window.py b/tests/gui/test_main_window.py index d7fb66a30..19333a7a5 100644 --- a/tests/gui/test_main_window.py +++ b/tests/gui/test_main_window.py @@ -30,8 +30,8 @@ def content_widget(qtbot: QtBot, mocker: MockerFixture) -> ContentWidget: # Setup mock_app = mocker.MagicMock() - dummy = mocker.MagicMock() - dz = DangerzoneGui(mock_app, dummy) + unsafe_converter = mocker.MagicMock() + dz = DangerzoneGui(mock_app, unsafe_converter) w = ContentWidget(dz) qtbot.addWidget(w) return w diff --git a/tests/isolation_provider/base.py b/tests/isolation_provider/base.py index 52a7d0f0b..0d650bdc1 100644 --- a/tests/isolation_provider/base.py +++ b/tests/isolation_provider/base.py @@ -13,7 +13,7 @@ @pytest.mark.skipif( - os.environ.get("DUMMY_CONVERSION", False), reason="dummy conversions not supported" + os.environ.get("UNSAFE_CONVERSION", False), reason="unsafe conversions not supported" ) @pytest.mark.skipif(not running_on_qubes(), reason="Not on a Qubes system") class IsolationProviderTest: diff --git a/tests/test_cli.py b/tests/test_cli.py index 148918be3..c7b14e555 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -132,8 +132,8 @@ def run_cli( # to tokenize it. args = (args,) - if os.environ.get("DUMMY_CONVERSION", False): - args = ("--unsafe-dummy-conversion", *args) + if os.environ.get("UNSAFE_CONVERSION", False): + args = ("--unsafe-conversion", *args) with tempfile.TemporaryDirectory() as t: tmp_dir = Path(t) @@ -289,11 +289,11 @@ def test_archive(self, tmp_path: Path, sample_pdf: str) -> None: assert os.path.exists(archived_doc_path) assert os.path.exists(safe_doc_path) - def test_dummy_conversion(self, tmp_path: Path, sample_pdf: str) -> None: - result = self.run_cli([sample_pdf, "--unsafe-dummy-conversion"]) + def test_unsafe_conversion(self, tmp_path: Path, sample_pdf: str) -> None: + result = self.run_cli([sample_pdf, "--unsafe-conversion"]) result.assert_success() - def test_dummy_conversion_bulk(self, tmp_path: Path, sample_pdf: str) -> None: + def test_unsafe_conversion_bulk(self, tmp_path: Path, sample_pdf: str) -> None: filenames = ["1.pdf", "2.pdf", "3.pdf"] file_paths = [] for filename in filenames: @@ -301,10 +301,14 @@ def test_dummy_conversion_bulk(self, tmp_path: Path, sample_pdf: str) -> None: shutil.copyfile(sample_pdf, doc_path) file_paths.append(doc_path) - result = self.run_cli(["--unsafe-dummy-conversion", *file_paths]) + result = self.run_cli(["--unsafe-conversion", *file_paths]) result.assert_success() +@pytest.mark.skipif( + os.environ.get("UNSAFE_CONVERSION", False), + reason="Not running extra formats in unsafe mode since test documents are not trusted" +) class TestExtraFormats(TestCli): @for_each_external_doc("*hwp*") def test_hancom_office(self, doc: str) -> None: