Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#8215 defect closed fixed (fixed)

Python 3: Session uid's are built to wrong type

Reported by: SamyCookie Owned by: Adi Roiban
Priority: normal Milestone: Twisted 16.1
Component: web Keywords:
Cc: Branch: branches/http-session-id-type-8215
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

As 'web.server.Session' documentation said at line 547, its uid should be 'bytes'. However in Python 3 they are made to 'str'. This behaviour causes issues with cookies retrieval (see #8077).

see my proposed solution at https://github.com/SamyCookie/twisted/commit/9ca7d578536e47baa6bb0b0e75e5bceb47b5ff65

Attachments (2)

wrong_uid_type-8215-1.patch (1.7 KB) - added by SamyCookie 22 months ago.
8215.diff (2.2 KB) - added by SamyCookie 22 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 22 months ago by SamyCookie

Summary: Python 3: Session uid's are built in wrong typePython 3: Session uid's are built to wrong type

Changed 22 months ago by SamyCookie

Attachment: wrong_uid_type-8215-1.patch added

comment:2 Changed 22 months ago by SamyCookie

Any thought about this ticket ?

comment:3 Changed 22 months ago by Thijs Triemstra

Keywords: review added

comment:4 Changed 22 months ago by hawkowl

Milestone: Python-3.xTwisted 16.1

This is a bit of a nasty bug that slipped under my radar.

It's not going to block 16.0 but I'll file it under things to block 16.1.

comment:5 Changed 22 months ago by Adi Roiban

Keywords: review removed
Milestone: Twisted 16.1
Owner: set to SamyCookie

Hi,

Thanks for the patch.

If you want feedback for your patch make sure the ticket is listed in the review queue https://twistedmatrix.com/trac/report/25 ... that is, it has the "review" keyword

Major comments:

  1. I think that the new test should be writen against makeSession and getSession as this is the public API and this should be the thing which breaks.

You can test against self.sessions[uid] ... not sure why this is public... but I would say that self.session should be make private and only use self.getSession

  1. The patch also needs a news file fragment so that the fix is advertise in the release notes. see https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Minor comments:

  1. I would prefer to see a new test on the public API which demonstrate how the current code fails for your usage :)
  1. Do we really need the local imports?

Please review my comments and resubmit a new patch.

Thanks again

comment:6 Changed 22 months ago by Adi Roiban

Milestone: Twisted 16.1

put back the milestone. sorry.

Changed 22 months ago by SamyCookie

Attachment: 8215.diff added

comment:7 Changed 22 months ago by SamyCookie

see uploaded patch or https://github.com/twisted/twisted/compare/trunk...SamyCookie:%238215

Major 1 and 2: done Minor 1: it should be done in #8077. This issue is only a minor part of this one. Minor 2: as you want, I just kept the previous written logic.

additionnal note: for more consistency, may it be interesting to use and store UUID type from Python standard library https://docs.python.org/2/library/uuid.html ?

comment:8 Changed 22 months ago by Adi Roiban

Keywords: review added

I assume your latest patch is ready for review.

Please don't forget to add the "review" keyword.

comment:9 Changed 22 months ago by Adi Roiban

Author: adiroiban
Branch: branches/http-session-id-type-8215

(In [46915]) Branching to http-session-id-type-8215.

comment:10 Changed 22 months ago by Adi Roiban

Thanks for the update.

Minor comments:

  1. The news fragment are fragment of our release notes and their tone should be that of a release note and not of a commit message. The wiki page contains a few examples and documentation about how to write those fragments.
  1. getSession documentation should be updated to inform that uid is of type bytes.
  1. test_getSession is in fact 2 tests :)
  1. Please avoid reusing the same variable name for different things and try to use names which describe the purpose / usage of a variable :)

I have update the branch based on the minor comments, so there is no required action on your side.. other than reviewing my changes :)

Since I have also made changes to this branch, it will need to be reviewed by someone else.

Will try to find another reviewer to check that changes. Hope this can be done soon.

Thanks again for your contribution. Much appreciated.

comment:11 Changed 22 months ago by hawkowl

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

Hi Adi,

Two things:

  • getSession's docstring needs an L{} where "keyerror" is mentioned
  • the test docstrings shouldn't use "it" (that is, change "it" to L{site.getSession})

Fix these two then merge. Thanks SamyCookie and Adi for the work on this.

comment:12 Changed 22 months ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46924]) Merge http-session-id-type-8215: In twisted.web use bytes for session id.

Authors: SamyCookie, adiroiban Reviewers: adiroiban, hawkowl Fixes: #8215

comment:13 Changed 22 months ago by Thijs Triemstra

Noticed that the topfile for this change wasn't placed in the correct topfiles folder.

comment:14 Changed 22 months ago by hawkowl

Gah -- thanks for spotting that. Added an item in the release managers notes (https://twistedmatrix.com/trac/wiki/ReleaseManagerNotes) so I can fix this in the 16.1 release.

comment:15 Changed 22 months ago by SamyCookie

Oops, sorry for the wrong topfile location, I'll be more careful next time :/ Thanks for helping me and merging this.

Note: See TracTickets for help on using tickets.