Skip to content

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

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.

Sign up for GitHub

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
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

orsenthil
Copy link
Member

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

[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
Copy link
Member Author

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
Member

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 reviewed 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
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should change the parse_multipart signature here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AdamGold
Copy link
Contributor

AdamGold commented Feb 15, 2021 •
edited
Loading

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).

@orsenthil
Copy link
Member Author

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 February 15, 2021 18:21
@orsenthil
Copy link
Member Author

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
gentoo-bot pushed a commit to gentoo/cpython that referenced this pull request 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
Reviewers

@AdamGold AdamGold AdamGold requested changes

@ned-deily ned-deily Awaiting requested review from ned-deily

Assignees

@ned-deily ned-deily

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@orsenthil @Fidget-Spinner @AdamGold @ned-deily @the-knights-who-say-ni @bedevere-bot