summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authormelanie witt <melwittt@gmail.com>2022-08-16 06:49:53 +0000
committermelanie witt <melwittt@gmail.com>2022-12-01 00:01:28 +0000
commit43af75243b79e95330ecd982552d568571623e39 (patch)
tree46276dbe606364903759c55899ef2af11eec05f0
parentd3b46af01b7afa1a9051cb440a7986bfcb1a59b1 (diff)
downloadnova-43af75243b79e95330ecd982552d568571623e39.tar.gz
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 I23ac1cc79482d0fabb359486a4b934463854cae5 (Allow TLS ciphers/protocols to be configurable for console proxies) is not in Train. The difference from the cherry picked change is because the flake8 version on the stable/train branch does not support f-strings [1]. Related-Bug: #1927677 Closes-Bug: #1986545 [1] https://lists.openstack.org/pipermail/openstack-discuss/2019-November/011027.html Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0) (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af) (cherry picked from commit 0e4a257e8636a979605c614a35e79ba47b74d870) (cherry picked from commit 3023e162e1a415ddaa70b4b8fbe24b1771dbe424) (cherry picked from commit 77bc3f004e7fe4077ea035c659630bedef1cfea1) (cherry picked from commit 746d654c23d75f084b6f0c70e6c32b97eebf419c)
-rw-r--r--nova/tests/unit/console/test_websocketproxy.py59
1 files changed, 26 insertions, 33 deletions
diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py
index 27677bbf78..46a51dbf7c 100644
--- a/nova/tests/unit/console/test_websocketproxy.py
+++ b/nova/tests/unit/console/test_websocketproxy.py
@@ -627,12 +627,12 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase):
self.wh.server.top_new_client(conn, address)
self.assertIsNone(self.wh._compute_rpcapi)
- def test_reject_open_redirect(self):
+ def test_reject_open_redirect(self, url='//example.com/%2F..'):
# This will test the behavior when an attempt is made to cause an open
# redirect. It should be rejected.
mock_req = mock.MagicMock()
mock_req.makefile().readline.side_effect = [
- b'GET //example.com/%2F.. HTTP/1.1\r\n',
+ ('GET %s HTTP/1.1\r\n' % url).encode('utf-8'),
b''
]
@@ -657,39 +657,32 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase):
result = output.readlines()
# Verify no redirect happens and instead a 400 Bad Request is returned.
- self.assertIn('400 URI must not start with //', result[0].decode())
+ # NOTE: As of python 3.10.6 there is a fix for this vulnerability,
+ # which will cause a 301 Moved Permanently error to be returned
+ # instead that redirects to a sanitized version of the URL with extra
+ # leading '/' characters removed.
+ # See https://github.com/python/cpython/issues/87389 for details.
+ # We will consider either response to be valid for this test. This will
+ # also help if and when the above fix gets backported to older versions
+ # of python.
+ errmsg = result[0].decode()
+ expected_nova = '400 URI must not start with //'
+ expected_cpython = '301 Moved Permanently'
+
+ self.assertTrue(expected_nova in errmsg or expected_cpython in errmsg)
+
+ # If we detect the cpython fix, verify that the redirect location is
+ # now the same url but with extra leading '/' characters removed.
+ if expected_cpython in errmsg:
+ location = result[3].decode()
+ location = location.removeprefix('Location: ').rstrip('\r\n')
+ self.assertTrue(
+ location.startswith('/example.com/%2F..'),
+ msg='Redirect location is not the expected sanitized URL',
+ )
def test_reject_open_redirect_3_slashes(self):
- # This will test the behavior when an attempt is made to cause an open
- # redirect. It should be rejected.
- mock_req = mock.MagicMock()
- mock_req.makefile().readline.side_effect = [
- b'GET ///example.com/%2F.. HTTP/1.1\r\n',
- b''
- ]
-
- client_addr = ('8.8.8.8', 54321)
- mock_server = mock.MagicMock()
- # This specifies that the server will be able to handle requests other
- # than only websockets.
- mock_server.only_upgrade = False
-
- # Constructing a handler will process the mock_req request passed in.
- handler = websocketproxy.NovaProxyRequestHandler(
- mock_req, client_addr, mock_server)
-
- # Collect the response data to verify at the end. The
- # SimpleHTTPRequestHandler writes the response data to a 'wfile'
- # attribute.
- output = io.BytesIO()
- handler.wfile = output
- # Process the mock_req again to do the capture.
- handler.do_GET()
- output.seek(0)
- result = output.readlines()
-
- # Verify no redirect happens and instead a 400 Bad Request is returned.
- self.assertIn('400 URI must not start with //', result[0].decode())
+ self.test_reject_open_redirect(url='///example.com/%2F..')
class NovaWebsocketSecurityProxyTestCase(test.NoDBTestCase):