Fixes #13098 - HTTP2 Server Error Handling is different than HTTP1 #13141

sbordet · 2025-05-16T10:06:14Z

Errors happening before the request is handled are now handled similarly to HTTP/1.1, and are now hitting the ErrorHandler, so that a 400 response can be generated.

TODO: * Restore use of HttpURI.build() in HttpSenderOverHTTP2. * Use HttpURI.Unsafe to write a test case. * HttpURI.Unsafe javadocs. Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Simone Bordet <[email protected]>

gregw · 2025-05-18T23:13:28Z

@lorban will h3 need similar changes?

gregw

CI is badly broken as well.

gregw · 2025-05-18T23:14:42Z

...http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderBlockParser.java

@@ -135,4 +139,68 @@ public MetaData parse(ByteBuffer buffer, int blockLength)
            }
        }
    }
+
+    public static class SessionFailureMetaData extends MetaData


gregw · 2025-05-18T23:14:57Z

...http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderBlockParser.java

+        }
+    }
+
+    public interface StreamFailureMetaData


Why is both a session and stream failure needed? Can't a session failure be detected by the fact that it has a failed stream?

Or perhaps it could just be a FailureMetaData with reason in the getFailure?

...http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderBlockParser.java

lorban · 2025-05-20T08:37:33Z

@gregw yes, H3 probably suffers from the same problem.

Signed-off-by: Simone Bordet <[email protected]>

sbordet · 2025-05-20T12:03:53Z

@gregw @lorban I have implemented also for HTTP/3.

...etty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackException.java

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

Signed-off-by: Simone Bordet <[email protected]>

…andler'.

* Fixed HTTP2Stream.reset() logic. Signed-off-by: Simone Bordet <[email protected]>

gregw · 2025-05-25T23:03:30Z

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

@@ -506,6 +507,27 @@ public String toString()
        }
    }

