From 450cfa87760892baaab4ab4d68eed52248e28dde Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Tue, 10 Apr 2018 09:26:51 -0500 Subject: [PATCH] Handle duplicate headers in the uri module (#33792) * Handle duplicate headers, and make it easier for users to use cookies, by providing a pre-built string * Ensure proper cookie ordering, make key plural * Add note about cookie sort order * Add tests for duplicate headers and cookies_string * Extend tests, normalize headers between py2 and py3 * Add some notes in test code * Don't use AttributeError, use six.PY3. Use better names. --- lib/ansible/module_utils/urls.py | 27 ++++++++++++++++++- test/integration/targets/uri/tasks/main.yml | 11 ++++++++ .../units/module_utils/urls/test_fetch_url.py | 19 ++++++++++++- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/lib/ansible/module_utils/urls.py b/lib/ansible/module_utils/urls.py index 1470367f5d2..c3d8ae9b9fc 100644 --- a/lib/ansible/module_utils/urls.py +++ b/lib/ansible/module_utils/urls.py @@ -51,6 +51,9 @@ except ImportError: import ansible.module_utils.six.moves.http_cookiejar as cookiejar import ansible.module_utils.six.moves.urllib.request as urllib_request import ansible.module_utils.six.moves.urllib.error as urllib_error + +from ansible.module_utils.six import PY3 + from ansible.module_utils.basic import get_distribution from ansible.module_utils._text import to_bytes, to_native, to_text @@ -1049,11 +1052,33 @@ def fetch_url(module, url, data=None, headers=None, method=None, url_password=password, http_agent=http_agent, force_basic_auth=force_basic_auth, follow_redirects=follow_redirects, client_cert=client_cert, client_key=client_key, cookies=cookies) - info.update(r.info()) + # Lowercase keys, to conform to py2 behavior, so that py3 and py2 are predictable + info.update(dict((k.lower(), v) for k, v in r.info().items())) + + # Don't be lossy, append header values for duplicate headers + # In Py2 there is nothing that needs done, py2 does this for us + if PY3: + temp_headers = {} + for name, value in r.headers.items(): + # The same as above, lower case keys to match py2 behavior, and create more consistent results + name = name.lower() + if name in temp_headers: + temp_headers[name] = ', '.join((temp_headers[name], value)) + else: + temp_headers[name] = value + info.update(temp_headers) + # parse the cookies into a nice dictionary + cookie_list = [] cookie_dict = dict() + # Python sorts cookies in order of most specific (ie. longest) path first. See ``CookieJar._cookie_attrs`` + # Cookies with the same path are reversed from response order. + # This code makes no assumptions about that, and accepts the order given by python for cookie in cookies: cookie_dict[cookie.name] = cookie.value + cookie_list.append((cookie.name, cookie.value)) + info['cookies_string'] = '; '.join('%s=%s' % c for c in cookie_list) + info['cookies'] = cookie_dict # finally update the result with a message about the fetch info.update(dict(msg="OK (%s bytes)" % r.headers.get('Content-Length', 'unknown'), url=r.geturl(), status=r.code)) diff --git a/test/integration/targets/uri/tasks/main.yml b/test/integration/targets/uri/tasks/main.yml index 612a376c5d5..61357cf8f63 100644 --- a/test/integration/targets/uri/tasks/main.yml +++ b/test/integration/targets/uri/tasks/main.yml @@ -375,6 +375,17 @@ failed_when: result is not failed when: has_httptester +- uri: + url: https://{{ httpbin_host }}/response-headers?Set-Cookie=Foo%3Dbar&Set-Cookie=Baz%3Dqux + register: result + +- assert: + that: + - result['set_cookie'] == 'Foo=bar, Baz=qux' + # Python sorts cookies in order of most specific (ie. longest) path first + # items with the same path are reversed from response order + - result['cookies_string'] == 'Baz=qux; Foo=bar' + - name: Write out netrc template template: src: netrc.j2 diff --git a/test/units/module_utils/urls/test_fetch_url.py b/test/units/module_utils/urls/test_fetch_url.py index 32836284b6e..ac1fe591134 100644 --- a/test/units/module_utils/urls/test_fetch_url.py +++ b/test/units/module_utils/urls/test_fetch_url.py @@ -9,6 +9,7 @@ import socket from ansible.module_utils.six import StringIO from ansible.module_utils.six.moves.http_cookiejar import Cookie +from ansible.module_utils.six.moves.http_client import HTTPMessage from ansible.module_utils.urls import fetch_url, urllib_error, ConnectionError, NoSSLError, httplib import pytest @@ -94,6 +95,15 @@ def test_fetch_url_params(open_url_mock, fake_ansible_module): def test_fetch_url_cookies(mocker, fake_ansible_module): def make_cookies(*args, **kwargs): cookies = kwargs['cookies'] + r = MagicMock() + try: + r.headers = HTTPMessage() + add_header = r.headers.add_header + except TypeError: + # PY2 + r.headers = HTTPMessage(StringIO()) + add_header = r.headers.addheader + r.info.return_value = r.headers for name, value in (('Foo', 'bar'), ('Baz', 'qux')): cookie = Cookie( version=0, @@ -114,14 +124,21 @@ def test_fetch_url_cookies(mocker, fake_ansible_module): rest=None ) cookies.set_cookie(cookie) + add_header('Set-Cookie', '%s=%s' % (name, value)) - return MagicMock() + return r mocker = mocker.patch('ansible.module_utils.urls.open_url', new=make_cookies) r, info = fetch_url(fake_ansible_module, 'http://ansible.com/') assert info['cookies'] == {'Baz': 'qux', 'Foo': 'bar'} + # Python sorts cookies in order of most specific (ie. longest) path first + # items with the same path are reversed from response order + assert info['cookies_string'] == 'Baz=qux; Foo=bar' + # The key here has a `-` as opposed to what we see in the `uri` module that converts to `_` + # Note: this is response order, which differs from cookies_string + assert info['set-cookie'] == 'Foo=bar, Baz=qux' def test_fetch_url_nossl(open_url_mock, fake_ansible_module, mocker):