Improve the default OAuth page renderers (#1140)

Author: seratch

bolt/pom.xml

@@ -13,6 +13,10 @@
     <version>1.29.2-SNAPSHOT</version>
     <packaging>jar</packaging>
 
+    <properties>
+        <commons-text.version>1.10.0</commons-text.version>
+    </properties>
+
     <dependencies>
         <dependency>
             <groupId>com.slack.api</groupId>
@@ -29,6 +33,11 @@
             <artifactId>slack-app-backend</artifactId>
             <version>${project.version}</version>
         </dependency>
+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-text</artifactId>
+            <version>${commons-text.version}</version>
+        </dependency>
 
         <dependency>
             <groupId>com.amazonaws</groupId>

bolt/src/main/java/com/slack/api/bolt/service/builtin/oauth/view/default_impl/OAuthDefaultInstallPageRenderer.java

@@ -1,6 +1,7 @@
 package com.slack.api.bolt.service.builtin.oauth.view.default_impl;
 
 import com.slack.api.bolt.service.builtin.oauth.view.OAuthInstallPageRenderer;
+import org.apache.commons.text.StringEscapeUtils;
 
 public class OAuthDefaultInstallPageRenderer implements OAuthInstallPageRenderer {
 
@@ -23,7 +24,8 @@ public class OAuthDefaultInstallPageRenderer implements OAuthInstallPageRenderer
 
     @Override
     public String render(String authorizeUrl) {
-        return PAGE_TEMPLATE.replaceAll("__URL__", authorizeUrl == null ? "" : authorizeUrl);
+        String url = StringEscapeUtils.escapeHtml4(authorizeUrl);
+        return PAGE_TEMPLATE.replaceAll("__URL__", url == null ? "" : url);
     }
 
 }

bolt/src/main/java/com/slack/api/bolt/service/builtin/oauth/view/default_impl/OAuthDefaultRedirectUriPageRenderer.java

@@ -2,6 +2,7 @@
 
 import com.slack.api.bolt.model.Installer;
 import com.slack.api.bolt.service.builtin.oauth.view.OAuthRedirectUriPageRenderer;
+import org.apache.commons.text.StringEscapeUtils;
 
 public class OAuthDefaultRedirectUriPageRenderer implements OAuthRedirectUriPageRenderer {
 
@@ -65,13 +66,18 @@ public String renderSuccessPage(Installer installer, String completionUrl) {
         String browserUrl = installer == null || installer.getTeamId() == null
                 ? "https://slack.com/"
                 : "https://app.slack.com/client/" + installer.getTeamId();
+
+        url = StringEscapeUtils.escapeHtml4(url);
+        browserUrl = StringEscapeUtils.escapeHtml4(browserUrl);
         return SUCCESS_PAGE_TEMPLATE
                 .replaceAll("__URL__", url == null ? "" : url)
                 .replaceAll("__BROWSER_URL__", browserUrl);
     }
 
     @Override
     public String renderFailurePage(String installPath, String reason) {
+        installPath = StringEscapeUtils.escapeHtml4(installPath);
+        reason = StringEscapeUtils.escapeHtml4(reason);
         return FAILURE_PAGE_TEMPLATE
                 .replaceAll("__INSTALL_PATH__", installPath == null ? "" : installPath)
                 .replaceAll("__REASON__", reason == null ? "unknown_error" : reason);

bolt/src/test/java/test_locally/app/OAuthCallbacksTest.java

@@ -7,12 +7,15 @@
 import com.slack.api.bolt.AppConfig;
 import com.slack.api.bolt.model.Bot;
 import com.slack.api.bolt.model.Installer;
+import com.slack.api.bolt.model.builtin.DefaultInstaller;
 import com.slack.api.bolt.request.Request;
 import com.slack.api.bolt.request.RequestHeaders;
 import com.slack.api.bolt.request.builtin.OAuthCallbackRequest;
 import com.slack.api.bolt.response.Response;
 import com.slack.api.bolt.service.InstallationService;
 import com.slack.api.bolt.service.builtin.ClientOnlyOAuthStateService;
+import com.slack.api.bolt.service.builtin.oauth.view.OAuthRedirectUriPageRenderer;
+import com.slack.api.bolt.service.builtin.oauth.view.default_impl.OAuthDefaultRedirectUriPageRenderer;
 import lombok.extern.slf4j.Slf4j;
 import org.junit.After;
 import org.junit.Before;
@@ -151,4 +154,65 @@ public void oauthPersistenceErrorCallback() throws Exception {
         Response response = app.run(req);
         assertThat(response.getStatusCode(), is(expectedStatusCode));
     }
+
+    @Test
+    public void testDefaultRenderer_renderSuccessPage() {
+        OAuthRedirectUriPageRenderer renderer = new OAuthDefaultRedirectUriPageRenderer();
+        {
+            Installer installer = new DefaultInstaller();
+            installer.setAppId("A123");
+            installer.setTeamId("T123");
+            String page = renderer.renderSuccessPage(installer, "https://www.example.com/?foo=bar&baz=123");
+            assertThat(page.contains("https://www.example.com/?foo=bar&baz=123"), is(false));
+            assertThat(page.contains("<meta http-equiv=\"refresh\" content=\"0; URL=https://www.example.com/?foo=bar&amp;baz=123\">"), is(true));
+            assertThat(page.contains("<a href=\"https://www.example.com/?foo=bar&amp;baz=123\">here</a>"), is(true));
+            assertThat(page.contains("<a href=\"https://app.slack.com/client/T123\" target=\"_blank\">"), is(true));
+        }
+        {
+            Installer installer = new DefaultInstaller();
+            installer.setAppId(null);
+            installer.setTeamId(null);
+            String page = renderer.renderSuccessPage(installer, null);
+            assertThat(page.contains("<meta http-equiv=\"refresh\" content=\"0; URL=slack://open\">"), is(true));
+            assertThat(page.contains("<a href=\"slack://open\">here</a>"), is(true));
+            assertThat(page.contains("<a href=\"https://slack.com/\" target=\"_blank\">"), is(true));
+        }
+        {
+            Installer installer = new DefaultInstaller();
+            installer.setAppId("A123");
+            installer.setTeamId("T123");
+            String page = renderer.renderSuccessPage(installer, null);
+            assertThat(page.contains("slack://app?team=T123&id=A123"), is(false));
+            assertThat(page.contains("<meta http-equiv=\"refresh\" content=\"0; URL=slack://app?team=T123&amp;id=A123\">"), is(true));
+            assertThat(page.contains("<a href=\"slack://app?team=T123&amp;id=A123\">here</a>"), is(true));
+            assertThat(page.contains("<a href=\"https://app.slack.com/client/T123\" target=\"_blank\">"), is(true));
+        }
+        {
+            Installer installer = new DefaultInstaller();
+            installer.setAppId("A123");
+            installer.setTeamId("T123");
+            installer.setEnterpriseId("E123");
+            installer.setIsEnterpriseInstall(true);
+            installer.setEnterpriseUrl("https://test.enterprise.slack.com/");
+            String page = renderer.renderSuccessPage(installer, null);
+            assertThat(page.contains("<meta http-equiv=\"refresh\" content=\"0; URL=https://test.enterprise.slack.com/manage/organization/apps/profile/A123/workspaces/add\">"), is(true));
+            assertThat(page.contains("<a href=\"https://test.enterprise.slack.com/manage/organization/apps/profile/A123/workspaces/add\">here</a>"), is(true));
+            assertThat(page.contains("<a href=\"https://app.slack.com/client/T123\" target=\"_blank\">"), is(true));
+        }
+    }
+
+    @Test
+    public void testDefaultRenderer_renderFailurePage() {
+        OAuthRedirectUriPageRenderer renderer = new OAuthDefaultRedirectUriPageRenderer();
+        {
+            String page = renderer.renderFailurePage("/slack/install", "access_denied");
+            assertThat(page.contains("<a href=\"/slack/install\">here</a> or contact the app owner (reason: access_denied)"), is(true));
+        }
+        {
+            String page = renderer.renderFailurePage("/slack/install&team=T123&foo=bar", "<b>test</b>");
+            assertThat(page.contains("/slack/install&team=T123&foo=bar"), is(false));
+            assertThat(page.contains("<b>test</b>"), is(false));
+            assertThat(page.contains("<a href=\"/slack/install&amp;team=T123&amp;foo=bar\">here</a> or contact the app owner (reason: &lt;b&gt;test&lt;/b&gt;)"), is(true));
+        }
+    }
 }

bolt/src/test/java/test_locally/app/OAuthStartTest.java

@@ -48,7 +48,7 @@ public String issueNewState(Request request, Response response) {
                 "</head>\n" +
                 "<body>\n" +
                 "<h2>Slack App Installation</h2>\n" +
-                "<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state=generated-state-value\"><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/[email protected] 2x\" /></a></p>\n" +
+                "<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&amp;scope=commands%2Cchat%3Awrite&amp;user_scope=&amp;state=generated-state-value\"><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/[email protected] 2x\" /></a></p>\n" +
                 "</body>\n" +
                 "</html>", response.getBody());
     }
@@ -78,7 +78,7 @@ public void start_state_validation_disabled() throws Exception {
                 "</head>\n" +
                 "<body>\n" +
                 "<h2>Slack App Installation</h2>\n" +
-                "<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state=\"><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/[email protected] 2x\" /></a></p>\n" +
+                "<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&amp;scope=commands%2Cchat%3Awrite&amp;user_scope=&amp;state=\"><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/[email protected] 2x\" /></a></p>\n" +
                 "</body>\n" +
                 "</html>", response.getBody());
     }

bolt/src/test/java/test_locally/app/OpenIDConnectTest.java

@@ -66,7 +66,7 @@ public void deleteNonceFromDatastore(String nonce) {
             "</head>\n" +
             "<body>\n" +
             "<h2>Slack App Installation</h2>\n" +
-            "<p><a href=\"https://slack.com/openid/connect/authorize?client_id=111.222&response_type=code&scope=openid%2Cemail%2Cprofile&state=generated-state-value&nonce=generated-nonce-value\"><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/[email protected] 2x\" /></a></p>\n" +
+            "<p><a href=\"https://slack.com/openid/connect/authorize?client_id=111.222&amp;response_type=code&amp;scope=openid%2Cemail%2Cprofile&amp;state=generated-state-value&amp;nonce=generated-nonce-value\"><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/[email protected] 2x\" /></a></p>\n" +
             "</body>\n" +
             "</html>";