fix(wsgi): ensure HTTPS: on environ value does not get parsed as a he…

…ader (#6290)

Fixes #6284

We have a check in the wsgi helper `get_request_header` which will try
to extract any `environ` values starting with `HTTP` into headers, but
`from_wsgi_header` will require that the input starts with `HTTP_`. This
means if `environ` has `["HTTPS"] = "on"` then we would try to convert
it, get `None` back and end up with a header key of `None` with the
value `"on"`.

This updates the caller of `from_wsgi_header` to check for `HTTP_`
prefix, and test whether the resulting name is `None` or not.

The Django integration also uses `from_wsgi_header`, but it's usage was
already safe/checking the result for `None`, but we added an explicit
test anyways.

I also added additional type hinting to wsgi and Django to try and help
catch this issue at development time.

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

- [ ] Title is accurate.
- [ ] No unnecessary changes are introduced.
- [ ] Description motivates each change.
- [ ] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Testing strategy adequately addresses listed risk(s).
- [ ] Change is maintainable (easy to change, telemetry, documentation).
- [ ] Release note makes sense to a user of the library.
- [ ] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [ ] 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)

ddtrace/contrib/django/utils.py

@@ -2,6 +2,7 @@
 from typing import Any
 from typing import Dict
 from typing import List
+from typing import Mapping
 from typing import Text
 from typing import Union
 
@@ -285,10 +286,11 @@ def _extract_body(request):
 
 
 def _get_request_headers(request):
+    # type: (Any) -> Mapping[str, str]
     if DJANGO22:
-        request_headers = request.headers
+        request_headers = request.headers  # type: Mapping[str, str]
     else:
-        request_headers = {}
+        request_headers = {}  # type: Mapping[str, str]
         for header, value in request.META.items():
             name = from_wsgi_header(header)
             if name:

ddtrace/contrib/wsgi/wsgi.py

@@ -15,6 +15,7 @@
     from typing import Callable
     from typing import Dict
     from typing import Iterable
+    from typing import Mapping
     from typing import Optional
 
     from ddtrace import Pin
@@ -272,13 +273,16 @@ def construct_url(environ):
 
 
 def get_request_headers(environ):
+    # type: (Mapping[str, str]) -> Mapping[str, str]
     """
     Manually grab the request headers from the environ dictionary.
     """
-    request_headers = {}
+    request_headers = {}  # type: Mapping[str, str]
     for key in environ.keys():
-        if key.startswith("HTTP"):
-            request_headers[from_wsgi_header(key)] = environ[key]
+        if key.startswith("HTTP_"):
+            name = from_wsgi_header(key)
+            if name:
+                request_headers[name] = environ[key]
     return request_headers
 
 

releasenotes/notes/fix-wsgi-header-extract-https-on-de3d657da3843d1f.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    wsgi: This fix resolves an issues when trying to parse the ``environ`` property ``HTTPS`` as an HTTP header.

tests/contrib/django/test_django_utils.py

@@ -0,0 +1,25 @@
+from django.test.client import RequestFactory
+import pytest
+
+from ddtrace.contrib.django.utils import DJANGO22
+from ddtrace.contrib.django.utils import _get_request_headers
+
+
+@pytest.mark.skipif(DJANGO22, reason="We only parse environ/headers on Django < 2.2.0")
+@pytest.mark.parametrize(
+    "meta,expected",
+    [
+        ({}, {}),
+        # This is a regression for #6284
+        # DEV: We were checking for `HTTP` prefix for headers instead of `HTTP_` which is required
+        ({"HTTPS": "on"}, {}),
+        ({"HTTP_HEADER": "value"}, {"Header": "value"}),
+    ],
+)
+def test_get_request_headers(meta, expected):
+    factory = RequestFactory()
+    request = factory.get("/")
+    request.META = meta
+
+    headers = _get_request_headers(request)
+    assert headers == expected

tests/contrib/wsgi/test_wsgi.py

@@ -333,6 +333,32 @@ def start_response(status, headers, exc_info=None):
     assert not hasattr(resp, "__len__"), "Iterables should not define __len__ attribute"
 
 
+@pytest.mark.parametrize(
+    "extra,expected",
+    [
+        ({}, {}),
+        # This is a regression for #6284
+        # DEV: We were checking for `HTTP` prefix for headers instead of `HTTP_` which is required
+        ({"HTTPS": "on"}, {}),
+        # Normal header
+        ({"HTTP_HEADER": "value"}, {"Header": "value"}),
+    ],
+)
+def test_get_request_headers(extra, expected):
+    # Normal environ stuff
+    environ = {
+        "PATH_INFO": "/",
+        "wsgi.url_scheme": "http",
+        "SERVER_NAME": "localhost",
+        "SERVER_PORT": "80",
+        "REQUEST_METHOD": "GET",
+    }
+    environ.update(extra)
+
+    headers = wsgi.get_request_headers(environ)
+    assert headers == expected
+
+
 @pytest.mark.snapshot()
 @pytest.mark.parametrize("schema_version", [None, "v0", "v1"])
 @pytest.mark.parametrize("service_name", [None, "mysvc"])