gobuffalo/buffalo

Session Cookie set twice in response headers

Closed this issue · 10 comments

saurori commented

Description

It looks like the Session Cookie is set twice in the response headers, and it is not the same value.

Steps to Reproduce the Problem

$ buffalo new coke
...
$ buffalo dev
...
$ curl -i localhost:3000             
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
Set-Cookie: _coke_session=MTYyMjg0ODIzOHxEdi1CQkFFQ180SUFBUkFCRUFBQU92LUNBQUVHYzNSeWFXNW5EQTRBREhKbGNYVmxjM1J2Y2w5cFpBWnpkSEpwYm1jTUZnQVVOVEEyWkdWaFlUazJPVGRrWVdKaU5UUXhPVGs9fH-MqWSmwpvvEGNKRnJBmQs1hgFfkOkbHfYtow7k8GAw; Path=/; Expires=Sun, 04 Jul 2021 23:10:38 GMT; Max-Age=2592000; HttpOnly
Set-Cookie: _coke_session=MTYyMjg0ODIzOHxEdi1CQkFFQ180SUFBUkFCRUFBQV82UF9nZ0FEQm5OMGNtbHVad3dPQUF4eVpYRjFaWE4wYjNKZmFXUUdjM1J5YVc1bkRCWUFGRFV3Tm1SbFlXRTVOamszWkdGaVlqVTBNVGs1Qm5OMGNtbHVad3dVQUJKaGRYUm9aVzUwYVdOcGRIbGZkRzlyWlc0SFcxMTFhVzUwT0FvaUFDRGRUdW1HY0V3b3FicVVISlpWVzhSRTIybWJjUUlSeEJDNkQyN1dqbWZYdUFaemRISnBibWNNQ1FBSFgyWnNZWE5vWHdkYlhYVnBiblE0Q2dRQUFudDl8s8JVDn8xNHPJmBatfG1XrRV993BtGnvEAWhaSGsUbRI=; Path=/; Expires=Sun, 04 Jul 2021 23:10:38 GMT; Max-Age=2592000; HttpOnly
Date: Fri, 04 Jun 2021 23:10:38 GMT
Transfer-Encoding: chunked

Expected Behavior

The response headers set the Session Cookie once (one header).

Actual Behavior

The response has two Set-Cookie headers for the Session.

Info

  • Buffalo version is: v0.16.23
  • go version go1.16.4 darwin/amd64
github-actions commented

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

saurori commented

Still relevant on:

  • Buffalo version is: v0.17.2
  • go version go1.16.6 darwin/amd64
saurori commented

I believe I've narrowed this down but someone more familiar with Session stores would have to confirm this behavior:

It appears that the duplicate session cookie is caused by this call to Session().Save() before session _flash_ is set (causing a difference):
https://github.com/gobuffalo/buffalo/blob/v0.17.2/request_logger.go#L45

I'm wondering if this session save is necessary in the RequestLogger since the sessionSaver middleware is already built in:
https://github.com/gobuffalo/buffalo/blob/v0.17.2/app.go#L79

paganotoni commented

Hey @saurori this makes total sense to me. Would you want to work on a PR for this one ? Thanks for the research you've done so far.

saurori commented

@paganotoni sure, I'll try and put together a PR tomorrow. It may just be a one-liner but I want to test a few more things.

  • 👍1
saurori commented

@paganotoni It seems that removing the c.Session().Save() in request_logger.go only masks the real problem. When trying to write a test for this fix, I noticed the session cookie was set 4 times in a normal request. Every time c.Session().Save() is called, if the session data has changed, a new Set-Cookie header is generated by gorilla/sessions.

I believe Session().Save() is only meant to be called once right before the response is written, likely in the default_context Render method about here. But Session().Save() is called in multiple places, including the flash persist.

I can make the above changes so session saving occurs once (assumes sessions only work with default auto renderer). However, since the function is exported, anyone can call c.Session().Save() from an arbitrary app handler and cause a duplicate session cookie. So to really fix the problem I would think the session Save() would have to not be exported (breaks API) or modified in some way.

So I'm not 100% sure how to proceed. Thoughts?

sio4 commented

Just FYI, if I ran the curl with a valid cookie, it just returns a single cookie as expected (while it returns two lines when I omit the cookie).

$ curl -v -H "Cookie: _coke_session=MTYyO...Ko3Eag=" localhost:3000
*   Trying 127.0.0.1:3000...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.68.0
> Accept: */*
> Cookie: _coke_session=MTYyO...Ko3Eag=
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/html; charset=utf-8
< Set-Cookie: _coke_session=MTYyO...BntLi4=; Path=/; Expires=Sat, 25 Sep 2021 11:55:55 GMT; Max-Age=2592000; HttpOnly
< Date: Thu, 26 Aug 2021 11:55:55 GMT
< Transfer-Encoding: chunked
<
saurori commented

PR for this (for notifications): #2193

saurori commented

Fixed in 0.18.3

paganotoni commented

Thanks for taking care of this one @saurori 👏 👏