fix(oauth): encode authorization URL parameters

slack_sdk/oauth/authorize_url_generator/__init__.py

@@ -1,7 +1,15 @@
 from typing import Optional, Sequence
+from urllib.parse import urlencode
 
 
 class AuthorizeUrlGenerator:
+    """Generate an OAuth authorization URL.
+
+    Pass the complete ``redirect_uri`` and original ``state`` as values,
+    not as pre-encoded authorization query components. This generator
+    handles the outer query encoding.
+    """
+
     def __init__(
         self,
         *,
@@ -20,16 +28,26 @@ def __init__(
     def generate(self, state: str, team: Optional[str] = None) -> str:
         scopes = ",".join(self.scopes) if self.scopes else ""
         user_scopes = ",".join(self.user_scopes) if self.user_scopes else ""
-        url = f"{self.authorization_url}?state={state}&client_id={self.client_id}&scope={scopes}&user_scope={user_scopes}"
+        params = {
+            "state": state,
+            "client_id": self.client_id,
+            "scope": scopes,
+            "user_scope": user_scopes,
+        }
         if self.redirect_uri is not None:
-            url += f"&redirect_uri={self.redirect_uri}"
+            params["redirect_uri"] = self.redirect_uri
         if team is not None:
-            url += f"&team={team}"
-        return url
+            params["team"] = team
+        query = urlencode(params, safe=":,/")
+        return f"{self.authorization_url}?{query}"
 
 
 class OpenIDConnectAuthorizeUrlGenerator:
-    """Refer to https://openid.net/specs/openid-connect-core-1_0.html."""
+    """Refer to https://openid.net/specs/openid-connect-core-1_0.html.
+
+    Supply ``redirect_uri``, ``state``, and ``nonce`` as values, without
+    pre-encoding them for the authorization query string.
+    """
 
     def __init__(
         self,
@@ -46,16 +64,16 @@ def __init__(
 
     def generate(self, state: str, nonce: Optional[str] = None, team: Optional[str] = None) -> str:
         scopes = ",".join(self.scopes) if self.scopes else ""
-        url = (
-            f"{self.authorization_url}?"
-            "response_type=code&"
-            f"state={state}&"
-            f"client_id={self.client_id}&"
-            f"scope={scopes}&"
-            f"redirect_uri={self.redirect_uri}"
-        )
+        params = {
+            "response_type": "code",
+            "state": state,
+            "client_id": self.client_id,
+            "scope": scopes,
+            "redirect_uri": self.redirect_uri,
+        }
         if team is not None:
-            url += f"&team={team}"
+            params["team"] = team
         if nonce is not None:
-            url += f"&nonce={nonce}"
-        return url
+            params["nonce"] = nonce
+        query = urlencode(params, safe=":,/")
+        return f"{self.authorization_url}?{query}"

tests/slack_sdk/oauth/authorize_url_generator/test_generator.py

@@ -1,4 +1,5 @@
 import unittest
+from urllib.parse import parse_qs, urlsplit
 
 from slack_sdk.oauth import AuthorizeUrlGenerator, OpenIDConnectAuthorizeUrlGenerator
 
@@ -70,3 +71,83 @@ def test_openid_connect(self):
             "&nonce=nnn"
         )
         self.assertEqual(expected, url)
+
+    def test_query_parameters_round_trip(self):
+        redirect_uri = "https://www.example.com/callback?view=home&lang=en"
+        generator = AuthorizeUrlGenerator(
+            client_id="111.222",
+            redirect_uri=redirect_uri,
+            scopes=["chat:write", "commands"],
+            user_scopes=["search:read"],
+        )
+        for state in (
+            "",
+            "plus+value",
+            "space value",
+            "key=value&other=value",
+            "percent%20value",
+            "fragment#value",
+            "日本語",
+        ):
+            with self.subTest(state=state):
+                url = generator.generate(state=state, team="T12345")
+                self.assertDictEqual(
+                    {
+                        "state": [state],
+                        "client_id": ["111.222"],
+                        "scope": ["chat:write,commands"],
+                        "user_scope": ["search:read"],
+                        "redirect_uri": [redirect_uri],
+                        "team": ["T12345"],
+                    },
+                    parse_qs(urlsplit(url).query, keep_blank_values=True),
+                )
+
+    def test_openid_connect_query_parameters_round_trip(self):
+        redirect_uri = "https://www.example.com/oidc/callback?view=home%20page&lang=en"
+        generator = OpenIDConnectAuthorizeUrlGenerator(
+            client_id="111.222",
+            redirect_uri=redirect_uri,
+            scopes=["openid", "profile"],
+        )
+        for value in (
+            "",
+            "plus+value",
+            "space value",
+            "key=value&other=value",
+            "percent%20value",
+            "fragment#value",
+            "日本語",
+        ):
+            with self.subTest(value=value):
+                state = f"state-{value}"
+                url = generator.generate(state=state, nonce=value, team="T12345")
+                self.assertDictEqual(
+                    {
+                        "response_type": ["code"],
+                        "state": [state],
+                        "client_id": ["111.222"],
+                        "scope": ["openid,profile"],
+                        "redirect_uri": [redirect_uri],
+                        "team": ["T12345"],
+                        "nonce": [value],
+                    },
+                    parse_qs(urlsplit(url).query, keep_blank_values=True),
+                )
+
+    def test_pre_encoded_authorization_values_are_not_decoded(self):
+        redirect_uri = "https://www.example.com/callback%3Fa%3Db"
+        generator = AuthorizeUrlGenerator(
+            client_id="111.222", redirect_uri=redirect_uri
+        )
+        url = generator.generate(state="abc%2B")
+        self.assertIn("state=abc%252B", url)
+        self.assertIn("redirect_uri=https://www.example.com/callback%253Fa%253Db", url)
+
+        openid_generator = OpenIDConnectAuthorizeUrlGenerator(
+            client_id="111.222", redirect_uri=redirect_uri
+        )
+        url = openid_generator.generate(state="abc%2B", nonce="x%26y")
+        self.assertIn("state=abc%252B", url)
+        self.assertIn("redirect_uri=https://www.example.com/callback%253Fa%253Db", url)
+        self.assertIn("nonce=x%2526y", url)