Skip to content

Commit 77bc3f0

Browse files
committed
Adapt websocketproxy tests for SimpleHTTPServer fix
In response to bug 1927677 we added a workaround to NovaProxyRequestHandler to respond with a 400 Bad Request if an open redirect is attempted: Ie36401c782f023d1d5f2623732619105dc2cfa24 I95f68be76330ff09e5eabb5ef8dd9a18f5547866 Recently in python 3.10.6, a fix has landed in cpython to respond with a 301 Moved Permanently to a sanitized URL that has had extra leading '/' characters removed. This breaks our existing unit tests which assume a 400 Bad Request as the only expected response. This adds handling of a 301 Moved Permanently response and asserts that the redirect location is the expected sanitized URL. Doing this instead of checking for a given python version will enable the tests to continue to work if and when the cpython fix gets backported to older python versions. While updating the tests, the opportunity was taken to commonize the code of two unit tests that were nearly identical. Conflicts: nova/tests/unit/console/test_websocketproxy.py NOTE(melwitt): The conflict is because change I58b0382c86d4ef798572edb63d311e0e3e6937bb (Refactor and rename test_tcp_rst_no_compute_rpcapi) is not in Victoria. Related-Bug: #1927677 Closes-Bug: #1986545 Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a (cherry picked from commit 15769b8) (cherry picked from commit 4a2b44c) (cherry picked from commit 0e4a257) (cherry picked from commit 3023e16)
1 parent 3224ceb commit 77bc3f0

File tree

1 file changed

+26
-35
lines changed

1 file changed

+26
-35
lines changed

nova/tests/unit/console/test_websocketproxy.py

Lines changed: 26 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -627,12 +627,12 @@ def test_tcp_rst_no_compute_rpcapi(self):
627627
self.wh.server.top_new_client(conn, address)
628628
self.assertIsNone(self.wh._compute_rpcapi)
629629

630-
def test_reject_open_redirect(self):
630+
def test_reject_open_redirect(self, url='//example.com/%2F..'):
631631
# This will test the behavior when an attempt is made to cause an open
632632
# redirect. It should be rejected.
633633
mock_req = mock.MagicMock()
634634
mock_req.makefile().readline.side_effect = [
635-
b'GET //example.com/%2F.. HTTP/1.1\r\n',
635+
f'GET {url} HTTP/1.1\r\n'.encode('utf-8'),
636636
b''
637637
]
638638

@@ -657,41 +657,32 @@ def test_reject_open_redirect(self):
657657
result = output.readlines()
658658

659659
# Verify no redirect happens and instead a 400 Bad Request is returned.
660-
self.assertIn('400 URI must not start with //', result[0].decode())
660+
# NOTE: As of python 3.10.6 there is a fix for this vulnerability,
661+
# which will cause a 301 Moved Permanently error to be returned
662+
# instead that redirects to a sanitized version of the URL with extra
663+
# leading '/' characters removed.
664+
# See https://github.com/python/cpython/issues/87389 for details.
665+
# We will consider either response to be valid for this test. This will
666+
# also help if and when the above fix gets backported to older versions
667+
# of python.
668+
errmsg = result[0].decode()
669+
expected_nova = '400 URI must not start with //'
670+
expected_cpython = '301 Moved Permanently'
671+
672+
self.assertTrue(expected_nova in errmsg or expected_cpython in errmsg)
673+
674+
# If we detect the cpython fix, verify that the redirect location is
675+
# now the same url but with extra leading '/' characters removed.
676+
if expected_cpython in errmsg:
677+
location = result[3].decode()
678+
location = location.removeprefix('Location: ').rstrip('\r\n')
679+
self.assertTrue(
680+
location.startswith('/example.com/%2F..'),
681+
msg='Redirect location is not the expected sanitized URL',
682+
)
661683

662684
def test_reject_open_redirect_3_slashes(self):
663-
# This will test the behavior when an attempt is made to cause an open
664-
# redirect. It should be rejected.
665-
mock_req = mock.MagicMock()
666-
mock_req.makefile().readline.side_effect = [
667-
b'GET ///example.com/%2F.. HTTP/1.1\r\n',
668-
b''
669-
]
670-
671-
# Collect the response data to verify at the end. The
672-
# SimpleHTTPRequestHandler writes the response data by calling the
673-
# request socket sendall() method.
674-
self.data = b''
675-
676-
def fake_sendall(data):
677-
self.data += data
678-
679-
mock_req.sendall.side_effect = fake_sendall
680-
681-
client_addr = ('8.8.8.8', 54321)
682-
mock_server = mock.MagicMock()
683-
# This specifies that the server will be able to handle requests other
684-
# than only websockets.
685-
mock_server.only_upgrade = False
686-
687-
# Constructing a handler will process the mock_req request passed in.
688-
websocketproxy.NovaProxyRequestHandler(
689-
mock_req, client_addr, mock_server)
690-
691-
# Verify no redirect happens and instead a 400 Bad Request is returned.
692-
self.data = self.data.decode()
693-
self.assertIn('Error code: 400', self.data)
694-
self.assertIn('Message: URI must not start with //', self.data)
685+
self.test_reject_open_redirect(url='///example.com/%2F..')
695686

696687
@mock.patch('websockify.websocketproxy.select_ssl_version')
697688
def test_ssl_min_version_is_not_set(self, mock_select_ssl):

0 commit comments

Comments
 (0)