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

New lines in header values #65

Closed
evanleck opened this issue Aug 21, 2019 · 11 comments
Closed

New lines in header values #65

evanleck opened this issue Aug 21, 2019 · 11 comments

Comments

@evanleck
Copy link

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
Copy link
Owner

ohler55 commented Aug 21, 2019

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
Copy link
Author

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
Copy link
Owner

ohler55 commented Aug 21, 2019

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
Copy link
Author

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
Copy link
Owner

ohler55 commented Aug 21, 2019

Fair enough. I'll take that approach.

@evanleck
Copy link
Author

Awesome, thank you so much!

@ohler55
Copy link
Owner

ohler55 commented Aug 21, 2019

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
Copy link
Author

No problem, I can wait. Thanks again

@evanleck
Copy link
Author

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

@ohler55
Copy link
Owner

ohler55 commented Aug 30, 2019

Yes indeed.

@evanleck
Copy link
Author

You're the man, thank you!

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