Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#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)

with_sessionid.PNG (32.5 KB) - added by Ramin Farajpour Cami 5 years ago.
with_sessionid.2.PNG (32.5 KB) - added by Ramin Farajpour Cami 5 years ago.
without_seeesionid.PNG (31.0 KB) - added by Ramin Farajpour Cami 5 years ago.
session.diff (1.2 KB) - added by Ramin Farajpour Cami 5 years ago.

Download all attachments as: .zip

Change History (43)

comment:1 Changed 5 years ago by Tim Graham

Cc: Collin Anderson added

I don't entirely understand the issue but making the change you suggest results in test failures in httpwrappers.

comment:2 Changed 5 years ago by Ramin Farajpour Cami

Hi Tim,

Any results on your testing?

comment:3 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Ramin Farajpour Cami

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 method delete_cookie,

comment:5 Changed 5 years ago by Tim Graham

What does "use sessionid= on request ajax" mean? Can you give some example code?

Changed 5 years ago by Ramin Farajpour Cami

Attachment: with_sessionid.PNG added

Changed 5 years ago by Ramin Farajpour Cami

Attachment: with_sessionid.2.PNG added

Changed 5 years ago by Ramin Farajpour Cami

Attachment: without_seeesionid.PNG added

comment:6 Changed 5 years ago by Ramin Farajpour Cami

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 ,

Last edited 5 years ago by Ramin Farajpour Cami (previous) (diff)

comment:7 Changed 5 years ago by Tim Graham

What does your Python code look like?

comment:8 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Collin Anderson

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 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Collin Anderson

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 Changed 5 years ago by Collin Anderson

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}
Last edited 5 years ago by Collin Anderson (previous) (diff)

comment:13 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Collin Anderson

Regarding two for loops - performance and speed: There's only a few iterations and not 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.

Last edited 5 years ago by Collin Anderson (previous) (diff)

comment:15 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Collin Anderson

"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).

comment:17 Changed 5 years ago by Ramin Farajpour Cami

Yes, Collin, waiting of response Tim ,

Changed 5 years ago by Ramin Farajpour Cami

Attachment: session.diff added

comment:18 Changed 5 years ago by Ramin Farajpour Cami

Has patch: set

comment:19 Changed 5 years ago by Tim Graham

Component: HTTP handlingcontrib.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.

comment:20 Changed 5 years ago by 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?

comment:21 in reply to:  20 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Ramin Farajpour Cami

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:

  1. Each session has a beginning and an end.
  1. Each session is relatively short-lived.
  1. Either the user agent or the origin server may terminate a session.
  1. 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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Ramin Farajpour Cami

Resolution: invalid
Status: newclosed

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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Tim Graham

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:

  1. Do this
  2. Do that
  3. ...

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 Changed 5 years ago by Ramin Farajpour Cami

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 Changed 5 years ago by Ramin Farajpour Cami

Resolution: invalid
Status: closednew

comment:32 Changed 5 years ago by Ramin Farajpour Cami

Type: BugCleanup/optimization

comment:33 Changed 5 years ago by Collin Anderson

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 Changed 5 years ago by Ramin Farajpour Cami

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

comment:35 Changed 5 years ago by 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?

comment:36 in reply to:  35 Changed 5 years ago by Ramin Farajpour Cami

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;
}
Last edited 5 years ago by Ramin Farajpour Cami (previous) (diff)

comment:37 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Tim Graham

Resolution: needsinfo
Status: newclosed

comment:39 Changed 5 years ago by Collin Anderson

"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?

Note: See TracTickets for help on using tickets.
Back to Top