Warn users when session cookies are split #483
Merged
Conversation
This file contains hidden or 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
JoelSpeed
force-pushed
the
split-warning
branch
from
7ef8613 to
5ddd307
Compare
pkg/sessions/cookie/session_store.go
Outdated
| // it into a slice of cookies which fit within the 4kb cookie limit indexing | ||
| // the cookies from 0 | ||
| func splitCookie(c *http.Cookie) []*http.Cookie { | ||
| logger.Printf("WARNING: Sessions exceeds 4kb cookie limit. Multiple cookies are required for this session. Please use server side session storage (eg. Redis).") |
There was a problem hiding this comment.
How do you feel about change WARNING to DEPRECATED?
There was a problem hiding this comment.
My thinking was that we haven't officially decided to deprecate yet, so shouldn't say it's deprecated? I was thinking we would add deprecated once we have gathered more feedback, though, changing it now, might make it easier to get feedback? 🤔
loshz
reviewed
Apr 12, 2020
pkg/sessions/cookie/session_store.go
Outdated
| // it into a slice of cookies which fit within the 4kb cookie limit indexing | ||
| // the cookies from 0 | ||
| func splitCookie(c *http.Cookie) []*http.Cookie { | ||
| logger.Printf("WARNING: Sessions exceeds 4kb cookie limit. Multiple cookies are required for this session. Please use server side session storage (eg. Redis).") |
There was a problem hiding this comment.
I think something like this reads better:
Suggested change
| logger.Printf("WARNING: Sessions exceeds 4kb cookie limit. Multiple cookies are required for this session. Please use server side session storage (eg. Redis).") | |
| logger.Printf("WARNING: Multiple cookies are required for this session as it exceeds the 4kb cookie limit. Please use server side session storage (eg. Redis) instead") |
JoelSpeed
force-pushed
the
split-warning
branch
from
5ddd307 to
fcd52e0
Compare
|
@syscll Applied your suggestion |
loshz
approved these changes
Apr 12, 2020
steakunderscore
approved these changes
Apr 12, 2020
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.
Description
Adds a warning to users when sessions are being split, recommending them to use a server side session storage implementation such as redis
Motivation and Context
To start potential changes inferred by #482
How Has This Been Tested?
Not tested but a minor change that shouldn't need much in the way of testing
Checklist: