Skip to content

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

Open
He-Pin wants to merge 2 commits intoapache: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 intoapache:mainfrom
He-Pin:port/http2-bad-header-response

Conversation

@He-Pin
Copy link
Member

@He-Pin He-Pin commented Mar 8, 2026

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
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

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
Member

There is also #961

@He-Pin He-Pin force-pushed the port/http2-bad-header-response branch from c7ae010 to a0f6379 Compare March 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
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@He-Pin He-Pin force-pushed the port/http2-bad-header-response branch from 53c8772 to 4696e75 Compare March 8, 2026 12:34
@He-Pin He-Pin changed the title fix: return HTTP 400 for malformed HTTP/2 request headers instead of silent stream reset fix: align HTTP/2 malformed-header handling Mar 8, 2026
@He-Pin
Copy link
Member Author

He-Pin commented Mar 8, 2026

Rebased and will update the comments too.

@He-Pin He-Pin force-pushed the port/http2-bad-header-response branch from 4696e75 to bad9ff0 Compare March 8, 2026 12:54
@He-Pin
Copy link
Member Author

He-Pin commented Mar 8, 2026

based on #961

@pjfanning
Copy link
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-Pin He-Pin changed the title fix: align HTTP/2 malformed-header handling fix: handle a ':path' with an asterisk as the RFC required Mar 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
Member Author

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
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
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.

Copy link
Contributor

@jrudolph jrudolph left a comment

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.

Sign up for free to 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