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

Eliminate unneeded work when missing signing keys #96

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Eliminate unneeded work when missing signing keys #96

wants to merge 2 commits into from

Conversation

smcmurray
Copy link

0 is a legitimate value for maxAge.
If there are no signing keys, you can abort sooner. There's no need to parse the signature cookie, etc. if you already know you will fail.

0 is a legitimate value for maxAge.
If there are no signing keys, you can abort sooner. There's no need to parse the signature cookie, etc. if you already know you will fail.
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Can you split this into two separate pull requests and add tests for the maxAge change?

@dougwilson dougwilson changed the title Allow maxAge of 0. Eliminate unneeded work when missing signing keys Eliminate unneeded work when missing signing keys May 20, 2018
@dougwilson dougwilson self-assigned this May 20, 2018
@dougwilson
Copy link
Contributor

Hi @smcmurray sorry this fell off my radar. Thanks for moving the maxAge into a different PR. I was actually just about to merge this, but realized it actually has a behavior change with this:

When reading a non-signed cookie with { signed: true } it would returned undefined and no error on the current version while this PR would make that raise an error.

I think this PR is the right change of behavior, though, and will put it into the 0.8.0 release.

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

None yet

2 participants