Fixes for OOB endpoint and KeycloakSanitizer (#16773)

(cherry picked from commit 91ac2fb9dd50808ff5c76d639594ba14a8d0d016)

services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java

@@ -275,6 +275,7 @@ public class FreeMarkerLoginFormsProvider implements LoginFormsProvider {
                         new OAuthGrantBean(accessCode, client, clientScopesRequested));
                 break;
             case CODE:
+                attributes.remove("message"); // No need to include "message" attribute as error is included in separate field anyway
                 attributes.put(OAuth2Constants.CODE, new CodeBean(accessCode, messageType == MessageType.ERROR ? getFirstMessageUnformatted() : null));
                 break;
             case X509_CONFIRM:

services/src/main/java/org/keycloak/theme/KeycloakSanitizerMethod.java

@@ -19,6 +19,7 @@ package org.keycloak.theme;
 
 import freemarker.template.TemplateMethodModelEx;
 import freemarker.template.TemplateModelException;
+import org.owasp.html.Encoding;
 
 import java.util.List;
 import java.util.regex.Matcher;
@@ -40,11 +41,36 @@ public class KeycloakSanitizerMethod implements TemplateMethodModelEx {
         }
         
         String html = list.get(0).toString();
+
+        html = decodeHtmlFull(html);
+
         String sanitized = KeycloakSanitizerPolicy.POLICY_DEFINITION.sanitize(html);
         
         return fixURLs(sanitized);
     }
 
