Fixed #24915 - Added stricter session key validation #4807
Conversation
| @@ -198,6 +198,20 @@ def test_invalid_key(self): | |||
| # session key; make sure that entry is manually deleted | |||
| session.delete('1') | |||
|
|
|||
| def test_session_key_validation(self): | |||
There was a problem hiding this comment.
This should be three tests, not one.
|
This looks good to me. At first, the double layer of properties seemed like overkill, but on second thought it seems OK to be cautious by maintaining the |
|
@carljm I have split the validation test into three tests as suggested. The doubly layered properties look a bit weird all right but I figured it was the ideal way to implement the additional functionality without changing the internal API (ostensibly for the reasons you mentioned, realistically because I don't have a good feel for the code base and took the cautious approach :-P). |
|
Looks good to me! Can you squash the second commit into the first (so there's just one commit with the proper commit message) and force push? Then this looks ready for checkin. |
Changed _session_key attribute to a property and implemented basic validation in the setter. The session key must be 'truthy' and at least 8 characters long. Otherwise, the value is set to None. https://code.djangoproject.com/ticket/24915
|
@carljm Done |
| @@ -161,10 +161,25 @@ def _get_or_create_session_key(self): | |||
| self._session_key = self._get_new_session_key() | |||
| return self._session_key | |||
|
|
|||
| def _validate_session_key(self, key): | |||
| """Key must be truthy and at least 8 characters long""" | |||
There was a problem hiding this comment.
Please include periods in docstrings.
There was a problem hiding this comment.
Could you explain why the limit is 8? (or mention it's somewhat arbitrary)
There was a problem hiding this comment.
Added periods to docstrings.
The 8 character limitation is indeed an arbitrary lower bound to enforce a minimum level of collision security. I went with 8 as it was suggested in the trac ticket.
|
New commit in response to @timgraham's comments. Held off on squashing to allow further development if necessary and make diff apparent. Let me know when to re-squash. |
|
Looks good. I added a sentence in the release notes and merged in f4416b1, thanks! |
Changed _session_key attribute to a property and implemented basic
validation in the setter. The session key must be 'truthy' and
at least 8 characters long. Otherwise, the value is set to None.
https://code.djangoproject.com/ticket/24915