fix: add missing access checks to semantic-layer and theme endpoints - #43389
fix: add missing access checks to semantic-layer and theme endpoints#43389rusackas wants to merge 8 commits into
Conversation
…ration Add SemanticLayer.raise_for_access(), mirroring the existing SemanticView.raise_for_access(), and call it from the layer GET, runtime schema, and views endpoints before returning or exercising a layer's decrypted configuration. Previously these endpoints resolved the layer by UUID with no per-object permission check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a layer Add security_manager.raise_for_editorship() checks to UpdateSemanticLayerCommand.validate, DeleteSemanticLayerCommand.validate, and CreateSemanticViewCommand.validate, mirroring the pattern already used by the sibling Update/Delete/BulkDeleteSemanticViewCommand classes. Previously these commands only checked existence/uniqueness, so anyone with generic write access to semantic layers could modify or delete any layer. Wire the new SemanticLayerForbiddenError into 403 responses on the layer PUT/DELETE routes, and handle SemanticViewForbiddenError per-item in the bulk semantic-view create loop so one forbidden item doesn't abort the whole batch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…system theme, and harden script-tag stripping Theme was missing from GAMMA_READ_ONLY_MODEL_VIEWS, so default Gamma role sync granted full Theme CRUD instead of read-only access like the sibling CssTemplate resource; move Theme writes to Alpha+ to match. UpdateThemeCommand.validate only rejected edits to is_system themes, not the theme currently promoted to the live system-default/dark slot (is_system_default/is_system_dark), which SetSystemDefaultThemeCommand never marks is_system. Add a non-admin guard on those flags so only an admin can edit the theme actively rendered for every user; map the new SystemThemeInUseError to a 403 in the PUT route. sanitize_svg_content's script-tag regex only matched a bare literal </script> closer, so a closing tag with trailing attributes/whitespace (e.g. </script foo>), which browsers still treat as a valid closer, survived removal. Make the closing-tag pattern tolerant of that, and add a second pass that strips any leftover <script>/</script> fragment, including an unterminated opener with no matching closer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
import_theme's overwrite branch only checked can_write('Theme') before
replacing an existing theme matched by UUID, unlike UpdateThemeCommand and
DeleteThemeCommand which both refuse to touch system-protected themes.
Since seeded themes have stable, publicly-listable UUIDs, an import
bundle with overwrite=true could replace a system theme, or the theme
currently in the system-default/dark slot, bypassing the protection the
direct update/delete routes already enforce. Raise ThemeImportError
before binding the existing row's id when is_system, is_system_default,
or is_system_dark is set.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
authentication:access-control
Code Review Agent Run #d2d62cActionable 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43389 +/- ##
==========================================
- Coverage 66.84% 66.79% -0.05%
==========================================
Files 2876 2876
Lines 164066 164435 +369
Branches 37863 37946 +83
==========================================
+ Hits 109666 109838 +172
- Misses 52226 52401 +175
- Partials 2174 2196 +22
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The regression test for unterminated script tags currently only verifies the removal of the To implement this, update the test case in |
…tale permissions test, allow admin theme import overwrite - Add unit tests for SemanticLayer.raise_for_access (all-datasources, perm-granted, perm-denied, no-perm-denied paths) to satisfy the 100%-coverage gate on superset/semantic_layers. - Update test_non_admin_cannot_set_system_default to log in as an Alpha user, since Theme was moved into GAMMA_READ_ONLY_MODEL_VIEWS: Gamma now gets rejected at the route-permission layer with FAB's generic message before reaching the admin-only check, while Alpha (which still has generic Theme write access) reaches the check and gets the specific message. Add a companion test asserting Gamma is blocked outright. - Mirror UpdateThemeCommand's admin carve-out in import_theme(): only reject overwriting the active system-default/dark theme via import for non-admins, so admins can still re-import the theme rendered for every user, same as they can update it directly. - Document the 403 response on the semantic-layer endpoints that gained a raise_for_access()/raise_for_editorship() check, per review feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nated-script XSS gap in SVG sanitizer - Add the missing 403 OpenAPI response to the semantic-view structure and delete endpoints, matching the raise_for_access/response_403 calls they already make. - Fix sanitize_svg_content to drop everything after an unterminated <script> opener instead of just the tag itself, since browsers treat an unterminated <script> as consuming the rest of the document. Strengthen the regression test to assert the payload text is gone too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
added
the
merge-if-green
| raise SemanticLayerNotFoundError() | ||
|
|
||
| try: | ||
| security_manager.raise_for_editorship(layer) |
There was a problem hiding this comment.
SemanticLayer has no editors relationship, and the create path does not assign an editor subject. With the default extra-editor resolver, this check therefore rejects every non-admin—even one granted the existing write permission—when they try to add a semantic view to a layer. Should this use the semantic-layer access/ownership policy (or persist editors) instead of generic editorship?
SemanticLayer and SemanticView have no `editors` relationship, so raise_for_editorship rejected every non-admin caller unconditionally, including a layer's own creator. Replace the raise_for_editorship calls in the semantic-layer create/update/delete commands with current_user_can_modify_object, a helper that falls back to comparing the resource's created_by against the current user when the resource has no editors to check. The helper is moved from commands/tag/utils.py (where it was added for tag-object writes) to the shared commands/utils.py so both the tag and semantic-layer commands can use it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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 #7d6154
Actionable Suggestions - 1
-
tests/unit_tests/commands/theme/test_import_themes.py - 1
- Missing admin dark-slot overwrite test · Line 95-120
Review Details
-
Files reviewed - 18 · Commit Range:
3f645b4..8d53c29- superset/commands/theme/import_themes.py
- superset/semantic_layers/api.py
- tests/integration_tests/themes/test_theme_api_permissions.py
- tests/unit_tests/commands/theme/test_import_themes.py
- tests/unit_tests/semantic_layers/models_test.py
- superset/utils/core.py
- tests/unit_tests/utils/test_core.py
- superset/commands/semantic_layer/create.py
- superset/commands/semantic_layer/delete.py
- superset/commands/semantic_layer/update.py
- superset/commands/tag/create.py
- superset/commands/tag/update.py
- superset/commands/tag/utils.py
- superset/commands/utils.py
- tests/unit_tests/commands/semantic_layer/create_test.py
- tests/unit_tests/commands/semantic_layer/delete_test.py
- tests/unit_tests/commands/semantic_layer/update_test.py
- tests/unit_tests/commands/test_utils.py
-
Files skipped - 0
-
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 an incremental AI Review. -
/review full- 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
Mirrors the existing is_system_default test for full branch coverage of the admin carve-out.
Code Review Agent Run #6178a7Actionable 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 |
|
The per-object access checks on the detail/views/runtime-schema endpoints and the editorship checks on update/delete look correct and fail-closed. Two items against the original scope before this is fully complete: (1) _serialize_layer still returns the fully decrypted configuration to any caller who passes the access check; since that blob holds the datasource connection settings, it would be worth masking the sensitive fields on serialization the way the Database API already does, rather than returning them in cleartext. (2) get_list, get, and runtime_schema do not carry the is_feature_enabled("SEMANTIC_LAYERS") guard that structure/views/delete do, so those routes stay reachable when the feature is off and the flag handling is inconsistent across the API. Worth adding for parity. |
4 participants
SUMMARY
CssTemplateis already treated.TESTING INSTRUCTIONS
pytest tests/unit_tests/semantic_layers/ tests/unit_tests/commands/semantic_layer/ tests/unit_tests/themes/ tests/unit_tests/commands/theme/ tests/unit_tests/utils/test_core.py— new/extended tests per change, each verified to fail pre-fix and pass post-fix.ADDITIONAL INFORMATION