Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6119 defect closed fixed (fixed)

twisted.web.http.Request.parseCookies has incomplete test coverage

Reported by: Jean-Paul Calderone Owned by: lvh
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/parsecookies-coverage-6119
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

It should have complete test coverage (found while porting http.py to Python 3).

Attachments (2)

6119.patch (685 bytes) - added by moijes12 4 years ago.
6119.2.patch (1.5 KB) - added by moijes12 4 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

Changed 4 years ago by moijes12

Attachment: 6119.patch added

comment:2 Changed 4 years ago by moijes12

Keywords: review added

Hi

As far as I can see, there are tests for verifying that http.Request parses cookies in the below ways:

  1. Extract cookies from header and add them to recieved_cookies.
  2. Extract cookies from multiple cookie headers.

I think there is not much to add here except add a check to see that parseCookies function returns None in case there is no cookie header. This is implemented by the below line in parseCookie

if cookieheaders is None:

return

I have added the test for the above check. May be we could check that recieved_cookies is empty if the cookie header was not contained in the request. We could implement that if required.

comment:3 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to moijes12
  1. parseCookies doesn't have an interesting return value, so there is no reason to assert anything about it. But you are right that that code path isn't covered. If there isn't any headers, then no cookies should be parsed.
  2. The except ValueError: is also not covered. You should test that malformed cookie-pair's are ignored (and other cookies set before or after are still properly parsed.

Changed 4 years ago by moijes12

Attachment: 6119.2.patch added

comment:4 Changed 4 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

I am attaching a patch which has the test covering the "ValueError" codepath i.e. it tests that malformed-cookie pair's are ignored. Here I have added 2 tests in a single test function.

  1. Test that malformed cookie-pair before, in middle or at end of header are ignored.
  1. Test that malformed URLs in multiple headers cookies are ignored as in 1.

I have used the code in test_parseCookie and test_parseCookiesMultipleHeaders for achieving this.

comment:5 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/parsecookies-coverage-6119

(In [39650]) Branching to 'parsecookies-coverage-6119'

comment:6 Changed 4 years ago by Richard Wall

(In [39651]) Apply 6119.2.patch from moijes12. Refs: #6119

comment:7 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to moijes12

Thanks moijes12,

Notes:

  • Merges cleanly
  • Coverage now seems complete

Points:

  1. Build Failures
    1. Trailing whitespace: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1150/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  2. source:branches/parsecookies-coverage-6119/twisted/web/test/test_http.py
    1. Should be two blank lines before "def test_parseCookiesNoCookie(self):"
    2. Builtin types such as None should be marked up with L{}
    3. test_parseCookiesNoCookie tests for a None return value, but the function always returns None. So perhaps there should be separate tests for return type and another test that req.received_cookies is empty if no cookies are supplied.
    4. test_parseCookiesMalformedCookie should probably be split into two separate tests.
    5. parseCookies does an lstrip on the value so add a test for leaving right hand space in the cookie value.
    6. And a test for removing leading space from cookie key
    7. parseCookies allows cookies with empty values. Add a test for that.
  3. parseCookie seems inadequate in various ways:
    1. Allows empty keys " = "
    2. Allows spaces in values "foo = bar baz; "
    3. Misinterprets quoted keys '"foo=bar";baz=qux'
    4. Raise a ticket

Ok. This started as a code review but I also fixed some things as a went.

So I'll force a new build and put it back up for review.

-RichardW.

comment:8 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: moijes12 deleted

comment:9 Changed 4 years ago by lvh

Keywords: review removed
Owner: set to lvh

Hi rwall,

Thanks for working on this!

The twistedchecker buildbot noted an issue:

* Module twisted.web.test.test_http W9010:1486,0: Trailing whitespace found in the end of line

Other than that, these tests look very reasonable, modulo the already existing bogus behavior which is addressed in another ticket.

Since these are two very minor changes, I'll do them.

comment:10 Changed 4 years ago by lvh

Resolution: fixed
Status: newclosed

(In [39897]) Merge parsecookies-coverage-6119: add tests for cookie parsing

Author: rwall, moijes12 Reviewer: lvh, rwall, tom.prince Fixes: #6119

The cookie parsing logic previously had no test coverage. Now it does.

comment:11 Changed 4 years ago by Jean-Paul Calderone

The branch is also introducing tests that pass None as a false value for a flag parameter (Request.__init__ queued). This works but is weird. The code should pass False instead.

Note: See TracTickets for help on using tickets.