You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2026/01/12 22:37:50 UTC
[PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
betodealmeida opened a new pull request, #37067:
URL: https://github.com/apache/superset/pull/37067
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR adds PKCE (Proof Key for Code Exchange - RFC 7636) support to the OAuth2 flow for database connections. PKCE is a security extension to OAuth2 that protects against authorization code interception attacks, which is espec
ially important for public clients or environments where the authorization code could be intercepted.
How it works:
1. When starting OAuth2, a cryptographically secure code_verifier is generated and stored in the JWT-encoded state
2. The code_challenge (SHA-256 hash of the verifier, base64url-encoded) is sent to the authorization server
3. When exchanging the authorization code for tokens, the original code_verifier is included
4. The authorization server verifies the challenge matches the verifier
Changes:
- Added generate_code_verifier() and generate_code_challenge() helper functions in superset/utils/oauth2.py
- Updated OAuth2State TypedDict to include optional code_verifier field
- Updated start_oauth2_dance() to generate PKCE code_verifier
- Updated get_oauth2_authorization_uri() to include code_challenge and code_challenge_method=S256 parameters
- Updated get_oauth2_token() to accept and forward the code_verifier parameter
- Updated OAuth2StoreTokenCommand to pass code_verifier from state to token exchange
All changes are backward compatible - databases that don't support PKCE will simply ignore the additional parameters, and existing OAuth2 flows without PKCE will continue to work.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A - No UI changes. The PKCE parameters are added transparently to the OAuth2 flow.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Added unit tests.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3741010632
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2743535470
##########
superset/commands/database/oauth2.py:
##########
@@ -50,9 +53,28 @@ def run(self) -> DatabaseUserOAuth2Tokens:
if oauth2_config is None:
raise OAuth2Error("No configuration found for OAuth2")
+ # Look up PKCE code_verifier from KV store (RFC 7636)
+ code_verifier = None
+ tab_id = self._state["tab_id"]
Review Comment:
That's very unlikely. We only use `total=False` because of the PKCE payload being optional, everything else will be there.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3827428011
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2718639087
##########
superset/db_engine_specs/base.py:
##########
@@ -499,6 +510,8 @@ def start_oauth2_dance(cls, database: Database) -> None:
# UUID to the original tab, and the second tab will use it when sending the
# message.
"tab_id": tab_id,
+ # PKCE code verifier stored in state to be retrieved during token exchange
+ "code_verifier": code_verifier,
Review Comment:
Great catch! Let me fix this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3740834816
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3827123498
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3820635257
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2718586758
##########
superset/db_engine_specs/base.py:
##########
@@ -499,6 +510,8 @@ def start_oauth2_dance(cls, database: Database) -> None:
# UUID to the original tab, and the second tab will use it when sending the
# message.
"tab_id": tab_id,
+ # PKCE code verifier stored in state to be retrieved during token exchange
+ "code_verifier": code_verifier,
Review Comment:
**Suggestion:** The PKCE code verifier is being embedded into the OAuth2 `state` payload, which is a signed but unencrypted JWT sent as a query parameter; this allows any interceptor who can see the authorization code to also decode the state and obtain the `code_verifier`, defeating the security guarantees of PKCE and reβenabling authorization code interception attacks. [security]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Database OAuth2 authentication compromise risk
- β OAuth2 redirect flow (DatabaseRestApi.oauth2) security degraded
- β οΈ Token exchange endpoint can be abused if intercepted
- β οΈ Unit tests currently assume state contains code_verifier
```
</details>
```suggestion
# PKCE code verifier is managed server-side and not stored in OAuth2 state
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Trigger a DB OAuth2 authorization flow by calling BaseEngineSpec.start_oauth2_dance
(superset/db_engine_specs/base.py:472-516). This method generates a PKCE code_verifier
(base.py:490) and inserts it into the OAuth2 state dict under the "code_verifier" key
(base.py:513-514).
2. The state dict is encoded and sent to the provider via
BaseEngineSpec.get_oauth2_authorization_uri, which calls encode_oauth2_state(state)
(base.py:572-576 -> encode_oauth2_state defined in superset/utils/oauth2.py:152). The
code_verifier is therefore embedded into the encoded state that becomes a query parameter
on the authorization request URL.
3. Inspect superset/utils/oauth2.py: around lines 152-165 β encode_oauth2_state includes
state keys in the JWT payload and (per grep results) will add "code_verifier" to the
encoded payload when present (utils/oauth2.py:163 shows payload["code_verifier"] =
state["code_verifier"]). Because the state is a signed JWT but not encrypted, any party
that can observe the authorization request/redirect (e.g., network traffic, logs, browser
history, referrers) and can decode the JWT (signed but not encrypted) can obtain the
code_verifier.
4. An attacker who intercepts the authorization code and the state (both are returned to
the redirect URI) can extract the code_verifier from the state and use it to redeem the
intercepted authorization code at the token endpoint. The token exchange path that accepts
a code_verifier exists (BaseEngineSpec.get_oauth2_token, base.py:589-614) and is called by
the server-side command that completes the flow
(superset/commands/database/oauth2.py:50-58 passes code_verifier from state into token
request). This demonstrates the concrete end-to-end leak: start_oauth2_dance stores
code_verifier in state -> encode_oauth2_state places it in JWT payload -> token exchange
consumes it from the received state.
5. Conclusion / why this is a problem: Embedding the PKCE secret (code_verifier) into the
client-visible state defeats PKCE's protection model; PKCE requires the verifier to remain
confidential between the client and the authorization server exchange step. The current
implementation places that secret into a signed-but-unencrypted state JWT that accompanies
the authorization code, enabling an interceptor to both obtain the authorization code and
the secret needed to redeem it.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 513:514
**Comment:**
*Security: The PKCE code verifier is being embedded into the OAuth2 `state` payload, which is a signed but unencrypted JWT sent as a query parameter; this allows any interceptor who can see the authorization code to also decode the state and obtain the `code_verifier`, defeating the security guarantees of PKCE and reβenabling authorization code interception attacks.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3826557261
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3740845133
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 20211f29b2fdbb6a44b9c19fb71991adc2153944 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/696577beb4404b00080e5266 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-37067--superset-docs-preview.netlify.app](https://deploy-preview-37067--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37067--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3786896469
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #37067:
URL: https://github.com/apache/superset/pull/37067
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2743763909
##########
superset/db_engine_specs/base.py:
##########
@@ -523,9 +530,9 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
max_column_name_length: int | None = None
try_remove_schema_from_table_name = True # pylint: disable=invalid-name
run_multiple_statements_as_one = False
- custom_errors: dict[
- Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]
- ] = {}
+ custom_errors: dict[Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]] = (
+ {}
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type mismatch in class attribute</b></div>
<div id="fix">
The custom_errors class attribute is typed as dict but assigned a tuple, which will cause an AttributeError at runtime when .items() is called on it in error handling code.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
custom_errors: dict[Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]] = {}
````
</div>
</details>
</div>
<small><i>Code Review Run #c1544d</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3787312448
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2743520507
##########
superset/superset_typing.py:
##########
@@ -354,7 +354,7 @@ class OAuth2TokenResponse(TypedDict, total=False):
refresh_token: str
-class OAuth2State(TypedDict):
+class OAuth2State(TypedDict, total=False):
Review Comment:
**Suggestion:** The `OAuth2State` TypedDict is declared with `total=False`, implying that all keys are optional, but all callers (such as `encode_oauth2_state` in `superset/utils/oauth2.py`) unconditionally index `database_id`, `user_id`, `default_redirect_uri`, and `tab_id`, so if any future caller constructs a partial state that type-checks, it will trigger a `KeyError` at runtime when encoding the state. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β DB OAuth2 start flow can crash with KeyError.
- β οΈ Tests or custom callers constructing partial states.
- β οΈ Encoded-state creation fails blocking OAuth2 redirects.
```
</details>
```suggestion
class OAuth2State(TypedDict):
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Trigger the OAuth2 encoding logic by calling encode_oauth2_state(state) in
superset/utils/oauth2.py:152 (function defined at lines 152-174). This function reads
state["database_id"], state["user_id"], state["default_redirect_uri"], and state["tab_id"]
unconditionally (see lines 156-162).
2. Construct an OAuth2State instance with one or more missing keys (e.g., {"database_id":
1, "user_id": 2} ) in application code or tests and pass it to encode_oauth2_state.
Because OAuth2State is declared total=False, type checkers allow the partial dict to
type-check, but at runtime encode_oauth2_state will attempt to index missing keys and
raise KeyError. The exact failure happens at superset/utils/oauth2.py:158 when accessing
state["database_id"] (and similarly for other keys).
3. Observe a KeyError traceback originating at superset/utils/oauth2.py:156-162 (payload
construction) causing the OAuth2 start flow to fail; the caller (e.g., whatever initiated
the OAuth2 dance) will receive an exception instead of a valid encoded state.
4. Note: decoding the state uses OAuth2StateSchema which enforces required fields
(superset/utils/oauth2.py:176-181). However, the encode path does not perform validation
and therefore is vulnerable to partial-state dicts constructed elsewhere. The TypedDict
being total=False permits creation of partial states that will crash at runtime when
encode_oauth2_state indexes missing keys.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/superset_typing.py
**Line:** 357:357
**Comment:**
*Possible Bug: The `OAuth2State` TypedDict is declared with `total=False`, implying that all keys are optional, but all callers (such as `encode_oauth2_state` in `superset/utils/oauth2.py`) unconditionally index `database_id`, `user_id`, `default_redirect_uri`, and `tab_id`, so if any future caller constructs a partial state that type-checks, it will trigger a `KeyError` at runtime when encoding the state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/commands/database/oauth2.py:
##########
@@ -50,9 +53,28 @@ def run(self) -> DatabaseUserOAuth2Tokens:
if oauth2_config is None:
raise OAuth2Error("No configuration found for OAuth2")
+ # Look up PKCE code_verifier from KV store (RFC 7636)
+ code_verifier = None
+ tab_id = self._state["tab_id"]
Review Comment:
**Suggestion:** The new PKCE lookup logic assumes that the OAuth2 state always contains a `tab_id` key and indexes it directly; however `OAuth2State` is defined as a non-total `TypedDict`, so if a caller ever passes a state without `tab_id` (for example, older tokens, custom clients, or future reuse of this command with a reduced state), this will raise a `KeyError` and break the token exchange instead of simply skipping PKCE. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β OAuth2 token exchange flow may raise KeyError and fail.
- β Database OAuth2 connection setup blocked on callback.
- β οΈ Breaks PKCE only for requests missing tab_id.
- β οΈ Unit/integration tests may surface regressions.
```
</details>
```suggestion
tab_uuid = None
tab_id = self._state.get("tab_id")
if tab_id:
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. The OAuth2 token exchange flow decodes the state in validate() at
superset/commands/database/oauth2.py:103 (self._state =
decode_oauth2_state(self._parameters["state"])). This populates self._state with the
TypedDict from superset/superset_typing.py:357 (OAuth2State, total=False).
2. Call OAuth2StoreTokenCommand.run() which executes the PKCE lookup starting at
superset/commands/database/oauth2.py:56-72. In the current code it indexes
self._state["tab_id"] directly at line 58.
3. If the decoded state does not include "tab_id" (allowed because OAuth2State is
TypedDict total=False β see superset/superset_typing.py:357), Python will raise a KeyError
at superset/commands/database/oauth2.py:58 before the code reaches the token exchange (the
call to get_oauth2_token at lines 74-78).
4. The failure occurs during handling of the OAuth2 provider callback (token exchange
command execution). Observed result: a KeyError traceback originating at
superset/commands/database/oauth2.py:58, aborting token storage and causing the OAuth2
flow to fail for that callback.
5. The proposed change (use self._state.get("tab_id") and guard) prevents KeyError and
will simply skip PKCE when tab_id is absent, restoring backward compatibility with
older/partial states. This is necessary because OAuth2State is explicitly non-total
(superset/superset_typing.py:357) so absence of tab_id is a valid/expected shape.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/database/oauth2.py
**Line:** 58:58
**Comment:**
*Possible Bug: The new PKCE lookup logic assumes that the OAuth2 state always contains a `tab_id` key and indexes it directly; however `OAuth2State` is defined as a non-total `TypedDict`, so if a caller ever passes a state without `tab_id` (for example, older tokens, custom clients, or future reuse of this command with a reduced state), this will raise a `KeyError` and break the token exchange instead of simply skipping PKCE.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -93,3 +101,96 @@ def test_get_oauth2_access_token_base_no_refresh(mocker: MockerFixture) -> None:
# check that token was deleted
db.session.delete.assert_called_with(token)
+
+
+def test_generate_code_verifier_length() -> None:
+ """
+ Test that generate_code_verifier produces a string of valid length (RFC 7636).
+ """
+ code_verifier = generate_code_verifier()
+ # RFC 7636 requires 43-128 characters
+ assert 43 <= len(code_verifier) <= 128
+
+
+def test_generate_code_verifier_uniqueness() -> None:
+ """
+ Test that generate_code_verifier produces unique values.
+ """
+ verifiers = {generate_code_verifier() for _ in range(100)}
+ # All generated verifiers should be unique
+ assert len(verifiers) == 100
+
+
+def test_generate_code_verifier_valid_characters() -> None:
+ """
+ Test that generate_code_verifier only uses valid characters (RFC 7636).
+ """
+ code_verifier = generate_code_verifier()
+ # RFC 7636 allows: [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~"
+ # URL-safe base64 uses: [A-Z] / [a-z] / [0-9] / "-" / "_"
+ valid_chars = set(
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"
+ )
+ assert all(char in valid_chars for char in code_verifier)
+
+
+def test_generate_code_challenge_s256() -> None:
+ """
+ Test that generate_code_challenge produces correct S256 challenge.
+ """
+ # Use a known code_verifier to verify the challenge computation
+ code_verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"
+
+ # Compute expected challenge manually
+ digest = hashlib.sha256(code_verifier.encode("ascii")).digest()
+ expected_challenge = base64.urlsafe_b64encode(digest).rstrip(b"=").decode("ascii")
+
+ code_challenge = generate_code_challenge(code_verifier)
+ assert code_challenge == expected_challenge
+
+
+def test_generate_code_challenge_rfc_example() -> None:
+ """
+ Test PKCE code challenge against RFC 7636 Appendix B example.
+
+ See: https://datatracker.ietf.org/doc/html/rfc7636#appendix-B
+ """
+ # RFC 7636 example code_verifier (Appendix B)
+ code_verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"
+ # RFC 7636 expected code_challenge for S256 method
+ expected_challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"
+
+ code_challenge = generate_code_challenge(code_verifier)
+ assert code_challenge == expected_challenge
+
+
+def test_encode_decode_oauth2_state(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that encode/decode cycle preserves state fields.
+ """
+ from superset.superset_typing import OAuth2State
+
+ mocker.patch(
+ "flask.current_app.config",
+ {
+ "SECRET_KEY": "test-secret-key",
+ "DATABASE_OAUTH2_JWT_ALGORITHM": "HS256",
Review Comment:
**Suggestion:** The test currently asserts that the decoded OAuth2 state does not contain a `code_verifier` field, which contradicts the PKCE design where the verifier must survive the encode/decode cycle so it can be sent when exchanging the authorization code; this test will lock in broken behavior and prevent PKCE from working correctly. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β PKCE support broken for OAuth2 database connections.
- β Token exchange missing code_verifier parameter.
- β οΈ Unit tests accept incorrect behavior masking regressions.
- β οΈ OAuth2 flows for public clients become less secure.
```
</details>
```suggestion
"code_verifier": "test-code-verifier",
}
with freeze_time("2024-01-01"):
encoded = encode_oauth2_state(state)
decoded = decode_oauth2_state(encoded)
assert decoded["database_id"] == 1
assert decoded["user_id"] == 2
assert decoded.get("code_verifier") == "test-code-verifier"
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Inspect encode_oauth2_state implementation at superset/utils/oauth2.py:152-173 β it
builds JWT payload with keys: exp, database_id, user_id, default_redirect_uri, tab_id (see
lines 156-162) and does NOT include code_verifier in the encoded payload.
2. Inspect decode_oauth2_state implementation at superset/utils/oauth2.py:204-218 β it
decodes the JWT and loads it via oauth2_state_schema (lines 211-216). The schema
(OAuth2StateSchema) defined at superset/utils/oauth2.py:176-195 only lists database_id,
user_id, default_redirect_uri, tab_id and does not accept or preserve code_verifier (lines
176-181, 189-194).
3. Run the unit test
tests/unit_tests/utils/oauth2_tests.py::test_encode_decode_oauth2_state which currently
creates a state without a code_verifier and asserts "code_verifier" not in decoded (tests
file lines shown in PR). Because encode_oauth2_state never includes code_verifier and the
schema excludes unknown keys, a code_verifier present in the input state would be dropped
during encode/decode β reproducing the assertion the test makes.
4. Real PKCE flow requires the code_verifier to survive encode/decode so it can be sent on
token exchange. To reproduce the breaking scenario concretely:
- Modify the test to include a code_verifier in the input state
(tests/unit_tests/utils/oauth2_tests.py: add "code_verifier": "test-code-verifier").
- Call encode_oauth2_state(state) (superset/utils/oauth2.py:152) then
decode_oauth2_state(encoded) (superset/utils/oauth2.py:204).
- Observe that decoded does NOT contain "code_verifier" because the payload omits it at
encode time (lines 156-162) and the schema excludes unknown fields (OAuth2StateSchema.
Meta unknown = EXCLUDE at line 198).
5. Conclusion: The test's current assertion that the decoded state must not contain
code_verifier masks a real problem β encode_oauth2_state and the schema must be updated to
preserve code_verifier if PKCE is intended. The improved test (proposed) will fail under
current code, proving the bug.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/oauth2_tests.py
**Line:** 172:179
**Comment:**
*Logic Error: The test currently asserts that the decoded OAuth2 state does not contain a `code_verifier` field, which contradicts the PKCE design where the verifier must survive the encode/decode cycle so it can be sent when exchanging the authorization code; this test will lock in broken behavior and prevent PKCE from working correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/db_engine_specs/base.py:
##########
@@ -474,10 +481,33 @@ 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),
+ )
+ db.session.flush()
Review Comment:
**Suggestion:** The PKCE `code_verifier` is written to the key-value store and then `OAuth2RedirectError` is raised without committing the transaction, so in configurations where sessions are rolled back on exceptions the `code_verifier` row will never actually be persisted and the subsequent token exchange will not find it, causing the PKCE flow to fail; replacing the flush with a commit (or otherwise ensuring a commit here) guarantees the verifier is durable before redirecting the user. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β OAuth2 PKCE token exchange fails for DB OAuth2 flows
- β οΈ OAuth2 database authentication unreliable in some deployments
- β οΈ Affects any engine using DATABASE_OAUTH2_CLIENTS config
```
</details>
```suggestion
db.session.commit()
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a Database with OAuth2 in app config and mark engine as OAuth2-enabled. (See
superset/db_engine_specs/base.py start_oauth2_dance where PKCE is generated and stored;
PKCE storage occurs at the snippet starting at superset/db_engine_specs/base.py:501.)
2. Trigger an OAuth2 authorization flow from a frontend operation that hits the code path
which raises OAuth2RedirectError. In practice this happens when a DB call raises an
OAuth2RedirectError and execute() calls start_oauth2_dance (execute handles exceptions and
calls cls.start_oauth2_dance(database) β see superset/db_engine_specs/base.py execute
method which flows into start_oauth2_dance).
3. start_oauth2_dance generates a code_verifier and calls KeyValueDAO.create_entry, then
calls db.session.flush() (lines shown in the existing_code block at
superset/db_engine_specs/base.py:501-509).
4. Because an OAuth2RedirectError is raised immediately after (the flow raises
OAuth2RedirectError to redirect the browser β see the raise at
superset/db_engine_specs/base.py around the OAuth redirect), some configurations roll back
the current DB session on exceptions. In those setups the earlier db.session.flush() does
not persist the KeyValueDAO entry (it can be rolled back), so the later token exchange
that looks up the verifier in the KV store fails to find it and PKCE token exchange is
rejected.
5. Reproduce locally by: (a) enabling DATABASE_OAUTH2_CLIENTS for an engine, (b) causing a
DB operation that triggers OAuth2 redirect (the path through execute() -> needs_oauth2()
-> start_oauth2_dance()), (c) inspect the KV store for the tab_id key β the entry will be
missing if the session was rolled back because only flush() was called. Observed failure:
the token exchange step cannot validate PKCE because stored verifier does not exist.
6. Explanation: a flush() only sends SQL to the DB within the transaction; without commit
the row can be rolled back by exception handlers. Replacing flush() with commit() (or
ensuring the entry is persisted outside the request transaction) guarantees durability
across the redirect and subsequent token exchange.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 509:509
**Comment:**
*Logic Error: The PKCE `code_verifier` is written to the key-value store and then `OAuth2RedirectError` is raised without committing the transaction, so in configurations where sessions are rolled back on exceptions the `code_verifier` row will never actually be persisted and the subsequent token exchange will not find it, causing the PKCE flow to fail; replacing the flush with a commit (or otherwise ensuring a commit here) guarantees the verifier is durable before redirecting the user.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3821346584
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3821074525
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3820845294
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2748310534
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -709,7 +724,11 @@ def test_oauth2_permissions(
)
assert response.status_code == 200
- get_oauth2_token.assert_called_with({"id": "one", "secret": "two"}, "XXX")
+ get_oauth2_token.assert_called_with(
+ {"id": "one", "secret": "two"},
+ "XXX",
+ code_verifier=None,
Review Comment:
**Suggestion:** Similar to the happy-path test, this permissions test now enforces that `get_oauth2_token` must always receive a `code_verifier` keyword argument (even when PKCE is not used), which can break third-party engine specs that still implement the older signature and undermines the stated backward-compatibility guarantee; the test should only assert the positional arguments for the non-PKCE case. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Unit tests fail in CI for engines with old signatures.
- β οΈ Tests incorrectly force API contract change.
- β οΈ May block PRs until tests/engine specs sync.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the unit test test_oauth2_permissions in tests/unit_tests/databases/api_test.py
(file lines ~665-739). The test patches SqliteEngineSpec.get_oauth2_token at line 697 and
then triggers the OAuth2 redirect handling via the client call at lines 717-724.
2. If the engine spec implementation under test still uses the older signature (two
positional args), the actual call will be recorded by the Mock as a positional 2-arg call.
3. The test reaches the assertion at ./tests/unit_tests/databases/api_test.py:727-731
which enforces that the call included the keyword code_verifier=None. Because Mock
recorded only positional arguments, Mock.assert_called_with(..., code_verifier=None) fails
with an AssertionError describing argument mismatch.
4. Reproduce by running pytest -q
tests/unit_tests/databases/api_test.py::test_oauth2_permissions against an environment
where engine specs have not been updated to accept a code_verifier kwarg β the assertion
at lines 727-731 will fail even though runtime behavior (no PKCE) is correct. Changing the
assertion to the improved_code (only positional args) verifies the existing
backward-compatible behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/api_test.py
**Line:** 730:730
**Comment:**
*Possible Bug: Similar to the happy-path test, this permissions test now enforces that `get_oauth2_token` must always receive a `code_verifier` keyword argument (even when PKCE is not used), which can break third-party engine specs that still implement the older signature and undermines the stated backward-compatibility guarantee; the test should only assert the positional arguments for the non-PKCE case.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -641,7 +648,11 @@ def test_oauth2_happy_path(
)
assert response.status_code == 200
- get_oauth2_token.assert_called_with({"id": "one", "secret": "two"}, "XXX")
+ get_oauth2_token.assert_called_with(
+ {"id": "one", "secret": "two"},
+ "XXX",
+ code_verifier=None,
Review Comment:
**Suggestion:** The new assertion forces `get_oauth2_token` to always be called with a `code_verifier` keyword argument (even when it is `None`), which contradicts the PR's claim of backward compatibility and can cause `TypeError: got an unexpected keyword argument 'code_verifier'` for engine specs that haven't been updated to accept this parameter; instead, the non-PKCE test should assert the existing positional arguments only. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Unit tests fail in CI for engines with old signatures.
- β οΈ Breaks backward-compatibility guarantee in tests.
- β οΈ Prevents merges until tests adjusted.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the unit test test_oauth2_happy_path in tests/unit_tests/databases/api_test.py
(file lines ~590-663). The test patches SqliteEngineSpec.get_oauth2_token at line 621 and
then exercises the API handler by calling the Flask test client at lines 642-648.
2. The production path under test calls the engine spec's get_oauth2_token implementation.
If that implementation (third-party or older engine spec) calls/accepts only positional
args (e.g., def get_oauth2_token(cfg, code): ...) the recorded call on the patched method
will be a positional call with two arguments.
3. After exercising the endpoint, the test reaches the assertion at file
./tests/unit_tests/databases/api_test.py:651-655 which currently requires the recorded
call to include the keyword argument code_verifier=None. Because the real call was
recorded as two positional arguments, Mock.assert_called_with(..., code_verifier=None)
will fail with an AssertionError (arg mismatch) even though the flow was otherwise correct
and backward-compatible.
4. Reproducing locally: run pytest -q
tests/unit_tests/databases/api_test.py::test_oauth2_happy_path against a codebase where an
engine spec's get_oauth2_token signature is the old positional-only form β the test will
fail at the assert_called_with at lines 651-655 with a mismatch describing positional vs
keyword args. The fix in improved_code removes the enforced keyword and matches existing
two-arg call pattern, keeping backward compatibility.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/api_test.py
**Line:** 654:654
**Comment:**
*Possible Bug: The new assertion forces `get_oauth2_token` to always be called with a `code_verifier` keyword argument (even when it is `None`), which contradicts the PR's claim of backward compatibility and can cause `TypeError: got an unexpected keyword argument 'code_verifier'` for engine specs that haven't been updated to accept this parameter; instead, the non-PKCE test should assert the existing positional arguments only.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(oauth2): add PKCE support for database OAuth2 authentication [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37067:
URL: https://github.com/apache/superset/pull/37067#issuecomment-3827247839
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org