Remove cookie doesn't seem to work in sub-sub paths like '/foo/bar'

[Zerowalker]

I noticed that removing cookies didn't work for me, and after troubleshooting I think I have identified it as being related to the paths.
When you remove cookies, the remove part just sets the max_age and expired, but ignores the path.
This seems to work if it's just / or /foo, but not if there's any more paths for some reason.

Here's an example that doesn't work, based on the private/signed example,
it should create and remove the cookie every other turn, but it doesn't work.
Change the path to just / or /foo and it works.
I am aware i specify the path when building the cookie, but that should not break it, as that makes the remove "unstable".

use axum::{routing::get, Router};
use std::net::SocketAddr;
use std::sync::OnceLock;
use tower_cookies::{Cookie, CookieManagerLayer, Cookies, Key};

const COOKIE_NAME: &str = "visited_private";
static KEY: OnceLock<Key> = OnceLock::new();

#[tokio::main]
async fn main() {
    let my_key: &[u8] = &[0; 64]; // Your real key must be cryptographically random
    KEY.set(Key::from(my_key)).ok();

    let app = Router::new()
        .route("/foo/bar", get(handler))
        .layer(CookieManagerLayer::new());

    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    let listener = tokio::net::TcpListener::bind(&addr).await.unwrap();
    axum::serve(listener, app.into_make_service())
        .await
        .unwrap();
}

pub fn create_cookie() -> Cookie<'static> {
    let ck = Cookie::build(Cookie::new(COOKIE_NAME, ""))
        .path("/")
        .build();
    ck
}

async fn handler(cookies: Cookies) -> String {
    let key = KEY.get().unwrap();
    let private_cookies = cookies.signed(key); // You can use `cookies.signed` as well

    let ck = private_cookies.get(COOKIE_NAME);

    if let Some(ck) = ck {
        private_cookies.remove(ck);
        "Counter has been reset".into()
    } else {
        private_cookies.add(create_cookie());
        "".into()
    }
}

[imbolc]

Hi :) Sorry for the late response. Let me know if you've solved this already.

I remember there was already an issue when someone tried to remove the cookie using the extracted one. I guess it's about the path, notice it's None:

[examples/bug.rs:39:9] ck = Cookie {
    cookie_string: Some(
        " visited_private=thNnmggU2ex3L5XXeMNfxf8Wl8STcVZTxscSFEKSxa0=",
    ),
    name: Indexed(
        1,
        16,
    ),
    value: Concrete(
        "",
    ),
    expires: None,
    max_age: None,
    domain: None,
    path: None,
    secure: None,
    http_only: None,
    same_site: None,
    partitioned: None,
}

If I just create a new cookie the code works:

private_cookies.remove(create_cookie());

But I personally always call Cookie::make_removal just in case.

I don't have time right now to investigate the issue. But if you'd like to dig into it, I'd start with trying to reproduce the bug using just underlying cookie::CookieJar to be sure the bug is belong to this crate.

[Zerowalker]

No worries!:)

Haven't solved it, but great discovery about the extracted one being the issue here.

I did a small test with cookie::CookieJar and it doesn't seem to change the path when getting it.

fn test() {
       use cookie::{Cookie, CookieJar};

       let mut jar = CookieJar::new();

       let ck = Cookie::build(("name", "gnu")).path("/").build();
       jar.add(ck.clone());
       let gck = jar.get("name").unwrap();

       println!("{:?}\n", ck);
       println!("{:?}", gck);
   }

this will print out this:

Cookie { cookie_string: None, name: Concrete("name"), value: Concrete("gnu"), expires: None, max_age: None, domain: None, path: Some(Concrete("/")), secure: None, http_only: None, same_site: None, partitioned: None }

Cookie { cookie_string: None, name: Concrete("name"), value: Concrete("gnu"), expires: None, max_age: None, domain: None, path: Some(Concrete("/")), secure: None, http_only: None, same_site: None, partitioned: None }

Which compared to the tower_cookies does have it's path intact.

[mbrc12]

I may be wrong here, but I think the issue is the round trip of the cookies. When the browser sends the cookies back, it does not indicate the metadata used to set the cookie, in particular, it does not return the path. It just looks like id=61addeaddae61; login=false.

So the extractor, which only gets to read the cookie headers, does not contain information about the path, and sets the path information to None. When you use this extracted cookie to remove the original cookie, nothing happens. This is because the spec says that to actually delete a cookie, you need to send a Set-Cookie with zero max-age or something, but more importantly, with the original path and domain.
I learnt about this from the related discussion here.

Note that path and domain default to the endpoint that sets these cookies. So, lets say you set a cookie
from example.com/ with path /. Then, attemping to remove it with the extracted cookie will work, since, even though the extracted path is None, it will default to / when sent back to the browser, which is correct. But if you set the same cookie (i.e.,with path /) from example.com/test, then it won't work since the path will default to /test.

To resolve this, the backend might need to store a copy of the cookies by name, and use these to delete, but that seems cumbersome to abstract out. Another solution could be to store the path right inside the cookie itself. For instance if the cookies are typed via serde, and can we add a field which contains the
path.

I realize that tower-cookies wishes to be generic enough to add specific "hacks" like these. Maybe a more specific crate on top of this can help.

[imbolc]

I think the issue is the round trip of the cookies.

Seems so, thank you for the insight 🙏

I realize that tower-cookies wishes to be generic enough to add specific "hacks" like these. Maybe a more specific crate on top of this can help.

Yeah, I'd like it to work with cookies set from other sources, e.g. frontend js. The realistic solution could be saving paths in separate cookies. I.e. for Cookie { name: "foo", path: "/bar" } add additional Cookie { name: "__tower-cookie-delete_foo", value: "/bar" }. But again the solution is partial as it won't work if someone tries to delete a cookie set from outside tower-cookies.

We could hide this solution behind a (certainly not default) feature, if you really need it. But I'd rather suggest adding a warning on deletion pitfalls and solutions to the docs.