orestbida / cookieconsent

:cookie: Simple cross-browser cookie-consent plugin written in vanilla js
https://playground.cookieconsent.orestbida.com/
MIT License
3.9k stars 406 forks source link

[Bug]: eraseCookies doesn't respect domain #718

Open imnasnainaec opened 1 month ago

imnasnainaec commented 1 month ago

Expected Behavior

When a domain is specified, only cookies that match that domain should be deleted.

Current Behavior

All cookies with name that match the cookies param are deleted.

Steps to reproduce

Use eraseCookies(/.*/, '/', 'specific.domain') and see all cookies deleted even though they have a different domain.

Proposed fix or additional info.

No response

Version

3.0.1

On which browser do you see the issue?

No response

imnasnainaec commented 1 month ago

Note that there is a test related to this that is passing but should fail because the "path" doesn't match: https://github.com/orestbida/cookieconsent/blob/master/tests/api.test.js#L180-L184

orestbida commented 1 month ago

At the moment the plugin tries 2 attempts to delete a cookie: https://github.com/orestbida/cookieconsent/blob/3c8df8e377fdbbcaadf248759a47faaec6ec3cda/src/utils/cookies.js#L331-L332

which explains your behaviour.

As for the path thing, I cannot reproduce that; if I specify a wrong path, cookies are not deleted.

it should check not just for name, but also for path and domain.

How would you do this with javascript? AFAIK you can't detect the path or domain where a cookie is set.

imnasnainaec commented 1 month ago

@orestbida The function does not work as described. Please look at the following two things:

  1. The one relevant eraseCookies test in the code is https://github.com/orestbida/cookieconsent/blob/master/tests/api.test.js#L180-L184 which creates a cookie with path=/ciao then erases a cookie with path '/'. That should not delete the cookie, but it does, as confirmed by expect(api.validCookie('test_cookie5')).toBe(false);.
  2. I added an eraseCookiesHelper test in https://github.com/orestbida/cookieconsent/pull/717/commits/4d9f58fc21ac69347e18f3f02dd812f6e28f4781 which failed: https://circleci.com/gh/orestbida/cookieconsent/571

Please fix/add more tests to match the eraseCookies behavior you expect.

imnasnainaec commented 1 month ago

You said

As for the path thing, I cannot reproduce that; if I specify a wrong path, cookies are not deleted.

What about eraseCookies(/.*/, '/', '.wrong.domain') ... is that not erasing any cookies?


You said

AFAIK you can't detect the path or domain where a cookie is set.

I find that to be true, which is the motivation for removing them completely in #717


Thank you for your patience. I may not have identified the problem completely correctly, but there is a problem and I think more unit tests will help flush it out.

imnasnainaec commented 1 month ago

I've added tests here that show my understanding of how the eraseCookies function should work but is failing: https://github.com/orestbida/cookieconsent/pull/719/files/daccb8bca482fabe254f181996283229de976beb#diff-8a3eb799e63916b7d0a306ae2f7ead84b72c44ec9959a099d7279890da95445e

orestbida commented 1 month ago

The tests are flawed. You are setting a cookie in a specific path, but you can't see it, unless your current page corresponds to that exact path.

You could add a condition to check if the cookie exists and that would fail, since document.cookie does not know of any cookie set in a more specific path than the current one.

it('Should erase cookie with specific path and domain', () => {
    document.cookie = 'test_cookie5=21; expires=Sun, 1 Jan 2063 00:00:00 UTC; path=/ciao; domain='+location.host;
    expect(api.validCookie('test_cookie5')).toBe(true); //fails
    ...
});
orestbida commented 1 month ago

I find that to be true, which is the motivation for removing them completely

You CAN delete a cookie in a different path/domain, but you must know beforehand the path/domain. They can't be retrieved via javascript.

What about eraseCookies(/.*/, '/', '.wrong.domain')

This is indeed deleting the cookies, and it's because of this https://github.com/orestbida/cookieconsent/issues/718#issuecomment-2286335183. So that is indeed a bug.

imnasnainaec commented 1 month ago

