Skip to content

Commit

Permalink
Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when i…
Browse files Browse the repository at this point in the history 
…nternally it throws "Response header too large" exception. (#12351)

* Introduced "maxResponseHeadersSize" parameter to AbstractProxyServlet.
* Introduced HttpGenerator.maxResponseHeadersSize and added checks to not exceed it.
* Fixed HttpParser to generate a 400 in case HttpParser.maxHeaderBytes are exceeded for a response.
* HTTP2Flusher now resets streams in case of failures.
* Removed cases in HTTP2Session where a GOAWAY frame was generated with lastStreamId=0.
  GOAWAY.lastStreamId=0 means that not a single request was processed by the server, which was obviously incorrect.
* Written tests for both ProxyHandler and ProxyServlet about max response headers size exceeded.
* Simplified server-side response header allocation for HTTP/1.1.

Signed-off-by: Simone Bordet <[email protected]>
  • Loading branch information
@sbordet
sbordet authored Oct 30, 2024
1 parent e2277f0 commit 85ce152
Show file tree
Hide file tree
Showing 13 changed files with 543 additions and 125 deletions.
2 changes: 1 addition & 1 deletion jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,6 @@ public BadMessageException(int code, String reason)
public BadMessageException(int code, String reason, Throwable cause)
{
super(code, reason, cause);
assert code >= 400 && code < 500;
assert HttpStatus.isClientError(code);
}
}
65 changes: 38 additions & 27 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ public class HttpGenerator
private static final Logger LOG = LoggerFactory.getLogger(HttpGenerator.class);

public static final boolean __STRICT = Boolean.getBoolean("org.eclipse.jetty.http.HttpGenerator.STRICT");

private static final byte[] __colon_space = new byte[]{':', ' '};
public static final MetaData.Response CONTINUE_100_INFO = new MetaData.Response(100, null, HttpVersion.HTTP_1_1, HttpFields.EMPTY);
private static final Index<Boolean> ASSUMED_CONTENT_METHODS = new Index.Builder<Boolean>()
.caseSensitive(false)
.with(HttpMethod.POST.asString(), Boolean.TRUE)
.with(HttpMethod.PUT.asString(), Boolean.TRUE)
.build();
public static final int CHUNK_SIZE = 12;

// states
public enum State
Expand All @@ -68,25 +73,14 @@ public enum Result
DONE // The current phase of generation is complete
}

// other statics
public static final int CHUNK_SIZE = 12;

private State _state = State.START;
private EndOfContent _endOfContent = EndOfContent.UNKNOWN_CONTENT;
private MetaData _info;

private long _contentPrepared = 0;
private boolean _noContentResponse = false;
private Boolean _persistent = null;

private static final Index<Boolean> ASSUMED_CONTENT_METHODS = new Index.Builder<Boolean>()
.caseSensitive(false)
.with(HttpMethod.POST.asString(), Boolean.TRUE)
.with(HttpMethod.PUT.asString(), Boolean.TRUE)
.build();

// data
private boolean _needCRLF = false;
private int _maxHeaderBytes;

public HttpGenerator()
{
Expand All @@ -103,6 +97,16 @@ public void reset()
_needCRLF = false;
}

public int getMaxHeaderBytes()
{
return _maxHeaderBytes;
}

public void setMaxHeaderBytes(int maxHeaderBytes)
{
_maxHeaderBytes = maxHeaderBytes;
}

