Repository: falconry/falcon

Issue:
falcon.to_query_str does not properly encode query parameters
# Version details

* Python version tested: 3.7.9
* Falcon version tested: 2.0.0

# Observed Behavior

`falcon.testing.TestClient`'s `simulate_*` methods do not correctly encode query parameters when using the `params` keyword argument.

```python
import urllib.parse

import falcon
import falcon.testing
import falcon.util


class SomeHandler:
    def on_get(self, request, response):
        print("parsed params       ", request.params)


app = falcon.API()
app.add_route("/", SomeHandler())

test_client = falcon.testing.TestClient(app)


# Try a params dict with the TestClient. Note that it does not encode any special characters, and this results in a
# query string that does not decode back to the same parameters.
#
# This is because falcon.testing uses falcon.util.to_query_str, which does not apply any encoding to the supplied query
# params dict.
params = {"foo": "bar", "baz": "boz&something"}
print("original params dict", params)
print("to_query_str        ", falcon.util.to_query_str(params))
response = test_client.simulate_get("/", params=params)
print(response.status_code)
# => original params dict {'foo': 'bar', 'baz': 'boz&something'}
# => to_query_str         ?foo=bar&baz=boz&something
# => parsed params        {'foo': 'bar', 'baz': 'boz', 'something': ''}
# => 200

# Now try manually encoding the params dict ourselves with urllib. Note that the query string is then correctly
# interpreted by falcon.
params = {"foo": "bar", "baz": "boz&something"}
encoded_params = urllib.parse.urlencode(params)
print("encoded params      ", encoded_params)
response = test_client.simulate_get("/", query_string=encoded_params)
print(response.status_code)
# => encoded params       foo=bar&baz=boz%26something
# => parsed params        {'foo': 'bar', 'baz': 'boz&something'}
# => 200
```

# Expected Behavior

I expect a WSGI app testing library that supports a way to specify query parameters as a dict to correctly encode my parameters so that the application interprets them the same way (although it is reasonable to require both sides to agree on how to handle lists as CSV vs repeating a query parameter)

I think that merely documenting this shortcoming is insufficient because the argument specifically exists to allow me to pass in query parameters as a dict.


# Root Cause

