bito-code-review[bot] commented on code in PR #44769:
URL: https://github.com/apache/superset/pull/44769#discussion_r4127488305
##########
superset/exceptions.py:
##########
@@ -439,6 +439,29 @@ def __init__(self, error: str):
)
+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,
+ )
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is valid and appropriate to address.
It highlights a security concern regarding the potential for unvalidated input
to be reflected in client-visible error messages, which is a common vector for
CWE-20. Applying this suggestion by moving provider-supplied error details into
an `extra` field, rather than the primary `message` field, improves the
security posture of the exception handling.
##########
tests/unit_tests/commands/databases/oauth2_test.py:
##########
@@ -107,8 +107,29 @@ def test_validate_database_not_found(
def test_validate_oauth2_error(mock_parameters: OAuth2ProviderResponseSchema)
-> None:
mock_parameters["error"] = "OAuth2 failure"
command = OAuth2StoreTokenCommand(mock_parameters)
- with pytest.raises(OAuth2Error, match="Something went wrong while doing
OAuth2"):
+ with pytest.raises(OAuth2RejectedError, match="OAuth2 failure") as
exc_info:
command.validate()
+ assert exc_info.value.status == 400
+
+
+def test_validate_missing_state(
+ mock_parameters: OAuth2ProviderResponseSchema,
+) -> None:
+ 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:
+ 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
Review Comment:
<!-- Bito Reply -->
The user has addressed the reviewer's suggestion by adding the requested
docstrings to `test_validate_missing_state` and `test_validate_invalid_state`
in `tests/unit_tests/commands/databases/oauth2_test.py`. The changes document
the expected HTTP 400 status for these scenarios, ensuring consistency with
existing tests.
##########
superset/commands/database/oauth2.py:
##########
@@ -118,17 +121,31 @@ def run(self) -> DatabaseUserOAuth2Tokens:
def validate(self) -> None:
if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ raise OAuth2RejectedError(
+ f"The OAuth2 provider denied the request: {error}"
+ )
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion correctly identified a security risk (CWE-117)
where reflecting unauthenticated, provider-controlled input into logs and
responses could lead to log injection. The user has addressed this by ensuring
that the provider error text is no longer interpolated into the exception
message, instead using fixed, application-authored messages. This approach
effectively mitigates the risk while maintaining useful error reporting.
##########
superset/commands/database/oauth2.py:
##########
@@ -118,17 +121,31 @@ def run(self) -> DatabaseUserOAuth2Tokens:
def validate(self) -> None:
if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ raise OAuth2RejectedError(
+ f"The OAuth2 provider denied the request: {error}"
+ )
+
+ try:
+ state = self._parameters["state"]
+ except KeyError:
+ raise OAuth2RejectedError(
+ "The OAuth2 callback is missing the state parameter"
+ ) from None
- self._state = decode_oauth2_state(self._parameters["state"])
+ try:
+ self._state = decode_oauth2_state(state)
+ except (jwt.PyJWTError, ValidationError):
+ raise OAuth2RejectedError("The OAuth2 state parameter is invalid")
from None
Review Comment:
<!-- Bito Reply -->
The suggestion from the reviewer is not required because the existing
`validate` method already handles `jwt.PyJWTError` and
`marshmallow.ValidationError`. Furthermore, `database_id` is defined as a
`fields.Int` in the schema, so missing or non-integer values are caught by
schema validation before the `post_load` dictionary lookup, preventing the
`KeyError` mentioned in the review. The current implementation correctly raises
`OAuth2RejectedError` with a 400 status for these cases, as verified by the
regression tests.
**superset/commands/database/oauth2.py**
```
try:
self._state = decode_oauth2_state(state)
except (jwt.PyJWTError, ValidationError):
raise OAuth2RejectedError("The OAuth2 state parameter is
invalid") from None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
