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

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

Open
imnasnainaec opened this issue Aug 9, 2024 · 18 comments
Open

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

imnasnainaec opened this issue Aug 9, 2024 · 18 comments
Labels
bug Something isn't working

Comments

@imnasnainaec
Copy link

imnasnainaec commented Aug 9, 2024

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 imnasnainaec added the bug Something isn't working label Aug 9, 2024
@github-actions github-actions bot added the triage yet to be reviewed label Aug 9, 2024
@imnasnainaec
Copy link
Author

imnasnainaec commented Aug 9, 2024

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
Copy link
Owner

At the moment the plugin tries 2 attempts to delete a cookie:

erase(cookieName);
erase(cookieName, domain);

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
Copy link
Author

imnasnainaec commented Aug 13, 2024

@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 4d9f58f which failed: https://circleci.com/gh/orestbida/cookieconsent/571

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

@imnasnainaec
Copy link
Author

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
Copy link
Author

imnasnainaec commented Aug 13, 2024

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
Copy link
Owner

orestbida commented Aug 13, 2024

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
Copy link
Owner

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.

@imnasnainaec
Copy link
Author

imnasnainaec commented Aug 13, 2024

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

imnasnainaec commented Aug 13, 2024

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

erase(cookieName);

@orestbida orestbida removed the triage yet to be reviewed label Aug 14, 2024
@orestbida
Copy link
Owner

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
Copy link
Author

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

@imnasnainaec imnasnainaec changed the title [Bug]: eraseCookies doesn't care about path or domain [Bug]: eraseCookies doesn't respect domain Aug 14, 2024
@imnasnainaec
Copy link
Author

@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
Copy link
Owner

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
Copy link
Author

The case

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

is covered in

erase(cookieName, customDomain);

@imnasnainaec
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants