Skip to content

preserve cookie escaping for old servers - #1453

Merged
asvetlov merged 9 commits into
aio-libs:masterfrom
thehesiod:cookie-fix
Dec 8, 2016
Merged

asvetlov merged 9 commits into
aio-libs:masterfrom
thehesiod:cookie-fix

Conversation

@thehesiod

@thehesiod thehesiod commented Dec 6, 2016 •

Copy link
Copy Markdown
Contributor

this preserves the coded value so quotes won't get inserted and break cookie processing from certain servers like: http://hydro1.sci.gsfc.nasa.gov/data/NLDAS/NLDAS_FORA0125_H.002/2010/360/NLDAS_FORA0125_H.A20101226.0000.002.grb which uses: http://disc.sci.gsfc.nasa.gov/alerts/access-to-ges-disc-data-will-require-all-users-to-be-registered-with-the-earthdata-login-system

This updates aiohttp to mimic chrome's behavior in terms of preserving the cooking encoding.

@thehesiod

thehesiod commented Dec 6, 2016 •

Copy link
Copy Markdown
Contributor Author

w/o this fix you hit max_redirects trying to re-auth since it doesn't recognize the auth cookie it set

@thehesiod

Copy link
Copy Markdown
Contributor Author

btw, for changes like these do we bump up versioning? What's the versioning strategy?

@asvetlov

asvetlov commented Dec 6, 2016

Copy link
Copy Markdown
Member
  1. What the change exactly does?
  2. Where are tests for proving new behavior?
  3. Couple existing tests a broken, I suspect they are broken in unexpected way, e.g. domain info was dropped.

@thehesiod

thehesiod commented Dec 6, 2016 •

Copy link
Copy Markdown
Contributor Author

There are two values for a cookie, a coded_value and the non-coded value. Python always encodes the coded_value according to cookie version 1 rules (double quoting the value if the value contains certain characters). The issue is when you've received a version=0 value from a server w/o dbl quoting, it would then dbl quote it and send it back causing the server to not recognize its own cookie. Actually the test is failing because we're now preserving the domain whereas it was not before. I'll check to see what is correct. Btw if anything we now preserve more information than less.

Basically we should send cookies back in the format they were received.

@thehesiod

Copy link
Copy Markdown
Contributor Author

ok updated the unittests to expect the domain post filter, and added one to verify that double quoted values are preserved. I believe the tests were previously incorrect as if you do an "output" pre filter it now matches post-filter.

@thehesiod

thehesiod commented Dec 6, 2016 •

Copy link
Copy Markdown
Contributor Author

it's kinda confusing given there's http.cookies.SimpleCookie, not to be mistaken by http.cookiejar.Cookie which is expected by http.cookiejar.CookieJar.set_cookie. And then aiohttp has its own CookieJar.

@codecov-io

codecov-io commented Dec 6, 2016 •

Copy link
Copy Markdown

Current coverage is 98.83% (diff: 100%)

Merging #1453 into master will increase coverage by <.01%

@@             master      #1453   diff @@
==========================================
  Files            30         30          
  Lines          6939       6943     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       1149       1149          
==========================================
+ Hits           6858       6862     +4   
  Misses           40         40          
  Partials         41         41          

Powered by Codecov. Last update fec083d...2d58bae

@thehesiod thehesiod changed the title fix cookies with old servers preserve cookie escaping for old servers Dec 6, 2016
Comment thread tests/test_cookiejar.py Outdated
cookies_sent = jar.filter_cookies(URL("http://www.example.com/"))
assert cookies_sent.output(header='Cookie:') == "Cookie: cookie=val"
assert cookies_sent.output(header='Cookie:') \
== "Cookie: cookie=val; Domain=www.example.com; Path=/"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is wrong as this will appear as 3 cookies: cookie, Domain and Path.
Please see rfc for Cookie header http://httpwg.org/specs/rfc6265.html#sane-cookie
Its the Set-Cookie header that can accept domain/expire/path option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah, sorry I was figuring that would get filtered out later, I've updated the code to filter these out upfront

@asvetlov
asvetlov merged commit 254fa79 into aio-libs:master Dec 8, 2016
@asvetlov

asvetlov commented Dec 8, 2016

Copy link
Copy Markdown
Member

Thank you

@thehesiod

Copy link
Copy Markdown
Contributor Author

thanks! sorry about the misstep :)

@lock

lock Bot commented Oct 29, 2019

Copy link
Copy Markdown

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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants