bito-code-review[bot] commented on code in PR #44769:
URL: https://github.com/apache/superset/pull/44769#discussion_r4127156619
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Unvalidated Input In Message</b></div>
<div id="fix">
`message=error` puts caller-supplied text — including the unvalidated
provider `error` query param embedded at `OAuth2StoreTokenCommand.validate`
(oauth2.py:124-125) — into the client-visible error message returned by the
`SupersetErrorException` handler. `OAuth2Error` deliberately keeps provider
payloads in `extra` (filtered by `SAFE_EXTRA_KEYS`); mirror that here.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #813107</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-117: Log Injection via Provider Error</b></div>
<div id="fix">
The provider-controlled `error` query param is now interpolated into the
`OAuth2RejectedError` message. This endpoint (`/api/v1/database/oauth2/`) is
unauthenticated (no `@protect()`), so an attacker can craft a callback URL
whose `error` value is reflected verbatim into the JSON response body and into
logs via the traceback in `error_handling.py` (`logger.warning(...,
exc_info=True)`). Previously `OAuth2Error` used a fixed message and kept the
raw value in `extra`, which `error_sanitization.py` drops. Sanitize/truncate
the value or keep a constant message.
([CWE-117](https://cwe.mitre.org/data/definitions/117.html))
</div>
</div>
<small><i>Code Review Run #813107</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstrings on new tests</b></div>
<div id="fix">
New tests `test_validate_missing_state` and `test_validate_invalid_state`
lack docstrings, which BITO.md adaptive rules 12147/12148 mandate for every new
test function. Sibling `test_validate_rejects_state_not_bound_to_session` (line
239) includes one — adding a brief docstring keeps the file consistent and
documents the expected 400 rejection per scenario.
</div>
</div>
<small><i>Code Review Run #813107</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missed decode failure modes</b></div>
<div id="fix">
`decode_oauth2_state` (`superset/utils/oauth2.py:417`) loads the JWT payload
through `oauth2_state_schema.load(payload)`; a signed token whose payload omits
a required field raises marshmallow `ValidationError`, and a payload with a
non-UUID `database_id` raises `KeyError` inside the schema. Neither is a
subclass of `jwt.PyJWTError`, so this handler misses them and the callback 500s
instead of returning the intended 4xx rejection.
</div>
</div>
<small><i>Code Review Run #813107</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
