fix(oauth2): log database token failures - #42644
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
The logging changes in this pull request are designed to avoid logging sensitive information by using structured logging that captures only metadata, such as superset/commands/database/oauth2.py |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42644 +/- ##
==========================================
- Coverage 66.64% 66.64% -0.01%
==========================================
Files 2863 2863
Lines 162155 162183 +28
Branches 37405 37409 +4
==========================================
+ Hits 108076 108085 +9
- Misses 52020 52035 +15
- Partials 2059 2063 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
authentication:sso
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
dosubot
Bot
added
change:backend
Code Review Agent Run #8ea754Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #d5dd55Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Richard's agent here: Thanks for the thorough OAuth2 hardening. I reviewed Blocking1. Refresh sanitization can lose the re-authentication signalCurrent flow
Engine specs whose Impact The token is deleted, but re-authentication may not start. If the replacement reaches browser or MCP consumers, it also lacks the authorization URL and tab metadata they require. Requested change Preserve the re-authentication decision before sanitizing the exception, or make the sanitized refresh marker unconditionally trigger the OAuth dance. Please add an end-to-end regression test using an engine spec with a vendor-specific OAuth exception. 2. Observability failures can escape after the token is committedThe new decorator order commits the token before event logging and metrics finish. Impact
The current event-log test only raises Requested change Make both callback observability hooks best-effort, and cover:
This also encompasses the existing StatsD review thread. Non-blocking clarification3. Clarify the API-response redaction guaranteeThe existing provider-denial path passes the callback’s arbitrary This predates the PR, so I would not block on it independently. However, the PR description should either:
Could we address the two blockers before merging and clarify the intended redaction boundary? |
69a0873 to
14ed35c
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #238c06
Actionable Suggestions - 1
-
superset/utils/oauth2.py - 1
- Avoid catching blind Exception · Line 180-180
Additional Suggestions - 3
-
superset/utils/oauth2.py - 1
-
Non-OAuth exception message lost · Line 180-188In the non-OAuth exception handler, only the exception type name is logged but the actual error message (str(ex)) is captured but not included in the structured log. This limits diagnostic capability when debugging token refresh failures.
-
-
superset/utils/log.py - 2
-
Weakened type annotation · Line 123-123The type hint change from `**payload_override: dict[str, Any]` to `**payload_override: Any` reduces type safety with no documented justification. This makes it harder for static type checkers to catch misuse at call sites. Either revert to the specific type or document why `Any` is necessary.
-
Inconsistent type annotation · Line 180-180Same type hint change applies here: `**payload_override: dict[str, Any] | None` became `**payload_override: Any`. Apply same decision as line 123 for consistency.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/databases/api.py - 1
- Intentional decorator reordering correct · Line 1457-1463
Review Details
-
Files reviewed - 10 · Commit Range:
bb48c71..14ed35c- superset/commands/database/oauth2.py
- superset/databases/api.py
- superset/db_engine_specs/base.py
- superset/exceptions.py
- superset/utils/log.py
- superset/utils/oauth2.py
- tests/unit_tests/commands/databases/oauth2_test.py
- tests/unit_tests/databases/oauth2_api_test.py
- tests/unit_tests/db_engine_specs/test_base.py
- tests/unit_tests/utils/oauth2_tests.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Requesting changes on the one remaining blocker from the earlier review. For context on the other two items: the OAuth re-authentication concern is effectively fixed in code — OAuth2TokenRefreshError subclasses OAuth2RedirectError, so needs_oauth2 still classifies the sanitized marker for every in-tree spec and start_oauth2_dance re-attaches the auth URL/tab — though it still lacks the requested end-to-end regression test with a vendor-specific OAuth exception. The API-response redaction wording is a non-blocking follow-up (the provider error query param is still reflected into the JSON body via OAuth2Error.extra; low severity, but the guarantee should be narrowed or the field sanitized). Nice work on the redaction boundary otherwise — I verified codes/secrets/tokens/provider payloads no longer reach logs, exceptions, or responses.
Blocker — observability failures can still escape after the token is committed. With @transaction() innermost, the single-use token is persisted before the event-logger and StatsD hooks run, but neither hook is best-effort, so a post-commit failure can turn a committed (un-retriable) callback into a 500 or mask the sanitized OAuth error. Details inline. The three requested regression tests are still missing — the added test_oauth2_callback_event_log_failure_preserves_business_write only injects SQLAlchemyError, which DBEventLogger already swallows, so it doesn't prove isolation from arbitrary logger failures.
14ed35c to
ca1f23d
Compare
|
Addressed the outstanding review feedback in
Validation: OAuth-focused selection 45 passed / 121 deselected, base engine refresh selection 5 passed / 69 deselected, and all changed-file pre-commit hooks passed (MyPy, Ruff format/lint, and Pylint included). All review threads have replies and are resolved. CI follow-up on the final head: current pre-commit, Python unit/integration, required Playwright/Cypress, CodeQL, dependency, license, and build checks passed. The non-blocking experimental embedded-dashboard Playwright jobs hit an unrelated frontend |
ca1f23d to
b1768e7
Compare
There was a problem hiding this comment.
Code Review Agent Run #5912dc
Actionable Suggestions - 1
-
superset/utils/oauth2.py - 1
- Replace broad Exception catch with specific · Line 183-183
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/db_engine_specs/test_base.py - 1
- Test checks wrong attribute for payload · Line 1180-1180
Review Details
-
Files reviewed - 11 · Commit Range:
0de0fcd..b1768e7- superset/commands/database/oauth2.py
- superset/databases/api.py
- superset/db_engine_specs/base.py
- superset/exceptions.py
- superset/utils/log.py
- superset/utils/oauth2.py
- superset/views/base_api.py
- tests/unit_tests/commands/databases/oauth2_test.py
- tests/unit_tests/databases/oauth2_api_test.py
- tests/unit_tests/db_engine_specs/test_base.py
- tests/unit_tests/utils/oauth2_tests.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # Engine specs can delegate to arbitrary provider clients that do not share an | ||
| # exception base class. Sanitize every other provider-boundary failure while | ||
| # preserving the refresh token for a later retry. | ||
| except Exception as ex: # pylint: disable=broad-except |
There was a problem hiding this comment.
Catching a broad Exception is discouraged. If this is intentional for handling various provider errors, consider documenting why and potentially narrowing the scope.
Code Review Run #5912dc
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
5 participants
SUMMARY
Why
OAuth2 database token exchange and refresh failures lacked queryable context, and the callback emitted an outcome-neutral event metric that did not represent failures. Provider exception details and callback request metadata also needed an end-to-end redaction boundary.
What
database_id, canonical engine, and exception type while excluding OAuth codes, tokens, exception text, and provider payloads.DatabaseRestApi.oauth2.success,.warning, or.erroris emitted after transaction completion.Blast radius
Limited to OAuth2 database token exchange/refresh error handling and callback observability. Success-path token persistence remains unchanged. Provider exceptions crossing the OAuth2 boundary are intentionally replaced with sanitized OAuth2 domain exceptions; OAuth-specific refresh failures still trigger token cleanup and re-authentication. The redaction guarantee covers logs, event-log request data, and provider exception/response-body details; it intentionally does not change the existing callback API contract that returns the provider-defined
errorcode inOAuth2Error.extra.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this is backend observability and error-handling work.
TESTING INSTRUCTIONS
Results:
pre-commit run --all-fileswas also attempted. Backend checks passed, but this worktree's frontend hooks cannot load the uninstalledglobandpostcss-styled-syntaxpackages (and docs Yarn cannot read the worktree user's.npmrc); the repository-wide formatter and Ruff hooks also report unrelated existing files outside this PR.RISK & ROLLBACK
Low-to-moderate risk: the success path is unchanged, but failure paths now expose only sanitized OAuth2 domain exceptions, and event logging runs after the token transaction. Revert the four PR commits (
0de0fcd076,b807e190bf,4246a417b6, andb1768e7a07) to restore the prior OAuth2 behavior.REVIEW GUIDANCE
Please focus on:
ADDITIONAL INFORMATION
DatabaseRestApi.oauth2counter with outcome-qualified counters documented inUPDATING.md.