diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java b/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java index 23774a2d45..3caf88a32f 100755 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java +++ b/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: diff --git a/services/src/main/java/org/keycloak/theme/KeycloakSanitizerMethod.java b/services/src/main/java/org/keycloak/theme/KeycloakSanitizerMethod.java index 6e41d05b6b..e28d2e0ab7 100644 --- a/services/src/main/java/org/keycloak/theme/KeycloakSanitizerMethod.java +++ b/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()) { diff --git a/services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java b/services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java index 878995610c..d3e9df2338 100644 --- a/services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java +++ b/services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java @@ -73,6 +73,22 @@ public class KeycloakSanitizerTest { html.set(0, "
"); assertResult("link
", html); + html.set(0, ""); + assertResult("link
", html); + + html.set(0, ""); + assertResult("link
", html); + + // Effectively same as previous case, but with \0 character added + html.set(0, ""); + assertResult("link
", html); + + html.set(0, ""); + assertResult("link
", html); + + html.set(0, ""); + assertResult("", html); + html.set(0, ""); assertResult("", html); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/InstalledAppRedirectPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/InstalledAppRedirectPage.java new file mode 100644 index 0000000000..12d101f244 --- /dev/null +++ b/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 Marek Posolda + */ +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 + } + + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/error/EscapeErrorPageTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/error/EscapeErrorPageTest.java index cf8b257859..06b7a1bee0 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/error/EscapeErrorPageTest.java +++ b/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("<img src="something">", ""); + checkMessage("<img src="something">", ""); } @Test public void hexEscape() { - checkMessage("<img src=something/>", ""); + checkMessage("<img src=something/>", ""); } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java index 03c44553b9..f3de5bd711 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java +++ b/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(ListBack to application
", installedAppPage.getPageTitleText()); + + error = ""; + 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: ", installedAppPage.getPageTitleText()); + } + @Test public void authorizationValidRedirectUri() throws IOException { ClientManager.realm(adminClient.realm("test")).clientId("test-app").addRedirectUris(oauth.getRedirectUri()); diff --git a/themes/src/main/resources/theme/base/login/code.ftl b/themes/src/main/resources/theme/base/login/code.ftl index 6830fc497b..bb0621db93 100755 --- a/themes/src/main/resources/theme/base/login/code.ftl +++ b/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">${msg("copyCodeInstruction")}
<#else> -${code.error}
+${kcSanitize(code.error)}
#if>