Skip to content

Commit 0b3adca

Browse files
sbordetjoakime
sbordet
and
joakime
authored
Fixes #13098 - HTTP2 Server Error Handling is different than HTTP1 (#13141)
* Made HTTP/2 and HTTP/3 behave like HTTP/1 when the client aborts the request abruptly. In HTTP/1, the server was sending an error response, but in HTTP/2/3, only a reset was sent, producing an empty page on browsers. Now in all HTTP versions, such type of failures are detected, will eventually go through the core `ErrorHandler` to produce an error page. * Introduced `HttpURI.Unsafe` to write test cases that require unsafe URIs, and updated `HttpURI` javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com> Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
1 parent 7c8c3bb commit 0b3adca

File tree

35 files changed

+1393
-259
lines changed

35 files changed

+1393
-259
lines changed

‎jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,9 @@ protected void sendHeaders(HttpExchange exchange, ByteBuffer contentBuffer, bool
6868
HttpRequest request = exchange.getRequest();
6969
Content.Source requestContent = request.getBody();
7070
long contentLength = requestContent == null ? -1 : requestContent.getLength();
71-
String path = request.getPath();
72-
String query = request.getQuery();
73-
if (query != null)
74-
path += "?" + query;
75-
metaData = new MetaData.Request(request.getMethod(), HttpURI.from(path), request.getVersion(), request.getHeaders(), contentLength, request.getTrailersSupplier());
71+
// URI validations already performed by using the java.net.URI class.
72+
HttpURI uri = HttpURI.from(null, null, -1, request.getPath(), request.getQuery(), null);
73+
metaData = new MetaData.Request(request.getMethod(), uri, request.getVersion(), request.getHeaders(), contentLength, request.getTrailersSupplier());
7674
if (LOG.isDebugEnabled())
7775
LOG.debug("Sending headers with content {} last={} for {}", BufferUtil.toDetailString(contentBuffer), lastContent, exchange.getRequest());
7876
headersCallback.iterate();

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

Lines changed: 55 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,14 @@
2929
import org.eclipse.jetty.util.URIUtil;
3030

3131
/**
32-
* Http URI.
33-
*
34-
* Both {@link Mutable} and {@link Immutable} implementations are available
35-
* via the static methods such as {@link #build()} and {@link #from(String)}.
36-
*
37-
* A URI such as
32+
* <p>Representation of HTTP URIs.</p>
33+
* <p>Both {@link Mutable} and {@link Immutable} implementations are available
34+
* via the static methods such as {@link #build()} and {@link #from(String)},
35+
* and {@link Unsafe} can be used for tests or invalid URIs.</p>
36+
* <p>An HTTP URI such as
3837
* {@code http://user@host:port/path;param1/%2e/f%6fo%2fbar%20bob;param2?query#fragment}
39-
* is split into the following optional elements:<ul>
38+
* is split into the following optional elements:</p>
39+
* <ul>
4040
* <li>{@link #getScheme()} - http:</li>
4141
* <li>{@link #getAuthority()} - //name@host:port</li>
4242
* <li>{@link #getHost()} - host</li>
@@ -58,26 +58,17 @@
5858
* expansion. A literal interpretation of the RFC can result in URI paths with ambiguities
5959
* when viewed as strings. For example, a URI of {@code /foo%2f..%2fbar} is technically a single
6060
* segment of "/foo/../bar", but could easily be misinterpreted as 3 segments resolving to "/bar"
61-
* by a file system.
62-
* </p>
63-
* <p>
64-
* Thus this class avoid and/or detects such ambiguities. Furthermore, by decoding characters and
61+
* by a file system.</p>
62+
* <p>Thus this class avoid and/or detects such ambiguities. Furthermore, by decoding characters and
6563
* removing parameters before relative path normalization, ambiguous paths will be resolved in such
66-
* a way to be non-standard-but-non-ambiguous to down stream interpretation of the decoded path string.
67-
* </p>
68-
* <p>
69-
* This class collates any {@link UriCompliance.Violation violations} against the specification
64+
* a way to be non-standard-but-non-ambiguous to down stream interpretation of the decoded path string.</p>
65+
* <p>This class collates any {@link UriCompliance.Violation violations} against the specification
7066
* and/or best practises in the {@link #getViolations()}. Users of this class should check against a
7167
* configured {@link UriCompliance} mode if the {@code HttpURI} is suitable for use
72-
* (see {@link UriCompliance#checkUriCompliance(UriCompliance, HttpURI, ComplianceViolation.Listener)}).
73-
* </p>
74-
* <p>
75-
* For example, implementations that wish to process ambiguous URI paths must configure the compliance modes
76-
* to accept them and then perform their own decoding of {@link #getPath()}.
77-
* </p>
78-
* <p>
79-
* If there are multiple path parameters, only the last one is returned by {@link #getParam()}.
80-
* </p>
68+
* (see {@link UriCompliance#checkUriCompliance(UriCompliance, HttpURI, ComplianceViolation.Listener)}).</p>
69+
* <p>For example, implementations that wish to process ambiguous URI paths must configure the compliance
70+
* modes to accept them and then perform their own decoding of {@link #getPath()}.</p>
71+
* <p>If there are multiple path parameters, only the last one is returned by {@link #getParam()}.</p>
8172
**/
8273
public interface HttpURI
8374
{
@@ -152,9 +143,20 @@ static Immutable from(String scheme, String host, int port, String pathQuery)
152143
return new Mutable(scheme, host, port, pathQuery).asImmutable();
153144
}
154145

146+
/**
147+
* <p>Creates a new {@code HttpURI} with the given arguments.</p>
148+
*
149+
* @param scheme the URI scheme (normalized to lower-case)
150+
* @param host the URI host
151+
* @param port the URI port, or {@code -1} for no port
152+
* @param path the URI path
153+
* @param query the URI query
154+
* @param fragment the URI fragment
155+
* @return a new {@code HttpURI}
156+
*/
155157
static Immutable from(String scheme, String host, int port, String path, String query, String fragment)
156158
{
157-
return new Immutable(scheme, host, port, path, query, fragment);
159+
return new Immutable(scheme, host, port, path, query, fragment, false);
158160
}
159161

160162
Immutable asImmutable();
@@ -300,16 +302,15 @@ private Immutable(Mutable builder)
300302
_violations = Collections.unmodifiableSet(EnumSet.copyOf(builder._violations));
301303
}
302304

