Skip to content

Commit 9efc8dc

Browse files
geoff-vajuanifioren
geoff-va
authored and
juanifioren
committed
Fix 355 - Don't duplicate ? if qs in redirect url
Instead of appending a ? or # (implicit), parse the query string and fragment for parameters. Add our parameters and rebuild the redirect uri This should hopfully be more robust to redirect_uri's containing query parametes / fragment parameters One thing that it will not account for is if the fragment doesn't contain a parameter, but a single argument. That would be lost. But I'm not sure if you could legally combine those in a fragment? eg: https://example.com/#anchor&param=value This also adds tests for AuthorizeError.create_uri and fixes updates the test_missing_nonce test to account for this change.
1 parent c20562e commit 9efc8dc

File tree

3 files changed

+201
-15
lines changed

3 files changed

+201
-15
lines changed

‎oidc_provider/lib/errors.py‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,18 @@
11
try:
2-
from urllib.parse import quote
2+
from urllib.parse import (
3+
urlparse,
4+
urlunparse,
5+
urlencode,
6+
parse_qsl
7+
)
8+
39
except ImportError:
4-
from urllib import quote
10+
from urllib import urlencode
11+
from urlparse import (
12+
urlparse,
13+
urlunparse,
14+
parse_qsl
15+
)
516

617

718
class RedirectUriError(Exception):
@@ -105,21 +116,33 @@ def __init__(self, redirect_uri, error, grant_type):
105116
self.grant_type = grant_type
106117

107118
def create_uri(self, redirect_uri, state):
108-
description = quote(self.description)
119+
"""Return uri with error, error_description and optional state"""
109120

110121
# See:
111122
# http://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthError
112-
hash_or_question = '#' if self.grant_type == 'implicit' else '?'
113-
114-
uri = '{0}{1}error={2}&error_description={3}'.format(
115-
redirect_uri,
116-
hash_or_question,
117-
self.error,
118-
description)
119-
120-
# Add state if present.
121-
uri = uri + ('&state={0}'.format(state) if state else '')
122-
123+
ours = dict({
124+
'error': self.error, 'error_description': self.description})
125+
if state:
126+
ours['state'] = state
127+
128+
parsed = list(urlparse(redirect_uri))
129+
query_params = dict(parse_qsl(parsed[4]))
130+
# This assumes redirect_url would never contain a non-parameter type
131+
# fragment
132+
frag_params = dict(parse_qsl(parsed[5]))
133+
134+
# Update the appropriate params with ours
135+
if self.grant_type == 'implicit':
136+
frag_params.update(ours)
137+
else:
138+
query_params.update(ours)
139+
140+
# Rebuild the uri with updated query and fragments
141+
query_params = urlencode(query_params)
142+
frag_params = urlencode(frag_params)
143+
parsed[4] = query_params
144+
parsed[5] = frag_params
145+
uri = urlunparse(parsed)
123146
return uri
124147

125148

‎oidc_provider/tests/cases/test_authorize_endpoint.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,8 @@ def test_missing_nonce(self):
532532

533533
response = self._auth_request('get', data, is_user_authenticated=True)
534534

535-
self.assertIn('#error=invalid_request', response['Location'])
535+
self.assertIn('#', response['Location'])
536+
self.assertIn('error=invalid_request', response['Location'])
536537

