Crate fails with tower_sessions_redis_store

[Elizafox]

• I have looked for existing issues (including closed) about this

Bug Report

Version

0.15.0

Platform

• Darwin ember.local 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:17:35 PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T8112 arm64
• FreeBSD river.z6a.info 13.2-RELEASE-p1 FreeBSD 13.2-RELEASE-p1 GENERIC amd64

Crates

• tower_sessions_redis_store (version 0.12.0)
• tower_sessions (version 0.12.1)

Description

Attempting to call AuthSession.login() with the tower_sessions_redis_store backend fails with:

2024-04-01T03:20:06.607300Z ERROR call:call:call:save: tower_sessions_core::session: /Users/elizabeth/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-sessions-core-0.12.1/src/session.rs:648: error=Parse Error: Could not convert to bool.
2024-04-01T03:20:06.607318Z ERROR call:call:call: tower_sessions::service: /Users/elizabeth/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-sessions-0.12.1/src/service.rs:244: failed to save session err=Parse Error: Could not convert to bool.

This does not happen with at least the Moka backend.

This is a regression, as it worked with prior versions.

[maxcountryman]

So far, I'm not able to reproduce this. I've tried to modify the sqlite example to use RedisStore and it seems to work as expected. Here's a diff:

diff --git a/examples/sqlite/Cargo.toml b/examples/sqlite/Cargo.toml
index ae87964..64bb358 100644
--- a/examples/sqlite/Cargo.toml
+++ b/examples/sqlite/Cargo.toml
@@ -20,8 +20,8 @@ time = "0.3.30"
 tokio = { version = "1.34.0", features = ["full"] }
 tower = "0.4.13"
 tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
