Skip to content

Commit

Permalink
Merge pull request adamfisk#330 from jekh/fix-npe-from-invalid-request
Browse files Browse the repository at this point in the history 
Fix NPE when failing to decode response from server
  • Loading branch information
@jekh
jekh authored Dec 23, 2016
2 parents 8ce850c + f1a86d4 commit 0d16280
Show file tree
Hide file tree
Showing 4 changed files with 183 additions and 104 deletions.
64 changes: 7 additions & 57 deletions src/main/java/org/littleshoot/proxy/impl/ClientToProxyConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import io.netty.buffer.Unpooled;
import io.netty.channel.Channel;
import io.netty.channel.ChannelPipeline;
import io.netty.handler.codec.http.DefaultFullHttpResponse;
import io.netty.handler.codec.http.DefaultHttpRequest;
import io.netty.handler.codec.http.FullHttpRequest;
import io.netty.handler.codec.http.FullHttpResponse;
Expand Down Expand Up @@ -428,7 +427,7 @@ void respond(ProxyToServerConnection serverConnection, HttpFilters filters,
// if this HttpResponse does not have any means of signaling the end of the message body other than closing
// the connection, convert the message to a "Transfer-Encoding: chunked" HTTP response. This avoids the need
// to close the client connection to indicate the end of the message. (Responses to HEAD requests "must be" empty.)
if (!ProxyUtils.isHead(currentHttpRequest) && !ProxyUtils.isResponseSelfTerminating(httpResponse)) {
if (!ProxyUtils.isHEAD(currentHttpRequest) && !ProxyUtils.isResponseSelfTerminating(httpResponse)) {
// if this is not a FullHttpResponse, duplicate the HttpResponse from the server before sending it to
// the client. this allows us to set the Transfer-Encoding to chunked without interfering with netty's
// handling of the response from the server. if we modify the original HttpResponse from the server,
Expand Down Expand Up @@ -482,7 +481,7 @@ boolean shouldSuppressInitialRequest() {

protected Future<?> execute() {
LOG.debug("Responding with CONNECT successful");
HttpResponse response = responseFor(HttpVersion.HTTP_1_1,
HttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1,
CONNECTION_ESTABLISHED);
response.headers().set(HttpHeaders.Names.CONNECTION, HttpHeaders.Values.KEEP_ALIVE);
ProxyUtils.addVia(response, proxyServer.getProxyAlias());
Expand Down Expand Up @@ -754,7 +753,7 @@ protected void exceptionCaught(Throwable cause) {
* descending ordering.
*
* Regarding the Javadoc of {@link HttpObjectAggregator} it's needed to have
* the {@link HttpResponseEncoder} or {@link HttpRequestEncoder} before the
* the {@link HttpResponseEncoder} or {@link io.netty.handler.codec.http.HttpRequestEncoder} before the
* {@link HttpObjectAggregator} in the {@link ChannelPipeline}.
*
* @param pipeline
Expand Down Expand Up @@ -999,7 +998,7 @@ private void writeAuthenticationRequired(String realm) {
+ "credentials (e.g., bad password), or your\n"
+ "browser doesn't understand how to supply\n"
+ "the credentials required.</p>\n" + "</body></html>\n";
DefaultFullHttpResponse response = responseFor(HttpVersion.HTTP_1_1,
FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1,
HttpResponseStatus.PROXY_AUTHENTICATION_REQUIRED, body);
HttpHeaders.setDate(response, new Date());
response.headers().set("Proxy-Authenticate",
Expand Down Expand Up @@ -1201,7 +1200,7 @@ private void stripHopByHopHeaders(HttpHeaders headers) {
*/
private boolean writeBadGateway(HttpRequest httpRequest) {
String body = "Bad Gateway: " + httpRequest.getUri();
DefaultFullHttpResponse response = responseFor(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body);
FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body);

if (ProxyUtils.isHEAD(httpRequest)) {
// don't allow any body content in response to a HEAD request
Expand All @@ -1220,7 +1219,7 @@ private boolean writeBadGateway(HttpRequest httpRequest) {
*/
private boolean writeBadRequest(HttpRequest httpRequest) {
String body = "Bad Request to URI: " + httpRequest.getUri();
DefaultFullHttpResponse response = responseFor(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_REQUEST, body);
FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_REQUEST, body);

if (ProxyUtils.isHEAD(httpRequest)) {
// don't allow any body content in response to a HEAD request
Expand All @@ -1240,7 +1239,7 @@ private boolean writeBadRequest(HttpRequest httpRequest) {
*/
private boolean writeGatewayTimeout(HttpRequest httpRequest) {
String body = "Gateway Timeout";
DefaultFullHttpResponse response = responseFor(HttpVersion.HTTP_1_1,
FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1,
HttpResponseStatus.GATEWAY_TIMEOUT, body);

if (httpRequest != null && ProxyUtils.isHEAD(httpRequest)) {
Expand Down Expand Up @@ -1299,55 +1298,6 @@ private boolean respondWithShortCircuitResponse(HttpResponse httpResponse) {
return true;
}

/**
* Factory for {@link DefaultFullHttpResponse}s.
*
* @param httpVersion
* @param status
* @param body
* @return
*/
private DefaultFullHttpResponse responseFor(HttpVersion httpVersion,
HttpResponseStatus status, String body) {
byte[] bytes = body.getBytes(Charset.forName("UTF-8"));
ByteBuf content = Unpooled.copiedBuffer(bytes);
return responseFor(httpVersion, status, content, bytes.length);
}

/**
* Factory for {@link DefaultFullHttpResponse}s.
*
* @param httpVersion
* @param status
* @param body
* @param contentLength
* @return
*/
private DefaultFullHttpResponse responseFor(HttpVersion httpVersion,
HttpResponseStatus status, ByteBuf body, int contentLength) {
DefaultFullHttpResponse response = body != null ? new DefaultFullHttpResponse(
HttpVersion.HTTP_1_1, status, body)
: new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, status);
if (body != null) {
response.headers().set(HttpHeaders.Names.CONTENT_LENGTH,
contentLength);
response.headers().set("Content-Type", "text/html; charset=UTF-8");
}
return response;
}

/**
* Factory for {@link DefaultFullHttpResponse}s.
*
* @param httpVersion
* @param status
* @return
*/
private DefaultFullHttpResponse responseFor(HttpVersion httpVersion,
HttpResponseStatus status) {
return responseFor(httpVersion, status, (ByteBuf) null, 0);
}

/**
* Identify the host and port for a request.
*
Expand Down
57 changes: 20 additions & 37 deletions src/main/java/org/littleshoot/proxy/impl/ProxyToServerConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,19 @@
import io.netty.channel.ChannelPipeline;
import io.netty.channel.socket.nio.NioSocketChannel;
import io.netty.channel.udt.nio.NioUdtProvider;
import io.netty.handler.codec.http.FullHttpResponse;
import io.netty.handler.codec.http.HttpContent;
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpMessage;
import io.netty.handler.codec.http.HttpMethod;
import io.netty.handler.codec.http.HttpObject;
import io.netty.handler.codec.http.HttpObjectAggregator;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpRequestEncoder;
import io.netty.handler.codec.http.HttpResponse;
import io.netty.handler.codec.http.HttpResponseDecoder;
import io.netty.handler.codec.http.HttpResponseEncoder;
import io.netty.handler.codec.http.HttpResponseStatus;
import io.netty.handler.codec.http.HttpVersion;
import io.netty.handler.codec.http.LastHttpContent;
import io.netty.handler.timeout.IdleStateHandler;
import io.netty.handler.traffic.GlobalTrafficShapingHandler;
Expand All @@ -44,7 +47,6 @@
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.UnknownHostException;
import java.util.LinkedList;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.RejectedExecutionException;
Expand Down Expand Up @@ -118,12 +120,6 @@ public class ProxyToServerConnection extends ProxyConnection<HttpResponse> {
* Keeps track of HttpRequests that have been issued so that we can
* associate them with responses that we get back
*/
private final Queue<HttpRequest> issuedRequests = new LinkedList<HttpRequest>();

/**
* While we're doing a chunked transfer, this keeps track of the HttpRequest
* to which we're responding.
*/
private volatile HttpRequest currentHttpRequest;

/**
Expand Down Expand Up @@ -223,6 +219,19 @@ protected void read(Object msg) {
protected ConnectionState readHTTPInitial(HttpResponse httpResponse) {
LOG.debug("Received raw response: {}", httpResponse);

if (httpResponse.getDecoderResult().isFailure()) {
LOG.debug("Could not parse response from server. Decoder result: {}", httpResponse.getDecoderResult().toString());

// create a "substitute" Bad Gateway response from the server, since we couldn't understand what the actual
// response from the server was. set the keep-alive on the substitute response to false so the proxy closes
// the connection to the server, since we don't know what state the server thinks the connection is in.
FullHttpResponse substituteResponse = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1,
HttpResponseStatus.BAD_GATEWAY,
"Unable to parse response from server");
HttpHeaders.setKeepAlive(substituteResponse, false);
httpResponse = substituteResponse;
}

currentFilters.serverToProxyResponseReceiving();

rememberCurrentResponse(httpResponse);
Expand Down Expand Up @@ -274,20 +283,14 @@ public HeadAwareHttpResponseDecoder(int maxInitialLineLength,

@Override
protected boolean isContentAlwaysEmpty(HttpMessage httpMessage) {
if (httpMessage instanceof HttpResponse) {
// Identify our current request
identifyCurrentRequest();
}

// The current HTTP Request can be null when this proxy is
// negotiating a CONNECT request with a chained proxy
// negotiating a CONNECT request with a chained proxy
// while it is running as a MITM. Since the response to a
// CONNECT request does not have any content, we return true.
if(currentHttpRequest == null) {
return true;
} else {
return HttpMethod.HEAD.equals(currentHttpRequest.getMethod()) ?
true : super.isContentAlwaysEmpty(httpMessage);
return ProxyUtils.isHEAD(currentHttpRequest) || super.isContentAlwaysEmpty(httpMessage);
}
}
};
Expand Down Expand Up @@ -355,7 +358,7 @@ protected void writeHttp(HttpObject httpObject) {
if (httpObject instanceof HttpRequest) {
HttpRequest httpRequest = (HttpRequest) httpObject;
// Remember that we issued this HttpRequest for later
issuedRequests.add(httpRequest);
currentHttpRequest = httpRequest;
}
super.writeHttp(httpObject);
}
Expand Down Expand Up @@ -491,26 +494,6 @@ protected HttpFilters getHttpFiltersFromProxyServer(HttpRequest httpRequest) {
* Private Implementation
**************************************************************************/

/**
* An HTTP response is associated with a single request, so we can pop the
* correct request off the queue.
*/
private void identifyCurrentRequest() {
LOG.debug("Remembering the current request.");
// I'm a little unclear as to when the request queue would
// ever actually be empty, but it is from time to time in practice.
// We've seen this particularly when behind proxies that govern
// access control on local networks, likely related to redirects.
if (!this.issuedRequests.isEmpty()) {
this.currentHttpRequest = this.issuedRequests.remove();
if (this.currentHttpRequest == null) {
LOG.warn("Got null HTTP request object.");
}
} else {
LOG.debug("Request queue is empty!");
}
}

/**
* Keeps track of the current HttpResponse so that we can associate its
* headers with future related chunks for this same transfer.
Expand Down
72 changes: 62 additions & 10 deletions src/main/java/org/littleshoot/proxy/impl/ProxyUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,19 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.udt.nio.NioUdtProvider;
import io.netty.handler.codec.http.DefaultFullHttpResponse;
import io.netty.handler.codec.http.DefaultHttpResponse;
import io.netty.handler.codec.http.FullHttpResponse;
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpMessage;
import io.netty.handler.codec.http.HttpMethod;
import io.netty.handler.codec.http.HttpObject;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpResponse;
import io.netty.handler.codec.http.HttpResponseStatus;
import io.netty.handler.codec.http.HttpVersion;
import io.netty.handler.codec.http.LastHttpContent;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.math.NumberUtils;
Expand All @@ -21,6 +25,7 @@

import java.io.IOException;
import java.net.InetAddress;
import java.nio.charset.StandardCharsets;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -386,16 +391,6 @@ public static boolean isContentAlwaysEmpty(HttpMessage msg) {
return false;
}

/**
* Returns true if the request is an HTTP HEAD request.
*
* @param request HTTP request
* @return true if request is a HEAD, otherwise false
*/
public static boolean isHead(HttpRequest request) {
return HttpMethod.HEAD.equals(request.getMethod());
}

/**
* Returns true if the HTTP response from the server is expected to indicate its own message length/end-of-message. Returns false
* if the server is expected to indicate the end of the HTTP entity by closing the connection.
Expand Down Expand Up @@ -585,4 +580,61 @@ public static boolean isUdtAvailable() {
return false;
}
}

/**
* Creates a new {@link FullHttpResponse} with the specified String as the body contents (encoded using UTF-8).
*
* @param httpVersion HTTP version of the response
* @param status HTTP status code
* @param body body to include in the FullHttpResponse; will be UTF-8 encoded
* @return new http response object
*/
public static FullHttpResponse createFullHttpResponse(HttpVersion httpVersion,
HttpResponseStatus status,
String body) {
byte[] bytes = body.getBytes(StandardCharsets.UTF_8);
ByteBuf content = Unpooled.copiedBuffer(bytes);

return createFullHttpResponse(httpVersion, status, "text/html; charset=utf-8", content, bytes.length);
}

/**
* Creates a new {@link FullHttpResponse} with no body content
*
* @param httpVersion HTTP version of the response
* @param status HTTP status code
* @return new http response object
*/
public static FullHttpResponse createFullHttpResponse(HttpVersion httpVersion,
HttpResponseStatus status) {
return createFullHttpResponse(httpVersion, status, null, null, 0);
}

/**
* Creates a new {@link FullHttpResponse} with the specified body.
*
* @param httpVersion HTTP version of the response
* @param status HTTP status code
* @param contentType the Content-Type of the body
* @param body body to include in the FullHttpResponse; if null
* @param contentLength number of bytes to send in the Content-Length header; should equal the number of bytes in the ByteBuf
* @return new http response object
*/
public static FullHttpResponse createFullHttpResponse(HttpVersion httpVersion,
HttpResponseStatus status,
String contentType,
ByteBuf body,
int contentLength) {
DefaultFullHttpResponse response;

if (body != null) {
response = new DefaultFullHttpResponse(httpVersion, status, body);
response.headers().set(HttpHeaders.Names.CONTENT_LENGTH, contentLength);
response.headers().set(HttpHeaders.Names.CONTENT_TYPE, contentType);
} else {
response = new DefaultFullHttpResponse(httpVersion, status);
}

return response;
}
}
Loading

0 comments on commit 0d16280

Please sign in to comment.