bpo-42967: only use '&' as a query string separator

[AdamGold]

bpo-42967: [security] urllib.parse.parse_qsl(): Web cache poisoning -
; as a query args separator

https://bugs.python.org/issue42967

Co-authored-by: Ken Jin 28750310+Fidget-Spinner@users.noreply.github.com

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@AdamGold

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[Fidget-Spinner]

@AdamGold I went ahead and re-ran the CLA bot and it seems you signed the CLA here :).

BTW, what are your thoughts about combining our PRs into one? I already added you as a Co-author to PR-24271, meaning the final commit will show us both as commit authors. I also wanted to use some really nice ideas from this PR if you're okay with it.

[AdamGold]

@Fidget-Spinner Sounds like a good idea, do you want me to checkout to your branch and push a commit?

[Fidget-Spinner]

@AdamGold sorry I changed my mind, I decided to close my PR in favor of this one, because I think 2 open PRs might split core devs' attention. I'll just review this one instead.

[Fidget-Spinner]

Some things:

• cgi.py should expose these new arguments IMO so that users can switch. Most of cgi.py's arguments are already passed directly into urllib.parse anyways (the docs also mention that behavior here). You can see the affected functions in my PR here https://github.com/python/cpython/pull/24271/files#
• Following that,FieldStorage should also accept the separator switch IMO, and you may have to pass the separator in a few more places (you can refer to my PR for that too, should be mostly painless :) ).

Other than those nits, and waiting for decision on the bpo about whether to boolean switch or allow user selection, I think this PR is quite well done :).

[AdamGold]

@Fidget-Spinner Thanks! much appreciated.
Regarding CGI and FieldStorage, isn't that used for POST requests?

[Fidget-Spinner]

@Fidget-Spinner Thanks! much appreciated.
Regarding CGI and FieldStorage, isn't that used for POST requests?

Yup, however it uses parse_qsl internally somewhere, so I'm guessing we probably still have to pass it in (most of its __init__ arguments are already directly passed to parse_qsl anyways).

[AdamGold]

@Fidget-Spinner Thanks! much appreciated.
Regarding CGI and FieldStorage, isn't that used for POST requests?

Yup, however it uses parse_qsl internally somewhere, so I'm guessing we probably still have to pass it in (most of its __init__ arguments are already directly passed to parse_qsl anyways).

From the docs:

The script’s input is connected to the client too, and sometimes the form data is read this way; at other times the form data is passed via the “query string” part of the URL

I honestly don't know enough about CGI to tell - how is the query string being used there?

[Fidget-Spinner]

I honestly don't know enough about CGI to tell - how is the query string being used there?

So I didn't know this either, but a POST form can contain a query string too... 😮; so FieldStorage has to parse query strings: https://stackoverflow.com/questions/14710061/is-it-valid-to-combine-a-form-post-with-a-query-string/14710450

[merwok]

I prefered the other version that always split on & and had a boolean param to also parse on ;. Waiting on Senthil’s and others’ opinion on the bug report.

[AdamGold]

So I didn't know this either, but a POST form can contain a query string too..

@Fidget-Spinner implemented.

[merwok]

Just a process note: github’s user experience is not great for reviewers when a PR has its history rewritten.
We prefer that people add new commits, and the final merge is always a squash merge. Thanks!
See https://devguide.python.org/pullrequest/#submitting

[AdamGold]

@merwok Just saw this comment - noted, will be following this from now on.

[orsenthil]

I prefered the other version that always split on & and had a boolean param to also parse on ;.

@merwok - This is a good thinking, and I originally thought along the lines of stick with '&' or just '&' and ';' in some form.

Taking werkzeug package example (used in Flask and other projects) made it think that it's very reasonable to with a separator argument, with default as & pallets/werkzeug@6503519#diff-2d06ddf3f490f8cbcda42ae788a0586f08d5c96e67d89784a785c8295759e60dR30 (changed since 2009)

1. The W3C recommendation -

Let strings be the result of strictly splitting the string payload on U+0026 AMPERSAND characters (&).

is taken care by this patch.

2. Using a bool to accommodate an older unrecommended separator will confuse people. But to accommodate the old un-changed practice, like you showed the example of debian bugs, providing the option to override separator made more sense, since it was explicit.

The breaking change is disallowing both & and ; as separator together, and due to attack-vector of cache-poisoning exposed as a vulnerability, the primary breaking change is about disallowing both these characters together. I think, seems reasonable thing to do, even as it potential break some software that were relying on it.

[orsenthil]

I think, using the separator with '&' as default seems much better than not providing any option change and strictly use '&' as this moment.

We will only need a documentation that highlights this behavior and change in behavior from past and this patch can be merged.

[AdamGold]

@orsenthil Where should I write this documentation?