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 headers are case sensitive, contrary to RFC 2616 #216

Closed
mattjonesorg opened this issue Aug 19, 2014 · 7 comments
Closed

HTTP headers are case sensitive, contrary to RFC 2616 #216

mattjonesorg opened this issue Aug 19, 2014 · 7 comments

Comments

@mattjonesorg
Copy link

Per the spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2), header "field names are case-insensitive."

However, when you call context.getHostname() or context.getAcceptEncoding(), or any of the other methods which simply wrap the getHeader call on the httpServletRequest, the request is case sensitive.

So, if the HTTP header is passed as:
Host: somehost.com

a call to getHostname() will not return "somehost.com" as expected, because the implementation for getHostname() looks up the header "host".

There are several solutions to this, but the one I'm thinking of is that the getHeaders() call should cache the keys in a map where the keys are lower case. Then, the wrapper methods should lookup the headers in the Context's map rather than passing the lookup to the HttpServletRequest's headers map.

Here's a failing test for ContextImplTest to demonstrate this behavior.

    public void headersShouldBeCaseInsensitive()
    {
        //say the httpServletRequest to return a certain value:
        when(httpServletRequest.getHeader("Host")).thenReturn("test.com");

        //init the context from a (mocked) servlet
        context.init(httpServletRequest, httpServletResponse);

        //make sure this is correct
        assertEquals("test.com", context.getHostname());
    }
@raphaelbauer
Copy link
Contributor

Hi Matt,

thanks for your report. It's really important that we are getting all those presumably tiny things right as well.

The api documentation of for instance "getHeader(String name)" says that: "The header name is case insensitive. ".
In your example above you are mocking the httpServletRequest with the "wrong behavior". So afaics the test does not test the behavior of a real application server.

But maybe there is a real problem somewhere - so my first question would be: Does this problem occur in reality on e.g. Jetty 9? (or the integration tests you can run with Ninja?).

Because if that occurs this might point to an implementation bug in Jetty imho...

Cheers,

Raphael

@mattjonesorg
Copy link
Author

Hello Raphael,

I'm glad that I entered the issue for discussion rather than submitting a pull request. This definitely is a real issue with AppEngine 1.9.6. Let me see if I can create a test that reproduces this with AppEngine natively, then I can report the issue to Google.

Thanks,

Matt

@raphaelbauer
Copy link
Contributor

Interesting.

I'd be great if you could also verify that the problem does not exist on our NinjaTest aka Jetty9 :)

@mattjonesorg
Copy link
Author

Very interesting. I was unable to reproduce this in GAE without Ninja.

@raphaelbauer
Copy link
Contributor

No idea how to help... I'd try to check if NinjaTest has that bug...

Other ideas: The local dev mode of GAE is often quite different to the production machines and servlet implementations. That can also be a cause of confusion... Try with a non-ninja application... I bet it has the same "bugs".

Ninja does only call the servlet api... So I am still not sure if that's a Ninja problem...

@mattjonesorg
Copy link
Author

I can no longer reproduce the issue. That's good news.

Sorry to bother you with this. I'm convinced now that this is neither a ninja issue or a GAE issue.

@raphaelbauer
Copy link
Contributor

No worries :) Great that it magically disappeared.. :)

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

No branches or pull requests

2 participants