fix(appsec): flask hangs on empty body requests [backport 1.18] (#7295)

Backport cc35faf from #7266 to 1.18.

This PR fix #7265

Local and debug Flask applications hang when Appsec
`_on_request_span_modifier` handler calls to `wsgi_input.read()` and the
body is empty

### Example

file1.py:
```python
@app.route("/test-body-hang", methods=["POST"])
def apsec_body_hang():
    return "OK", 200


if __name__ == "__main__":
    app.run(port=8000)
```
then:
```bash
DD_APPSEC_ENABLED=true poetry run ddtrace-run python -m file1:app
# or
DD_APPSEC_ENABLED=true python -m ddtrace-run python file1.py
```

If you make an empty POST request `curl -X POST '127.0.0.1:8000/'` Flask
hangs when the ASM handler tries to read the body

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

Author: avara1986

ddtrace/appsec/handlers.py

@@ -55,7 +55,7 @@ def _on_request_span_modifier(request, environ, _HAS_JSON_MIXIN, exception_type)
                 seekable = False
             if not seekable:
                 content_length = int(environ.get("CONTENT_LENGTH", 0))
-                body = wsgi_input.read(content_length) if content_length else wsgi_input.read()
+                body = wsgi_input.read(content_length) if content_length else b""
                 environ["wsgi.input"] = BytesIO(body)
 
         try:

releasenotes/notes/asm-fix-flask-body-blocking-aa4836d70f8e1ea8.yaml

@@ -0,0 +1,4 @@
+---
+issues:
+  - |
+    ASM: fix a body read problem on some corner case where passing empty content length makes wsgi.input.read() blocks.

tests/appsec/app.py

@@ -1,3 +1,5 @@
+""" This Flask application is imported on tests.appsec.appsec_utils.gunicorn_server
+"""
 from flask import Flask
 
 
@@ -9,4 +11,13 @@
 
 @app.route("/")
 def index():
-    return "OK", 200
+    return "OK_index", 200
+
+
+@app.route("/test-body-hang", methods=["POST"])
+def apsec_body_hang():
+    return "OK_test-body-hang", 200
+
+
+if __name__ == "__main__":
+    app.run(debug=True, port=8000)

tests/appsec/appsec_utils.py

@@ -0,0 +1,89 @@
+from contextlib import contextmanager
+import os
+import signal
+import subprocess
+import sys
+
+from ddtrace.internal.utils.retry import RetryError
+from ddtrace.vendor import psutil
+from tests.webclient import Client
+
+
+ROOT_DIR = os.path.dirname(os.path.abspath(__file__))
+ROOT_PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+
+def _build_env():
+    environ = dict(PATH="%s:%s" % (ROOT_PROJECT_DIR, ROOT_DIR), PYTHONPATH="%s:%s" % (ROOT_PROJECT_DIR, ROOT_DIR))
+    if os.environ.get("PATH"):
+        environ["PATH"] = "%s:%s" % (os.environ.get("PATH"), environ["PATH"])
+    if os.environ.get("PYTHONPATH"):
+        environ["PYTHONPATH"] = "%s:%s" % (os.environ.get("PYTHONPATH"), environ["PYTHONPATH"])
+    return environ
+
+
+@contextmanager
+def gunicorn_server(appsec_enabled="true", remote_configuration_enabled="true", token=None):
+    cmd = ["gunicorn", "-w", "3", "-b", "0.0.0.0:8000", "tests.appsec.app:app"]
+    for res in appsec_application_server(
+        cmd, appsec_enabled=appsec_enabled, remote_configuration_enabled=remote_configuration_enabled, token=token
+    ):
+        yield res
+
+
+@contextmanager
+def flask_server(appsec_enabled="true", remote_configuration_enabled="true", token=None):
+    cmd = ["python", "tests/appsec/app.py", "--no-reload"]
+    for res in appsec_application_server(
+        cmd, appsec_enabled=appsec_enabled, remote_configuration_enabled=remote_configuration_enabled, token=token
+    ):
+        yield res
+
+
+def appsec_application_server(cmd, appsec_enabled="true", remote_configuration_enabled="true", token=None):
+    env = _build_env()
+    env["DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"] = "0.5"
+    env["DD_REMOTE_CONFIGURATION_ENABLED"] = remote_configuration_enabled
+    if token:
+        env["_DD_REMOTE_CONFIGURATION_ADDITIONAL_HEADERS"] = "X-Datadog-Test-Session-Token:%s," % (token,)
+    if appsec_enabled:
+        env["DD_APPSEC_ENABLED"] = appsec_enabled
+    env["DD_TRACE_AGENT_URL"] = os.environ.get("DD_TRACE_AGENT_URL", "")
+
+    server_process = subprocess.Popen(
+        cmd,
+        env=env,
+        stdout=sys.stdout,
+        stderr=sys.stderr,
+        preexec_fn=os.setsid,
+    )
+    try:
+        client = Client("http://0.0.0.0:8000")
+
+        try:
+            print("Waiting for server to start")
+            client.wait(max_tries=100, delay=0.1)
+            print("Server started")
+        except RetryError:
+            raise AssertionError(
+                "Server failed to start, see stdout and stderr logs"
+                "\n=== Captured STDOUT ===\n%s=== End of captured STDOUT ==="
+                "\n=== Captured STDERR ===\n%s=== End of captured STDERR ==="
+                % (server_process.stdout, server_process.stderr)
+            )
+        # If we run a Gunicorn application, we want to get the child's pid, see test_remoteconfiguration_e2e.py
+        parent = psutil.Process(server_process.pid)
+        children = parent.children(recursive=True)
+
+        yield server_process, client, (children[1].pid if len(children) > 1 else None)
+        try:
+            client.get_ignored("/shutdown")
+        except Exception:
+            raise AssertionError(
+                "\n=== Captured STDOUT ===\n%s=== End of captured STDOUT ==="
+                "\n=== Captured STDERR ===\n%s=== End of captured STDERR ==="
+                % (server_process.stdout, server_process.stderr)
+            )
+    finally:
+        os.killpg(os.getpgid(server_process.pid), signal.SIGTERM)
+        server_process.wait()

tests/appsec/test_handlers.py

@@ -0,0 +1,47 @@
+from tests.appsec.appsec_utils import flask_server
+from tests.appsec.appsec_utils import gunicorn_server
+
+
+def test_flask_when_appsec_reads_empty_body_hang():
+    """A bug was detected when running a Flask application locally
+
+    file1.py:
+        app.run(debug=True, port=8000)
+
+    then:
+       DD_APPSEC_ENABLED=true poetry run ddtrace-run python -m file1:app
+       DD_APPSEC_ENABLED=true python -m ddtrace-run python file1.py
+
+    If you make an empty POST request (curl -X POST '127.0.0.1:8000/'), Flask hangs when the ASM handler tries to read
+    an empty body
+    """
+    with flask_server(remote_configuration_enabled="false", token=None) as context:
+        _, flask_client, pid = context
+
+        response = flask_client.get("/")
+
+        assert response.status_code == 200
+        assert response.content == b"OK_index"
+
+        response = flask_client.post("/test-body-hang")
+
+        assert response.status_code == 200
+        assert response.content == b"OK_test-body-hang"
+
+
+def test_gunicorn_when_appsec_reads_empty_body_no_hang():
+    """In relation to the previous test, Gunicorn works correctly, but we added a regression test to ensure that
+    no similar bug arises in a production application.
+    """
+    with gunicorn_server(remote_configuration_enabled="false", token=None) as context:
+        _, gunicorn_client, pid = context
+
+        response = gunicorn_client.get("/")
+
+        assert response.status_code == 200
+        assert response.content == b"OK_index"
+
+        response = gunicorn_client.post("/test-body-hang")
+
+        assert response.status_code == 200
+        assert response.content == b"OK_test-body-hang"

tests/appsec/test_remoteconfiguration_e2e.py

@@ -1,12 +1,10 @@
 import base64
-from contextlib import contextmanager
 import datetime
 import hashlib
 import json
 from multiprocessing.pool import ThreadPool
 import os
 import signal
-import subprocess
 import sys
 import time
 import uuid
@@ -16,72 +14,7 @@
 from ddtrace import tracer
 from ddtrace.internal.compat import httplib
 from ddtrace.internal.compat import parse
-from ddtrace.internal.utils.retry import RetryError
-from ddtrace.vendor import psutil
-from tests.webclient import Client
-
-
-ROOT_DIR = os.path.dirname(os.path.abspath(__file__))
-ROOT_PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
-
-
-def _build_env():
-    environ = dict(PATH="%s:%s" % (ROOT_PROJECT_DIR, ROOT_DIR), PYTHONPATH="%s:%s" % (ROOT_PROJECT_DIR, ROOT_DIR))
-    if os.environ.get("PATH"):
-        environ["PATH"] = "%s:%s" % (os.environ.get("PATH"), environ["PATH"])
-    if os.environ.get("PYTHONPATH"):
-        environ["PYTHONPATH"] = "%s:%s" % (os.environ.get("PYTHONPATH"), environ["PYTHONPATH"])
-    return environ
-
-
-@contextmanager
-def gunicorn_server(appsec_enabled="true", remote_configuration_enabled="true", token=None):
-    cmd = ["gunicorn", "-w", "3", "-b", "0.0.0.0:8000", "tests.appsec.app:app"]
-    env = _build_env()
-    env["DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"] = "0.5"
-    env["DD_REMOTE_CONFIGURATION_ENABLED"] = remote_configuration_enabled
-    if token:
-        env["_DD_REMOTE_CONFIGURATION_ADDITIONAL_HEADERS"] = "X-Datadog-Test-Session-Token:%s," % (token,)
-    if appsec_enabled:
-        env["DD_APPSEC_ENABLED"] = appsec_enabled
-    env["DD_TRACE_AGENT_URL"] = os.environ.get("DD_TRACE_AGENT_URL", "")
-    server_process = subprocess.Popen(
-        cmd,
-        env=env,
-        stdout=sys.stdout,
-        stderr=sys.stderr,
-        close_fds=True,
-        preexec_fn=os.setsid,
-    )
-    try:
-        client = Client("http://0.0.0.0:8000")
-        try:
-            print("Waiting for server to start")
-            client.wait(max_tries=100, delay=0.1)
-            print("Server started")
-        except RetryError:
-            raise AssertionError(
-                "Server failed to start, see stdout and stderr logs"
-                "\n=== Captured STDOUT ===\n%s=== End of captured STDOUT ==="
-                "\n=== Captured STDERR ===\n%s=== End of captured STDERR ==="
-                % (server_process.stdout, server_process.stderr)
-            )
-        time.sleep(1)
-        parent = psutil.Process(server_process.pid)
-        children = parent.children(recursive=True)
-
-        yield server_process, client, children[1].pid
-        try:
-            client.get_ignored("/shutdown")
-        except Exception:
-            raise AssertionError(
-                "\n=== Captured STDOUT ===\n%s=== End of captured STDOUT ==="
-                "\n=== Captured STDERR ===\n%s=== End of captured STDERR ==="
-                % (server_process.stdout, server_process.stderr)
-            )
-    finally:
-        server_process.terminate()
-        server_process.wait()
+from tests.appsec.appsec_utils import gunicorn_server
 
 
 def _get_agent_client():
@@ -261,7 +194,7 @@ def _request_200(client, debug_mode=False, max_retries=7, sleep_time=1):
     previous = False
     for _ in range(max_retries):
         results = _multi_requests(client, debug_mode)
-        check = all(response.status_code == 200 and response.content == b"OK" for response in results)
+        check = all(response.status_code == 200 and response.content == b"OK_index" for response in results)
         if check:
             if previous:
                 return