Skip to content

Allow unescaped equal signs in query parameter values #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 27, 2019
Merged

Allow unescaped equal signs in query parameter values #39

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6df3936
add failing test mentioned in #11, comment out invalid tests mentione…
mahmoud Sep 6, 2017
83d5cb2
split encoding/decoding for URL query parameter keys and values. the …
mahmoud Sep 6, 2017
a7283b6
add test to check that equals signs are not unnecessarily escaped in …
mahmoud Sep 6, 2017
005e04d
Merge branch 'master' into query_value_encoding
mahmoud Mar 26, 2019
ff24885
update tests to reflect new unescaped-equal-signs-in-query-param-valu…
mahmoud Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 43 additions & 19 deletions hyperlink/_url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,10 @@ def __nonzero__(self):
_SCHEMELESS_PATH_DELIMS = _ALL_DELIMS - _SCHEMELESS_PATH_SAFE
_FRAGMENT_SAFE = _UNRESERVED_CHARS | _PATH_SAFE | set(u'/?')
_FRAGMENT_DELIMS = _ALL_DELIMS - _FRAGMENT_SAFE
_QUERY_SAFE = _UNRESERVED_CHARS | _FRAGMENT_SAFE - set(u'&=+')
_QUERY_DELIMS = _ALL_DELIMS - _QUERY_SAFE
_QUERY_VALUE_SAFE = _UNRESERVED_CHARS | _FRAGMENT_SAFE - set(u'&+')
_QUERY_VALUE_DELIMS = _ALL_DELIMS - _QUERY_VALUE_SAFE
_QUERY_KEY_SAFE = _UNRESERVED_CHARS | _QUERY_VALUE_SAFE - set(u'=')
_QUERY_KEY_DELIMS = _ALL_DELIMS - _QUERY_KEY_SAFE


