diff --git a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java index fe8dfdce51..2757b36889 100755 --- a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java +++ b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java @@ -170,7 +170,7 @@ public class OAuthRequestAuthenticator { KeycloakUriBuilder redirectUriBuilder = deployment.getAuthUrl().clone() .queryParam(OAuth2Constants.RESPONSE_TYPE, OAuth2Constants.CODE) .queryParam(OAuth2Constants.CLIENT_ID, deployment.getResourceName()) - .queryParam(OAuth2Constants.REDIRECT_URI, Encode.encodeQueryParamAsIs(url)) // Need to encode uri ourselves as queryParam() will not encode % characters. + .queryParam(OAuth2Constants.REDIRECT_URI, url) .queryParam(OAuth2Constants.STATE, state) .queryParam("login", "true"); if(loginHint != null && loginHint.length() > 0){ diff --git a/common/src/main/java/org/keycloak/common/util/Encode.java b/common/src/main/java/org/keycloak/common/util/Encode.java index 63b8f3653f..b19536240f 100755 --- a/common/src/main/java/org/keycloak/common/util/Encode.java +++ b/common/src/main/java/org/keycloak/common/util/Encode.java @@ -24,6 +24,7 @@ import java.nio.ByteBuffer; import java.nio.charset.CharacterCodingException; import java.nio.charset.Charset; import java.nio.charset.CharsetDecoder; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -36,7 +37,7 @@ import java.util.regex.Pattern; */ public class Encode { - private static final String UTF_8 = "UTF-8"; + private static final String UTF_8 = StandardCharsets.UTF_8.name(); private static final Pattern PARAM_REPLACEMENT = Pattern.compile("_resteasy_uri_parameter"); @@ -84,9 +85,7 @@ public class Encode case '@': continue; } - StringBuffer sb = new StringBuffer(); - sb.append((char) i); - pathEncoding[i] = URLEncoder.encode(sb.toString()); + pathEncoding[i] = URLEncoder.encode(String.valueOf((char) i)); } pathEncoding[' '] = "%20"; System.arraycopy(pathEncoding, 0, matrixParameterEncoding, 0, pathEncoding.length); @@ -119,9 +118,7 @@ public class Encode queryNameValueEncoding[i] = "+"; continue; } - StringBuffer sb = new StringBuffer(); - sb.append((char) i); - queryNameValueEncoding[i] = URLEncoder.encode(sb.toString()); + queryNameValueEncoding[i] = URLEncoder.encode(String.valueOf((char) i)); } /* @@ -159,9 +156,7 @@ public class Encode queryStringEncoding[i] = "%20"; continue; } - StringBuffer sb = new StringBuffer(); - sb.append((char) i); - queryStringEncoding[i] = URLEncoder.encode(sb.toString()); + queryStringEncoding[i] = URLEncoder.encode(String.valueOf((char) i)); } } @@ -194,7 +189,7 @@ public class Encode */ public static String encodeFragment(String value) { - return encodeValue(value, queryNameValueEncoding); + return encodeValue(value, queryStringEncoding); } /** @@ -221,18 +216,19 @@ public class Encode public static String decodePath(String path) { Matcher matcher = encodedCharsMulti.matcher(path); - StringBuffer buf = new StringBuffer(); + int start=0; + StringBuilder builder = new StringBuilder(); CharsetDecoder decoder = Charset.forName(UTF_8).newDecoder(); while (matcher.find()) { + builder.append(path, start, matcher.start()); decoder.reset(); String decoded = decodeBytes(matcher.group(1), decoder); - decoded = decoded.replace("\\", "\\\\"); - decoded = decoded.replace("$", "\\$"); - matcher.appendReplacement(buf, decoded); + builder.append(decoded); + start = matcher.end(); } - matcher.appendTail(buf); - return buf.toString(); + builder.append(path, start, path.length()); + return builder.toString(); } private static String decodeBytes(String enc, CharsetDecoder decoder) @@ -264,7 +260,7 @@ public class Encode public static String encodeNonCodes(String string) { Matcher matcher = nonCodes.matcher(string); - StringBuffer buf = new StringBuffer(); + StringBuilder builder = new StringBuilder(); // FYI: we do not use the no-arg matcher.find() @@ -276,29 +272,32 @@ public class Encode while (matcher.find(idx)) { int start = matcher.start(); - buf.append(string.substring(idx, start)); - buf.append("%25"); + builder.append(string.substring(idx, start)); + builder.append("%25"); idx = start + 1; } - buf.append(string.substring(idx)); - return buf.toString(); + builder.append(string.substring(idx)); + return builder.toString(); } - private static boolean savePathParams(String segment, StringBuffer newSegment, List params) + public static boolean savePathParams(String segment, StringBuilder newSegment, List params) { boolean foundParam = false; // Regular expressions can have '{' and '}' characters. Replace them to do match segment = PathHelper.replaceEnclosedCurlyBraces(segment); Matcher matcher = PathHelper.URI_TEMPLATE_PATTERN.matcher(segment); + int start = 0; while (matcher.find()) { + newSegment.append(segment, start, matcher.start()); foundParam = true; String group = matcher.group(); // Regular expressions can have '{' and '}' characters. Recover earlier replacement params.add(PathHelper.recoverEnclosedCurlyBraces(group)); - matcher.appendReplacement(newSegment, "_resteasy_uri_parameter"); + newSegment.append("_resteasy_uri_parameter"); + start = matcher.end(); } - matcher.appendTail(newSegment); + newSegment.append(segment, start, segment.length()); return foundParam; } @@ -309,11 +308,11 @@ public class Encode * @param encoding * @return */ - private static String encodeValue(String segment, String[] encoding) + public static String encodeValue(String segment, String[] encoding) { ArrayList params = new ArrayList(); boolean foundParam = false; - StringBuffer newSegment = new StringBuffer(); + StringBuilder newSegment = new StringBuilder(); if (savePathParams(segment, newSegment, params)) { foundParam = true; @@ -411,21 +410,21 @@ public class Encode return encodeFromArray(nameOrValue, queryNameValueEncoding, true); } - private static String encodeFromArray(String segment, String[] encodingMap, boolean encodePercent) + protected static String encodeFromArray(String segment, String[] encodingMap, boolean encodePercent) { - StringBuffer result = new StringBuffer(); + StringBuilder result = new StringBuilder(); for (int i = 0; i < segment.length(); i++) { - if (!encodePercent && segment.charAt(i) == '%') + char currentChar = segment.charAt(i); + if (!encodePercent && currentChar == '%') { - result.append(segment.charAt(i)); + result.append(currentChar); continue; } - int idx = segment.charAt(i); - String encoding = encode(idx, encodingMap); + String encoding = encode(currentChar, encodingMap); if (encoding == null) { - result.append(segment.charAt(i)); + result.append(currentChar); } else { @@ -461,20 +460,20 @@ public class Encode return encoded; } - private static String pathParamReplacement(String segment, List params) + public static String pathParamReplacement(String segment, List params) { - StringBuffer newSegment = new StringBuffer(); + StringBuilder newSegment = new StringBuilder(); Matcher matcher = PARAM_REPLACEMENT.matcher(segment); int i = 0; + int start = 0; while (matcher.find()) { + newSegment.append(segment, start, matcher.start()); String replacement = params.get(i++); - // double encode slashes, so that slashes stay where they are - replacement = replacement.replace("\\", "\\\\"); - replacement = replacement.replace("$", "\\$"); - matcher.appendReplacement(newSegment, replacement); + newSegment.append(replacement); + start = matcher.end(); } - matcher.appendTail(newSegment); + newSegment.append(segment, start, segment.length()); segment = newSegment.toString(); return segment; } @@ -505,6 +504,38 @@ public class Encode } return decoded; } + + /** + * decode an encoded map + * + * @param map + * @param charset + * @return + */ + public static MultivaluedHashMap decode(MultivaluedHashMap map, String charset) + { + if (charset == null) + { + charset = UTF_8; + } + MultivaluedHashMap decoded = new MultivaluedHashMap(); + for (Map.Entry> entry : map.entrySet()) + { + List values = entry.getValue(); + for (String value : values) + { + try + { + decoded.add(URLDecoder.decode(entry.getKey(), charset), URLDecoder.decode(value, charset)); + } + catch (UnsupportedEncodingException e) + { + throw new RuntimeException(e); + } + } + } + return decoded; + } public static MultivaluedHashMap encode(MultivaluedHashMap map) { diff --git a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java index f064163cd2..a03c53cbe8 100755 --- a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java +++ b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java @@ -614,7 +614,7 @@ public class KeycloakUriBuilder { if (value == null) throw new IllegalArgumentException("A passed in value was null"); if (query == null) query = ""; else query += "&"; - query += Encode.encodeQueryParam(name) + "=" + Encode.encodeQueryParam(value.toString()); + query += Encode.encodeQueryParamAsIs(name) + "=" + Encode.encodeQueryParamAsIs(value.toString()); } return this; } diff --git a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java index 86b3ecb21e..be74b74de2 100755 --- a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java +++ b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java @@ -345,7 +345,7 @@ public class BaseSAML2BindingBuilder { logger.debugv("saml document: {0}", documentAsString); byte[] responseBytes = documentAsString.getBytes(GeneralConstants.SAML_CHARSET); - return RedirectBindingUtil.deflateBase64URLEncode(responseBytes); + return RedirectBindingUtil.deflateBase64Encode(responseBytes); } @@ -370,7 +370,7 @@ public class BaseSAML2BindingBuilder { } catch (InvalidKeyException | SignatureException e) { throw new ProcessingException(e); } - String encodedSig = RedirectBindingUtil.base64URLEncode(sig); + String encodedSig = RedirectBindingUtil.base64Encode(sig); builder.queryParam(GeneralConstants.SAML_SIGNATURE_REQUEST_KEY, encodedSig); } return builder.build(); diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/web/util/RedirectBindingUtil.java b/saml-core/src/main/java/org/keycloak/saml/processing/web/util/RedirectBindingUtil.java index 587113c5b5..9c0938fc52 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/web/util/RedirectBindingUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/web/util/RedirectBindingUtil.java @@ -60,6 +60,19 @@ public class RedirectBindingUtil { return URLDecoder.decode(str, GeneralConstants.SAML_CHARSET_NAME); } + /** + * On the byte array, apply base64 encoding + * + * @param stringToEncode + * + * @return + * + * @throws IOException + */ + public static String base64Encode(byte[] stringToEncode) throws IOException { + return Base64.encodeBytes(stringToEncode, Base64.DONT_BREAK_LINES); + } + /** * On the byte array, apply base64 encoding following by URL encoding * diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java index f2fdd0c322..9027ed5367 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java @@ -91,7 +91,7 @@ public abstract class OIDCRedirectUriBuilder { @Override public OIDCRedirectUriBuilder addParam(String paramName, String paramValue) { - String param = paramName + "=" + Encode.encodeQueryParam(paramValue); + String param = paramName + "=" + Encode.encodeQueryParamAsIs(paramValue); if (fragment == null) { fragment = new StringBuilder(param); } else { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriStateTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriStateTest.java new file mode 100644 index 0000000000..db410d5e71 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriStateTest.java @@ -0,0 +1,68 @@ +/* + * Copyright 2016 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.oauth; + +import java.net.MalformedURLException; +import java.net.URL; + +import org.junit.Before; +import org.junit.Test; +import org.keycloak.protocol.oidc.utils.OIDCResponseType; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; +import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.util.OAuthClient; + +public class OAuthRedirectUriStateTest extends AbstractTestRealmKeycloakTest { + + @Override + public void configureTestRealm(RealmRepresentation testRealm) { + } + + @Before + public void clientConfiguration() { + oauth.clientId("test-app"); + oauth.responseType(OIDCResponseType.CODE); + oauth.stateParamRandom(); + } + + void assertStateReflected(String state) { + oauth.stateParamHardcoded(state); + + OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password"); + Assert.assertNotNull(response.getCode()); + + URL url; + try { + url = new URL(driver.getCurrentUrl()); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + Assert.assertTrue(url.getQuery().contains("state=" + state)); + } + + @Test + public void testSimpleStateParameter() { + assertStateReflected("VeryLittleGravitasIndeed"); + } + + @Test + public void testJsonStateParameter() { + assertStateReflected("%7B%22csrf_token%22%3A%2B%22hlvZNIsWyqdkEhbjlQIia0ty2YY4TXat%22%2C%2B%22destination%22%3A%2B%22eyJhbGciOiJIUzI1NiJ9.Imh0dHA6Ly9sb2NhbGhvc3Q6NTAwMC9wcml2YXRlIg.T18WeIV29komDl8jav-3bSnUZDlMD8VOfIrd2ikP5zE%22%7D"); + } +}