Improve the default OAuth page renderers not to embed any params without escaping them (#882)

seratch · web-flow · commit 7b4b082b896b · 2023-04-13T12:30:18.000+09:00
diff --git a/setup.py b/setup.py
@@ -58,7 +58,7 @@
     ),
     include_package_data=True,  # MANIFEST.in
     install_requires=[
-        "slack_sdk>=3.20.2,<4",
+        "slack_sdk>=3.21.1,<4",
     ],
     setup_requires=["pytest-runner==5.2"],
     tests_require=async_test_dependencies,
diff --git a/slack_bolt/oauth/internals.py b/slack_bolt/oauth/internals.py
@@ -1,3 +1,4 @@
+import html
 from logging import Logger
 from typing import Optional
 from typing import Union
@@ -32,7 +33,7 @@ def _build_callback_success_response(  # type: ignore
         debug_message = f"Handling an OAuth callback success (request: {request.query})"
         self._logger.debug(debug_message)
 
-        html = self._redirect_uri_page_renderer.render_success_page(
+        page_content = self._redirect_uri_page_renderer.render_success_page(
             app_id=installation.app_id,
             team_id=installation.team_id,
             is_enterprise_install=installation.is_enterprise_install,
@@ -44,7 +45,7 @@ def _build_callback_success_response(  # type: ignore
                 "Content-Type": "text/html; charset=utf-8",
                 "Set-Cookie": self._state_utils.build_set_cookie_for_deletion(),
             },
-            body=html,
+            body=page_content,
         )
 
     def _build_callback_failure_response(  # type: ignore
@@ -60,14 +61,13 @@ def _build_callback_failure_response(  # type: ignore
         # Adding a bit more details to the error code to help installers understand what's happening.
         # This modification in the HTML page works only when developers use this built-in failure handler.
         detailed_error = build_detailed_error(reason)
-        html = self._redirect_uri_page_renderer.render_failure_page(detailed_error)
         return BoltResponse(
             status=status,
             headers={
                 "Content-Type": "text/html; charset=utf-8",
                 "Set-Cookie": self._state_utils.build_set_cookie_for_deletion(),
             },
-            body=html,
+            body=self._redirect_uri_page_renderer.render_failure_page(detailed_error),
         )
 
 
@@ -85,7 +85,7 @@ def _build_default_install_page_html(url: str) -> str:
 </head>
 <body>
 <h2>Slack App Installation</h2>
-<p><a href="{url}"><img alt=""Add to Slack"" height="40" width="139" src="https://platform.slack-edge.com/img/add_to_slack.png" srcset="https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/add_to_slack@2x.png 2x" /></a></p>
+<p><a href="{html.escape(url)}"><img alt=""Add to Slack"" height="40" width="139" src="https://platform.slack-edge.com/img/add_to_slack.png" srcset="https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/add_to_slack@2x.png 2x" /></a></p>
 </body>
 </html>
 """  # noqa: E501
@@ -142,4 +142,4 @@ def build_detailed_error(reason: str) -> str:
     elif reason == "storage_error":
         return f"{reason}: The app's server encountered an issue. Contact the app developer."
     else:
-        return f"{reason}: This error code is returned from Slack. Refer to the documents for details."
+        return f"{html.escape(reason)}: This error code is returned from Slack. Refer to the documents for details."
diff --git a/tests/adapter_tests_async/test_async_falcon.py b/tests/adapter_tests_async/test_async_falcon.py
@@ -201,5 +201,5 @@ def test_oauth(self):
         response = client.simulate_get("/slack/install")
         assert response.status_code == 200
         assert response.headers.get("content-type") == "text/html; charset=utf-8"
-        assert response.headers.get("content-length") == "597"
+        assert response.headers.get("content-length") == "609"
         assert "https://slack.com/oauth/v2/authorize?state=" in response.text
diff --git a/tests/adapter_tests_async/test_async_fastapi.py b/tests/adapter_tests_async/test_async_fastapi.py
@@ -209,7 +209,7 @@ async def endpoint(req: Request):
         response = client.get("/slack/install", allow_redirects=False)
         assert response.status_code == 200
         assert response.headers.get("content-type") == "text/html; charset=utf-8"
-        assert response.headers.get("content-length") == "597"
+        assert response.headers.get("content-length") == "609"
         assert "https://slack.com/oauth/v2/authorize?state=" in response.text
 
     def test_custom_props(self):
diff --git a/tests/adapter_tests_async/test_async_sanic.py b/tests/adapter_tests_async/test_async_sanic.py
@@ -221,6 +221,6 @@ async def endpoint(req: Request):
 
         # NOTE: Although sanic-testing 0.6 does not have this value,
         # Sanic apps properly generate the content-length header
-        # assert response.headers.get("content-length") == "597"
+        # assert response.headers.get("content-length") == "609"
 
         assert "https://slack.com/oauth/v2/authorize?state=" in response.text
diff --git a/tests/adapter_tests_async/test_async_starlette.py b/tests/adapter_tests_async/test_async_starlette.py
@@ -219,5 +219,5 @@ async def endpoint(req: Request):
         response = client.get("/slack/install", allow_redirects=False)
         assert response.status_code == 200
         assert response.headers.get("content-type") == "text/html; charset=utf-8"
-        assert response.headers.get("content-length") == "597"
+        assert response.headers.get("content-length") == "609"
         assert "https://slack.com/oauth/v2/authorize?state=" in response.text
diff --git a/tests/slack_bolt/oauth/test_internals.py b/tests/slack_bolt/oauth/test_internals.py
@@ -1,4 +1,4 @@
-from slack_bolt.oauth.internals import build_detailed_error
+from slack_bolt.oauth.internals import build_detailed_error, _build_default_install_page_html
 
 
 class TestOAuthInternals:
@@ -23,3 +23,26 @@ def test_build_detailed_error_others(self):
         assert result.startswith(
             "access_denied: This error code is returned from Slack. Refer to the documents for details."
         )
+
+    def test_build_detailed_error_others_with_tags(self):
+        result = build_detailed_error("<b>test</b>")
+        assert result.startswith(
+            "&lt;b&gt;test&lt;/b&gt;: This error code is returned from Slack. Refer to the documents for details."
+        )
+
+    def test_build_default_install_page_html(self):
+        test_patterns = [
+            {
+                "input": "https://slack.com/oauth/v2/authorize?state=random&client_id=111.222&scope=commands",
+                "expected": "https://slack.com/oauth/v2/authorize?state=random&amp;client_id=111.222&amp;scope=commands",
+            },
+            {
+                "input": "<b>test</b>",
+                "expected": "&lt;b&gt;test&lt;/b&gt;",
+            },
+        ]
+        for pattern in test_patterns:
+            url = pattern["input"]
+            result = _build_default_install_page_html(url)
+            assert url not in result
+            assert pattern["expected"] in result
