Merge pull request #722 from codecov/dorian/CODE-628/state-in-redis

implement redirection to the provided url after authentication

Author: dorian-writes-code

Diff for: codecov/settings_base.py

@@ -185,6 +185,7 @@
 ARCHIVE_BUCKET_NAME = "codecov"
 ENCRYPTION_SECRET = get_config("setup", "encryption_secret")
 
+COOKIE_SAME_SITE = "Lax"
 COOKIE_SECRET = get_config("setup", "http", "cookie_secret")
 COOKIES_DOMAIN = get_config("setup", "http", "cookies_domain", default=".codecov.io")
 SESSION_COOKIE_DOMAIN = get_config(
@@ -228,5 +229,10 @@
 
 IS_ENTERPRISE = get_settings_module() == SettingsModule.ENTERPRISE.value
 IS_DEV = get_settings_module() == SettingsModule.DEV.value
+
 DATA_UPLOAD_MAX_MEMORY_SIZE = get_config("setup", "http", "upload_max_memory_size", default=2621440)
 FILE_UPLOAD_MAX_MEMORY_SIZE = get_config("setup", "http", "file_upload_max_memory_size", default=2621440)
+
+
+CORS_ALLOWED_ORIGIN_REGEXES = []
+CORS_ALLOWED_ORIGINS = []

Diff for: codecov/settings_dev.py

@@ -36,5 +36,11 @@
 
 CODECOV_DASHBOARD_URL = "http://localhost:3000"
 
+CORS_ALLOWED_ORIGINS = [
+    CODECOV_DASHBOARD_URL,
+    "http://localhost",
+    "http://localhost:9000",
+]
+
 COOKIES_DOMAIN = "localhost"
 SESSION_COOKIE_DOMAIN = "localhost"

Diff for: codecov/settings_prod.py

@@ -41,6 +41,12 @@
     CODECOV_DASHBOARD_URL,
     "https://gazebo.netlify.app",  # to access unreleased URL of gazebo
 ]
+# We are also using the CORS settings to verify if the domain is safe to
+# Redirect after authentication, update this setting with care
+CORS_ALLOWED_ORIGIN_REGEXES = []
 
 DATA_UPLOAD_MAX_MEMORY_SIZE = 15000000
-SILENCED_SYSTEM_CHECKS = ['urls.W002']
+SILENCED_SYSTEM_CHECKS = ["urls.W002"]
+
+# Reinforcing the Cookie SameSite configuration to be sure it's Lax in prod
+COOKIE_SAME_SITE = "Lax"

Diff for: codecov/settings_staging.py

@@ -56,3 +56,7 @@
 ]
 
 DATA_UPLOAD_MAX_MEMORY_SIZE = 15000000
+
+# Same site is set to none on Staging as we want to be able to call the API
+# From Netlify preview deploy
+COOKIE_SAME_SITE = "None"

Diff for: codecov_auth/admin.py

@@ -21,7 +21,7 @@ def impersonate_owner(self, request, queryset):
         "staff_user",
         owner.username,
         domain=settings.COOKIES_DOMAIN,
-        samesite="Lax",
+        samesite=settings.COOKIE_SAME_SITE,
     )
     return response
 

Diff for: codecov_auth/tests/unit/views/test_base.py

@@ -1,9 +1,93 @@
-from django.test import TestCase, RequestFactory
+from django.test import TestCase, RequestFactory, override_settings
+from django.core.exceptions import SuspiciousOperation
+import pytest
 from unittest.mock import patch
-from codecov_auth.views.base import LoginMixin
+
+from codecov_auth.views.base import LoginMixin, StateMixin
 from codecov_auth.tests.factories import OwnerFactory
 
 
