3.7.0 regression: aiohttp route matching is now pre url decode instead of post decode

[thehesiod]

Pre 3.7.0 (3.6.3 and before) you could write a route regex match to work on the post-decoded url, however now it swapped to pre-decode causing our route handlers to 404

💡 To Reproduce Running the following server

import aiohttp
from aiohttp import web
import yarl

async def hello(request: web.Request):
    return web.Response(text=str(dict(request.match_info)))

def main():
    app = web.Application()
    app.add_routes([web.get('/hello', hello)])
    app.add_routes([web.get('/{user_ids:(([0-9]+)|(u_[0-9a-f]{16}))(,(([0-9]+)|(u_[0-9a-f]{16})))*}/hello', hello)])
    web.run_app(app, port=8888)

if __name__ == '__main__':
    print(f'aiohttp version: {aiohttp.__version__}')
    print(f'yarl version: {yarl.__version__}')
    main()

with aiohttp 3.6.3 + yarl 1.5.1

and hitting curl 'http://0.0.0.0:8888/467%2C802%2C24834%2C24952%2C25362%2C40574/hello'

results in a 200

However running the server w/ 3.7.0-3.7.4.post0 + same yarl results in 404.

📋 Your version of the Python 3.8.5

📋 Your version of the aiohttp/yarl/multidict distributions various, see above

If I revert the changes to _http_parser.pyx introduced in https://github.com/aio-libs/aiohttp/commit/d3219238447f09162f2c38c63ae136a94f39c594 it works again.

[thehesiod]

I found that by setting AIOHTTP_NO_EXTENSIONS=1 it also works. So there's a discrepancy between the cython and python code

[thehesiod]

so python version just does URL(path) whereas cython does _parse_url

[webknjaz]

so python version just does URL(path) whereas cython does _parse_url

This was adjusted by https://github.com/aio-libs/aiohttp/pull/5498. It was a prerequisite for fixing the security vulnerability https://github.com/aio-libs/aiohttp/security/advisories/GHSA-v6wp-4m6f-gcjg.

[thehesiod]

The issue still occurs in 3.7.4.post0, I'll check if the difference between the python and cython still exists

[thehesiod]

on confirmed the behavior difference between python and cython versions don't exist in 3.7.4.post0, however 3.7.4 still exhibits the behavioral change so updated description

[webknjaz]

Could you send a PR with a failing regression test?

[thehesiod]

@webknjaz https://github.com/aio-libs/aiohttp/pull/5633

[webknjaz]

@webknjaz #5633

Thanks, this better clarifies what exactly you expect to work.

If I revert the changes to _http_parser.pyx introduced in d321923 it works again.

@serhiy-storchaka @asvetlov FYI

[webknjaz]

Now that the regression test is in, somebody needs to dig in and figure out what do we want to adjust for this to work. I wonder if changing https://github.com/aio-libs/aiohttp/blob/09ac1cb/aiohttp/web_urldispatcher.py#L350 would be a good idea or maybe we need to make the change in the parser code :man_shrugging:.

[thehesiod]

ideally whomever made the change that caused this regression could chime in :)

[webknjaz]

I guess. But I imagine this may block the fix for quite a while. So if anybody wants to do Git archeology and attempt to understand the best way of addressing this issue — feel free to step up and do so.

[asvetlov]

Interesting. I doubt if I can find time for this until next week.

[webknjaz]

@asvetlov note that https://github.com/aio-libs/aiohttp/pull/5635 added xfailing regression tests for this bug.

[asvetlov]

Yes, I know. They are really very useful. @thehesiod thank you very much! The problem is: I cannot fix the problem quickly and don't want aiohttp 3.8 blocking.

The good news is: the fix can land in aiohttp 3.8.1 just when it will be ready.

For now, I think that some yarl bugfixes are required first.

[webknjaz]

This won't make it into the 3.8 release stream.