Opened 2 years ago

Closed 22 months ago

#8077 defect closed duplicate (duplicate)

Session cookie under python3 never found

Reported by: elliotta Owned by: Pawel
Priority: normal Milestone: Twisted 16.1
Component: web Keywords:
Cc: jeff@… Branch:
Author:

Description

Under python 3.5.0 and Twisted trunk from svn, Request.getSession() never finds a matching session.

Looking in my browser's logs, I see a cookie name "b'TWISTED_SESSION'" instead of "TWISTED_SESSION". I confirmed this on the python side by adding a print statement to web.http's Request class's getCookie function, where I see that the key is b'TWISTED_SESSION' and the cookie is b"b'TWISTED_SESSION". So instead of returning the existing session, a new session is created each time.

Attachments (1)

8077.diff (2.5 KB) - added by Pawel 2 years ago.

Download all attachments as: .zip

Change History (11)

Changed 2 years ago by Pawel

Attachment: 8077.diff added

comment:1 Changed 2 years ago by Pawel

Keywords: review added

Simple example to reproduce this:

import sys
from twisted.python import log
from twisted.web import server, resource
from twisted.internet import reactor

class Simple(resource.Resource):
    isLeaf = True
    def render_GET(self, request):
        request.getSession()
        return b"<html>Hello, world!</html>"

log.startLogging(sys.stdout)
site = server.Site(Simple())
reactor.listenTCP(8080, site)
reactor.run()

Above works ok in python2 but when you run with python3 it produces following output containing invalid cookie name:

/d/twisted> curl -v "http://localhost:8080"
* Rebuilt URL to: http://localhost:8080/
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.35.0
> Host: localhost:8080
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Mon, 23 Nov 2015 17:17:56 GMT
* Server TwistedWeb/15.4.0 is not blacklisted
< Server: TwistedWeb/15.4.0
< Content-Type: text/html
< Content-Length: 26
< Set-Cookie: b'TWISTED_SESSION'=6840c840a395b181332960f34344173a; Path=b'/'

It was caused by different behavior of addCookie in cases where cookie name is byte string. It was happening because of line 1073 of twisted.web.http, there is different behavior of old style % formatting of byte strings between python versions.

In python3

In [3]: "%s=%s" % (b'aa', b'bb')
Out[3]: "b'aa'=b'bb'"

in python2:

In [1]: "%s=%s" % (b'aa', b'bb')
Out[1]: 'aa=bb'

Fix in attached diff file.

Last edited 2 years ago by Pawel (previous) (diff)

comment:2 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to Pawel

Many thanks for discovering and reporting this bug.

I think that the fix should be in the addCookie code... and if required in the getCookie... but I hope that addCookie is enough.

cookie names and all its components should be handled bytes.

The new tests requires a docstring describing its purpose :)

I think that the new test should be dedicated to addCookie and check that it sets the right cookie name on the wire, in the serialized format.

The integration tests for default session cookie is also fine.

Do you also get this bug in the released Twisted 15.4.0 ?

Again, many thanks for your contribution.

In case the spam filter produce false positives please feel fee to join #twisted-dev and I will try to help.

PS: Here are some tests I wrote for the cookie handling part.. which are not yet merged. Maybe they can help :) https://github.com/twisted/twisted/compare/trunk...secure-cookie-6932-2

comment:3 Changed 2 years ago by Pawel

Keywords: review added
Owner: changed from Pawel to Adi Roiban

cookie names and all its components should be handled bytes.

yes you are right. This is what happens now if you look at line 1010 of twisted.web.http it converts cookie strings to bytes by using NetworkString utility from compat. addCookie appends cookies to self.cookies and write() converts them to bytes. At the moment addCookie() function does not need bytes, it accepts unicode or ascii strings all right, and seems like this is what it always done so I think we should keep this behavior.

We could add some update to addCookie but only update that seems meaningful IMO is replacing old style modulo style formatting with plain string.format() - benefit of format() is that if user passes byte string as cookie part it will be correctly converted to unicode in python3 and to 8 bit string in Python2. You think we should do this?

Do you also get this bug in the released Twisted 15.4.0 ?

I use code from github twisted trunk branch so probably yes.

I think that the new test should be dedicated to addCookie and check that it sets the right cookie name on the wire, in the serialized format.

cookie is written to request in write() from line 958 onwards. This is big function that does lots of things so could be difficult to write actual unit test. But I will check what we can do here, more tests is always better!

Last edited 2 years ago by Pawel (previous) (diff)

comment:4 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Pawel

Your tests is very good as it checks the whole session integration, but I would also like to have a more focused "unit" test.

The integration test can tell us that some functionality is broken, and the unit test can pin-point the source of this error.

If the results are only observed when addCookie is used together with a write, then the unit test for addCookie will use the write() call.

I agree that this parts is not state of the art. Feel free to extract the cookie writing part into a private method which can be used to help write a test for the addCookie code.


Regarding the string formating please check the discussion from here https://twistedmatrix.com/trac/ticket/5911

I am not sure where do you want to change the string formating, so mabye you can just ignore that discussion.

As long as your changes don't break backward compatibilty, feel free to refactor the code.

I guess the the trouble line is

cookie = '%s=%s' % (k, v)

Maybe #5911 can still help with some example of unit tests... and there are also some unit teste in the unmerged code from herehttps://github.com/twisted/twisted/compare/trunk...secure-cookie-6932-2

Looking forward for a new patch.

Thanks!

comment:5 Changed 2 years ago by Jeffrey C. Ollie

Cc: jeff@… added

Any progress on this?

comment:6 Changed 22 months ago by SamyCookie

While I was working to port my project (which use a Twisted HTTP server with templating), I got this bug with wrong cookie session, mainly due to bad types conversion (str/bytes).

The first issue is well detected and explain by pawelmhm, but my current and personal fix was rewriting addCookie to work with 'bytes' instead of 'str' as recommended by #8067. example: https://github.com/SamyCookie/twisted/commit/13cec543558689a00e37f86931a63f3c8bcd3406

The second one, with Python 3, session's uid are generated as 'str' by '_mkuid' and stored as keys in 'Site.sessions', but 'Request.getSession' (line 399) try retreive sessions by sending to 'Site.getSession' an uid in 'bytes'. In that case, sessions cannot be found because types are different. See my proposed solution for that at #8215.

comment:7 Changed 22 months ago by hawkowl

Milestone: Twisted 16.1

comment:8 Changed 22 months ago by Adi Roiban

SamyCookie's #8215 landed on trunk and made a few improvements in this direction.

It his ticket/issue still valid or with #8215 everything works fine ?

Thanks!

comment:9 Changed 22 months ago by SamyCookie

No, #8215 does not fix this issue, but was a first step to that direction. It is even now worse, but expected. A cookie is now generated in client side is now like that

=> b'TWISTED_SESSION'=b'5624ef31d39275b12a7ee36ceb15fab0'

instead of

=> TWISTED_SESSION=5624ef31d39275b12a7ee36ceb15fab0

and previously #8215, it was

=> b'TWISTED_SESSION'=5624ef31d39275b12a7ee36ceb15fab0

Fixing #8067 will fix this current wrong behaviour in Python 3.

comment:10 Changed 22 months ago by Adi Roiban

Resolution: duplicate
Status: newclosed

ok. thanks. if this will be fixe in #8067 then this is a duplicate of #8067 :)

Note: See TracTickets for help on using tickets.