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.

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

Removing cookies by name #199

Closed
imbolc opened this issue Apr 2, 2022 · 4 comments
Closed

Removing cookies by name #199

imbolc opened this issue Apr 2, 2022 · 4 comments

Comments

@imbolc
Copy link

imbolc commented Apr 2, 2022

Maybe you're willing to accept something like this here?

The api would become

CookieJar::remove(&mut self, name: impl Into<RemovalCookie>);

pub enum RemovalCookie {
    Cookie(Cookie<'static>),
    Name(Cow<'static, str>),
}

impl From<&'static str> for RemovalCookie {}
impl From<String> for RemovalCookie {}
impl From<Cookie<'static>> for RemovalCookie {}

The implementation is here: https://github.com/imbolc/tower-cookies/blob/2bbd9226188c6a18c80086b397aa34fcc42d8f68/src/lib.rs#L198

@Zerowalker
Copy link

I second this, being the one that pushed @imbolc into making this by asking/requesting this at imbolc/tower-cookies#14

@SergioBenitez
Copy link
Member

SergioBenitez commented Sep 14, 2022

I'm not sure what's being proposed, exactly. Given the API you're suggesting, it seems like the crux of the proposal is to allow cookies to be removed via either 1) an &str name (this is new) or 2) a Cookie (as it is today). Am I understanding correctly?

It seems that given imbolc/tower-cookies#14 (comment), the reason the API is as it is today is understood: to avoid the common case where a user thinks a cookie will be removed but, because of a mismatched path or domain, the browser will fail to do so because the incorrect removal cookie was generated. I think any API that makes it trivial to remove a Cookie without specifying a path and/or domain will bring about difficulties.

Given that, perhaps we could consider an API that looks like:

fn remove_named<P, D>(&mut self, name: &str, path: P, domain: D)
    where P: Into<Option<&str>>, D: Into<Option<&str>>;

Such an API use would look like:

let mut jar = some_jar;
jar.remove_named("cookie_name", None, None);
jar.remove_named("cookie_name", "/", None);
jar.remove_named("cookie_name", None, "crates.io");
jar.remove_named("cookie_name", "/", "crates.io");

This is in contrast to the API now:

let mut jar = some_jar;
jar.remove(Cookie::named("cookie_name"));
jar.remove(Cookie::build("cookie_name", "").path("/").finish());
jar.remove(Cookie::build("cookie_name", "").domain("crates.io").finish());
jar.remove(Cookie::build("cookie_name", "").path("/").domain("crates.io").finish());

The first API suffers from unnamed parameters in an API that may be security sensitive while being pithy. The latter suffers from verbosity but is fairly clear otherwise. The latter also has the advantage that you can take a Cookie from the client and pass it directly to remove() and be guaranteed that the cookie will be properly removed. This is an important feature.

Both seem unideal in some ways, so let me propose yet another idea:

// Change `remove()` to:
fn remove<C: Into<Cookie<'_>>(&mut self, cookie: C);

// Change `Cookie::named()` to:
pub fn named<N>(name: N) -> CookieBuilder<'c>
    where N: Into<Cow<'c, str>>;

// Add the following:
impl Into<Cookie<'_>> for CookieBuilder<'_>;

Now, the API use would look like:

let mut jar = some_jar;
jar.remove(Cookie::named("cookie_name"));
jar.remove(Cookie::named("cookie_name").path("/"));
jar.remove(Cookie::named("cookie_name").domain("crates.io"));
jar.remove(Cookie::named("cookie_name").path("/").domain("crates.io"));

This is almost as pithy as the remove_named API while also being extremely clear and affording the "pass an existing cookie to be removed". What do you think? Would this resolve the original concern, or is there a strong desire to be able to remove without creating a Cookie at all?

@imbolc
Copy link
Author

imbolc commented Sep 15, 2022

  1. an &str name (this is new) or 2) a Cookie (as it is today). Am I understanding correctly?

Exactly

user thinks a cookie will be removed but, because of a mismatched path or domain, the browser will fail to do so because the incorrect removal cookie was generated

Right. I've learned about path and domain years ago, but frameworks I used afterwards let me happily forget about them e.g.:

Even Cookie::new doesn't require domain and path to be specified, so I don't see why remove should:

    pub fn new<N, V>(name: N, value: V) -> Self
    {
        Cookie {
            ...
            domain: None,
            path: None,

I think any API that makes it trivial to remove a Cookie without specifying a path and/or domain will bring about difficulties.

Would you maybe elaborate on this? In particular I don't see how passing Cookie::named("cookie_name") instead of cookie_name makes the removal less trivial.

The first API suffers from unnamed parameters in an API that may be security sensitive while being pithy. The latter suffers from verbosity but is fairly clear otherwise.

Agreed, I certainly wouldn't use jar.remove_named("cookie_name", None, None);. It's not clear if it's (path, domain) or (domain, path) and the compiler won't help here as both are strings.

Would this resolve the original concern, or is there a strong desire to be able to remove without creating a Cookie at all?

It's nice to remove a need for .finish() regardless of the issue. But my idea is about making the api more intuitive and it seems to me that people, coming from other frameworks, expect removal by a string.

@SergioBenitez
Copy link
Member

Finally address and released in v0.18.0-rc.0 - CHANGELOG incoming, but docs and message in 49ff7b0 should suffice for now.

If you could give a try, I'd be grateful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants