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

Returned null in case of empty cookie value #196

Merged
merged 7 commits into from
Feb 16, 2021

Conversation

vsin12
Copy link
Contributor

@vsin12 vsin12 commented Jun 17, 2020

  1. some of the old sites by mistake send a blank cookie, and whenever a blank cookie is encountered, failed to parse cookie error comes up - so I made a fix of checking the length and accordingly calling callback with null value.

Note: Either this fix, or CookieJar constructor needs to take SetCookieOptions too, as ignoreError is part of SetCookieOptions interface

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): viralsinha <v***@p***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated.

@ruoho
Copy link
Contributor

ruoho commented Aug 24, 2020

Hi @vsin12, would you be able to update the PR with a test?

@vsin12
Copy link
Contributor Author

vsin12 commented Aug 24, 2020

@ruoho : Sorry, but I didn't get you ? Do you want me to give a scenario while it was breaking or you want me to write a test to check my pr ?

Copy link
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote: Note: Either this fix, or CookieJar constructor needs to take SetCookieOptions too, as ignoreError is part of SetCookieOptions interface

ignoreError doesn't apply to storage: ignoreError - boolean - default false - silently ignore things like parse errors and invalid domains. Store errors aren't ignored by this option -- so I'm not sure we need to do any more here, in terms of implementation. We might want to start a project to review ignoreError and its applications --- it might be better to have that apply globally, but we'd have to think that through.

In terms of this P/R; I think I'd like to see the change use validators, as suggested, and then have a simple unit test added that demonstrates the use-case (an empty cookie).

lib/cookie.js Outdated Show resolved Hide resolved
lib/cookie.js Show resolved Hide resolved
Co-authored-by: Andrew Waterman <[email protected]>
@ShivanKaul ShivanKaul linked an issue Sep 21, 2020 that may be closed by this pull request
@awaterma
Copy link
Member

I think we still need a unit test for this fix. :)

@awaterma awaterma self-assigned this Oct 20, 2020
@medelibero-sfdc
Copy link
Contributor

@vsin12 can you change this PR to allow edits from maintainers? I have a few changes I would like to push to this PR.

@vsin12
Copy link
Contributor Author

vsin12 commented Nov 23, 2020

@vsin12 can you change this PR to allow edits from maintainers? I have a few changes I would like to push to this PR.

Done.

@vsin12 vsin12 requested a review from awaterma November 24, 2020 18:19
Copy link
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll discuss this today; I think we want to add a suggestion to return an error when presented with an empty cookie.

if(!validators.isNonEmptyString(cookie) && !validators.isObject(cookie) && ( cookie instanceof String && cookie.length == 0)) {
return cb(null);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at this in context, and I think this is fine. Will discuss today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment below; I think we may want an Error here.

this.callback
);
},
"results in a error being returned because of missing parameters": function(err, cookies) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually return an error, we are invoking the callback with a Null here. Should we invoke the CB with an Error()?

Copy link
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. We may want to add error checking/throwing at some point, but that's probably in a major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CookieJar().setCookie crashes when empty cookie is passed
4 participants