Cookies without an expiration date are also saved

[NabiKAZ]

Details: https://github.com/jkhsjdhjs/node-fetch-cookies/issues/25

[jkhsjdhjs]

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]

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 https://github.com/jkhsjdhjs/node-fetch-cookies/issues/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]

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]

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]

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:

https://github.com/jkhsjdhjs/node-fetch-cookies/blob/db40f03ecfcaff613202637191060bf9d63d355c/src/cookie.mjs#L150-L155

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.