From 1215c27d7e792371779c280c38b61fd0dc1774fc Mon Sep 17 00:00:00 2001 From: Jan Brauer Date: Fri, 7 Jul 2017 09:07:21 +0200 Subject: [PATCH] Redirect URIs must match exactly. (#191) * Test redirect_uri construction This was a test marked as TODO. * Remove duplicate test * Add tests to exactly match redirect URIs * Redirect URIs must match exactly. To quote from the specification at http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest: Redirection URI to which the response will be sent. This URI MUST exactly match one of the Redirection URI values for the Client pre-registered at the OpenID Provider, with the matching performed as described in Section 6.2.1 of [RFC3986] (Simple String Comparison). --- oidc_provider/lib/endpoints/authorize.py | 5 +- oidc_provider/lib/endpoints/token.py | 7 +-- oidc_provider/lib/utils/common.py | 13 ----- .../tests/test_authorize_endpoint.py | 54 ++++++++++++++++++- oidc_provider/tests/test_token_endpoint.py | 47 +++++----------- 5 files changed, 70 insertions(+), 56 deletions(-) diff --git a/oidc_provider/lib/endpoints/authorize.py b/oidc_provider/lib/endpoints/authorize.py index aae16ed..463a35c 100644 --- a/oidc_provider/lib/endpoints/authorize.py +++ b/oidc_provider/lib/endpoints/authorize.py @@ -30,7 +30,7 @@ from oidc_provider.models import ( UserConsent, ) from oidc_provider import settings -from oidc_provider.lib.utils.common import cleanup_url_from_query_string, get_browser_state_or_default +from oidc_provider.lib.utils.common import get_browser_state_or_default logger = logging.getLogger(__name__) @@ -93,8 +93,7 @@ class AuthorizeEndpoint(object): if self.is_authentication and not self.params['redirect_uri']: logger.debug('[Authorize] Missing redirect uri.') raise RedirectUriError() - clean_redirect_uri = cleanup_url_from_query_string(self.params['redirect_uri']) - if not (clean_redirect_uri in self.client.redirect_uris): + if not (self.params['redirect_uri'] in self.client.redirect_uris): logger.debug('[Authorize] Invalid redirect uri: %s', self.params['redirect_uri']) raise RedirectUriError() diff --git a/oidc_provider/lib/endpoints/token.py b/oidc_provider/lib/endpoints/token.py index ae1eb98..08597c5 100644 --- a/oidc_provider/lib/endpoints/token.py +++ b/oidc_provider/lib/endpoints/token.py @@ -3,7 +3,6 @@ import hashlib import logging import re from django.contrib.auth import authenticate -from oidc_provider.lib.utils.common import cleanup_url_from_query_string try: from urllib.parse import unquote @@ -43,8 +42,7 @@ class TokenEndpoint(object): self.params['client_id'] = client_id self.params['client_secret'] = client_secret - self.params['redirect_uri'] = unquote( - self.request.POST.get('redirect_uri', '').split('?', 1)[0]) + self.params['redirect_uri'] = self.request.POST.get('redirect_uri', '') self.params['grant_type'] = self.request.POST.get('grant_type', '') self.params['code'] = self.request.POST.get('code', '') self.params['state'] = self.request.POST.get('state', '') @@ -93,8 +91,7 @@ class TokenEndpoint(object): raise TokenError('invalid_client') if self.params['grant_type'] == 'authorization_code': - clean_redirect_uri = cleanup_url_from_query_string(self.params['redirect_uri']) - if not (clean_redirect_uri in self.client.redirect_uris): + if not (self.params['redirect_uri'] in self.client.redirect_uris): logger.debug('[Token] Invalid redirect uri: %s', self.params['redirect_uri']) raise TokenError('invalid_client') diff --git a/oidc_provider/lib/utils/common.py b/oidc_provider/lib/utils/common.py index c4778bd..09bdf00 100644 --- a/oidc_provider/lib/utils/common.py +++ b/oidc_provider/lib/utils/common.py @@ -11,19 +11,6 @@ except ImportError: from urllib.parse import urlsplit, urlunsplit -def cleanup_url_from_query_string(uri): - """ - Function used to clean up the uri from any query string, used i.e. by endpoints to validate redirect_uri - - :param uri: URI to clean from query string - :type uri: str - :return: cleaned URI without query string - """ - clean_uri = urlsplit(uri) - clean_uri = urlunsplit(clean_uri._replace(query='')) - return clean_uri - - def redirect(uri): """ Custom Response object for redirecting to a Non-HTTP url scheme. diff --git a/oidc_provider/tests/test_authorize_endpoint.py b/oidc_provider/tests/test_authorize_endpoint.py index af104f0..e03644e 100644 --- a/oidc_provider/tests/test_authorize_endpoint.py +++ b/oidc_provider/tests/test_authorize_endpoint.py @@ -1,3 +1,5 @@ +from oidc_provider.lib.errors import RedirectUriError + try: from urllib.parse import urlencode except ImportError: @@ -249,9 +251,13 @@ class AuthorizationCodeFlowTestCase(TestCase, AuthorizeEndpointMixin): self.assertEqual(is_code_ok, True, msg='Code returned is invalid or missing.') def test_response_uri_is_properly_constructed(self): + """ + Check that the redirect_uri matches the one configured for the client. + Only 'state' and 'code' should be appended. + """ data = { 'client_id': self.client.client_id, - 'redirect_uri': self.client.default_redirect_uri + "?redirect_state=xyz", + 'redirect_uri': self.client.default_redirect_uri, 'response_type': 'code', 'scope': 'openid email', 'state': self.state, @@ -260,7 +266,51 @@ class AuthorizationCodeFlowTestCase(TestCase, AuthorizeEndpointMixin): response = self._auth_request('post', data, is_user_authenticated=True) - # TODO + parsed = urlsplit(response['Location']) + params = parse_qs(parsed.query or parsed.fragment) + state = params['state'][0] + self.assertEquals(self.state, state, msg="State returned is invalid or missing") + + is_code_ok = is_code_valid(url=response['Location'], + user=self.user, + client=self.client) + self.assertTrue(is_code_ok, msg='Code returned is invalid or missing') + + self.assertEquals(set(params.keys()), set(['state', 'code']), msg='More than state or code appended as query params') + + self.assertTrue(response['Location'].startswith(self.client.default_redirect_uri), msg='Different redirect_uri returned') + + def test_unknown_redirect_uris_are_rejected(self): + """ + If a redirect_uri is not registered with the client the request must be rejected. + See http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest. + """ + data = { + 'client_id': self.client.client_id, + 'response_type': 'code', + 'redirect_uri': 'http://neverseenthis.com', + 'scope': 'openid email', + 'state': self.state, + } + + response = self._auth_request('get', data) + self.assertIn(RedirectUriError.error, response.content.decode('utf-8'), msg='No redirect_uri error') + + def test_manipulated_redirect_uris_are_rejected(self): + """ + If a redirect_uri does not exactly match the registered uri it must be rejected. + See http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest. + """ + data = { + 'client_id': self.client.client_id, + 'response_type': 'code', + 'redirect_uri': self.client.default_redirect_uri + "?some=query", + 'scope': 'openid email', + 'state': self.state, + } + + response = self._auth_request('get', data) + self.assertIn(RedirectUriError.error, response.content.decode('utf-8'), msg='No redirect_uri error') def test_public_client_auto_approval(self): """ diff --git a/oidc_provider/tests/test_token_endpoint.py b/oidc_provider/tests/test_token_endpoint.py index 46e96e4..8215558 100644 --- a/oidc_provider/tests/test_token_endpoint.py +++ b/oidc_provider/tests/test_token_endpoint.py @@ -339,12 +339,12 @@ class TokenTestCase(TestCase): response = self._post_request(post_data) self.assertIn('invalid_grant', response.content.decode('utf-8')) - def test_client_redirect_url(self): + def test_client_redirect_uri(self): """ - Validate that client redirect URIs with query strings match registered - URIs, and that unregistered URIs are rejected. - - source: https://github.com/jerrykan/django-oidc-provider/blob/2f54e537666c689dd8448f8bbc6a3a0244b01a97/oidc_provider/tests/test_token_endpoint.py + Validate that client redirect URIs exactly match registered + URIs, and that unregistered URIs or URIs with query parameters are rejected. + See http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest and + http://openid.net/specs/openid-connect-core-1_0.html#HybridTokenRequest. """ SIGKEYS = self._get_keys() code = self._create_code() @@ -354,15 +354,19 @@ class TokenTestCase(TestCase): post_data['redirect_uri'] = 'http://invalid.example.org' response = self._post_request(post_data) + self.assertIn('invalid_client', response.content.decode('utf-8')) - self.assertIn('invalid_client', response.content.decode('utf-8')), - - # Registered URI contained a query string - post_data['redirect_uri'] = 'http://example.com/?client=OidcClient' + # Registered URI, but with query string appended + post_data['redirect_uri'] = self.client.default_redirect_uri + '?foo=bar' response = self._post_request(post_data) + self.assertIn('invalid_client', response.content.decode('utf-8')) - self.assertNotIn('invalid_client', response.content.decode('utf-8')), + # Registered URI + post_data['redirect_uri'] = self.client.default_redirect_uri + + response = self._post_request(post_data) + self.assertNotIn('invalid_client', response.content.decode('utf-8')) def test_request_methods(self): """ @@ -440,29 +444,6 @@ class TokenTestCase(TestCase): False, msg='Client authentication fails using HTTP Basic Auth.') - def test_client_redirect_url(self): - """ - Validate that client redirect URIs with query strings match registered - URIs, and that unregistered URIs are rejected. - """ - SIGKEYS = self._get_keys() - code = self._create_code() - post_data = self._auth_code_post_data(code=code.code) - - # Unregistered URI - post_data['redirect_uri'] = 'http://invalid.example.org' - - response = self._post_request(post_data) - - self.assertIn('invalid_client', response.content.decode('utf-8')), - - # Registered URI contained a query string - post_data['redirect_uri'] = 'http://example.com/?client=OidcClient' - - response = self._post_request(post_data) - - self.assertNotIn('invalid_client', response.content.decode('utf-8')), - def test_access_token_contains_nonce(self): """ If present in the Authentication Request, Authorization Servers MUST