Skip to content

Commit

Permalink
Merge pull request #78 from enote-kane/bugfix/check-state
Browse files Browse the repository at this point in the history 
OAuth client did not check the 'state' value for /authorization using…
  • Loading branch information
@rfk
rfk committed Apr 29, 2020
2 parents 1060958 + 8906e52 commit c6afe09
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 59 deletions.
19 changes: 18 additions & 1 deletion fxa/oauth.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,16 @@ def authorize_code(self, sessionOrAssertion, scope=None, client_id=None,
client_id = self.client_id
assertion = self._get_identity_assertion(sessionOrAssertion, client_id)
url = "/authorization"

# Although not relevant in this scenario from a security perspective,
# we generate a random 'state' and check the returned redirect URL
# for completeness.
state = base64.urlsafe_b64encode(os.urandom(24)).decode('utf-8')

body = {
"client_id": client_id,
"assertion": assertion,
"state": "x", # state is required, but we don't use it
"state": state
}
if scope is not None:
body["scope"] = scope
Expand All @@ -167,6 +173,17 @@ def authorize_code(self, sessionOrAssertion, scope=None, client_id=None,
# This flow is designed for web-based redirects.
# In order to get the code we must parse it from the redirect url.
query_params = parse_qs(urlparse(resp["redirect"]).query)

# Check that the 'state' parameter is present and the same we provided
if "state" not in query_params:
error_msg = "state missing in OAuth response"
raise OutOfProtocolError(error_msg)

if state != query_params["state"][0]:
error_msg = "state mismatch in OAuth response (wanted: '{}', got: '{}')".format(
state, query_params["state"][0])
raise OutOfProtocolError(error_msg)

try:
return query_params["code"][0]
except (KeyError, IndexError, ValueError):
Expand Down
33 changes: 18 additions & 15 deletions fxa/tests/test_core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ def wait_for_email(m):
# If everything went well, verify_email_code should return an empty json object
response = self.client.verify_email_code(m["headers"]["x-uid"],
m["headers"]["x-verify-code"])
self.assertEquals(response, {})
self.assertEqual(response, {})

def test_send_unblock_code(self):
acct = TestEmailAccount(email="block-{uniq}@{hostname}")
Expand All @@ -188,7 +188,7 @@ def test_send_unblock_code(self):

# Initiate sending unblock code
response = self.client.send_unblock_code(acct.email)
self.assertEquals(response, {})
self.assertEqual(response, {})

m = acct.wait_for_email(lambda m: "x-unblock-code" in m["headers"])
if not m:
Expand Down Expand Up @@ -302,16 +302,16 @@ def has_verify_code(m):

# Check that encryption keys have been preserved.
session2.fetch_keys()
self.assertEquals(self.session.keys, session2.keys)
self.assertEqual(self.session.keys, session2.keys)

def test_get_identity_assertion(self):
assertion = self.session.get_identity_assertion("http://example.com")
data = browserid.verify(assertion, audience="http://example.com")
self.assertEquals(data["status"], "okay")
self.assertEqual(data["status"], "okay")
expected_issuer = urlparse(self.session.server_url).hostname
self.assertEquals(data["issuer"], expected_issuer)
self.assertEqual(data["issuer"], expected_issuer)
expected_email = "{0}@{1}".format(self.session.uid, expected_issuer)
self.assertEquals(data["email"], expected_email)
self.assertEqual(data["email"], expected_email)

def test_get_identity_assertion_handles_duration(self):
millis = int(round(time.time() * 1000))
Expand All @@ -338,29 +338,32 @@ def test_get_identity_assertion_accepts_service(self):
assertion = self.session.get_identity_assertion("http://example.com",
service="test-me")
data = browserid.verify(assertion, audience="http://example.com")
self.assertEquals(data["status"], "okay")
self.assertEqual(data["status"], "okay")

def test_totp(self):
resp = self.session.totp_create()

# Double create causes a client error
with self.assertRaises(fxa.errors.ClientError):
self.session.totp_create()

# Created but not verified returns false (and deletes the token)
self.assertFalse(self.session.totp_exists())
# Should exist even if not verified
self.assertTrue(self.session.totp_exists())

# Creating again should work this time
# Creating again should work unless verified
resp = self.session.totp_create()

# Set session unverified to test next call
self.session.verified = False

# Verify the code
code = pyotp.TOTP(resp["secret"]).now()
self.assertTrue(self.session.totp_verify(code))
self.assertTrue(self.session.verified)

# Should exist now
# Should exist
self.assertTrue(self.session.totp_exists())

# Double create causes a client error
with self.assertRaises(fxa.errors.ClientError):
self.session.totp_create()

# Remove the code
resp = self.session.totp_delete()

Expand Down
4 changes: 2 additions & 2 deletions fxa/tests/test_crypto.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ def test_xor(self):

def test_hkdf_namespace_handle_unicode_strings(self):
kw = hkdf_namespace(text_type("foobar"))
self.assertEquals(kw, b"identity.mozilla.com/picl/v1/foobar")
self.assertEqual(kw, b"identity.mozilla.com/picl/v1/foobar")

def test_hkdf_namespace_handle_bytes_strings(self):
kw = hkdf_namespace("foobar".encode('utf-8'))
self.assertEquals(kw, b"identity.mozilla.com/picl/v1/foobar")
self.assertEqual(kw, b"identity.mozilla.com/picl/v1/foobar")
82 changes: 50 additions & 32 deletions fxa/tests/test_oauth.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,48 +272,57 @@ class TestAuthClientAuthorizeCode(unittest.TestCase):
server_url = TEST_SERVER_URL

def setUp(self):
def authorization_callback(request):
data = json.loads(_decoded(request.body))
headers = {
'Content-Type': 'application/json'
}
body = {
'redirect': 'https://relier/page?code=qed&state={}'.format(data["state"])
}
return (200, headers, json.dumps(body))

self.client = Client("abc", "xyz", server_url=self.server_url)
body = '{"redirect": "https://relier/page?code=qed&state=blah"}'
responses.add(responses.POST,
'https://server/v1/authorization',
body=body,
content_type='application/json')
responses.add_callback(responses.POST,
'https://server/v1/authorization',
callback=authorization_callback,
content_type='application/json')

@responses.activate
def test_authorize_code_with_default_arguments(self):
assertion = "A_FAKE_ASSERTION"
code = self.client.authorize_code(assertion)
self.assertEquals(code, "qed")
self.assertEqual(code, "qed")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
})

