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

Cookies without an expiration date are also saved #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NabiKAZ
Copy link

@NabiKAZ NabiKAZ commented Feb 8, 2024

Details: #25

@jkhsjdhjs
Copy link
Owner

Thanks for your interest in this project! I just had a look at the change and I think it can be solved more elegantly. The check cookie.expiry === null you added is already present in Cookie.hasExpired(), it's just that withSession is passed as false by the save() function, so session cookies are not included.

Maybe the save() function should be modified so the user can decide whether they want to save session cookies or not. This could be done by adding an additional boolean parameter to the function. Would this solve the problem for you?

@NabiKAZ
Copy link
Author

NabiKAZ commented Feb 10, 2024

It is true that the problem can be solved by sending true in save. This is not interesting because hasExpired is not checked and the cookie is always saved, and this was the case that I was solving this way and I think it was wrong. I already wrote about this at #25 which you read.
Now it is not a matter of not checking the expiration date and even force writing the cookie.
As I mentioned, the issue is cookies without an expiration date, for which the user's decision is not required, because the behavior of a normal browser is to save and keep them until the session is open. Here we can take the same procedure and save them without the user announcing anything. Adding === null was also by me to solve the same issue.

@jkhsjdhjs
Copy link
Owner

This is not interesting because hasExpired is not checked and the cookie is always saved

I don't understand, hasExpired() is checked for every cookie, but depending on the parameter it either returns true or false for session cookies. Cookie.hasExpired() already includes a check for expiry === null, so I don't get why you want to add one to CookieJar.cookiesValid() (which otherwise already calls hasExpired()) as well. This just seems redundant to me.

I thought about it, and I think you're saying that node-fetch-cookies should always save session cookies, no matter the withSession/sessionEnded parameters. If that's what you're saying, I think it should be handled differently, the withSession parameter may as well be removed in this case, as it otherwise doesn't serve any purpose.

However, I'm not sure if I agree to always saving session cookies. Usually, you only load cookies from a file on application startup, which would define the start of a new session for me. Similarly, a browser also doesn't save session cookies, it only keeps them for a session, i.e. until it is restarted. However, I can see that you might want to keep a session across application restarts, i.e. why one would want to save session cookies.
This is why I think it would be best to let the user decide what they want to happen, via an additional parameter to CookieJar.save().

@NabiKAZ
Copy link
Author

NabiKAZ commented Feb 11, 2024

Currently, the problem is that hasExpired treats cookies without an expiration date as expired and does not store them, while we need to store them just like a browser would.
If we use withSession, it is true that cookies with no date are stored, but no check is made on expiration dates and all cookies with past dates are also stored, which is not true.
This was my take on the story. The outline of the problem is clear and you will definitely say the best solution.

@jkhsjdhjs
Copy link
Owner

If we use withSession, it is true that cookies with no date are stored, but no check is made on expiration dates and all cookies with past dates are also stored, which is not true.

This isn't correct, if the parameter to Cookie.hasExpired() is true, it only reports cookies without a date as expired. So if a cookie is not a session cookie (i.e. if it has an expiration date), the parameter is ignored and the date will be checked in any case:

hasExpired(sessionEnded) {
return (
(sessionEnded && this.expiry === null) ||
(this.expiry && this.expiry < new Date())
);
}

The above code only returns true in 2 cases:

  1. sessionEnded is true and the cookie is a session cookie (expiry === null)
  2. It isn't a session cookie (i.e. expiry is truthy) and and the cookie has expired (expiry < new Date())

In all other cases, the function returns false.
Thus, sessionEnded only influences the decision for session cookies, other cookies are reported as expired depending on their expiry date.

If this library really saves expired cookies for you we'd have to investigate it, please post examples of it in this case, as I don't see how that would be possible just by looking at the code.

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

Successfully merging this pull request may close these issues.

2 participants