bito-code-review[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2744319064
##########
superset/db_engine_specs/base.py:
##########
@@ -608,10 +613,35 @@ def start_oauth2_dance(cls, database: Database) -> None:
tab sends a message to the original tab informing that authorization
was
successful (or not), and then closes. The original tab will
automatically
re-run the query after authorization.
+
+ PKCE (RFC 7636) is used to protect against authorization code
interception
+ attacks. A code_verifier is generated and stored server-side in the KV
store,
+ while the code_challenge (derived from the verifier) is sent to the
+ authorization server.
"""
+ # Prevent circular import.
+ from superset.daos.key_value import KeyValueDAO
+
tab_id = str(uuid4())
default_redirect_uri = url_for("DatabaseRestApi.oauth2",
_external=True)
+ # Generate PKCE code verifier (RFC 7636)
+ code_verifier = generate_code_verifier()
+
+ # Store the code_verifier server-side in the KV store, keyed by tab_id.
+ # This avoids exposing it in the URL/browser history via the JWT state.
+ KeyValueDAO.delete_expired_entries(KeyValueResource.PKCE_CODE_VERIFIER)
+ KeyValueDAO.create_entry(
+ resource=KeyValueResource.PKCE_CODE_VERIFIER,
+ value={"code_verifier": code_verifier},
+ codec=JsonKeyValueCodec(),
+ key=UUID(tab_id),
+ expires_on=datetime.now() + timedelta(minutes=5),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Datetime without timezone information</b></div>
<div id="fix">
Replace `datetime.now()` with `datetime.now(timezone.utc)` or use
`datetime.utcnow()` to ensure timezone-aware datetime objects.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
@@ -639,1 +639,1 @@
- expires_on=datetime.now() + timedelta(minutes=5),
+ expires_on=datetime.now(timezone.utc) + timedelta(minutes=5),
```
</div>
</details>
</div>
<small><i>Code Review Run #00c8f3</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/superset_typing.py:
##########
@@ -356,7 +356,7 @@ class OAuth2TokenResponse(TypedDict, total=False):
refresh_token: str
-class OAuth2State(TypedDict):
+class OAuth2State(TypedDict, total=False):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect Optional Fields in TypedDict</b></div>
<div id="fix">
Adding `total=False` to `OAuth2State` makes all fields optional, but the
code in `encode_oauth2_state`, `decode_oauth2_state`, and other usages assumes
they are always present (e.g., direct key access like `state["database_id"]`).
This would cause MyPy type errors and potential runtime KeyErrors if fields are
ever missing, though currently all usages provide them.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
class OAuth2State(TypedDict):
````
</div>
</details>
</div>
<small><i>Code Review Run #00c8f3</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]
