Refactor cert error handling to act on single errors

With QtNetwork, we can get multiple errors at once (reproduced e.g. by
using the Superfish certificate on badssl.com). With QtWebEngine, we can
only get one error.

Things like "is this error coming from a third-party resource" are hard
to reason about if there is a list of errors, so change
CertificateErrorWrapper so that it's exposed as one single error to the
rest of the code.

7 files changed, 63 insertions, 69 deletions

diff --git a/qutebrowser/browser/shared.py b/qutebrowser/browser/shared.py
index a03a4f6e2..7e2b6dc2a 100644
--- a/qutebrowser/browser/shared.py
+++ b/qutebrowser/browser/shared.py
@@ -23,10 +23,10 @@ import os
 import sys
 import html
 import netrc
-from typing import Callable, Mapping, List, Optional
+from typing import Callable, Mapping, List, Optional, Iterable
 import tempfile
 
-from PyQt5.QtCore import QUrl
+from PyQt5.QtCore import QUrl, pyqtBoundSignal
 
 from qutebrowser.config import config
 from qutebrowser.utils import (usertypes, message, log, objreg, jinja, utils,
@@ -155,35 +155,36 @@ def javascript_log_message(level, source, line, msg):
     logger(logstring)
 
 
-def ignore_certificate_errors(url, errors, abort_on):
+def ignore_certificate_error(
+        url: QUrl,
+        error: usertypes.AbstractCertificateErrorWrapper,
+        abort_on: Iterable[pyqtBoundSignal],
+) -> bool:
     """Display a certificate error question.
 
     Args:
         url: The URL the errors happened in
-        errors: A list of QSslErrors or QWebEngineCertificateErrors
+        errors: A single error.
+        abort_on: Signals aborting a question.
 
     Return:
         True if the error should be ignored, False otherwise.
     """
     conf = config.instance.get('content.tls.certificate_errors', url=url)
-    log.network.debug(f"Certificate errors {errors!r}, config {conf}")
+    log.network.debug(f"Certificate error {error!r}, config {conf}")
 
-    for error in errors:
-        assert error.is_overridable(), repr(error)
+    assert error.is_overridable(), repr(error)
 
     if conf == 'ask':
         err_template = jinja.environment.from_string("""
-            Errors while loading <b>{{url.toDisplayString()}}</b>:<br/>
-            <ul>
-            {% for err in errors %}
-                <li>{{err}}</li>
-            {% endfor %}
-            </ul>
+            <p>Error while loading <b>{{url.toDisplayString()}}</b>:</p>
+
+            <p>{{error|replace('\n', '<br>')|safe}}</p>
         """.strip())
-        msg = err_template.render(url=url, errors=errors)
+        msg = err_template.render(url=url, error=error)
 
         urlstr = url.toString(QUrl.RemovePassword | QUrl.FullyEncoded)
-        ignore = message.ask(title="Certificate errors - continue?", text=msg,
+        ignore = message.ask(title="Certificate error - continue?", text=msg,
                              mode=usertypes.PromptMode.yesno, default=False,
                              abort_on=abort_on, url=urlstr)
         if ignore is None:
@@ -191,8 +192,7 @@ def ignore_certificate_errors(url, errors, abort_on):
             ignore = False
         return ignore
     elif conf == 'load-insecurely':
-        for err in errors:
-            message.error(f'Certificate error: {err}')
+        message.error(f'Certificate error: {error}')
         return True
     elif conf == 'block':
         return False
diff --git a/qutebrowser/browser/webengine/certificateerror.py b/qutebrowser/browser/webengine/certificateerror.py
index 8cd3f25b3..4df7ce8ab 100644
--- a/qutebrowser/browser/webengine/certificateerror.py
+++ b/qutebrowser/browser/webengine/certificateerror.py
@@ -19,6 +19,7 @@
 
 """Wrapper over a QWebEngineCertificateError."""
 
+from PyQt5.QtCore import QUrl
 from PyQt5.QtWebEngineWidgets import QWebEngineCertificateError
 
 from qutebrowser.utils import usertypes, utils, debug
@@ -28,21 +29,21 @@ class CertificateErrorWrapper(usertypes.AbstractCertificateErrorWrapper):
 
     """A wrapper over a QWebEngineCertificateError."""
 
-    def __init__(self, error):
-        super().__init__(error)
+    def __init__(self, error: QWebEngineCertificateError) -> None:
+        self._error = error
         self.ignore = False
 
-    def __str__(self):
+    def __str__(self) -> str:
         return self._error.errorDescription()
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return utils.get_repr(
-            self, error=debug.qenum_key(QWebEngineCertificateError,
-                                        self._error.error()),
+            self,
+            error=debug.qenum_key(QWebEngineCertificateError, self._error.error()),
             string=str(self))
 
-    def url(self):
+    def url(self) -> QUrl:
         return self._error.url()
 
-    def is_overridable(self):
+    def is_overridable(self) -> bool:
         return self._error.isOverridable()
diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py
index 69ddbe6e1..3c113e55c 100644
--- a/qutebrowser/browser/webengine/webenginetab.py
+++ b/qutebrowser/browser/webengine/webenginetab.py
@@ -1519,8 +1519,8 @@ class WebEngineTab(browsertab.AbstractTab):
         log.network.debug("Certificate error: {}".format(error))
 
         if error.is_overridable():
-            error.ignore = shared.ignore_certificate_errors(
-                url, [error], abort_on=[self.abort_questions])
+            error.ignore = shared.ignore_certificate_error(
+                url, error, abort_on=[self.abort_questions])
         else:
             log.network.error("Non-overridable certificate error: "
                               "{}".format(error))
diff --git a/qutebrowser/browser/webkit/certificateerror.py b/qutebrowser/browser/webkit/certificateerror.py
index 38166ec35..68c0de775 100644
--- a/qutebrowser/browser/webkit/certificateerror.py
+++ b/qutebrowser/browser/webkit/certificateerror.py
@@ -17,8 +17,9 @@
 # You should have received a copy of the GNU General Public License
 # along with qutebrowser.  If not, see <https://www.gnu.org/licenses/>.
 
-"""Wrapper over a QSslError."""
+"""A wrapper over a list of QSslErrors."""
 
+from typing import Sequence
 
 from PyQt5.QtNetwork import QSslError
 
@@ -27,21 +28,27 @@ from qutebrowser.utils import usertypes, utils, debug
 
 class CertificateErrorWrapper(usertypes.AbstractCertificateErrorWrapper):
 
-    """A wrapper over a QSslError."""
+    """A wrapper over a list of QSslErrors."""
 
-    def __str__(self):
-        return self._error.errorString()
+    def __init__(self, errors: Sequence[QSslError]) -> None:
+        self._errors = tuple(errors)  # needs to be hashable
 
-    def __repr__(self):
+    def __str__(self) -> str:
+        return '\n'.join('- ' + err.errorString() for err in self._errors)
+
+    def __repr__(self) -> str:
         return utils.get_repr(
-            self, error=debug.qenum_key(QSslError, self._error.error()),
+            self,
+            errors=[debug.qenum_key(QSslError, err.error()) for err in self._errors],
             string=str(self))
 
-    def __hash__(self):
-        return hash(self._error)
+    def __hash__(self) -> int:
+        return hash(self._errors)
 
-    def __eq__(self, other):
-        return self._error == other._error
+    def __eq__(self, other: object) -> bool:
+        if not isinstance(other, CertificateErrorWrapper):
+            return NotImplemented
+        return self._errors == other._errors
 
-    def is_overridable(self):
+    def is_overridable(self) -> bool:
         return True
diff --git a/qutebrowser/browser/webkit/network/networkmanager.py b/qutebrowser/browser/webkit/network/networkmanager.py
index f81e97f52..a600667e9 100644
--- a/qutebrowser/browser/webkit/network/networkmanager.py
+++ b/qutebrowser/browser/webkit/network/networkmanager.py
@@ -22,7 +22,7 @@
 import collections
 import html
 import dataclasses
-from typing import TYPE_CHECKING, Dict, MutableMapping, Optional, Sequence
+from typing import TYPE_CHECKING, Dict, MutableMapping, Optional, Set
 
 from PyQt5.QtCore import pyqtSlot, pyqtSignal, QUrl, QByteArray
 from PyQt5.QtNetwork import (QNetworkAccessManager, QNetworkReply, QSslSocket,
@@ -122,7 +122,7 @@ def init():
         QSslSocket.setDefaultCiphers(good_ciphers)
 
 
-_SavedErrorsType = MutableMapping[urlutils.HostTupleType, Sequence[QSslError]]
+_SavedErrorsType = MutableMapping[urlutils.HostTupleType, Set[QSslError]]
 
 
 class NetworkManager(QNetworkAccessManager):
@@ -141,8 +141,8 @@ class NetworkManager(QNetworkAccessManager):
                  (or None for generic network managers)
         _tab_id: The tab ID this NetworkManager is associated with.
                  (or None for generic network managers)
-        _rejected_ssl_errors: A {QUrl: [SslError]} dict of rejected errors.
-        _accepted_ssl_errors: A {QUrl: [SslError]} dict of accepted errors.
+        _rejected_ssl_errors: A {QUrl: {SslError}} dict of rejected errors.
+        _accepted_ssl_errors: A {QUrl: {SslError}} dict of accepted errors.
         _private: Whether we're in private browsing mode.
         netrc_used: Whether netrc authentication was performed.
 
@@ -172,8 +172,8 @@ class NetworkManager(QNetworkAccessManager):
         self._set_cookiejar()
         self._set_cache()
         self.sslErrors.connect(self.on_ssl_errors)
-        self._rejected_ssl_errors: _SavedErrorsType = collections.defaultdict(list)
-        self._accepted_ssl_errors: _SavedErrorsType = collections.defaultdict(list)
+        self._rejected_ssl_errors: _SavedErrorsType = collections.defaultdict(set)
+        self._accepted_ssl_errors: _SavedErrorsType = collections.defaultdict(set)
         self.authenticationRequired.connect(self.on_authentication_required)
         self.proxyAuthenticationRequired.connect(self.on_proxy_authentication_required)
         self.netrc_used = False
@@ -221,18 +221,17 @@ class NetworkManager(QNetworkAccessManager):
 
     # No @pyqtSlot here, see
     # https://github.com/qutebrowser/qutebrowser/issues/2213
-    def on_ssl_errors(self, reply, errors):  # noqa: C901 pragma: no mccabe
+    def on_ssl_errors(self, reply, qt_errors):  # noqa: C901 pragma: no mccabe
         """Decide if SSL errors should be ignored or not.
 
         This slot is called on SSL/TLS errors by the self.sslErrors signal.
 
         Args:
             reply: The QNetworkReply that is encountering the errors.
-            errors: A list of errors.
+            qt_errors: A list of errors.
         """
-        errors = [certificateerror.CertificateErrorWrapper(e) for e in errors]
-        log.network.debug("Certificate errors: {!r}".format(
-            ' / '.join(str(err) for err in errors)))
+        errors = certificateerror.CertificateErrorWrapper(qt_errors)
+        log.network.debug("Certificate errors: {!r}".format(errors))
         try:
             host_tpl: Optional[urlutils.HostTupleType] = urlutils.host_tuple(
                 reply.url())
@@ -242,10 +241,8 @@ class NetworkManager(QNetworkAccessManager):
             is_rejected = False
         else:
             assert host_tpl is not None
-            is_accepted = set(errors).issubset(
-                self._accepted_ssl_errors[host_tpl])
-            is_rejected = set(errors).issubset(
-                self._rejected_ssl_errors[host_tpl])
+            is_accepted = errors in self._accepted_ssl_errors[host_tpl]
+            is_rejected = errors in self._rejected_ssl_errors[host_tpl]
 
         log.network.debug("Already accepted: {} / "
                           "rejected {}".format(is_accepted, is_rejected))
@@ -257,15 +254,13 @@ class NetworkManager(QNetworkAccessManager):
             return
 
         abort_on = self._get_abort_signals(reply)
-        ignore = shared.ignore_certificate_errors(reply.url(), errors,
-                                                  abort_on=abort_on)
+        ignore = shared.ignore_certificate_error(reply.url(), errors, abort_on=abort_on)
         if ignore:
             reply.ignoreSslErrors()
-            err_dict = self._accepted_ssl_errors
-        else:
-            err_dict = self._rejected_ssl_errors
-        if host_tpl is not None:
-            err_dict[host_tpl] += errors
+            if host_tpl is not None:
+                self._accepted_ssl_errors[host_tpl].add(errors)
+        elif host_tpl is not None:
+            self._rejected_ssl_errors[host_tpl].add(errors)
 
     def clear_all_ssl_errors(self):
         """Clear all remembered SSL errors."""
diff --git a/qutebrowser/utils/usertypes.py b/qutebrowser/utils/usertypes.py
index f078418bc..dd2535e26 100644
--- a/qutebrowser/utils/usertypes.py
+++ b/qutebrowser/utils/usertypes.py
@@ -483,9 +483,6 @@ class AbstractCertificateErrorWrapper:
 
     """A wrapper over an SSL/certificate error."""
 
-    def __init__(self, error: Any) -> None:
-        self._error = error
-
     def __str__(self) -> str:
         raise NotImplementedError
 
diff --git a/tests/unit/utils/usertypes/test_misc.py b/tests/unit/utils/usertypes/test_misc.py
index 6ed0149b4..216ec70b2 100644
--- a/tests/unit/utils/usertypes/test_misc.py
+++ b/tests/unit/utils/usertypes/test_misc.py
@@ -21,12 +21,6 @@
 from qutebrowser.utils import usertypes
 
 
-def test_abstract_certificate_error_wrapper():
-    err = object()
-    wrapper = usertypes.AbstractCertificateErrorWrapper(err)
-    assert wrapper._error is err
-
-
 def test_unset_object_identity():
     assert usertypes.Unset() is not usertypes.Unset()
     assert usertypes.UNSET is usertypes.UNSET