#585 Prevent decoding of parameter values in the ParametersNameDecodingHandler Interceptor & write corresponding test case

CHANGELOG.md

@@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+## [0.7.3] - 2023-09-04
+
+### Fixed
+
+- Fixed bug that caused the ParametersNameDecodingHandler to decode query parameter values in addition to names
+
 ## [0.7.2] - 2023-09-01
 
 ### Changed

components/http/okHttp/src/main/java/com/microsoft/kiota/http/middleware/ParametersNameDecodingHandler.java

@@ -5,6 +5,8 @@
 import java.util.Objects;
 import java.util.AbstractMap.SimpleEntry;
 import java.util.Map.Entry;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
@@ -61,7 +63,8 @@ public Response intercept(final Chain chain) throws IOException {
                 nameOption.parametersToDecode.length == 0) {
                     return chain.proceed(request);
                 }
-            String query = originalUri.query();
+            // Must use .encodedQuery() because .query() method decodes both parameter values and names
+            String query = originalUri.encodedQuery();
             if (query == null || query.isEmpty()) {
                 return chain.proceed(request);
             }
@@ -71,7 +74,8 @@ public Response intercept(final Chain chain) throws IOException {
             if (span != null) {
                 builder.tag(Span.class, span);
             }
-            final HttpUrl newUrl = originalUri.newBuilder().query(query).build();
+            // Must use .encodedQuery() because .query() method decodes both parameter values and names
+            final HttpUrl newUrl = originalUri.newBuilder().encodedQuery(query).build();
             return chain.proceed(builder.url(newUrl).build());
         } finally {
             if (scope != null) {
@@ -91,15 +95,37 @@ public Response intercept(final Chain chain) throws IOException {
     @Nonnull
     public static String decodeQueryParameters(@Nullable final String original, @Nonnull final char[] charactersToDecode) {
         Objects.requireNonNull(charactersToDecode);
-        String decoded = original == null ? "" : new StringBuffer(original).toString();
+
+        if (original == null || original.isBlank() || charactersToDecode.length == 0) {
+            return "";
+        }
+
+        String[] encodedQueryParameters = 
+                (original.startsWith("?") ? original.substring(1) : original)
+                .split("&");
+
+        final ArrayList<SimpleEntry<String, String>> toDecode = new ArrayList<SimpleEntry<String, String>>();
+        for (final String encodedQueryParameter : encodedQueryParameters) {
+            String[] nameAndValue = encodedQueryParameter.split("=", 2);
+            // Use query parameter value as simple entry key and query parameter name as simple entry value to allow
+            // for in-place updating of query parameter name during iteration and prevent the need to add or remove keys.
+            // Note: query parameter values may not be unique, so a LinkedHashMap or equivalent would not be appropriate
+            toDecode.add(new SimpleEntry<String, String>(nameAndValue.length > 1 ? nameAndValue[1] : "", nameAndValue[0]));
+        }
+
         final ArrayList<SimpleEntry<String, String>> symbolsToReplace = new ArrayList<SimpleEntry<String, String>>(charactersToDecode.length);
         for (final char charToReplace : charactersToDecode) {
-            symbolsToReplace.add(new SimpleEntry<String,String>("%" + String.format("%x", (int)charToReplace), String.valueOf(charToReplace)));
+            symbolsToReplace.add(new SimpleEntry<String, String>("%" + String.format("%x", (int) charToReplace), String.valueOf(charToReplace)));
         }
+
         for (final Entry<String, String> symbolToReplace : symbolsToReplace) {
-            decoded = decoded.replace(symbolToReplace.getKey(), symbolToReplace.getValue());
+            for (final Entry<String, String> queryParameter : toDecode) {
+                queryParameter.setValue(queryParameter.getValue().replaceAll("(?i)"+Pattern.quote(symbolToReplace.getKey()), symbolToReplace.getValue()));
+            }
         }
-        return decoded;
+
+        return toDecode.stream()
+                .map(tuple -> tuple.getKey().isBlank() ? tuple.getValue() : tuple.getValue() + "=" + tuple.getKey())
+                .collect(Collectors.joining("&"));
     }
-
 }

components/http/okHttp/src/test/java/com/microsoft/kiota/http/ParametersNameDecodingHandlerTest.java

@@ -0,0 +1,50 @@
+package com.microsoft.kiota.http;
+
+import com.microsoft.kiota.http.middleware.ParametersNameDecodingHandler;
+import com.microsoft.kiota.http.middleware.options.ParametersNameDecodingOption;
+
+import okhttp3.Interceptor;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+import okhttp3.Response;
+
+import java.io.IOException;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+public class ParametersNameDecodingHandlerTest {
+
+    private static Stream<Arguments> originalAndExpectedUrls() {
+        return Stream.of(
+                Arguments.of("https://www.google.com/", "https://www.google.com/"),
+                Arguments.of("https://www.google.com/?q=1%2B2", "https://www.google.com/?q=1%2B2"),
+                Arguments.of("https://www.google.com/?q=M%26A", "https://www.google.com/?q=M%26A"),
+                Arguments.of("https://www.google.com/?q%2D1=M%26A", "https://www.google.com/?q-1=M%26A"),
+                Arguments.of("https://www.google.com/?q%2D1&q=M%26A=M%26A", "https://www.google.com/?q-1&q=M%26A=M%26A")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("originalAndExpectedUrls")
+    public void defaultParameterNameDecodingHandlerOnlyDecodesNamesNotValues(String original, String expectedResult) throws IOException {
+        Interceptor[] interceptors = new Interceptor[] {
+                new ParametersNameDecodingHandler( new ParametersNameDecodingOption() {
+                    {
+                        parametersToDecode = new char[] { '$', '.', '-', '~', '+', '&' };
+                    }
+                })
+        };
+        final OkHttpClient client = KiotaClientFactory.create(interceptors).build();
+        final Request request = new Request.Builder().url(original).build();
+        final Response response = client.newCall(request).execute();
+
+        assertNotNull(response);
+        assertEquals(expectedResult, response.request().url().toString());
+    }
+}

gradle.properties

@@ -26,7 +26,7 @@ org.gradle.caching=true
 mavenGroupId         = com.microsoft.kiota
 mavenMajorVersion = 0
 mavenMinorVersion = 7
-mavenPatchVersion = 2
+mavenPatchVersion = 3
 mavenArtifactSuffix = 
 
 #These values are used to run functional tests