Skip to content

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

Merged
ned-deily merged 3 commits intopython:3.6from
orsenthil:backport-fcbe0cb-3.6
Feb 15, 2021
Merged

[3.6] bpo-42967: only use '&' as a query string separator (GH-24297) #24532
ned-deily merged 3 commits intopython:3.6from
orsenthil:backport-fcbe0cb-3.6

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

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
Member

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 reviewed changes
Lib/cgi.py Outdated
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

@orsenthil orsenthil Feb 15, 2021

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

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

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

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

1 more reviewer

@AdamGold AdamGold AdamGold requested changes

Reviewers whose approvals may not affect merge requirements

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