Skip to content

Commit 85ce152

Browse files
sbordetsbordet
authoredOct 30, 2024
Fixes jetty#5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. (jetty#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 <simone.bordet@gmail.com>
1 parent e2277f0 commit 85ce152

File tree

13 files changed

+543
-125
lines changed

13 files changed

+543
-125
lines changed
 

‎jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,6 @@ public BadMessageException(int code, String reason)
4949
public BadMessageException(int code, String reason, Throwable cause)
5050
{
5151
super(code, reason, cause);
52-
assert code >= 400 && code < 500;
52+
assert HttpStatus.isClientError(code);
5353
}
5454
}

‎jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

+38-27
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,14 @@ public class HttpGenerator
4141
private static final Logger LOG = LoggerFactory.getLogger(HttpGenerator.class);
4242

4343
public static final boolean __STRICT = Boolean.getBoolean("org.eclipse.jetty.http.HttpGenerator.STRICT");
44-
4544
private static final byte[] __colon_space = new byte[]{':', ' '};
4645
public static final MetaData.Response CONTINUE_100_INFO = new MetaData.Response(100, null, HttpVersion.HTTP_1_1, HttpFields.EMPTY);
46+
private static final Index<Boolean> ASSUMED_CONTENT_METHODS = new Index.Builder<Boolean>()
47+
.caseSensitive(false)
48+
.with(HttpMethod.POST.asString(), Boolean.TRUE)
49+
.with(HttpMethod.PUT.asString(), Boolean.TRUE)
50+
.build();
51+
public static final int CHUNK_SIZE = 12;
4752

4853
// states
4954
public enum State
@@ -68,25 +73,14 @@ public enum Result
6873
DONE // The current phase of generation is complete
6974
}
7075

71-
// other statics
72-
public static final int CHUNK_SIZE = 12;
73-
7476
private State _state = State.START;
7577
private EndOfContent _endOfContent = EndOfContent.UNKNOWN_CONTENT;
7678
private MetaData _info;
77-
7879
private long _contentPrepared = 0;
7980
private boolean _noContentResponse = false;
8081
private Boolean _persistent = null;
81-
82-
private static final Index<Boolean> ASSUMED_CONTENT_METHODS = new Index.Builder<Boolean>()
83-
.caseSensitive(false)
84-
.with(HttpMethod.POST.asString(), Boolean.TRUE)
85-
.with(HttpMethod.PUT.asString(), Boolean.TRUE)
86-
.build();
87-
88-
// data
8982
private boolean _needCRLF = false;
83+
private int _maxHeaderBytes;
9084

9185
public HttpGenerator()
9286
{
@@ -103,6 +97,16 @@ public void reset()
10397
_needCRLF = false;
10498
}
10599

100+
public int getMaxHeaderBytes()
101+
{
102+
return _maxHeaderBytes;
103+
}
104+
105+
public void setMaxHeaderBytes(int maxHeaderBytes)
106+
{
107+
_maxHeaderBytes = maxHeaderBytes;
108+
}
109+
106110
public State getState()
107111
{
108112
return _state;
@@ -594,28 +598,28 @@ private void generateHeaders(ByteBuffer header, ByteBuffer content, boolean last
594598
HttpField field = fields.getField(f);
595599
HttpHeader h = field.getHeader();
596600
if (h == null)
601+
{
597602
putTo(field, header);
603+
}
598604
else
599605
{
600606
switch (h)
601607
{
602-
case CONTENT_LENGTH:
608+
case CONTENT_LENGTH ->
609+
{
603610
if (contentLength < 0)
604611
contentLength = field.getLongValue();
605612
else if (contentLength != field.getLongValue())
606613
throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, String.format("Incorrect Content-Length %d!=%d", contentLength, field.getLongValue()));
607614
contentLengthField = true;
608-
break;
609-
610-
case CONTENT_TYPE:
615+
}
616+
case CONTENT_TYPE ->
611617
{
612618
// write the field to the header
613619
contentType = true;
614620
putTo(field, header);
615-
break;
616621
}
617-
618-
case TRANSFER_ENCODING:
622+
case TRANSFER_ENCODING ->
619623
{
620624
if (http11)
621625
{
@@ -627,10 +631,8 @@ else if (contentLength != field.getLongValue())
627631
transferEncoding = transferEncoding.withValues(field.getValues());
628632
chunkedHint |= field.contains(HttpHeaderValue.CHUNKED.asString());
629633
}
630-
break;
631634
}
632-
633-
case CONNECTION:
635+
case CONNECTION ->
634636
{
635637
// Save to connection field for processing when all other fields are known
636638
if (connection == null)
@@ -641,13 +643,11 @@ else if (contentLength != field.getLongValue())
641643
connectionClose |= field.contains(HttpHeaderValue.CLOSE.asString());
642644
connectionKeepAlive |= field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
643645
connectionUpgrade |= field.contains(HttpHeaderValue.UPGRADE.asString());
644-
break;
645646
}
646-
647-
default:
648-
putTo(field, header);
647+
default -> putTo(field, header);
649648
}
650649
}
650+
checkMaxHeaderBytes(header);
651651
}
652652
}
653653