def _make_decode_map(delims, allow_percent=False):
Expand Down Expand Up @@ -191,8 +193,10 @@ def _make_quote_map(safe_chars):
_PATH_PART_QUOTE_MAP = _make_quote_map(_PATH_SAFE)
_SCHEMELESS_PATH_PART_QUOTE_MAP = _make_quote_map(_SCHEMELESS_PATH_SAFE)
_PATH_DECODE_MAP = _make_decode_map(_PATH_DELIMS)
_QUERY_PART_QUOTE_MAP = _make_quote_map(_QUERY_SAFE)
_QUERY_DECODE_MAP = _make_decode_map(_QUERY_DELIMS)
_QUERY_KEY_QUOTE_MAP = _make_quote_map(_QUERY_KEY_SAFE)
_QUERY_KEY_DECODE_MAP = _make_decode_map(_QUERY_KEY_DELIMS)
_QUERY_VALUE_QUOTE_MAP = _make_quote_map(_QUERY_VALUE_SAFE)
_QUERY_VALUE_DECODE_MAP = _make_decode_map(_QUERY_VALUE_DELIMS)
_FRAGMENT_QUOTE_MAP = _make_quote_map(_FRAGMENT_SAFE)
_FRAGMENT_DECODE_MAP = _make_decode_map(_FRAGMENT_DELIMS)
_UNRESERVED_DECODE_MAP = dict([(k, v) for k, v in _HEX_CHAR_MAP.items()
Expand Down Expand Up @@ -263,17 +267,28 @@ def _encode_path_parts(text_parts, rooted=False, has_scheme=True,
return tuple(encoded_parts)


def _encode_query_part(text, maximal=True):
def _encode_query_key(text, maximal=True):
"""
Percent-encode a single query string key or value.
"""
if maximal:
bytestr = normalize('NFC', text).encode('utf8')
return u''.join([_QUERY_PART_QUOTE_MAP[b] for b in bytestr])
return u''.join([_QUERY_PART_QUOTE_MAP[t] if t in _QUERY_DELIMS else t
return u''.join([_QUERY_KEY_QUOTE_MAP[b] for b in bytestr])
return u''.join([_QUERY_KEY_QUOTE_MAP[t] if t in _QUERY_KEY_DELIMS else t
for t in text])


def _encode_query_value(text, maximal=True):
"""
Percent-encode a single query string key or value.
"""
if maximal:
bytestr = normalize('NFC', text).encode('utf8')
return u''.join([_QUERY_VALUE_QUOTE_MAP[b] for b in bytestr])
return u''.join([_QUERY_VALUE_QUOTE_MAP[t]
if t in _QUERY_VALUE_DELIMS else t for t in text])


def _encode_fragment_part(text, maximal=True):
"""Quote the fragment part of the URL. Fragments don't have
subdelimiters, so the whole URL fragment can be passed.
Expand Down Expand Up @@ -455,9 +470,14 @@ def _decode_path_part(text, normalize_case=False):
_decode_map=_PATH_DECODE_MAP)


def _decode_query_part(text, normalize_case=False):
def _decode_query_key(text, normalize_case=False):
return _percent_decode(text, normalize_case=normalize_case,
_decode_map=_QUERY_DECODE_MAP)
_decode_map=_QUERY_KEY_DECODE_MAP)


def _decode_query_value(text, normalize_case=False):
return _percent_decode(text, normalize_case=normalize_case,
_decode_map=_QUERY_VALUE_DECODE_MAP)


def _decode_fragment_part(text, normalize_case=False):
Expand Down Expand Up @@ -1199,9 +1219,9 @@ def to_uri(self):
userinfo=new_userinfo,
host=self.host.encode("idna").decode("ascii"),
path=new_path,
query=tuple([tuple(_encode_query_part(x, maximal=True)
if x is not None else None
for x in (k, v))
query=tuple([(_encode_query_key(k, maximal=True),
_encode_query_value(v, maximal=True)
if v is not None else None)
for k, v in self.query]),
fragment=_encode_fragment_part(self.fragment, maximal=True)
)
Expand Down Expand Up @@ -1245,9 +1265,9 @@ def to_iri(self):
host=textHost,
path=[_decode_path_part(segment)
for segment in self.path],
query=[tuple(_decode_query_part(x)
if x is not None else None
for x in (k, v))
query=[(_decode_query_key(k),
_decode_query_value(v)
if v is not None else None)
for k, v in self.query],
fragment=_decode_fragment_part(self.fragment))

Expand Down Expand Up @@ -1283,10 +1303,14 @@ def to_text(self, with_password=False):
has_scheme=bool(scheme),
has_authority=bool(authority),
maximal=False)
query_string = u'&'.join(
u'='.join((_encode_query_part(x, maximal=False)
for x in ([k] if v is None else [k, v])))
for (k, v) in self.query)
query_parts = []
for k, v in self.query:
if v is None:
query_parts.append(_encode_query_key(k, maximal=False))
else:
query_parts.append(u'='.join((_encode_query_key(k, maximal=False),
_encode_query_value(v, maximal=False))))
query_string = u'&'.join(query_parts)

fragment = self.fragment

Expand Down
12 changes: 10 additions & 2 deletions hyperlink/test/test_url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,10 +532,18 @@ def test_parseEqualSignInParamValue(self):
"""
u = URL.from_text('http://localhost/?=x=x=x')
self.assertEqual(u.get(''), ['x=x=x'])
self.assertEqual(u.to_text(), 'http://localhost/?=x%3Dx%3Dx')
# TODO: see #38
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a TODO? Isn't #38 the bug that specifically asks this not to be done?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test updated. basically this was a legacy twisted test that I was worried about breaking. The TODO was more like "TODO: fix this in Twisted's test suite". Does Twisted even still have the t.p.url tests duplicated in it? If so those will have to change/come out before updating to the 19.x hyperlink release. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah doesn't look like the Twisted test suite has had the deep tests removed yet: https://github.com/twisted/twisted/blob/8292869715d9471681ffff32296bc73b946868a3/src/twisted/python/test/test_url.py#L479

Not sure why that should block a hyperlink review/release though. Whomever does the upgrade on the Twisted side (i.e., me or you, but maybe someone else) can remove the corresponding tests based on info that'll be in the hyperlink changelog.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I feel like one should feel badly about pip install of the latest version causing downstreams' tests to fail with ~no warning ;). That said, this might be what Twisted's own compatibility policy might call a "gross violation of specifications" and there's no compatible fix-forward way to handle this (i.e.: twisted is just directly testing the wrong thing); so I remove the objection unless you can think of a way to emit a warning rather than fail.

# self.assertEqual(u.to_text(), 'http://localhost/?=x%3Dx%3Dx')
u = URL.from_text('http://localhost/?foo=x=x=x&bar=y')
self.assertEqual(u.query, (('foo', 'x=x=x'), ('bar', 'y')))
self.assertEqual(u.to_text(), 'http://localhost/?foo=x%3Dx%3Dx&bar=y')
# TODO: see #38
# self.assertEqual(u.to_text(), 'http://localhost/?foo=x%3Dx%3Dx&bar=y')

u = URL.from_text('https://example.com/?argument=3&argument=4&operator=%3D')
iri = u.to_iri()
self.assertEqual(iri.get('operator'), ['='])
# assert that the equals is not unnecessarily escaped
self.assertEqual(iri.to_uri().get('operator'), ['='])

def test_empty(self):
"""
Expand Down