Set the Secure flag on Popcode's session token cookie #1587
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
outoftime
requested changes
Oct 12, 2018
| {expires: new Date(Date.now() + SESSION_TTL_MS)}, | ||
| { | ||
| expires: new Date(Date.now() + SESSION_TTL_MS), | ||
| secure: true, |
There was a problem hiding this comment.
@andy11 this seems like a reasonable change, but unfortunately it does break behavior in development, where people will typically not be running Popcode over https. So, we should make this behavior configurable, by:
- Introducing an environment variable e.g.
REQUIRE_SECURE_SESSION_COOKIE - Passing it through to the client-side code via Webpack (with a default of
false) - Adding it with value
truein our Travis configuration (I will do this once the code is ready) - Exposing it as a boolean via the
configmodule
The specific behavior is retaining the user session on refresh—you’ll find that right now, if you log in and then refresh the page with this branch checked out, you’ll be logged back out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sets the Secure flag to
trueon the Popcode's session token cookie.It would be nice to also set the HttpOnly flag, but this is not supported: js-cookie/js-cookie#344.
NOTE: This should not be considered an important security issue, just a minor nit that's probably worth merging eventually :-)