@@ -887,12 +887,23 @@ else if (http10)
887887

888888
// end the header.
889889
header.put(HttpTokens.CRLF);
890+
891+
checkMaxHeaderBytes(header);
892+
}
893+
894+
private void checkMaxHeaderBytes(ByteBuffer header)
895+
{
896+
int maxHeaderBytes = getMaxHeaderBytes();
897+
if (maxHeaderBytes > 0 && header.position() > maxHeaderBytes)
898+
throw new BufferOverflowException();
890899
}
891900

892901
private static void putContentLength(ByteBuffer header, long contentLength)
893902
{
894903
if (contentLength == 0)
904+
{
895905
header.put(CONTENT_LENGTH_0);
906+
}
896907
else
897908
{
898909
header.put(HttpHeader.CONTENT_LENGTH.getBytesColonSpace());

‎jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java

+36-11
Original file line numberDiff line numberDiff line change
@@ -676,7 +676,10 @@ private void quickStart(ByteBuffer buffer)
676676
if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes)
677677
{
678678
LOG.warn("padding is too large >{}", _maxHeaderBytes);
679-
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
679+
if (_requestParser)
680+
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
681+
else
682+
throw new HttpException.RuntimeException(_responseStatus, "Bad Response");
680683
}
681684
}
682685
}
@@ -740,10 +743,15 @@ private boolean parseLine(ByteBuffer buffer)
740743
else
741744
{
742745
if (_requestParser)
746+
{
743747
LOG.warn("request is too large >{}", _maxHeaderBytes);
748+
throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431);
749+
}
744750
else
751+
{
745752
LOG.warn("response is too large >{}", _maxHeaderBytes);
746-
throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431);
753+
throw new HttpException.RuntimeException(_responseStatus, "Response Header Bytes Too Large");
754+
}
747755
}
748756
}
749757

@@ -865,7 +873,10 @@ else if (Violation.CASE_INSENSITIVE_METHOD.isAllowedBy(_complianceMode))
865873
break;
866874

867875
default:
868-
throw new BadMessageException(_requestParser ? "No URI" : "No Status");
876+
if (_requestParser)
877+
throw new BadMessageException("No URI");
878+
else
879+
throw new HttpException.RuntimeException(_responseStatus, "No Status");
869880
}
870881
break;
871882

@@ -1261,10 +1272,12 @@ protected boolean parseFields(ByteBuffer buffer)
12611272
if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes)
12621273
{
12631274
boolean header = _state == State.HEADER;
1264-
LOG.warn("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes);
1265-
throw new BadMessageException(header
1266-
? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431
1267-
: HttpStatus.PAYLOAD_TOO_LARGE_413);
1275+
if (debugEnabled)
1276+
LOG.debug("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes);
1277+
if (_requestParser)
1278+
throw new BadMessageException(header ? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431 : HttpStatus.PAYLOAD_TOO_LARGE_413);
1279+
// There is no equivalent of 431 for response headers.
1280+
throw new HttpException.RuntimeException(_responseStatus, "Response Header Fields Too Large");
12681281
}
12691282

12701283
switch (_fieldState)
@@ -1744,17 +1757,29 @@ else if (isTerminated())
17441757
if (debugEnabled)
17451758
LOG.debug("{} EOF in {}", this, _state);
17461759
setState(State.CLOSED);
1747-
_handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400));
1760+
if (_requestParser)
1761+
_handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400, "Early EOF"));
1762+
else
1763+
_handler.badMessage(new HttpException.RuntimeException(_responseStatus, "Early EOF"));
17481764
break;
17491765
}
17501766
}
17511767
}
17521768
catch (Throwable x)
17531769
{
17541770
BufferUtil.clear(buffer);
1755-
HttpException bad = x instanceof HttpException http
1756-
? http
1757-
: new BadMessageException(_requestParser ? "Bad Request" : "Bad Response", x);
1771+
HttpException bad;
1772+
if (x instanceof HttpException http)
1773+
{
1774+
bad = http;
1775+
}
1776+
else
1777+
{
1778+
if (_requestParser)
1779+
bad = new BadMessageException("Bad Request", x);
1780+
else
1781+
bad = new HttpException.RuntimeException(_responseStatus, "Bad Response", x);
1782+
}
17581783
badMessage(bad);
17591784
}
17601785
return false;

