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

Headers are downcased #1436

Closed
bensie opened this issue Mar 17, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@bensie
Copy link
Contributor

commented Mar 17, 2015

Just updated from the latest 4.x 14.0 Ubuntu package to the latest 5.x Ubuntu 14.04 package to test the new version (nginx).

Noticed that the names of all headers set by the application are now downcased, when in 4.x they were preserved as declared. Is this expected behavior or a bug?

4.x
Access-Control-Allow-Origin: *
X-Content-Type-Options: nosniff
Cache-Control: no-cache
Strict-Transport-Security: max-age=31536000
X-Request-Id: 653c4064-8661-4069-99eb-7c545c1021cd
X-Runtime: 0.002611
X-Powered-By: Phusion Passenger

5.x
access-control-allow-origin: *
cache-control: no-cache
strict-transport-security: max-age=31536000
x-content-type-options: nosniff
X-Powered-By: Phusion Passenger
x-request-id: f9c6a526-80bc-4cf1-a135-6e63f8c3a25f
x-runtime: 0.002673
@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

This is expected behavior. We downcase all headers as an optimization for our internal hash tables. According to the HTTP specification, headers are case-insensitive, so this should not cause any problems for compliant apps.

@bensie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2015

👍 thanks for clarifying!

@rolftimmermans

This comment has been minimized.

Copy link

commented Apr 25, 2015

Is there a way to turn off this optimisation and revert to the old behaviour? The application headers are merged with Apache's default headers, and the result is that the headers are mixed case.

HTTP/1.1 201 Created
Date: Sat, 25 Apr 2015 11:46:51 GMT
Server: Apache/2
cache-control: no-cache

Although still correct, we run an API where people might inspect the server headers and we think this isn't as aesthetically pleasing as consistent header casing (and we prefer Apache's style).

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 25, 2015

No, this is not possible. Downcasing headers is fundamental to the codebase.

What could be done is re-capitalizing the headers, but this would make header processing slightly slower.

@rolftimmermans

This comment has been minimized.

Copy link

commented Apr 25, 2015

A slight performance impact is understandable. Are you interested in supporting an option to recapitalise headers? I can try to propose a patch, but I could use some general pointers how you think it could best be implemented.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 25, 2015

Your use case is very special, and we'd rather divert our development resources to higher priority issues, so I think a patch would be best.

The place that you should be patching is ext/common/agents/HelperAgent/RequestHandler/ForwardResponse.h, function constructHeaderBuffersForResponse. There is a while loop in there that iterates over the application response headers hash table, req->appResponse.headers, and constructs response header buffers from them. That's where the transformation needs to happen. You can allocate the memory for strings of the transformed response by using req->pool. See MemoryKit/palloc.h and the Raptor blog for information about that mechanism. Note that the hash table maps LStrings to LStrings. An LString is a non-contiguous string with parts linked to each other through pointers. More information about LString is available in DataStructures/LString.h and the Raptor blog.

Other things that need to be done:

  • A configuration option for Apache.
  • A configuration option for Nginx.
  • I'd like this to be a global configuration option, not a per-request configuration option. It's unlikely that anybody wants to customize this on a per-request or even per-app basis. By making this a global configuration option, the existance of the feature has a limited impact on performance.
  • Documentation for this configuration option.
  • Signing the contributor agreement.
@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

We've had a discussion with Enterprise customer about this issue. They told us that their healthcheck scripts fail because of this header downcasing issue. They're using the Ruby Net::HTTP library, which doesn't handle headers case-insensitively as it should.

This is a bug in Net::HTTP, but upgrading Ruby isn't so easy. I imagine that there are quite a lot of HTTP libraries out there that are broken in a similar way.

Perhaps what we can do is to use downcased headers for lookup, but preserve the original headers for the purpose of generating a response. This should be faster than re-capitalizing headers.

@FooBarWidget FooBarWidget changed the title Downcased Headers Headers are downcased Apr 30, 2015

@rolftimmermans

This comment has been minimized.

Copy link

commented Apr 30, 2015

That would be a preferable solution for us as well, because then the app has complete control over the headers.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

The cost of that approach would be slightly higher memory usage (the downcased and the original version of the header have to be simultaneously be kept in memory), but I believe this overhead is small enough to be unnoticeable.

@lynxnathan

This comment has been minimized.

Copy link

commented May 1, 2015

So, we just spent about 3 hours trying to find out where those headers were being changed. After we removed Rails from an application which also used Grape this issue started happening. As far as I'm aware, the developer of the application or the middleware should have the final say since, as everyone should be aware by now, developers don't necessarily follow the specification to the letter, and this is especially true since you can easily use those headers in, say, a javascript application where those using responses from passenger may not be aware theres an specification for HTTP at all.

I'm aware I'm not an Enterprise costumer, so this may have little impact on your choices, but I greatly believe you should not be messing with something like this.

@estevaoam

This comment has been minimized.

Copy link

commented May 1, 2015

👍 to @lynxnathan comment.

IMHO, there is no reason to transform a header field given by the application and return it instead of the original one.

Perhaps what we can do is to use downcased headers for lookup, but preserve the original headers for the purpose of generating a response. This should be faster than re-capitalizing headers.

This should be the default behavior.

@FooBarWidget FooBarWidget self-assigned this May 20, 2015

FooBarWidget added a commit that referenced this issue Jun 1, 2015

FooBarWidget added a commit that referenced this issue Jun 1, 2015

Merge branch 'GH-1436' into GH-1436-master
Conflicts:
	ext/common/agents/HelperAgent/AdminServer.h
@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Jun 1, 2015

A fix for this issue is currently being tested.

FooBarWidget added a commit that referenced this issue Jun 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.