-tower-sessions = { version = "0.12.0", default-features = false, features = [
+tower-sessions = { version = "0.12.1", default-features = false, features = [
     "signed",
 ] }
-tower-sessions-sqlx-store = { version = "0.12.0", features = ["sqlite"] }
+tower-sessions-redis-store = { version = "0.12.0" }
 thiserror = "1.0.56"
diff --git a/examples/sqlite/src/web/app.rs b/examples/sqlite/src/web/app.rs
index 70e72ff..cf8aebc 100644
--- a/examples/sqlite/src/web/app.rs
+++ b/examples/sqlite/src/web/app.rs
@@ -1,14 +1,16 @@
 use axum_login::{
     login_required,
-    tower_sessions::{ExpiredDeletion, Expiry, SessionManagerLayer},
+    tower_sessions::{Expiry, SessionManagerLayer},
     AuthManagerLayerBuilder,
 };
 use axum_messages::MessagesManagerLayer;
 use sqlx::SqlitePool;
 use time::Duration;
-use tokio::{signal, task::AbortHandle};
 use tower_sessions::cookie::Key;
-use tower_sessions_sqlx_store::SqliteStore;
+use tower_sessions_redis_store::{
+    fred::{clients::RedisPool, interfaces::ClientLike, types::RedisConfig},
+    RedisStore,
+};
 
 use crate::{
     users::Backend,
@@ -32,14 +34,12 @@ impl App {
         //
         // This uses `tower-sessions` to establish a layer that will provide the session
         // as a request extension.
-        let session_store = SqliteStore::new(self.db.clone());
-        session_store.migrate().await?;
+        let pool = RedisPool::new(RedisConfig::default(), None, None, None, 6)?;
+
+        let redis_conn = pool.connect();
+        pool.wait_for_connect().await?;
 
-        let deletion_task = tokio::task::spawn(
-            session_store
-                .clone()
-                .continuously_delete_expired(tokio::time::Duration::from_secs(60)),
-        );
+        let session_store = RedisStore::new(pool);
 
         // Generate a cryptographic key to sign the session cookie.
         let key = Key::generate();
@@ -65,36 +65,10 @@ impl App {
         let listener = tokio::net::TcpListener::bind("0.0.0.0:3000").await.unwrap();
 
         // Ensure we use a shutdown signal to abort the deletion task.
-        axum::serve(listener, app.into_make_service())
-            .with_graceful_shutdown(shutdown_signal(deletion_task.abort_handle()))
-            .await?;
+        axum::serve(listener, app.into_make_service()).await?;
 
-        deletion_task.await??;
+        redis_conn.await??;
 
         Ok(())
     }
 }
-
-async fn shutdown_signal(deletion_task_abort_handle: AbortHandle) {
-    let ctrl_c = async {
-        signal::ctrl_c()
-            .await
-            .expect("failed to install Ctrl+C handler");
-    };
-
-    #[cfg(unix)]
-    let terminate = async {
-        signal::unix::signal(signal::unix::SignalKind::terminate())
-            .expect("failed to install signal handler")
-            .recv()
-            .await;
-    };
-
-    #[cfg(not(unix))]
-    let terminate = std::future::pending::<()>();
-
-    tokio::select! {
-        _ = ctrl_c => { deletion_task_abort_handle.abort() },
-        _ = terminate => { deletion_task_abort_handle.abort() },
-    }
-}

[Elizafox]

This issue is utterly baffling to me.

I thought I traced it down to my CSRF code (and I don't know why my CSRF code would cause this, because it's really simple), which inserts tokens and a timestamp into the session at a key. I removed everywhere it touches the session, and login works. But now logout consistently has the same problem, and I am at a loss as to why. I can't reproduce it in the example crate, either... I'm honestly completely baffled. I don't even know where bool could be coming from in this context.

[maxcountryman]

I suspect the bool is from here. But I can't explain why it's not reproducible with the example.

What version of Redis are you using?

[Elizafox]

I suspect the bool is from here. But I can't explain why it's not reproducible with the example.

What version of Redis are you using?

7.2.4

[Elizafox]

If you want to give reproduction a spin, you can check out my app at https://github.com/Elizafox/shadyurl-rust

Maybe it'll help debug the issue?

[maxcountryman]

I seem to be stuck with: "Bad Request: No CSRF token" on the log in page. I set a generated token, but nonetheless I see the same error message.

[maxcountryman]

I'm not exactly sure why this worked before and why it's not working now, but I noticed something in trying to debug the CSRF token issue: this line is redundant (spoiler: if you comment it out, it'll address the error you reported).

In order to get Safari to set a cookie on localhost, I needed to set with_secure to false, so after doing so I was able to see the bool error you originally reported.

However, it occurred to me that setting the session layer twice might be problematic (axum-login's middleware is a service of type CookieManager<SessionManager<AuthManager<...>, ...>>, i.e. the session manager is already installed by it).

I'm not sure if it makes sense to install the session manager twice over the same graph of routes, but nothing stops us from doing so.

[Elizafox]

Huh! I didn't know that was redundant. Well, that fixes the problem.

Thanks for helping me debug this :).

I have no idea why CSRF wouldn't work, that's odd. I can't reproduce it here. I'm also getting the same issues with keydb 6.3.4 btw.

[maxcountryman]

The CSRF issue root cause was Safari not setting a cookie at all because the server is not using encryption; in this case Safari saw Secure and refused to set the cookie knowing that localhost is not using SSL/TLS. So for local development, I usually have to set with_secure to false to coax Safari into setting the cookie like it should.

[maxcountryman]

I think I understand a bit more now about why this worked before: in updating the Redis store, I made a small, but important, semantic change to the save implementation to use XX. This is a safeguard against upstream bugs in tower-sessions and is not reflected in other stores for now.

In doing so, I did uncover a bug in cycle_id, patched in tower-sessions 0.12.1. However, this is also relevant to doubling the session manager layer. Basically what's happening is we're running the middleware twice and where previously it wasn't an issue to save without a key existing XX makes that an error (because create is the only safe way to create a session per the API semantics). In other words, we're saving when we should be creating a session.

Internally, tower-sessions will call SessionStore::create so long as it doesn't have a session ID. When it does have a session ID, then it invokes SessionStore::save. So, through an interaction between the two layers, we're ending up in a state where SessionStore::save is being called when it shouldn't; I wonder if cycle_id isn't the culprit.

To illustrate this, I augmented RedisStore locally to give us more details about why it's failing when we've installed SessionManager twice:

2024-04-02T23:46:33.601613Z ERROR tower_sessions_core::session: error=Tried to save a non-existing key: ag46eaCVBcKKU2C89Po0qg
2024-04-02T23:46:33.601642Z ERROR tower_sessions::service: failed to save session err=Tried to save a non-existing key: ag46eaCVBcKKU2C89Po0qg

Where the keys I have in my Redis are:

127.0.0.1:6379[1]> keys *
1) "ygjaFHHJEfYFNdERhNpgqQ"

Maybe this is a bug in tower-sessions but again I'm not sure if we should directly support multiple installations of the session manager like this.

[Elizafox]

Thank you so much for your help! I don't know if it's a bug, maybe it's a race?