public State getState()
{
return _state;
Expand Down Expand Up @@ -594,28 +598,28 @@ private void generateHeaders(ByteBuffer header, ByteBuffer content, boolean last
HttpField field = fields.getField(f);
HttpHeader h = field.getHeader();
if (h == null)
{
putTo(field, header);
}
else
{
switch (h)
{
case CONTENT_LENGTH:
case CONTENT_LENGTH ->
{
if (contentLength < 0)
contentLength = field.getLongValue();
else if (contentLength != field.getLongValue())
throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, String.format("Incorrect Content-Length %d!=%d", contentLength, field.getLongValue()));
contentLengthField = true;
break;

case CONTENT_TYPE:
}
case CONTENT_TYPE ->
{
// write the field to the header
contentType = true;
putTo(field, header);
break;
}

case TRANSFER_ENCODING:
case TRANSFER_ENCODING ->
{
if (http11)
{
Expand All @@ -627,10 +631,8 @@ else if (contentLength != field.getLongValue())
transferEncoding = transferEncoding.withValues(field.getValues());
chunkedHint |= field.contains(HttpHeaderValue.CHUNKED.asString());
}
break;
}

case CONNECTION:
case CONNECTION ->
{
// Save to connection field for processing when all other fields are known
if (connection == null)
Expand All @@ -641,13 +643,11 @@ else if (contentLength != field.getLongValue())
connectionClose |= field.contains(HttpHeaderValue.CLOSE.asString());
connectionKeepAlive |= field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
connectionUpgrade |= field.contains(HttpHeaderValue.UPGRADE.asString());
break;
}

default:
putTo(field, header);
default -> putTo(field, header);
}
}
checkMaxHeaderBytes(header);
}
}

Expand Down Expand Up @@ -887,12 +887,23 @@ else if (http10)

// end the header.
header.put(HttpTokens.CRLF);

checkMaxHeaderBytes(header);
}

private void checkMaxHeaderBytes(ByteBuffer header)
{
int maxHeaderBytes = getMaxHeaderBytes();
if (maxHeaderBytes > 0 && header.position() > maxHeaderBytes)
throw new BufferOverflowException();
}

private static void putContentLength(ByteBuffer header, long contentLength)
{
if (contentLength == 0)
{
header.put(CONTENT_LENGTH_0);
}
else
{
header.put(HttpHeader.CONTENT_LENGTH.getBytesColonSpace());
Expand Down
47 changes: 36 additions & 11 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,10 @@ private void quickStart(ByteBuffer buffer)
if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes)
{
LOG.warn("padding is too large >{}", _maxHeaderBytes);
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
if (_requestParser)
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
else
throw new HttpException.RuntimeException(_responseStatus, "Bad Response");
}
}
}
Expand Down Expand Up @@ -740,10 +743,15 @@ private boolean parseLine(ByteBuffer buffer)
else
{
if (_requestParser)
{
LOG.warn("request is too large >{}", _maxHeaderBytes);
throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431);
}
else
{
LOG.warn("response is too large >{}", _maxHeaderBytes);
throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431);
throw new HttpException.RuntimeException(_responseStatus, "Response Header Bytes Too Large");
}
}
}

Expand Down Expand Up @@ -865,7 +873,10 @@ else if (Violation.CASE_INSENSITIVE_METHOD.isAllowedBy(_complianceMode))
break;

default:
throw new BadMessageException(_requestParser ? "No URI" : "No Status");
if (_requestParser)
throw new BadMessageException("No URI");
else
throw new HttpException.RuntimeException(_responseStatus, "No Status");
}
break;

Expand Down Expand Up @@ -1261,10 +1272,12 @@ protected boolean parseFields(ByteBuffer buffer)
if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes)
{
boolean header = _state == State.HEADER;
LOG.warn("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes);
throw new BadMessageException(header
? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431
: HttpStatus.PAYLOAD_TOO_LARGE_413);
if (debugEnabled)
LOG.debug("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes);
if (_requestParser)
throw new BadMessageException(header ? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431 : HttpStatus.PAYLOAD_TOO_LARGE_413);
// There is no equivalent of 431 for response headers.
throw new HttpException.RuntimeException(_responseStatus, "Response Header Fields Too Large");
}

switch (_fieldState)
Expand Down Expand Up @@ -1744,17 +1757,29 @@ else if (isTerminated())
if (debugEnabled)
LOG.debug("{} EOF in {}", this, _state);
setState(State.CLOSED);
_handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400));
if (_requestParser)
_handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400, "Early EOF"));
else
_handler.badMessage(new HttpException.RuntimeException(_responseStatus, "Early EOF"));
break;
}
}
}
catch (Throwable x)
{
BufferUtil.clear(buffer);
HttpException bad = x instanceof HttpException http
? http
: new BadMessageException(_requestParser ? "Bad Request" : "Bad Response", x);
HttpException bad;
if (x instanceof HttpException http)
{
bad = http;
}
else
{
if (_requestParser)
bad = new BadMessageException("Bad Request", x);
else
bad = new HttpException.RuntimeException(_responseStatus, "Bad Response", x);
}
badMessage(bad);
}
return false;
Expand Down
30 changes: 14 additions & 16 deletions ...re/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -740,11 +740,6 @@ public boolean goAway(GoAwayFrame frame, Callback callback)
}

