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 (original) (raw)

Expand Up

@@ -426,6 +426,22 @@ def test_ftp_nonexisting(self):

self.assertFalse(e.exception.filename)

self.assertTrue(e.exception.reason)

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

Expand Down