Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

Merged
merged 9 commits into from
Oct 30, 2024
Merged

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3f903e2
Fixes #12348 - HttpClientTransportDynamic does not initialize low-lev…
sbordet Oct 4, 2024
e5a672c
Updates after review.
sbordet Oct 6, 2024
53dff62
Fixed test that was relying on default configuration, rather than exp…
sbordet Oct 6, 2024
ad00741
Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when i…
sbordet Oct 7, 2024
af31788
Updates from review.
sbordet Oct 8, 2024
8a8ba79
Updates from review.
sbordet Oct 8, 2024
e5a89c3
Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/5685/proxy-respon…
sbordet Oct 18, 2024
92c516e
Simplified server-side response header allocation for HTTP/1.1.
sbordet Oct 29, 2024
9fb5bd8
Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/5685/proxy-respon…
sbordet Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java
View file
Open in desktop
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
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if 0 is a sensible default, even if we always call the setter before using the generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not check for response headers max size before, so 0 keeps the original behavior.


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;
}
Comment on lines +100 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a duplicate in the server as we already have HttpConfiguration.getResponseHeaderSize(), and we have use overflowing of that buffer to enforce a header size.

So why are we duplicating this mechanism? Can't the client just allocate a buffer of a specific size when it is asked for a HEADERs buffer?


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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading