Repository navigation
fix(api): improved request handling and embedded dashboard scoping - #42930
Conversation
Code Review Agent Run #ac918fActionable Suggestions - 0Additional Suggestions - 3
Filtered 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 |
dosubot
Bot
added
api
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42930 +/- ##
==========================================
- Coverage 66.66% 66.63% -0.03%
==========================================
Files 2873 2874 +1
Lines 163654 163856 +202
Branches 37771 37814 +43
==========================================
+ Hits 109093 109182 +89
- Misses 52432 52530 +98
- Partials 2129 2144 +15
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation of To resolve this, you should explicitly disable proxy usage for these connections by setting the Would you like me to fetch all other comments on this PR to validate them and implement a comprehensive fix? superset/commands/dataset/importers/v1/utils.py |
rusackas
changed the title
rusackas
added
the
merge-if-green
Code Review Agent Run #cb2fbcActionable 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 |
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
Reviewed the request-handling and embedded-scoping changes. The controls hold together well.
The data-URL import re-checks the connected peer address on the socket for every hop, not just the initial hostname, and disables proxies so the check applies to the real destination, with the redirect handler re-validating each hop. The OAuth2 callback requires an authenticated non-guest session whose id matches the signed state, which is the right enforcement point given the callback route is unauthenticated by necessity. Guest scoping is tightened consistently: the deck.gl child leg binds the requested datasource to the child chart, native-filter series columns and the series-limit term are validated like other value-returning terms, and the jinja metric macro constrains a guest to datasets reachable through a granted dashboard. Annotation data shares the dataframe cache entry, so binding the requesting user and per-source RLS into the key is the correct fix. The task list fails closed for a request-bound principal with no user id, while the internal executor keeps loading its task by uuid.
Two non-blocking notes: disabling proxies for the import fetch is right for the default config but will affect deployments that need an egress proxy to reach legitimate external URLs, so a short release note would help; and patch coverage is on the low side, mostly on the importer connection path, so a couple of integration tests there would raise confidence. Looks good to me.
|
Went through Bito's three "Additional Suggestions" on this one. The annotation cache-key one is a misread. Same for the backslash-redirect test. The missing cache-header test was fair though, added one in 2e2cdb0. |
Code Review Agent Run #8966e6Actionable 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 |
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
Reviewed the embedded-dashboard scoping and request-handling changes — the guest access paths stay correctly bounded (dashboard-membership + dataset allowlist) and several term/validation checks are tightened. LGTM.
|
@sha174n added an UPDATING.md note on the proxy-disabling behavior, and a test covering |
Code Review Agent Run #ebb973Actionable 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 |
11aeb2f to
d5cf4c6
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 #315649
Actionable Suggestions - 4
-
tests/unit_tests/jinja_context_test.py - 1
- Duplicate DatasetDAO mock setup · Line 1768-1768
-
superset/utils/network.py - 1
- Missing unit tests for is_safe_ip · Line 47-57
-
tests/unit_tests/extensions/storage/test_api.py - 1
- Missing status code assertion · Line 99-103
-
tests/unit_tests/daos/test_tasks.py - 1
- Incorrect mock patch path · Line 407-407
Additional Suggestions - 3
-
tests/unit_tests/tasks/test_filters.py - 1
-
Incorrect fixture parameter type · Line 28-28The `app_context` parameter is annotated as `None`, but the fixture's actual return type is `Iterator[None]`. Update the parameter type to `Iterator[None]` for accurate type checking and to prevent mypy warnings.
-
-
superset/extensions/storage/api.py - 1
-
CWE-356: Cache-Disabling Product UI · Line 100-100Inline import of `jsonify` should move to module-level per BITO.md rule [12745]: imports should only be inline to prevent circular dependencies, and such cases must include an explanatory comment. Since `from flask import g, request` is already at module level without circular import issues, `jsonify` should follow the same pattern for consistency.
-
-
tests/unit_tests/commands/databases/oauth2_test.py - 1
-
Incomplete security validation coverage · Line 218-235The test covers `get_user_id` returning `None` and user ID mismatch, but the third security check in `validate()` — `security_manager.is_guest_user()` — is untested. Add a case where `get_user_id` returns 1 (matching state) but the user is a guest.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/extensions/cache_middleware.py - 1
- Incomplete _info endpoint exclusion · Line 32-33
Review Details
-
Files reviewed - 25 · Commit Range:
44bf81c..d5cf4c6- superset/commands/database/oauth2.py
- superset/commands/dataset/importers/v1/utils.py
- superset/common/query_context_processor.py
- superset/extensions/cache_middleware.py
- superset/extensions/storage/api.py
- superset/jinja_context.py
- superset/security/manager.py
- superset/tasks/context.py
- superset/tasks/decorators.py
- superset/tasks/filters.py
- superset/tasks/manager.py
- superset/tasks/scheduler.py
- superset/utils/link_redirect.py
- superset/utils/network.py
- tests/unit_tests/commands/databases/oauth2_test.py
- tests/unit_tests/common/test_query_context_processor.py
- tests/unit_tests/daos/test_tasks.py
- tests/unit_tests/databases/api_test.py
- tests/unit_tests/datasets/commands/importers/v1/import_test.py
- tests/unit_tests/extensions/storage/test_api.py
- tests/unit_tests/extensions/test_cache_middleware.py
- tests/unit_tests/jinja_context_test.py
- tests/unit_tests/security/manager_test.py
- tests/unit_tests/tasks/test_filters.py
- tests/unit_tests/utils/test_link_redirect.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
Code Review Agent Run #4181d0Actionable 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 |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…proxies on guarded import fetch TaskFilter fails closed for request-bound principals; internal executor reads use the existing skip_base_filter path so task execution still loads its own task. Import fetch disables proxies so peer validation checks the destination. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sts for session binding Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stored values are scoped to the requesting user, so the cache-control header on ExtensionStorageRestApi.response() needs coverage to catch a silent regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review feedback: adds a unit test asserting load_data's opener disables proxy usage when DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS is False, and documents the behavior change for deployments relying on an egress proxy for dataset import URLs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…inding The callback now requires an authenticated user whose id matches the one carried in the OAuth2 state, so this test (added on master after this branch was cut) short-circuited before reaching the token exchange and never emitted the log line it asserts on. Patch `get_user_id` the same way the four sibling tests in this file already do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cee3f80 to
b30d92f
Compare
Code Review Agent Run #aa69d1Actionable Suggestions - 0Additional Suggestions - 6
Filtered 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 |
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
The `skip_base_filters changes look concerning. Let's make sure they're safe, so if you could double check that those paths are safe and don't cause privilege escalation.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ilter TaskFilter's docstring now states the request-scoped vs. internal- plumbing split explicitly, and each skip_base_filter=True call site in the executor/scheduler points back to it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ae54c8a to
62e4d46
Compare
Code Review Agent Run #ce93b8Actionable 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 |
3 participants
SUMMARY
A set of correctness fixes to request handling, embedded-dashboard scope, and per-user response caching.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend behavior).
TESTING INSTRUCTIONS
pytest tests/unit_tests/utils/test_link_redirect.py tests/unit_tests/tasks/test_filters.py tests/unit_tests/extensions/ tests/unit_tests/commands/databases/oauth2_test.py tests/unit_tests/datasets/commands/importers/v1/import_test.py tests/unit_tests/security/manager_test.py tests/unit_tests/common/test_query_context_processor.py tests/unit_tests/jinja_context_test.pyADDITIONAL INFORMATION