@responses.activate
def test_authorize_code_with_explicit_scope(self):
assertion = "A_FAKE_ASSERTION"
code = self.client.authorize_code(assertion, scope="profile:email")
self.assertEquals(code, "qed")
self.assertEqual(code, "qed")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
"scope": "profile:email",
})

@responses.activate
def test_authorize_code_with_explicit_client_id(self):
assertion = "A_FAKE_ASSERTION"
code = self.client.authorize_code(assertion, client_id="cba")
self.assertEquals(code, "qed")
self.assertEqual(code, "qed")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": "cba",
"state": "x",
"state": AnyStringValue(),
})

@responses.activate
Expand All @@ -325,12 +334,12 @@ def test_authorize_code_with_pkce_challenge(self):
self.assertEqual(sorted(verifier),
["code_verifier"])
code = self.client.authorize_code(assertion, **challenge)
self.assertEquals(code, "qed")
self.assertEqual(code, "qed")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
"code_challenge": challenge["code_challenge"],
"code_challenge_method": challenge["code_challenge_method"],
})
Expand All @@ -344,12 +353,12 @@ def test_authorize_code_with_session_object(self):
audience=TEST_SERVER_URL,
service=self.client.client_id
)
self.assertEquals(code, "qed")
self.assertEqual(code, "qed")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": "IDENTITY",
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
})


Expand All @@ -368,25 +377,25 @@ def setUp(self):
def test_authorize_token_with_default_arguments(self):
assertion = "A_FAKE_ASSERTION"
token = self.client.authorize_token(assertion)
self.assertEquals(token, "izatoken")
self.assertEqual(token, "izatoken")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
"response_type": "token",
})

@responses.activate
def test_authorize_token_with_explicit_scope(self):
assertion = "A_FAKE_ASSERTION"
token = self.client.authorize_token(assertion, scope="storage")
self.assertEquals(token, "izatoken")
self.assertEqual(token, "izatoken")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
"response_type": "token",
"scope": "storage",
})
Expand All @@ -395,12 +404,12 @@ def test_authorize_token_with_explicit_scope(self):
def test_authorize_token_with_explicit_client_id(self):
assertion = "A_FAKE_ASSERTION"
token = self.client.authorize_token(assertion, client_id="cba")
self.assertEquals(token, "izatoken")
self.assertEqual(token, "izatoken")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": assertion,
"client_id": "cba",
"state": "x",
"state": AnyStringValue(),
"response_type": "token",
})

Expand All @@ -413,12 +422,12 @@ def test_authorize_token_with_session_object(self):
audience=TEST_SERVER_URL,
service=self.client.client_id
)
self.assertEquals(token, "izatoken")
self.assertEqual(token, "izatoken")
req_body = json.loads(_decoded(responses.calls[0].request.body))
self.assertEquals(req_body, {
self.assertEqual(req_body, {
"assertion": "IDENTITY",
"client_id": self.client.client_id,
"state": "x",
"state": AnyStringValue(),
"response_type": "token",
})

Expand Down Expand Up @@ -591,3 +600,12 @@ def test_monkey_patch_for_gevent(self):
self.assertEqual(fxa._utils.requests, grequests)

fxa._utils.requests = old_requests


class AnyStringValue:

def __eq__(self, other):
return isinstance(other, six.string_types)

def __repr__(self):
return 'any string'
18 changes: 9 additions & 9 deletions fxa/tests/test_requests_auth_plugin.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def __init__(self, *args, **kwargs):
return_value=mocked_core_client())
def test_audience_is_parsed(self, client_patch):
self.auth(Request())
self.assertEquals(self.auth.audience, "http://www.example.com/")
self.assertEqual(self.auth.audience, "http://www.example.com/")

@mock.patch('fxa.core.Client',
return_value=mocked_core_client())
Expand Down Expand Up @@ -86,12 +86,12 @@ def test_memory_cache_is_used(self, client_patch):

# First call should set the cache value
auth(Request())
self.assertEquals(client_patch.return_value.login.return_value.
get_identity_assertion.call_count, 1)
self.assertEqual(client_patch.return_value.login.return_value.
get_identity_assertion.call_count, 1)
# Second call should use the cache value
auth(Request())
self.assertEquals(client_patch.return_value.login.return_value.
get_identity_assertion.call_count, 1)
self.assertEqual(client_patch.return_value.login.return_value.
get_identity_assertion.call_count, 1)

@mock.patch('fxa.core.Client',
return_value=mocked_core_client())
Expand Down Expand Up @@ -157,13 +157,13 @@ def test_memory_cache_is_used(self, oauth_client_patch,
core_client_patch):
# First call should set the cache value
self.auth(Request())
self.assertEquals(core_client_patch.call_count, 1)
self.assertEquals(oauth_client_patch.call_count, 1)
self.assertEqual(core_client_patch.call_count, 1)
self.assertEqual(oauth_client_patch.call_count, 1)

# Second call should use the cache value
self.auth(Request())
self.assertEquals(core_client_patch.call_count, 1)
self.assertEquals(oauth_client_patch.call_count, 1)
self.assertEqual(core_client_patch.call_count, 1)
self.assertEqual(oauth_client_patch.call_count, 1)

@mock.patch('fxa.core.Client',
return_value=mocked_core_client())
Expand Down

0 comments on commit c6afe09

Please sign in to comment.