diff --git a/src/sentry/auth/providers/oauth2.py b/src/sentry/auth/providers/oauth2.py index 3f11932652e1a4..42d58b277d78a5 100644 --- a/src/sentry/auth/providers/oauth2.py +++ b/src/sentry/auth/providers/oauth2.py @@ -23,6 +23,8 @@ if TYPE_CHECKING: from sentry.auth.helper import AuthHelper +logger = logging.getLogger(__name__) + ERR_INVALID_STATE = "An error occurred while validating your request." @@ -129,7 +131,16 @@ def dispatch(self, request: HttpRequest, pipeline: AuthHelper) -> HttpResponseBa code = request.GET.get("code") if error: - return pipeline.error(error) + logger.warning( + "oauth2.auth.callback.error_param", + extra={ + "provider": ( + pipeline.provider.key if getattr(pipeline, "provider", None) else None + ), + "error": error, + }, + ) + return pipeline.error(ERR_INVALID_STATE) if state != pipeline.fetch_state("state"): return pipeline.error(ERR_INVALID_STATE) @@ -143,7 +154,7 @@ def dispatch(self, request: HttpRequest, pipeline: AuthHelper) -> HttpResponseBa return pipeline.error(data["error_description"]) if "error" in data: - logging.info("Error exchanging token: %s", data["error"]) + logger.info("Error exchanging token: %s", data["error"]) return pipeline.error("Unable to retrieve your token") # we can either expect the API to be implicit and say "im looking for diff --git a/src/sentry/identity/oauth2.py b/src/sentry/identity/oauth2.py index 6c1f6f0815b97a..668b558d12b4e6 100644 --- a/src/sentry/identity/oauth2.py +++ b/src/sentry/identity/oauth2.py @@ -363,7 +363,14 @@ def dispatch(self, request: HttpRequest, pipeline: IdentityPipeline) -> HttpResp IntegrationPipelineErrorReason.TOKEN_EXCHANGE_MISMATCHED_STATE, extra={"error": error}, ) - return pipeline.error(f"{ERR_INVALID_STATE}\nError: {error}") + logger.warning( + "oauth2.callback.error_param", + extra={ + "provider": pipeline.provider.key if pipeline.provider else None, + "error": error, + }, + ) + return pipeline.error(ERR_INVALID_STATE) if state != pipeline.fetch_state("state"): extra = { diff --git a/tests/sentry/auth/providers/test_oauth2.py b/tests/sentry/auth/providers/test_oauth2.py index 9a0dedc496d0f8..c670348c571c0b 100644 --- a/tests/sentry/auth/providers/test_oauth2.py +++ b/tests/sentry/auth/providers/test_oauth2.py @@ -1,11 +1,13 @@ from collections.abc import Mapping from functools import cached_property from typing import Any +from unittest.mock import MagicMock import pytest +from django.test import RequestFactory from sentry.auth.exceptions import IdentityNotValid -from sentry.auth.providers.oauth2 import OAuth2Provider +from sentry.auth.providers.oauth2 import ERR_INVALID_STATE, OAuth2Callback, OAuth2Provider from sentry.models.authidentity import AuthIdentity from sentry.models.authprovider import AuthProvider from sentry.testutils.cases import TestCase @@ -48,3 +50,26 @@ def test_refresh_identity_without_refresh_token(self) -> None: provider = DummyOAuth2Provider() with pytest.raises(IdentityNotValid): provider.refresh_identity(auth_identity) + + +@control_silo_test +class OAuth2CallbackAuthViewTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.request_factory = RequestFactory() + self.callback = OAuth2Callback() + self.pipeline = MagicMock() + provider = MagicMock() + provider.key = "dummy" + self.pipeline.provider = provider + + def test_error_query_param_returns_generic_message(self) -> None: + request = self.request_factory.get( + "/", {"error": "10'XOR(1*if(now()=sysdate(),sleep(15),0))XOR'Z"} + ) + self.pipeline.error.return_value = object() + + response = self.callback.dispatch(request, self.pipeline) + + assert response is self.pipeline.error.return_value + self.pipeline.error.assert_called_once_with(ERR_INVALID_STATE) diff --git a/tests/sentry/identity/test_oauth2.py b/tests/sentry/identity/test_oauth2.py index 3df0c8881e6d90..49c8c56abc906b 100644 --- a/tests/sentry/identity/test_oauth2.py +++ b/tests/sentry/identity/test_oauth2.py @@ -9,7 +9,7 @@ from requests.exceptions import SSLError import sentry.identity -from sentry.identity.oauth2 import OAuth2CallbackView, OAuth2LoginView +from sentry.identity.oauth2 import ERR_INVALID_STATE, OAuth2CallbackView, OAuth2LoginView from sentry.identity.pipeline import IdentityPipeline from sentry.identity.providers.dummy import DummyProvider from sentry.integrations.types import EventLifecycleOutcome @@ -157,6 +157,23 @@ def test_api_error(self, mock_record: MagicMock) -> None: assert_failure_metric(mock_record, ApiUnauthorized('{"token": "a-fake-token"}')) + def test_error_query_param_returns_generic_message(self, mock_record: MagicMock) -> None: + request = RequestFactory().get( + "/", {"error": "10'XOR(1*if(now()=sysdate(),sleep(15),0))XOR'Z"} + ) + request.subdomain = None + + pipeline = MagicMock() + provider = MagicMock() + provider.key = "dummy" + pipeline.provider = provider + pipeline.error.return_value = object() + + response = self.view.dispatch(request, pipeline) + + assert response is pipeline.error.return_value + pipeline.error.assert_called_once_with(ERR_INVALID_STATE) + @control_silo_test class OAuth2LoginViewTest(TestCase):