fix quote-preservation in QUERY_STRING

[helmutg]

QUERY_STRING is supposed to be faithful to the transmitted value. If one requests e.g. /?!%21, the QUERY_STRING should preserve both the unquoted and the quoted exclamation mark. Doing otherwise breaks digest authentication in e.g. a WSGI middleware.

We cannot use yarl here as preserving the original representation is a non-goal for yarl. We must resort to re-parsing the raw_path.

Link: https://github.com/aio-libs/yarl/issues/498 Signed-off-by: Helmut Grohne helmut.grohne@intenta.de

[etianen]

Nice spot!

Can you add a test case for this behavior please?

[etianen]

Hmm... do you have any reference for this requirement? I'm struggling to find it in PEP 333

[helmutg]

I don't think it is defined that clearly in a PEP. The PEP itself is borrows a lot from the CGI interface. The requirement comes more from RFC7616 section 3.4.6. The Authorization header duplicates the request uri and implementations are required to test them for equality. While Digest authentication is a protocol that works over bare HTTP, it can cryptographically prevent changing the request uri in transit and it also can prevent replay attacks. This comparison requires that any digest authentication implementation has access to the relevant parts it has to compare, which happens to include the QUERY_STRING.

Another hint at this is trying wsgiref. If you inspect the QUERY_STRING there, you'll notice that it preserves any quoting or lack thereof.

[helmutg]

I also looked into writing a test case, but that is quite difficult as the session's request method is not faithful to the URI representation and unquotes it. We'd need a different http client library here. Here is the relevant snippets I came up with in tests/test_wsgi.py.

def quoting_application(environ, start_response):
    assert environ["QUERY_STRING"] == "!%21"
    return noop_application(environ, start_response)

And inside EnvironTest:

def testPreserveQuoting(self):
    with self.run_server(quoting_application) as client:
        client.assert_response(path="/?!%21")

Doesn't work as is though. The QUERY_STRING comes back as !! with and without my PR.

[etianen]

Given the HTTP request is serialized over a network socket, surely the client library is irrelevant? The query string will be quoted on the wire.

I think your conversion might not be right:

urllib.parse.urlsplit("foo?foo=!").query 'foo=!'

On Fri, 1 Oct 2021 at 07:48, helmutg @.***> wrote:

I also looked into writing a test case, but that is quite difficult as the session's request method is not faithful to the URI representation and unquotes it. We'd need a different http client library here. Here is the relevant snippets I came up with in tests/test_wsgi.py.

def quoting_application(environ, start_response): assert environ["QUERY_STRING"] == "!%21" return noop_application(environ, start_response)

And inside EnvironTest:

def testPreserveQuoting(self): with self.run_server(quoting_application) as client: client.assert_response(path="/?!%21")

Doesn't work as is though. The QUERY_STRING comes back as !! with and without my PR.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etianen/aiohttp-wsgi/pull/31#issuecomment-931955728, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABEKCCQIWBGJJVP3K3OAWTUEVKR7ANCNFSM5FCLWYVQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[helmutg]

The client library very much is relevant when it turns a url http://somesite/?!%21 into a request path sent over the wire that happens to be /?!! as it did not preserve the quoting (which is ok most of the time, but not sufficient for writing the test case).

Why do you think the conversion would be wrong? That sounds totally right to me. What would you have expected here?

[etianen]

Sorry, you're right about the conversion. I was having a stupid day yesterday.

So your hypothesis is that the aiohttp client is sending http://somesite/?!%21 as http://somesite/?!! over the wire? Seems reasonable. But I think we still need tests for this behaviour.

I think the way to test this is by making a raw TCP connection to the socket using asyncio.create_connection(), and sending a really basic HTTP 1.1 GET request as text. That way there's no ambiguity over the bytes on the wire.

[helmutg]

I think that the issue reported here has been fixed in aiohttp by passing encoded=True to the yarl.URL constructor used for creating .rel_url. The relevant commit likely is https://github.com/aio-libs/aiohttp/commit/f2afa2f054ba9e6c5d142e00233f0073925e7893.

[etianen]

Cool, that makes life easier! Thanks for following this up!

On Mon, 20 Dec 2021 at 11:09, helmutg @.***> wrote:

I think that the issue reported here has been fixed in aiohttp by passing encoded=True to the yarl.URL constructor used for creating .rel_url. The relevant commit likely is @.*** https://github.com/aio-libs/aiohttp/commit/f2afa2f054ba9e6c5d142e00233f0073925e7893 .

— Reply to this email directly, view it on GitHub https://github.com/etianen/aiohttp-wsgi/pull/31#issuecomment-997825732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABEKCBCVQJIIR3AG56NSRLUR4FHHANCNFSM5FCLWYVQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>