+def set_up_mixin(to=None):
+    query_string = {"to": to} if to else None
+    mixin = StateMixin()
+    mixin.request = RequestFactory().get("", query_string)
+    mixin.service = "github"
+    return mixin
+
+
+def test_generate_state_without_redirection_url(mock_redis):
+    mixin = set_up_mixin()
+    state = mixin.generate_state()
+    assert (
+        mock_redis.get(f"oauth-state-{state}").decode("utf-8")
+        == "http://localhost:3000/gh"
+    )
+
+
+def test_generate_state_with_path_redirection_url(mock_redis):
+    mixin = set_up_mixin("/gh/codecov")
+    state = mixin.generate_state()
+    assert mock_redis.get(f"oauth-state-{state}").decode("utf-8") == "/gh/codecov"
+
+
+@override_settings(CORS_ALLOWED_ORIGINS=["https://app.codecov.io"])
+def test_generate_state_with_safe_domain_redirection_url(mock_redis):
+    mixin = set_up_mixin("https://app.codecov.io/gh/codecov")
+    state = mixin.generate_state()
+    assert (
+        mock_redis.get(f"oauth-state-{state}").decode("utf-8")
+        == "https://app.codecov.io/gh/codecov"
+    )
+
+
+@override_settings(CORS_ALLOWED_ORIGINS=[])
+@override_settings(CORS_ALLOWED_ORIGIN_REGEXES=[r"^(https:\/\/)?(.+)\.codecov\.io$"])
+def test_generate_state_with_safe_domain_regex_redirection_url(mock_redis):
+    mixin = set_up_mixin("https://app.codecov.io/gh/codecov")
+    state = mixin.generate_state()
+    assert (
+        mock_redis.get(f"oauth-state-{state}").decode("utf-8")
+        == "https://app.codecov.io/gh/codecov"
+    )
+
+
+@override_settings(CORS_ALLOWED_ORIGINS=[])
+@override_settings(CORS_ALLOWED_ORIGIN_REGEXES=[])
+def test_generate_state_with_unsafe_domain(mock_redis):
+    mixin = set_up_mixin("http://hacker.com/i-steal-cookie")
+    state = mixin.generate_state()
+    assert mock_redis.keys("*") != []
+    assert (
+        mock_redis.get(f"oauth-state-{state}").decode("utf-8")
+        == "http://localhost:3000/gh"
+    )
+
+
+@override_settings(CORS_ALLOWED_ORIGINS=[])
+@override_settings(CORS_ALLOWED_ORIGIN_REGEXES=[])
+def test_generate_state_when_wrong_url(mock_redis):
+    mixin = set_up_mixin("http://localhost:]/")
+    state = mixin.generate_state()
+    assert mock_redis.keys("*") != []
+    assert (
+        mock_redis.get(f"oauth-state-{state}").decode("utf-8")
+        == "http://localhost:3000/gh"
+    )
+
+
+def test_get_redirection_url_from_state_no_state(mock_redis):
+    mixin = set_up_mixin()
+    with pytest.raises(SuspiciousOperation):
+        mixin.get_redirection_url_from_state("not exist")
+
+
+def test_get_redirection_url_from_state_give_url(mock_redis):
+    mixin = set_up_mixin()
+    mock_redis.set(f"oauth-state-abc", "http://localhost/gh/codecov")
+    assert mixin.get_redirection_url_from_state("abc") == "http://localhost/gh/codecov"
+    assert mock_redis.get(f"oauth-state-abc") is None
+
+
 class LoginMixinTests(TestCase):
     def setUp(self):
         self.mixin_instance = LoginMixin()

Diff for: codecov_auth/tests/unit/views/test_github.py

@@ -11,40 +11,48 @@
 from codecov_auth.models import Owner
 
 
-def test_get_github_redirect(client):
+def _get_state_from_redis(mock_redis):
+    key_redis = mock_redis.keys("*")[0].decode()
+    return key_redis.replace("oauth-state-", "")
+
+
+def test_get_github_redirect(client, mock_redis):
     url = reverse("github-login")
     res = client.get(url)
+    state = _get_state_from_redis(mock_redis)
     assert res.status_code == 302
     assert (
         res.url
-        == "https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook&client_id=3d44be0e772666136a13"
+        == f"https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook&client_id=3d44be0e772666136a13&state={state}"
     )
 
 
-def test_get_github_redirect_with_ghpr_cookie(client, settings):
+def test_get_github_redirect_with_ghpr_cookie(client, mock_redis, settings):
     settings.COOKIES_DOMAIN = ".simple.site"
     client.cookies = SimpleCookie({"ghpr": "true"})
     url = reverse("github-login")
     res = client.get(url)
+    state = _get_state_from_redis(mock_redis)
     assert res.status_code == 302
     assert (
         res.url
-        == "https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook%2Crepo&client_id=3d44be0e772666136a13"
+        == f"https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook%2Crepo&client_id=3d44be0e772666136a13&state={state}"
     )
     assert "ghpr" in res.cookies
     ghpr_cooke = res.cookies["ghpr"]
     assert ghpr_cooke.value == "true"
     assert ghpr_cooke.get("domain") == ".simple.site"
 
 
-def test_get_github_redirect_with_private_url(client, settings):
+def test_get_github_redirect_with_private_url(client, mock_redis, settings):
     settings.COOKIES_DOMAIN = ".simple.site"
     url = reverse("github-login")
     res = client.get(url, {"private": "true"})
