You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by "verdier (via GitHub)" <gi...@apache.org> on 2026/10/12 12:14:43 UTC

[PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

verdier opened a new pull request, #45214:
URL: https://github.com/apache/superset/pull/45214

   ### SUMMARY
   #40650 enforces datasource access in the RLS rule commands, and #41294 extends the check to partial updates that omit `tables`. When the payload does carry `tables`, though, `UpdateRLSRuleCommand` only checks the submitted tables. A caller with access to dataset B but not to dataset A can therefore rebind a rule from A to B, which lifts the filter from A.
   
   The update now always checks the rule's current tables, then the submitted ones when `tables` is present.
   
   ### TESTING INSTRUCTIONS
   `pytest tests/unit_tests/commands/security/rls_test.py`
   
   The new test `test_update_rls_rule_forbidden_when_retargeting_away_from_denied_tables` fails on master and passes with this change.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   Affects unreleased code only: #40650 is not in 6.1.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   
   https://claude.ai/code/session_01Muk4ptfk2ka1wrxivBHH7U
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #45214:
URL: https://github.com/apache/superset/pull/45214#discussion_r4247945248


##########
superset/commands/security/update.py:
##########
@@ -69,6 +69,11 @@ def validate(self) -> None:
         # Datasource access is validated before revealing whether the
         # requested name is already in use, so an unauthorized caller can't
         # use the duplicate-name response to enumerate rule names.
+        # Any update mutates the rule, including one that rebinds it to other
+        # tables, which lifts the filter from the tables it leaves. The caller
+        # must therefore access the rule's current tables, and the new ones
+        # when ``tables`` is present.
+        raise_for_datasource_access(self._model.tables)

Review Comment:
   **Suggestion:** A concurrent rebind after this check lets a caller authorized only for the old table modify the rule now bound to an inaccessible table; this transaction takes no lock.
   
   **Assessment:** 🔴 `Critical` · 🔁 `Occurrence: Rarely` · 🏷️ `Race condition`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5882028967cb4c0a821671e029cf0890&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5882028967cb4c0a821671e029cf0890&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/commands/security/update.py
   **Line:** 76:76
   **Comment:**
   	*Race Condition: A concurrent rebind after this check lets a caller authorized only for the old table modify the rule now bound to an inaccessible table; this transaction takes no lock.
   
   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
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45214&comment_hash=efcccf3e2fca211729fec6d13f3002ef2cb05432a503a549ede8778ecd509e95&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45214&comment_hash=efcccf3e2fca211729fec6d13f3002ef2cb05432a503a549ede8778ecd509e95&reaction=dislike'>👎</a>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

Posted by "verdier (via GitHub)" <gi...@apache.org>.

verdier commented on code in PR #45214:
URL: https://github.com/apache/superset/pull/45214#discussion_r4248569337


##########
superset/commands/security/update.py:
##########
@@ -69,6 +69,11 @@ def validate(self) -> None:
         # Datasource access is validated before revealing whether the
         # requested name is already in use, so an unauthorized caller can't
         # use the duplicate-name response to enumerate rule names.
+        # Any update mutates the rule, including one that rebinds it to other
+        # tables, which lifts the filter from the tables it leaves. The caller
+        # must therefore access the rule's current tables, and the new ones
+        # when ``tables`` is present.
+        raise_for_datasource_access(self._model.tables)

Review Comment:
   This check-then-write window exists in every Superset command (RLS create/delete and the partial-update branch from #41294 included); none of them lock. Exploiting it needs a privileged user to rebind the rule within the same few milliseconds, which the caller cannot trigger. Locking only here would make this command the odd one out, and it does nothing on SQLite. This PR closes the deterministic rebind bypass and opens nothing new, so I'd keep it focused.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on PR #45214:
URL: https://github.com/apache/superset/pull/45214#issuecomment-6124184707

   ## CodeAnt PR Risk: High Risk
   
   - The PR should not merge yet because concurrent table rebinding can make the datasource access check stale.
   - The update does not lock or revalidate table bindings before applying other fields, allowing a caller to change a rule after it is rebound to an inaccessible datasource.
   
   **Assessed commit:** `25f22595173e`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.

codecov[bot] commented on PR #45214:
URL: https://github.com/apache/superset/pull/45214#issuecomment-6124399739

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45214?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 65.30%. Comparing base ([`f4fe4e1`](https://app.codecov.io/gh/apache/superset/commit/f4fe4e13c47d204dffca5e9408e388811d220f9f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`25f2259`](https://app.codecov.io/gh/apache/superset/commit/25f22595173ea6cbbc50c0110cebcf0693f97ddd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on master.
   > :exclamation:  There is a different number of reports uploaded between BASE (f4fe4e1) and HEAD (25f2259). Click for more details.
   > 
   > <details><summary>HEAD has 4 uploads less than BASE</summary>
   >
   >| Flag | BASE (f4fe4e1) | HEAD (25f2259) |
   >|------|------|------|
   >|python|7|3|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #45214       +/-   ##
   ===========================================
   - Coverage   82.96%   65.30%   -17.67%     
   ===========================================
     Files        3022     3022               
     Lines      193912   193912               
     Branches    45137    45137               
   ===========================================
   - Hits       160882   126625    -34257     
   - Misses      29958    64456    +34498     
   + Partials     3072     2831      -241     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.20% <0.00%> (ø)` | |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.59% <100.00%> (ø)` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.02% <0.00%> (ø)` | |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.68% <100.00%> (-33.00%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/45214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45214?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(rls): check the rule's current tables when an update rebinds it [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #45214:
URL: https://github.com/apache/superset/pull/45214#discussion_r4248575092


##########
superset/commands/security/update.py:
##########
@@ -69,6 +69,11 @@ def validate(self) -> None:
         # Datasource access is validated before revealing whether the
         # requested name is already in use, so an unauthorized caller can't
         # use the duplicate-name response to enumerate rule names.
+        # Any update mutates the rule, including one that rebinds it to other
+        # tables, which lifts the filter from the tables it leaves. The caller
+        # must therefore access the rule's current tables, and the new ones
+        # when ``tables`` is present.
+        raise_for_datasource_access(self._model.tables)

Review Comment:
   ✅ **Customized review instruction saved!**
   
   **Instruction:**
   > Do not flag the existing check-then-write race in this update command as a new regression; recommend locking only when it can be addressed consistently across commands and supported backends.
   
   **Applied to:**
     - `superset/commands/security/update.py`
   
   ---
   💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org