codeant-ai-for-open-source[bot] commented on code in PR #38452: URL: https://github.com/apache/superset/pull/38452#discussion_r2990528126
########## tests/unit_tests/commands/security/rls_validation_test.py: ########## @@ -0,0 +1,234 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.security.create import CreateRLSRuleCommand +from superset.commands.security.update import UpdateRLSRuleCommand +from superset.exceptions import SupersetSecurityException + + +def test_create_rls_rule_with_subquery_fails_when_flag_off(): + data = { + "name": "malicious", + "clause": "1=1 OR EXISTS (SELECT 1 FROM users)", + "tables": [1], + "roles": [1], + } + mock_table = MagicMock() + mock_table.database = MagicMock() + mock_table.database.backend = "postgresql" + mock_table.database_id = 1 + mock_table.catalog = None + mock_table.schema = "public" + + with ( + patch("superset.commands.security.create.db.session.query") as mq, + patch("superset.models.helpers.is_feature_enabled", return_value=False), + ): + mq.return_value.filter.return_value.all.return_value = [mock_table] + command = CreateRLSRuleCommand(data) + with pytest.raises(SupersetSecurityException) as ex: + command.validate() + assert "Custom SQL fields cannot contain sub-queries" in str(ex.value) + + +def test_create_rls_rule_with_subquery_passes_when_flag_on(): + from superset.sql.parse import RLSMethod + + data = { + "name": "complex_but_allowed", + "clause": "1=1 OR EXISTS (SELECT 1 FROM users)", + "tables": [1], + "roles": [1], + } + mock_table = MagicMock() + mock_table.database = MagicMock() + mock_table.database.backend = "postgresql" + mock_table.database.db_engine_spec.get_rls_method.return_value = ( + RLSMethod.AS_PREDICATE + ) + mock_table.database_id = 1 + mock_table.catalog = None + mock_table.schema = "public" + + with ( + patch("superset.commands.security.create.db.session.query") as mq, + patch("superset.models.helpers.is_feature_enabled", return_value=True), + patch("superset.utils.rls.apply_rls") as m_apply, + ): + mq.return_value.filter.return_value.all.return_value = [mock_table] + command = CreateRLSRuleCommand(data) + # Should not raise + command.validate() + # Verify that apply_rls was called to protect the subquery + assert m_apply.called + + +def test_update_rls_rule_with_subquery_fails_when_flag_off(): + data = { + "name": "malicious_update", + "clause": "1=1 OR EXISTS (SELECT 1 FROM users)", + "tables": [1], + "roles": [1], + } + mock_table = MagicMock() + mock_table.database = MagicMock() + mock_table.database.backend = "postgresql" + mock_table.catalog = None + mock_table.schema = "public" + + mock_model = MagicMock() + + with ( + patch("superset.commands.security.update.RLSDAO.find_by_id") as mfind, + patch("superset.commands.security.update.db.session.query") as mq, + patch("superset.models.helpers.is_feature_enabled", return_value=False), + ): + mfind.return_value = mock_model + mq.return_value.filter.return_value.all.return_value = [mock_table] + + command = UpdateRLSRuleCommand(1, data) + with pytest.raises(SupersetSecurityException) as ex: + command.validate() + assert "Custom SQL fields cannot contain sub-queries" in str(ex.value) + + +def test_validate_rls_simple_clause_passes(): + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + # Should not raise for simple equality or basic WHERE fragments + # This addresses the reviewer concern about false rejections. + validate_adhoc_subquery( + "client_id = 9", mock_db, None, "public", "postgresql", is_predicate=True + ) + validate_adhoc_subquery( + "organization_id IS NOT NULL", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + + +def test_validate_rls_jinja_passes(): + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + # Jinja templates should not break the parser + validate_adhoc_subquery( + "client_id = {{ current_user_id() }}", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + validate_adhoc_subquery( + "dept IN ({{ \"'IT', 'HR'\" }})", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + + +def test_validate_rls_jinja_with_subquery_fails(): + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + # Malicious subquery WITH Jinja should still be blocked + with pytest.raises(SupersetSecurityException): + validate_adhoc_subquery( + "1=1 OR EXISTS (SELECT 1 FROM users) -- {{ ignore_me }}", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + + +def test_validate_rls_set_operations_fail(): + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + # UNION/EXCEPT/INTERSECT should be treated as subqueries/complex SQL + with pytest.raises(SupersetSecurityException): + validate_adhoc_subquery( + "id IN (SELECT id FROM table1 UNION SELECT id FROM table2)", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + + +def test_validate_predicate_rejects_set_operations(): + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + with pytest.raises(SupersetSecurityException) as ex: + validate_adhoc_subquery( + "status = 'active' UNION SELECT id FROM other_table", + mock_db, + None, + "public", + "postgresql", + is_predicate=True, + ) + assert "set operations are not allowed" in str(ex.value) + + +def test_validate_adhoc_subquery_preserves_jinja_on_non_predicate(): + """Non-predicate callers receive the original SQL unchanged when RLS is applied.""" + from unittest.mock import patch + + from superset.models.helpers import validate_adhoc_subquery + + mock_db = MagicMock() + sql = "SUM(CASE WHEN {{ filter_col }} = 1 THEN (SELECT amount FROM t2) END)" + + with ( + patch("superset.models.helpers.is_feature_enabled", return_value=True), + patch("superset.utils.rls.apply_rls"), + ): + result = validate_adhoc_subquery(sql, mock_db, None, "public", "postgresql") + assert "{{" in result Review Comment: **Suggestion:** The test claims to verify behavior when RLS is applied, but it never asserts that `apply_rls` was actually invoked. If parsing/regression skips RLS application, the current assertions can still pass, creating a false positive. Mock `apply_rls` with a concrete return value and assert the call. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ RLS-application regressions may slip through unit tests. - ⚠️ Security-path coverage weaker than test name implies. ``` </details> ```suggestion patch("superset.utils.rls.apply_rls", return_value=True) as m_apply, ): result = validate_adhoc_subquery(sql, mock_db, None, "public", "postgresql") assert m_apply.called ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `test_validate_adhoc_subquery_preserves_jinja_on_non_predicate` at `tests/unit_tests/commands/security/rls_validation_test.py:203`. 2. In `validate_adhoc_subquery` (`superset/models/helpers.py:67-93`), `apply_rls` is called only when `parsed_statement.has_subquery()` is true. 3. If `has_subquery()` behavior regresses in `superset/sql/parse.py:928-952` and returns false, control goes to `helpers.py:120` returning original SQL unchanged. 4. Current assertions (`"{{" in result`, `"__jinja__" not in result`) at `rls_validation_test.py:218-219` still pass even though `apply_rls` path was skipped. 5. Therefore this test does not currently prove RLS application happened; adding `assert m_apply.called` closes that coverage gap. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/commands/security/rls_validation_test.py **Line:** 215:217 **Comment:** *Logic Error: The test claims to verify behavior when RLS is applied, but it never asserts that `apply_rls` was actually invoked. If parsing/regression skips RLS application, the current assertions can still pass, creating a false positive. Mock `apply_rls` with a concrete return value and assert the call. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=c12c879f7cd353271840a41767b75fb6022040873f9bdb6554b74c220ea8f6f0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=c12c879f7cd353271840a41767b75fb6022040873f9bdb6554b74c220ea8f6f0&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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
