fix(security): reject unknown fields on guest-token RLS rules#41217
fix(security): reject unknown fields on guest-token RLS rules#41217rusackas wants to merge 2 commits into
Conversation
RlsRuleSchema inherited from PermissiveSchema, which silently drops unknown fields. A guest-token rls rule that used a mistyped or legacy scope key -- most commonly datasource instead of dataset -- therefore had that key stripped during deserialization, leaving a rule with no dataset. get_guest_rls_filters treats a rule with no dataset as global, so the clause was applied to every dataset the embedded resource can reach rather than the single dataset the caller intended. Make RlsRuleSchema strict (Meta.unknown = RAISE) so an unexpected field raises a ValidationError (HTTP 400) before a token is issued, instead of producing a silently-global rule. This aligns runtime validation with the documented RlsRule contract (dataset and clause). Valid dataset-scoped and global rules are unaffected; the rest of the guest-token payload remains permissive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
api
Code Review Agent Run #1e90bdActionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41217 +/- ##
==========================================
- Coverage 64.36% 64.36% -0.01%
==========================================
Files 2651 2651
Lines 144812 144814 +2
Branches 33417 33417
==========================================
- Hits 93208 93205 -3
- Misses 49935 49938 +3
- Partials 1669 1671 +2
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. Allowing Here is the implementation: from marshmallow import fields, validate
class RlsRuleSchema(Schema):
# ...
dataset = fields.Integer(validate=validate.Range(min=1))
clause = fields.String(required=True)Regarding other comments on this PR, there are no additional review comments in the provided context. If you have more comments to address, please provide them, and I will be happy to help validate and implement fixes for them. superset/security/api.py |
A bare Integer field allowed dataset=0 (and false, coerced to 0) which reads as falsy in get_guest_rls_filters and silently widens a scoped rule to every dataset. Constrain dataset to strict, positive integers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #9eea2cActionable 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 |
2 participants
SUMMARY
RlsRuleSchema(used to validate therlsrules inPOST /api/v1/security/guest_token/) inherits fromPermissiveSchema, whoseMeta.unknown = EXCLUDEsilently drops any field it doesn't recognize. So a guest-token RLS rule that uses a mistyped or legacy scope key — most commonlydatasource: { id, type }(a leftover from older payloads) instead of the documenteddataset: <int>— has that key stripped during deserialization. The resulting rule has nodataset.SecurityManager.get_guest_rls_filters()treats a rule with nodatasetas global (if not rule.get("dataset"),superset/security/manager.py), so the clause ends up applied to every dataset the embedded resource can reach instead of the single dataset the caller intended. The visible symptom is confusing query errors (e.g.column "…" does not existwhen the clause lands on an unrelated dataset), and the rule's effective scope silently differs from what the integrator configured.This makes
RlsRuleSchemastrict (Meta.unknown = RAISE) so an unexpected field raises aValidationError(HTTP 400) that names the offending field, before a token is ever issued — instead of quietly producing a global rule. This:RlsRulecontract (the OpenAPI spec advertises onlydatasetandclause).{"dataset": 41, "clause": "…"}) and intentionally global ({"clause": "…"}) rules still load.GuestTokenCreateSchema,UserSchema,ResourceSchema) permissive, so integrator-specific extra fields elsewhere are unaffected.Framed as input-validation hardening / DX, not a privilege-escalation fix: the
guest_tokenendpoint is called by a trusted integrator holdingcan_grant_guest_token, and RLS clauses are additive restrictions — so the impact is incorrect/over-broad rule scoping and confusing errors, not a Superset access-control bypass.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — API-only change.
TESTING INSTRUCTIONS
Unit tests added in
tests/unit_tests/security/api_test.py:test_rls_rule_schema_accepts_dataset_scoped_rule—{"dataset": 41, "clause": "…"}still loads.test_rls_rule_schema_accepts_global_rule—{"clause": "…"}(nodataset) still loads.test_rls_rule_schema_rejects_unknown_scope_key—{"datasource": {…}, "clause": "…"}now raisesValidationErrornamingdatasource.test_rls_rule_schema_rejects_unknown_fields— any other unknown field is rejected.test_rls_rule_schema_requires_clause—clauseremains required.Manual:
POST /api/v1/security/guest_token/with anrlsentry usingdatasourceinstead ofdatasetnow returns HTTP 400 pointing at thedatasourcefield, instead of issuing a token whose rule is silently global.ADDITIONAL INFORMATION
This tightens an API contract: integrators that were sending extra fields inside RLS rules (previously dropped) will now get a 400. Documented in
UPDATING.md.🤖 Generated with Claude Code