Updating the handling of Set-Cookie in http responses #121

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@dwnichols
Contributor

dwnichols commented Jan 30, 2012

Two small changes to more accurately handle Set-Cookie.

First commit, to handle a response with two cookies in the response header. Commit comment:
When an http response has two Set-Cookie in the header, response['Set-Cookie'] will return a single string. This string will be the two returned cookies separated by a comma, concatenated together. Instead this should use get_fields('Set-Cookie') which returns an array of returned cookie strings.

Second commit to allow '=' within a cookie value. Commit comment:
The syntax of cookies is described in http://www.ietf.org/rfc/rfc2109.txt, section 4.1. Cookies are attribute-value pairs, so split should only return 2 values. This will allow '=' within the value of a cookie.

dwnichols added some commits Jan 30, 2012

When an http response has two Set-Cookie in the header, response['Set…
…-Cookie'] will return a single string. This string will be the two returned cookies separated by a comma, concatenated together. Instead this should use get_fields('Set-Cookie') which returns an array of returned cookie strings.
The syntax of cookies is described in http://www.ietf.org/rfc/rfc2109…
….txt, section 4.1. Cookies are attribute-value pairs, so split should only return 2 values. This will allow '=' within the value of a cookie.
@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Apr 12, 2012

Owner

Sorry for overlooking this. Looks good, just needs some specs. If you could add them, I'll happily pull.

Owner

jnunemaker commented Apr 12, 2012

Sorry for overlooking this. Looks good, just needs some specs. If you could add them, I'll happily pull.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 7, 2012

Owner

Closing. Please re-open if you add specs and get it working. Thanks!

Owner

jnunemaker commented Sep 7, 2012

Closing. Please re-open if you add specs and get it working. Thanks!

@jnunemaker jnunemaker closed this Jan 1, 2013

@azurewraith

This comment has been minimized.

Show comment Hide comment
@azurewraith

azurewraith Jan 9, 2013

+1, ran into this today.

+1, ran into this today.

@farleyknight

This comment has been minimized.

Show comment Hide comment
@farleyknight

farleyknight Jan 21, 2013

+1 Same here.

+1 Same here.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Jan 21, 2013

Owner

I'm more interested in a pull request that I can maintain than +1's right now, but thanks for letting me know. If either of you would like to tackle, I'd love it.

Owner

jnunemaker commented Jan 21, 2013

I'm more interested in a pull request that I can maintain than +1's right now, but thanks for letting me know. If either of you would like to tackle, I'd love it.

@dwnichols

This comment has been minimized.

Show comment Hide comment
@dwnichols

dwnichols Jun 11, 2013

Contributor

Oddly, I am not able to reopen this or push directly to the above commits, so here's updated commits with tests.

bd32f52
45fdfd8

Contributor

dwnichols commented Jun 11, 2013

Oddly, I am not able to reopen this or push directly to the above commits, so here's updated commits with tests.

bd32f52
45fdfd8

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Jun 24, 2013

Owner

Can you just open a new pull request? Thanks!

Owner

jnunemaker commented Jun 24, 2013

Can you just open a new pull request? Thanks!

@dwnichols

This comment has been minimized.

Show comment Hide comment
@dwnichols

dwnichols Jun 26, 2013

Contributor

Not a problem! See pull request 218

Contributor

dwnichols commented Jun 26, 2013

Not a problem! See pull request 218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment