Multiple Set-Cookie headers in response

[nt0xa]

Here is small example:

package main

import (
	"net/http"

	"github.com/alexedwards/scs"
)

var sessionManager = scs.NewCookieManager("u46IpCV9y5Vlur8YvODJEhgOY8m9JVE4")

func main() {
	mux := http.NewServeMux()
	mux.HandleFunc("/auth", login)

	http.ListenAndServe(":4000", sessionManager.Use(mux))
}

func login(w http.ResponseWriter, r *http.Request) {
	session := sessionManager.Load(r)
        
        // authenticate user ...

	err := session.RenewToken(w)
	if err != nil {
		http.Error(w, err.Error(), 500)
                return
	}

	if err := session.PutInt(w, "userId", 1); err != nil {
		http.Error(w, err.Error(), 500)
		return
	}

	if err := session.PutBool(w, "isAdmin", true); err != nil {
		http.Error(w, err.Error(), 500)
		return
	}
}

Here is /auth response:

HTTP/1.1 200 OK
Set-Cookie: session=rXoTm-uiZM8eYszKWKTbUP-D-SBV0Pdx8DyMpX7jL55yVcOwRxLROJSeDHeuW0iYifwVpUnEiXyhU_H-Vl-1-2LpnjHnOvx1TS1yYuoccQP6P56iEXyCzngQRt_UkHGG5Wva5-0; Path=/; HttpOnly
Set-Cookie: session=6HjYzvYsALD_UvkBNlrheCKijlQDAAd2rMsWN_URq7uO12n2ng2t-7PYSmHSV8l3n0TKtb6_y0-DKuc--uikxCBJ-NuvR0a91vj7dauPu_TMrCGtfa1cOgLpr1R2MhTCDt4qUNwmBNEblJrViAiW; Path=/; HttpOnly
Set-Cookie: session=ewJmYE3psbCYtX39Zgj-Utv5XTjCJ5SfbsO32daXPyOovU0y0O2OPQtv6QlL9Zv-yZ-XJuYqPvkZQ5tt_tjLqtpKdvohTOLAjLp7XO9yfVjV5rgtC6hG5b9W0Hb_8shOUdZdKZdM936IEAbaRkltlDOEYqBSbFcXoZIjJUKs; Path=/; HttpOnly
Date: Thu, 26 Oct 2017 09:08:01 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8

As you can see there are three different Set-Cookie headers with the same cookie-name in response which is wrong according to RFC 6265:

Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name.

[alexedwards]

Thanks.

I've pushed a fix in this commit: c052ae9

The response for your example now is:

$ curl -I localhost:4000/auth
HTTP/1.1 200 OK
Set-Cookie: session=K1v4RXFtITKE0Q1GsQmmQrGwWn7iLULhq-sFXEQD0t0umOl14hioyBWx_bg8NTlRrMJ2NlVMMUh0MNO97G4918JIjzr0YciqO2Vv6kFbBFuWWdQmxGtT_2Gw5kpieUOWZZH_qK__x0XRd4jl2h6AesnY15QiPUutDQYiJO3v; Path=/; HttpOnly
Date: Thu, 26 Oct 2017 11:00:34 GMT
Content-Type: text/plain; charset=utf-8

[nt0xa]

Thank you!

[flisky]

I don't think it's the right way to fix this problem.
write is the most expensive method in this project, but we call it at every change.

I propose we would expose write function, and get rid of the first ResponseWriter parameter for all remaining API. People call Write manually after they have done all the changes.

However, it's such an overhaul. Any thoughts, @alexedwards ?

[alexedwards]

@flisky It's a difficult choice. Making the saving of session data manual, via an exported Write or Save method would improve performance, but it reduces convenience.

Previous versions of SCS used to call write from the middleware, which was great because it meant data was automatically saved and the write only happened once, after all the changes. The downside was it didn't work properly with frameworks which mung http.ResponseWriter (like Echo), and it was also a bit awkward for unit testing handlers.

I haven't thought it through properly, but perhaps having an AutoSave option is a possibility. So when set to true, SCS works like it currently does. But if set to false, the user must remember to call an exposed Write or Save method manually.

[dhui]

I agree with @flisky in that modifying session data should be separate from storing/saving session data. For any non-cookie store, the performance impact adds up significantly since each IO-bound round trip is expensive.

I like the idea of using middleware to save the session. Would providing a separate middleware for non-http.Handler compatible middleware fix the autosaving problem? Echo provides a middleware wrapper that looks like it should work: https://godoc.org/github.com/labstack/echo#WrapMiddleware What were the specific issues that were encountered?

Also, implementing an autosave option seems like it will be messy since the session lifecycle management be split between the manager and session.

[dhui]

Also, any plans to push a new release with this commit: c052ae9 ?

I'd like to pin to a version of scs instead of relying on the master branch

[alexedwards]

@dhui Done https://github.com/alexedwards/scs/releases/tag/v1.2.0

[alexedwards]

I've opened a separate issue to discuss whether modifying session data should be separate from storing/saving session data: #36