inconsistent query arg handling between python and C http request parsers with yarl 1.6.0

[mwhudson]

🐞 Describe the bug

If you upgrade to yarl 1.6.0 and are using the C HttpRequestParser class, your query arguments are not dequoted (by which I mean, request.query['foo'] is something like %7B%22val%22: 3%7D and not {"val": 3}). If you use the Python HttpRequestParser class, yarl 1.6.0 does not change the behaviour.

💡 To Reproduce

>>> from aiohttp import http_parser
>>> p = http_parser.HttpRequestParserC(1, None); p.feed_data(b'GET /test?b=%22val%22 HTTP/1.1\r\n\r\n')[0][0][0].url.query['b']
'%22val%22'
>>> p = http_parser.HttpRequestParserPy(1, None); p.feed_data(b'GET /test?b=%22val%22 HTTP/1.1\r\n\r\n')[0][0][0].url.query['b']
'"val"'

(with yarl 1.5.1 both return "val").

📋 Your version of the Python

3.6 and 3.8 at least -- don't think it matters

📋 Your version of the aiohttp/yarl/multidict distributions

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.6.2
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: fafhrd91@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.6/dist-packages
Requires: attrs, async-timeout, yarl, multidict, chardet, typing-extensions, idna-ssl

$ python -m pip show multidict
Name: multidict
Version: 4.7.6
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.6/dist-packages
Requires: 

$ python -m pip show yarl
Name: yarl
Version: 1.6.0
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.6/dist-packages
Requires: idna, multidict, typing-extensions

📋 Additional context

The reason for the inconsistency is that the Python parser calls URL(path) whereas the C parser calls URL.build(path=path) and the latter appears to have changed behaviour in 1.6.0.

[danking]

This bit us as well.

Here's an example curl

(base) # curl 'localhost:5000/x?q=tag%3Dfoo'

Here's a simple server:

from web_common import setup_common_static_routes
from aiohttp import web

routes = web.RouteTableDef()


@routes.get('/x')
async def get_jobs(request):
    print(f'q {request.query.get("q")}')
    return web.Response()


app = web.Application()

setup_common_static_routes(routes)
app.add_routes(routes)

web.run_app(app,
            host='0.0.0.0',
            port=5000)

aiohttp 3.6.0, yarl 1.6.0 the log shows the %3D was not decoded. aiohttp 3.6.0, yarl 1.5.1 the log shows tag=foo.

[TBBle]

I've been poking at this. It affects path parameters as well as query parameters.

The problem appears to be yarl 1.6.0 fixed a bug where yarl.URL.build expects unencoded segments, but was accidentally ignoring % and hence passing already-urlencoded segments through unchanged. Now it converts % into %25C, and later the urldecoding gives you the urlencoded segments from the raw HTTP request back, instead of the segments that the client had urlencoded before putting into the raw HTTP request.

This doesn't affect HttpRequestParserPy because it uses yarl.URL, which does support already-encoded URLs. Although we should be passing encoded=True in the call in HttpRequestParserPy.parse_message, so that it knows it's already-encoded, rather than detecting it. It only makes a difference if yarl's urlencoding formula wants to encode something aiohttp has excluded from encoding:

>>> yarl.URL("/test/%val%22",encoded=True)
URL('/test/%val%22')
>>> yarl.URL("/test/%val%22",encoded=False) # The default for encoded
URL('/test/%25val%22')

Edit: Ah, I didn't notice there was an open PR to fix this already.

[dcecile]

On October 12, aiohttp 3.6.3 was released which restricts yarl to <1.6.0, see bbceca7