Possible minor security issue with client session handling

[meteficha]

I was reviewing the client session code and this caught my eye:

    sess date = Map.unions $ do
      raw <- [v | (k, v) <- W.requestHeaders req, k == "Cookie"]
      val <- [v | (k, v) <- parseCookies raw, k == sessionName]
      let host = "" -- fixme, properly lock sessions to client address
      maybe [] return $ decodeClientSession key date host val

If I'm understanding the code correctly, we're:

• Taking all cookie headers.
• Taking all cookies with the target name (default _SESSION).
• Decoding them all separately.
• Merging the contents of all of them.

As the code has almost no comments, it's not clear if the merging is intended behavior or just a convenient way that works for the common cases (namely, zero and one). However, it's certainly unexpected for me to find this behavior, and I have already looked at the snippet before.

The clientsession library guarantees that the session data was constructed by the server at a point in the past, and that the server may trust it. The behavior above allows one to take unions of any two or more valid sessions.

On most applications this should be harmless as the last session is going to override most of the earlier ones. However, if some application in the wild maintained some invariant between session variables, or conditionally added some session variables, they may not be prepared to handle these unions.

As I cannot imagine how someone would be using this feature, I assume it's a misfeature. I propose that we force at most one session cookie to be present. In other words, if more than one session cookie is found, ignore them all.

[snoyberg]

I think you're correct, and making that change seems correct to me.

[snoyberg]

@meteficha Are you able to implement this change?

[meteficha]

Yes, adding to my todo list.