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

this.keys but no opts means cookies don't get signed #53

Open
jergason opened this issue Dec 9, 2014 · 4 comments
Open

this.keys but no opts means cookies don't get signed #53

jergason opened this issue Dec 9, 2014 · 4 comments
Assignees

Comments

@jergason
Copy link

jergason commented Dec 9, 2014

In Cookie.prototype.set (https://github.com/expressjs/cookies/blob/2dcb71f130a7eaafd16e71b9af70debe11d4c93f/lib/cookies.js#L69), the signed variable is true if opts.signed or this.keys is truthy. However, the check for whether to sign keys or not also checks if opts exists.

This means that if this.keys is truthy, but opts is undefined, the signed variable will be true but the key still won't be signed. My expectation is they key should be signed if signed == true, and it shouldn't depend on the existence of opts.

@dougwilson dougwilson added the bug label Dec 9, 2014
@dougwilson dougwilson self-assigned this Dec 9, 2014
@dougwilson
Copy link
Contributor

That seems to make sense, though I'm not sure how easy this will be to fix without including it in the major version bump, because I wonder how many people have the keys set but no options given. I'll have to think about it some. Also input from @jonathanong

@jonathanong
Copy link
Member

Ha was gonna remove that in the next major

@dougwilson
Copy link
Contributor

Cool, that was my suspicion. It seems weird that the existence of keys caused cookies to automatically sign. So I would almost say that the current behavior is intended: you only get signed cookies if you sent {signed: true} to .set. Does that sound right @jonathanong ?

@dougwilson dougwilson removed the bug label Dec 9, 2014
@jonathanong
Copy link
Member

Current behavior is intended :( I wanted the next version to have .sign() and .encrypt() as separate methods

@dougwilson dougwilson added this to the 0.8 milestone Sep 10, 2018
@dougwilson dougwilson removed this from the 0.8 milestone Oct 10, 2019
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

No branches or pull requests

3 participants