-
Notifications
You must be signed in to change notification settings - Fork 278
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
Cannot set cookies with domain localhost #215
Comments
Without this config, the default URL is 'localhost' which causes some issues when setting cookies because of a bug in tough-cookie which treats localhost like a public domain. Because you can't set cookies on a public domain, cookies were not being set in the tests. Setting the testURL to a real URL fixes this issue. @lfdebrux has raised an issue with tough-cookie here: salesforce/tough-cookie#215
Without this config, the default URL is 'localhost' which causes some issues when setting cookies because of a bug in tough-cookie which treats localhost like a public domain. Because you can't set cookies on a public domain, cookies were not being set in the tests. Setting the testURL to a real URL fixes this issue. @lfdebrux has raised an issue with tough-cookie here: salesforce/tough-cookie#215
We're not seeing However, We are thinking of just updating the error in this case; but I'll let @ruoho-sfdc comment further. |
Hi @lfdebrux, I just tried to recreate this issue with the following code but I was not able to.
What am I doing wrong? It looks like the same code? If you try this against the master branch are you able to reproduce the problem? Thanks, |
Hi @medelibero-sfdc, I'm not sure why your setup isn't raising the error... if I checkout the current version of the master branch and run your snippet I do get the error 🤔
|
@ifdebrux -- could you also kindly let us know what version of NodeJS you are testing with? |
@awaterma I tested with Node 12 (v12.22.6), Node 14 (v14.17.6), and Node 16 (v16.11.0). All versions produced the same error. |
@ifdebrux -- I can reproduce this now in a vows unit test. I'll look into the issue a bit further and see if I can propose a fix.
Result:
|
I've looked into the RFCs for this; and I think our current behavior is acceptable.
There is also some discussion on the following thread, which also speaks to how several browsers handle the issue: https://stackoverflow.com/questions/1134290/cookies-on-localhost-with-explicit-domain And there are similar issues filed with other cookie based projects, for example:
I'll use this work item to check in the test updates; so you and others at least have a direction to go with local cookie development. If you can build an argument from the RFCs that show that we have taken the wrong approach; we'll gladly reconsider! Pull requests and accompanying tests really help us move along issues! Please take a look at the sample above, discussion links and let us know your thoughts. |
Hi @awaterma, thanks for the detailed investigation! I guess I'm not so bothered by the correctness of what tough-cookie does, it's more about the fact that it is surprising, and it took me a long time when I initially hit this issue to figure out what was going on. I guess it's possible I'm the only one who was confused by the error message, but I think like you said tweaking the error message might be helpful. Or maybe just adding something to the docs for the |
That's a great suggestion @lfdebrux. I'll see if I can make a better error message, or update the docs a bit to make this behavior more clear for others. :) |
@GH-215 -- Tests that document localhost behavior when set as domain.
When using option
rejectPublicSuffixes: true
(the default), trying to set a cookie withDomain=localhost
throws the errorCookie has domain set to a public suffix
, which I believe is incorrect.How to reproduce
Run the following code with the latest master of tough-cookie (commit 1b25269)
Expected result
I can set cookies with
Domain=localhost
without an error.Workaround
Turning off the
rejectPublicSuffixes
option when creating theCookieJar
causes the cookie to be set without errors, as expected.The text was updated successfully, but these errors were encountered: