Admin panel login page doesn't work on WebKit (Safari, GNOME Web)

[m4tx]

When using Safari or GNOME Web (or likely any other Webkit-based web browser) the admin panel login page doesn't do anything useful when valid credentials are provided—instead, it just redirects back to the login page. This can be verified by using the admin example: https://github.com/cot-rs/cot/tree/master/examples/admin

The reason for that is that the Set-cookie header that is used to send the Session ID to the user has the Secure property set. Contrary to other popular browsers (Chrome/Firefox) and what MDN says, Webkit doesn't treat localhost in any special case and just ignores the Set-cookie header when HTTPS is not used on localhost.

One way to fix this would be to skip making the cookie Secure in the development mode. This would require creating a new config key to control this. It should still be true by default so that we have a sensible value for production systems.

---

Relevant WebKit bug reports:

https://bugs.webkit.org/show_bug.cgi?id=232088
https://bugs.webkit.org/show_bug.cgi?id=218980

[vsagar25]

@m4tx - Thanks for looking into this

[ElijahAhianyo]

Hi @m4tx, Rust beginner here, I would like to take a stab at this with some guidance.

I skimmed through the code and setting with_secure(false) in SessionManagerlayer here as a test fixed the issue on safari for me.

Regarding your suggested approach of introducing a new config key to control this behavior in development mode, where do you envision this config setting should reside? Should it be part of ProjectConfig or SessionMiddleware?

If it belongs in ProjectConfig, would the existing debug flag be sufficient, given that debug=true is typically interpreted as a development environment? My initial thought is to infer the environment from the existing config(without setting an additional flag), since at this point(

cot/cot/src/project.rs

Line 1119 in 382ed4d

let handler = self.project.middlewares(handler, &self.context);

) we already have access to that information.

Alternatively, I wonder if we could make the SessionMiddleware API more flexible by wrapping it around the SessionManagerLayer provided by tower. This would allow users to configure additional settings beyond just the secure flag..
something along the lines of :

pub struct SessionManager {
   // we can modify the signature to support other stores like `CachingSessionStore` if we want although I'm not sure if we want to or how best to go about it
    inner: SessionManagerLayer<MemoryStore>,
}

impl SessionManager {
    pub fn new() -> Self {
        let store = MemoryStore::default();
        let layer = SessionManagerLayer::new(store);
        Self { inner: layer }
    }

    pub fn with_secure(mut self, secure: bool) -> Self {
        self.inner = self.inner.with_secure(secure);
        self
    }

    // Other configuration methods would be added similarly.
    // For example:
    // pub fn with_name(mut self, name: impl Into<Cow<'static, str>>) -> Self {
    //     self.inner = self.inner.with_name(name);
    //     self
    // }

    pub fn layer<S>(self, inner: S) -> <SessionManagerLayer<MemoryStore> as tower::Layer<S>>::Service {
        self.inner.layer(inner)
    }
}

Let me know what you think

[m4tx]

@ElijahAhianyo hey!

Regarding your suggested approach of introducing a new config key to control this behavior in development mode, where do you envision this config setting should reside? Should it be part of ProjectConfig or SessionMiddleware?

If it belongs in ProjectConfig, would the existing debug flag be sufficient, given that debug=true is typically interpreted as a development environment? My initial thought is to infer the environment from the existing config(without setting an additional flag), since at this point(

cot/cot/src/project.rs

Line 1119 in 382ed4d
let handler = self.project.middlewares(handler, &self.context);
) we already have access to that information.

I think it could still be useful to be able to disable Secure cookies even for production mode—potentially for some services meant to be running in intranets where HTTPS is not needed. Hence I think it should be a separate config variable, rather than it being tied to the debug mode.

I think we could create a new structure inside ProjectConfig and call it SessionConfig or something similar. We could have something like this in the TOML config:

[session]
cookie_secure = false

Later, we could other options to this as well.

// we can modify the signature to support other stores like CachingSessionStore if we want although I'm not sure if we want to or how best to go about it
inner: SessionManagerLayer,

Yeah, supporting other memory stores is definitely something we'll want to have very soon—memory store is unsuitable for production systems, because it's not persistent across server restarts. I think we can just use dyn SessionStore and construct the exact store depending on the config, but this is definitely outside the scope of this issue.

pub fn with_secure(mut self, secure: bool) -> Self {
    self.inner = self.inner.with_secure(secure);
    self
}

// Other configuration methods would be added similarly.
// For example:
// pub fn with_name(mut self, name: impl Into<Cow<'static, str>>) -> Self {
//     self.inner = self.inner.with_name(name);
//     self
// }

Yeah, that sounds reasonable. I think we should aim to have something similar to LiveReloadMiddleware: a new() function that constructs a session store with default settings (in this case we can have methods like with_secure() as you suggest, or just secure() to modify specific settings) and from_context() which would read the config from the config structure.

[seqre]

I think we could create a new structure inside ProjectConfig and call it SessionConfig or something similar. We could have something like this in the TOML config:

[session]
cookie_secure = false

I agree, however, I'd consider placing all middleware configurations one level deeper, eg.

[middleware]
[middleware.session]
cookie_secure = false

This will ensure that some random middleware configuration won't clash with Cot's settings.

[ElijahAhianyo]

@m4tx @seqre I raised a draft PR here(#251) which I had some initials concerns on, feel free to leave a comment when you can