Repository: ansible/ansible
Pull request: Handle duplicate headers in the uri module
##### SUMMARY
...and make it easier for users to use cookies by providing a pre-built string

FIxes #33325 
##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
uri

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
devel
```


##### ADDITIONAL INFORMATION
<!---
Include additional information to help people understand the change here.
For bugs that don't have a linked bug report, a step-by-step reproduction
of the problem is helpful.
  -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```

```

Problem statement:
uri module doesn’t handle multiple cookies with same name
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report
 - Feature Idea
 - Documentation Report

##### COMPONENT NAME
<!--- Name of the module/plugin/task/feature -->
    module : uri
##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
Current master
```

##### CONFIGURATION
<!---
If using Ansible 2.4 or above, paste the results of "ansible-config dump --only-changed"

Otherwise, mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).

-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say "N/A" for anything that is not platform-specific.
Also mention the specific version of what you are trying to control,
e.g. if this is a network bug the version of firmware on the network device.
-->

##### SUMMARY
<!--- Explain the problem briefly -->

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: Platform login request
      uri :
        url:  "{{ platform_url }}/login"
        method: POST
        headers:
           Content-Type: "application/json"
        body: "{{ platform_login_request | to_json }}"
        status_code: 200
        validate_certs: no
        timeout: 20
      register: login_result

    - name: Get Pairing Info
      uri :
        url:  "{{ platform_url }}//nodes"
        method: GET
        headers:
           cookie: "{{ login_result.set_cookie}}"
        status_code: 200
        timeout: 20
        validate_certs: no
      
```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->

I'm getting 
set_cookie": “X-JSESSIONID=e80e63ec-defa-44d7-bd6d-b42991d8e1d8; Path=/; Secure; HttpOnly, X-JSESSIONID=deleteMe; Path=/; Max-Age=0; Expires=Sun, 26-Nov-2017 19:35:20 GMT; Secure, X-JSESSIONI=58051d1d-bf80-486c-96ff-2b8fe4dab4ce; Path=/; Secure; HttpOnly, rememberMe=deleteMe; Path=/; Max-Age=0; Expires=Sun, 26-Nov-2017 19:35:21 GMT",

If the server sends the same cookie with same name, the last one should take precedence, but URI module keeps both of them. This behavior is different from that of CURL/python request or urllib2 

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes below -->
```

```

Hints from issue discussion:
If there are two cookies with the same name, the last one should replace the first one. URI modules set cookies keep both of them. 
Please fill out the template with actionable information

needs_info
cc @romeotheriault
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
This is incorrect. From https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

> Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded

`curl` and `python` will preserve these headers in compliance with the above statement from the rfc.  They do not remove them:

```
$ curl -I http://localhost:2015/
HTTP/1.1 200 OK
Accept-Ranges: bytes
Content-Length: 0
Content-Type: text/html; charset=utf-8
Etag: "p0npiv0"
Last-Modified: Fri, 08 Dec 2017 19:16:07 GMT
Server: Caddy
Set-Cookie: foo=bar
Set-Cookie: foo=baz
Date: Fri, 08 Dec 2017 19:17:59 GMT
```

```
>>> from urllib.request import urlopen
>>> f = urlopen('http://localhost:2015')
>>> f.getheaders()
[('Accept-Ranges', 'bytes'), ('Content-Length', '0'), ('Content-Type', 'text/html; charset=utf-8'), ('Etag', '"p0npiv0"'), ('Last-Modified', 'Fri, 08 Dec 2017 19:16:07 GMT'), ('Server', 'Caddy'), ('Set-Cookie', 'foo=bar'), ('Set-Cookie', 'foo=baz'), ('Date', 'Fri, 08 Dec 2017 19:19:58 GMT'), ('Connection', 'close')]
```

If you have further questions please stop by IRC or the mailing list:

   * IRC: #ansible on irc.freenode.net
   * mailing list: https://groups.google.com/forum/#!forum/ansible-project 
I think I was not clear.  The problem is with the second request.  The client receives the response with cookies and sends  the second request with the cookies received from the  first response.

In this case, if the first response had 2 cookies with the same key,  the later one will take precedence.  The second request will set only the latest cookie header.

E.g
   Response 1 : Set-Cookie : foo=bar , foo=bar2
   Send another request using the cooke from the response1
        Curl/python requests will have only the  foo=bar2 as cookie
        ansible url module will have  both foo=bar and foo=bar2 as cookies

Ah, I see, so because Ansible is converting the cookies and headers into a dict, the operation is lossy.

This may require us to add an additional return value that maintains the list of tuples, so that we don't only include the last value.
A current work around, would be to use `login_result.cookies` instead of `login_result.set_cookie`:

```
    - name: Get Pairing Info
      uri :
        url:  "{{ platform_url }}//nodes"
        method: GET
        headers:
           cookie: "{{ result.cookies|dictsort|map('join', '=')|join('; ') }}"
        status_code: 200
        timeout: 20
        validate_certs: no
```
@haneefarkin are you using python3?

Also, please check the following PR and see if it does what you need.  Notice that it also adds a new `cookie_string` to make it easier on the user, for sending the cookies in future requests.

https://github.com/ansible/ansible/pull/33792

needs_info
@haneefarkin This issue is waiting for your response. Please respond or the issue will be closed.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: needs_info_base --->
@haneefarkin This issue is waiting for your response. Please respond or the issue will be closed.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: needs_info_base --->

Commit message: Add some notes in test code

Patch to review:
diff --git a/lib/ansible/module_utils/urls.py b/lib/ansible/module_utils/urls.py
index 1470367f5d2961..9d4c17cfefd0c3 100644
--- a/lib/ansible/module_utils/urls.py
+++ b/lib/ansible/module_utils/urls.py
@@ -1049,11 +1049,36 @@ 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 duplciate headers
+        try:
+            # Py3
+            _h = {}
+            for k, v in r.headers.items():
+                # The same as above, lower case keys to match py2 behavior, and create more consistent results
+                _k = k.lower()
+                if _k in _h:
+                    _h[_k] = ', '.join((_h[_k], v))
+                else:
+                    _h[_k] = v
+            info.update(_h)
+        except AttributeError:
+            # In Py2 there is nothing that needs done, py2 does this for us
+            pass
+
         # 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 612a376c5d5fcd..61357cf8f631e6 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 32836284b6e0b4..ac1fe591134101 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 @@
 
 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):


Code review comments:
diff_hunk: @@ -1049,11 +1049,36 @@ 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 duplciate headers
+        try:
+            # Py3
+            _h = {}
+            for k, v in r.headers.items():
+                # The same as above, lower case keys to match py2 behavior, and create more consistent results
+                _k = k.lower()
+                if _k in _h:
+                    _h[_k] = ', '.join((_h[_k], v))
+                else:
+                    _h[_k] = v
+            info.update(_h)
path: lib/ansible/module_utils/urls.py
text: @user1:
Please use more descriptive variable names for at least `_h` and `_k`, and probably `k` and `v` also. I think it's fine to use some single letter variables in list/dict comprehensions or simple loops, but once there is some additional logic I think readability is improved by using longer names.

Also, it should be fine to reassign the lowercase version of `k` (or whatever it ends up being) to itself since the original value isn't used anywhere else and the type hasn't changed.

diff_hunk: @@ -1049,11 +1049,36 @@ 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 duplciate headers
path: lib/ansible/module_utils/urls.py
text: @user1:
Fix spelling of `duplicate`.