fix(dataset): apply the import overwrite permission check to identity-matched datasets - #43058
fix(dataset): apply the import overwrite permission check to identity-matched datasets#43058sha174n wants to merge 1 commit into
Conversation
…-matched datasets `SqlaTable.import_from_dict` matches an existing row on any of the model's unique constraints, which include `(database_id, catalog, schema, table_name)` as well as `uuid`. A dataset import whose config carried a fresh UUID but a physical identity already held by an active dataset was therefore matched and updated on that identity without passing through the overwrite permission gate, which only ran on a UUID match. Resolve an active identity collision to the existing dataset before the gate runs (aligning the UUID so the subsequent import updates that row deterministically), so the same editor/admin overwrite check applies. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
authentication:access-control
Code Review Agent Run #d39f2fActionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| active_twin = ( | ||
| db.session.query(SqlaTable) | ||
| .filter_by( | ||
| database_id=config["database_id"], | ||
| catalog=config.get("catalog"), | ||
| schema=config.get("schema"), | ||
| table_name=config["table_name"], | ||
| ) | ||
| .first() |
There was a problem hiding this comment.
Suggestion: The active-twin probe uses a raw catalog=None equality, while import_from_dict omits nullable fields from its uniqueness predicate. If the incoming config has no catalog and the existing active dataset stores the database default catalog, this query misses the twin, but the later import lookup still matches it by database, schema, and table name. The import then updates the active dataset without reaching the overwrite permission check. Use the same null-aware/default-catalog identity predicate as the DAO or resolve the identity through the shared uniqueness helper. [security]
Severity Level: Critical 🚨
- ❌ Unauthorized imports can overwrite active datasets.
- ❌ Dataset SQL and metadata can be clobbered.
- ⚠️ Affects catalog-default database imports.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 261:269
**Comment:**
*Security: The active-twin probe uses a raw `catalog=None` equality, while `import_from_dict` omits nullable fields from its uniqueness predicate. If the incoming config has no catalog and the existing active dataset stores the database default catalog, this query misses the twin, but the later import lookup still matches it by database, schema, and table name. The import then updates the active dataset without reaching the overwrite permission check. Use the same null-aware/default-catalog identity predicate as the DAO or resolve the identity through the shared uniqueness helper.
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| config["uuid"] = str(active_twin.uuid) | ||
| existing = find_existing_for_import(SqlaTable, config["uuid"]) |
There was a problem hiding this comment.
Suggestion: The collision path rewrites the caller-owned config before the defensive copy later in the function. Consequently, callers retaining the uploaded configuration observe its fresh UUID replaced with the victim dataset's UUID; reusing or retrying that same dictionary can then address the victim by UUID and take a different overwrite path than the original import. Copy the input before resolving the collision, or keep the resolved UUID in a separate local import configuration. [stale reference]
Severity Level: Minor 🧹
- ⚠️ Reused import configs retain the victim UUID.
- ⚠️ Retries can select different UUID-based branches.
- ⚠️ Affects callers retaining bundle configuration dictionaries.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 272:273
**Comment:**
*Stale Reference: The collision path rewrites the caller-owned `config` before the defensive copy later in the function. Consequently, callers retaining the uploaded configuration observe its fresh UUID replaced with the victim dataset's UUID; reusing or retrying that same dictionary can then address the victim by UUID and take a different overwrite path than the original import. Copy the input before resolving the collision, or keep the resolved UUID in a separate local import configuration.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43058 +/- ##
==========================================
- Coverage 66.55% 66.54% -0.01%
==========================================
Files 2864 2864
Lines 161894 161900 +6
Branches 37305 37307 +2
==========================================
Hits 107742 107742
- Misses 52106 52110 +4
- Partials 2046 2048 +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:
|
1 participant
SqlaTable.import_from_dictmatches an existing row on any of the model's unique constraints, which include(database_id, catalog, schema, table_name)as well asuuid. A dataset import whose config carried a fresh UUID but a physical identity already held by an active dataset was therefore matched and updated on that identity without passing through the overwrite permission gate, which only ran on a UUID match.This resolves an active identity collision to the existing dataset before the gate runs (aligning the UUID so the subsequent import updates that row deterministically), so the same editor/admin overwrite check applies. Adds a regression test.
This mirrors the treatment already applied to the dashboard importer for slug collisions.