303-
private Immutable(String scheme, String host, int port, String path, String query, String fragment)
305+
private Immutable(String scheme, String host, int port, String path, String query, String fragment, boolean unsafe)
304306
{
305307
_uri = null;
306-
307-
_scheme = URIUtil.normalizeScheme(scheme);
308+
_scheme = unsafe ? scheme : URIUtil.normalizeScheme(scheme);
308309
_user = null;
309310
_host = host;
310-
_port = (port > 0) ? port : URIUtil.UNDEFINED_PORT;
311+
_port = unsafe || port > 0 ? port : URIUtil.UNDEFINED_PORT;
311312
_path = path;
312-
_canonicalPath = _path == null ? null : URIUtil.canonicalPath(_path);
313+
_canonicalPath = unsafe || path == null ? path : URIUtil.canonicalPath(path);
313314
_param = null;
314315
_query = query;
315316
_fragment = fragment;
@@ -506,6 +507,29 @@ public String toString()
506507
}
507508
}
508509

510+
/**
511+
* <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.</p>
512+
* <p>This class should be primarily used for testing, since possibly invalid URI may break
513+
* arbitrary code.</p>
514+
*/
515+
class Unsafe extends Immutable
516+
{
517+
/**
518+
* <p>Creates a new unsafe {@link HttpURI} with the given arguments.</p>
519+
*
520+
* @param scheme the URI scheme
521+
* @param host the URI host
522+
* @param port the URI port, or {@code -1} for no port
523+
* @param path the URI path
524+
* @param query the URI query
525+
* @param fragment the URI fragment
526+
*/
527+
public Unsafe(String scheme, String host, int port, String path, String query, String fragment)
528+
{
529+
super(scheme, host, port, path, query, fragment, true);
530+
}
531+
}
532+
509533
class Mutable implements HttpURI
510534
{
511535
private enum State
@@ -1692,7 +1716,7 @@ private void checkSegment(String uri, boolean dotOrEncoded, int segment, int end
16921716
if (ambiguous != null)
16931717
{
16941718
// The segment is always ambiguous.
1695-
if (Boolean.TRUE.equals(ambiguous))
1719+
if (ambiguous)
16961720
addViolation(Violation.AMBIGUOUS_PATH_SEGMENT);
16971721
// The segment is ambiguous only when followed by a parameter.
16981722
if (param)

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

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,4 +331,121 @@ public String toString()
331331
return String.format("%s{s=%d,h=%d,cl=%d}", getHttpVersion(), getStatus(), headers.size(), getContentLength());
332332
}
333333
}
334+
335+
/**
336+
* <p>Marker interface to signal that a {@link MetaData} could not be created,
337+
* for example due to an invalid URI or invalid headers.</p>
338+
*/
339+
public interface Failed
340+
{
341+
/**
342+
* <p>Inspects the given {@link MetaData}, and if it is an instance
343+
* of this interface return its failure, otherwise returns {@code null}.</p>
344+
*
345+
* @param metaData the {@link MetaData} to inspect
346+
* @return the failure if the given {@link MetaData} is an instance of this
347+
* interface, otherwise {@code null}
348+
*/
349+
static Throwable getFailure(MetaData metaData)
350+
{
351+
return metaData instanceof Failed f ? f.getFailure() : null;
352+
}
353+
354+
/**
355+
* <p>Creates a new failed {@link MetaData.Request}, with method {@code GET},
356+
* empty {@link HttpURI} and empty {@link HttpFields}, with the given
357+
* {@link HttpVersion} and failure.</p>
358+
*
359+
* @param httpVersion the HTTP version
360+
* @param failure the failure cause
361+
* @return a new failed {@link MetaData.Request}
362+
*/
363+
static MetaData.Request newFailedMetaDataRequest(HttpVersion httpVersion, Throwable failure)
364+
{
365+
return new FailedMetaDataRequest(httpVersion, failure);
366+
}
367+
368+
/**
369+
* <p>Creates a new failed {@link MetaData.Response}, with status {@code 0},
370+
* no reason, empty {@link HttpFields}, with the given {@link HttpVersion}
371+
* and failure.</p>
372+
*
373+
* @param httpVersion the HTTP version
374+
* @param failure the failure cause
375+
* @return a new failed {@link MetaData.Response}
376+
*/
377+
static MetaData.Response newFailedMetaDataResponse(HttpVersion httpVersion, Throwable failure)
378+
{
379+
return new FailedMetaDataResponse(httpVersion, failure);
380+
}
381+
382+
/**
383+
* <p>Creates a new failed {@link MetaData}, with empty {@link HttpFields},
384+
* with the given {@link HttpVersion} and failure.</p>
385+
*
386+
* @param httpVersion the HTTP version
387+
* @param failure the failure cause
388+
* @return a new failed {@link MetaData}
389+
*/
390+
static MetaData newFailedMetaData(HttpVersion httpVersion, Throwable failure)
391+
{
392+
return new FailedMetaData(httpVersion, failure);
393+
}
394+
395+
/**
396+
* @return the failure cause
397+
*/
398+
Throwable getFailure();
399+
}
400+
401+
private static class FailedMetaDataRequest extends MetaData.Request implements Failed
402+
{
403+
private final Throwable failure;
404+
405+
private FailedMetaDataRequest(HttpVersion httpVersion, Throwable failure)
406+
{
407+
super("GET", HttpURI.build(), httpVersion, HttpFields.EMPTY);
408+
this.failure = Objects.requireNonNull(failure);
409+
}
410+
411+
@Override
412+
public Throwable getFailure()
413+
{
414+
return failure;
415+
}
416+
}
417+
418+
private static class FailedMetaDataResponse extends MetaData.Response implements Failed
419+
{
420+
private final Throwable failure;
421+
422+
private FailedMetaDataResponse(HttpVersion httpVersion, Throwable failure)
423+
{
424+
super(0, null, httpVersion, HttpFields.EMPTY);
425+
this.failure = Objects.requireNonNull(failure);
426+
}
427+
428+
@Override
429+
public Throwable getFailure()
430+
{
431+
return failure;
432+
}
433+
}
434+
435+
private static class FailedMetaData extends MetaData implements Failed
436+
{
437+
private final Throwable failure;
438+
439+
private FailedMetaData(HttpVersion httpVersion, Throwable failure)
440+
{
441+
super(httpVersion, HttpFields.EMPTY);
442+
this.failure = Objects.requireNonNull(failure);
443+
}
444+
445+
@Override
446+
public Throwable getFailure()
447+
{
448+
return failure;
449+
}
450+
}
334451
}

