Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookies for different domains with same name disappear #1125

Closed
JeckLabs opened this issue Aug 25, 2016 · 14 comments
Closed

Cookies for different domains with same name disappear #1125

JeckLabs opened this issue Aug 25, 2016 · 14 comments

Comments

@JeckLabs
Copy link

It seems that cookies are stored in key / value dictionary that causing the problem in the storing cookies from different domains / subdomains in one session.

with aiohttp.ClientSession() as client:

    async with client.get('https://httpbin.org/cookies/set?test=ok') as resp:
        logging.info(await resp.json())

    async with client.get('https://httpbin.org/cookies') as resp:
        logging.info(await resp.json())

    async with client.get('https://http2bin.org/cookies/set?test=fail') as resp:
        logging.info(await resp.json())

    async with client.get('https://httpbin.org/cookies') as resp:
        logging.info(await resp.json())

    logger.info(client.cookies)
2016-08-25 21:57:38 INFO {'cookies': {'test_cookie': 'ok'}}
2016-08-25 21:57:39 INFO {'cookies': {'test_cookie': 'ok'}}
2016-08-25 21:57:39 INFO {'cookies': {'test_cookie': 'fail'}}
2016-08-25 21:57:39 INFO {'cookies': {}}
@asvetlov asvetlov added this to the 1.0 milestone Aug 30, 2016
@iglv
Copy link

iglv commented Aug 31, 2016

We are experiencing the same problem.

JeckLabs pushed a commit to JeckLabs/aiohttp that referenced this issue Sep 5, 2016
@JeckLabs
Copy link
Author

JeckLabs commented Sep 5, 2016

Some WIP  JeckLabs@2259d07

@asvetlov
Copy link
Member

asvetlov commented Sep 5, 2016

Your WIP hack http.cookiejar.Cookie in ugly way, using very private API.
It's not acceptable.

Another question scaring me is: what to do with ClientSession.cookies?

@JeckLabs
Copy link
Author

JeckLabs commented Sep 5, 2016

Hmm, I think I have not used any private API, only public methods, and mock objects. That is the way requests do (see - http://docs.python-requests.org/en/master/_modules/requests/cookies/ ). I agree that's ugly, but allows use standard library code without copy/paste.

I think ClientSession.cookies return a list of Cookie in master. I fix that in JeckLabs@668cbee#diff-7dd84b5ef8d5eea2de1dfc5329411dfcL497 but maybe I missing something.

And now I have problems with tests:

In https://github.com/KeepSafe/aiohttp/blob/master/tests/test_client_functional.py#L1410 setting 3 cookies but expecting only 2.

In https://github.com/KeepSafe/aiohttp/blob/master/tests/test_client_functional.py#L1395 security flag is set but expecting receive cookie through insecure HTTP protocol.

@asvetlov
Copy link
Member

asvetlov commented Sep 5, 2016

I know how requests is implemented and considering it very dirty hack.
Better to copy-paste than mocking.
If aiohttp.CookieJar has bugs -- we should fix them.
Borrowing http.cookiejar implementation ideas is perfectly fine though.

ClientSession.cookies is SimpleCookie actually with dict-like interface.

@JeckLabs
Copy link
Author

JeckLabs commented Sep 5, 2016

So I can copy lib from https://hg.python.org/cpython/file/tip/Lib/test/test_http_cookiejar.py rewrite some method, and then copy tests from https://hg.python.org/cpython/file/tip/Lib/test/test_http_cookiejar.py , it's ok? Looks a bit tricky...

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2016

No, it's not OK.
I've wrote borrow implementation ideas but not "copy-paste the whole documentation along with obsolete modes and unused parts".
Do you want to use cookie policies? Do you want to support netscape-styled cookies? Really?

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2016

And, please, before starting to hack please tell me what do you want to do with ClientSession.cookies?

@JeckLabs
Copy link
Author

JeckLabs commented Sep 6, 2016

I can implement wrapper with SimpleCookie-like interface, it's relatively easy.
Existing aiohttp.CookieJar can be fixed to support multi-domain cookies, but I am don't understand what existed tests do, there may be some more bugs. It seems a lot of work to implement correct cookies support from scratch.

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2016

SimpleCookie is dict-like object where keys are cookie names.
It's accessible by cookies[name].
But for supporting non-overlapped domains the access should be cookies[domain][name].

@JeckLabs
Copy link
Author

JeckLabs commented Sep 6, 2016

Yes, cookies[doman][path][name] to be precise.
For backwards compatibility, I think your can leave SimpleCookie interface for ClientSession.cookies. Advanced users may access cookies structure via some method or additional property.

@asvetlov
Copy link
Member

asvetlov commented Sep 7, 2016

Yesterday I've tried to port http.cookiejar into aiohttp with appropriate API changing.
I've found the library has very ugly implementation and the only 85% test covered (which means all corner cases are not covered at all).
I just don't trust it and not sure if it works properly with all cookie flawors supposed to be supported by the library (it has a code for obsolete and outdated standards not used nowdays anymore).

aiohttp.CookieJar is much cleaner. I'm going to fix it's bugs.

@asvetlov asvetlov modified the milestones: 1.1, 1.0 Sep 16, 2016
@asvetlov
Copy link
Member

Fixed by #1173

@lock
Copy link

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.
Projects
None yet
Development

No branches or pull requests

3 participants