codeant-ai-for-open-source[bot] commented on code in PR #42819: URL: https://github.com/apache/superset/pull/42819#discussion_r3726354595
########## superset/daos/security.py: ########## @@ -15,9 +15,27 @@ # specific language governing permissions and limitations # under the License. +from typing import Optional + from superset.connectors.sqla.models import RowLevelSecurityFilter from superset.daos.base import BaseDAO +from superset.extensions import db class RLSDAO(BaseDAO[RowLevelSecurityFilter]): - pass + @classmethod + def validate_uniqueness(cls, name: str, rule_id: Optional[int] = None) -> bool: + """ + Validate that the RLS rule name is unique. + + :param name: RLS rule name + :param rule_id: id of the rule being updated, excluded from the check so + that saving a rule without renaming it is not treated as a collision + :return: True if the name is unique, False otherwise + """ + query = db.session.query(RowLevelSecurityFilter).filter( + RowLevelSecurityFilter.name == name + ) + if rule_id is not None: + query = query.filter(RowLevelSecurityFilter.id != rule_id) + return not db.session.query(query.exists()).scalar() Review Comment: **Suggestion:** The existence query is only a preflight check and is not atomic with the subsequent insert or update. Two concurrent requests can both observe the name as available, after which one write still raises the database unique-constraint error and the API returns the generic opaque SQLAlchemy message instead of the promised descriptive field error. Catch the unique-constraint violation and translate it to the same validation response, while retaining the database constraint as the authoritative guard. [race condition] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Concurrent RLS creation can return opaque `422` errors. - โ ๏ธ Concurrent renames can lose the descriptive validation message. - โ No duplicate database rows are created due to the constraint. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=696ecfba81d24312917dc4eb4dc8e8ec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=696ecfba81d24312917dc4eb4dc8e8ec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/daos/security.py **Line:** 36:41 **Comment:** *Race Condition: The existence query is only a preflight check and is not atomic with the subsequent insert or update. Two concurrent requests can both observe the name as available, after which one write still raises the database unique-constraint error and the API returns the generic opaque SQLAlchemy message instead of the promised descriptive field error. Catch the unique-constraint violation and translate it to the same validation response, while retaining the database constraint as the authoritative guard. 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%2F42819&comment_hash=375f3ef581083901a82faa34e6c18c586d1234be5f50090bfc3833c2dc298c72&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42819&comment_hash=375f3ef581083901a82faa34e6c18c586d1234be5f50090bfc3833c2dc298c72&reaction=dislike'>๐</a> ########## superset/commands/security/create.py: ########## @@ -46,6 +47,12 @@ def run(self) -> Any: return RLSDAO.create(attributes=self._properties) def validate(self) -> None: + name = self._properties.get("name") + if name and not RLSDAO.validate_uniqueness(name): + raise ValidationError( + {"name": [_("A rule with this name already exists.")]} + ) Review Comment: **Suggestion:** The duplicate-name check runs before datasource existence and access validation, so a caller who can invoke RLS creation but lacks access to the submitted datasource receives a duplicate-name response for an existing name and a forbidden response for a nonexistent name. This exposes global rule-name existence and changes the expected authorization response; perform datasource authorization before revealing whether the name is already used. The update command has the same ordering issue. [security] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ RLS users can enumerate globally used rule names. - โ ๏ธ Unauthorized datasource requests reveal duplicate-name status. - โ ๏ธ Update requests expose the same name-existence distinction. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=791531d6a26449959550646407d6f418&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=791531d6a26449959550646407d6f418&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/security/create.py **Line:** 50:54 **Comment:** *Security: The duplicate-name check runs before datasource existence and access validation, so a caller who can invoke RLS creation but lacks access to the submitted datasource receives a duplicate-name response for an existing name and a forbidden response for a nonexistent name. This exposes global rule-name existence and changes the expected authorization response; perform datasource authorization before revealing whether the name is already used. The update command has the same ordering issue. 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%2F42819&comment_hash=65d7c70ead9e2a951c6251cf9d6422f39443286c9c97145c287c4a27600e198d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42819&comment_hash=65d7c70ead9e2a951c6251cf9d6422f39443286c9c97145c287c4a27600e198d&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]