+    state = _get_state_from_redis(mock_redis)
     assert res.status_code == 302
     assert (
         res.url
-        == "https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook%2Crepo&client_id=3d44be0e772666136a13"
+        == f"https://github.com/login/oauth/authorize?response_type=code&scope=user%3Aemail%2Cread%3Aorg%2Crepo%3Astatus%2Cwrite%3Arepo_hook%2Crepo&client_id=3d44be0e772666136a13&state={state}"
     )
     assert "ghpr" in res.cookies
     ghpr_cooke = res.cookies["ghpr"]
@@ -118,8 +126,10 @@ async def is_student(*args, **kwargs):
             as_tuple=mocker.MagicMock(return_value=("a", "b"))
         ),
     )
+
     url = reverse("github-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gh")
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "github-token" in res.cookies
     assert "github-username" in res.cookies
@@ -181,15 +191,25 @@ def test_get_github_already_with_code_github_error(
     async def helper_func(*args, **kwargs):
         raise TorngitClientGeneralError(403, "response", "message")
 
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gh")
+
     mocker.patch.object(Github, "get_authenticated_user", side_effect=helper_func)
     url = reverse("github-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "github-token" not in res.cookies
     assert "github-username" not in res.cookies
     assert res.url == "/"
 
 
+def test_state_not_known(client, mocker, db, mock_redis, settings):
+    url = reverse("github-login")
+    res = client.get(url, {"code": "aaaaaaa", "state": "doesnt exist"})
+    assert res.status_code == 400
+    assert "github-token" not in res.cookies
+    assert "github-username" not in res.cookies
+
+
 def test_get_github_already_with_code_with_email(
     client, mocker, db, mock_redis, settings
 ):
@@ -226,8 +246,9 @@ async def is_student(*args, **kwargs):
             as_tuple=mocker.MagicMock(return_value=("a", "b"))
         ),
     )
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gh")
     url = reverse("github-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "github-token" in res.cookies
     assert "github-username" in res.cookies
@@ -281,8 +302,9 @@ async def is_student(*args, **kwargs):
             as_tuple=mocker.MagicMock(return_value=("a", "b"))
         ),
     )
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gh")
     url = reverse("github-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "github-token" in res.cookies
     assert "github-username" in res.cookies
@@ -345,7 +367,8 @@ async def is_student(*args, **kwargs):
         ),
     )
     url = reverse("github-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gh")
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "github-token" in res.cookies
     assert "github-username" in res.cookies

Diff for: codecov_auth/tests/unit/views/test_gitlab.py

@@ -9,7 +9,12 @@
 from codecov_auth.models import Session
 
 
-def test_get_gitlab_redirect(client, settings, mocker):
+def _get_state_from_redis(mock_redis):
+    key_redis = mock_redis.keys("*")[0].decode()
+    return key_redis.replace("oauth-state-", "")
+
+
+def test_get_gitlab_redirect(client, settings, mock_redis, mocker):
     mocker.patch(
         "codecov_auth.views.gitlab.uuid4",
         return_value=UUID("fbdf86c6c8d64ed1b814e80b33df85c9"),
@@ -23,10 +28,11 @@ def test_get_gitlab_redirect(client, settings, mocker):
     settings.GITLAB_REDIRECT_URI = "http://localhost/login/gitlab"
     url = reverse("gitlab-login")
     res = client.get(url, SERVER_NAME="localhost:8000")
+    state = _get_state_from_redis(mock_redis)
     assert res.status_code == 302
     assert (
         res.url
-        == "https://gitlab.com/oauth/authorize?response_type=code&client_id=testfiuozujcfo5kxgigugr5x3xxx2ukgyandp16x6w566uits7f32crzl4yvmth&redirect_uri=http%3A%2F%2Flocalhost%2Flogin%2Fgitlab&state=fbdf86c6c8d64ed1b814e80b33df85c9"
+        == f"https://gitlab.com/oauth/authorize?response_type=code&client_id=testfiuozujcfo5kxgigugr5x3xxx2ukgyandp16x6w566uits7f32crzl4yvmth&redirect_uri=http%3A%2F%2Flocalhost%2Flogin%2Fgitlab&state={state}"
     )
 
 
@@ -70,7 +76,8 @@ async def helper_list_teams_func(*args, **kwargs):
         ),
     )
     url = reverse("gitlab-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gl")
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "gitlab-token" in res.cookies
     assert "gitlab-username" in res.cookies
@@ -97,7 +104,8 @@ async def helper_func(*args, **kwargs):
 
     mocker.patch.object(Gitlab, "get_authenticated_user", side_effect=helper_func)
     url = reverse("gitlab-login")
-    res = client.get(url, {"code": "aaaaaaa"})
+    mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gl")
+    res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
     assert res.status_code == 302
     assert "gitlab-token" not in res.cookies
     assert "gitlab-username" not in res.cookies