Changes from gocardless/client-library-templates

gocardless_pro/api_client.py

@@ -158,7 +158,27 @@ def _handle_errors(self, response):
         raise exception_class(error)
 
     def _url_for(self, path):
-        return urlparse.urljoin(self.base_url, path)
+        return urlparse.urljoin(self.base_url, self._validate_path(path))
+
+    @staticmethod
+    def _validate_path(path):
+        """Check that a request path cannot move the request off the configured base URL.
+
+        ``urljoin`` resolves ``path`` against ``base_url`` like a browser resolves a link,
+        so an absolute or scheme-relative path replaces the configured origin while the
+        access token stays attached. Checked with ``urlsplit``, the same parser ``urljoin``
+        itself uses. Dot segments are left alone, since they resolve against ``base_url``
+        and can't leave its origin.
+        """
+        parsed = urlparse.urlsplit(path)
+
+        if parsed.scheme or parsed.netloc:
+            raise errors.GoCardlessProError(
+                "Invalid request path '{0}': a path may not specify a scheme or a host, only "
+                'a location relative to the configured base URL'.format(path)
+            )
+
+        return path
 
     def _headers(self, custom_headers):
         headers = self._default_headers()

gocardless_pro/services/base_service.py

@@ -5,12 +5,13 @@
 
 import re
 import time
+from urllib.parse import quote
 from requests import Timeout, ConnectionError
 from uuid import uuid4
 
 from .. import list_response
 from ..api_response import ApiResponse
-from ..errors import MalformedResponseError
+from ..errors import GoCardlessProError, MalformedResponseError
 
 class BaseService(object):
     """Base class for API service classes."""
@@ -81,4 +82,36 @@ def _resource_for(self, response):
             return list_response.ListResponse(records, api_response)
 
     def _sub_url_params(self, url, params):
-        return re.sub(r':(\w+)', lambda match: params[match.group(1)], url)
+        return re.sub(r':(\w+)',
+                      lambda match: _escape_url_param(match.group(1), params[match.group(1)]),
+                      url)
+
+
+FORBIDDEN_URL_PARAM_CHARACTERS = re.compile(r'[/?#\x00-\x1f\x7f]')
+
+
+def _escape_url_param(key, value):
+    """Escape a value before it is interpolated into a request path.
+
+    A URL parameter is a single path segment, so values that could move the request to a
+    different endpoint - path separators, control characters, '.', '..' (escaping can't make
+    these safe - a resolver strips them regardless), and empty values - are rejected instead.
+    """
+    value = str(value)
+
+    if value == '':
+        raise GoCardlessProError("No value provided for URL parameter '{}'".format(key))
+
+    if value in ('.', '..'):
+        raise GoCardlessProError(
+            "Invalid value for URL parameter '{}': '{}' would change which endpoint "
+            "the request is sent to".format(key, value)
+        )
+
+    if FORBIDDEN_URL_PARAM_CHARACTERS.search(value):
+        raise GoCardlessProError(
+            "Invalid value for URL parameter '{}': '{}' contains a character that is not "
+            "allowed in a path segment".format(key, value)
+        )
+
+    return quote(value, safe='')

tests/api_client_test.py

@@ -173,3 +173,69 @@ def test_handles_string_error_response():
 
     assert exception.value.message == 'bank_account_exists'
     assert exception.value.code == 400
+
+@responses.activate
+def test_rejects_absolute_url_as_path():
+    # An absolute URL in `path` would replace the configured base URL while the access token is
+    # still attached, handing the token to whichever host the URL names.
+    responses.add(responses.GET, 'http://elsewhere.example.com/capture', body='{}')
+
+    with pytest.raises(errors.GoCardlessProError):
+        client.get('http://elsewhere.example.com/capture')
+
+    assert len(responses.calls) == 0
+
+@responses.activate
+def test_rejects_scheme_relative_url_as_path():
+    responses.add(responses.GET, 'http://elsewhere.example.com/capture', body='{}')
+
+    with pytest.raises(errors.GoCardlessProError):
+        client.get('//elsewhere.example.com/capture')
+
+    assert len(responses.calls) == 0
+
+@responses.activate
+def test_rejects_absolute_url_hidden_behind_whitespace():
+    # `urlsplit` and `urljoin` both strip surrounding whitespace and control characters, so the
+    # check has to see the same value the join would.
+    responses.add(responses.GET, 'http://elsewhere.example.com/capture', body='{}')
+
+    with pytest.raises(errors.GoCardlessProError):
+        client.get('\thttp://elsewhere.example.com/capture')
+
+    assert len(responses.calls) == 0
+
+@responses.activate
+def test_rejects_absolute_url_for_every_verb():
+    responses.add(responses.GET, 'https://elsewhere.example.com/capture', body='{}')
+    responses.add(responses.POST, 'https://elsewhere.example.com/capture', body='{}')
+    responses.add(responses.PUT, 'https://elsewhere.example.com/capture', body='{}')
+    responses.add(responses.DELETE, 'https://elsewhere.example.com/capture', body='{}')
+
+    url = 'https://elsewhere.example.com/capture'
+    with pytest.raises(errors.GoCardlessProError):
+        client.get(url)
+    with pytest.raises(errors.GoCardlessProError):
+        client.post(url, body={})
+    with pytest.raises(errors.GoCardlessProError):
+        client.put(url, body={})
+    with pytest.raises(errors.GoCardlessProError):
+        client.delete(url, body={})
+
+    assert len(responses.calls) == 0
+
+@responses.activate
+def test_allows_query_string_in_path():
+    responses.add(responses.GET, 'http://example.com/test', body='{}')
+    client.get('/test?page=1')
+
+    assert responses.calls[0].request.url.startswith('http://example.com/test')
+
+@responses.activate
+def test_dot_segments_stay_on_the_configured_base_url():
+    # Dot segments resolve against `base_url`, so they can reach another path on the same origin
+    # but cannot leave it. They are allowed through, and this pins that behaviour.
+    responses.add(responses.GET, 'http://example.com/other', body='{}')
+    client.get('/test/../other')
+
+    assert responses.calls[0].request.url.startswith('http://example.com/')