http-client-branch: fix issues with redirect including Upgrade header when it should not, Expect containing more than one value, and SECURE redirect policy http-client-branch
authorchegar
Mon, 26 Feb 2018 18:18:49 +0000
branchhttp-client-branch
changeset 56186 261b5b524dd3
parent 56185 f195e5048667
child 56187 61ba287288d9
http-client-branch: fix issues with redirect including Upgrade header when it should not, Expect containing more than one value, and SECURE redirect policy
src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java
src/java.net.http/share/classes/jdk/internal/net/http/HttpRequestImpl.java
src/java.net.http/share/classes/jdk/internal/net/http/RedirectFilter.java
test/jdk/java/net/httpclient/DigestEchoServer.java
test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java
--- a/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java	Mon Feb 26 18:16:46 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java	Mon Feb 26 18:18:49 2018 +0000
@@ -305,6 +305,7 @@
             } else {
                 exchange.serverauth = au;
             }
+            req = HttpRequestImpl.newInstanceForAuthentication(req);
             addBasicCredentials(req, proxy, pw);
             return req;
         } else if (au.retries > retry_limit) {
@@ -330,6 +331,7 @@
             } else {
                 exchange.serverauth = au;
             }
+            req = HttpRequestImpl.newInstanceForAuthentication(req);
             addBasicCredentials(req, proxy, au.credentials);
             au.retries++;
             return req;
--- a/src/java.net.http/share/classes/jdk/internal/net/http/HttpRequestImpl.java	Mon Feb 26 18:16:46 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/HttpRequestImpl.java	Mon Feb 26 18:18:49 2018 +0000
@@ -121,14 +121,30 @@
         this.authority = null;
     }
 
-    /** Creates a HttpRequestImpl using fields of an existing request impl. */
-    public HttpRequestImpl(URI uri,
-                           String method,
-                           HttpRequestImpl other) {
+    /** Returns a new instance suitable for redirection. */
+    public static HttpRequestImpl newInstanceForRedirection(URI uri,
+                                                            String method,
+                                                            HttpRequestImpl other) {
+        return new HttpRequestImpl(uri, method, other);
+    }
+
+    /** Returns a new instance suitable for authentication. */
+    public static HttpRequestImpl newInstanceForAuthentication(HttpRequestImpl other) {
+        return new HttpRequestImpl(other.uri(), other.method(), other);
+    }
+
+    /**
+     * Creates a HttpRequestImpl using fields of an existing request impl.
+     * The newly created HttpRequestImpl does not copy the system headers.
+     */
+    private HttpRequestImpl(URI uri,
+                            String method,
+                            HttpRequestImpl other) {
         this.method = method == null? "GET" : method;
         this.userHeaders = other.userHeaders;
         this.isWebSocket = other.isWebSocket;
-        this.systemHeaders = other.systemHeaders;
+        this.systemHeaders = new HttpHeadersImpl();
+        this.systemHeaders.setHeader("User-Agent", USER_AGENT);
         this.uri = uri;
         this.proxy = other.proxy;
         this.expectContinue = other.expectContinue;
--- a/src/java.net.http/share/classes/jdk/internal/net/http/RedirectFilter.java	Mon Feb 26 18:16:46 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/RedirectFilter.java	Mon Feb 26 18:18:49 2018 +0000
@@ -30,6 +30,7 @@
 import java.net.URI;
 import java.net.http.HttpClient;
 import java.net.http.HttpHeaders;
+import jdk.internal.net.http.common.Log;
 import jdk.internal.net.http.common.Utils;
 
 class RedirectFilter implements HeaderFilter {
@@ -66,7 +67,7 @@
     }
 
     /**
-     * checks to see if new request needed and returns it.
+     * Checks to see if a new request is needed and returns it.
      * Null means response is ok to return to user.
      */
     private HttpRequestImpl handleResponse(Response r) {
@@ -76,11 +77,12 @@
         }
         if (rcode >= 300 && rcode <= 399) {
             URI redir = getRedirectedURI(r.headers());
+            Log.logTrace("response code: {0}, redirected URI: {1}", rcode, redir);
             if (canRedirect(redir) && ++exchange.numberOfRedirects < max_redirects) {
-                //System.out.println("Redirecting to: " + redir);
-                return new HttpRequestImpl(redir, method, request);
+                Log.logTrace("redirecting to: {0}", redir);
+                return HttpRequestImpl.newInstanceForRedirection(redir, method, request);
             } else {
-                //System.out.println("Redirect: giving up");
+                Log.logTrace("not redirecting");
                 return null;
             }
         }
@@ -109,7 +111,8 @@
             case NEVER:
                 return false;
             case SECURE:
-                return newScheme.equalsIgnoreCase("https");
+                return newScheme.equalsIgnoreCase(oldScheme)
+                        || newScheme.equalsIgnoreCase("https");
             case SAME_PROTOCOL:
                 return newScheme.equalsIgnoreCase(oldScheme);
             default:
--- a/test/jdk/java/net/httpclient/DigestEchoServer.java	Mon Feb 26 18:16:46 2018 +0000
+++ b/test/jdk/java/net/httpclient/DigestEchoServer.java	Mon Feb 26 18:18:49 2018 +0000
@@ -742,6 +742,14 @@
                 System.out.println(type + ": Got " + he.getRequestMethod()
                     + ": " + he.getRequestURI()
                     + "\n" + DigestEchoServer.toString(he.getRequestHeaders()));
+
+                // Assert only a single value for Expect. Not directly related
+                // to digest authentication, but verifies good client behaviour.
+                List<String> expectValues = he.getRequestHeaders().get("Expect");
+                if (expectValues != null && expectValues.size() > 1) {
+                    throw new IOException("Expect:  " + expectValues);
+                }
+
                 if (!isAuthentified(he)) {
                     try {
                         requestAuthentication(he);
--- a/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java	Mon Feb 26 18:16:46 2018 +0000
+++ b/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java	Mon Feb 26 18:18:49 2018 +0000
@@ -534,6 +534,18 @@
         }
         boolean endStreamReceived = endStream;
         HttpHeadersImpl headers = decodeHeaders(frames);
+
+        // Strict to assert Client correctness. Not all servers are as strict,
+        // but some are known to be.
+        Optional<?> disallowedHeader = headers.firstValue("Upgrade");
+        if (disallowedHeader.isPresent()) {
+            throw new IOException("Unexpected Upgrade in headers:" + headers);
+        }
+        disallowedHeader = headers.firstValue("HTTP2-Settings");
+        if (disallowedHeader.isPresent())
+            throw new IOException("Unexpected HTTP2-Settings in headers:" + headers);
+
+
         Queue q = new Queue(sentinel);
         streams.put(streamid, q);
         exec.submit(() -> {