http-rs
regenerating session don't send a new cookie
SessionMiddleware seems to ignore session regeneration. The middleware should reset the session cookie with a new id after session.regenerate(), but instead it continues to use the old cookie. This can be a security vulnerability because it allows session fixation attacks.
Here is a simple test case. The session id should change after logging in, but it doesn't.
use tide::sessions::{MemoryStore, SessionMiddleware};
use tide::{Redirect, Request, Response};
#[async_std::main]
async fn main() -> tide::Result<()> {
let mut app = tide::new();
app.with(SessionMiddleware::new(
MemoryStore::new(),
b"01234567890123456789012345678901",
));
app.at("/").get(index);
app.at("/login").get(login);
app.listen("127.0.0.1:8081").await?;
Ok(())
}
async fn index(req: Request<()>) -> tide::Result {
let session = req.session();
let user: String = session.get("user").unwrap_or("guest".to_string());
let mut resp: Response = format!(
r#"
Logged in as {}<br>
Session id: {}<br>
<a href="/login">Login</a>"#,
user,
session.id()
)
.into();
resp.set_content_type("text/html");
Ok(resp)
}
async fn login(mut req: Request<()>) -> tide::Result {
let session = req.session_mut();
session.regenerate();
session.insert("user", "admin")?;
Ok(Redirect::see_other("/").into())
}
Hi @Randati, the session is regenerated but tide don't send the new cookie on Redirect ( ping @yoshuawuyts, I think is ok to not doit but just to confirm ). If you send a Response you would see the change in the session id.
You can see the headers of the actual example code
req:
GET /login HTTP/1.1
Host: localhost:8081
Cookie: tide.sid=U%2Fq2s7AVUz8iigWrrJTK+iGTfDDzXn3nKhTeaBQ56BQ%3DQYJQNb7vi0+OuavD13EbjynrnFdlCDJbyuZyyX+Cbl3wnCUlJMZ+io+Q8O4uPaucyyyH5jSJ5gd96HrZ1ATxpQ%3D%3D
res:
HTTP/1.1 303 See Other
content-length: 0
date: Thu, 17 Dec 2020 14:42:43 GMT
location: /
@pepoviola odd, thanks for digging deeper into this. I would have expected the redirect still to have the right headers applied to it. This indeed registers as a bug, and we should make sure to have the right headers applied to it.
Hi @yoshuawuyts , thanks for the reply and clarification. I can check why we don't send the right headers. Thx!
Hi @yoshuawuyts, I take another look of this issue and the problem here is that we are regenerating the session but not the cookie id ( as @Randati mention ). So the current session is changed but when the pages reload is back to the original one since the cookie id isn't changed.
I tracked the code and look like we are cloning the session to set the request ext
request.set_ext(session.clone());
let mut response = next.run(request).await;
if session.is_destroyed() { (...)
But the we continue to use the same session and if the endpoint ( like in this test case ) change the session we don't know about that in the middleware. I think we need some kind of arc/mutex to allow mutate the session by the endpoint ( or other middleware ) but I'm not sure how is the best approach to implement that.
Related lines: https://github.com/http-rs/tide/blob/main/src/sessions/middleware.rs#L95-L97
Trace:
- first request ( session created and set cookie )
[2020-12-18T22:35:13Z INFO tide::log::middleware] <-- Request received
[2020-12-18T22:35:13Z TRACE async_session::memory_store] storing session by id `TCcNZEfNFVUVy8Jo3P0pUju0EHJC2z9F2Q2xL7J7/EU=`
[2020-12-18T22:35:13Z INFO tide::log::middleware] --> Response sent
- second request ( with session.regenerate call in endpoint )
[2020-12-18T22:35:20Z INFO tide::log::middleware] <-- Request received
[2020-12-18T22:35:20Z TRACE async_session::memory_store] loading session by id `TCcNZEfNFVUVy8Jo3P0pUju0EHJC2z9F2Q2xL7J7/EU=`
[2020-12-18T22:35:20Z TRACE async_session::session] regenerate call for session id `TCcNZEfNFVUVy8Jo3P0pUju0EHJC2z9F2Q2xL7J7/EU=`
[2020-12-18T22:35:20Z TRACE async_session::session] new session id `+mLrLlbMa6PRKl0uCpY4iCvh4VV59qa04uH0cjBU5kQ=`
[2020-12-18T22:35:20Z TRACE async_session::memory_store] storing session by id `TCcNZEfNFVUVy8Jo3P0pUju0EHJC2z9F2Q2xL7J7/EU=`
[2020-12-18T22:35:20Z INFO tide::log::middleware] --> Response sent
Thx!
Here's a hacky temporary workaround which seems to do the job (replace the location with where you need to redirect):
req.session_mut().regenerate();
req.session_mut().insert("user", "admin")?;
let mut resp: tide::Response = r#"
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Document</title>
<script>window.location.href="/"</script>
</head>
</html>
"#
.into();
resp.set_content_type(mime::HTML);
Ok(resp)
Hi @jlkiri, thanks for the workaround. I just changed the issue title since the bug is that we are not sending a new cookie when we call session.regenerate. The fix for this is still in progress.
Thx!
I'm facing same issue, so implemeted some workaround like below. It seems to work as intended.
/// A workaround for tide issue #762.
/// https://github.com/http-rs/tide/issues/762
pub trait SessionWorkaroundExt {
/// Session key of regeneration flag.
const REGENERATION_MARK_KEY: &'static str;
/// Marks the session for ID regeneration.
fn mark_for_regenerate(&mut self);
/// Checks whether the session should regenerate the ID.
/// The session key `REGENERATION_MARK` will be removved.
fn should_regenerate(&mut self) -> bool;
}
impl SessionWorkaroundExt for Session {
const REGENERATION_MARK_KEY: &'static str = "sid-regenerate";
fn mark_for_regenerate(&mut self) {
self.insert(Self::REGENERATION_MARK_KEY, true).expect("Boolean should be serialized");
}
fn should_regenerate(&mut self) -> bool {
let previously_changed = self.data_changed();
let regenerate = self.get(Self::REGENERATION_MARK_KEY).unwrap_or_default();
self.remove(Self::REGENERATION_MARK_KEY);
if !previously_changed {
self.reset_data_changed();
}
regenerate
}
}
In SessionStore::store_session():
if session.should_regenerate() {
session.regenerate();
}
In endpoints:
request.session_mut().mark_for_regenerate();
