Repository navigation
fix(security): prevent scalar control string iteration in guest anti-tamper check (#43576) - #43697
FrancescoCastaldi wants to merge 25 commits into
Conversation
…ion in guest security check
…ed guest tamper check
…in guest security manager
…e filter tamper validation
…st SQL filter check
…wed_sql_from_query_context
… payload validation
…kroachDbEngineSpec
…herDuckEngineSpec
…tests for DynamoDBEngineSpec
Code Review Agent Run #f468fcActionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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}
|
|
||
| metadata = GSheetsEngineSpec.metadata | ||
| assert "Google Sheets" in metadata["description"] | ||
| assert metadata["logo"] == "google-sheets.png" |
There was a problem hiding this comment.
Suggestion: GSheets metadata currently advertises google-sheets.svg, so this assertion fails every time the metadata test runs. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Often
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_gsheets.py
**Line:** 1122:1122
**Comment:**
*Api Mismatch: GSheets metadata currently advertises `google-sheets.svg`, so this assertion fails every time the metadata test runs.
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
|
The flagged issue in the unit test is correct. The test assertion is failing because the GSheets metadata is returning I have checked the available PR comments, and there are no other comments to address. Would you like me to proceed with updating the test file? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43697 +/- ##
========================================
Coverage 82.36% 82.37%
========================================
Files 2997 2998 +1
Lines 185523 185713 +190
Branches 42938 42981 +43
========================================
+ Hits 152799 152974 +175
- Misses 29970 29981 +11
- Partials 2754 2758 +4
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #165dcbActionable 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 |
|
Fixed the missing \TimeGrain\ import in \ ests/unit_tests/db_engine_specs/test_firebolt.py\ and verified Google Sheets metadata assertions in commit 14c596b. CI checks are now running with the fix in place. |
Code Review Agent Run #32b24bActionable 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 |
github-actions
Bot
added
the
requires:rebase
…mper-and-specs # Conflicts: # tests/unit_tests/security/manager_test.py
|
Merged latest upstream master into branch, resolved merge conflict in \ ests/unit_tests/security/manager_test.py\ by integrating the upstream scalar control tests alongside the scalar columns/metrics test suite, and verified all lint checks pass cleanly. PR is now fully up to date and mergeable. |
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #d45178Actionable 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 |
github-actions
Bot
added
the
requires:rebase
|
Merged latest \master\ and resolved merge conflicts with recent MSSQL/security tests. Clean and ready for review. |
github-actions
Bot
added
requires:rebase
CodeAnt PR Risk: Low Risk
Assessed commit: |
github-actions
Bot
removed
the
requires:rebase
This branch has not been deployed
1 participant
Summary
This PR addresses Issue #43576 (Guest embedded dashboard payload rejection on charts storing single-value scalar controls such as
heatmap_v2) and strengthens security manager tamper checks and dialect test coverage across Superset.Problem Fixed (Issue #43576)
Embedded dashboard charts whose controls store a single scalar string value (e.g.
groupby: "division") rather than a list were previously iterated directly as iterables in_columns_metrics_modifiedand related helpers insuperset/security/manager.py. This caused Python to decompose the string character-by-character (e.g.{"d", "i", "v", "s", "o", "n"}), resulting in a false-positive rejection ("Guest user cannot modify chart payload").Key Changes
superset/security/manager.py):_ensure_listhelper to safely normalize scalar values (strings, numbers, dicts), sequences, and nulls._columns_metrics_modified,_stored_param_values,_native_filter_query_modified,_collect_stored_orderby_entries, and_sql_filters_modifiedto prevent character-level iteration bugs.tests/unit_tests/security/manager_test.py):heatmap_v2).tests/unit_tests/db_engine_specs/.Testing Instructions
pytest tests/unit_tests/security/manager_test.pypytest tests/unit_tests/db_engine_specs/