The tests are flawed. You are setting a cookie in a specific path, but you can't see it, unless your current page corresponds to that exact path.

You could add a condition to check if the cookie exists and that would fail, since document.cookie does not know of any cookie set in a more specific path than the current one.

it('Should erase cookie with specific path and domain', () => {
    document.cookie = 'test_cookie5=21; expires=Sun, 1 Jan 2063 00:00:00 UTC; path=/ciao; domain='+location.host;
    expect(api.validCookie('test_cookie5')).toBe(true); //fails
    ...
});

I find that to be true, which is the motivation for removing them completely

You CAN delete a cookie in a different path/domain, but you must know beforehand the path/domain. They can't be retrieved via javascript.

What about eraseCookies(/.*/, '/', '.wrong.domain')

This is indeed deleting the cookies, and it's because of this #718 (comment). So that is indeed a bug.

This reinforces my sense that the path parameter in eraseCookies doesn't do anything.

orestbida commented 1 month ago

I already said twice that it DOES do something.

You CAN delete a cookie by providing a path, but you can't test if it exists or not, so you have to know what was set where.

Here is the most basic pure example:

// set cookie in the /demo path
document.cookie = "test=d; expires=Fri, 31 Dec 2025 23:59:59 GMT; path=/demo";

// delete cookie, set on /demo, whether you are on the same path or not
document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/demo";
imnasnainaec commented 1 month ago

I recognize that you have multiple times stated that specifying path does something. In the example you just gave with document.cookie, you're showing that the path does matter for setting a cookie, and that the cookie can be deleted even from a different path. That is true and useful.

You acknowledged that there is a bug. I'm trying to flush out its consequences on how eraseCookies uses the path parameter. Is it not correct that document.cookie = "test=d; expires=Fri, 31 Dec 2025 23:59:59 GMT; path=/demo"; can be deleted with document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/demo"; and with document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC"; or even document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/other"; ?

If that is the case, then what is the difference of outcome between eraseCookies("test", "/demo") and eraseCookies("test", "/other")?

orestbida commented 1 month ago

If you are on http://domain.com and you set a cookie on http://domain.com/demo:

document.cookie = "test=d; expires=Fri, 31 Dec 2025 23:59:59 GMT; path=/demo";

that cookie can only be deleted by this:

document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/demo";

these do not work:

document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC";
document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/other";
imnasnainaec commented 1 month ago

Thanks for bearing with me. I've updated the tests in #719 and it should now just be failing on the bug you mentioned, coming from erase(cookieName); in https://github.com/orestbida/cookieconsent/blob/3c8df8e377fdbbcaadf248759a47faaec6ec3cda/src/utils/cookies.js#L331

orestbida commented 1 month ago

Simply deleting the first erase function might not be the best approach. Since we don't know how a cookie is set (with/without a specific domain), we have to try both by default.

If a cookie is set with an explicit domain:

document.cookie = "test=d; expires=Fri, 31 Dec 2025 23:59:59 GMT; domain=domain.com";

it can only be deleted with:

document.cookie = "test=; expires=Thu, 01 Jan 1970 00:00:00 UTC; domain=domain.com"; // or .domain.com

Instead, we should check if the user specifies a custom domain value. If a domain is provided, we should call the erase method with the domain parameter only.

imnasnainaec commented 1 month ago

Affirmative. I've just pushed to #719 a possible approach.

imnasnainaec commented 1 month ago

@orestbida Thank you again for putting up with my earlier misunderstanding related to path. #719 is ready for review; I hope it is better suited for the actual bug.

orestbida commented 1 month ago

Mhm, you are currently trying to delete the cookie 2 times, both with a specific domain field. If a cookie is set without a specific domain field it will not be deleted.

This should technically do the job:

if (!customDomain) {
    erase(cookieName);
}

erase(cookieName, domain);
imnasnainaec commented 1 month ago

The case

if (!customDomain) {
    erase(cookieName);
}

is covered in

erase(cookieName, customDomain);
imnasnainaec commented 4 weeks ago

... because the erase function has (domain ? '; domain=.' + domain : '')