Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[3.6] bpo-42967: only use '&' as a query string separator (GH-24297) #24532

Merged
merged 3 commits into from Feb 15, 2021
Merged

[3.6] bpo-42967: only use '&' as a query string separator (GH-24297) #24532

merged 3 commits into from Feb 15, 2021

Conversation

Reviewers

@AdamGold AdamGold

@ned-deily ned-deily

Assignees

@ned-deily ned-deily

Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@orsenthil @Fidget-Spinner @AdamGold @ned-deily @the-knights-who-say-ni @bedevere-bot
@orsenthil
Copy link
Member

@orsenthil orsenthil commented Feb 15, 2021 •
edited by bedevere-bot

[3.6] bpo-42967: only use '&' as a query string separator (GH-24297)

Backport of fcbe0cb to 3.6

https://bugs.python.org/issue42967

https://bugs.python.org/issue42967
@bedevere-bot bedevere-bot mentioned this pull request Feb 15, 2021
Merged
@AdamGold @orsenthil
bpo-42967: [security] Address a web cache-poisoning issue reported in…
54a1189 
… urllib.parse.parse_qsl().

urllib.parse will only us "&" as query string separator by default
instead of both ";" and "&" as allowed in earlier versions. An optional
argument seperator with default value "&" is added to specify the
separator.

Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Éric Araujo <merwok@netwok.org>.
(cherry picked from commit fcbe0cb)

Co-authored-by: Adam Goldschmidt <adamgold7@gmail.com>
@orsenthil orsenthil force-pushed the backport-fcbe0cb-3.6 branch from 5e91b18 to 54a1189 Feb 15, 2021
@orsenthil
Copy link
Member Author

@orsenthil orsenthil commented Feb 15, 2021

The idle test case failing seems more like an unrelated flake to me.

@orsenthil orsenthil mentioned this pull request Feb 15, 2021
Merged
@Fidget-Spinner
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner commented Feb 15, 2021

Other than the docs issues mentioned in #24528 , this LGTM!
The idlelib tests are from this line here https://github.com/python/cpython/blob/3.6/Lib/idlelib/idle_test/test_configdialog.py#L104 ,
which seem pretty unrelated.

AdamGold
AdamGold suggested changes Feb 15, 2021
View changes
Lib/cgi.py Outdated
@@ -156,7 +160,7 @@ def parse(fp=None, environ=os.environ, keep_blank_values=0, strict_parsing=0):
if environ['REQUEST_METHOD'] == 'POST':
ctype, pdict = parse_header(environ['CONTENT_TYPE'])
if ctype == 'multipart/form-data':
return parse_multipart(fp, pdict)
return parse_multipart(fp, pdict, separator=separator)
Copy link
Contributor

@AdamGold AdamGold Feb 15, 2021 •
edited

I believe we should change the parse_multipart signature here

Copy link
Member Author

@orsenthil orsenthil Feb 15, 2021

Thank you!. Yes, I had noticed in my local, but then lost it. :( - And unfortunately, the existing tests didn't cover to catch too.

@orsenthil
Fix the parse_multipart call.
cf5abe2
@AdamGold
Copy link
Contributor

@AdamGold AdamGold commented Feb 15, 2021 •
edited

We are adding a separator parameter to FieldStorage but I can not see any call to FieldStorage. In other versions, that call is within parse_multipart (therefore the signature changed). I think we can remove separator from FieldStorage altogether (but there are tests for it in this version - so that's odd).

@Fidget-Spinner
fix docs and make logic clearer
b4b1ac1
@orsenthil
Copy link
Member Author

@orsenthil orsenthil commented Feb 15, 2021

We are adding a separator parameter to FieldStorage but I can not see any call to FieldStorage.

It is being called from test() function, which gets called if cgi.py is used as __main__. There is a path to usage, so leaving the changes and sending to parse_qsl in the FieldStorage seems okay to me. (instead of removing it).

@orsenthil orsenthil requested a review from ned-deily Feb 15, 2021
@orsenthil
Copy link
Member Author

@orsenthil orsenthil commented Feb 15, 2021

Hi Ned, the patch against 3.6 is complete. You could merge this when you get a chance and cut the release. Thank you.

@ned-deily ned-deily merged commit 5c17dfc into python:3.6 Feb 15, 2021
4 of 5 checks passed
gentoo-bot pushed a commit to gentoo/cpython that referenced this issue Mar 4, 2021
@orsenthil @mgorny
[3.6] bpo-42967: only use '&' as a query string separator (pythonGH-2…
255d1d4 
…4297)  (pythonGH-24532)

bpo-42967: [security] Address a web cache-poisoning issue reported in
urllib.parse.parse_qsl().

urllib.parse will only us "&" as query string separator by default
instead of both ";" and "&" as allowed in earlier versions. An optional
argument seperator with default value "&" is added to specify the
separator.

Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Adam Goldschmidt <adamgold7@gmail.com>

Rebased for Python 2.7 by Michał Górny
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment