Repository: eclipse-vertx/vert.x

Problem statement:
HttpClientImple DEFAULT_HANDLER uses uri.getQuery instead of getRawQuery when extract redirection uri from response location header   
Hi,

I found that Vertx HttpClient DEFAULT_HANDLER uses java.net.URI.getQuery instead of getRawQuery when extracts a redirection uri from response location header.

Even Vertx HttpUitls.resolveURIReference uses java.net.URI.getQuery and then redirects it with decoded query.

java.net.URI.getQuery returns decoded query according to documents.
```
String java.net.URI.getQuery()

Returns the decoded query component of this URI.

The string returned by this method is equal to that returned by the getRawQuery method except that all sequences of escaped octets are decoded.
```

In my case, there are many cases that location header value includes URIEncoded query because my service covers not only english alphabet but also ascii unsafe language. 

Redirection target server still needs URIEncoded query for ascii unsafe language.

I've made a patch for that referring to Apache httpClient where uses getRawQuery and then applied it with HttpClient.setRedirectinHandler.

I guess this issue not only for mine.
I think java.net.URI.getRawQuery is proper rather than getQuery.

Do you have any reason for using getQuery I'm not aware of?




  


Hints:
I don't think there is a specific reason to getQuery. If you provide a
patch with a test which fails before the fix and passes after, it would
most certainly be merged.

2018-01-08 4:56 GMT+01:00 Jaehee Ahn <notifications@github.com>:

> Hi,
>
> I found that Vertx HttpClient DEFAULT_HANDLER uses java.net.URI.getQuery
> instead of getRawQuery when extracts a redirection uri from response
> location header.
>
> Even Vertx HttpUitls.resolveURIReference uses java.net.URI.getQuery and
> then redirects it with decoded query.
>
> java.net.URI.getQuery returns decoded query according to documents.
>
> String java.net.URI.getQuery()
>
> Returns the decoded query component of this URI.
>
> The string returned by this method is equal to that returned by the getRawQuery method except that all sequences of escaped octets are decoded.
>
> In my case, there are many cases that location header value includes
> URIEncoded query because my service covers not only english alphabet but
> also ascii unsafe language.
>
> Redirection target server still needs URIEncoded query for ascii unsafe
> language.
>
> I've made a patch for that referring to Apache httpClient where uses
> getRawQuery and then applied it with HttpClient.setRedirectinHandler.
>
> I guess this issue not only for mine.
> I think java.net.URI.getRawQuery is proper rather than getQuery.
>
> or Do you have any reason for using getQuery?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/eclipse/vert.x/issues/2263>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABbltvsUtqaJLc05jalK8P6G5yQqqozyks5tIZHlgaJpZM4RV7Hx>
> .
>

can you give an example where this is raising an issue ?
This is a sample code for raising this issue.
You can just run main and then the console output tells what happens in each redirection handler, one is DEFAULT_HANDLER and the other is my patch version handler.

You just change  QUERY_WITH_ASCII_UNSAFE variable assignment code between 2 cases
CASE 1. query is "한글", korean languae
CASE 2. query is "A B+C", which has '+'

The console output tells the results about each case
```
DEFAULT_HANDLER
ORIGINAL TEXT [한글] TARGET SERVER RECEIVED TEXT [￭ﾕﾜ￪ﾸﾀ]
ORIGINAL TEXT [A B+C] TARGET SERVER RECEIVED TEXT [A B C]

PATCH_HANDLER
ORIGINAL TEXT [한글] TARGET SERVER RECEIVED TEXT [한글]
ORIGINAL TEXT [A B+C] TARGET SERVER RECEIVED TEXT [A B+C]
```

```
#DELETE CODE 
```
I'm sorry for too much long comment. if it's not proper, please let me know. I will attach the file instead.

--------------------- UPDATED
I just attached a small reproducer to the new comment
https://github.com/eclipse/vert.x/issues/2263#issuecomment-356842787  
  
can you jsut provide a small reproducer of the issue ?
After below the  server  started, call
```java
 vertx.createHttpClient().getAbs("http://localhost:7002/first/call/from/client", resp->{}).setFollowRedirects(true).end();
```
and then check what the target receives for query param.

### server
```java
		Router router = Router.router(vertx);
		
		//------------------------------------------
		// path for sending response with location to client
		//------------------------------------------
		router.route("/first/call/from/client").handler(rc ->{
			
			rc.request().bodyHandler(buf -> {
				
				String encoded1 = "한글";
				String encoded2 = "A B+C";
				
				try {
					encoded1 = URLEncoder.encode(encoded1, "UTF-8");
					encoded2 = URLEncoder.encode(encoded2, "UTF-8");
				} catch (UnsupportedEncodingException e) {
					e.printStackTrace();
				}
				
				rc.response()
					.putHeader("location", "htttp://localhost:7002/redirected/from/client?encoded1=" + encoded1 + "&encoded2=" + encoded2)
					.setStatusCode(302)
					.end();
			});
			
		});
		
		//------------------------------------------
		// path for redirection target
		//------------------------------------------
		router.route("/redirected/from/client").handler(rc ->{
			
			String encoded1 = rc.request().params().get("encoded1");
			String encoded2 = rc.request().params().get("encoded2");
			Buffer body = Buffer.buffer();
			
			// what the redirection target have received compared with original query of location header value
			body.appendString("TARGET SERVER RECEIVED TEXT [" + encoded1 + "] [" + encoded2 + "]");
			
			rc.response()
				.putHeader("Content-Length", String.valueOf(body.length()))
				.write(body.toString())
				.end();
			
		});
		
		
		//------------------------------------------
		// start server with port 7002
		//------------------------------------------
		
		Future<Void> started = Future.<Void>future();
		
		vertx.createHttpServer()
			.requestHandler(router::accept)
			.listen(7002);
```

Patch:
diff --git a/src/main/java/io/vertx/core/http/impl/HttpClientImpl.java b/src/main/java/io/vertx/core/http/impl/HttpClientImpl.java
--- a/src/main/java/io/vertx/core/http/impl/HttpClientImpl.java
+++ b/src/main/java/io/vertx/core/http/impl/HttpClientImpl.java
@@ -11,8 +11,23 @@
 
 package io.vertx.core.http.impl;
 
-import io.vertx.core.*;
-import io.vertx.core.http.*;
+import io.vertx.core.Closeable;
+import io.vertx.core.Context;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.MultiMap;
+import io.vertx.core.VertxException;
+import io.vertx.core.http.HttpClient;
+import io.vertx.core.http.HttpClientOptions;
+import io.vertx.core.http.HttpClientRequest;
+import io.vertx.core.http.HttpClientResponse;
+import io.vertx.core.http.HttpConnection;
+import io.vertx.core.http.HttpHeaders;
+import io.vertx.core.http.HttpMethod;
+import io.vertx.core.http.HttpVersion;
+import io.vertx.core.http.RequestOptions;
+import io.vertx.core.http.WebSocket;
+import io.vertx.core.http.WebsocketVersion;
 import io.vertx.core.impl.ContextImpl;
 import io.vertx.core.impl.ContextInternal;
 import io.vertx.core.impl.VertxInternal;
@@ -75,8 +90,9 @@ public class HttpClientImpl implements HttpClient, MetricsProvider {
           return null;
         }
         String requestURI = uri.getPath();
-        if (uri.getQuery() != null) {
-          requestURI += "?" + uri.getQuery();
+        String query = uri.getQuery();
+        if (query != null) {
+          requestURI += "?" + query;
         }
         return Future.succeededFuture(createRequest(m, uri.getHost(), port, ssl, requestURI, null));
       }
diff --git a/src/main/java/io/vertx/core/http/impl/HttpUtils.java b/src/main/java/io/vertx/core/http/impl/HttpUtils.java
--- a/src/main/java/io/vertx/core/http/impl/HttpUtils.java
+++ b/src/main/java/io/vertx/core/http/impl/HttpUtils.java
@@ -34,12 +34,7 @@
 import java.util.List;
 import java.util.Map;
 
-import static io.vertx.core.http.Http2Settings.DEFAULT_ENABLE_PUSH;
-import static io.vertx.core.http.Http2Settings.DEFAULT_HEADER_TABLE_SIZE;
-import static io.vertx.core.http.Http2Settings.DEFAULT_INITIAL_WINDOW_SIZE;
-import static io.vertx.core.http.Http2Settings.DEFAULT_MAX_CONCURRENT_STREAMS;
-import static io.vertx.core.http.Http2Settings.DEFAULT_MAX_FRAME_SIZE;
-import static io.vertx.core.http.Http2Settings.DEFAULT_MAX_HEADER_LIST_SIZE;
+import static io.vertx.core.http.Http2Settings.*;
 
 /**
  * Various http utils.
@@ -175,19 +170,19 @@ public static URI resolveURIReference(URI base, String ref) throws URISyntaxExce
       scheme = _ref.getScheme();
       authority = _ref.getAuthority();
       path = removeDots(_ref.getPath());
-      query = _ref.getQuery();
+      query = _ref.getRawQuery();
     } else {
       if (_ref.getAuthority() != null) {
         authority = _ref.getAuthority();
         path = _ref.getPath();
-        query = _ref.getQuery();
+        query = _ref.getRawQuery();
       } else {
         if (_ref.getPath().length() == 0) {
           path = base.getPath();
-          if (_ref.getQuery() != null) {
-            query = _ref.getQuery();
+          if (_ref.getRawQuery() != null) {
+            query = _ref.getRawQuery();
           } else {
-            query = base.getQuery();
+            query = base.getRawQuery();
           }
         } else {
           if (_ref.getPath().startsWith("/")) {
@@ -208,7 +203,7 @@ public static URI resolveURIReference(URI base, String ref) throws URISyntaxExce
             }
             path = removeDots(mergedPath);
           }
-          query = _ref.getQuery();
+          query = _ref.getRawQuery();
         }
         authority = base.getAuthority();
       }


Test patch:
diff --git a/src/test/java/io/vertx/test/core/HttpTest.java b/src/test/java/io/vertx/test/core/HttpTest.java
--- a/src/test/java/io/vertx/test/core/HttpTest.java
+++ b/src/test/java/io/vertx/test/core/HttpTest.java
@@ -644,7 +644,7 @@ private void testURIAndPath(String uri, String path) {
 
   @Test
   public void testParamUmlauteDecoding() throws UnsupportedEncodingException {
-    testParamDecoding("äüö");
+    testParamDecoding("\u00e4\u00fc\u00f6");
   }
 
   @Test
@@ -669,7 +669,7 @@ public void testParamNormalDecoding() throws UnsupportedEncodingException {
 
   @Test
   public void testParamAltogetherDecoding() throws UnsupportedEncodingException {
-    testParamDecoding("äüö+% hello");
+    testParamDecoding("\u00e4\u00fc\u00f6+% hello");
   }
 
   private void testParamDecoding(String value) throws UnsupportedEncodingException {
@@ -3813,6 +3813,46 @@ class MockResp implements HttpClientResponse {
     }
   }
 
+  @Test
+  public void testFollowRedirectEncodedParams() throws Exception {
+    String value1 = "\ud55c\uae00", value2 = "A B+C", value3 = "123 \u20ac";
+    server.requestHandler(req -> {
+      switch (req.path()) {
+        case "/first/call/from/client":
+          StringBuilder location = null;
+          try {
+            location = new StringBuilder()
+              .append(req.scheme()).append("://").append(DEFAULT_HTTP_HOST).append(':').append(DEFAULT_HTTP_PORT)
+              .append("/redirected/from/client?")
+              .append("encoded1=").append(URLEncoder.encode(value1, "UTF-8")).append('&')
+              .append("encoded2=").append(URLEncoder.encode(value2, "UTF-8")).append('&')
+              .append("encoded3=").append(URLEncoder.encode(value3, "UTF-8"));
+          } catch (UnsupportedEncodingException e) {
+            fail(e);
+          }
+          req.response()
+            .setStatusCode(302)
+            .putHeader("location", location.toString())
+            .end();
+          break;
+        case "/redirected/from/client":
+          assertEquals(value1, req.params().get("encoded1"));
+          assertEquals(value2, req.params().get("encoded2"));
+          assertEquals(value3, req.params().get("encoded3"));
+          req.response().end();
+          break;
+        default:
+          fail("Unknown path: " + req.path());
+      }
+    });
+    startServer();
+    client.get(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, "/first/call/from/client", resp -> {
+      assertEquals(200, resp.statusCode());
+      testComplete();
+    }).setFollowRedirects(true).end();
+    await();
+  }
+
   @Test
   public void testServerResponseCloseHandlerNotHoldingLock() throws Exception {
     server.requestHandler(req -> {