Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect request URIs with HTTP/2 #1274

Closed
richdougherty opened this issue Jul 10, 2017 · 6 comments · Fixed by #1385
Closed

Incorrect request URIs with HTTP/2 #1274

richdougherty opened this issue Jul 10, 2017 · 6 comments · Fixed by #1385
Assignees
Labels
bug play-akka-http-integration t:http2 Issues related to support HTTP2
Projects
Milestone

Comments

@richdougherty
Copy link
Contributor

This issue occurs with Play 2.6.1 which uses Akka HTTP 10.0.9. A Play user has reported an issue with handling requests with query parameters in the path, e.g. /test?param=s. Issue: playframework/playframework#7596

  • When requests are served with HTTP/1 the request URI parsed by Akka HTTP is correct (http://localhost:9000/test?param=s). When served with HTTP/2 the request URI is incorrect: the query part of the URI is included in the URI path (https://localhost:9443/test%3Fparam=s).
  • In addition, the special Raw-Request-URI header is correctly provided with the HTTP/1 request, but not with the HTTP/2 request.

You can see debug output showing the HttpRequest values here: playframework/playframework#7596 (comment).

@ktoso ktoso added 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted bug t:http2 Issues related to support HTTP2 labels Jul 10, 2017
@ktoso ktoso added this to the 10.0.10 milestone Jul 10, 2017
@ktoso
Copy link
Member

ktoso commented Jul 10, 2017

Thanks for reporting! Entire weeks following this one we'll work on http so will fix this then :)

@richdougherty
Copy link
Contributor Author

OK thanks, someone on the Play team may have time but we can discuss how we're placed when you have time.

By the way, I've traced the error to the http2.RequestParsing object. It has a line:

val uri = Uri(scheme, authority).withPath(Uri.Path(path))

Unfortunately the value of path can be /test?param=s, which is a path and a query string, but it is parsed with the Uri.Path.apply method, which only handles paths. In contrast, HTTP/1's HttpRequestParser uses a specialised URI parsing method: Uri.parseHttpRequestTarget to parse the target of an HTTP/1 request. Maybe HTTP/2 needs a special URI parsing method too, which parses a scheme, authority and pathAndQuery into a Uri?

The http2.RequestParsing class also needs a line like the one from HTTP/1's HttpRequestParser:

val allHeaders0 =
  if (rawRequestUriHeader) `Raw-Request-URI`(uriBytes.decodeString(HttpCharsets.`US-ASCII`.nioCharset)) :: headers
  else headers

E.g. something like:

if (rawRequestUriHeader) headers += `Raw-Request-URI`(scheme + "://" + authority + pathAndQuery)

@jrudolph
Copy link
Member

Thanks for bringing it up, @richdougherty.

In contrast, HTTP/1's HttpRequestParser uses a specialised URI parsing method: Uri.parseHttpRequestTarget to parse the target of an HTTP/1 request. Maybe HTTP/2 needs a special URI parsing method too, which parses a scheme, authority and pathAndQuery into a Uri?

Yes, from a cursory look into it that might be the best solution.

@ktoso ktoso added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Jul 18, 2017
@richdougherty
Copy link
Contributor Author

Hi, just wondering if you've had a chance to look at it? We might be able to look at it soon if you're too busy.

@jrudolph
Copy link
Member

@richdougherty, sorry, no didn't get to it so far. Would be nice if you could help us out with that one.

@richdougherty
Copy link
Contributor Author

OK, I think I should have a chance too look at it soon - hopefully next week :)

@ktoso ktoso added 3 - in progress Someone is working on this ticket and removed 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted labels Aug 28, 2017
@ktoso ktoso removed 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding 3 - in progress Someone is working on this ticket labels Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug play-akka-http-integration t:http2 Issues related to support HTTP2
Projects
No open projects
HTTP/2
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants