Fixed #27775 -- Respect custom expiry in signed cookies.#7885
Closed
pelme wants to merge 1 commit intodjango:masterfrom
Closed
Fixed #27775 -- Respect custom expiry in signed cookies.#7885pelme wants to merge 1 commit intodjango:masterfrom
pelme wants to merge 1 commit intodjango:masterfrom
Conversation
eb49825 to
32dfc4f
Compare
timgraham
reviewed
Jan 21, 2017
Member
timgraham
left a comment
There was a problem hiding this comment.
Is there a ticket for this? All non-trivial PRs need one, otherwise, they don't appear in the review queue.
Member
There was a problem hiding this comment.
Please don't make unrelated whitespace changes.
32dfc4f to
687d15a
Compare
The custom expiry value `_session_expiry` is stored in the session itself. The signed cookie needs to first be loaded, and then the value needs to be checked for validity. This PR calls `signing.loads()` twice which may incur a small performance penalty. The proper solution would be to refactor the session expiry as discussed in django#19201. However, this fix should not really make that refactor any harder an may be an acceptable fix in the meantime. Refs django#19200, django#19201.
687d15a to
b2fa2c7
Compare
Member
Author
|
@timgraham now there is a proper ticket for this and I've described the motivation for this patched in the ticket: |
timgraham
reviewed
Feb 4, 2017
| max_age = max_age - timezone.now() | ||
|
|
||
| # Handle None and 0 | ||
| if not max_age: |
Member
There was a problem hiding this comment.
Test coverage seems lacking. For example, commenting this if-block out doesn't result in any failures. Could you add some tests as needed for all the changes?
|
@pelme I could add some tests to it if you like? |
Member
|
@virtuallight, feel free to add tests and send a new PR. Closing this one due to inactivity. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The custom expiry value
_session_expiryis stored in the sessionitself. The signed cookie needs to first be loaded, and then the value
needs to be checked for validity.
This PR calls
signing.loads()twice which may incur a small performancepenalty. The proper solution would be to refactor the session expiry
as discussed in #19201. However, this fix should not really make that
refactor any harder an may be an acceptable fix in the meantime.
Refs #19200, #19201.