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

@@ -51,6 +51,9 @@
 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))

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

test/units/module_utils/urls/test_fetch_url.py

@@ -9,6 +9,7 @@
 
 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 make_cookies(*args, **kwargs):
                 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):