Review comment fixes

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
eclipse-ee4j · May 13, 2022 · 2c1a3e3 · 2c1a3e3
1 parent 75e5fce
commit 2c1a3e3
Show file tree

Hide file tree

Showing 8 changed files with 188 additions and 22 deletions.
diff --git a/...tty-connector/src/main/java/org/glassfish/jersey/netty/connector/JerseyClientHandler.java b/...tty-connector/src/main/java/org/glassfish/jersey/netty/connector/JerseyClientHandler.java
@@ -20,15 +20,18 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeoutException;
 
+import javax.ws.rs.core.HttpHeaders;
 import javax.ws.rs.core.Response;
 
 import org.glassfish.jersey.client.ClientProperties;
 import org.glassfish.jersey.client.ClientRequest;
 import org.glassfish.jersey.client.ClientResponse;
 import org.glassfish.jersey.netty.connector.internal.NettyInputStream;
+import org.glassfish.jersey.netty.connector.internal.RedirectException;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
@@ -49,12 +52,15 @@
  */
 class JerseyClientHandler extends SimpleChannelInboundHandler<HttpObject> {
 
-    private static final String LOCATION_HEADER = "Location";
+    private static final int DEFAULT_MAX_REDIRECTS = 5;
 
+    // Modified only by the same thread. No need to synchronize it.
+    private final Set<URI> redirectUriHistory;
     private final ClientRequest jerseyRequest;
     private final CompletableFuture<ClientResponse> responseAvailable;
     private final CompletableFuture<?> responseDone;
     private final boolean followRedirects;
+    private final int maxRedirects;
     private final NettyConnector connector;
 
     private NettyInputStream nis;
@@ -63,12 +69,14 @@ class JerseyClientHandler extends SimpleChannelInboundHandler<HttpObject> {
     private boolean readTimedOut;
 
     JerseyClientHandler(ClientRequest request, CompletableFuture<ClientResponse> responseAvailable,
-                        CompletableFuture<?> responseDone, NettyConnector connector) {
+                        CompletableFuture<?> responseDone, Set<URI> redirectUriHistory, NettyConnector connector) {
+        this.redirectUriHistory = redirectUriHistory;
         this.jerseyRequest = request;
         this.responseAvailable = responseAvailable;
         this.responseDone = responseDone;
         // Follow redirects by default
         this.followRedirects = jerseyRequest.resolveProperty(ClientProperties.FOLLOW_REDIRECTS, true);
+        this.maxRedirects = jerseyRequest.resolveProperty(NettyClientProperties.MAX_REDIRECTS, DEFAULT_MAX_REDIRECTS);
         this.connector = connector;
     }
 
@@ -99,18 +107,30 @@ protected void notifyResponse() {
                           || responseStatus == HttpResponseStatus.SEE_OTHER.code()
                           || responseStatus == HttpResponseStatus.TEMPORARY_REDIRECT.code()
                           || responseStatus == HttpResponseStatus.PERMANENT_REDIRECT.code())) {
-              String location = cr.getHeaderString(LOCATION_HEADER);
-              try {
-                  URI newUri = URI.create(location);
-                  ClientRequest newReq = new ClientRequest(jerseyRequest);
-                  newReq.setUri(newUri);
-                  // Do not complete responseAvailable and try with new URI
-                  // FIXME: This loops forever if HTTP response code is always a redirect.
-                  // Currently there is no client property to specify a limit of redirections.
-                  connector.execute(newReq, responseAvailable);
-              } catch (RuntimeException e) {
-                  // It could happen if location header is wrong
-                  responseAvailable.completeExceptionally(e);
+              String location = cr.getHeaderString(HttpHeaders.LOCATION);
+              if (location == null || location.isEmpty()) {
+                  responseAvailable.completeExceptionally(new RedirectException(LocalizationMessages.REDIRECT_NO_LOCATION()));
+              } else {
+                  try {
+                      URI newUri = URI.create(location);
+                      boolean alreadyRequested = !redirectUriHistory.add(newUri);
+                      if (alreadyRequested) {
+                          // infinite loop detection
+                          responseAvailable.completeExceptionally(
+                                  new RedirectException(LocalizationMessages.REDIRECT_INFINITE_LOOP()));
+                      } else if (redirectUriHistory.size() > maxRedirects) {
+                          // maximal number of redirection
+                          responseAvailable.completeExceptionally(
+                                  new RedirectException(LocalizationMessages.REDIRECT_LIMIT_REACHED(maxRedirects)));
+                      } else {
+                          ClientRequest newReq = new ClientRequest(jerseyRequest);
+                          newReq.setUri(newUri);
+                          connector.execute(newReq, redirectUriHistory, responseAvailable);
+                      }
+                  } catch (IllegalArgumentException e) {
+                      responseAvailable.completeExceptionally(
+                              new RedirectException(LocalizationMessages.REDIRECT_ERROR_DETERMINING_LOCATION(location)));
+                  }
               }
           } else {
               responseAvailable.complete(cr);

diff --git a/...y-connector/src/main/java/org/glassfish/jersey/netty/connector/NettyClientProperties.java b/...y-connector/src/main/java/org/glassfish/jersey/netty/connector/NettyClientProperties.java
@@ -68,4 +68,18 @@ public class NettyClientProperties {
      * @see javax.net.ssl.SSLParameters#setEndpointIdentificationAlgorithm(String)
      */
     public static final String ENABLE_SSL_HOSTNAME_VERIFICATION = "jersey.config.client.tls.enableHostnameVerification";
+
+    /**
+     * The maximal number of redirects during single request.
+     * <p/>
+     * Value is expected to be positive {@link Integer}. Default value is {@value #DEFAULT_MAX_REDIRECTS}.
+     * <p/>
+     * HTTP redirection must be enabled by property {@link org.glassfish.jersey.client.ClientProperties#FOLLOW_REDIRECTS},
+     * otherwise {@code MAX_REDIRECTS} is not applied.
+     *
+     * @since 2.36
+     * @see org.glassfish.jersey.client.ClientProperties#FOLLOW_REDIRECTS
+     * @see org.glassfish.jersey.netty.connector.internal.RedirectException
+     */
+    public static final String MAX_REDIRECTS = "jersey.config.client.NettyConnectorProvider.maxRedirects";
 }
diff --git a/...rs/netty-connector/src/main/java/org/glassfish/jersey/netty/connector/NettyConnector.java b/...rs/netty-connector/src/main/java/org/glassfish/jersey/netty/connector/NettyConnector.java
@@ -22,9 +22,11 @@
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutorService;
@@ -152,7 +154,7 @@ class NettyConnector implements Connector {
     public ClientResponse apply(ClientRequest jerseyRequest) {
         try {
             CompletableFuture<ClientResponse> response = new CompletableFuture<>();
-            execute(jerseyRequest, response);
+            execute(jerseyRequest, new HashSet<>(), response);
             return response.join();
         } catch (CompletionException cex) {
             final Throwable t = cex.getCause() == null ? cex : cex.getCause();
@@ -172,11 +174,11 @@ public Future<?> apply(final ClientRequest jerseyRequest, final AsyncConnectorCa
                 jerseyCallback.failure(th);
             }
         }, executorService);
-        execute(jerseyRequest, response);
+        execute(jerseyRequest, new HashSet<>(), response);
         return response;
     }
 
-    protected void execute(final ClientRequest jerseyRequest,
+    protected void execute(final ClientRequest jerseyRequest, final Set<URI> redirectUriHistory,
             final CompletableFuture<ClientResponse> responseAvailable) {
         Integer timeout = jerseyRequest.resolveProperty(ClientProperties.READ_TIMEOUT, 0);
         if (timeout == null || timeout < 0) {
@@ -298,7 +300,8 @@ protected void initChannel(SocketChannel ch) throws Exception {
             // assert: it is ok to abort the entire response, if responseDone is completed exceptionally - in particular, nothing
             //         will leak
             final Channel ch = chan;
-            JerseyClientHandler clientHandler = new JerseyClientHandler(jerseyRequest, responseAvailable, responseDone, this);
+            JerseyClientHandler clientHandler =
+                    new JerseyClientHandler(jerseyRequest, responseAvailable, responseDone, redirectUriHistory, this);
             // read timeout makes sense really as an inactivity timeout
             ch.pipeline().addLast(READ_TIMEOUT_HANDLER,
                                   new IdleStateHandler(0, 0, timeout, TimeUnit.MILLISECONDS));

diff --git a/...nector/src/main/java/org/glassfish/jersey/netty/connector/internal/RedirectException.java b/...nector/src/main/java/org/glassfish/jersey/netty/connector/internal/RedirectException.java
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2022 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0, which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the
+ * Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
+ * version 2 with the GNU Classpath Exception, which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ */
+
+package org.glassfish.jersey.netty.connector.internal;
+
+import org.glassfish.jersey.client.ClientProperties;
+
+/**
+ * This Exception is used only if {@link ClientProperties#FOLLOW_REDIRECTS} is set to {@code true}.
+ * <p/>
+ * This exception is thrown when any of the Redirect HTTP response status codes (301, 302, 303, 307, 308) is received and:
+ * <ul>
+ * <li>
+ * the chained redirection count exceeds the value of
+ * {@link org.glassfish.jersey.netty.connector.NettyClientProperties#MAX_REDIRECTS}
+ * </li>
+ * <li>
+ * or an infinite redirection loop is detected
+ * </li>
+ * <li>
+ * or Location response header is missing, empty or does not contain a valid {@link java.net.URI}.
+ * </li>
+ * </ul>
+ *
+ */
+public class RedirectException extends Exception {
+
+    private static final long serialVersionUID = 4357724300486801294L;
+
+    /**
+     * Constructor.
+     *
+     * @param message the detail message. The detail message is saved for
+     *                later retrieval by the {@link #getMessage()} method.
+     */
+    public RedirectException(String message) {
+        super(message);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param message the detail message. The detail message is saved for
+     *                later retrieval by the {@link #getMessage()} method.
+     */
+    public RedirectException(String message, Throwable t) {
+        super(message, t);
+    }
+}
diff --git a/...connector/src/main/resources/org/glassfish/jersey/netty/connector/localization.properties b/...connector/src/main/resources/org/glassfish/jersey/netty/connector/localization.properties
@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2016, 2021 Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2016, 2022 Oracle and/or its affiliates. All rights reserved.
 #
 # This program and the accompanying materials are made available under the
 # terms of the Eclipse Public License v. 2.0, which is available at
@@ -19,4 +19,7 @@ wrong.read.timeout=Unexpected ("{0}") READ_TIMEOUT.
 wrong.max.pool.size=Unexpected ("{0}") maximum number of connections per destination.
 wrong.max.pool.total=Unexpected ("{0}") maximum number of connections total.
 wrong.max.pool.idle=Unexpected ("{0}") maximum number of idle seconds.
-
+redirect.no.location="Received redirect that does not contain a location or the location is empty."
+redirect.error.determining.location="Error determining redirect location: ({0})."
+redirect.infinite.loop="Infinite loop in chained redirects detected."
+redirect.limit.reached="Max chained redirect limit ({0}) exceeded."
diff --git a/...tty-connector/src/test/java/org/glassfish/jersey/netty/connector/FollowRedirectsTest.java b/...tty-connector/src/test/java/org/glassfish/jersey/netty/connector/FollowRedirectsTest.java
@@ -17,12 +17,14 @@
 package org.glassfish.jersey.netty.connector;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import java.net.URI;
 import java.util.logging.Logger;
 
 import javax.ws.rs.GET;
 import javax.ws.rs.Path;
+import javax.ws.rs.ProcessingException;
 import javax.ws.rs.client.Client;
 import javax.ws.rs.client.ClientBuilder;
 import javax.ws.rs.client.WebTarget;
@@ -32,14 +34,15 @@
 import org.glassfish.jersey.client.ClientConfig;
 import org.glassfish.jersey.client.ClientProperties;
 import org.glassfish.jersey.logging.LoggingFeature;
+import org.glassfish.jersey.netty.connector.internal.RedirectException;
 import org.glassfish.jersey.server.ResourceConfig;
 import org.glassfish.jersey.test.JerseyTest;
 import org.junit.Test;
 
 public class FollowRedirectsTest extends JerseyTest {
 
     private static final Logger LOGGER = Logger.getLogger(FollowRedirectsTest.class.getName());
-    private static final String REDIRECT_URL = "http://localhost:9998/test";
+    private static final String TEST_URL = "http://localhost:9998/test";
 
     @Path("/test")
     public static class RedirectResource {
@@ -51,7 +54,19 @@ public String get() {
         @GET
         @Path("redirect")
         public Response redirect() {
-            return Response.seeOther(URI.create(REDIRECT_URL)).build();
+            return Response.seeOther(URI.create(TEST_URL)).build();
+        }
+
+        @GET
+        @Path("loop")
+        public Response loop() {
+            return Response.seeOther(URI.create(TEST_URL + "/loop")).build();
+        }
+
+        @GET
+        @Path("redirect2")
+        public Response redirect2() {
+            return Response.seeOther(URI.create(TEST_URL + "/redirect")).build();
         }
     }
 
@@ -109,4 +124,40 @@ public void testDontFollowPerRequestOverride() {
         assertEquals(303, r.getStatus());
         client.close();
     }
+
+    @Test
+    public void testInfiniteLoop() {
+        WebTarget t = target("test/loop");
+        t.property(ClientProperties.FOLLOW_REDIRECTS, true);
+        try {
+            t.request().get();
+            fail("Expected exception");
+        } catch (ProcessingException e) {
+            assertEquals(RedirectException.class, e.getCause().getClass());
+            assertEquals(LocalizationMessages.REDIRECT_INFINITE_LOOP(), e.getCause().getMessage());
+        }
+    }
+
+    @Test
+    public void testRedirectLimitReached() {
+        WebTarget t = target("test/redirect2");
+        t.property(ClientProperties.FOLLOW_REDIRECTS, true);
+        t.property(NettyClientProperties.MAX_REDIRECTS, 1);
+        try {
+            t.request().get();
+            fail("Expected exception");
+        } catch (ProcessingException e) {
+            assertEquals(RedirectException.class, e.getCause().getClass());
+            assertEquals(LocalizationMessages.REDIRECT_LIMIT_REACHED(1), e.getCause().getMessage());
+        }
+    }
+
+    @Test
+    public void testRedirectNoLimitReached() {
+        WebTarget t = target("test/redirect2");
+        t.property(ClientProperties.FOLLOW_REDIRECTS, true);
+        Response r = t.request().get();
+        assertEquals(200, r.getStatus());
+        assertEquals("GET", r.readEntity(String.class));
+    }
 }
diff --git a/docs/src/main/docbook/appendix-properties.xml b/docs/src/main/docbook/appendix-properties.xml
@@ -1810,6 +1810,18 @@
                             </para>
                         </entry>
                     </row>
+                    <row>
+                        <entry>&jersey.netty.NettyClientProperties.MAX_REDIRECTS;</entry>
+                        <entry><literal>jersey.config.client.NettyConnectorProvider.maxRedirect</literal></entry>
+                        <entry>
+                            <para>
+                                This property determines the maximal number of redirects during single request. Value is expected to be
+                                positive number. Default value is 5.
+                                HTTP redirection must be enabled by property org.glassfish.jersey.client.ClientProperties.FOLLOW_REDIRECTS
+                                otherwise &jersey.netty.NettyClientProperties.MAX_REDIRECTS; is not applied.
+                            </para>
+                        </entry>
+                    </row>
                 </tbody>
             </tgroup>
         </table>

diff --git a/docs/src/main/docbook/jersey.ent b/docs/src/main/docbook/jersey.ent
@@ -537,6 +537,7 @@
 <!ENTITY jersey.netty.NettyClientProperties.IDLE_CONNECTION_PRUNE_TIMEOUT "<link xlink:href='&jersey.javadoc.uri.prefix;/netty/connector/NettyClientProperties.html#IDLE_CONNECTION_PRUNE_TIMEOUT'>NettyClientProperties.IDLE_CONNECTION_PRUNE_TIMEOUT</link>" >
 <!ENTITY jersey.netty.NettyClientProperties.MAX_CONNECTIONS "<link xlink:href='&jersey.javadoc.uri.prefix;/netty/connector/NettyClientProperties.html#MAX_CONNECTIONS'>NettyClientProperties.MAX_CONNECTIONS</link>" >
 <!ENTITY jersey.netty.NettyClientProperties.MAX_CONNECTIONS_TOTAL "<link xlink:href='&jersey.javadoc.uri.prefix;/netty/connector/NettyClientProperties.html#MAX_CONNECTIONS_TOTAL'>NettyClientProperties.MAX_CONNECTIONS_TOTAL</link>" >
+<!ENTITY jersey.netty.NettyClientProperties.MAX_REDIRECTS "<link xlink:href='&jersey.javadoc.uri.prefix;/netty/connector/NettyClientProperties.html#MAX_REDIRECTS'>NettyClientProperties.MAX_REDIRECTS</link>" >
 <!ENTITY jersey.netty.NettyConnectorProvider "<link xlink:href='&jersey.javadoc.uri.prefix;/netty/connector/NettyConnectorProvider.html'>NettyConnectorProvider</link>">
 <!ENTITY jersey.server.ApplicationHandler "<link xlink:href='&jersey.javadoc.uri.prefix;/server/ApplicationHandler.html'>ApplicationHandler</link>">
 <!ENTITY jersey.server.BackgroundScheduler "<link xlink:href='&jersey.javadoc.uri.prefix;/server/BackgroundScheduler.html'>@BackgroundScheduler</link>">