From 819c6c3bc1964e4c5429e4343c619bf7daa550f1 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Tue, 30 Jun 2020 10:55:38 +0200 Subject: [PATCH 2/3] idp_oidc: check length of authorize's redirect_uri (#44589) --- src/authentic2_idp_oidc/app_settings.py | 4 ++ src/authentic2_idp_oidc/models.py | 11 +++-- src/authentic2_idp_oidc/views.py | 10 +++-- tests/test_idp_oidc.py | 58 +++++++++++++++---------- 4 files changed, 51 insertions(+), 32 deletions(-) diff --git a/src/authentic2_idp_oidc/app_settings.py b/src/authentic2_idp_oidc/app_settings.py index 8f814d2c..77d1c126 100644 --- a/src/authentic2_idp_oidc/app_settings.py +++ b/src/authentic2_idp_oidc/app_settings.py @@ -61,6 +61,10 @@ class AppSettings(object): def PASSWORD_GRANT_RATELIMIT(self): return self._setting('PASSWORD_GRANT_RATELIMIT', '100/m') + @property + def REDIRECT_URI_MAX_LENGTH(self): + return self._setting('REDIRECT_URI_MAX_LENGTH', 1024) + app_settings = AppSettings('A2_IDP_OIDC_') app_settings.__name__ = __name__ sys.modules[__name__] = app_settings diff --git a/src/authentic2_idp_oidc/models.py b/src/authentic2_idp_oidc/models.py index 0f20f77a..9eb84fcd 100644 --- a/src/authentic2_idp_oidc/models.py +++ b/src/authentic2_idp_oidc/models.py @@ -32,7 +32,7 @@ from authentic2.a2_rbac.models import OrganizationalUnit from authentic2.models import Service from authentic2.utils import to_iter -from . import utils, managers +from . import utils, managers, app_settings def generate_uuid(): @@ -192,7 +192,10 @@ class OIDCClient(Service): def get_wanted_attributes(self): return self.oidcclaim_set.filter(name__isnull=False).values_list('value', flat=True) - def is_valid_redirect_uri(self, redirect_uri): + def validate_redirect_uri(self, redirect_uri): + if len(redirect_uri) > app_settings.REDIRECT_URI_MAX_LENGTH: + raise ValueError('redirect_uri length > %s' % app_settings.REDIRECT_URI_MAX_LENGTH) + parsed_uri = urlparse.urlparse(redirect_uri) for valid_redirect_uri in self.redirect_uris.split(): parsed_valid_uri = urlparse.urlparse(valid_redirect_uri) @@ -214,8 +217,8 @@ class OIDCClient(Service): else: if parsed_uri.path.rstrip('/') != parsed_valid_uri.path.rstrip('/'): continue - return True - return False + return + raise ValueError('redirect_uri is not declared') def scope_set(self): return utils.scope_set(self.scope) diff --git a/src/authentic2_idp_oidc/views.py b/src/authentic2_idp_oidc/views.py index f142f45e..163b9841 100644 --- a/src/authentic2_idp_oidc/views.py +++ b/src/authentic2_idp_oidc/views.py @@ -141,10 +141,12 @@ def authorize(request, *args, **kwargs): error_description='authz endpoint is configured ' 'for resource owner password credential grant type') - if not client.is_valid_redirect_uri(redirect_uri): - messages.warning(request, _('Authorization request is invalid')) - logger.warning(u'idp_oidc: authorization request error, unknown redirect_uri redirect_uri=%r client_id=%r', - redirect_uri, client_id) + try: + client.validate_redirect_uri(redirect_uri) + except ValueError as e: + messages.warning(request, _('Authorization request is invalid: %s') % e) + logger.warning(u'idp_oidc: authorization request error, invalid redirect_uri %r (client_id=%r): %s', + redirect_uri, client_id, e) return redirect(request, 'auth_homepage') fragment = client.authorization_flow == client.FLOW_IMPLICIT diff --git a/tests/test_idp_oidc.py b/tests/test_idp_oidc.py index 0a6fff34..fa822856 100644 --- a/tests/test_idp_oidc.py +++ b/tests/test_idp_oidc.py @@ -1236,36 +1236,46 @@ def test_claim_templated(oidc_settings, normal_oidc_client, simple_user, app): assert user_info['family_name'].endswith(simple_user.last_name) -def test_client_is_valid_redirect_uri(): +def test_client_validate_redirect_uri(): client = OIDCClient(redirect_uris='''http://example.com http://example2.com/ http://example3.com/toto http://*example4.com/ http://example5.com/toto* ''') - assert client.is_valid_redirect_uri('http://example.com') - assert client.is_valid_redirect_uri('http://example.com/') - assert not client.is_valid_redirect_uri('http://coin.example.com/') - assert not client.is_valid_redirect_uri('http://example.com/toto/') - assert not client.is_valid_redirect_uri('http://coin.example.com') - assert client.is_valid_redirect_uri('http://example2.com') - assert client.is_valid_redirect_uri('http://example2.com/') - assert not client.is_valid_redirect_uri('http://example3.com/') - assert not client.is_valid_redirect_uri('http://example3.com') - assert client.is_valid_redirect_uri('http://example3.com/toto') - assert client.is_valid_redirect_uri('http://example3.com/toto/') - assert client.is_valid_redirect_uri('http://example4.com/') - assert client.is_valid_redirect_uri('http://example4.com') - assert client.is_valid_redirect_uri('http://coin.example4.com') - assert client.is_valid_redirect_uri('http://coin.example4.com/') - assert not client.is_valid_redirect_uri('http://coinexample4.com') - assert not client.is_valid_redirect_uri('http://coinexample4.com/') - assert client.is_valid_redirect_uri('http://example5.com/toto') - assert client.is_valid_redirect_uri('http://example5.com/toto/') - assert client.is_valid_redirect_uri('http://example5.com/toto/tata') - assert client.is_valid_redirect_uri('http://example5.com/toto/tata/') - assert not client.is_valid_redirect_uri('http://example5.com/tototata/') - assert not client.is_valid_redirect_uri('http://example5.com/tototata') + # ok + for uri in [ + 'http://example.com', + 'http://example.com/', + 'http://example2.com', + 'http://example2.com/', + 'http://example3.com/toto', + 'http://example3.com/toto/', + 'http://example4.com/', + 'http://example4.com', + 'http://coin.example4.com', + 'http://coin.example4.com/', + 'http://example5.com/toto', + 'http://example5.com/toto/', + 'http://example5.com/toto/tata', + 'http://example5.com/toto/tata/']: + client.validate_redirect_uri(uri) + # nok + for uri in [ + 'http://coin.example.com/', + 'http://example.com/toto/', + 'http://coin.example.com', + 'http://example3.com/', + 'http://example3.com', + 'http://coinexample4.com', + 'http://coinexample4.com/', + 'http://example5.com/tototata/', + 'http://example5.com/tototata']: + with pytest.raises(ValueError, match=r'is not declared'): + client.validate_redirect_uri(uri) + client.validate_redirect_uri('http://example5.com/toto/' + 'a' * 500) + with pytest.raises(ValueError, match=r'redirect_uri length >'): + client.validate_redirect_uri('http://example5.com/toto/' + 'a' * 1024) def test_filter_api_users(app, oidc_client, admin, simple_user, role_random): -- 2.26.2