public: split BadDataError and Exception error handling

src/auslib/web/public/base.py

@@ -68,35 +68,37 @@ def unicode(error):
     return problem(400, "Unicode Error", "Connexion was unable to parse some unicode data correctly.")
 
 
-@flask_app.errorhandler(Exception)
-def generic(error):
-    """Deals with any unhandled exceptions. If the exception is not a
-    BadDataError, it will be sent to Sentry, and a 400 will be returned,
+@flask_app.errorhandler(BadDataError)
+def baddata(error):
+    """Deals with BadDataError exceptions by returning a 400,
     because BadDataErrors are considered to be the client's fault.
-    Otherwise, the error is just re-raised (which causes a 500)."""
-
+    """
     # Escape exception messages before replying with them, because they may
     # contain user input.
     # See https://bugzilla.mozilla.org/show_bug.cgi?id=1332829 for background.
     # We used to look at error.message here, but that disappeared from many
-    # Exception classes in Python 3, so args is the safer better.
+    # Exception classes in Python 3, so args is the safer bet.
     # We may want to stop returning messages like this to the client altogether
     # both because it's ugly and potentially can leak things, but it's also
     # extremely helpful for debugging BadDataErrors, because we don't send
     # information about them to Sentry.
-
     if hasattr(error, "args"):
         message = " ".join(str(a) for a in error.args)
     else:
         message = repr(error)
-    if isinstance(error, BadDataError):
-        return Response(status=400, mimetype="text/plain", response=html.escape(message, quote=False))
+
+    return Response(status=400, mimetype="text/plain", response=html.escape(message, quote=False))
+
+
+@flask_app.errorhandler(Exception)
+def generic(error):
+    """Deals with any unhandled exceptions. It will be sent to Sentry, and re-raised (which causes a 500)."""
 
     # Sentry doesn't handle exceptions for `@flask_app.errorhandler(Exception)`
     # implicitly. If Sentry is not configured, the following call returns None.
     capture_exception(error)
 
-    return Response(status=500, mimetype="text/plain", response=html.escape(message, quote=False))
+    raise
 
 
 # Keeping static files endpoints here due to an issue when returning response for static files.

tests/web/test_client.py

@@ -2215,6 +2215,7 @@ class ClientTestWithErrorHandlers(ClientTestCommon):
 
     def setUp(self):
         app.config["DEBUG"] = True
+        app.config["PROPAGATE_EXCEPTIONS"] = False
         app.config["ALLOWLISTED_DOMAINS"] = {"a.com": ("a",)}
         dbo.setDb("sqlite:///:memory:")
         self.metadata.create_all(dbo.engine)
@@ -2301,44 +2302,6 @@ def test404ResponseForNonUpdateEndpoint(self, path):
         ret = self.client.get(path)
         self.assertEqual(ret.status_code, 404)
 
-    def testErrorMessageOn500(self):
-        with mock.patch("auslib.web.public.client.getQueryFromURL") as m:
-            m.side_effect = Exception("I break!")
-            ret = self.client.get("/update/4/b/1.0/1/p/l/a/a/a/a/1/update.xml")
-            self.assertEqual(ret.status_code, 500)
-            self.assertEqual(ret.mimetype, "text/plain")
-            self.assertEqual("I break!", ret.get_data(as_text=True))
-
-    def testErrorMessageOn500withSimpleArgs(self):
-        with mock.patch("auslib.web.public.client.getQueryFromURL") as m:
-            m.side_effect = Exception("I break!")
-            m.side_effect.args = ("one", "two", "three")
-            ret = self.client.get("/update/4/b/1.0/1/p/l/a/a/a/a/1/update.xml")
-            self.assertEqual(ret.status_code, 500)
-            self.assertEqual(ret.mimetype, "text/plain")
-            data = ret.get_data(as_text=True)
-            for arg in ("one", "two", "three"):
-                self.assertIn(arg, data)
-
-    def testErrorMessageOn500withComplexArgs(self):
-        with mock.patch("auslib.web.public.client.getQueryFromURL") as m:
-            m.side_effect = Exception("I break!")
-            m.side_effect.args = ("one", ("two", "three"))
-            ret = self.client.get("/update/4/b/1.0/1/p/l/a/a/a/a/1/update.xml")
-            self.assertEqual(ret.status_code, 500)
-            self.assertEqual(ret.mimetype, "text/plain")
-            data = ret.get_data(as_text=True)
-            for arg in ("one", "two", "three"):
-                self.assertIn(arg, data)
-
-    def testEscapedOutputOn500(self):
-        with mock.patch("auslib.web.public.client.getQueryFromURL") as m:
-            m.side_effect = Exception("50.1.0zibj5<img src%3da onerror%3dalert(document.domain)>")
-            ret = self.client.get("/update/4/b/1.0/1/p/l/a/a/a/a/1/update.xml")
-            self.assertEqual(ret.status_code, 500)
-            self.assertEqual(ret.mimetype, "text/plain")
-            self.assertEqual("50.1.0zibj5&lt;img src%3da onerror%3dalert(document.domain)&gt;", ret.get_data(as_text=True))
-
     def testEscapedOutputOn400(self):
         with mock.patch("auslib.web.public.client.getQueryFromURL") as m:
             m.side_effect = BadDataError("Version number 50.1.0zibj5<img src%3da onerror%3dalert(document.domain)> is invalid.")
@@ -2361,9 +2324,9 @@ def testSentryRealError(self):
             m.side_effect = Exception("exterminate!")
             ret = self.client.get("/update/4/b/1.0/1/p/l/a/a/a/a/1/update.xml")
             self.assertEqual(ret.status_code, 500)
-            self.assertEqual(ret.mimetype, "text/plain")
+            self.assertEqual(ret.mimetype, "application/problem+json")
             self.assertTrue(sentry.called)
-            self.assertEqual("exterminate!", ret.get_data(as_text=True))
+            self.assertNotIn("exterminate!", ret.get_data(as_text=True))
 
     def testNonSubstitutedUrlVariablesReturnEmptyUpdate(self):
         request1 = "/update/1/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/update.xml"