Opened 4 years ago

Closed 11 months ago

#5911 enhancement closed fixed (fixed)

Support the 'HttpOnly' flag when setting cookies

Reported by: reed Owned by: hawkowl
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, d.vinella@… Branch: branches/web-http-cookie-httponly-5911-5
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

The Twisted 'web' module doesn't support the addition of the 'HttpOnly' flag when setting a cookie.

http://www.owasp.org/index.php/HTTPOnly

Attachments (7)

support_httponly_flag.diff (1.3 KB) - added by reed 4 years ago.
Add support for HttpOnly flag in cookies
support_httponly_flag.2.diff (2.6 KB) - added by reed 4 years ago.
Same as above but includes a test
5911-3.patch (4.1 KB) - added by d.vinella 12 months ago.
5911-4.patch (4.3 KB) - added by d.vinella 12 months ago.
5911-5.patch (4.3 KB) - added by d.vinella 11 months ago.
5911-6.patch (4.3 KB) - added by d.vinella 11 months ago.
5911-7.patch (4.3 KB) - added by d.vinella 11 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 4 years ago by reed

Add support for HttpOnly flag in cookies

comment:2 Changed 4 years ago by reed

  • Keywords review added

While Python has supported the 'HttpOnly' flag in cookies for quite a while (http://bugs.python.org/issue1638033), it doesn't look like there's a 'httponly' attribute, at least according to http://docs.python.org/library/cookielib.html#cookie-objects. I will follow-up with that to see if that's true, and if so, get it fixed upstream.

New patch with a test coming up.

Changed 4 years ago by reed

Same as above but includes a test

comment:3 Changed 4 years ago by therve

  • Keywords review removed
  • Owner set to reed

Thanks for your patch! There is one main problem:

  1. Your test doesn't match your change. The test applies to the client, not server Request.addCookie.

Changed 12 months ago by d.vinella

comment:4 Changed 12 months ago by d.vinella

  • Keywords review added
  • Owner reed deleted

A new patch with correct tests. Please review!

comment:5 Changed 12 months ago by d.vinella

  • Cc d.vinella@… added

comment:6 Changed 12 months ago by adiroiban

  • Author set to adiroiban
  • Branch set to branches/web-http-cookie-httponly-5911

(In [45878]) Branching to web-http-cookie-httponly-5911.

comment:7 Changed 12 months ago by adiroiban

  • Keywords review removed
  • Owner set to d.vinella

Many thanks for you work on this!


Can you please also document the types of the parameters ?

We want them to be bytes... so we should let users know that no explicit encoding is done at this level and that they should not pass text/Unicode.

Also, the test should be written with explict bytes marker b'key' instead of just 'b'

We should try to write code so that it is Py3 friendly ... and in Py3 all texts are now Unicode.


Maybe rename test_addCookie - > test_addCookieWithMinimumArguments

And have a docstring like: Add a 'Set-Cookie' header with just key and value.


Similar: test_addCookieWithAttibutes - > test_addCookieWithAllArguments

Docstring: Add a 'Set-Cookie' header with key and value and all the supported options.


assertIn is pretty weak... can we use assertEqual ? I case the order is not the same, can we use a cookie parser to read the result and check it in that way.

My problem is that assertIn tests will still pass even there is junk between the asserted bits. ... for example 'foo=bar ; junk HttpOnly'


Please check my comments and resubmit.

I have sent your previous patch to the builders.

Thanks!

Changed 12 months ago by d.vinella

comment:8 Changed 12 months ago by d.vinella

  • Keywords review added
  • Owner d.vinella deleted

Thanks for your review, Adi. Here it is a new patch with all the changes. Please review

comment:9 Changed 12 months ago by adiroiban

  • Branch changed from branches/web-http-cookie-httponly-5911 to branches/web-http-cookie-httponly-5911-2

(In [45886]) Branching to web-http-cookie-httponly-5911-2.

comment:10 Changed 12 months ago by adiroiban

  • Keywords review removed
  • Owner set to d.vinella

Changes look good but test are failing with the latest patch.

Please check the buildbot results https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/web-http-cookie-httponly-5911-2

Please fix the test and resubmit.

Since the http module is ported on Py3 we need to make sure changes work on both python 2.7 and a recent py3 version... ex 3.4

Thanks!

Changed 11 months ago by d.vinella

comment:11 Changed 11 months ago by d.vinella

  • Keywords review added
  • Owner d.vinella deleted

This patch passes tests on py3.5 too, please review!

comment:12 Changed 11 months ago by adiroiban

  • Branch changed from branches/web-http-cookie-httponly-5911-2 to branches/web-http-cookie-httponly-5911-3

(In [45890]) Branching to web-http-cookie-httponly-5911-3.

comment:13 Changed 11 months ago by adiroiban

  • Keywords review removed
  • Owner set to d.vinella

There are still some errors on py 3.4

builtins.TypeError: unsupported operand type(s) for %: 'bytes' and 'tuple'

twisted.web.test.test_http.RequestTests.test_addCookieWithMinimumArguments

Please try to fix them and submit a new patch.

Thanks!

Changed 11 months ago by d.vinella

comment:14 Changed 11 months ago by d.vinella

  • Keywords review added
  • Owner d.vinella deleted

Here it is a new patch, please review!

One concern I'd like to raise is the potential api breaking change for the max_age param: previously it would accept strings and ints, it was undocumented and never used anywhere in the twised (nor nevow) codebase. Now it is more restrictive and it accepts ints only. If anyone out there is using max_age with a string (or bytes), it will break for her. I don't know if some isinstance trick is stylistically acceptable or should be avoided so any advice is welcome for this review.

comment:15 Changed 11 months ago by adiroiban

  • Branch changed from branches/web-http-cookie-httponly-5911-3 to branches/web-http-cookie-httponly-5911-4

(In [45949]) Branching to web-http-cookie-httponly-5911-4.

comment:16 Changed 11 months ago by adiroiban

Thanks for your patch. It looks good.

I don't know how to proceed with the max_age.

I would say that we should continue to accept str/bytes but raise a deprecation warning.

Also, maybe we can deprecate the whole max_age argument and add a new maxAge argument to comply with the coding standard.

I will open a discussion on the mailing list to get some feedback.

I am leaving this as review so that other can provide their feedback.

Many thanks!

comment:17 Changed 11 months ago by hawkowl

  • Keywords review removed
  • Owner set to d.vinella

Hi! Thanks for the patch.

I think we can sidestep this issue because at https://github.com/twisted/twisted/blob/trunk/twisted/web/http.py#L1010 , it is changed from a str to bytes as the cookies are added. This ensures that all keys/values are ASCII-encodable as well, while concatenating bytes has no such check -- and I believe things get a bit wacky with non-ASCII headers, so we should keep the check anyway.

Please revert the str -> bytes changes, and document the API as taking str for now, and we can deal with addCookie taking str and not bytes in another ticket (I filed #8067 for it).

The rest of the API docs and changes look fine, just put up another patch with the str/bytes changes reverted as above, and resubmit for review.

Changed 11 months ago by d.vinella

comment:18 Changed 11 months ago by d.vinella

  • Keywords review added
  • Owner d.vinella deleted

Thank you for your review, hawkowl. Here it is the patch with str inputs. Please review!

comment:19 Changed 11 months ago by adiroiban

  • Branch changed from branches/web-http-cookie-httponly-5911-4 to branches/web-http-cookie-httponly-5911-5

(In [46001]) Branching to web-http-cookie-httponly-5911-5.

comment:20 Changed 11 months ago by adiroiban

Thanks for the patch.

I have sent the patch to buildbots and they look green.

I will let hawkowl to the final review... as at this stage I am confused regarding the handling of str/unicode and bytes in Twisted Web.

Thanks again for your contribution.

comment:21 Changed 11 months ago by hawkowl

  • Keywords review removed
  • Owner set to d.vinella

Hi, thanks for the updated patch!

The code is all good, but I'd like the topfile to mention why this is a good change -- what benefit httpOnly gives twisted.web users, so they know why they should use our spiffy new features :)

You don't have to upload another patch, just comment with it here and I'll amend it before merging. :)

comment:22 Changed 11 months ago by d.vinella

  • Keywords review added
  • Owner d.vinella deleted

Thank you for the review, hawkowl, the extendend topfile line could be like this

twisted.web.http.Request.addCookie now supports httpOnly attribute to set 
cookies that the browser will not expose through channels other than HTTP 
and HTTPS requests (i.e. they will not be accessible through JavaScript).

plus an additional, optional, extension

These are are suitable to securely store auth tokens.

comment:23 Changed 11 months ago by hawkowl

  • Keywords review removed
  • Owner set to hawkowl

Thanks! I'll merge this when I get back into work tomorrow :)

comment:24 Changed 11 months ago by hawkowl

  • Resolution set to fixed
  • Status changed from new to closed

(In [46010]) Merge web-http-cookie-httponly-5911-5: Support the 'HttpOnly' flag when setting cookies

Author: d.vinella Reviewers: adiroiban, hawkowl Fixes: #5911

Note: See TracTickets for help on using tickets.