fix: handle a ':path' with an asterisk as the RFC required#981
fix: handle a ':path' with an asterisk as the RFC required#981He-Pin wants to merge 2 commits intoapache:mainfrom
Conversation
http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala
Show resolved
Hide resolved
| 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") |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
|
There is also #961 |
c7ae010 to
a0f6379
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
53c8772 to
4696e75
Compare
|
Rebased and will update the comments too. |
4696e75 to
bad9ff0
Compare
|
based on #961 |
|
Isn't the main reason for this PR is to fix this test: "handle a ':path' with an asterisk" |
| `absolute-path` ~ optional('?' ~ rawQueryString) // origin-form | ||
| ) // TODO: asterisk-form | ||
| | '*' ~ run(setPath(Path.Empty)) ~ run { _rawQueryString = None } // asterisk-form (for OPTIONS requests) | ||
| ) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
jrudolph
left a comment
There was a problem hiding this comment.
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.
Motivation
The HTTP/2
:pathpseudo-header was not fully handled per RFC 7540:*) used byOPTIONSandCONNECTrequests was rejected with a parse error instead of being accepted.:pathwas detected in the decompression layer rather than in the header-parsing layer, so it did not throw the correct connection-level protocol error.CONNECTrequests — which per RFC 7540 §8.3 must not include:pathor:scheme— were not validated at all.Modification
:pathin parsing of HTTP/2 HeadersResult
Connectionheader, empty:path,CONNECTwith:path/:scheme) continue to triggerGOAWAY(PROTOCOL_ERROR), maintaining full RFC 7540 compliance.OPTIONS *andCONNECTrequests with the asterisk-form:pathare now correctly accepted and routed.References