From cb3d64b05ac0b428452081d32c5520bcc4ce2275 Mon Sep 17 00:00:00 2001 From: CShiru <1848252061@qq.com> Date: Sun, 10 May 2020 00:14:14 +0800 Subject: [PATCH 1/3] add feature --- .../main/java/retrofit2/RequestFactory.java | 28 +++-- .../src/main/java/retrofit2/Retrofit.java | 26 ++++- .../java/retrofit2/StringConverterTest.java | 103 ++++++++++++++++++ 3 files changed, 147 insertions(+), 10 deletions(-) create mode 100644 retrofit/src/test/java/retrofit2/StringConverterTest.java diff --git a/retrofit/src/main/java/retrofit2/RequestFactory.java b/retrofit/src/main/java/retrofit2/RequestFactory.java index bea554efae..ca399f2a1c 100644 --- a/retrofit/src/main/java/retrofit2/RequestFactory.java +++ b/retrofit/src/main/java/retrofit2/RequestFactory.java @@ -438,16 +438,30 @@ private ParameterHandler parseParameterAnnotation( } ParameterizedType parameterizedType = (ParameterizedType) type; Type iterableType = Utils.getParameterUpperBound(0, parameterizedType); - Converter converter = retrofit.stringConverter(iterableType, annotations); - return new ParameterHandler.Query<>(name, converter, encoded).iterable(); + try { + Converter converter = retrofit.stringConverter(iterableType, annotations); + return new ParameterHandler.Query<>(name, converter, encoded).iterable(); + } catch (RuntimeException e) { + // Wide exception range because factories are user code. + throw parameterError(method, e, p, "Unable to create @Query converter for %s", type); + } } else if (rawParameterType.isArray()) { Class arrayComponentType = boxIfPrimitive(rawParameterType.getComponentType()); - Converter converter = - retrofit.stringConverter(arrayComponentType, annotations); - return new ParameterHandler.Query<>(name, converter, encoded).array(); + try { + Converter converter = + retrofit.stringConverter(arrayComponentType, annotations); + return new ParameterHandler.Query<>(name, converter, encoded).array(); + } catch (RuntimeException e) { + throw parameterError(method, e, p, "Unable to create @Query converter for %s", type); + } } else { - Converter converter = retrofit.stringConverter(type, annotations); - return new ParameterHandler.Query<>(name, converter, encoded); + try { + Converter converter = retrofit.stringConverter(type, annotations); + return new ParameterHandler.Query<>(name, converter, encoded); + } catch (RuntimeException e) { + // Wide exception range because factories are user code. + throw parameterError(method, e, p, "Unable to create @Query converter for %s", type); + } } } else if (annotation instanceof QueryName) { diff --git a/retrofit/src/main/java/retrofit2/Retrofit.java b/retrofit/src/main/java/retrofit2/Retrofit.java index a2fc45a691..199487e780 100644 --- a/retrofit/src/main/java/retrofit2/Retrofit.java +++ b/retrofit/src/main/java/retrofit2/Retrofit.java @@ -387,19 +387,39 @@ public Converter nextResponseBodyConverter( /** * Returns a {@link Converter} for {@code type} to {@link String} from the available {@linkplain * #converterFactories() factories}. + * + * @throws IllegalArgumentException if no converter available for {@code type}. */ public Converter stringConverter(Type type, Annotation[] annotations) { Objects.requireNonNull(type, "type == null"); Objects.requireNonNull(annotations, "annotations == null"); - for (int i = 0, count = converterFactories.size(); i < count; i++) { - Converter converter = - converterFactories.get(i).stringConverter(type, annotations, this); + int start = 0; + int n = 0; + for (Converter.Factory converterFactory : converterFactories) { + n++; + Converter converter = converterFactory.stringConverter(type, annotations, this); if (converter != null) { //noinspection unchecked return (Converter) converter; } } + for (Annotation annotation : annotations) { + if (annotation.annotationType().getSimpleName().equals("Query")) { + if (type == String.class || n >= 3) { + // Nothing matched. Resort to default converter which just calls toString(). + //noinspection unchecked + return (Converter) BuiltInConverters.ToStringConverter.INSTANCE; + } + StringBuilder builder = + new StringBuilder("Could not locate String converter for ").append(type).append(".\n"); + builder.append(" Tried:"); + for (int i = start, count = converterFactories.size(); i < count; i++) { + builder.append("\n * ").append(converterFactories.get(i).getClass().getName()); + } + throw new IllegalArgumentException(builder.toString()); + } + } // Nothing matched. Resort to default converter which just calls toString(). //noinspection unchecked diff --git a/retrofit/src/test/java/retrofit2/StringConverterTest.java b/retrofit/src/test/java/retrofit2/StringConverterTest.java new file mode 100644 index 0000000000..19eb4f7e50 --- /dev/null +++ b/retrofit/src/test/java/retrofit2/StringConverterTest.java @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2013 Square, Inc. + * + * 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 retrofit2; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.PARAMETER; +import static java.lang.annotation.RetentionPolicy.RUNTIME; +import static junit.framework.TestCase.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.core.StringContains.containsString; + +import java.lang.annotation.Annotation; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import java.lang.reflect.Type; +import java.util.concurrent.atomic.AtomicReference; +import okhttp3.ResponseBody; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import retrofit2.http.GET; +import retrofit2.http.Query; + +public final class StringConverterTest { + + @Rule public final MockWebServer server = new MockWebServer(); + + @Rule public ExpectedException exception = ExpectedException.none(); + + interface Annotated { + + @GET("/") + Call queryString(@Query("word") String word); + + @GET("/") + Call queryObject(@Foo @Query("foo") Object foo); + + @Target({PARAMETER, METHOD}) + @Retention(RUNTIME) + @interface Foo {} + } + + @Test + public void queryObjectWithStringConverter() { + final AtomicReference annotationsRef = new AtomicReference<>(); + class MyConverterFactory extends Converter.Factory { + @Override + public Converter stringConverter( + Type type, Annotation[] annotations, Retrofit retrofit) { + annotationsRef.set(annotations); + + return (Converter) String::valueOf; + } + } + Retrofit retrofit = + new Retrofit.Builder() + .baseUrl(server.url("/")) + .addConverterFactory(new MyConverterFactory()) + .build(); + Annotated annotated = retrofit.create(Annotated.class); + annotated.queryObject(null); // Trigger internal setup. + + Annotation[] annotations = annotationsRef.get(); + assertThat(annotations).hasAtLeastOneElementOfType(Annotated.Foo.class); + } + + @Test + public void queryObjectWithoutStringConverter() { + exception.expect(IllegalArgumentException.class); + exception.expectMessage( + containsString( + "Unable to create @Query converter for class java.lang.Object (parameter #1)")); + Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); + Annotated annotated = retrofit.create(Annotated.class); + annotated.queryObject(null); // Trigger internal setup. + } + + @Test + public void queryStringWithoutStringConverter() { + Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); + Annotated annotated = retrofit.create(Annotated.class); + try { + annotated.queryString(null); // Trigger internal setup. + assertTrue(true); + } catch (Exception e) { + assertTrue(false); + } + } +} From a61b8f66a3da1c031b2e5f2d7e9dde9c623b3d59 Mon Sep 17 00:00:00 2001 From: CShiru <1848252061@qq.com> Date: Fri, 29 May 2020 22:19:44 +0800 Subject: [PATCH 2/3] test --- retrofit/src/test/java/retrofit2/RetrofitTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/retrofit/src/test/java/retrofit2/RetrofitTest.java b/retrofit/src/test/java/retrofit2/RetrofitTest.java index fd958efa56..61215567c8 100644 --- a/retrofit/src/test/java/retrofit2/RetrofitTest.java +++ b/retrofit/src/test/java/retrofit2/RetrofitTest.java @@ -62,8 +62,10 @@ import retrofit2.http.Query; public final class RetrofitTest { + @Rule public final MockWebServer server = new MockWebServer(); + interface CallMethod { @GET("/") Call disallowed(); From e3ff7a6e444643e70a4fe5095276f107147cdab8 Mon Sep 17 00:00:00 2001 From: CShiru <1848252061@qq.com> Date: Fri, 29 May 2020 22:27:40 +0800 Subject: [PATCH 3/3] fix issue 3080 --- .../main/java/retrofit2/RequestBuilder.java | 11 +++- .../test/java/retrofit2/MalformedUrlTest.java | 56 +++++++++++++++++++ .../src/test/java/retrofit2/RetrofitTest.java | 2 - 3 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 retrofit/src/test/java/retrofit2/MalformedUrlTest.java diff --git a/retrofit/src/main/java/retrofit2/RequestBuilder.java b/retrofit/src/main/java/retrofit2/RequestBuilder.java index d47f61fea5..6e66b52224 100644 --- a/retrofit/src/main/java/retrofit2/RequestBuilder.java +++ b/retrofit/src/main/java/retrofit2/RequestBuilder.java @@ -236,7 +236,16 @@ Request.Builder get() { } else { // No query parameters triggered builder creation, just combine the relative URL and base URL. //noinspection ConstantConditions Non-null if urlBuilder is null. - url = baseUrl.resolve(relativeUrl); + StringBuilder relativeUrlBuilder = new StringBuilder(relativeUrl); + if (relativeUrl.length() > 0) { + if (relativeUrlBuilder.charAt(0) != '/') { + if (!relativeUrl.contains("://")) { + relativeUrlBuilder.insert(0, '/'); + } + } + } + + url = baseUrl.resolve(relativeUrlBuilder.toString()); if (url == null) { throw new IllegalArgumentException( "Malformed URL. Base: " + baseUrl + ", Relative: " + relativeUrl); diff --git a/retrofit/src/test/java/retrofit2/MalformedUrlTest.java b/retrofit/src/test/java/retrofit2/MalformedUrlTest.java new file mode 100644 index 0000000000..33d0d473ec --- /dev/null +++ b/retrofit/src/test/java/retrofit2/MalformedUrlTest.java @@ -0,0 +1,56 @@ +package retrofit2; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import okhttp3.ResponseBody; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.Rule; +import org.junit.Test; +import retrofit2.http.GET; +import retrofit2.http.PUT; +import retrofit2.http.Path; + +public class MalformedUrlTest { + @Rule public final MockWebServer server = new MockWebServer(); + + interface Service { + @PUT("user:email={email}/login") + Call login(@Path(value = "email") String email); + + @GET("./{email}") + Call loginRelative(@Path("email") String email); + } + + @Test + public void malformedUrlTest() throws Exception { + Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); + Service example = retrofit.create(Service.class); + + server.enqueue(new MockResponse().addHeader("Content-Type", "text/plain").setBody("Success")); + + Call callLogin = example.login("username"); + Response responseLogin = callLogin.execute(); + assertEquals("Success", responseLogin.body().string()); + } + + @Test + public void relativePathTest() throws Exception { + Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); + Service example = retrofit.create(Service.class); + + server.enqueue(new MockResponse().addHeader("Content-Type", "text/plain").setBody("Success")); + + Call callLoginRel = example.loginRelative("username"); + try { + Response responseLoginRel = callLoginRel.execute(); + assertEquals("Success", responseLoginRel.body().string()); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e) + .hasMessage("@Path parameters shouldn't perform path traversal ('.' or '..'): username"); + } + } +} diff --git a/retrofit/src/test/java/retrofit2/RetrofitTest.java b/retrofit/src/test/java/retrofit2/RetrofitTest.java index 61215567c8..fd958efa56 100644 --- a/retrofit/src/test/java/retrofit2/RetrofitTest.java +++ b/retrofit/src/test/java/retrofit2/RetrofitTest.java @@ -62,10 +62,8 @@ import retrofit2.http.Query; public final class RetrofitTest { - @Rule public final MockWebServer server = new MockWebServer(); - interface CallMethod { @GET("/") Call disallowed();