+    /**
+     * <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.</p>


Suggested change

* <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.</p>

* <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.

* Primarily intended for testing, as un-validated may break arbitrary code.</p>

It's also used by Jetty HttpClient to allow it to make requests that other servers would allow, but we don't.

An example would be using the %00 in the request path.
In jetty-12.0.x HEAD our HttpURI and MetaData classes both reject that pct-encoding, but it's 100% allowed to be sent from a user-agent.

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

gregw · 2025-05-25T23:07:29Z

.../jetty-http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/internal/MetaDataBuilder.java

+                .scheme(_scheme)
+                .host(_authority == null ? null : _authority.getHost())
+                .port(_authority == null ? -1 : _authority.getPort())
+                .pathQuery(_path);


Do we need?

Suggested change

.pathQuery(_path);

.pathQuery(_path)

.asImmutable();

It creates another object in the hot path, and the return type is already just HttpURI, not its Mutable subclass.

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

lorban · 2025-05-27T08:21:35Z

...etty-http2/jetty-http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackException.java

+        private final boolean request;
+        private final boolean response;
+
+        public StreamException(boolean request, boolean response, String messageFormat, Object... args)


Why are these exceptions taking format + args instead of a pre-formatted string?

...-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java

lorban · 2025-05-27T08:27:32Z

...ort/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpSenderOverHTTP3.java

-                .path(path)
-                .query(request.getQuery());
+            // URI validations already performed by using the java.net.URI class.
+            HttpURI uri = new HttpURI.Unsafe(request.getScheme(), request.getHost(), request.getPort(), path, request.getQuery(), null);


A side-effect of this change makes the HttpURI immutable.

I wonder if the MetaData.Request constructors shouldn't always call immutable() on the HttpURI parameter they are given?

It would be another allocation in the hot path, and the signature returns the immutable HttpURI type.

lorban · 2025-05-27T08:29:47Z

...tp3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/metadata/MetaDataBuilder.java

+                .scheme(_scheme)
+                .host(_authority == null ? null : _authority.getHost())
+                .port(_authority == null ? -1 : _authority.getPort())
+                .pathQuery(_path);


immutable()?

Signed-off-by: Simone Bordet <[email protected]>

lorban

There is still the oddity of StreamException and HpackException taking format + args, otherwise LGTM.

joakime · 2025-05-30T19:12:53Z

Can this be merged ?

joakime and others added 5 commits May 6, 2025 09:03

WIP on ErrorHandler

8dc146f

Work in progress, but functionality should be complete.

8d38f41

TODO: * Restore use of HttpURI.build() in HttpSenderOverHTTP2. * Use HttpURI.Unsafe to write a test case. * HttpURI.Unsafe javadocs. Signed-off-by: Simone Bordet <[email protected]>

Fixed checkstyle.

3f8681f

Signed-off-by: Simone Bordet <[email protected]>

Fixed checkstyle.

95ee744

Signed-off-by: Simone Bordet <[email protected]>

Fixed HpackException constructors.

f612259

Signed-off-by: Simone Bordet <[email protected]>

sbordet added the build-all-tests label May 16, 2025

Fixed test and added ee10 test.

6648f35

Signed-off-by: Simone Bordet <[email protected]>

sbordet mentioned this pull request May 17, 2025

HTTP2 Server Error Handling is different than HTTP1 #13098

Closed

sbordet linked an issue May 17, 2025 that may be closed by this pull request

HTTP2 Server Error Handling is different than HTTP1 #13098

Closed

sbordet marked this pull request as ready for review May 17, 2025 17:39

sbordet requested review from gregw and joakime May 17, 2025 17:39

sbordet changed the title ~~Fix/12.0.x/http2 badmessage errorhandler~~ Fixes #13098 - HTTP2 Server Error Handling is different than HTTP1 May 17, 2025

sbordet added this to Jetty 12.0.22 - FROZEN May 17, 2025

sbordet moved this to 👀 In review in Jetty 12.0.22 - FROZEN May 17, 2025

sbordet self-assigned this May 17, 2025

gregw requested changes May 18, 2025

View reviewed changes

Implemented same mechanism for HTTP/3.

cfb3a8f

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested review from gregw and lorban May 20, 2025 12:03

lorban requested changes May 20, 2025

View reviewed changes

...etty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackException.java Outdated Show resolved Hide resolved

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Updates from review.

4e695c1

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested a review from lorban May 20, 2025 14:01

sbordet added 4 commits May 21, 2025 15:46

Updates from review.

b1f434e

Signed-off-by: Simone Bordet <[email protected]>

Fixed HTTP/1.1 HttpURI construction in HttpSenderOverHTTP.

341f6df

Signed-off-by: Simone Bordet <[email protected]>

Made HttpURI.Unsafe not do any checks on the input parameters.

45d339c

Signed-off-by: Simone Bordet <[email protected]>

Merged branch 'jetty-12.0.x' into 'fix/12.0.x/http2-badmessage-errorh…

5ea39cf

…andler'.

* Undeprecated HttpURI.from() with all the URI parameters.

68808e1

* Fixed HTTP2Stream.reset() logic. Signed-off-by: Simone Bordet <[email protected]>

gregw requested changes May 25, 2025

View reviewed changes

lorban requested changes May 27, 2025

View reviewed changes

Updates from review.

b3634e5

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested review from gregw and lorban May 29, 2025 10:29

joakime added the Sponsored This issue affects a user with a commercial support agreement label May 29, 2025

lorban approved these changes May 30, 2025

View reviewed changes

sbordet merged commit 0b3adca into jetty-12.0.x Jun 2, 2025
10 checks passed

github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.0.22 - FROZEN Jun 2, 2025

sbordet deleted the fix/12.0.x/http2-badmessage-errorhandler branch June 2, 2025 13:26

	* <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.</p>
	* <p>An unsafe {@link HttpURI} that accepts URI parts without checking whether they are valid.
	* Primarily intended for testing, as un-validated may break arbitrary code.</p>

Fixes #13098 - HTTP2 Server Error Handling is different than HTTP1 #13141

Fixes #13098 - HTTP2 Server Error Handling is different than HTTP1 #13141

Uh oh!

Conversation

sbordet commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregw commented May 18, 2025

Uh oh!

gregw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lorban commented May 20, 2025

Uh oh!

sbordet commented May 20, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorban left a comment

Choose a reason for hiding this comment

Uh oh!

joakime commented May 30, 2025

Uh oh!

Uh oh!

Uh oh!

sbordet commented May 16, 2025 •

edited

Loading