Repository: falconry/falcon
Pull request #1879 (2021-03-07 17:43:39)

fix(uri): change encode to escape percent
# Summary of Changes

Changed the default behavior of `uri.encode` and `uri.envode_value` to escape all escapable characters even if they look like they've already been escaped. For example, '%26' would be escaped as '%2526' to conform with behavior of `urllib.parse.quote`. This ensures that decoding an encoded value returns the same original value. Previously, encoding '%26' returned '%26' which, decoded, become '&'.

A `check_is_escaped` flag has been added to the encoder method to allow the ability to retain the original behavior of ignoring previously escaped values (currently being used in `location`, `content_location`, and `append_link()`).

# Related Issues

Address bug in #1872 while retaining fix for #68.

# 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.)

If you have *any* questions to *any* of the points above, just **submit and ask**! This checklist is here to *help* you, not to deter you from contributing!

*PR template inspired by the attrs project.*


codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1879?src=pr&el=h1) Report
> Merging [#1879](https://codecov.io/gh/falconry/falcon/pull/1879?src=pr&el=desc) (8cc64b5) into [master](https://codecov.io/gh/falconry/falcon/commit/a5e72b287efb2b3da632cf6547ed3f07d8ec5380?el=desc) (a5e72b2) will **decrease** coverage by `63.99%`.
> The diff coverage is `85.71%`.

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

```diff
@@             Coverage Diff              @@
##            master    #1879       +/-   ##
============================================
- Coverage   100.00%   36.00%   -64.00%     
============================================
  Files           55       62        +7     
  Lines         5218     6585     +1367     
  Branches       842     1064      +222     
============================================
- Hits          5218     2371     -2847     
- Misses           0     4161     +4161     
- Partials         0       53       +53     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1879?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/util/uri.py](https://codecov.io/gh/falconry/falcon/pull/1879/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvdXJpLnB5) | `28.96% <50.00%> (-71.04%)` | :arrow_down: |
| [falcon/response.py](https://codecov.io/gh/falconry/falcon/pull/1879/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlLnB5) | `36.55% <100.00%> (ø)` | |
| [falcon/util/reader.py](https://codecov.io/gh/falconry/falcon/pull/1879/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/1879/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/1879/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/1879/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/1879/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/1879/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/1879/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `22.96% <0.00%> (-77.04%)` | :arrow_down: |
| [falcon/forwarded.py](https://codecov.io/gh/falconry/falcon/pull/1879/diff?src=pr&el=tree#diff-ZmFsY29uL2ZvcndhcmRlZC5weQ==) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
| ... and [49 more](https://codecov.io/gh/falconry/falcon/pull/1879/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1879?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/1879?src=pr&el=footer). Last update [a5e72b2...8cc64b5](https://codecov.io/gh/falconry/falcon/pull/1879?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

kgriffs: @MinesJA Thanks again for your help on this. We would like to include this fix in 3.0.0rc1 if possible. Do you have time to address the feedback items today? If not, no problem, just let me know and I can put up some patches on your feature branch.
MinesJA: > n for your help on this. We would like to include this fix in 3.0.0rc1 if possible. Do you have time to address the feedback it

For sure! I'll push something up in the next couple hours if that's ok.
MinesJA: Ready for review @kgriffs @vytas7 

Patch:
diff --git a/docs/_newsfragments/1872.bugfix.rst b/docs/_newsfragments/1872.bugfix.rst
new file mode 100644
index 0000000..6285abf
--- /dev/null
+++ b/docs/_newsfragments/1872.bugfix.rst
@@ -0,0 +1,7 @@
+:attr:`falcon.Uri.encode` and :attr:`falcon.Uri.encode_value` now escape all
+percent characters by default even if it appears they have already been escaped. 
+The :attr:`falcon.Uri.encode_check_escaped` and :attr:`falcon.Uri.encode_value_check_escaped` 
+methods have been added to give the option of retaining the previous behavior where needed. 
+These new methods have been applied to the :attr:`falcon.Response.location`, 
+:attr:`falcon.Response.content_location`, :attr:`falcon.Response.append_link` 
+attrs and methods to retain previous behavior.
\ No newline at end of file
diff --git a/docs/api/util.rst b/docs/api/util.rst
index c9c7a5b..53216ae 100644
--- a/docs/api/util.rst
+++ b/docs/api/util.rst
@@ -9,7 +9,8 @@ URI
 ---
 
 .. automodule:: falcon.uri
-    :members: encode, encode_value, decode, parse_host,
+    :members: encode, encode_value, encode_check_escaped,
+        encode_value_check_escaped, decode, parse_host,
         parse_query_string, unquote_string
 
 Date and Time
diff --git a/falcon/response.py b/falcon/response.py
index e5985ca..f3b8706 100644
--- a/falcon/response.py
+++ b/falcon/response.py
@@ -31,8 +31,8 @@ from falcon.util import http_cookies
 from falcon.util import structures
 from falcon.util import TimezoneGMT
 from falcon.util.deprecation import deprecated
-from falcon.util.uri import encode as uri_encode
-from falcon.util.uri import encode_value as uri_encode_value
+from falcon.util.uri import encode_check_escaped as uri_encode
+from falcon.util.uri import encode_value_check_escaped as uri_encode_value
 
 
 GMT_TIMEZONE = TimezoneGMT()
diff --git a/falcon/uri.py b/falcon/uri.py
index d351957..e8e813e 100644
--- a/falcon/uri.py
+++ b/falcon/uri.py
@@ -15,7 +15,9 @@ in the `falcon` module, and so must be explicitly imported::
 
 from falcon.util.uri import decode
 from falcon.util.uri import encode
+from falcon.util.uri import encode_check_escaped
 from falcon.util.uri import encode_value
+from falcon.util.uri import encode_value_check_escaped
 from falcon.util.uri import parse_host
 from falcon.util.uri import parse_query_string
 from falcon.util.uri import unquote_string
diff --git a/falcon/util/uri.py b/falcon/util/uri.py
index 3e55624..0ecbc8f 100644
--- a/falcon/util/uri.py
+++ b/falcon/util/uri.py
@@ -68,7 +68,7 @@ def _create_char_encoder(allowed_chars):
     return lookup.__getitem__
 
 
-def _create_str_encoder(is_value):
+def _create_str_encoder(is_value, check_is_escaped=False):
 
     allowed_chars = _UNRESERVED if is_value else _ALL_ALLOWED
     allowed_chars_plus_percent = allowed_chars + '%'
@@ -79,9 +79,14 @@ def _create_str_encoder(is_value):
         if not uri.rstrip(allowed_chars):
             return uri
 
-        if not uri.rstrip(allowed_chars_plus_percent):
+        if check_is_escaped and not uri.rstrip(allowed_chars_plus_percent):
             # NOTE(kgriffs): There's a good chance the string has already
             # been escaped. Do one more check to increase our certainty.
+            # NOTE(minesja): Per issue #1872, there's only certain situations
+            # in which we should check again (ex. location, content_location,
+            # append_link).In all other cases we should allow characters that
+            # could appear escaped to still be encoded (ex. '%' would be encoded
+            # as '%25).
             tokens = uri.split('%')
             for token in tokens[1:]:
                 hex_octet = token[:2]
@@ -139,7 +144,6 @@ Returns:
 
 """
 
-
 encode_value = _create_str_encoder(True)
 encode_value.name = 'encode_value'
 encode_value.__doc__ = """Encodes a value string according to RFC 3986.
@@ -168,6 +172,59 @@ Returns:
 
 """
 
+encode_check_escaped = _create_str_encoder(False, True)
+encode_check_escaped.name = 'encode_check_escaped'
+encode_check_escaped.__doc__ = """Encodes a full or relative URI according to RFC 3986.
+
+RFC 3986 defines a set of "unreserved" characters as well as a
+set of "reserved" characters used as delimiters. This function escapes
+all other "disallowed" characters by percent-encoding them unless they
+appear to have been previously encoded. For example, '%26' will not be
+encoded again as it follows the format of an encoded value.
+
+Note:
+    This utility is faster in the average case than the similar
+    `quote` function found in ``urlib``. It also strives to be easier
+    to use by assuming a sensible default of allowed characters.
+
+Args:
+    uri (str): URI or part of a URI to encode.
+
+Returns:
+    str: An escaped version of `uri`, where all disallowed characters
+    have been percent-encoded.
+
+"""
+
+encode_value_check_escaped = _create_str_encoder(True, True)
+encode_value_check_escaped.name = 'encode_value_check_escaped'
+encode_value_check_escaped.__doc__ = """Encodes a value string according to RFC 3986.
+
+RFC 3986 defines a set of "unreserved" characters as well as a
+set of "reserved" characters used as delimiters. Disallowed characters
+are percent-encoded in a way that models ``urllib.parse.quote(safe="~")``
+unless they appear to have been previously encoded. For example, '%26'
+will not be encoded again as it follows the format of an encoded value.
+
+All reserved characters are lumped together into a single set of
+"delimiters", and everything in that set is escaped.
+
+Note:
+    This utility is faster in the average case than the similar
+    `quote` function found in ``urlib``. It also strives to be easier
+    to use by assuming a sensible default of allowed characters.
+
+Args:
+    uri (str): URI fragment to encode. It is assumed not to cross delimiter
+        boundaries, and so any reserved URI delimiter characters
+        included in it will be percent-encoded.
+
+Returns:
+    str: An escaped version of `uri`, where all disallowed characters
+    have been percent-encoded.
+
+"""
+
 
 def _join_tokens_bytearray(tokens):
     decoded_uri = bytearray(tokens[0])
@@ -492,6 +549,8 @@ __all__ = [
     'decode',
     'encode',
     'encode_value',
+    'encode_check_escaped',
+    'encode_value_check_escaped',
     'parse_host',
     'parse_query_string',
     'unquote_string',


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 53b1728..5871f80 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -239,33 +239,37 @@ class TestFalconUtils:
                     '?limit=3&e%C3%A7ho=true')
         assert uri.encode(url) == expected
 
+        # NOTE(minesja): Addresses #1872
+        assert uri.encode('%26') == '%2526'
+        assert uri.decode(uri.encode('%26')) == '%26'
+
     def test_uri_encode_double(self):
         url = 'http://example.com/v1/fiz bit/messages'
         expected = 'http://example.com/v1/fiz%20bit/messages'
-        assert uri.encode(uri.encode(url)) == expected
+        assert uri.encode_check_escaped(uri.encode_check_escaped(url)) == expected
 
         url = 'http://example.com/v1/fizbit/messages?limit=3&e\u00e7ho=true'
         expected = ('http://example.com/v1/fizbit/messages'
                     '?limit=3&e%C3%A7ho=true')
-        assert uri.encode(uri.encode(url)) == expected
+        assert uri.encode_check_escaped(uri.encode_check_escaped(url)) == expected
 
         url = 'http://example.com/v1/fiz%bit/mess%ages/%'
         expected = 'http://example.com/v1/fiz%25bit/mess%25ages/%25'
-        assert uri.encode(uri.encode(url)) == expected
+        assert uri.encode_check_escaped(uri.encode_check_escaped(url)) == expected
 
         url = 'http://example.com/%%'
         expected = 'http://example.com/%25%25'
-        assert uri.encode(uri.encode(url)) == expected
+        assert uri.encode_check_escaped(uri.encode_check_escaped(url)) == expected
 
         # NOTE(kgriffs): Specific example cited in GH issue
         url = 'http://something?redirect_uri=http%3A%2F%2Fsite'
-        assert uri.encode(url) == url
+        assert uri.encode_check_escaped(url) == url
 
         hex_digits = 'abcdefABCDEF0123456789'
         for c1 in hex_digits:
             for c2 in hex_digits:
                 url = 'http://example.com/%' + c1 + c2
-                encoded = uri.encode(uri.encode(url))
+                encoded = uri.encode_check_escaped(uri.encode_check_escaped(url))
                 assert encoded == url
 
     def test_uri_encode_value(self):
@@ -277,6 +281,10 @@ class TestFalconUtils:
         assert uri.encode_value('ab/cd') == 'ab%2Fcd'
         assert uri.encode_value('ab+cd=42,9') == 'ab%2Bcd%3D42%2C9'
 
+        # NOTE(minesja): Addresses #1872
+        assert uri.encode_value('%26') == '%2526'
+        assert uri.decode(uri.encode_value('%26')) == '%26'
+
     def test_uri_decode(self, decode_approach):
         assert uri.decode('abcd') == 'abcd'
         assert uri.decode('ab%20cd') == 'ab cd'