+
+    // Fully decode HTML. Assume it can be encoded multiple times
+    private String decodeHtmlFull(String html) {
+        if (html == null) return null;
+
+        int MAX_DECODING_COUNT = 5; // Max count of attempts for decoding HTML (in case it was encoded multiple times)
+        String decodedHtml;
+
+        for (int i = 0; i < MAX_DECODING_COUNT; i++) {
+            decodedHtml = Encoding.decodeHtml(html);
+            if (decodedHtml.equals(html)) {
+                // HTML is decoded. We can return it
+                return html;
+            } else {
+                // Next attempt
+                html = decodedHtml;
+            }
+        }
+
+        return "";
+    }
+
     private String fixURLs(String msg) {
         Matcher matcher = HREF_PATTERN.matcher(msg);
         if (matcher.find()) {

services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java

@@ -73,6 +73,22 @@ public class KeycloakSanitizerTest {
         html.set(0, "<p><a href=\"javascript:alert('hello!');\">link</a></p>");
         assertResult("<p>link</p>", html);
 
+        html.set(0, "<p><a href=\"javascript:alert(document.domain);\">link</a></p>");
+        assertResult("<p>link</p>", html);
+
+        html.set(0, "<p><a href=\"javascript&colon;alert(document.domain);\">link</a></p>");
+        assertResult("<p>link</p>", html);
+
+        // Effectively same as previous case, but with \0 character added
+        html.set(0, "<p><a href=\"javascript&\0colon;alert(document.domain);\">link</a></p>");
+        assertResult("<p>link</p>", html);
+
+        html.set(0, "<p><a href=\"javascript&amp;amp;\0colon;alert(document.domain);\">link</a></p>");
+        assertResult("<p>link</p>", html);
+
+        html.set(0, "<p><a href=\"javascript&amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;\0colon;alert(document.domain);\">link</a></p>");
+        assertResult("", html);
+
         html.set(0, "<p><a href=\"https://localhost?key=123&msg=abc\">link</a></p>");
         assertResult("<p><a href=\"https://localhost?key=123&msg=abc\" rel=\"nofollow\">link</a></p>", html);
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/InstalledAppRedirectPage.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright 2022 Red Hat, Inc. and/or its affiliates
+ *  and other contributors as indicated by the @author tags.
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ */
+
+package org.keycloak.testsuite.pages;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.junit.Assert;
+import org.keycloak.OAuth2Constants;
+import org.keycloak.common.util.KeycloakUriBuilder;
+import org.keycloak.services.Urls;
+import org.openqa.selenium.By;
+import org.openqa.selenium.NoSuchElementException;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.support.FindBy;
+
+/**
+ * Page represented by code.ftl. It is used by "Installed applications" (KeycloakInstalled)
+ *
+ * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+public class InstalledAppRedirectPage extends AbstractPage {
+
+    @FindBy(id = "code")
+    private WebElement code;
+
+    @FindBy(id = "kc-page-title")
+    private WebElement pageTitle;
+
+    @FindBy(className = "alert-error")
+    private WebElement errorBox;
+
+    @Override
+    public void open() {
+        throw new UnsupportedOperationException("Use method: open(code, error, errorDescription)");
+    }
+
+
+    public void open(String realmName, String code, String error, String errorDescription) {
+        try {
+            KeycloakUriBuilder kcUriBuilder = KeycloakUriBuilder.fromUri(Urls.realmInstalledAppUrnCallback(new URI(oauth.AUTH_SERVER_ROOT), realmName));
+            if (code != null) {
+                kcUriBuilder.queryParam(OAuth2Constants.CODE, code);
+            }
+            if (error != null) {
+                kcUriBuilder.queryParam(OAuth2Constants.ERROR, error);
+            }
+            if (errorDescription != null) {
+                kcUriBuilder.queryParam(OAuth2Constants.ERROR_DESCRIPTION, errorDescription);
+            }
+            String oobEndpointUri = kcUriBuilder.build().toString();
+            driver.navigate().to(oobEndpointUri);
+        } catch (URISyntaxException use) {
+            throw new IllegalArgumentException(use);
+        }
+    }
+
+    @Override
+    public boolean isCurrent() {
+        throw new UnsupportedOperationException("Use method 'isCurrentExpectSuccess' or 'isCurrentExpectError'");
+    }
+
+
+    public String getSuccessCode() {
+        Assert.assertEquals("Success code", getPageTitleText());
+        return code.getAttribute("value");
+    }
+
+    public String getPageTitleText() {
+        return pageTitle.getText();
+    }
+
+    // Check if link is present inside title or error box
+    public void assertLinkBackToApplicationNotPresent() {
+        try {
+            pageTitle.findElement(By.tagName("a"));
+            throw new AssertionError("Link was present inside title");
+        } catch (NoSuchElementException nsee) {
+            // Ignore
+        }
+
+        try {
+            errorBox.findElement(By.tagName("a"));
+            throw new AssertionError("Link was present inside error box");
+        } catch (NoSuchElementException nsee) {
+            // Ignore
+        }
+
+    }
+}

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/error/EscapeErrorPageTest.java

@@ -56,12 +56,12 @@ public class EscapeErrorPageTest extends AbstractKeycloakTest {
 
     @Test
     public void ampersandEscape() {
-        checkMessage("&lt;img src=&quot;something&quot;&gt;", "<img src=\"something\">");
+        checkMessage("&lt;img src=&quot;something&quot;&gt;", "");
     }
 
     @Test
     public void hexEscape() {
-        checkMessage("&#x3C;img src&#61;something&#x2F;&#x3E;", "<img src=something/>");
+        checkMessage("&#x3C;img src&#61;something&#x2F;&#x3E;", "");
     }
 
     @Test

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java

@@ -31,7 +31,7 @@ import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.AssertEvents;
 import org.keycloak.testsuite.pages.ErrorPage;
-import org.keycloak.testsuite.pages.PageUtils;
+import org.keycloak.testsuite.pages.InstalledAppRedirectPage;
 import org.keycloak.testsuite.util.ClientManager;
 import org.keycloak.testsuite.util.OAuthClient;
 import org.openqa.selenium.By;
@@ -58,6 +58,9 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
     @Page
     private ErrorPage errorPage;
 
+    @Page
+    private InstalledAppRedirectPage installedAppPage;
+
     @Override
     public void addTestRealms(List<RealmRepresentation> testRealms) {
         RealmRepresentation realmRepresentation = loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class);
@@ -92,16 +95,30 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
 
         oauth.doLogin("test-user@localhost", "password");
 
-        String title = PageUtils.getPageTitle(driver);
+        installedAppPage.getSuccessCode();
-        Assert.assertEquals("Success code", title);
-
-        driver.findElement(By.id(OAuth2Constants.CODE)).getAttribute("value");
 
         events.expectLogin().detail(Details.REDIRECT_URI, oauth.AUTH_SERVER_ROOT + "/realms/test/protocol/openid-connect/oauth/oob").assertEvent().getDetails().get(Details.CODE_ID);
 
         ClientManager.realm(adminClient.realm("test")).clientId("test-app").removeRedirectUris(Constants.INSTALLED_APP_URN);
     }
 
+    @Test
+    public void authorizationRequestInstalledAppErrors() throws IOException {
+        String error = "<p><a href=\"javascript&amp;colon;alert(document.domain);\">Back to application</a></p>";
+        installedAppPage.open("test", null, error, null);
+
+        // Assert text escaped and not "a" link present
+        installedAppPage.assertLinkBackToApplicationNotPresent();
+        Assert.assertEquals("Error code: <p>Back to application</p>", installedAppPage.getPageTitleText());
+
+        error = "<p><a href=\"http://foo.com\">Back to application</a></p>";
+        installedAppPage.open("test", null, error, null);
+
+        // In this case, link is not sanitized as it is valid link, however it is escaped and not shown as a link
+        installedAppPage.assertLinkBackToApplicationNotPresent();
+        Assert.assertEquals("Error code: <p><a href=\"http://foo.com\" rel=\"nofollow\">Back to application</a></p>", installedAppPage.getPageTitleText());
+    }
+
     @Test
     public void authorizationValidRedirectUri() throws IOException {
         ClientManager.realm(adminClient.realm("test")).clientId("test-app").addRedirectUris(oauth.getRedirectUri());

themes/src/main/resources/theme/base/login/code.ftl

@@ -4,7 +4,7 @@
         <#if code.success>
             ${msg("codeSuccessTitle")}
         <#else>
-            ${msg("codeErrorTitle", code.error)}
+            ${kcSanitize(msg("codeErrorTitle", code.error))}
         </#if>
     <#elseif section = "form">
         <div id="kc-code">
@@ -12,7 +12,7 @@
                 <p>${msg("copyCodeInstruction")}</p>
                 <input id="code" class="${properties.kcTextareaClass!}" value="${code.code}"/>
             <#else>
-                <p id="error">${code.error}</p>
+                <p id="error">${kcSanitize(code.error)}</p>
             </#if>
         </div>
     </#if>