Store.Get permits invalid cookie names

[carbocation]

I encountered this problem in the following fashion:

func Session(r *http.Request) *sessions.Session {
    sess, err := sessionStore.Get(r, "invalid cookie name")
    if err != nil {
        panic(err)
    }

    return sess
}

I could not retrieve session data saved in this way. It turns out that spaces are disallowed in cookie names[1] - and Gorilla's session store name directly corresponds with a cookie name. Removing the spaces (in this case, converting "invalid cookie name" into "invalidcookiename") solves the problem.

I would propose either that internally, this method convert the value into a valid cookie name (suboptimal because then it doesn't reflect what the creator passed it) or that it trigger an error if an invalid cookie name gets passed to it.

1 = http://stackoverflow.com/a/1969339/199475

[kisielk]

I'm not sure the sessions package is the right place to tackle this, it seems like a bit of a rabbit hole since the behaviour is browser-dependant. In my case I was able to actually set the cookie in Chrome, and it even sends it back to the server. It seems the net/http package ignores it when reading cookies because it fails the isCookieNameValid test in the parser, which checks if the cookie name characters are all valid tokens. Maybe the net/http package should use the same check when setting cookies as well? It seems odd to be allowed to set cookies that you can't read back. I think it would be worthwhile to bring it up in a Go issue and see what the devs say.

[carbocation]

@bradfitz raises the point that the go compatibility guarantee prevents changing the function signature for setting cookies in net/http. Therefore, while the most ideal place for fixing this problem would be in the standard library, it cannot happen there.

[kisielk]

Makes sense. So one unfortunate thing about the design of this package is that Get is just an interface. We could fix this for all the stores in this package but it would still affect third-party stores of which there are many.

[kisielk]

Going to close this as wontfix for now, I don't really see a clean way of doing it with the current programming model.

If a future version of this package is created, I think it would be best implemented as having one top-level type that calls down to the stores instead of the user of the package doing so directly. That could be used to solve issues like this and #48 in a better way.