You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/09/29 17:31:32 UTC
(superset) branch master updated: fix(api): return 4xx from oauth2 callback errors, not 500 (#44769)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new f639b22c6c8 fix(api): return 4xx from oauth2 callback errors, not 500 (#44769)
f639b22c6c8 is described below
commit f639b22c6c869dde21c1fb22c416085745202341
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed Sep 30 03:31:25 2026 +1000
fix(api): return 4xx from oauth2 callback errors, not 500 (#44769)
---
docs/developer_docs/api.mdx | 5 ++
superset/commands/database/oauth2.py | 27 ++++++--
superset/exceptions.py | 23 +++++++
tests/unit_tests/commands/databases/oauth2_test.py | 73 ++++++++++++++++++++--
tests/unit_tests/databases/api_test.py | 8 +--
tests/unit_tests/databases/oauth2_api_test.py | 32 +++++++++-
6 files changed, 151 insertions(+), 17 deletions(-)
diff --git a/docs/developer_docs/api.mdx b/docs/developer_docs/api.mdx
index a9d5f4dbb37..0ccc26deab8 100644
--- a/docs/developer_docs/api.mdx
+++ b/docs/developer_docs/api.mdx
@@ -195,6 +195,11 @@ update preserves the stored secrets; send new values to replace them.
</details>
+The database OAuth2 callback at `/api/v1/database/oauth2/` returns HTTP 400
+when authorization is denied or callback state is missing or invalid. Provider
+error text is not echoed in the response; authorization denials use the message
+"The OAuth2 provider denied the request". Restart the OAuth2 flow to retry.
+
#### Data Exploration
<details>
diff --git a/superset/commands/database/oauth2.py b/superset/commands/database/oauth2.py
index 1d09f5cc76f..dc019226269 100644
--- a/superset/commands/database/oauth2.py
+++ b/superset/commands/database/oauth2.py
@@ -21,13 +21,16 @@ from functools import partial
from typing import cast
from uuid import UUID
+import jwt
+from marshmallow import ValidationError
+
from superset import db, security_manager
from superset.commands.base import BaseCommand
from superset.commands.database.exceptions import DatabaseNotFoundError
from superset.daos.database import DatabaseUserOAuth2TokensDAO
from superset.daos.key_value import KeyValueDAO
from superset.databases.schemas import OAuth2ProviderResponseSchema
-from superset.exceptions import OAuth2Error
+from superset.exceptions import OAuth2Error, OAuth2RejectedError
from superset.key_value.types import JsonKeyValueCodec, KeyValueResource
from superset.models.core import Database, DatabaseUserOAuth2Tokens
from superset.superset_typing import OAuth2State
@@ -117,18 +120,30 @@ class OAuth2StoreTokenCommand(BaseCommand):
)
def validate(self) -> None:
- if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ if self._parameters.get("error"):
+ raise OAuth2RejectedError("The OAuth2 provider denied the request")
- self._state = decode_oauth2_state(self._parameters["state"])
+ try:
+ state = self._parameters["state"]
+ except KeyError:
+ raise OAuth2RejectedError(
+ "The OAuth2 callback is missing the state parameter"
+ ) from None
+
+ try:
+ self._state = decode_oauth2_state(state)
+ except (jwt.PyJWTError, ValidationError):
+ raise OAuth2RejectedError("The OAuth2 state parameter is invalid") from None
# Bind the callback to the current session: require an authenticated,
# non-guest user whose id matches the one carried in the state.
user_id = get_user_id()
if user_id is None or security_manager.is_guest_user():
- raise OAuth2Error("The OAuth2 callback requires an authenticated user")
+ raise OAuth2RejectedError(
+ "The OAuth2 callback requires an authenticated user"
+ )
if user_id != self._state["user_id"]:
- raise OAuth2Error("The OAuth2 state belongs to a different user")
+ raise OAuth2RejectedError("The OAuth2 state belongs to a different user")
if database := DatabaseUserOAuth2TokensDAO.get_database(
self._state["database_id"]
diff --git a/superset/exceptions.py b/superset/exceptions.py
index 72ba6c63c9a..86d950da3a7 100644
--- a/superset/exceptions.py
+++ b/superset/exceptions.py
@@ -439,6 +439,29 @@ class OAuth2Error(SupersetErrorException):
)
+class OAuth2RejectedError(SupersetErrorException):
+ """
+ Exception for when the OAuth2 callback itself can't be completed.
+
+ Raised when the provider denies authorization or the callback request
+ can't be trusted (missing/invalid state, no authenticated session).
+ These are conditions the caller could correct by retrying the OAuth2
+ flow, so unlike ``OAuth2Error`` this maps to a 4xx response instead of
+ a 500.
+ """
+
+ status = 400
+
+ def __init__(self, error: str):
+ super().__init__(
+ SupersetError(
+ message=error,
+ error_type=SupersetErrorType.OAUTH2_REDIRECT_ERROR,
+ level=ErrorLevel.WARNING,
+ )
+ )
+
+
class SupersetDisallowedSQLFunctionException(SupersetErrorException):
"""
Disallowed function found on SQL statement
diff --git a/tests/unit_tests/commands/databases/oauth2_test.py b/tests/unit_tests/commands/databases/oauth2_test.py
index 6958b90d82b..d389bc59a85 100644
--- a/tests/unit_tests/commands/databases/oauth2_test.py
+++ b/tests/unit_tests/commands/databases/oauth2_test.py
@@ -20,7 +20,9 @@ import traceback
from typing import Any
from unittest.mock import MagicMock
+import jwt
import pytest
+from flask import current_app
from pytest_mock import MockerFixture
from requests.exceptions import HTTPError
@@ -28,7 +30,7 @@ from superset.commands.database.exceptions import DatabaseNotFoundError
from superset.commands.database.oauth2 import OAuth2StoreTokenCommand
from superset.daos.database import DatabaseUserOAuth2TokensDAO
from superset.databases.schemas import OAuth2ProviderResponseSchema
-from superset.exceptions import OAuth2Error
+from superset.exceptions import OAuth2Error, OAuth2RejectedError
from superset.models.core import Database
from superset.utils.oauth2 import decode_oauth2_state, encode_oauth2_state
@@ -104,11 +106,68 @@ def test_validate_database_not_found(
command.validate()
-def test_validate_oauth2_error(mock_parameters: OAuth2ProviderResponseSchema) -> None:
- mock_parameters["error"] = "OAuth2 failure"
+@pytest.mark.parametrize("error", ["access_denied", "provider-sentinel\r\nFORGED LOG"])
+def test_validate_oauth2_error(
+ mock_parameters: OAuth2ProviderResponseSchema,
+ error: str,
+) -> None:
+ """Reject provider errors without reflecting their text in responses or logs."""
+ mock_parameters["error"] = error
command = OAuth2StoreTokenCommand(mock_parameters)
- with pytest.raises(OAuth2Error, match="Something went wrong while doing OAuth2"):
+ with pytest.raises(OAuth2RejectedError) as exc_info:
command.validate()
+ assert exc_info.value.status == 400
+ assert exc_info.value.to_dict()["message"] == (
+ "The OAuth2 provider denied the request"
+ )
+ assert error not in str(exc_info.value.to_dict())
+ assert error not in "".join(traceback.format_exception(exc_info.value))
+
+
+def test_validate_missing_state(
+ mock_parameters: OAuth2ProviderResponseSchema,
+) -> None:
+ """Reject callbacks missing state with HTTP 400."""
+ del mock_parameters["state"]
+ command = OAuth2StoreTokenCommand(mock_parameters)
+ with pytest.raises(OAuth2RejectedError) as exc_info:
+ command.validate()
+ assert exc_info.value.status == 400
+
+
+def test_validate_invalid_state(
+ mock_parameters: OAuth2ProviderResponseSchema,
+) -> None:
+ """Reject callbacks with an invalid JWT with HTTP 400."""
+ mock_parameters["state"] = "not-a-valid-jwt"
+ command = OAuth2StoreTokenCommand(mock_parameters)
+ with pytest.raises(OAuth2RejectedError) as exc_info:
+ command.validate()
+ assert exc_info.value.status == 400
+
+
+@pytest.mark.parametrize("database_id", [None, "not-an-integer"])
+def test_validate_invalid_state_payload(
+ mock_parameters: OAuth2ProviderResponseSchema,
+ database_id: str | None,
+) -> None:
+ """Reject signed state with missing or invalid required fields with HTTP 400."""
+ payload = dict(decode_oauth2_state(mock_parameters["state"]))
+ if database_id is None:
+ del payload["database_id"]
+ else:
+ payload["database_id"] = database_id
+ mock_parameters["state"] = jwt.encode(
+ payload,
+ current_app.config["SECRET_KEY"],
+ algorithm=current_app.config["DATABASE_OAUTH2_JWT_ALGORITHM"],
+ )
+ with pytest.raises(OAuth2RejectedError) as exc_info:
+ OAuth2StoreTokenCommand(mock_parameters).validate()
+ assert exc_info.value.status == 400
+ assert exc_info.value.to_dict()["message"] == (
+ "The OAuth2 state parameter is invalid"
+ )
def test_run_success(
@@ -227,9 +286,11 @@ def test_validate_rejects_state_not_bound_to_session(
command = OAuth2StoreTokenCommand(mock_parameters)
mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=2)
- with pytest.raises(OAuth2Error):
+ with pytest.raises(OAuth2RejectedError) as exc_info:
command.validate()
+ assert exc_info.value.status == 400
mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=None)
- with pytest.raises(OAuth2Error):
+ with pytest.raises(OAuth2RejectedError) as exc_info:
command.validate()
+ assert exc_info.value.status == 400
diff --git a/tests/unit_tests/databases/api_test.py b/tests/unit_tests/databases/api_test.py
index 33e08e54ac9..1638cc62e37 100644
--- a/tests/unit_tests/databases/api_test.py
+++ b/tests/unit_tests/databases/api_test.py
@@ -1100,14 +1100,14 @@ def test_oauth2_error(
},
)
- assert response.status_code == 500
+ assert response.status_code == 400
assert response.json == {
"errors": [
{
- "message": "Something went wrong while doing OAuth2",
+ "message": "The OAuth2 provider denied the request",
"error_type": "OAUTH2_REDIRECT_ERROR",
- "level": "error",
- "extra": {"error": "Something bad hapened"},
+ "level": "warning",
+ "extra": None,
}
]
}
diff --git a/tests/unit_tests/databases/oauth2_api_test.py b/tests/unit_tests/databases/oauth2_api_test.py
index 3b6c074f2f9..a47e5056d46 100644
--- a/tests/unit_tests/databases/oauth2_api_test.py
+++ b/tests/unit_tests/databases/oauth2_api_test.py
@@ -23,7 +23,7 @@ import pytest
from pytest_mock import MockerFixture
from requests.exceptions import HTTPError
-from superset import db
+from superset import db, security_manager
from superset.commands.database.exceptions import DatabaseNotFoundError
from superset.daos.database import DatabaseUserOAuth2TokensDAO
from superset.exceptions import OAuth2Error
@@ -151,6 +151,8 @@ def test_oauth2_callback_redacts_exchange_exception_from_all_logs(
"get_database",
return_value=database,
)
+ mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1)
+ mocker.patch.object(security_manager, "is_guest_user", return_value=False)
mocker.patch.object(event_logger, "log")
with caplog.at_level(logging.DEBUG):
@@ -273,3 +275,31 @@ def test_oauth2_callback_metric_failure_preserves_oauth_error(
) in caplog.messages
assert "metrics-payload-sentinel" not in caplog.text
assert "metrics-payload-sentinel" not in response.get_data(as_text=True)
+
+
+@pytest.mark.parametrize(
+ "case",
+ ["provider_denied", "missing_state", "invalid_state", "anonymous"],
+)
+def test_oauth2_callback_rejects_bad_requests_with_4xx(
+ client: Any,
+ full_api_access: None,
+ case: str,
+) -> None:
+ """
+ None of these callback requests can ever succeed: the provider denied
+ consent, the state is missing/tampered, or there's no session (anonymous
+ request) to bind the callback to. They must be reported as client errors,
+ not crash the app.
+ """
+ query_strings = {
+ "provider_denied": {"error": "access_denied", "state": callback_state()},
+ "missing_state": {"code": "XXX"},
+ "invalid_state": {"code": "XXX", "state": "not-a-valid-jwt"},
+ "anonymous": {"code": "XXX", "state": callback_state()},
+ }
+
+ response = client.get("/api/v1/database/oauth2/", query_string=query_strings[case])
+
+ assert 400 <= response.status_code < 500
+ assert response.json["errors"][0]["message"]