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

Combine cookies from original request and session file #932

Merged
merged 11 commits into from
Jun 18, 2020
55 changes: 54 additions & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,6 @@ Simple example:
$ http PUT httpbin.org/put name=John [email protected]

.. code-block:: http

PUT / HTTP/1.1
Accept: application/json, */*;q=0.5
Accept-Encoding: gzip, deflate
Expand Down Expand Up @@ -1739,6 +1738,60 @@ exchange after it has been created, specify the session name via
# But it is not updated:
$ http --session-read-only=./ro-session.json httpbin.org/headers Custom-Header:new-value

Cookie Storage Behaviour
------------------------

**TL;DR:** Cookie storage priority: Server response > Command line request > Session file

To set a cookie within a Session there are three options:

1. Get a `Set-Cookie` header in a response from a server

.. code-block:: bash

$ http --session=./session.json httpbin.org/cookie/set?foo=bar

2. Set the cookie name and value through the command line as seen in `cookies`_

.. code-block:: bash

$ http --session=./session.json httpbin.org/headers Cookie:foo=bar

3. Manually set cookie parameters in the json file of the session

.. code-block:: json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We tried to emphasize lines 12-18 in this code block using :emphasize-lines:, but we weren't able to get it to render properly.

{
"__meta__": {
"about": "HTTPie session file",
"help": "https://httpie.org/doc#sessions",
"httpie": "2.2.0-dev"
},
"auth": {
"password": null,
"type": null,
"username": null
},
"cookies": {
"foo": {
"expires": null,
"path": "/",
"secure": false,
"value": "bar"
}
}
}

Cookies will be set in the session file with the priority specified above. For example, a cookie
set through the command line will overwrite a cookie of the same name stored
in the session file. If the server returns a `Set-Cookie` header with a
cookie of the same name, the returned cookie will overwrite the preexisting cookie.

Expired cookies are never stored. If a cookie in a session file expires, it will be removed before
sending a new request. If the server expires an existing cookie, it will also be removed from the
session file.


Config
======

Expand Down
10 changes: 9 additions & 1 deletion httpie/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"""
import os
import re

from http.cookies import SimpleCookie
from pathlib import Path
from typing import Iterable, Optional, Union
from urllib.parse import urlsplit
Expand Down Expand Up @@ -76,7 +78,13 @@ def update_headers(self, request_headers: RequestHeadersDict):
continue # Ignore explicitly unset headers

value = value.decode('utf8')
if name == 'User-Agent' and value.startswith('HTTPie/'):
if name.lower() == 'user-agent' and value.startswith('HTTPie/'):
continue

if name.lower() == 'cookie':
for cookie_name, morsel in SimpleCookie(value).items():
self['cookies'][cookie_name] = {'value': morsel.value}
Copy link
Member

Choose a reason for hiding this comment

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

What is missing now is to define & document & test priority rules. With this implementation, request cookies seem to win over response cookies. But it feels like it should be the other way, i.e., last seen cookies should be the ones that get stored in the session. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added a new test, test_cookie_storage_priority in TestSessions to define and document the priority rules! Is this sufficient, or should there also be documentation in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After our call this morning, @gmelodie and I discussed the possible behaviours of cookies expiring in sessions and which cookies should take precedent.

Our understanding was regardless of the expiry behaviour, the storage priority should still be server set > command line set > session file. For that reason, we didn't write further related tests. Here's a couple of example cases illustrating why we didn't think further tests were necessary:

  • Case 1: Server expires a cookie. If a server expires a cookie, it will be removed from the session file regardless of how it was set (test_sessions.py already has test covering this)
  • Case 2: Cookies in session file expire. Cookies stored in a session file that expire will never be sent to the server and will be removed when the request is made
  • Case 3: User sets an expired cookie in the command line. We haven't been able to find a way for a user to set an expired cookie in the command line. However, if one is set, we would assume the intention would be to remove the cookie from the session. As this cookie would replace the cookie in the session file, and expired cookies are cleared from the session file when the request is sent, this behaviour is satisfied.

Copy link
Member

Choose a reason for hiding this comment

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

Our understanding was regardless of the expiry behaviour, the storage priority should still be server set > command line set > session file.

Agree 👍🏻

del request_headers[name]
continue

for prefix in SESSION_IGNORED_HEADER_PREFIXES:
Expand Down
141 changes: 115 additions & 26 deletions tests/test_sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,27 @@ def env(self):
return MockEnvironment(config_dir=self.config_dir)


class CookieTestBase:
def setup_method(self, method):
self.config_dir = mk_config_dir()

orig_session = {
'cookies': {
'cookie1': {
'value': 'foo',
},
'cookie2': {
'value': 'foo',
}
}
}
self.session_path = self.config_dir / 'test-session.json'
self.session_path.write_text(json.dumps(orig_session))

def teardown_method(self, method):
shutil.rmtree(self.config_dir)


class TestSessionFlow(SessionTestBase):
"""
These tests start with an existing session created in `setup_method()`.
Expand Down Expand Up @@ -191,13 +212,7 @@ def test_download_in_session(self, httpbin):
os.chdir(cwd)


class TestExpiredCookies:

def setup_method(self, method):
self.config_dir = mk_config_dir()

def teardown_method(self, method):
shutil.rmtree(self.config_dir)
class TestExpiredCookies(CookieTestBase):

@pytest.mark.parametrize(
argnames=['initial_cookie', 'expired_cookie'],
Expand All @@ -213,29 +228,16 @@ def test_removes_expired_cookies_from_session_obj(self, initial_cookie, expired_
assert expired_cookie not in session.cookies

def test_expired_cookies(self, httpbin):
orig_session = {
'cookies': {
'to_expire': {
'value': 'foo'
},
'to_stay': {
'value': 'foo'
},
}
}
session_path = self.config_dir / 'test-session.json'
session_path.write_text(json.dumps(orig_session))

r = http(
'--session', str(session_path),
'--session', str(self.session_path),
'--print=H',
httpbin.url + '/cookies/delete?to_expire',
httpbin.url + '/cookies/delete?cookie2',
)
assert 'Cookie: to_expire=foo; to_stay=foo' in r
assert 'Cookie: cookie1=foo; cookie2=foo' in r

updated_session = json.loads(session_path.read_text())
assert 'to_stay' in updated_session['cookies']
assert 'to_expire' not in updated_session['cookies']
updated_session = json.loads(self.session_path.read_text())
assert 'cookie1' in updated_session['cookies']
assert 'cookie2' not in updated_session['cookies']

@pytest.mark.parametrize(
argnames=['headers', 'now', 'expected_expired'],
Expand Down Expand Up @@ -277,3 +279,90 @@ def test_expired_cookies(self, httpbin):
)
def test_get_expired_cookies_manages_multiple_cookie_headers(self, headers, now, expected_expired):
assert get_expired_cookies(headers, now=now) == expected_expired


class TestCookieStorage(CookieTestBase):

@pytest.mark.parametrize(
argnames=['new_cookies', 'new_cookies_dict', 'expected'],
argvalues=[(
'new=bar',
{'new': 'bar'},
'cookie1=foo; cookie2=foo; new=bar'
),
(
'new=bar;chocolate=milk',
{'new': 'bar', 'chocolate': 'milk'},
'chocolate=milk; cookie1=foo; cookie2=foo; new=bar'
),
(
'new=bar; chocolate=milk',
{'new': 'bar', 'chocolate': 'milk'},
'chocolate=milk; cookie1=foo; cookie2=foo; new=bar'
),
(
'new=bar;; chocolate=milk;;;',
{'new': 'bar', 'chocolate': 'milk'},
'cookie1=foo; cookie2=foo; new=bar'
),
(
'new=bar; chocolate=milk;;;',
{'new': 'bar', 'chocolate': 'milk'},
'chocolate=milk; cookie1=foo; cookie2=foo; new=bar'
)
]
)
def test_existing_and_new_cookies_sent_in_request(self, new_cookies, new_cookies_dict, expected, httpbin):
r = http(
'--session', str(self.session_path),
'--print=H',
httpbin.url,
'Cookie:' + new_cookies,
)
# Note: cookies in response are in alphabetical order
assert 'Cookie: ' + expected in r

updated_session = json.loads(self.session_path.read_text())
for name, value in new_cookies_dict.items():
assert name, value in updated_session['cookies']
assert 'Cookie' not in updated_session['headers']

@pytest.mark.parametrize(
argnames=['cli_cookie', 'set_cookie', 'expected'],
argvalues=[(
'',
'/cookies/set/cookie1/bar',
'bar'
),
(
'cookie1=not_foo',
'/cookies/set/cookie1/bar',
'bar'
),
(
'cookie1=not_foo',
'',
'not_foo'
),
(
'',
'',
'foo'
)
]
)
def test_cookie_storage_priority(self, cli_cookie, set_cookie, expected, httpbin):
"""
Expected order of priority for cookie storage in session file:
1. set-cookie (from server)
2. command line arg
3. cookie already stored in session file
"""
r = http(
'--session', str(self.session_path),
httpbin.url + set_cookie,
'Cookie:' + cli_cookie,
)
updated_session = json.loads(self.session_path.read_text())

assert updated_session['cookies']['cookie1']['value'] == expected