LukeMathWalker / zero-to-production

Code for "Zero To Production In Rust", a book on API development using Rust.
https://www.zero2prod.com
Apache License 2.0
5.43k stars 470 forks source link

login flash messages only work because you're talking to localhost #234

Open rlpowell opened 7 months ago

rlpowell commented 7 months ago

I was very confused when after switching to actix-web-flash-messages, both the login::an_error_flash_message_is_set_on_failure test and the functionality in question stopped working.

I eventually discovered that this is a side effect of my setup; I have host: in the local config set to 0.0.0.0 because I'm running my tests on my linux box but my browser is on my windows box (and publishing ports from a container doesn't work when the port is opened on loopback, at least in my podman config).

The specific issue is that actix-web-flash-messages sets Secure on the cookies, which the previous manual cookie solution did not. This appears to be important to you (see https://github.com/LukeMathWalker/actix-web-flash-messages/pull/6 ), but you should be aware it makes things weird in testing situations.

FWIW, the reason the test works with 127.0.0.1 is https://github.com/pfernie/cookie_store/blob/403db6b2bd5acb4844aa9ac5b10f909d273678ee/src/cookie_store.rs#L1552 , so, pretty hacky. :D

To get the browser to work I had to use SSH port forwarding so that I could go to http://localhost/ in the browser, even though it's not actually on the local host.

Fun side note: the version of curl in Debian bullseye also fails to be usable with these flash cookies, even on localhost, because https://github.com/curl/curl/commit/c495dcd02e885fc3f35164b1c3c5f72fa4b60c46

Anyway, having lost ~3 hours of my life to this, I thought you should be aware that it's a bit fragile.

rlpowell commented 7 months ago

I question whether flash messages actually need the Secure flag at all, but I haven't actually thought about the threat model there at all.

LukeMathWalker commented 7 months ago

It really depends on what information is travelling in those messages, which is highly application-specific. In our newsletter, we could downgrade them to be non-secure and we wouldn't really diminish our security posture.

Thanks for the investigation btw! It's probably worth to disable the Secure flag in our tests (and provide the necessary hooks in the flash message crate to support it). I'll keep it in mind for the next revision of the book while keeping this issue open for others that may bump into the same issues.

rlpowell commented 7 months ago

It is worth noting that the same issue exists with manually testing the admin dashboard at the end of 10.8.1. I was able to fix it like so:

    let server = HttpServer::new(move || {
        App::new()
            .wrap(message_framework.clone())
            .wrap(
                SessionMiddleware::builder(redis_store.clone(), secret_key.clone())
                    .cookie_secure(false)
                    .build(),
            )

Obviously not OK for production, but 🤷