Repository: falconry/falcon
Pull request #2043 (2022-03-15 14:58:15)

fix(request): prevent IndexError from broken Forwarded header
# Summary of Changes

Fix a wrongful assumption where `forwarded_scheme` and `forwarded_host` assume that if a `Forwarded` header is present, it is valid.
When a `Forwarded` header cannot be parsed properly (or is plain empty), an empty list is returned by `forwarded`.
This resulted in an `IndexError` in `forwarded_scheme` and `forwarded_host`.
This PR changes that to use the existing fallback logic instead.

While we could fall back on checking other headers (`X-Forwarded-For` and such) I think that if the `Forwarded` header is present but doesn't contain any relevant or correct entries, we can simply assume that there are none.
Please let me know if you have a different opinion.

# Related Issues

N/A

# Pull Request Checklist

This is just a reminder about the most common mistakes.  Please make sure that you tick all *appropriate* boxes.  But please read our [contribution guide](../CONTRIBUTING.md) at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing to do.

- [X] Applied changes to both WSGI and ASGI code paths and interfaces (where applicable).
- [X] Added **tests** for changed code.
- [X] Prefixed code comments with GitHub nick and an appropriate prefix.
- [X] Coding style is consistent with the rest of the framework.
- [X] Updated **documentation** for changed code.
    - [X] Added docstrings for any new classes, functions, or modules.
    - [X] Updated docstrings for any modifications to existing code.
    - [X] Updated both WSGI and ASGI docs (where applicable).
    - [X] Added references to new classes, functions, or modules to the relevant RST file under `docs/`.
    - [X] Updated all relevant supporting documentation files under `docs/`.
    - [X] A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
    - [X] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/usage/restructuredtext/directives.html?highlight=versionadded#directive-versionadded).