‎jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java

+14-16
Original file line numberDiff line numberDiff line change
@@ -740,11 +740,6 @@ public boolean goAway(GoAwayFrame frame, Callback callback)
740740
}
741741

742742
private GoAwayFrame newGoAwayFrame(int error, String reason)
743-
{
744-
return newGoAwayFrame(getLastRemoteStreamId(), error, reason);
745-
}
746-
747-
private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String reason)
748743
{
749744
byte[] payload = null;
750745
if (reason != null)
@@ -753,7 +748,7 @@ private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String rea
753748
reason = reason.substring(0, Math.min(reason.length(), 32));
754749
payload = reason.getBytes(StandardCharsets.UTF_8);
755750
}
756-
return new GoAwayFrame(lastRemoteStreamId, error, payload);
751+
return new GoAwayFrame(getLastRemoteStreamId(), error, payload);
757752
}
758753

759754
@Override
@@ -1267,15 +1262,20 @@ boolean hasHighPriority()
12671262
return false;
12681263
}
12691264

1270-
@Override
1271-
public void failed(Throwable x)
1265+
public void closeAndFail(Throwable failure)
12721266
{
12731267
if (stream != null)
12741268
{
12751269
stream.close();
12761270
stream.getSession().removeStream(stream);
12771271
}
1278-
super.failed(x);
1272+
failed(failure);
1273+
}
1274+
1275+
public void resetAndFail(Throwable x)
1276+
{
1277+
if (stream != null)
1278+
stream.reset(new ResetFrame(stream.getId(), ErrorCode.CANCEL_STREAM_ERROR.code), Callback.from(() -> failed(x)));
12791279
}
12801280

12811281
/**
@@ -1808,10 +1808,8 @@ private void onGoAway(GoAwayFrame frame)
18081808

18091809
if (failStreams)
18101810
{
1811-
// Must compare the lastStreamId only with local streams.
1812-
// For example, a client that sent request with streamId=137 may send a GOAWAY(4),
1813-
// where streamId=4 is the last stream pushed by the server to the client.
1814-
// The server must not compare its local streamId=4 with remote streamId=137.
1811+
// The lastStreamId carried by the GOAWAY is that of a local stream,
1812+
// so the lastStreamId must be compared only to local streams ids.
18151813
Predicate<Stream> failIf = stream -> stream.isLocal() && stream.getId() > frame.getLastStreamId();
18161814
failStreams(failIf, "closing", false);
18171815
}
@@ -1839,7 +1837,7 @@ private void onShutdown()
18391837
case REMOTELY_CLOSED ->
18401838
{
18411839
closed = CloseState.CLOSING;
1842-
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
1840+
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
18431841
zeroStreamsAction = () -> terminate(goAwayFrame);
18441842
failure = new ClosedChannelException();
18451843
failStreams = true;
@@ -1869,7 +1867,7 @@ private void onShutdown()
18691867
}
18701868
else
18711869
{
1872-
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
1870+
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
18731871
abort(reason, cause, Callback.from(() -> terminate(goAwayFrame)));
18741872
}
18751873
}
@@ -1998,7 +1996,7 @@ private void onWriteFailure(Throwable x)
19981996
closed = CloseState.CLOSING;
19991997
zeroStreamsAction = () ->
20001998
{
2001-
GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason);
1999+
GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
20022000
terminate(goAwayFrame);
20032001
};
20042002
failure = x;

‎jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java

+6
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,15 @@ public void reset(ResetFrame frame, Callback callback)
179179
}
180180
session.dataConsumed(this, flowControlLength);
181181
if (resetFailure != null)
182+
{
183+
close();
184+
session.removeStream(this);
182185
callback.failed(resetFailure);
186+
}
183187
else
188+
{
184189
session.reset(this, frame, callback);
190+
}
185191
}
186192

187193
private boolean startWrite(Callback callback)

0 commit comments

Comments
 (0)
Please sign in to comment.