From 9410b68c1df82b03d568e584bf87942d5224d865 Mon Sep 17 00:00:00 2001
From: Alex Pyrgiotis <alex.p@freedom.press>
Date: Fri, 28 Jul 2023 18:47:16 +0300
Subject: [PATCH] Sanitize progress reports in a provider-agnostic way

Update the common `print_progress()` method in the base
`IsolationProvider` class, with two extra features:

1. Always sanitize the provided text argument.
2. Mark the sanitized text argument as untrusted.

This is default behavior from now on, since this function is commonly
used to parse progress reports from the conversion sandbox.
---
 CHANGELOG.md                          |  2 +
 dangerzone/isolation_provider/base.py | 16 +++++++-
 tests/isolation_provider/test_base.py | 56 +++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 tests/isolation_provider/test_base.py

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f615ac4..23049b3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,8 @@ since 0.4.1, and this project adheres to [Semantic Versioning](https://semver.or
 
 - Continuously scan our Python dependencies and container image for
   vulnerabilities ([issue #222](https://github.com/freedomofpress/dangerzone/issues/222))
+- Sanitize potentially unsafe characters from strings that are shown in a user's
+  terminal.
 
 ## Dangerzone 0.4.1
 
diff --git a/dangerzone/isolation_provider/base.py b/dangerzone/isolation_provider/base.py
index aa02c54..38f02c7 100644
--- a/dangerzone/isolation_provider/base.py
+++ b/dangerzone/isolation_provider/base.py
@@ -6,6 +6,7 @@ from typing import Callable, Optional
 from colorama import Fore, Style
 
 from ..document import Document
+from ..util import replace_control_chars
 
 log = logging.getLogger(__name__)
 
@@ -49,7 +50,7 @@ class IsolationProvider(ABC):
     ) -> bool:
         pass
 
-    def print_progress(
+    def _print_progress(
         self, document: Document, error: bool, text: str, percentage: float
     ) -> None:
         s = Style.BRIGHT + Fore.YELLOW + f"[doc {document.id}] "
@@ -64,6 +65,19 @@ class IsolationProvider(ABC):
         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, 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
diff --git a/tests/isolation_provider/test_base.py b/tests/isolation_provider/test_base.py
new file mode 100644
index 0000000..c87acc7
--- /dev/null
+++ b/tests/isolation_provider/test_base.py
@@ -0,0 +1,56 @@
+import pytest
+from colorama import Style
+from pytest_mock import MockerFixture
+
+from dangerzone.document import Document
+from dangerzone.isolation_provider import base, container, qubes
+
+from .. import sanitized_text, uncommon_text
+
+
+@pytest.mark.parametrize(
+    "provider",
+    [
+        container.Container(enable_timeouts=False),
+        qubes.Qubes(),
+    ],
+    ids=["Container", "Qubes"],
+)
+def test_print_progress(
+    provider: base.IsolationProvider,
+    uncommon_text: str,
+    sanitized_text: str,
+    mocker: MockerFixture,
+) -> None:
+    """Test that the print_progress() method of our isolation providers sanitizes text.
+
+    Iterate our isolation providers and make sure that their print_progress() methods
+    sanitizes the provided text, before passing it to the logging functions and other
+    callbacks.
+    """
+    d = Document()
+    provider.progress_callback = mocker.MagicMock()
+    log_info_spy = mocker.spy(base.log, "info")
+    log_error_spy = mocker.spy(base.log, "error")
+    _print_progress_spy = mocker.spy(provider, "_print_progress")
+
+    for error, untrusted_text, sanitized_text in [
+        (True, "normal text", "UNTRUSTED> normal text"),
+        (False, "normal text", "UNTRUSTED> normal text"),
+        (True, uncommon_text, "UNTRUSTED> " + sanitized_text),
+        (False, uncommon_text, "UNTRUSTED> " + sanitized_text),
+    ]:
+        log_info_spy.reset_mock()
+        log_error_spy.reset_mock()
+
+        provider.print_progress(d, error, untrusted_text, 0)
+        provider.progress_callback.assert_called_with(error, sanitized_text, 0)  # type: ignore [union-attr]
+        _print_progress_spy.assert_called_with(d, error, sanitized_text, 0)
+        if error:
+            assert log_error_spy.call_args[0][0].endswith(
+                sanitized_text + Style.RESET_ALL
+            )
+            log_info_spy.assert_not_called()
+        else:
+            assert log_info_spy.call_args[0][0].endswith(sanitized_text)
+            log_error_spy.assert_not_called()