fix(asm): capture XML parsing errors on body parse (#4559) (#4575)

* fix(asm): capture XML parsing errors on body parse

Signed-off-by: Juanjo Alvarez <[email protected]>

* chore(asm): add some tests for bad XML not raising an exception but logging a warning

Signed-off-by: Juanjo Alvarez <[email protected]>

* Update ddtrace/contrib/django/utils.py

Co-authored-by: Brett Langdon <[email protected]>

* Update releasenotes/notes/capture-xml-parsing-errors-e6c8c761ed026ce3.yaml

Co-authored-by: Brett Langdon <[email protected]>

* chore(asm): fix empty xml body flask test

Signed-off-by: Juanjo Alvarez <[email protected]>

* chore(asm): workaround some odd interaction between caplog and hypothesis

Signed-off-by: Juanjo Alvarez <[email protected]>

Signed-off-by: Juanjo Alvarez <[email protected]>
Co-authored-by: Brett Langdon <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 91e5dfd)

Co-authored-by: Juanjo Alvarez Martinez <[email protected]>

Author: mergify[bot]

ddtrace/contrib/django/utils.py

@@ -273,6 +273,8 @@ def _extract_body(request):
             OSError,
             ValueError,
             JSONDecodeError,
+            xmltodict.expat.ExpatError,
+            xmltodict.ParsingInterrupted,
         ):
             log.warning("Failed to parse request body")
             # req_body is None

ddtrace/contrib/flask/patch.py

@@ -385,7 +385,16 @@ def traced_wsgi_app(pin, wrapped, instance, args, kwargs):
                     req_body = request.form.to_dict()
                 else:
                     req_body = request.get_data()
-            except (AttributeError, RuntimeError, TypeError, BadRequest, ValueError, JSONDecodeError):
+            except (
+                AttributeError,
+                RuntimeError,
+                TypeError,
+                BadRequest,
+                ValueError,
+                JSONDecodeError,
+                xmltodict.expat.ExpatError,
+                xmltodict.ParsingInterrupted,
+            ):
                 log.warning("Failed to parse werkzeug request body", exc_info=True)
             finally:
                 # Reset wsgi input to the beginning

ddtrace/contrib/pylons/middleware.py

@@ -110,7 +110,14 @@ def __call__(self, environ, start_response):
                     else:  # text/plain, xml, others: take them as strings
                         req_body = request.body.decode("UTF-8")
 
-                except (AttributeError, OSError, ValueError, JSONDecodeError):
+                except (
+                    AttributeError,
+                    OSError,
+                    ValueError,
+                    JSONDecodeError,
+                    xmltodict.expat.ExpatError,
+                    xmltodict.ParsingInterrupted,
+                ):
                     log.warning("Failed to parse request body", exc_info=True)
                     # req_body is None
 

releasenotes/notes/capture-xml-parsing-errors-e6c8c761ed026ce3.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    ASM: Do not raise exceptions when failing to parse XML request body.

tests/contrib/django/test_django_appsec.py

@@ -207,8 +207,11 @@ def test_django_request_body_plain_attack(client, test_spans, tracer):
         assert query == "1' or '1' = '1'"
 
 
-def test_django_request_body_json_empty(caplog, client, test_spans, tracer):
-    with caplog.at_level(logging.WARNING), override_global_config(dict(_appsec_enabled=True)), override_env(
+def test_django_request_body_json_bad(caplog, client, test_spans, tracer):
+    # Note: there is some odd interaction between hypotheses or pytest and
+    # caplog where if you set this to WARNING the second test won't get
+    # output unless you set all to DEBUG.
+    with caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)), override_env(
         dict(DD_APPSEC_RULES=RULES_GOOD_PATH)
     ):
         payload = '{"attack": "bad_payload",}'
@@ -225,6 +228,23 @@ def test_django_request_body_json_empty(caplog, client, test_spans, tracer):
         assert "Failed to parse request body" in caplog.text
 
 
+def test_django_request_body_xml_bad_logs_warning(caplog, client, test_spans, tracer):
+    # see above about caplog
+    with caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)), override_env(
+        dict(DD_APPSEC_RULES=RULES_GOOD_PATH)
+    ):
+        _, response = _aux_appsec_get_root_span(
+            client,
+            test_spans,
+            tracer,
+            payload="bad xml",
+            content_type="application/xml",
+        )
+
+        assert response.status_code == 200
+        assert "Failed to parse request body" in caplog.text
+
+
 def test_django_path_params(client, test_spans, tracer):
     with override_global_config(dict(_appsec_enabled=True)):
         root_span, _ = _aux_appsec_get_root_span(

tests/contrib/flask/test_flask_appsec.py

@@ -229,7 +229,25 @@ def test_flask_body_xml_attack(self):
             assert query == {"attack": "1' or '1' = '1'"}
 
     def test_flask_body_json_empty_body_logs_warning(self):
-        with self._caplog.at_level(logging.WARNING), override_global_config(dict(_appsec_enabled=True)):
+        with self._caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)):
             self._aux_appsec_prepare_tracer()
             self.client.post("/", data="", content_type="application/json")
             assert "Failed to parse werkzeug request body" in self._caplog.text
+
+    def test_flask_body_json_bad_logs_warning(self):
+        with self._caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)):
+            self._aux_appsec_prepare_tracer()
+            self.client.post("/", data="not valid json", content_type="application/json")
+            assert "Failed to parse werkzeug request body" in self._caplog.text
+
+    def test_flask_body_xml_bad_logs_warning(self):
+        with self._caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)):
+            self._aux_appsec_prepare_tracer()
+            self.client.post("/", data="bad xml", content_type="application/xml")
+            assert "Failed to parse werkzeug request body" in self._caplog.text
+
+    def test_flask_body_xml_empty_logs_warning(self):
+        with self._caplog.at_level(logging.DEBUG), override_global_config(dict(_appsec_enabled=True)):
+            self._aux_appsec_prepare_tracer()
+            self.client.post("/", data="", content_type="application/xml")
+            assert "Failed to parse werkzeug request body" in self._caplog.text