diff --git a/gocardless_pro/api_client.py b/gocardless_pro/api_client.py index 281e1ecb..c2b82504 100644 --- a/gocardless_pro/api_client.py +++ b/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() diff --git a/gocardless_pro/services/base_service.py b/gocardless_pro/services/base_service.py index 3cc96b86..2c367cf4 100644 --- a/gocardless_pro/services/base_service.py +++ b/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='') diff --git a/tests/api_client_test.py b/tests/api_client_test.py index e719da04..40e3e82d 100644 --- a/tests/api_client_test.py +++ b/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/')