Skip to content

Cookie filter#799

Closed
panda73111 wants to merge 28 commits intoaio-libs:masterfrom
panda73111:cookie_filter
Closed

Cookie filter#799
panda73111 wants to merge 28 commits intoaio-libs:masterfrom
panda73111:cookie_filter

Conversation

@panda73111
Copy link
Copy Markdown
Contributor

@panda73111 panda73111 commented Feb 22, 2016

Relevant issue: #792
This branch will add cookie management adhering to RFC 6265.
Right now, it adds a simple filter for cookies sent, regarding their "Domain" attribute.

  • Cookies passed to ClientSession without a "Domain" attribute (as SimpleCookie() or dict/tuple) are still shared across all requests, to keep backwards compatibility.
  • Leading dots are ignored (Domain=.example.com becomes Domain=example.com)
  • Cookies with matching domain are sent (Domain=example.com is sent to sub.example.com, but not the other way around)
  • Only shared cookies are sent to addresses that are not a domain, like an IP address

More filters (for the remaining attributes and for storing incoming cookies) will be added as soon as I have the time, until eventually ClientSession behaves like stated in RFC 6265.
CORS is completely new to me and low priority until further research.
This branch can be merged into master at any time, I'll add pull requests as needed and all tests pass.

  • The Domain Attribute
  • The Secure Attribute
  • The Path Attribute
  • The Expires Attribute
  • The Max-Age Attribute

EDIT 23.02.2016:

  • The Domain attribute of received cookies is set to the response hostname if empty, their host-only-flag is set to True
  • Cookies from IP addresses are rejected

EDIT 29.02.2016:

  • Cookies with secure-flag set are only sent to secure hosts (https://)

EDIT 18.03.2016:

  • Cookies with matching path are sent
  • If received cookies do not have the path-attribute defined, it is set to the response path

EDIT 20.03.2016:

  • Stored cookies are deleted when the time defined in the max-age- or expires-attribute is reached. A timer is set via loop.call_later()/call_at() when a cookie is added, that removes the cookie from the jar.

kxepal
kxepal reviewed Feb 22, 2016
View reviewed changes
aiohttp/client.py Outdated
"""Returns this session's cookies filtered by their attributes"""
# TODO: filter by 'expires', 'path', ...
netloc = urllib.parse.urlsplit(url).netloc
is_ip = IPV4_PATTERN.match(netloc) is not None
Copy link
Copy Markdown
Member

@kxepal kxepal Feb 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipaddress.IPAddress(netloc) to support both IPv4 and IPv6 in the right way without regexps (which accepts invalid IPv4 addresses).

@asvetlov asvetlov added this to the 0.22 milestone Feb 24, 2016
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Feb 24, 2016

Looks interesting. I need reread RFC first for making careful review.
I hope to make it during weekend.

Anyway I have a strong feeling that we need to fix cookies problem in the next aiohttp release.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 18, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.24% when pulling 4959234 on panda73111:cookie_filter into 496779e on KeepSafe:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 19, 2016

Coverage Status

Coverage decreased (-0.3%) to 98.119% when pulling 1cfb2c6 on panda73111:cookie_filter into 496779e on KeepSafe:master.

@panda73111
Copy link
Copy Markdown
Contributor Author

panda73111 commented Mar 20, 2016

I added the Expires- and Max-Age-attributes, by scheduling deletion of the cookie via call_later()/call_at(). But this gets problematic as soon as the user changes these attributes, the cookie would still be removed at the original timestamp. How should this be handled? For "Expires", an absolute date, the cookie could just remain in the cookie jar, and just not be sent if the time of sending the request is after the date defined in the "Expires" attribute. But "Max-Age" only consists of a time span in seconds, of which it is not possible to know when it has been set by the user.

@panda73111
Copy link
Copy Markdown
Contributor Author

panda73111 commented Apr 6, 2016

...Or maybe any other comments? This is basically done, as it is.

@panda73111
Added more tests
7917132 
(invalid values, host only cookie, domain matching, path matching, date parsing)
@auvipy
Copy link
Copy Markdown

auvipy commented Apr 24, 2016

you could fix the merge conflicts and failing builds

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 24, 2016

Coverage Status

Coverage decreased (-1.1%) to 97.068% when pulling f8c0e9a on panda73111:cookie_filter into 024f6e1 on KeepSafe:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 24, 2016

Coverage Status

Coverage increased (+0.04%) to 98.26% when pulling f6a5da4 on panda73111:cookie_filter into 024f6e1 on KeepSafe:master.

@panda73111
Copy link
Copy Markdown
Contributor Author

panda73111 commented Apr 24, 2016

Well, I wanted my code to get approved first, since I don't want to create a merge commit every time master becomes incompatible, until this gets merged or rejected eventually.

And what's up with these coverage comments? Shouldn't these be posted only after all tests pass? The next commit makes it up to 98.26% again only because flake8 is satisfied...
(Also, coveralls doesn't let me view 'changed' lines, because "The owner of this repo needs to re-authorize with github; their OAuth credentials are no longer valid so the file cannot be pulled from the github API.")

@KostyaEsmukov
Copy link
Copy Markdown

KostyaEsmukov commented May 23, 2016

Any update on this?
@asvetlov @fafhrd91

fafhrd91
fafhrd91 reviewed May 23, 2016
View reviewed changes
aiohttp/helpers.py
return non_matching.startswith("/")

@classmethod
def _parse_date(cls, date_str):
Copy link
Copy Markdown
Member

@fafhrd91 fafhrd91 May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we reuse some existing function?

@fafhrd91
Copy link
Copy Markdown
Member

fafhrd91 commented May 23, 2016

i am +1 for this change, but we need opinion from @asvetlov before inclusion.

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Jun 3, 2016

Squashed and merged as 0016999
@panda73111 thanks you very much for your hard work

@asvetlov asvetlov closed this Jun 3, 2016
@asvetlov asvetlov mentioned this pull request Jun 3, 2016
Closed
asvetlov pushed a commit that referenced this pull request Jun 3, 2016
@panda73111 @asvetlov
Implement cookie filter #799
0016999
@asvetlov asvetlov mentioned this pull request Jul 16, 2016
Closed
@lock
Copy link
Copy Markdown

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

0.22

Development

Successfully merging this pull request may close these issues.

7 participants

@panda73111 @asvetlov @coveralls @auvipy @KostyaEsmukov @fafhrd91 @kxepal