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.

* 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.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
  • Loading branch information
@sbordet
sbordet committed Oct 7, 2024
1 parent 53dff62 commit ad00741
Show file tree
Hide file tree
Showing 12 changed files with 510 additions and 111 deletions.
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 @@ -590,28 +594,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 @@ -620,10 +624,8 @@ else if (contentLength != field.getLongValue())
transferEncoding = field;
chunkedHint = field.contains(HttpHeaderValue.CHUNKED.asString());
}
break;
}

case CONNECTION:
case CONNECTION ->
{
String value = field.getValue();

Expand Down Expand Up @@ -677,13 +679,11 @@ else if (field.contains(HttpHeaderValue.KEEP_ALIVE.asString()))
{
putTo(field, header);
}
break;
}

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

Expand Down Expand Up @@ -798,12 +798,23 @@ else if (response != null)

// 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
10 changes: 6 additions & 4 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 @@ -1259,10 +1259,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 BadMessageException(HttpStatus.BAD_REQUEST_400, "Response Header Fields Too Large");
}

switch (_fieldState)
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 ad00741

Please sign in to comment.