From 2346964c6cb1f8cdb0c2839e117b2d9c776158f2 Mon Sep 17 00:00:00 2001 From: Avzel <65988622+Avzel@users.noreply.github.com> Date: Sat, 2 Sep 2023 15:58:45 -0400 Subject: [PATCH 1/2] #585 Prevent decoding of parameter values in the ParametersNameDecodingHandler Interceptor & write corresponding test case --- .../ParametersNameDecodingHandler.java | 40 ++++++++++++--- .../ParametersNameDecodingHandlerTest.java | 50 +++++++++++++++++++ 2 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 components/http/okHttp/src/test/java/com/microsoft/kiota/http/ParametersNameDecodingHandlerTest.java diff --git a/components/http/okHttp/src/main/java/com/microsoft/kiota/http/middleware/ParametersNameDecodingHandler.java b/components/http/okHttp/src/main/java/com/microsoft/kiota/http/middleware/ParametersNameDecodingHandler.java index 5c78b16be..d4da6b956 100644 --- a/components/http/okHttp/src/main/java/com/microsoft/kiota/http/middleware/ParametersNameDecodingHandler.java +++ b/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> toDecode = new ArrayList>(); + 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(nameAndValue.length > 1 ? nameAndValue[1] : "", nameAndValue[0])); + } + final ArrayList> symbolsToReplace = new ArrayList>(charactersToDecode.length); for (final char charToReplace : charactersToDecode) { - symbolsToReplace.add(new SimpleEntry("%" + String.format("%x", (int)charToReplace), String.valueOf(charToReplace))); + symbolsToReplace.add(new SimpleEntry("%" + String.format("%x", (int) charToReplace), String.valueOf(charToReplace))); } + for (final Entry symbolToReplace : symbolsToReplace) { - decoded = decoded.replace(symbolToReplace.getKey(), symbolToReplace.getValue()); + for (final Entry 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("&")); } - } diff --git a/components/http/okHttp/src/test/java/com/microsoft/kiota/http/ParametersNameDecodingHandlerTest.java b/components/http/okHttp/src/test/java/com/microsoft/kiota/http/ParametersNameDecodingHandlerTest.java new file mode 100644 index 000000000..20dd993fa --- /dev/null +++ b/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 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()); + } +} From 99efc9587764e4b54238bfd09c76c564746cdedf Mon Sep 17 00:00:00 2001 From: Avzel <65988622+Avzel@users.noreply.github.com> Date: Mon, 4 Sep 2023 10:26:13 -0400 Subject: [PATCH 2/2] Bump patch version & add changelog entry --- CHANGELOG.md | 6 ++++++ gradle.properties | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8084cf309..5ca99f607 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/gradle.properties b/gradle.properties index 54cd0a615..ae3d83dc2 100644 --- a/gradle.properties +++ b/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