falcon.uri.encode_value does not encode the percent character

[vytas7]

falcon.uri.encode_value says:

An escaped version of uri, where all disallowed characters have been percent-encoded.

However, it seems that the percent character itself is not encoded...

Cf

>>> from urllib.parse import quote
>>> from falcon import uri
>>> quote('%26')
'%2526'
>>> uri.encode('%26')
'%26'
>>> uri.encode_value('%26')
'%26'
>>> uri.decode(uri.encode_value('%26'))
'&'
>>> uri.decode(quote('%26'))
'%26'

[vytas7]

Found this when trying to fix #1871...

[MinesJA]

I'd be interested in taking this.

[vytas7]

That's awesome @MinesJA !
(If you're quick, this might still make it into 3.0.)

[MinesJA]

Will try to be quick then!

[MinesJA]

So I think I found the cause of the bug, but had some questions on the functionality we're looking for. In this snippet, we check to see if a string has already been encoded, so if we encounter a string like 'abc%26def' we assume the '%26' was a result of the string already being encoded:

  tokens = uri.split('%')
  for token in tokens[1:]:
  hex_octet = token[:2]

  if not len(hex_octet) == 2:
      break

  if not (hex_octet[0] in _HEX_DIGITS and
              hex_octet[1] in _HEX_DIGITS):
      break
  else:
      # NOTE(kgriffs): All percent-encoded sequences were
      # valid, so assume that the string has already been
      # encoded.
      return uri

But it looks like 'quote' doesn't do that. I did notice this comment and was wondering why it was that we think there's a good chance the string has already been escaped by this point?

# NOTE(kgriffs): There's a good chance the string has already
# been escaped. Do one more check to increase our certainty.

[vytas7]

Heh, we need to check with @kgriffs then.

However, personally, I think this is a bug regardless of the good intentions mentioned above.
There might be cases where one wants to encode the whole URI as a parameter, and pass it along with other query parameters. (see, for instance this exotic use case: #1856).

Not encoding already escaped characters could then mangle that URI in the same way as, for instance, to_query_str was doing in #1871.

@kgriffs thoughts?

[kgriffs]

Here is the original issue: #68

Admittedly my original attempt at solution was implemented at the wrong layer; encoder() itself shouldn't short-circuit the encoding, at least not by default.

That being said, we should probably minimize breaking changes by retaining the current behavior for these Response properties/methods:

location
content_location
append_link()

[MinesJA]

Ok cool, do you think the best way to handle this would be to split the method to retain current behavior for those properties/methods but expose general encode / encode_value methods that don't perform that check? I can submit a pr that demonstrates that if it's easier to continue the discussion on an actual proposal.

[vytas7]

@MinesJA Alternatively, we could probably add a new parameter to these methods controlling this behaviour (to detect and ignore existing escapes). The above mentioned properties could then use the new parameter to retain the backwards compatible behaviour.

[MinesJA]

Ahh, that's much simpler. Ok, let me try to put something together for review.

[MinesJA]

@kgriffs @vytas7 Submitted a pr for this. Let me know what you think.