Repository: dwavesystems/dwave-cloud-client
Pull request #607 (2024-02-28 22:56:42)

Few minor auth flow security and performance improvements
Updating the auth flow according to [OAuth 2.0 Security Best Current Practice](https://www.ietf.org/archive/id/draft-ietf-oauth-security-topics-25.html), motivated by #605.


Patch:
diff --git a/dwave/cloud/auth/flows.py b/dwave/cloud/auth/flows.py
index 679d752..6b1092e 100644
--- a/dwave/cloud/auth/flows.py
+++ b/dwave/cloud/auth/flows.py
@@ -258,11 +258,11 @@ class LeapAuthFlow(AuthFlow):
 
     @staticmethod
     def _infer_leap_success_uri(leap_api_endpoint: str) -> str:
-        return urljoin(leap_api_endpoint, '/leap/openid/success')
+        return urljoin(leap_api_endpoint, '/leap/openid/success/')
 
     @staticmethod
     def _infer_leap_error_uri(leap_api_endpoint: str) -> str:
-        return urljoin(leap_api_endpoint, '/leap/openid/error')
+        return urljoin(leap_api_endpoint, '/leap/openid/error/')
 
     @classmethod
     def from_config_model(cls, config: ClientConfig, **kwargs) -> LeapAuthFlow:
@@ -378,7 +378,12 @@ class LeapAuthFlow(AuthFlow):
 
             # when code received, exchange it for token
             try:
-                self.fetch_token(code=app.query.get('code'), state=app.query.get('state'))
+                # verify state (Cross-Site Request Forgery protection)
+                state = app.query.get('state')
+                if state != self.state:
+                    raise ValueError('State mismatch')
+
+                self.fetch_token(code=app.query.get('code'), state=state)
 
             except OAuthError as exc:
                 # store for main thread
@@ -399,7 +404,7 @@ class LeapAuthFlow(AuthFlow):
                 return add_params_to_uri(error_uri, query)
 
             # redirect to leap success page
-            return add_params_to_uri(success_uri, app.query)
+            return success_uri
 
         app = RequestCaptureAndRedirectApp(
             message=self._REDIRECT_DONE_MSG,
diff --git a/releasenotes/notes/auth-flow-security-improvements-c30c5bdaecbfafe6.yaml b/releasenotes/notes/auth-flow-security-improvements-c30c5bdaecbfafe6.yaml
new file mode 100644
index 0000000..92d3cda
--- /dev/null
+++ b/releasenotes/notes/auth-flow-security-improvements-c30c5bdaecbfafe6.yaml
@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Use fixed authorization success URL, without any authorization query
+    parameters appended (to minimize exposure), as they are not needed on the
+    authorization server's success page.
+  - |
+    Improve CSRF protection on localhost redirect URI during authorization code
+    exchange. Previously, ``state`` was verified only if supplied. Now it's
+    mandatory. Please note this is an additional CSRF protection on top of the
+    PKCE, just in case PKCE is not supported (or properly implemented) server-side.


Test patch:
diff --git a/tests/auth/test_flows.py b/tests/auth/test_flows.py
index 8c897a4..6031d96 100644
--- a/tests/auth/test_flows.py
+++ b/tests/auth/test_flows.py
@@ -290,6 +290,7 @@ class TestLeapAuthFlowRedirect(unittest.TestCase):
             location = response.history[0].headers.get('Location')
             self.assertTrue(location.startswith(config.leap_api_endpoint))
             self.assertIn('/success', location)
+            self.assertEqual(urlsplit(location).query, '')
 
             f.join()
             fetch_token.assert_called_once_with(code=mock_code, state=ctx['state'])
@@ -314,8 +315,8 @@ class TestLeapAuthFlowRedirect(unittest.TestCase):
             # `params` determine if auth was denied or approved
             self.authorize_ready.wait()
 
-            query = self.authorize_response_params.copy()
-            query.update(state=self.authorize_query['state'])
+            query = dict(state=self.authorize_query['state'])
+            query.update(self.authorize_response_params)
             return requests.get(self.authorize_query['redirect_uri'], params=query)
 
         def run(self):
@@ -454,3 +455,38 @@ class TestLeapAuthFlowRedirect(unittest.TestCase):
             # oauth exception raised by the redirect flow runner
             with self.assertRaisesRegex(type(error), str(error)):
                 t.exception()
+
+    @mock.patch('click.echo', return_value=None)
+    def test_csrf_failure_during_code_exchange(self, m):
+        # error case: access authorized, but code exchange fails due to state mismatch
+        config = ClientConfig(leap_api_endpoint='https://example.com/leap')
+        flow = LeapAuthFlow.from_config_model(config)
+
+        code = '1234'
+
+        error = ValueError("State mismatch")
+
+        with mock.patch.object(flow, 'fetch_token') as fetch_token:
+            authorize_response_params = dict(code=code, state='spoofed')
+
+            t = self.FlowRunner(flow=flow, authorize_response_params=authorize_response_params)
+            t.start()
+
+            response = t.authorize()
+
+            # check response is a redirect to leap error page
+            self.assert_redirect_to_error_page(response, flow)
+
+            # and error page params are constructed properly
+            expected_params = dict(error=type(error).__name__,
+                                   error_description=str(error),
+                                   state=authorize_response_params['state'])
+            self.assert_redirect_params(response, expected_params)
+
+            # token fetch is never attempted (state precondition failed)
+            t.join()
+            fetch_token.assert_not_called()
+
+            # oauth exception raised by the redirect flow runner
+            with self.assertRaisesRegex(type(error), str(error)):
+                t.exception()