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

tests/test_http.py::test_inject_space failing after bpo-43882 fix #193

Closed
mgorny opened this issue May 7, 2021 · 3 comments
Closed

tests/test_http.py::test_inject_space failing after bpo-43882 fix #193

mgorny opened this issue May 7, 2021 · 3 comments

Comments

@mgorny
Copy link
Contributor

mgorny commented May 7, 2021

bpo-43882 fix changed urllib.parse functions behavior from passing \r\n through to stripping them entirely. As a result, test_inject_space now fails:

_______________________________________________________________ test_inject_space ________________________________________________________________
tests/test_http.py:738: in test_inject_space
    assert req.uri == "/?q=%20HTTP/1.1%0D%0Aignore-http:"
E   AssertionError: assert '/?q=%20HTTP/1.1ignore-http:' == '/?q=%20HTTP/1...0Aignore-http:'
E     - /?q=%20HTTP/1.1ignore-http:
E     + /?q=%20HTTP/1.1%0D%0Aignore-http:
E     ?                ++++++

Given the following comment:

        # "\r\nignore-http:" suffix is nuance for current server implementation
        # please only pay attention to space after "?q="

maybe it would be sufficient to replace the assert with a .startswith("/?q=%20HTTP/1.1", maybe combined with not "\n" in ....

@temoto
Copy link
Member

temoto commented May 13, 2021

@mgorny can you share Python -V which does that?

Please try 08d6993

@mgorny
Copy link
Contributor Author

mgorny commented May 13, 2021

@mgorny can you share Python -V which does that?

To some degree, yes.

$ python3.9 --version
Python 3.9.5
$ python3.8 --version
Python 3.8.10

However, since this is a security fix, we've also backported it to 3.9.4, 3.8.9 and earlier branches on Gentoo. So please don't rely on a specific version.

Please try 08d6993

I can confirm that the test passes for me after cherry-picking that commit.

@temoto temoto closed this as completed in 08d6993 May 13, 2021
@temoto
Copy link
Member

temoto commented May 13, 2021

@mgorny thanks, it's merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants