bpo-34711: Return HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory by aixtools · Pull Request #9687 · python/cpython

aixtools · 2018-10-03T09:38:28Z

Please provide guidelines on how much, if any comments, to have in Lib/http/server.py.

I assume there is too much now, while my gut says no comments may only make people wonder why this is being done.

https://bugs.python.org/issue34711

asvetlov · 2018-10-03T10:06:35Z

Would you add a unit test?

aixtools · 2018-10-03T16:34:19Z

I do not understand the question. This PR is to allow an existing test to pass in unique situations - rather than code exceptions in the test.

Issue17324: -- see https://bugs.python.org/issue17324#msg183247 says, literally:

Note that the server has served the contents of "foo.txt" as "foo.txt/". It shouldn't do that!

My current understanding is that the goal is to bring the existing "unit test" test_httpservers and Lib/http/server.py into sync with the quote above.

It is know "known" that Windows, under certain conditions, and AIX (always) will return a filename ending in "/" as "OK" rather than as "NOT_FOUND".

However, if you could explain what you feel is missing perhaps this could be expanded with another unit test.

asvetlov · 2018-10-03T20:25:36Z

You are adding a new code to fix a bug.
A test for checking the behavior to never fall into the problem again as a result of some http server refactoring is very desirable.

aixtools · 2018-10-04T10:10:51Z

You are adding a new code to fix a bug.
A test for checking the behavior to never fall into the problem again as a result of some http server refactoring is very desirable.

Well, since you can think of it, I guess you are much smarter than I. I can not figure out how to test this in a way it is not already being tested.

In the test I am "adding this code for" it tests:

    def test_get(self):
        #constructs the path relative to the root directory of the HTTPServer
        response = self.request(self.base_url + '/test')
        self.check_status_and_reason(response, HTTPStatus.OK, data=self.data)
        # check for trailing "/" which should return 404. See Issue17324
        response = self.request(self.base_url + '/test/')
        self.check_status_and_reason(response, HTTPStatus.NOT_FOUND)
        response = self.request(self.base_url + '/')
        self.check_status_and_reason(response, HTTPStatus.OK)
        response = self.request(self.base_url)
        self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
        response = self.request(self.base_url + '/?hi=2')
        self.check_status_and_reason(response, HTTPStatus.OK)
        response = self.request(self.base_url + '?hi=1')
        self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
        self.assertEqual(response.getheader("Location"),
                         self.base_url + "/?hi=1")
        response = self.request('/ThisDoesNotExist')
        self.check_status_and_reason(response, HTTPStatus.NOT_FOUND)
        response = self.request('/' + 'ThisDoesNotExist' + '/')
        self.check_status_and_reason(response, HTTPStatus.NOT_FOUND)

and

    def test_path_without_leading_slash(self):
        response = self.request(self.tempdir_name + '/test')
        self.check_status_and_reason(response, HTTPStatus.OK, data=self.data)
        response = self.request(self.tempdir_name + '/test/')
        self.check_status_and_reason(response, HTTPStatus.NOT_FOUND)
        response = self.request(self.tempdir_name + '/')
        self.check_status_and_reason(response, HTTPStatus.OK)
        response = self.request(self.tempdir_name)
        self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
        response = self.request(self.tempdir_name + '/?hi=2')
        self.check_status_and_reason(response, HTTPStatus.OK)
        response = self.request(self.tempdir_name + '?hi=1')
        self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
        self.assertEqual(response.getheader("Location"),
                         self.tempdir_name + "/?hi=1")

Please help me out with howto test this addition - that is not already tested above?

aixtools · 2018-10-05T20:49:18Z

On 03/10/2018 22:26, Andrew Svetlov wrote: You are adding a new code to fix a bug. A test for checking the behavior to never fall into the problem again as a result of some http server refactoring is very desirable.

Hello Andrew, a) I would gladly write a unittest - but I am confused because the issue I am correcting is to correct a existing unittest. b) from the responses to earlier PR's (modify open() so that despite that some OS accept a trailing slash as a filename - refuse to open a filename with a trailing slash; b) modify the existing unittest so that the platform differences are accepted (sometimes trailing slash should be 404 error, other platforms should be okay). The first was regarding as too deep, the second as too superficial - and so now the unittest is left asis because it is specifically for the module and modify the code so that the premises of the unittest are met. Please help me out with what you think needs testing - and I'll try to come up with something/ Sincerely, Michael

