bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. by corona10 · Pull Request #1216 · python/cpython
def test_ftp_illegalhost(self): illegal_ftp_hosts = [ 'ftp://foo:bar%0d%0aINJECTED@example.net/file.png', 'fTp://foo:bar%0d%0aINJECTED@example.net/file.png', 'FTP://foo:bar%0d%0aINJECTED@example.net/file.png', 'ftp://foo:bar%0aINJECTED@example.net/file.png', 'fTp://foo:bar%0aINJECTED@example.net/file.png', 'FTP://foo:bar%0aINJECTED@example.net/file.png'
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that it's interesting to test different cases, I suggest to only test %0d%0a and %0a, so only keep two tests using ftp:// prefix.
for host in illegal_ftp_hosts: with self.assertRaises(urllib.error.URLError) as e: urlopen(host) self.assertFalse(e.exception.filename) self.assertTrue(e.exception.reason)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is invalid: it must check that the error contains "illegal host". In my case, the test only succeded because of another URLError.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, the test pass if the connection fails with <urlopen error ftp error ConnectionRefusedError(111, 'Connection refused')>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above, the tests should also include injection attempts on directories and filenames. After all, these portions of a URL illicit different FTP commands at different times.
@patch.object(urllib.request, 'MAXFTPCACHE', 0) def test_ftp_cache_pruning(self): self.fakeftp()