Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unset client cookies on session destroy #4

Merged
merged 2 commits into from
Jul 25, 2022
Merged

Unset client cookies on session destroy #4

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dac90b
On session destroy unset the cookie for the client.
Ptrskay3 Jul 25, 2022
73cf0df
style: consistent ordering
Ptrskay3 Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ async-session = "3.0.0"
futures = "0.3.21"
tower = "0.4.12"
http-body = "0.4.5"
tracing = "0.1"

[dependencies.axum]
version = "0.5.7"
Expand Down
59 changes: 39 additions & 20 deletions src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use async_session::{
use axum::{
http::{
header::{HeaderValue, COOKIE, SET_COOKIE},
Request,
Request, StatusCode,
},
response::Response,
};
Expand Down Expand Up @@ -166,6 +166,20 @@ impl<Store: SessionStore> SessionLayer<Store> {
cookie
}

fn build_removal_cookie(&self, secure: bool) -> Cookie<'static> {
let mut cookie = Cookie::build(self.cookie_name.clone(), "")
.http_only(true)
.same_site(self.same_site_policy)
.secure(secure)
.finish();

cookie.make_removal();

self.sign_cookie(&mut cookie);

cookie
}

// the following is reused verbatim from
// https://github.com/SergioBenitez/cookie-rs/blob/master/src/secure/signed.rs#L33-L43
/// Signs the cookie's value providing integrity and authenticity.
Expand Down Expand Up @@ -266,26 +280,31 @@ where
let mut response = inner.call(request).await?;

if session.is_destroyed() {
session_layer
.store
.destroy_session(session)
.await
.expect("Could not destroy session.");
if let Err(e) = session_layer.store.destroy_session(session).await {
tracing::error!("Failed to destroy session: {:?}", e);
*response.status_mut() = StatusCode::INTERNAL_SERVER_ERROR;
}

let removal_cookie = session_layer.build_removal_cookie(session_layer.secure);

response.headers_mut().insert(
SET_COOKIE,
HeaderValue::from_str(&removal_cookie.to_string()).unwrap(),
);
} else if session_layer.save_unchanged || session.data_changed() {
let cookie = session_layer
.store
.store_session(session)
.await
.expect("Could not store session.")
.map(|cookie_value| {
session_layer.build_cookie(session_layer.secure, cookie_value)
});

if let Some(cookie) = cookie {
response.headers_mut().insert(
SET_COOKIE,
HeaderValue::from_str(&cookie.to_string()).unwrap(),
);
match session_layer.store.store_session(session).await {
Ok(Some(cookie_value)) => {
let cookie = session_layer.build_cookie(session_layer.secure, cookie_value);
response.headers_mut().insert(
SET_COOKIE,
HeaderValue::from_str(&cookie.to_string()).unwrap(),
);
}
Ok(None) => {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to handle this branch. I suppose due to our setup, this basically never happens.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Ptrskay3 Ptrskay3 Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Err(e) => {
*response.status_mut() = StatusCode::INTERNAL_SERVER_ERROR;
tracing::error!("Failed to reach session storage: {:?}", e);
}
}
}

Expand Down