Skip to content

Commit

Permalink
Review comment fixes
Browse files Browse the repository at this point in the history
Signed-off-by: Jorge Bescos Gascon <[email protected]>
  • Loading branch information
jbescos committed May 13, 2022
1 parent 75e5fce commit e835077
Show file tree
Hide file tree
Showing 7 changed files with 187 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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."
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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));
}
}
12 changes: 12 additions & 0 deletions docs/src/main/docbook/appendix-properties.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down

0 comments on commit e835077

Please sign in to comment.