fix(asm): improve cookies management for fastapi by using starlette parser (#7873)

Improve cookie management for fastapi and ASM by using starlette parser.
This ensure that data sent to the waf before the request is processed by
the customer code gets exactly the same cookies data as customer code.
Previously, computation was done on the middleware level with Python
http.cookies

Also, minor improvement of asm fast api test for cookies to handle some
corner case. It will also be tested in system tests.

FastAPI ASM support is still a private wip feature, and this is a fix
for #7220

## 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.

Co-authored-by: Alberto Vara <[email protected]>

Author: christophe-papazian

ddtrace/contrib/asgi/middleware.py

@@ -1,4 +1,3 @@
-from http import cookies
 import sys
 from typing import Any
 from typing import Mapping
@@ -159,7 +158,6 @@ async def __call__(self, scope, receive, send):
                 span.set_tag(ANALYTICS_SAMPLE_RATE_KEY, sample_rate)
 
             host_header = None
-            request_cookies = None
             for key, value in scope["headers"]:
                 if key == b"host":
                     try:
@@ -168,10 +166,7 @@ async def __call__(self, scope, receive, send):
                         log.warning(
                             "failed to decode host header, host from http headers will not be considered", exc_info=True
                         )
-                elif key == b"cookie":
-                    c = cookies.SimpleCookie(bytes_to_str(value))
-                    request_cookies = {k: v.value for k, v in c.items()}
-
+                    break
             method = scope.get("method")
             server = scope.get("server")
             scheme = scope.get("scheme", "http")
@@ -212,7 +207,6 @@ async def __call__(self, scope, receive, send):
                 query=query_string,
                 request_headers=headers,
                 raw_uri=url,
-                request_cookies=request_cookies,
                 parsed_query=parsed_query,
                 request_body=body,
                 peer_ip=peer_ip,

ddtrace/contrib/starlette/patch.py

@@ -4,6 +4,7 @@
 from typing import Optional  # noqa:F401
 
 import starlette
+from starlette import requests as starlette_requests
 from starlette.middleware import Middleware
 
 from ddtrace import config
@@ -131,8 +132,18 @@ def traced_handler(wrapped, instance, args, kwargs):
             request_spans,
             resource_paths,
         )
+    request_cookies = ""
+    for name, value in scope.get("headers"):
+        if name == b"cookie":
+            request_cookies = value.decode("utf-8", errors="ignore")
+            break
     if request_spans:
-        trace_utils.set_http_meta(request_spans[0], "starlette", request_path_params=scope.get("path_params"))
+        trace_utils.set_http_meta(
+            request_spans[0],
+            "starlette",
+            request_path_params=scope.get("path_params"),
+            request_cookies=starlette_requests.cookie_parser(request_cookies),
+        )
     core.dispatch("asgi.start_request", ("starlette",))
     if core.get_item(HTTP_REQUEST_BLOCKED):
         raise trace_utils.InterruptException("starlette")

tests/contrib/fastapi/test_fastapi_appsec.py

@@ -1,5 +1,6 @@
 import json
 
+import fastapi
 from fastapi import Request
 from fastapi.responses import PlainTextResponse
 from fastapi.testclient import TestClient
@@ -298,29 +299,32 @@ def test_route():
 
 
 def test_request_suspicious_request_block_match_cookies(app, client, tracer, test_spans):
-    @app.get("/")
-    def test_route():
-        return PlainTextResponse("OK")
+    @app.get("/test_cookies", response_class=PlainTextResponse)
+    def test_route(response: fastapi.Response):
+        response.status_code = fastapi.status.HTTP_201_CREATED
+        return "OK Cookie"
 
     # value jdfoSDGFkivRG_234 must be blocked
     with override_global_config(dict(_asm_enabled=True)), override_env(dict(DD_APPSEC_RULES=RULES_SRB)):
         _aux_appsec_prepare_tracer(tracer)
-        resp = client.get("/", cookies={"keyname": "jdfoSDGFkivRG_234"})
-        assert resp.status_code == 403
+        resp = client.get("/test_cookies", cookies={"keyname": "jdfoSDGFkivRG_234 45"})
         assert get_response_body(resp) == constants.BLOCKED_RESPONSE_JSON
+        assert resp.status_code == 403
         root_span = get_root_span(test_spans)
         loaded = json.loads(root_span.get_tag(APPSEC.JSON))
         assert [t["rule"]["id"] for t in loaded["triggers"]] == ["tst-037-008"]
     # other value must not be blocked
     with override_global_config(dict(_asm_enabled=True)), override_env(dict(DD_APPSEC_RULES=RULES_SRB)):
         _aux_appsec_prepare_tracer(tracer)
-        resp = client.get("/", cookies={"keyname": "jdfoSDGFHappykivRG_234"})
-        assert resp.status_code == 200
+        resp = client.get("/test_cookies", cookies={"keyname": "jdfoSDGFHappykivRG_234"})
+        assert resp.status_code == 201
+        assert get_response_body(resp) == "OK Cookie"
     # appsec disabled must not block
     with override_global_config(dict(_asm_enabled=False)), override_env(dict(DD_APPSEC_RULES=RULES_SRB)):
         _aux_appsec_prepare_tracer(tracer, asm_enabled=False)
-        resp = client.get("/", cookies={"keyname": "jdfoSDGFkivRG_234"})
-        assert resp.status_code == 200
+        resp = client.get("/test_cookies", cookies={"keyname": "jdfoSDGFkivRG_234"})
+        assert resp.status_code == 201
+        assert get_response_body(resp) == "OK Cookie"
 
 
 def test_request_suspicious_request_block_match_path_params(app, client, tracer, test_spans):