taleinat · 2018-10-26T13:50:39Z

Which test(s) currently fail(s) on AIX? Can you perhaps supply a link to such failures on a buildbot?

Is there a test case for the similar issue with junctions on Windows? I assume not since this would be failing on our Windows CI builds. If this fixes that too, it could be good to add such a test.

aixtools · 2018-10-29T19:32:00Z

On 10/26/2018 4:51 PM, Tal Einat wrote: Which test(s) currently fail(s) on AIX? Can you perhaps supply a link to such failures on a buildbot?

This is also available on the bots, but rather than having to search in 5000 to 20000 lines of text, here is the issue: test_all (test.test_httpservers.MiscTestCase) ... ok ====================================================================== FAIL: test_get (test.test_httpservers.SimpleHTTPServerTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/data/prj/python/git/python3-3.8/Lib/test/test_httpservers.py", line 422, in test_get self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) File "/data/prj/python/git/python3-3.8/Lib/test/test_httpservers.py", line 374, in check_status_and_reason self.assertEqual(response.status, status) AssertionError: 200 != <HTTPStatus.NOT_FOUND: 404> ====================================================================== FAIL: test_path_without_leading_slash (test.test_httpservers.SimpleHTTPServerTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/data/prj/python/git/python3-3.8/Lib/test/test_httpservers.py", line 523, in test_path_without_leading_slash self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) File "/data/prj/python/git/python3-3.8/Lib/test/test_httpservers.py", line 374, in check_status_and_reason self.assertEqual(response.status, status) AssertionError: 200 != <HTTPStatus.NOT_FOUND: 404>

…

---------------------------------------------------------------------- Ran 64 tests in 1.779s FAILED (failures=2, skipped=13) /data/prj/python/git/python3-3.8/Lib/test/support/__init__.py:1572: ResourceWarning: unclosed <socket.socket [closed] fd=5, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6> gc.collect() test test_httpservers failed test_httpservers failed == Tests result: FAILURE == 1 test failed: test_httpservers Total duration: 2 sec 35 ms Tests result: FAILURE AIX is not the only platform that will open a file with a trailing slash. FYI: this is the third solution, as the others were rejected - see PR9360 and PR9675.

Is there a test case for the similar issue with junctions on Windows? I assume not since this would be failing on our Windows CI builds. If this fixes that too, it could be good to add such a test. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

ncoghlan

LGTM, just waiting for the new CI run that was triggered by my rewording of the NEWS entry.

Reject as HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory

4318068

the-knights-who-say-ni added the CLA signed label Oct 3, 2018

bedevere-bot added the awaiting review label Oct 3, 2018

Reword NEWS entry

6bb9045

ncoghlan approved these changes Dec 26, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 26, 2018

ncoghlan merged commit 2062a20 into python:master Dec 26, 2018

bedevere-bot removed the awaiting merge label Dec 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-34711: Return HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory#9687

bpo-34711: Return HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory#9687
ncoghlan merged 2 commits into
python:masterfrom
aixtools:bpo-34711

aixtools commented Oct 3, 2018 •

edited by bedevere-bot

Loading

Uh oh!

asvetlov commented Oct 3, 2018

Uh oh!

aixtools commented Oct 3, 2018

Uh oh!

asvetlov commented Oct 3, 2018

Uh oh!

aixtools commented Oct 4, 2018

Uh oh!

aixtools commented Oct 5, 2018 via email

Uh oh!

taleinat commented Oct 26, 2018

Uh oh!

aixtools commented Oct 29, 2018 via email

Uh oh!

ncoghlan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Uh oh!

Conversation

aixtools commented Oct 3, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Oct 3, 2018

Uh oh!

aixtools commented Oct 3, 2018

Uh oh!

asvetlov commented Oct 3, 2018

Uh oh!

aixtools commented Oct 4, 2018

Uh oh!

aixtools commented Oct 5, 2018 via email

Uh oh!

taleinat commented Oct 26, 2018

Uh oh!

aixtools commented Oct 29, 2018 via email

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aixtools commented Oct 3, 2018 •

edited by bedevere-bot

Loading