This is caused by `falcon.util.misc.to_query_str` not applying any URL encoding to the query parameters as it constructs the query string. `to_query_str` looks like it is currently only used by `falcon.testing.simulate_request`.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1873?src=pr&el=h1) Report
> Merging [#1873](https://codecov.io/gh/falconry/falcon/pull/1873?src=pr&el=desc) (dd485fd) into [master](https://codecov.io/gh/falconry/falcon/commit/c7df22a7b72a84cfe6831e4132f6f988a6332706?el=desc) (c7df22a) will **decrease** coverage by `64.13%`.
> The diff coverage is `16.66%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1873/graphs/tree.svg?width=650&height=150&src=pr&token=zOtkfNQ3h6)](https://codecov.io/gh/falconry/falcon/pull/1873?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##            master    #1873       +/-   ##
============================================
- Coverage   100.00%   35.86%   -64.14%     
============================================
  Files           62       62               
  Lines         6554     6555        +1     
  Branches      1053     1053               
============================================
- Hits          6554     2351     -4203     
- Misses           0     4151     +4151     
- Partials         0       53       +53     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1873?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/util/misc.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvbWlzYy5weQ==) | `37.41% <16.66%> (-62.59%)` | :arrow_down: |
| [falcon/util/reader.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvcmVhZGVyLnB5) | `12.62% <0.00%> (-87.38%)` | :arrow_down: |
| [falcon/asgi/stream.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RyZWFtLnB5) | `13.58% <0.00%> (-86.42%)` | :arrow_down: |
| [falcon/asgi/structures.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RydWN0dXJlcy5weQ==) | `14.58% <0.00%> (-85.42%)` | :arrow_down: |
| [falcon/asgi/reader.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVhZGVyLnB5) | `16.41% <0.00%> (-83.59%)` | :arrow_down: |
| [falcon/middleware.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL21pZGRsZXdhcmUucHk=) | `18.36% <0.00%> (-81.64%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `19.63% <0.00%> (-80.37%)` | :arrow_down: |
| [falcon/asgi/app.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `23.16% <0.00%> (-76.84%)` | :arrow_down: |
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `23.44% <0.00%> (-76.56%)` | :arrow_down: |
| [falcon/asgi/request.py](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVxdWVzdC5weQ==) | `23.55% <0.00%> (-76.45%)` | :arrow_down: |
| ... and [43 more](https://codecov.io/gh/falconry/falcon/pull/1873/diff?src=pr&el=tree-more) | |

------

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

vytas7: Indeed, it's awkward in Python. It's one of those few cases where functional languages like Haskell would shine like anything :slightly_smiling_face: 

Patch:
diff --git a/docs/_newsfragments/1871.bugfix.rst b/docs/_newsfragments/1871.bugfix.rst
new file mode 100644
index 0000000..72a284c
--- /dev/null
+++ b/docs/_newsfragments/1871.bugfix.rst
@@ -0,0 +1,4 @@
+The :func:`~falcon.to_query_str` method now correctly encodes parameter keys
+and values. As a result, the `params` parameter in
+:func:`~falcon.testing.simulate_request` will now correctly pass values
+containing special characters (such as ``'&'``) to the application.
diff --git a/falcon/util/misc.py b/falcon/util/misc.py
index 89f3e4a..bf6e112 100644
--- a/falcon/util/misc.py
+++ b/falcon/util/misc.py
@@ -34,6 +34,7 @@ import unicodedata
 
 from falcon import status_codes
 from falcon.constants import PYPY
+from falcon.uri import encode_value
 # NOTE(vytas): Hoist `deprecated` here since it is documented as part of the
 # public Falcon interface.
 from .deprecation import deprecated
@@ -230,7 +231,7 @@ def to_query_str(params, comma_delimited_lists=True, prefix=True):
             v = 'false'
         elif isinstance(v, list):
             if comma_delimited_lists:
-                v = ','.join(map(str, v))
+                v = ','.join(map(encode_value, map(str, v)))
             else:
                 for list_value in v:
                     if list_value is True:
@@ -238,15 +239,15 @@ def to_query_str(params, comma_delimited_lists=True, prefix=True):
                     elif list_value is False:
                         list_value = 'false'
                     else:
-                        list_value = str(list_value)
+                        list_value = encode_value(str(list_value))
 
-                    query_str += k + '=' + list_value + '&'
+                    query_str += encode_value(k) + '=' + list_value + '&'
 
                 continue
         else:
-            v = str(v)
+            v = encode_value(str(v))
 
-        query_str += k + '=' + v + '&'
+        query_str += encode_value(k) + '=' + v + '&'
 
     return query_str[:-1]
 


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 3eccb34..53b1728 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -211,6 +211,21 @@ class TestFalconUtils:
 
         assert expected == garbage_out
 
+    @pytest.mark.parametrize('csv', [True, False])
+    @pytest.mark.parametrize('params', [
+        {'a & b': 'a and b', 'b and c': 'b & c'},
+        {'apples and oranges': '🍏 & 🍊'},
+        {'garbage': ['&', '&+&', 'a=1&b=2', 'c=4&'], 'one': '1'},
+        {'&': '&amp;', '™': '&trade;', '&&&': ['&amp;', '&amp;', '&amp;']},
+
+        # NOTE(vytas): Would fail because of https://github.com/falconry/falcon/issues/1872
+        # {'&': '%26', '&&': '%26', '&&&': ['%26', '%2', '%']},
+    ])
+    def test_to_query_str_encoding(self, params, csv):
+        query_str = falcon.to_query_str(params, comma_delimited_lists=csv, prefix=False)
+
+        assert uri.parse_query_string(query_str, csv=csv) == params
+
     def test_uri_encode(self):
         url = 'http://example.com/v1/fizbit/messages?limit=3&echo=true'
         assert uri.encode(url) == url