From 30d0e9d22dae51392e5a3748a1c68c116667359a Mon Sep 17 00:00:00 2001 From: Marek Posolda Date: Thu, 2 Feb 2023 08:34:30 +0100 Subject: [PATCH] Fixes for OOB endpoint and KeycloakSanitizer (#16774) (cherry picked from commit 91ac2fb9dd50808ff5c76d639594ba14a8d0d016) --- .../FreeMarkerLoginFormsProvider.java | 1 + .../theme/KeycloakSanitizerMethod.java | 26 +++++ .../keycloak/theme/KeycloakSanitizerTest.java | 16 +++ .../pages/InstalledAppRedirectPage.java | 107 ++++++++++++++++++ .../testsuite/error/EscapeErrorPageTest.java | 4 +- .../oauth/AuthorizationCodeTest.java | 27 ++++- .../main/resources/theme/base/login/code.ftl | 4 +- 7 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/InstalledAppRedirectPage.java 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 b2df16336f09..ba21cc33ac6c 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 @@ protected Response createResponse(LoginFormsPages page) { 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 6e41d05b6bc5..e28d2e0ab798 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 @@ 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 Object exec(List list) throws TemplateModelException { } 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 878995610ce8..d3e9df2338fa 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 void testUrls() throws Exception { html.set(0, "

link

"); assertResult("

link

", html); + html.set(0, "

link

"); + assertResult("

link

", html); + + html.set(0, "

link

"); + assertResult("

link

", html); + + // Effectively same as previous case, but with \0 character added + html.set(0, "

link

"); + assertResult("

link

", html); + + html.set(0, "

link

"); + assertResult("

link

", html); + + html.set(0, "

link

"); + assertResult("", html); + html.set(0, "

link

"); assertResult("

link

", 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 000000000000..12d101f244ac --- /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 cf8b25785930..06b7a1bee05c 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 void URL() { @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 03c44553b961..f3de5bd711fe 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.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 testRealms) { RealmRepresentation realmRepresentation = loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class); @@ -92,16 +95,30 @@ public void authorizationRequestInstalledApp() throws IOException { oauth.doLogin("test-user@localhost", "password"); - String title = PageUtils.getPageTitle(driver); - Assert.assertEquals("Success code", title); - - driver.findElement(By.id(OAuth2Constants.CODE)).getAttribute("value"); + installedAppPage.getSuccessCode(); 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 = "

Back to application

"; + installedAppPage.open("test", null, error, null); + + // Assert text escaped and not "a" link present + installedAppPage.assertLinkBackToApplicationNotPresent(); + Assert.assertEquals("Error code:

Back to application

", installedAppPage.getPageTitleText()); + + error = "

Back to application

"; + 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:

Back to application

", 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 6830fc497bff..bb0621db93f6 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))} <#elseif section = "form">
@@ -12,7 +12,7 @@

${msg("copyCodeInstruction")}

<#else> -

${code.error}

+

${kcSanitize(code.error)}