fix(semantic_layers): mask write-only config fields, gate flag-off endpoints - #43474
fix(semantic_layers): mask write-only config fields, gate flag-off endpoints#43474rusackas wants to merge 4 commits into
Conversation
…dpoints _serialize_layer now redacts configuration properties a connector's get_configuration_schema marks writeOnly, mirroring PASSWORD_MASK usage elsewhere. get_list, get, and runtime_schema now carry the same SEMANTIC_LAYERS feature-flag guard already present on structure/views/delete.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The security concern is valid. In the current implementation of To resolve this, the function should fail closed by raising an exception or returning an empty/masked configuration when the schema cannot be retrieved. Here is a corrected implementation: try:
schema = cls.get_configuration_schema()
except Exception:
# Fail closed: return an empty configuration or raise an error
return {}I have checked the available PR comments, and there are no other pending review comments to address. Would you like me to proceed with any other changes? superset/semantic_layers/api.py |
![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 #124a5d
Actionable Suggestions - 1
-
superset/semantic_layers/api.py - 1
- Duplicate authorization check pattern · Line 714-714
Additional Suggestions - 2
-
tests/unit_tests/semantic_layers/api_test.py - 1
-
Missing test coverage for connections · Line 2653-2682The `connections()` endpoint at api.py:1024 also has `is_feature_enabled("SEMANTIC_LAYERS")` guard that returns 404 when disabled. No corresponding test exists for this method.
-
-
superset/semantic_layers/api.py - 1
-
Masking bypass for empty-string write-only values · Line 112-112The condition `and value` in the dict comprehension on line 112 silently skips masking for empty-string secret fields (e.g., `"password": ""`). While this matches the unit test at `api_test.py:1187`, it creates a subtle data-loss path: an empty string in the request body will be preserved in the response, leaking information about which fields are write-only. The comment `leaves an unset write-only field alone` implies a deliberate design choice, but the resulting response `{password: ""}` on a field marked `writeOnly` is indistinguishable from a typo or misconfiguration.
-
Review Details
-
Files reviewed - 2 · Commit Range:
f67f6d8..f67f6d8- superset/semantic_layers/api.py
- tests/unit_tests/semantic_layers/api_test.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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43474 +/- ##
==========================================
+ Coverage 78.90% 78.97% +0.07%
==========================================
Files 2878 2878
Lines 165031 165555 +524
Branches 38135 38252 +117
==========================================
+ Hits 130218 130748 +530
+ Misses 32363 32321 -42
- Partials 2450 2486 +36
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:
|
… missing coverage test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #f8c9a6Actionable 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 |
Address two review findings: a missing/unregistered connector class made _mask_configuration a no-op instead of failing closed like the schema-load-error path, and UpdateSemanticLayerCommand wrote a round-tripped PASSWORD_MASK sentinel straight back into storage, overwriting the real stored credential on a name-only edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #ebc696Actionable 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 |
…t just write-only ones Once a GET fell back to masking every value (schema unavailable at read time), the update path only restored write-only keys, so a name-only save could persist the literal mask into non-secret fields like `account` once the schema became available again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #59ae53Actionable Suggestions - 0Additional Suggestions - 1
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 |
2 participants
SUMMARY
Two follow-ups on the semantic-layer/theme access-check work from #43389, for consistency:
_serialize_layer'sconfigurationblob is now passed through a masking step before it's returned. A connector publishes its configuration shape viaget_configuration_schema; any property marked"writeOnly": true(the standard JSON Schema way of flagging a field that's set but never echoed back, e.g. a password or API key) gets replaced with the samePASSWORD_MASKused elsewhere in the codebase, rather than returned as-is.get_list,get, andruntime_schemanow carry the sameSEMANTIC_LAYERSfeature-flag guard thatstructure,views, anddeletealready have, so the flag gating is consistent across the whole API surface.Behavior is unchanged for connectors that don't mark any config field
writeOnly.TESTING INSTRUCTIONS
pytest tests/unit_tests/semantic_layers/— new unit tests for_mask_configuration(redacts marked fields, leaves unset/falsy values alone, no-ops when there's no registered connector or the schema can't be loaded) and for the three new feature-flag guards (both through the client and via direct unwrapped calls, matching the existing pattern for the other guarded endpoints).ADDITIONAL INFORMATION