Merge pull request #4208 from ASzc/KEYCLOAK-4758

KEYCLOAK-4758

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){

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();
+         pathEncoding[i] = URLEncoder.encode(String.valueOf((char) i));
-         sb.append((char) i);
-         pathEncoding[i] = URLEncoder.encode(sb.toString());
       }
       pathEncoding[' '] = "%20";
       System.arraycopy(pathEncoding, 0, matrixParameterEncoding, 0, pathEncoding.length);
@@ -119,9 +118,7 @@ public class Encode
                queryNameValueEncoding[i] = "+";
                continue;
          }
-         StringBuffer sb = new StringBuffer();
+         queryNameValueEncoding[i] = URLEncoder.encode(String.valueOf((char) i));
-         sb.append((char) i);
-         queryNameValueEncoding[i] = URLEncoder.encode(sb.toString());
       }
 
       /*
@@ -159,9 +156,7 @@ public class Encode
                queryStringEncoding[i] = "%20";
                continue;
          }
-         StringBuffer sb = new StringBuffer();
+         queryStringEncoding[i] = URLEncoder.encode(String.valueOf((char) i));
-         sb.append((char) i);
-         queryStringEncoding[i] = URLEncoder.encode(sb.toString());
       }
    }
 
@@ -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("\\", "\\\\");
+         builder.append(decoded);
-         decoded = decoded.replace("$", "\\$");
+         start = matcher.end();
-         matcher.appendReplacement(buf, decoded);
       }
-      matcher.appendTail(buf);
+      builder.append(path, start, path.length());
-      return buf.toString();
+      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));
+         builder.append(string.substring(idx, start));
-         buf.append("%25");
+         builder.append("%25");
          idx = start + 1;
       }
-      buf.append(string.substring(idx));
+      builder.append(string.substring(idx));
-      return buf.toString();
+      return builder.toString();
    }
 
-   private static boolean savePathParams(String segment, StringBuffer newSegment, List<String> params)
+   public static boolean savePathParams(String segment, StringBuilder newSegment, List<String> 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<String> params = new ArrayList<String>();
       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(currentChar, encodingMap);
-         String encoding = encode(idx, 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<String> params)
+   public static String pathParamReplacement(String segment, List<String> 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 
+     	 newSegment.append(replacement);
-         replacement = replacement.replace("\\", "\\\\");
+		 start = matcher.end();
-         replacement = replacement.replace("$", "\\$");
-         matcher.appendReplacement(newSegment, replacement);
       }
-      matcher.appendTail(newSegment);
+  	  newSegment.append(segment, start, segment.length());
       segment = newSegment.toString();
       return segment;
    }
@@ -506,6 +505,38 @@ public class Encode
       return decoded;
    }
    
+   /**
+    * decode an encoded map
+    *
+    * @param map
+    * @param charset
+    * @return
+    */
+   public static MultivaluedHashMap<String, String> decode(MultivaluedHashMap<String, String> map, String charset)
+   {
+      if (charset == null)
+      {
+         charset = UTF_8;
+      }
+      MultivaluedHashMap<String, String> decoded = new MultivaluedHashMap<String, String>();
+      for (Map.Entry<String, List<String>> entry : map.entrySet())
+      {
+         List<String> 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<String, String> encode(MultivaluedHashMap<String, String> map)
    {
        MultivaluedHashMap<String, String> decoded = new MultivaluedHashMap<String, String>();

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;
     }

saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java

@@ -345,7 +345,7 @@ public class BaseSAML2BindingBuilder<T extends 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<T extends 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();

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
      *

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 {

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");
+    }
+}