ohler55 / agoo

A High Performance HTTP Server for Ruby
MIT License
911 stars 39 forks source link

New lines in header values #65

Closed evanleck closed 5 years ago

evanleck commented 5 years ago

I've run into a problem running Agoo behind an HTTP/2 capable load balancer where both Safari and curl throw a protocol error when new line characters \n are present in header values, specifically the Set-Cookie header in my case. Normally, I'd use a middleware to transform the values on the way out, but the problem is with Rack and, I think, Agoo's conformance to Rack's spec.

Rack's spec says:

The values of the header must be Strings, consisting of lines (for multiple header values, e.g. multiple Set-Cookie values) separated by “\n”.

Apparently, it's up to the handler to transform the lines into proper HTTP headers, without the new lines. There's a comment toward the bottom of that issue that illustrates what I mean and the next comment, from a Rack contributor, confirms that's what Rack expects to happen.

I can put together a working example if you'd like me to.

ohler55 commented 5 years ago

Let me look into it. Agoo may not be converting the \n to a ; on the way out. It is in the outgoing direction you are referring to, right?

evanleck commented 5 years ago

It's outgoing, but it shouldn't be converting \n to a ;. It should convert e.g. this:

Set-cookie:foo=bar;
foo2=bar2;
foo3=bar3;

Into this:

Set-Cookie:foo=bar;
Set-Cookie:foo2=bar2;
Set-Cookie:foo3=bar3;
ohler55 commented 5 years ago

I believe this is also conformant:

Set-Cookie:foo=bar;foo2=bar2;foo3=bar3

Are there some issues with that approach that I'm not aware of?

evanleck commented 5 years ago

Yeah, it's not conformant because semicolons are used as directive separators in the Set-Cookie header (the MDN page has some examples).

Other HTTP headers (e.g. the Link header) implement a "list" style where values can be separated with commas, but since the e.g. Expires directive contains a comma, Set-Cookie can't use that format either. Per RFC 6265, section 3:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into a single header field. The usual mechanism for folding HTTP headers fields (i.e., as defined in RFC2616) might change the semantics of the Set-Cookie header field because the %x2C (",") character is used by Set-Cookie in a way that conflicts with such folding.

Based on that, it's my understanding that multiple Set-Cookies are needed.

ohler55 commented 5 years ago

Fair enough. I'll take that approach.

evanleck commented 5 years ago

Awesome, thank you so much!

ohler55 commented 5 years ago

If it can wait a week or two I'd like to release with the new feature I'm working on of GraphQL subscription support. I'll have the fix in this weekend if you can live off the develop branch until the next release.

evanleck commented 5 years ago

No problem, I can wait. Thanks again

evanleck commented 5 years ago

It looks like this was fixed with 6228b648a2d7c3d45f7d777ebeba2e0dbf37c1e2 and release with 2.10.0, yeah?

ohler55 commented 5 years ago

Yes indeed.

evanleck commented 5 years ago

You're the man, thank you!