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

Fix bug in cookie store #634

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Fix bug in cookie store #634

merged 1 commit into from
Apr 11, 2019

Conversation

geekjob
Copy link
Contributor

@geekjob geekjob commented Feb 21, 2019

I’m using SOA (Service Oriented Architecture) application. There I’m having different services that wroted on different languages. For store session I’m using Redis. In PHP and others applications I'm implementing express-session like handlers for share sessions between the services.

But I’m also using third party services, and they can generate broken session record without cookie section. If this section is missed, then all my Node.js applications are went down with an error.

@dougwilson
Copy link
Contributor

Hi @geekjob-ru sorry, I thought I replied to this (not sure why I thought that). Thank you for this pull request! Just a couple things to comment about:

  1. We just need to add at lest one test that encounters this condition so it can be tested for to prevent regression. If you're not sure how to do this, please let me know and I can help you make a test 👍

  2. There likely shouldn't be a hard-coded magic constant that users cannot change in the code 36e5.

  3. And finally, this brings up the question on if this is actually what should happen in this condition or not; as in should what you did here be the right course of action or should this module treat it as an error (a catchable error, though) or should this module treat this condition as if the session is not present in the store?

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.

So I didn't hear anything back yet, so to at least get the uncaught error fixed, I pushed up a test to your PR and just changed the fix to catch and forward it.

If the idea is to make that bad session actually work, it would need to have the issues in my original review addressed.

@dougwilson dougwilson merged commit 85682a2 into expressjs:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants