#27328 closed Cleanup/optimization (needsinfo)
return `Set-Cookie` if sessionid= None value
Reported by: | Ramin Farajpour Cami | Owned by: | nobody |
---|---|---|---|
Component: | contrib.sessions | Version: | 1.10 |
Severity: | Normal | Keywords: | |
Cc: | Collin Anderson | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi,
i use request AJAX, when i haven't sessionid i can send request i see response
POST http://IP/session HTTP/1.1 Host: IP Connection: keep-alive Content-Length: 0 Accept: application/json, text/javascript, */*; q=0.01 Origin: http://IP X-Requested-With: XMLHttpRequest User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36 X-CSRFToken: BF8nOVWsMJaX9Gi3aJijGSO97iTyLpNY Referer: http://172.16.20.141/ramin Accept-Encoding: gzip, deflate Accept-Language: en-US,en;q=0.8 Cookie: sessionid=;csrftoken=BF8nOVWsMJaX9Gi3aJijGSO97iTyLpNY
you see here Cookie: sessionid=;csrftoken=BF8nOVWsMJaX9Gi3aJijGSO97iTyLpNY
sessionid is sessionid=
send to django server
HTTP/1.1 200 OK Date: Sun, 09 Oct 2016 08:17:15 GMT Content-Type: application/json Connection: keep-alive Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/ Content-Length: 18
set Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
in response django
i see your code https://github.com/django/django/blob/master/django/http/cookie.py#L74 in if val
is empty , you need change or
to and
,
Attachments (4)
Change History (43)
comment:1 by , 8 years ago
Cc: | added |
---|
comment:3 by , 8 years ago
Here are the test failures:
====================================================================== FAIL: test_invalid_cookies (httpwrappers.tests.CookieTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 58, in testPartExecutor yield File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 600, in run testMethod() File "/home/tim/code/django/tests/httpwrappers/tests.py", line 778, in test_invalid_cookies self.assertEqual(parse_cookie('a=b; "; c=d'), {'a': 'b', '': '"', 'c': 'd'}) File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 820, in assertEqual assertion_func(first, second, msg=msg) File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 1111, in assertDictEqual self.fail(self._formatMessage(msg, standardMsg)) File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 665, in fail raise self.failureException(msg) AssertionError: {'c': 'd', 'a': 'b'} != {'': '"', 'c': 'd', 'a': 'b'} - {'a': 'b', 'c': 'd'} + {'': '"', 'a': 'b', 'c': 'd'} ? +++++++++ ====================================================================== FAIL: test_python_cookies (httpwrappers.tests.CookieTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 58, in testPartExecutor yield File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 600, in run testMethod() File "/home/tim/code/django/tests/httpwrappers/tests.py", line 753, in test_python_cookies {'keebler': '"E=mc2', 'L': '\\"Loves\\"', 'fudge': '\\012', '': '"'} File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 820, in assertEqual assertion_func(first, second, msg=msg) File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 1111, in assertDictEqual self.fail(self._formatMessage(msg, standardMsg)) File "/opt/python3.5.2/lib/python3.5/unittest/case.py", line 665, in fail raise self.failureException(msg) AssertionError: {'fudge': '\\012', 'L': '\\"Loves\\"', 'keebler': '"E=mc2'} != {'fudge': '\\012', '': '"', 'L': '\\"Loves\\"', 'keebler': '"E=mc2'} - {'L': '\\"Loves\\"', 'fudge': '\\012', 'keebler': '"E=mc2'} + {'': '"', 'L': '\\"Loves\\"', 'fudge': '\\012', 'keebler': '"E=mc2'} ? +++++++++
I guess dropping those cookies without a key might be fine, but I still don't understand the situation where you're running into this.
comment:4 by , 8 years ago
Thanks Tim,
i use python 2, do you test this?
self.assertEqual(parse_cookie('sessionid=; csrftoken=d'))
my means this is when i use sessionid=
on request ajax , response this Set-Cookie: sessionid=;
my opinion when sessionid is empty should not be sessionid "Set-Cookie" on response ,
code parse_cookie : https://github.com/django/django/blob/master/django/http/cookie.py#L59&L77
i see code here :
https://github.com/django/django/blob/master/django/http/response.py#L221&L227
when set on request sessionid=
empty , pass from parse_cookie
and in call delete_cookie
and response Set-Cookie: sessionid=;
, While this is should be not set on Set-Cookie
sessionid,
my opinion when
sessionid=
should avoid call methoddelete_cookie
,
comment:5 by , 8 years ago
What does "use sessionid= on request ajax" mean? Can you give some example code?
by , 8 years ago
Attachment: | with_sessionid.PNG added |
---|
by , 8 years ago
Attachment: | with_sessionid.2.PNG added |
---|
by , 8 years ago
Attachment: | without_seeesionid.PNG added |
---|
comment:6 by , 8 years ago
I'm still unconvinced why response sessionid is empty , https://github.com/django/django/blob/master/django/http/cookie.py#L74 ,
please see image without_sessionid nothing Set-Cookie but with sessionid=
response Set-Cookie
,
comment:8 by , 8 years ago
run this on browser and show response from network browser for see request/response:
def index(req): sess = {'sessionid':''} ## if there is csrftoken, update dict with sessionid req.COOKIES.update(sess) print req.COOKIES return JsonResponse({'foo':'bar'})
comment:9 by , 8 years ago
Whenever Django sees an invalid sessionid
cookie, it tells the browser to delete that cookie.
The only way to tell a browser to delete a cookie is by expiring it. Django uses a past expiration date of 01-Jan-1970
and sets Max-Age=0
,
(valid for 0 seconds).
This happens both in the case of an empty sessionid
cookie, like your case:
$ curl --silent -i https://www.djangoproject.com/admin/login/ -H'Referer: https://www.djangoproject.com/' -H'Cookie: sessionid=;csrftoken=d' -d'csrfmiddlewaretoken=d' | grep Set-Cookie Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/ Set-Cookie: csrftoken=d; expires=Tue, 10-Oct-2017 17:05:40 GMT; HttpOnly; Max-Age=31449600; Path=/; Secure
But it also happens when the sessionid
cookie is sent, but it's invalid:
$ curl --silent -i https://www.djangoproject.com/admin/login/ -H'Referer: https://www.djangoproject.com/' -H'Cookie: sessionid=bad_invalid_data_here;csrftoken=d' -d'csrfmiddlewaretoken=d' | grep Set-Cookie Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/ Set-Cookie: csrftoken=d; expires=Tue, 10-Oct-2017 17:05:40 GMT; HttpOnly; Max-Age=31449600; Path=/; Secure
Why are you sending a sessionid
cookie in the first place? It seems to me if you don't have a valid sessionid
cookie, you shouldn't be sending it to Django. Does that sound right? Either that or can you ignore the Set-Cookie:
header if it says Max-Age=0
?
comment:10 by , 8 years ago
Thanks Collin,
i know, sessionid is invalid , I'm still unconvinced why sessionid
is empty call method delete_cookie
while sessionid
is empty, if you look rails when send empty sessionid nothing response Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
in your code you have checked
if key or val: # unquote using Python's algorithm. cookiedict[key] = http_cookies._unquote(val)
if key or val:
but val is here empty string and value key
is sessionid
and value val
is ''
, if sessionid
is bad i means bad value you'r are right, no sessionid is empty pass parse_cookie
method and call delete_cookie
why you in line https://github.com/django/django/blob/master/django/http/cookie.py#L74 use or
? why not and
? for check empty value sessionid on request AJAX no browser,
comment:11 by , 8 years ago
Hi Ramin,
The goal of parse_cookie()
is to try to give an exact as possible dict
representation of the Cookie:
header. It seems intuitive to me that sessionid=; csrftoken=d
would translate to {'sessionid': '', 'csrftoken': 'd'}
. You want parse_cookie()
to ignore cookies with no value, but Django has always kept cookies with empty values like these, even before I refactored the parse_cookie code recently. Before that Django used Python's Cookie library to parse cookies, which also keeps cookies with empty values.
Even if we were to change Django to not delete empty sessionid
cookies, that should be a change to the _session_ code (to not call delete_cookie
in that case), not a change to the cookie parsing code. Does that seem right?
comment:12 by , 8 years ago
If you want, you can easily create a Middleware that removes these cookies, and then place it before SessionMiddleware:
class RemoveEmptyCookiesMiddleware: def process_request(request): request.COOKIES = {k: v for k, v in request.COOKIES.items() if k and v}
comment:13 by , 8 years ago
Even if we were to change Django to not delete empty sessionid cookies, that should be a change to the _session_ code (to not call delete_cookie in that case), not a change to the cookie parsing code. Does that seem right?
Hi Collin,
you are right,
and then place it before SessionMiddleware:
class RemoveEmptyCookiesMiddleware: def process_request(request): request.COOKIES = {k: v for k, v in request.COOKIES.items() if k and v}
i'm confused , we have here two for
loop 1- process_request()
2- parse_cookie()
, what happen for performance and speed !!!???
so, do you think eligible this method(RemoveEmptyCookiesMiddleware
) merged to django master?
comment:14 by , 8 years ago
regarding two for
loops - performance and speed: There's only a few iterations and no much work per iteration, so I doubt it will have much impact, but you could time it out if you want.
"do you think eligible this method merged to django master? " - My guess is different people might want different behaviors as far as which cookies to keep and which to remove, and it's unclear how many people would actually find this useful. It's only 3 lines of code if anyone wants to add this code by hand, so there's not much gain in adding this to django master. It's easier to customize if it's not in django master.
comment:15 by , 8 years ago
So if there's any changes to make as a result of this ticket, is that alluded to in comment:11: not deleting empty session ID cookies? Is the SessionMiddleware doing that as a result of [393c0e24]?
comment:16 by , 8 years ago
"result of [393c0e24]" - Yes, that looks right. If we wanted to make this change, I'd recommend changing if settings.SESSION_COOKIE_NAME in request.COOKIES
to if request.COOKIES.get(settings.SESSION_COOKIE_NAME)
.
by , 8 years ago
Attachment: | session.diff added |
---|
comment:18 by , 8 years ago
Has patch: | set |
---|
comment:19 by , 8 years ago
Component: | HTTP handling → contrib.sessions |
---|---|
Needs tests: | set |
I wish I understood the use case better. Anyway, a test is also needed and there's a syntax error (double "if") in the patch. Pull requests are preferred if you can provide that.
follow-up: 21 comment:20 by , 8 years ago
This change is growing on me. It does seem a hair more beginner friendly. I also wish I understood the use case better. Ramin: how is the current behavior problematic? Why is this an issue? What's so bad about the Set-Cookie
header in this case?
comment:21 by , 8 years ago
Replying to Collin Anderson:
This change is growing on me. It does seem a hair more beginner friendly. I also wish I understood the use case better. Ramin: how is the current behavior problematic? Why is this an issue? What's so bad about the
Set-Cookie
header in this case?
I said above, there isn't problem but sessionid is empty any checked, you think current behavior is good, close this ticket,
thanks,
comment:23 by , 8 years ago
Needs tests: | unset |
---|
What's still missing for me is a description of your use case at a high level. What task are you trying to accomplish in your project? Give a sample view or something that demonstrates why this change is useful.
comment:24 by , 8 years ago
Hi Tim,
sorry for delay,
Cookies are set using the Set-Cookie HTTP header, sent in an HTTP response from the web server. This header instructs the web browser to store the cookie and send it back in future requests to the server (the browser will, of course, ignore this header if it does not support cookies or has disabled cookies).
As an example, the browser sends its first request to the homepage of the www.example.org website:
GET /index.html HTTP/1.1 Host: www.example.org …
The server responds with two Set-Cookie
headers:
HTTP/1.0 200 OK Content-type: text/html Set-Cookie: sessionid=12132313; Expires=Wed, 09 Jun 2021 10:18:14 GMT …
The server's HTTP response contains the contents of the website's homepage.
The value of a cookie can be modified by the server by including a Set-Cookie header in response to a page request. The browser then replaces the old value with the new value.
There are many different potential contexts and thus many
different potential types of session. The designers' paradigm for
sessions created by the exchange of cookies has these key attributes:
- Each session has a beginning and an end.
- Each session is relatively short-lived.
- Either the user agent or the origin server may terminate a session.
- The session is implicit in the exchange of state information.
for DEMO Video Tim i will send email for you (timograham@…) , not here Tim, sorry because this is a my real projects
comment:25 by , 8 years ago
Your last comment describes how cookies work but I don't see a description of the problem and how this ticket fixes it. I didn't see that information in the video either. Anyway, for everyone else following this ticket, it would be a lot more helpful if you could list the steps to reproduce the issue here as well as the actual (current) and expected (after the patch) results. Thanks.
comment:26 by , 8 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
do you see my video,? you see sessionid alwasy in my case empty,
I have nothing more to tell you. i will change code django local machin ,
now i close PR ,
Thanks,
comment:27 by , 8 years ago
One of the steps in your video involves using the browser console to set document.cookie = 'sessionid='
. I don't understand what condition that's simulating or if this is some improvement for the case where someone actually does that.
comment:28 by , 8 years ago
Yes, I show after login and manually set seseionid= on browser you're right, I say first this issue when Ajax send with sessionid empty if set cookie in Ajax sessionid= save this other browser for example when user logout sessions empty (is not expired) with every request browser send to server sessionid empty
This I'd was demo,
comment:29 by , 8 years ago
I'm sorry but I can't make sense of that comment and I'm still missing the information requested in comment:23. I guess I would suggest either writing up explicit steps to reproduce the issue:
- Do this
- Do that
- ...
Expected result: ...
Actual result: ...
... or, as I feel language might be the barrier here, try to find someone who speaks your native language and might be able to more easily answer my questions so I can understand it.
comment:30 by , 8 years ago
question : what happen when set sessionid=''
?
status now : sessionid django when sessionid on logout set date 1970 for define browser this sessionid is expire sessionid store on server django
best practice : check sessionid is empty for call extra methods set-cookie
and delete-cookie
(set-cookie and delete-cookie methods call for expire sessionid on browser or CookieJar
)
Note : i test my real app port 8000 and show demo video of this,
Steps :
1 - login to django app for example on ip (127.0.0.1:8000) you have set sessionid OK,
2- set sessionid=''
from AJAX document.cookie="sessionid="
this means set sessionid on cookie success from request (this idea of me create a scenario for this)
3- you see on request header Cookie : sessionid=""; sessionid="jaksdnkjasdkasjdaskd" va ... with set document cookie on ajax or browser (Step 2),there isn't problem here with two sessionid= because django use cookie_parse , cookie parse is a dict
accept one sessionid
4- now logout from app
5- again set step 2 ,
6- you see sessionid is set on user is logout also i display on demo video cookie path is /user
(/user
this path first time i run step 2)
7- problem is here with refresh page sessionid=''
is not expire and call always extra method set-cookie
and delete-cookie
in every request on browser, with call this there is problem because path cookie on browser set path:/user
is not '/' root , sessionid=''
this step of my video,
comment:31 by , 8 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
comment:32 by , 8 years ago
Type: | Bug → Cleanup/optimization |
---|
comment:33 by , 8 years ago
Why are you doing step 2 at all? This doesn't seem necessary or helpful to me: document.cookie="sessionid="
"set sessionid on cookie success from request" - That will happen in any case, you don't need to pre-set the cookie.
comment:34 by , 8 years ago
Hi Collin,
because easy access to browser cookies . Transparently creates a "vitual cookie jar" for storing many "virtual cookies" (key-value pairs) in one actual cookie,
i show to you with step 2 sessionid=''
cookie does not expire, in different path browser,
i think you should see this for help togather :
https://drive.google.com/file/d/0B0zktfkIvV-LVWRscmpTSVRFQVk/view?usp=sharing
follow-up: 36 comment:35 by , 8 years ago
It seems like there's no use case where the current behavior causes a problem -- it only happens when someone is tampering with document.cookie
. Is that correct?
comment:36 by , 8 years ago
Replying to Tim Graham:
It seems like there's no use case where the current behavior causes a problem -- it only happens when someone is tampering with
document.cookie
. Is that correct?
Yes,there is many idea on projecs django for use javascript document.cookie
this is a example :
function Language() { if (getCookie('language') == 'EN') { document.getElementById('btn').innerHTML = getCookie('language'); } } function setCookie(sName, sValue, oExpires, sPath, sDomain, bSecure) { var sCookie = sessionid+ "=" +'test'; sCookie += "; expires=" + 'test'; sCookie += "; path=" + 'path'; sCookie += "; domain=" + 'domains'; document.cookie = sCookie; }
comment:37 by , 8 years ago
What would be a purpose of manipulating the sessionid in JavaScript like that? I'm not familiar with the "virtual cookies" concept in your earlier comment.
comment:38 by , 8 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:39 by , 8 years ago
"because easy access to browser cookies . Transparently creates a "vitual cookie jar" for storing many "virtual cookies" (key-value pairs) in one actual cookie," - I don't think that's true.
If you want to access the actual sessionid
cookie using javascript, you need to set SESSION_COOKIE_HTTPONLY = False
. Does that help?
I don't entirely understand the issue but making the change you suggest results in test failures in
httpwrappers
.