fix(rls): preserve tables/roles on partial RLS rule updates#41294
Open
rusackas wants to merge 1 commit into
Open
fix(rls): preserve tables/roles on partial RLS rule updates#41294rusackas wants to merge 1 commit into
rusackas wants to merge 1 commit into
Conversation
UpdateRLSRuleCommand defaulted missing tables/roles to [] and
unconditionally wrote them back during validate(), so a partial PUT (e.g.
{"name": "new name"}) replaced the rule's table and role bindings with empty
lists — silently changing which rows the filter constrains.
Only resolve and overwrite the relationships that are actually present in the
request body, leaving omitted bindings untouched. Also mirror the create-path
invariant by adding Length(1) to the tables field in RLSPutSchema, so tables
cannot be set to an empty list on update.
Adds unit tests covering partial updates that omit tables/roles and updates
that touch only one of the two relationships.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Code Review Agent Run #377065Actionable 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 |
dosubot
Bot
added
api
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+59
to
+68
| if "tables" in self._properties: | ||
| tables = ( | ||
| db.session.query(SqlaTable) | ||
| .filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined] | ||
| .all() | ||
| ) | ||
| if len(tables) != len(self._tables): | ||
| raise DatasourceNotFoundValidationError() | ||
| raise_for_datasource_access(tables) | ||
| self._properties["tables"] = tables |
Contributor
There was a problem hiding this comment.
Suggestion: The datasource access check is now gated behind "tables" in self._properties, so a partial update that omits tables skips authorization against the rule's existing table bindings. This lets a caller update sensitive fields (like clause/roles) on an RLS rule tied to datasources they cannot access. Keep partial-update behavior, but enforce access on the effective table set: use submitted tables when present, otherwise validate against self._model.tables. [security]
Severity Level: Critical 🚨
- ❌ PUT /api/v1/rowlevelsecurity/<pk> skips table access check.
- ❌ Non-authorized users can modify RLS clause/roles bindings.
- ⚠️ Weakens dataset isolation enforced via row level security.Steps of Reproduction ✅
1. The RLS REST API update endpoint `PUT /api/v1/rowlevelsecurity/<pk>` is implemented in
`superset/row_level_security/api.py:181-241` as `RLSRestApi.put`, which loads the request
body with `RLSPutSchema` (`edit_model_schema.load(request.json)` at line 235) and then
calls `UpdateRLSRuleCommand(pk, item).run()` at line 240.
2. `RLSPutSchema` in `superset/row_level_security/schemas.py:147-187` defines `tables` as
an optional field (`required=False` at lines 167-172), so a payload like `{"clause":
"1=1"}` is valid and will be passed to `UpdateRLSRuleCommand` without a `"tables"` key.
3. `UpdateRLSRuleCommand.validate` in `superset/commands/security/update.py:49-68` loads
the existing rule via `RLSDAO.find_by_id` (lines 50-52) and then only performs the
datasource lookup and access check when `"tables" in self._properties` (line 59). If
`"tables"` is absent from the payload, the block at lines 59-68 (including
`raise_for_datasource_access(tables)` at line 67) is completely skipped, and
`self._model.tables` is never checked.
4. `raise_for_datasource_access` in `superset/commands/security/utils.py:29-42` enforces
that `security_manager.can_access_datasource(datasource=table)` is True for every
referenced table, otherwise raising `RLSDatasourceForbiddenError`, which `RLSRestApi.put`
would convert into HTTP 403 (handling at `superset/row_level_security/api.py:258-264`).
Because `UpdateRLSRuleCommand.validate` never calls `raise_for_datasource_access` when
`tables` is omitted, a caller who can hit `PUT /api/v1/rowlevelsecurity/<pk>` but for whom
`can_access_datasource` would return False for one or more of `self._model.tables` can
still successfully send a partial update payload without `"tables"` (e.g. `{"clause":
"new_clause", "roles": [...]}`), and the request will return 200 without any datasource
authorization being enforced against the rule's existing table bindings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/security/update.py
**Line:** 59:68
**Comment:**
*Security: The datasource access check is now gated behind `"tables" in self._properties`, so a partial update that omits `tables` skips authorization against the rule's existing table bindings. This lets a caller update sensitive fields (like `clause`/`roles`) on an RLS rule tied to datasources they cannot access. Keep partial-update behavior, but enforce access on the effective table set: use submitted tables when present, otherwise validate against `self._model.tables`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
The flagged security issue is correct. When performing a partial update that omits the To resolve this, you should ensure that even if Here is a concise fix for if "tables" in self._properties:
tables = (
db.session.query(SqlaTable)
.filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined]
.all()
)
if len(tables) != len(self._tables):
raise DatasourceNotFoundValidationError()
raise_for_datasource_access(tables)
self._properties["tables"] = tables
else:
# Always validate access to existing tables if not updating them
raise_for_datasource_access(self._model.tables)There are no other comments in the provided PR review data to address. superset/commands/security/update.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41294 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144953 +1
Branches 33433 33435 +2
==========================================
- Hits 93272 93266 -6
- Misses 49996 49999 +3
- Partials 1684 1688 +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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
UpdateRLSRuleCommandreadtables/rolesfrom the request body defaulting to[], and unconditionally wrote them back duringvalidate(). As a result, a partialPUTthat omitted those fields (e.g.{"name": "new name"}) replaced the rule's table and role bindings with empty lists — silently changing which rows the RLS filter constrains.This makes partial updates behave as expected:
validate()now only resolves and overwrites the relationships that are actually present in the request body. Omittedtables/rolesare left untouched.RLSPutSchema.tablesgainsLength(1), mirroringRLSPostSchema, sotablescannot be set to an empty list on update either.roleskeeps no minimum-length constraint, matching the create schema (base filters may legitimately specify zero excluded roles).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend command/schema behavior.
TESTING INSTRUCTIONS
Unit tests added in
tests/unit_tests/commands/security/rls_test.py:tables/rolesdoes not resolve or write those keys (existing bindings preserved).rolesdoes not resolve or overwritetables.Run:
pytest tests/unit_tests/commands/security/rls_test.pyADDITIONAL INFORMATION