- [x] Changes (and possible deprecations) have [towncrier](https://towncrier.readthedocs.io/en/actual-freaking-docs/index.html) news fragments under `docs/_newsfragments/`, with the file name format `{issue_number}.{fragment_type}.rst`. (Run `towncrier --draft` to ensure it renders correctly.)

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) Report
> Merging [#2043](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) (fd69eef) into [master](https://codecov.io/gh/falconry/falcon/commit/395c773dc96b980422f999476d8d5483f75b8df5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) (395c773) will **decrease** coverage by `11.81%`.
> The diff coverage is `100.00%`.

```diff
@@             Coverage Diff              @@
##            master    #2043       +/-   ##
============================================
- Coverage   100.00%   88.18%   -11.82%     
============================================
  Files           63       54        -9     
  Lines         6707     5213     -1494     
  Branches      1238      794      -444     
============================================
- Hits          6707     4597     -2110     
- Misses           0      601      +601     
- Partials         0       15       +15     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) | Coverage Δ | |
|---|---|---|
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `100.00% <100.00%> (ø)` | |
| [falcon/asgi/\_request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvX3JlcXVlc3RfaGVscGVycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/\_\_init\_\_.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvX19pbml0X18ucHk=) | `27.27% <0.00%> (-72.73%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `38.05% <0.00%> (-61.95%)` | :arrow_down: |
| [falcon/responders.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3Jlc3BvbmRlcnMucHk=) | `64.70% <0.00%> (-35.30%)` | :arrow_down: |
| [falcon/testing/client.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3Rlc3RpbmcvY2xpZW50LnB5) | `67.01% <0.00%> (-32.99%)` | :arrow_down: |
| [falcon/app\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FwcF9oZWxwZXJzLnB5) | `68.13% <0.00%> (-31.87%)` | :arrow_down: |
| [falcon/media/validators/jsonschema.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL21lZGlhL3ZhbGlkYXRvcnMvanNvbnNjaGVtYS5weQ==) | `69.04% <0.00%> (-30.96%)` | :arrow_down: |
| [falcon/util/\_\_init\_\_.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3V0aWwvX19pbml0X18ucHk=) | `76.08% <0.00%> (-23.92%)` | :arrow_down: |
| [falcon/util/sync.py](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL3V0aWwvc3luYy5weQ==) | `76.74% <0.00%> (-23.26%)` | :arrow_down: |
| ... and [35 more](https://codecov.io/gh/falconry/falcon/pull/2043/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry). Last update [395c773...fd69eef](https://codecov.io/gh/falconry/falcon/pull/2043?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry).

TBoshoven: Sorry for taking a while. Merged `master` into this branch.

Patch:
diff --git a/docs/_newsfragments/2043.bugfix.rst b/docs/_newsfragments/2043.bugfix.rst
new file mode 100644
index 0000000..5f77808
--- /dev/null
+++ b/docs/_newsfragments/2043.bugfix.rst
@@ -0,0 +1,3 @@
+Request attributes :attr:`~falcon.Request.forwarded_scheme` and
+:attr:`~falcon.Request.forwarded_host` now no longer raise an ``IndexError``
+while processing an invalid or empty ``Forwarded`` header.
diff --git a/falcon/asgi/request.py b/falcon/asgi/request.py
index 8a12aee..db70522 100644
--- a/falcon/asgi/request.py
+++ b/falcon/asgi/request.py
@@ -617,8 +617,12 @@ class Request(request.Request):
         # try...catch that will usually result in a relatively expensive
         # raised exception.
         if b'forwarded' in self._asgi_headers:
-            first_hop = self.forwarded[0]
-            scheme = first_hop.scheme or self.scheme
+            forwarded = self.forwarded
+            if forwarded:
+                # Use first hop, fall back on own scheme
+                scheme = forwarded[0].scheme or self.scheme
+            else:
+                scheme = self.scheme
         else:
             # PERF(kgriffs): This call should normally succeed, so
             # just go for it without wasting time checking it
@@ -654,8 +658,12 @@ class Request(request.Request):
         # try...catch that will usually result in a relatively expensive
         # raised exception.
         if b'forwarded' in self._asgi_headers:
-            first_hop = self.forwarded[0]
-            host = first_hop.host or self.netloc
+            forwarded = self.forwarded
+            if forwarded:
+                # Use first hop, fall back on self
+                host = forwarded[0].host or self.netloc
+            else:
+                host = self.netloc
         else:
             # PERF(kgriffs): This call should normally succeed, assuming
             # that the caller is expecting a forwarded header, so
diff --git a/falcon/request.py b/falcon/request.py
index 529b033..088501c 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -752,8 +752,12 @@ class Request:
         # try...catch that will usually result in a relatively expensive
         # raised exception.
         if 'HTTP_FORWARDED' in self.env:
-            first_hop = self.forwarded[0]
-            scheme = first_hop.scheme or self.scheme
+            forwarded = self.forwarded
+            if forwarded:
+                # Use first hop, fall back on own scheme
+                scheme = forwarded[0].scheme or self.scheme
+            else:
+                scheme = self.scheme
         else:
             # PERF(kgriffs): This call should normally succeed, so
             # just go for it without wasting time checking it
@@ -845,8 +849,12 @@ class Request:
         # try...catch that will usually result in a relatively expensive
         # raised exception.
         if 'HTTP_FORWARDED' in self.env:
-            first_hop = self.forwarded[0]
-            host = first_hop.host or self.netloc
+            forwarded = self.forwarded
+            if forwarded:
+                # Use first hop, fall back on self
+                host = forwarded[0].host or self.netloc
+            else:
+                host = self.netloc
         else:
             # PERF(kgriffs): This call should normally succeed, assuming
             # that the caller is expecting a forwarded header, so


Test patch:
diff --git a/tests/test_request_forwarded.py b/tests/test_request_forwarded.py
index 025d124..66e4e40 100644
--- a/tests/test_request_forwarded.py
+++ b/tests/test_request_forwarded.py
@@ -90,11 +90,28 @@ def test_forwarded_host(asgi):
     assert req.forwarded[1].host == 'suchproxy01.suchtesting.com'
 
     assert req.forwarded_host == 'something.org'
+    assert req.forwarded_scheme == 'http'
     assert req.forwarded_uri != req.uri
     assert req.forwarded_uri == 'http://something.org/languages'
     assert req.forwarded_prefix == 'http://something.org'
 
 
+def test_forwarded_invalid(asgi):
+    req = create_req(
+        asgi,
+        host='suchproxy02.suchtesting.com',
+        path='/languages',
+        headers={'Forwarded': 'invalid'},
+    )
+
+    assert req.forwarded == []
+
+    assert req.forwarded_host == 'suchproxy02.suchtesting.com'
+    assert req.forwarded_scheme == 'http'
+    assert req.forwarded_uri == req.uri
+    assert req.forwarded_prefix == 'http://suchproxy02.suchtesting.com'
+
+
 def test_forwarded_multiple_params(asgi):
     req = create_req(
         asgi,