537538
def test_idtoken_token_response(self):
538539
"""

‎oidc_provider/tests/cases/test_error.py‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
from django.test import TestCase
2+
from oidc_provider.lib.errors import AuthorizeError
3+
4+
try:
5+
from urllib.parse import (
6+
urlparse,
7+
parse_qsl
8+
)
9+
except ImportError:
10+
from urlparse import (
11+
urlparse,
12+
parse_qsl
13+
)
14+
15+
16+
def compare(expected, created):
17+
"""Compare expected and created urls"""
18+
ex_parsed = list(urlparse(expected))
19+
ex_qp = dict(parse_qsl(ex_parsed[4]))
20+
ex_frag = dict(parse_qsl(ex_parsed[5]))
21+
22+
cr_parsed = list(urlparse(created))
23+
cr_qp = dict(parse_qsl(cr_parsed[4]))
24+
cr_frag = dict(parse_qsl(cr_parsed[5]))
25+
26+
# Validate scheme, netloc, path match
27+
assert ex_parsed[:3] == cr_parsed[:3]
28+
# Validate qp and frags match
29+
assert ex_qp == cr_qp
30+
assert ex_frag == cr_frag
31+
32+
33+
class TestImplicitAuthorizeErrorNonImplicit(TestCase):
34+
"""Tests with grant_type code - all responses in query params"""
35+
redirect_uri = 'https://example.com/'
36+
grant_type = 'code'
37+
error = 'login_required'
38+
desc = 'The+Authorization+Server+requires+End-User+authentication'
39+
40+
def test_no_params(self):
41+
"""Test with a path only and no query/frag params"""
42+
redirect_uri = self.redirect_uri + 'path'
43+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
44+
created_uri = error.create_uri(redirect_uri, '')
45+
expected_uri = '{}?error={}&error_description={}'.format(
46+
redirect_uri, self.error, self.desc)
47+
compare(expected_uri, created_uri)
48+
49+
def test_query_params_only(self):
50+
"""Test with query param in redirect uri"""
51+
redirect_uri = self.redirect_uri + "path/?action=something"
52+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
53+
created_uri = error.create_uri(redirect_uri, '')
54+
expected_uri = '{}&error={}&error_description={}'.format(
55+
redirect_uri, self.error, self.desc)
56+
compare(expected_uri, created_uri)
57+
58+
def test_frag_params_only(self):
59+
"""Test with fragment params only"""
60+
redirect_uri = self.redirect_uri + 'path'
61+
frag = '#action=something'
62+
error = AuthorizeError(redirect_uri + frag, self.error, self.grant_type)
63+
created_uri = error.create_uri(redirect_uri + frag, '')
64+
expected_uri = '{}path?error={}&error_description={}{}'.format(
65+
self.redirect_uri, self.error, self.desc, frag)
66+
compare(expected_uri, created_uri)
67+
68+
def test_query_and_frag_params(self):
69+
"""Test with both qp's and fragment"""
70+
redirect_uri = self.redirect_uri + 'path?my_qp=test'
71+
frag = '#action=something'
72+
error = AuthorizeError(redirect_uri + frag, self.error, self.grant_type)
73+
created_uri = error.create_uri(redirect_uri + frag, '')
74+
expected_uri = '{}path?my_qp=test&error={}&error_description={}{}' \
75+
.format(self.redirect_uri, self.error, self.desc, frag)
76+
compare(expected_uri, created_uri)
77+
78+
def test_with_state(self):
79+
"""Test with state"""
80+
redirect_uri = self.redirect_uri + 'path'
81+
state = 'my_state'
82+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
83+
created_uri = error.create_uri(redirect_uri, state)
84+
expected_uri = '{}path?error={}&error_description={}&state={}' \
85+
.format(self.redirect_uri, self.error, self.desc, state)
86+
compare(expected_uri, created_uri)
87+
88+
def test_with_deep_link(self):
89+
"""Test with a non-http schema; deep link style (think slack://)"""
90+
redirect_uri = 'slack://example.com/path'
91+
state = 'my_state'
92+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
93+
created_uri = error.create_uri(redirect_uri, state)
94+
expected_uri = '{}?error={}&error_description={}&state={}' \
95+
.format(redirect_uri, self.error, self.desc, state)
96+
compare(expected_uri, created_uri)
97+
98+
99+
class TestImplicitAuthorizeErrorImplicit(TestCase):
100+
"""Tests with grant_type code - all responses in query params"""
101+
redirect_uri = 'https://example.com/'
102+
grant_type = 'implicit'
103+
error = 'login_required'
104+
desc = 'The+Authorization+Server+requires+End-User+authentication'
105+
106+
def test_no_params(self):
107+
"""Test with a path only and no query/frag params"""
108+
redirect_uri = self.redirect_uri + 'path'
109+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
110+
created_uri = error.create_uri(redirect_uri, '')
111+
expected_uri = '{}#error={}&error_description={}'.format(
112+
redirect_uri, self.error, self.desc)
113+
compare(expected_uri, created_uri)
114+
115+
def test_query_params_only(self):
116+
"""Test with query param in redirect uri"""
117+
redirect_uri = self.redirect_uri + "path/?action=something"
118+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
119+
created_uri = error.create_uri(redirect_uri, '')
120+
expected_uri = '{}#error={}&error_description={}'.format(
121+
redirect_uri, self.error, self.desc)
122+
compare(expected_uri, created_uri)
123+
124+
def test_frag_params_only(self):
125+
"""Test with fragment params only"""
126+
redirect_uri = self.redirect_uri + 'path'
127+
frag = '#action=something'
128+
error = AuthorizeError(redirect_uri + frag, self.error, self.grant_type)
129+
created_uri = error.create_uri(redirect_uri + frag, '')
130+
expected_uri = '{}path{}&error={}&error_description={}'.format(
131+
self.redirect_uri, frag, self.error, self.desc)
132+
compare(expected_uri, created_uri)
133+
134+
def test_query_and_frag_params(self):
135+
"""Test with both qp's and fragment"""
136+
redirect_uri = self.redirect_uri + 'path?my_qp=test'
137+
frag = '#action=something'
138+
error = AuthorizeError(redirect_uri + frag, self.error, self.grant_type)
139+
created_uri = error.create_uri(redirect_uri + frag, '')
140+
expected_uri = '{}path?my_qp=test{}&error={}&error_description={}' \
141+
.format(self.redirect_uri, frag, self.error, self.desc)
142+
compare(expected_uri, created_uri)
143+
144+
def test_with_state(self):
145+
"""Test with state"""
146+
redirect_uri = self.redirect_uri + 'path'
147+
state = 'my_state'
148+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
149+
created_uri = error.create_uri(redirect_uri, state)
150+
expected_uri = '{}path#error={}&error_description={}&state={}' \
151+
.format(self.redirect_uri, self.error, self.desc, state)
152+
compare(expected_uri, created_uri)
153+
154+
def test_with_deep_link(self):
155+
"""Test with a non-http schema; deep link style (think slack://)"""
156+
redirect_uri = 'slack://example.com/path'
157+
state = 'my_state'
158+
error = AuthorizeError(redirect_uri, self.error, self.grant_type)
159+
created_uri = error.create_uri(redirect_uri, state)
160+
expected_uri = '{}#error={}&error_description={}&state={}' \
161+
.format(redirect_uri, self.error, self.desc, state)
162+
compare(expected_uri, created_uri)

0 commit comments

Comments
 (0)