Skip to content

fix: handle a ':path' with an asterisk as the RFC required - #981

Open
He-Pin wants to merge 2 commits into
apache:mainfrom
He-Pin:port/http2-bad-header-response
Open

fix: handle a ':path' with an asterisk as the RFC required#981
He-Pin wants to merge 2 commits into
apache:mainfrom
He-Pin:port/http2-bad-header-response

Conversation

@He-Pin

@He-PinHe-Pin commented Mar 8, 2026

Copy link
Copy Markdown
Member

Motivation

The HTTP/2 :path pseudo-header was not fully handled per RFC 7540:

  • The asterisk-form (*) used by OPTIONS and CONNECT requests was rejected with a parse error instead of being accepted.
  • An empty :path was detected in the decompression layer rather than in the header-parsing layer, so it did not throw the correct connection-level protocol error.
  • CONNECT requests — which per RFC 7540 §8.3 must not include :path or :scheme — were not validated at all.

Modification

  1. hardend the :path in parsing of HTTP/2 Headers

Result

  • Structural protocol violations (pseudo-header ordering, Connection header, empty :path, CONNECT with :path/:scheme) continue to trigger GOAWAY(PROTOCOL_ERROR), maintaining full RFC 7540 compliance.
  • OPTIONS * and CONNECT requests with the asterisk-form :path are now correctly accepted and routed.
  • All relevant RFC 7540 §8.1.2 and §8.3 conformance requirements are covered by tests.

References

override def parse(name: String, value: String, parserSettings: ParserSettings): (Uri.Path, Option[String]) =
try {
if (value.isEmpty) {
protocolError("Pseudo-header ':path' must not be empty")

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The ":path" pseudo-header field includes the path and query parts
of the target URI (the "path-absolute" production and optionally a
'?' character followed by the "query" production (see Sections 3.3
and 3.4 of [RFC3986]). A request in asterisk form includes the
value '*' for the ":path" pseudo-header field.

 This pseudo-header field MUST NOT be empty for "http" or "https"
URIs; "http" or "https" URIs that do not contain a path component
MUST include a value of '/'. The exception to this rule is an
OPTIONS request for an "http" or "https" URI that does not include
a path component; these MUST include a ":path" pseudo-header field
with a value of '*' (see [[RFC7230], Section 5.3.4](https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.4)).

Belshe, et al. Standards Track [Page 55]

RFC 7540 HTTP/2 May 2015

All HTTP/2 requests MUST include exactly one valid value for the
":method", ":scheme", and ":path" pseudo-header fields, unless it is
a CONNECT request (Section 8.3). An HTTP request that omits
mandatory pseudo-header fields is malformed (Section 8.1.2.6).

HTTP/2 does not define a way to carry the version identifier that is
included in the HTTP/1.1 request line.

@pjfanning

Copy link
Copy Markdown
Member

There is also #961

@He-Pin
He-Pinforce-pushed the port/http2-bad-header-response branch from c7ae010 to a0f6379CompareMarch 8, 2026 09:40
@He-Pin
He-Pin marked this pull request as ready for review March 8, 2026 09:53
@He-Pin
He-Pin marked this pull request as draft March 8, 2026 09:56
@He-Pin
He-Pinforce-pushed the port/http2-bad-header-response branch from 53c8772 to 4696e75CompareMarch 8, 2026 12:34
@He-PinHe-Pin changed the title fix: return HTTP 400 for malformed HTTP/2 request headers instead of silent stream resetfix: align HTTP/2 malformed-header handlingMar 8, 2026
@He-Pin

Copy link
Copy Markdown
MemberAuthor

Rebased and will update the comments too.

@He-Pin
He-Pinforce-pushed the port/http2-bad-header-response branch from 4696e75 to bad9ff0CompareMarch 8, 2026 12:54
@He-Pin

Copy link
Copy Markdown
MemberAuthor

based on #961

@pjfanning

Copy link
Copy Markdown
Member

Isn't the main reason for this PR is to fix this test: "handle a ':path' with an asterisk"

@He-Pin
He-Pin marked this pull request as ready for review March 8, 2026 13:13
@He-PinHe-Pin changed the title fix: align HTTP/2 malformed-header handlingfix: handle a ':path' with an asterisk as the RFC requiredMar 8, 2026
`absolute-path` ~ optional('?' ~ rawQueryString) // origin-form
) // TODO: asterisk-form
| '*' ~ run(setPath(Path.Empty)) ~ run { _rawQueryString = None } // asterisk-form (for OPTIONS requests)
)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This pseudo-header field MUST NOT be empty for "http" or "https"
URIs; "http" or "https" URIs that do not contain a path component
MUST include a value of '/'. The exception to this rule is an
OPTIONS request for an "http" or "https" URI that does not include
a path component; these MUST include a ":path" pseudo-header field
with a value of '*' (see [RFC7230], Section 5.3.4).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a test for this behavior? We need to demonstrate how users can deal with these requests. Path.Empty is likely not what user code wants to see for these requests.

// mandatory pseudo-header fields is malformed

// [assume CONNECT not supported]
// 8.3. HTTP/2 CONNECT Method

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we support CONNECT at all? Is that even something that user code could implement?

Since we make the logic more strict, it could still be ok to fix this here but we should make sure we don't mask an error that reports that CONNECT is not supported with a different parsing error now.

@jrudolphjrudolph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure how important these changes are. CONNECT is not really supported (afair), so I'm somewhat reluctant changing behaviour and adding extra code for that.

The OPTIONS change seems like a thing that might be needed (though, it's somewhat weird that no one found it missing so far). It doesn't feel fully addressed, though, tests and demonstrations how to use it are missing.

In any case, these should be two separate PRs, but right now I'm not convinced any of these are changes we should do.

He-Pinand others added 2 commits June 15, 2026 12:01
@He-Pin
He-Pinforce-pushed the port/http2-bad-header-response branch from bad9ff0 to 7dea820CompareJune 15, 2026 04:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@He-Pin@pjfanning@jrudolph