Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8067 defect closed fixed (fixed)

twisted.web.http.Request.addCookie should handle both bytes and str arguments

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Twisted 16.1
Component: web Keywords:
Cc: Branch: branches/addCookie-str-8067
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description (last modified by hawkowl)

A potential thing to be mindful of is http://stackoverflow.com/questions/4400678/http-header-should-use-what-character-encoding -- the current implementation enforces ASCII, but users may wish to put random bytes into the cookie if they so wish, but enforcing ASCII is a good default.

Change History (17)

comment:1 Changed 3 years ago by SamyCookie

I agree with this ticket, 'addCookie' with str parameters provoke #8077 issue in Python 3.

see my proposed solution there: https://github.com/twisted/twisted/compare/trunk...SamyCookie:8067

Last edited 3 years ago by SamyCookie (previous) (diff)

comment:2 Changed 3 years ago by hawkowl

Headers now operates on str and bytes, so I think it's reasonable to allow this to handle both properly.

comment:3 Changed 3 years ago by hawkowl

Description: modified (diff)
Summary: twisted.web.http.Request.addCookie takes str parameters, not bytestwisted.web.http.Request.addCookie should handle both bytes and str arguments

comment:4 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/addCookie-str-8067

(In [46936]) Branching to addCookie-str-8067.

comment:5 Changed 3 years ago by SamyCookie

I have not checked if it is working with str, but I suppose it is. Some tests are now failing with my changes, I will propose a better patch.

comment:6 Changed 3 years ago by Adi Roiban

This should be a duplicate of #8077

comment:7 Changed 3 years ago by hawkowl

@SamyCookie I've got a patch in the works, I should have it up for review when I'm back from food :)

comment:8 Changed 3 years ago by hawkowl

There's some implementation details that are similar to http://twistedmatrix.com/trac/ticket/8129 -- so I'm going to do something like that (handling both bytes and unicode). Unicode is fine for 99% of cases, and the bytes fallback allows people to send binary garbage or other encodings fine.

comment:9 Changed 3 years ago by Adi Roiban

Milestone: Twisted 16.1

+1 for more Unicode love :)

comment:10 Changed 3 years ago by SamyCookie

@hawkowl Ok, I'll wait your solution for review. I was trying last hour to propose mine, but Python 2/3 encodings make me crazy.

+1 either for a bytes/str/unicode agnostic solution !

comment:11 Changed 3 years ago by hawkowl

Keywords: review added

Okay, I'm reasonably happy with this. Builders spun, please review.

comment:12 Changed 3 years ago by SamyCookie

It was pretty much that I want to write (but I failed to write an _ensureBytes function correctly :/ ), hence I am totally OK with the logic.

Minor: just saw L1540 and L1595, you forgot to reuse 'expectedCookieValue' variable.

Tests are fine but seems redundant. Is it possible to factorize them ?

comment:13 Changed 3 years ago by SamyCookie

The proposed patch is correctly fixing #8077 wrong behaviour to me.

comment:14 Changed 3 years ago by Adi Roiban

Here is my quick review

I remember that someone else tried to fix this and we hit a problem when the max_age was passed as int.

Major comment:

1 . I think that this branch also needs a removal note as from now on max_age will no longer accept an int... it used to work with an int.

Minor comment:

  1. Maybe we can have a helper self.assertCookies([expectedCookieValue], req) which should reduce the duplicate code for req.write() and then split and then filter the lines.
  1. Do we need the special handling of None in _ensureBytes ? It will fail anyway.

Leaving this for review

Thanks!

comment:15 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Please see my the previous comments, mine and from SamyCookie

Since SamyCookie also commented about code duplication in test I would take it as a major issue.

Please address the comments and merge.

Thanks!

comment:16 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [46977]) Merge addCookie-str-8067: Make twisted.web.http.Request.addCookie take bytes and unicode arguments

Author: hawkowl Reviewers: SamyCookie, adiroiban Fixes: #8067

comment:17 Changed 3 years ago by SamyCookie

Thanks for fixing this, cookie sessions should be OK now with Python 3 (#8077).

Note: See TracTickets for help on using tickets.