Unset client cookies on session destroy #4
Conversation
Other improvements: avoid panic on session storage failure, add tracing to error cases
| HeaderValue::from_str(&cookie.to_string()).unwrap(), | ||
| ); | ||
| } | ||
| Ok(None) => {} |
There was a problem hiding this comment.
I'm not sure how to handle this branch. I suppose due to our setup, this basically never happens.
There was a problem hiding this comment.
I wonder we could reach this branch if we were to clear the storage (perhaps manually, as an example) while clients still hold cookies. If that's a valid case, would it make sense to send the removal cookie here too?
There was a problem hiding this comment.
I've looked into this a little more thoroughly:
Any reasonable session store implementation should call into_cookie_value on async_session::Session. The only way for into_cookie_value to return None is when you explicitly call clone on the session. However, I can imagine some degenerate async_session implementation that breaks this somehow. I don't believe that we should send a removal cookie here.
|
This looks great! Thank you for putting it together. I have one question about the branch you mentioned but I'm happy to merge this as-is. |
|
Version |
I think it's reasonable to unset cookies at client side whenever the session is destroyed. (At least actix-sessions does this.)
Also, in my opinion it's a much nicer experience to avoid panics, because that causes the response to hang and never arrive. Instead, on storage failure we should send back a 500 INTERNAL_SERVER_ERROR. Also added tracing errors to appropriate places.
Let me know what do you think about these changes.