‎jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpSenderOverHTTP2.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,8 @@ protected void sendHeaders(HttpExchange exchange, ByteBuffer contentBuffer, bool
7272
else
7373
{
7474
String path = relativize(request.getPath());
75-
HttpURI uri = HttpURI.build()
76-
.scheme(request.getScheme())
77-
.host(request.getHost())
78-
.port(request.getPort())
79-
.path(path)
80-
.query(request.getQuery());
75+
// URI validations already performed by using the java.net.URI class.
76+
HttpURI uri = HttpURI.from(request.getScheme(), request.getHost(), request.getPort(), path, request.getQuery(), null);
8177
metaData = new MetaData.Request(request.getMethod(), uri, HttpVersion.HTTP_2, request.getHeaders(), -1, request.getTrailersSupplier());
8278
}
8379

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -643,17 +643,22 @@ public void onWindowUpdate(HTTP2Stream stream, WindowUpdateFrame frame)
643643
@Override
644644
public void onStreamFailure(int streamId, int error, String reason)
645645
{
646-
Callback callback = Callback.from(() -> reset(getStream(streamId), new ResetFrame(streamId, error), Callback.NOOP));
647-
Throwable failure = toFailure(error, reason);
648-
if (LOG.isDebugEnabled())
649-
LOG.debug("Stream #{} failure {}", streamId, this, failure);
650646
HTTP2Stream stream = getStream(streamId);
647+
Callback callback = Callback.from(() -> reset(stream, new ResetFrame(streamId, error), Callback.NOOP));
648+
Throwable failure = toFailure(error, reason);
651649
if (stream != null)
652-
failStream(stream, error, reason, failure, callback);
650+
onStreamFailure(stream, error, reason, failure, callback);
653651
else
654652
callback.succeeded();
655653
}
656654

655+
protected void onStreamFailure(Stream stream, int error, String reason, Throwable failure, Callback callback)
656+
{
657+
if (LOG.isDebugEnabled())
658+
LOG.debug("Stream #{} failure {}", stream.getId(), this, failure);
659+
failStream(stream, error, reason, failure, callback);
660+
}
661+
657662
@Override
658663
public void onConnectionFailure(int error, String reason)
659664
{

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -162,31 +162,41 @@ public void data(DataFrame frame, Callback callback)
162162
@Override
163163
public void reset(ResetFrame frame, Callback callback)
164164
{
165+
if (LOG.isDebugEnabled())
166+
LOG.debug("Resetting {} {}", frame, this);
167+
165168
int flowControlLength;
169+
boolean reset = true;
166170
Throwable resetFailure = null;
167171
try (AutoLock ignored = lock.lock())
168172
{
169-
if (isReset())
173+
flowControlLength = drain();
174+
if (localReset)
170175
{
176+
reset = false;
171177
resetFailure = failure;
172178
}
173179
else
174180
{
175181
localReset = true;
176182
failure = new EOFException("reset");
177183
}
178-
flowControlLength = drain();
179184
}
185+
180186
session.dataConsumed(this, flowControlLength);
181-
if (resetFailure != null)
187+
188+
if (reset)
182189
{
183-
close();
184-
session.removeStream(this);
185-
callback.failed(resetFailure);
190+
session.reset(this, frame, callback);
186191
}
187192
else
188193
{
189-
session.reset(this, frame, callback);
194+
close();
195+
session.removeStream(this);
196+
if (resetFailure == null)
197+
callback.succeeded();
198+
else
199+
callback.failed(resetFailure);
190200
}
191201
}
192202

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.eclipse.jetty.http2.Flags;
2121
import org.eclipse.jetty.http2.frames.ContinuationFrame;
2222
import org.eclipse.jetty.http2.frames.HeadersFrame;
23+
import org.eclipse.jetty.http2.hpack.HpackException;
2324
import org.eclipse.jetty.io.RetainableByteBuffer;
2425

2526
public class ContinuationBodyParser extends BodyParser
@@ -119,18 +120,16 @@ private boolean onHeaders(ByteBuffer buffer)
119120
HeadersFrame frame = new HeadersFrame(getStreamId(), metaData, headerBlockFragments.getPriorityFrame(), headerBlockFragments.isEndStream());
120121
headerBlockFragments.reset();
121122

122-
if (metaData == HeaderBlockParser.SESSION_FAILURE)
123+
Throwable metaDataFailure = MetaData.Failed.getFailure(metaData);
124+
if (metaDataFailure instanceof HpackException.SessionException)
123125
return false;
124126

125-
if (metaData != HeaderBlockParser.STREAM_FAILURE)
126-
{
127-
notifyHeaders(frame);
128-
}
129-
else
127+
if (metaDataFailure != null)
130128
{
131129
if (!rateControlOnEvent(frame))
132130
return connectionFailure(buffer, ErrorCode.ENHANCE_YOUR_CALM_ERROR.code, "invalid_headers_frame_rate");
133131
}
132+
notifyHeaders(frame);
134133
return true;
135134
}
136135

0 commit comments

Comments
 (0)