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

HTTP/2 with HttpServletRequest.getHeader("Host") returns null on Jetty 10, but a valid value on Jetty 9 #5304

Closed
joakime opened this issue Sep 21, 2020 · 4 comments · Fixed by #5307

Comments

@joakime
Copy link
Contributor

joakime commented Sep 21, 2020

Jetty version
10.0.x

Description
If you make an HTTP/2 request, and then in your servlet you use HttpServletRequest.getHeader("Host") the value is null on Jetty 10.
But it returns a value on Jetty 9.

Screenshot of Jetty 9 (notice the "Host" header in the enumerated headers)

jetty-9-chrome-http2-authority-header

Screenshot of Jetty 10 (no "Host" header in the enumerated headers)

jetty-10-chrome-http2-authority-header

@joakime joakime added High Priority Bug For general bugs on Jetty side labels Sep 21, 2020
@gregw
Copy link
Contributor

gregw commented Sep 21, 2020

I don't think we should always add a Host header for HTTP/2, as that is a lot of wasted work for apps that don't use the Host header. Instead we could either write an optional customizer and/or rewrite rule that will add a Host header from the authority of a HTTP/2 request

@gregw gregw added Enhancement and removed Bug For general bugs on Jetty side High Priority labels Sep 21, 2020
@joakime
Copy link
Contributor Author

joakime commented Sep 21, 2020

Note (to others that read this) that to get the authority from the HttpServletRequest, you can use the existing methods ...

  • HttpServletRequest.getServerName()
  • HttpServletRequest.getServerPort()

See: https://github.com/eclipse-ee4j/servlet-api/blob/4.0.4-RELEASE/api/src/main/java/javax/servlet/ServletRequest.java#L214-L228

@joakime
Copy link
Contributor Author

joakime commented Sep 21, 2020

We could probably improve the HostHeaderCustomizer to set the Host header too.

https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/HostHeaderCustomizer.java

@joakime
Copy link
Contributor Author

joakime commented Sep 21, 2020

@gregw I think we should have a module to enable the HostHeaderCustomizer for http2 on jetty-home.

gregw added a commit that referenced this issue Sep 21, 2020
Updated HostHeaderCustomizer to actually add the Host header, either from values passed in the custructor or from the getServerName and getServerPort methods.

The HttpURI is no longer updated.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Sep 22, 2020
 + Found and fixed bug in HttpFields
 + Added port normalization support to HttpScheme
 + added test

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Sep 22, 2020
 + refixed bug in HttpFields

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Sep 22, 2020
 + updated more options doco and handling

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Sep 22, 2020
 + still fixing HttpFields bug

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Sep 22, 2020
updates from review
@gregw gregw self-assigned this Sep 22, 2020
gregw added a commit that referenced this issue Sep 22, 2020
* Issue #5264 Jetty Home warning

Warn when using jetty home as a jetty base

* Issue #5304 HTTP2 HostHeader

 + updated more options doco and handling

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #5264 Jetty Home warning

updates from review
@gregw gregw linked a pull request Sep 23, 2020 that will close this issue
gregw added a commit that referenced this issue Sep 23, 2020
* Issue #5304 HTTP2 HostHeader

Updated HostHeaderCustomizer to actually add the Host header, either from values passed in the custructor or from the getServerName and getServerPort methods.

The HttpURI is no longer updated.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #5304 HTTP2 HostHeader

 + Found and fixed bug in HttpFields
 + Added port normalization support to HttpScheme
 + added test

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* blank line

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #5304 HTTP2 HostHeader

 + refixed bug in HttpFields

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #5304 HTTP2 HostHeader

 + still fixing HttpFields bug

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #5304 HTTP2 Host Header

updates from review
gregw added a commit that referenced this issue Dec 7, 2020
refactored common code
fast alphabet lookup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants