"%" is not escaped in path, user and password in URL.build()

[serhiy-storchaka]

>>> from yarl import URL
>>> URL.build(scheme="http", host="example.com", path="/%25")
URL('http://example.com/%25')
>>> URL.build(scheme="http", host="example.com", path="/%25").path
'/%'
>>> URL.build(scheme="http", host="example.com", user="%25")
URL('http://%25@example.com')
>>> URL.build(scheme="http", host="example.com", user="%25").user
'%'
>>> URL.build(scheme="http", host="example.com", password="%25")
URL('http://:%25@example.com')
>>> URL.build(scheme="http", host="example.com", password="%25").password
'%'

All other parts are properly escaped. For example:

>>> URL.build(scheme="http", host="example.com", fragment="%25")
URL('http://example.com/#%25')
>>> URL.build(scheme="http", host="example.com", fragment="%25").fragment
'%'
>>> URL.build(scheme="http", host="example.com", query={"%25": "%25"})
URL('http://example.com/?%2525=%2525')
>>> URL.build(scheme="http", host="example.com", query={"%25": "%25"}).query
<MultiDictProxy('%25': '%25')>

yarl 1.5.1

[webknjaz]

Is it a bug report or a feature request? AFAIU it's not explicitly documented (nor tested) how this API should behave. I think we need to do this first in order to decide how to move forward.

cc @asvetlov

[serhiy-storchaka]

It looks like a bug. The path is interpreted as (partially) encoded while encoded=False (by default). How otherwise I can create a URL with path containing literal %25?

[webknjaz]

Fair enough. This does seem inconsistent. FWIW the note @ https://yarl.readthedocs.io/en/latest/api.html#yarl.URL says:

Sometimes encoding performed by yarl is not acceptable for certain WEB server.

Passing encoded=True parameter prevents URL auto-encoding, user is responsible about URL correctness.

Don’t use this option unless there is no other way for keeping URL attributes not touched.

Any URL manipulations don’t guarantee correct encoding, URL parts could be re-quoted even if encoded parameter was explicitly set.

Even though it's written in one place, it seems to be referring to any uses of the encoded throughout the lib.

From what I can gather, you're supposed to take care of doing encoding by yourself if you use encoded=True but if encoded=False is used, YARL attempts to guess whether to autoencode the input and when you pass %25 it treats that as already encoded string trying to emulate a typical web-browser behavior.

This is why I originally stated that the corner cases are poorly documented.

[webknjaz]

P.S. It's worth nothing that yarl relies on stdlib urllib.parse and I have a hunch that it may affect some of the behaviors. It definitely needs more explicit test cases.

[serhiy-storchaka]

Seems the problem is due to using the same quoters for URL() and URL.build(). In URL() components can be partially encoded. They can contain %-escaped parts and characters which need to be encoded. But URL.build() is expected to get non-encoded values, so path "/%25" means "/%25", not "/%". Quoters in URL.build() should use requote=False.

[serhiy-storchaka]

There is the same issue with with_xxx() methods:

>>> URL('http://example.org').with_path('/%2d').path
'/-'
>>> URL('http://example.org').with_name('%2d').path
'/-'
>>> URL('http://example.org').with_fragment('%2d').fragment
'-'
>>> URL('http://example.org').with_user('%2d').user
'-'
>>> URL('http://example.org').with_password('%2d').password
'-'

[besfahbod]

Thanks for this fix! It addresses a major issue with getting Unicode values in and out intact.

Although, I should note that, the change, although fixing a bug, was a large change in behavior for the main API, and is probably breaking many application code, such as aiohttp. I, personally, ended up here when trying to understand why a minor-version upgrade resulted in so many test failures in our application code.

My suggestion would have been to roll this out as a major-version update, considering it a API-breaking change. I guess it's a bit too late for that, though. But, worth considering if something like this happen again, or someone thinks it's really worth making that happen now.