Merge pull request from GHSA-fhpf-pp6p-55qc

scope cookies by default

Author: twm

changelog.d/339.bugfix.rst

@@ -0,0 +1 @@
+Cookies specified as a dict were sent to every domain, not just the domain of the request, potentially exposing them on redirect. See `GHSA-fhpf-pp6p-55qc <https://github.com/twisted/treq/security/advisories/GHSA-fhpf-pp6p-55qc>`_.

src/treq/client.py

@@ -3,7 +3,7 @@
 import uuid
 import warnings
 from collections.abc import Mapping
-from http.cookiejar import CookieJar
+from http.cookiejar import CookieJar, Cookie
 from urllib.parse import quote_plus, urlencode as _urlencode
 
 from twisted.internet.interfaces import IProtocol
@@ -30,7 +30,7 @@
 from treq.auth import add_auth
 from treq import multipart
 from treq.response import _Response
-from requests.cookies import cookiejar_from_dict, merge_cookies
+from requests.cookies import merge_cookies
 
 
 _NOTHING = object()
@@ -43,6 +43,56 @@ def urlencode(query, doseq):
     return s
 
 
+def _scoped_cookiejar_from_dict(url_object, cookie_dict):
+    """
+    Create a CookieJar from a dictionary whose cookies are all scoped to the
+    given URL's origin.
+
+    @note: This does not scope the cookies to any particular path, only the
+        host, port, and scheme of the given URL.
+    """
+    cookie_jar = CookieJar()
+    if cookie_dict is None:
+        return cookie_jar
+    for k, v in cookie_dict.items():
+        secure = url_object.scheme == 'https'
+        port_specified = not (
+            (url_object.scheme == "https" and url_object.port == 443)
+            or (url_object.scheme == "http" and url_object.port == 80)
+        )
+        port = str(url_object.port)
+        domain = url_object.host
+        netscape_domain = domain if '.' in domain else domain + '.local'
+
+        cookie_jar.set_cookie(
+            Cookie(
+                # Scoping
+                domain=netscape_domain,
+                port=port,
+                secure=secure,
+                port_specified=port_specified,
+
+                # Contents
+                name=k,
+                value=v,
+
+                # Constant/always-the-same stuff
+                version=0,
+                path="/",
+                expires=None,
+                discard=False,
+                comment=None,
+                comment_url=None,
+                rfc2109=False,
+                path_specified=False,
+                domain_specified=False,
+                domain_initial_dot=False,
+                rest=[],
+            )
+        )
+    return cookie_jar
+
+
 class _BodyBufferingProtocol(proxyForInterface(IProtocol)):
     def __init__(self, original, buffer, finished):
         self.original = original
@@ -98,7 +148,9 @@ class HTTPClient:
     def __init__(self, agent, cookiejar=None,
                  data_to_body_producer=IBodyProducer):
         self._agent = agent
-        self._cookiejar = cookiejar or cookiejar_from_dict({})
+        if cookiejar is None:
+            cookiejar = CookieJar()
+        self._cookiejar = cookiejar
         self._data_to_body_producer = data_to_body_producer
 
     def get(self, url, **kwargs):
@@ -195,7 +247,7 @@ def request(
             headers.setRawHeaders(b'Content-Type', [contentType])
 
         if not isinstance(cookies, CookieJar):
-            cookies = cookiejar_from_dict(cookies)
+            cookies = _scoped_cookiejar_from_dict(parsed_url, cookies)
 
         cookies = merge_cookies(self._cookiejar, cookies)
         wrapped_agent = CookieAgent(self._agent, cookies)

src/treq/test/test_testing.py

@@ -3,6 +3,7 @@
 """
 from functools import partial
 from inspect import getmembers, isfunction
+from json import dumps
 
 from unittest.mock import ANY
 
@@ -32,6 +33,26 @@ def render(self, request):
         return b"I'm a teapot"
 
 
+class _RedirectResource(Resource):
+    """
+    Resource that redirects to a different domain.
+    """
+    isLeaf = True
+
+    def render(self, request):
+        if b'redirected' not in request.uri:
+            request.redirect(b'https://example.org/redirected')
+        return dumps(
+            {
+                key.decode("charmap"): [
+                    value.decode("charmap")
+                    for value in values
+                ]
+                for key, values in
+                request.requestHeaders.getAllRawHeaders()}
+        ).encode("utf-8")
+
+
 class _NonResponsiveTestResource(Resource):
     """Resource that returns NOT_DONE_YET and never finishes the request"""
     isLeaf = True
@@ -272,8 +293,10 @@ def test_handles_successful_asynchronous_requests_with_streaming(self):
 
     def test_session_persistence_between_requests(self):
         """
-        Calling request.getSession() in the wrapped resource will return
-        a session with the same ID, until the sessions are cleaned.
+        Calling request.getSession() in the wrapped resource will return a
+        session with the same ID, until the sessions are cleaned; in other
+        words, cookies are propagated between requests when the result of
+        C{response.cookies()} is passed to the next request.
         """
         rsrc = _SessionIdTestResource()
         stub = StubTreq(rsrc)
@@ -304,6 +327,24 @@ def test_session_persistence_between_requests(self):
         sid_4 = self.successResultOf(resp.content())
         self.assertEqual(sid_3, sid_4)
 
+    def test_different_domains(self):
+        """
+        Cookies manually specified as part of a dictionary are not relayed
+        through redirects.
+
+        (This is really more of a test for scoping of cookies within treq
+        itself, rather than just for testing.)
+        """
+        rsrc = _RedirectResource()
+        stub = StubTreq(rsrc)
+        d = stub.request(
+            "GET", "http://example.com/",
+            cookies={"not-across-redirect": "nope"}
+        )
+        resp = self.successResultOf(d)
+        received = self.successResultOf(resp.json())
+        self.assertNotIn('not-across-redirect', received.get('Cookie', [''])[0])
+
 
 class HasHeadersTests(TestCase):
     """

src/treq/test/test_treq_integration.py

@@ -29,6 +29,7 @@ def print_response(response):
         print('---')
         print(response.code)
         print(response.headers)
+        print(response.request.headers)
         text = yield treq.text_content(response)
         print(text)
         print('---')