private GoAwayFrame newGoAwayFrame(int error, String reason)
{
return newGoAwayFrame(getLastRemoteStreamId(), error, reason);
}

private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String reason)
{
byte[] payload = null;
if (reason != null)
Expand All @@ -753,7 +748,7 @@ private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String rea
reason = reason.substring(0, Math.min(reason.length(), 32));
payload = reason.getBytes(StandardCharsets.UTF_8);
}
return new GoAwayFrame(lastRemoteStreamId, error, payload);
return new GoAwayFrame(getLastRemoteStreamId(), error, payload);
}

@Override
Expand Down Expand Up @@ -1267,15 +1262,20 @@ boolean hasHighPriority()
return false;
}

@Override
public void failed(Throwable x)
public void closeAndFail(Throwable failure)
{
if (stream != null)
{
stream.close();
stream.getSession().removeStream(stream);
}
super.failed(x);
failed(failure);
}

public void resetAndFail(Throwable x)
{
if (stream != null)
stream.reset(new ResetFrame(stream.getId(), ErrorCode.CANCEL_STREAM_ERROR.code), Callback.from(() -> failed(x)));
}

/**
Expand Down Expand Up @@ -1808,10 +1808,8 @@ private void onGoAway(GoAwayFrame frame)

if (failStreams)
{
// Must compare the lastStreamId only with local streams.
// For example, a client that sent request with streamId=137 may send a GOAWAY(4),
// where streamId=4 is the last stream pushed by the server to the client.
// The server must not compare its local streamId=4 with remote streamId=137.
// The lastStreamId carried by the GOAWAY is that of a local stream,
// so the lastStreamId must be compared only to local streams ids.
Predicate<Stream> failIf = stream -> stream.isLocal() && stream.getId() > frame.getLastStreamId();
failStreams(failIf, "closing", false);
}
Expand Down Expand Up @@ -1839,7 +1837,7 @@ private void onShutdown()
case REMOTELY_CLOSED ->
{
closed = CloseState.CLOSING;
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
zeroStreamsAction = () -> terminate(goAwayFrame);
failure = new ClosedChannelException();
failStreams = true;
Expand Down Expand Up @@ -1869,7 +1867,7 @@ private void onShutdown()
}
else
{
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
abort(reason, cause, Callback.from(() -> terminate(goAwayFrame)));
}
}
Expand Down Expand Up @@ -1998,7 +1996,7 @@ private void onWriteFailure(Throwable x)
closed = CloseState.CLOSING;
zeroStreamsAction = () ->
{
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
terminate(goAwayFrame);
};
failure = x;
Expand Down
6 changes: 6 additions & 0 deletions ...ore/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,15 @@ public void reset(ResetFrame frame, Callback callback)
}
session.dataConsumed(this, flowControlLength);
if (resetFailure != null)
{
close();
session.removeStream(this);
callback.failed(resetFailure);
}
else
{
session.reset(this, frame, callback);
}
}

private boolean startWrite(Callback callback)
Expand Down
Loading

0 comments on commit 85ce152

Please sign in to comment.