From 74ded0302ed523d828cb58501d78e4c3617e07b2 Mon Sep 17 00:00:00 2001 From: Andrei Savu Date: Thu, 9 Feb 2012 16:04:45 +0200 Subject: [PATCH] Fixed double query parameter & URL encoding / decoding bugs --- .../cloudstack/filters/QuerySigner.java | 33 ++++++------ .../ReEncodeQueryWithDefaultURLEncoder.java | 2 +- .../features/SSHKeyPairAsyncClientTest.java | 15 +++--- .../features/SSHKeyPairClientExpectTest.java | 6 +-- ...eEncodeQueryWithJavaNetURLEncoderTest.java | 4 +- .../org/jclouds/http/utils/ModifyRequest.java | 30 +++++------ .../jclouds/http/utils/ModifyRequestTest.java | 51 ++++++++++--------- 7 files changed, 72 insertions(+), 69 deletions(-) diff --git a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java index c03a1e47e7..1e8573f595 100644 --- a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java +++ b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java @@ -18,23 +18,16 @@ */ package org.jclouds.cloudstack.filters; -import static com.google.common.base.Preconditions.checkNotNull; - -import java.util.Map.Entry; - -import javax.annotation.Resource; -import javax.inject.Inject; -import javax.inject.Named; -import javax.inject.Provider; -import javax.inject.Singleton; -import javax.ws.rs.core.UriBuilder; - +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Multimap; import org.jclouds.Constants; import org.jclouds.crypto.Crypto; import org.jclouds.crypto.CryptoStreams; import org.jclouds.http.HttpException; import org.jclouds.http.HttpRequest; -import org.jclouds.http.HttpRequestFilter; import org.jclouds.http.HttpUtils; import org.jclouds.http.internal.SignatureWire; import org.jclouds.http.utils.ModifyRequest; @@ -43,11 +36,15 @@ import org.jclouds.logging.Logger; import org.jclouds.rest.RequestSigner; import org.jclouds.util.Strings2; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSortedSet; -import com.google.common.collect.Multimap; +import javax.annotation.Resource; +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Provider; +import javax.inject.Singleton; +import javax.ws.rs.core.UriBuilder; +import java.util.Map.Entry; + +import static com.google.common.base.Preconditions.checkNotNull; /** * @@ -85,7 +82,7 @@ public class QuerySigner implements AuthenticationFilter, RequestSigner { public HttpRequest filter(HttpRequest request) throws HttpException { checkNotNull(request, "request must be present"); - Multimap decodedParams = ModifyRequest.parseQueryToMap(request.getEndpoint().getQuery()); + Multimap decodedParams = ModifyRequest.parseQueryToMap(request.getEndpoint().getRawQuery()); addSigningParams(decodedParams); String stringToSign = createStringToSign(request, decodedParams); String signature = sign(stringToSign); diff --git a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithDefaultURLEncoder.java b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithDefaultURLEncoder.java index c221c89077..06058535a2 100644 --- a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithDefaultURLEncoder.java +++ b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithDefaultURLEncoder.java @@ -47,7 +47,7 @@ public class ReEncodeQueryWithDefaultURLEncoder implements HttpRequestFilter { public HttpRequest filter(HttpRequest request) throws HttpException { UriBuilder builder = builders.get(); builder.uri(request.getEndpoint()); - Multimap map = ModifyRequest.parseQueryToMap(request.getEndpoint().getQuery()); + Multimap map = ModifyRequest.parseQueryToMap(request.getEndpoint().getRawQuery()); builder.replaceQuery(""); for (String key : map.keySet()) builder.queryParam(key, getOnlyElement(map.get(key))); diff --git a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SSHKeyPairAsyncClientTest.java b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SSHKeyPairAsyncClientTest.java index 6ddc763d5b..c60dcfc94d 100644 --- a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SSHKeyPairAsyncClientTest.java +++ b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SSHKeyPairAsyncClientTest.java @@ -18,12 +18,9 @@ */ package org.jclouds.cloudstack.features; -import java.io.IOException; -import java.lang.reflect.Method; -import java.net.URLEncoder; - import com.google.common.base.Functions; import com.google.inject.TypeLiteral; +import org.jclouds.cloudstack.filters.QuerySigner; import org.jclouds.cloudstack.options.ListSSHKeyPairsOptions; import org.jclouds.crypto.SshKeys; import org.jclouds.functions.IdentityFunction; @@ -37,6 +34,12 @@ import org.jclouds.rest.functions.ReturnVoidOnNotFoundOr404; import org.jclouds.rest.internal.RestAnnotationProcessor; import org.testng.annotations.Test; +import java.io.IOException; +import java.lang.reflect.Method; +import java.net.URLEncoder; + +import static org.testng.Assert.assertEquals; + /** * Tests behavior of {@code SSHKeyPairAsyncClient} * @@ -112,7 +115,8 @@ public class SSHKeyPairAsyncClientTest extends BaseCloudStackAsyncClientTestbuilder() .put("Accept", "application/json") diff --git a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithJavaNetURLEncoderTest.java b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithJavaNetURLEncoderTest.java index 0d1b58d7a4..c4c9f13e43 100644 --- a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithJavaNetURLEncoderTest.java +++ b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/ReEncodeQueryWithJavaNetURLEncoderTest.java @@ -21,6 +21,7 @@ package org.jclouds.cloudstack.filters; import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.sun.jersey.api.uri.UriBuilderImpl; +import com.sun.jersey.api.uri.UriComponent; import org.jclouds.http.HttpRequest; import org.jclouds.util.Strings2; import org.testng.annotations.Test; @@ -58,6 +59,7 @@ public class ReEncodeQueryWithJavaNetURLEncoderTest { } }).getInstance(ReEncodeQueryWithDefaultURLEncoder.class).filter(request); - assertEquals(request.getEndpoint().toASCIIString(), "http://localhost?foo=" + URLEncoder.encode(input)); + assertEquals(request.getEndpoint().toASCIIString(), "http://localhost?foo=" + + UriComponent.encode(input, UriComponent.Type.QUERY_PARAM)); } } diff --git a/core/src/main/java/org/jclouds/http/utils/ModifyRequest.java b/core/src/main/java/org/jclouds/http/utils/ModifyRequest.java index 522e16ef29..7dc82f3da4 100644 --- a/core/src/main/java/org/jclouds/http/utils/ModifyRequest.java +++ b/core/src/main/java/org/jclouds/http/utils/ModifyRequest.java @@ -18,20 +18,6 @@ */ package org.jclouds.http.utils; -import static com.google.common.base.Preconditions.checkNotNull; -import static org.jclouds.io.Payloads.newUrlEncodedFormPayload; - -import java.net.URI; -import java.util.Comparator; -import java.util.Iterator; -import java.util.Map; - -import org.jclouds.javax.annotation.Nullable; -import javax.ws.rs.core.UriBuilder; - -import org.jclouds.http.HttpRequest; -import org.jclouds.util.Strings2; - import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; @@ -39,6 +25,18 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.Multimap; +import org.jclouds.http.HttpRequest; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.util.Strings2; + +import javax.ws.rs.core.UriBuilder; +import java.net.URI; +import java.util.Comparator; +import java.util.Iterator; +import java.util.Map; + +import static com.google.common.base.Preconditions.checkNotNull; +import static org.jclouds.io.Payloads.newUrlEncodedFormPayload; /** * @@ -159,7 +157,7 @@ public class ModifyRequest { else map.put(in, null); } else { - String[] parts = Strings2.urlDecode(in).split("&"); + String[] parts = in.split("&"); for (String part : parts) { parseKeyValueFromStringToMap(part, map); } @@ -172,7 +170,7 @@ public class ModifyRequest { int indexOfFirstEquals = stringToParse.indexOf('='); String key = indexOfFirstEquals == -1 ? stringToParse : stringToParse.substring(0, indexOfFirstEquals); String value = indexOfFirstEquals == -1 ? null : stringToParse.substring(indexOfFirstEquals + 1); - map.put(key, value); + map.put(key, Strings2.urlDecode(value)); } public static String makeQueryLine(Multimap params, diff --git a/core/src/test/java/org/jclouds/http/utils/ModifyRequestTest.java b/core/src/test/java/org/jclouds/http/utils/ModifyRequestTest.java index fa9fd3409d..d38676fe84 100644 --- a/core/src/test/java/org/jclouds/http/utils/ModifyRequestTest.java +++ b/core/src/test/java/org/jclouds/http/utils/ModifyRequestTest.java @@ -46,56 +46,56 @@ public class ModifyRequestTest { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")).build(); assertEquals(ModifyRequest.endpoint(request, URI.create("http://bar")), HttpRequest.builder().method("GET") - .endpoint(URI.create("http://bar")).build()); + .endpoint(URI.create("http://bar")).build()); } public void testReplaceHeader() { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "bar")).build(); + .headers(ImmutableMultimap.of("foo", "bar")).build(); assertEquals( - ModifyRequest.replaceHeader(request, "foo", "baz"), - HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "baz")).build()); + ModifyRequest.replaceHeader(request, "foo", "baz"), + HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) + .headers(ImmutableMultimap.of("foo", "baz")).build()); } public void testRemoveHeader() { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "bar")).build(); + .headers(ImmutableMultimap.of("foo", "bar")).build(); assertEquals(ModifyRequest.removeHeader(request, "foo"), - HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")).build()); + HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")).build()); } public void testReplaceHeaders() { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "bar", "rabbit", "tree")).build(); + .headers(ImmutableMultimap.of("foo", "bar", "rabbit", "tree")).build(); assertEquals( - ModifyRequest.replaceHeaders(request, - ImmutableMultimap.of("foo", "bar", "rabbit", "robot", "robert", "baz")), - HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "bar", "rabbit", "robot", "robert", "baz")).build()); + ModifyRequest.replaceHeaders(request, + ImmutableMultimap.of("foo", "bar", "rabbit", "robot", "robert", "baz")), + HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) + .headers(ImmutableMultimap.of("foo", "bar", "rabbit", "robot", "robert", "baz")).build()); } public void testPutHeadersAddsAnotherValue() { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap.of("foo", "bar")).build(); + .headers(ImmutableMultimap.of("foo", "bar")).build(); assertEquals( - ModifyRequest.putHeaders(request, ImmutableMultimap.of("foo", "baz")), - HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .headers(ImmutableMultimap. builder().put("foo", "bar").put("foo", "baz").build()) - .build()); + ModifyRequest.putHeaders(request, ImmutableMultimap.of("foo", "baz")), + HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) + .headers(ImmutableMultimap.builder().put("foo", "bar").put("foo", "baz").build()) + .build()); } public void testPutFormParamsAddsAnotherValue() { HttpRequest request = HttpRequest.builder().method("GET").endpoint(URI.create("http://foo")) - .payload(Payloads.newStringPayload("foo=bar")).build(); + .payload(Payloads.newStringPayload("foo=bar")).build(); Payload payload = Payloads.newStringPayload("foo=bar&foo=baz"); payload.getContentMetadata().setContentType(MediaType.APPLICATION_FORM_URLENCODED); assertEquals(ModifyRequest.putFormParams(request, ImmutableMultimap.of("foo", "baz")), HttpRequest.builder() - .method("GET").endpoint(URI.create("http://foo")).payload(payload).build()); + .method("GET").endpoint(URI.create("http://foo")).payload(payload).build()); } public void testParseBase64InForm() { @@ -106,8 +106,8 @@ public class ModifyRequestTest { expects.put("Value", "dGVzdA=="); expects.put("InstanceId", "1"); assertEquals( - expects, - parseQueryToMap("Version=2010-06-15&Action=ModifyInstanceAttribute&Attribute=userData&Value=dGVzdA%3D%3D&InstanceId=1")); + parseQueryToMap("Version=2010-06-15&Action=ModifyInstanceAttribute&Attribute=userData&Value=dGVzdA%3D%3D&InstanceId=1"), + expects); } @Test @@ -137,10 +137,13 @@ public class ModifyRequestTest { "DOA2IHV9cUfEDBm1Be5TbpadWwSbS/05E+FARH2/MCO932UgcKUq5PGymS0249fLCBPci5zoLiG5vIym+1ij1hL/nHvkK99NIwe7io+Lmp" + "9OcF3PTsm3Rgh5T09cRHGX9horp0VoAVa9vKJx6C1/IEHVnG8p0YPPa1lmemvx5kNBEiyoNQNYa34EiFkcJfP6rqNgvY8h/j4nE9SXoUCC" + "/g6frhMFMOL0tzYqvz0Lczqm1Oh4RnSn3O9X4R934p28qqAobe337hmlLUdb6H5zuf+NwCh0HdZ"; - String queryString = "publickey=" + Strings2.urlEncode(key); - Multimap parsedMap = parseQueryToMap(queryString); - Set expected = ImmutableSet.of(Strings2.urlEncode(key)); + Set expected = ImmutableSet.of(key); + + Multimap parsedMap = parseQueryToMap("a=1&b=1+2&publickey=" + Strings2.urlEncode(key)); + assertEquals(parsedMap.get("publickey"), expected); + + parsedMap = parseQueryToMap("publickey=" + Strings2.urlEncode(key)); assertEquals(parsedMap.get("publickey"), expected); }