Conversation
…ard.anchor extra.dashboard.anchor comes from the untyped marshmallow Dict `extra` field on a report/alert schedule, so an API caller can send any JSON value. When it is a non-string scalar (int, float, bool) or a JSON string that parses to a scalar, json.loads(anchor)/set(...) raises a raw TypeError that is not a subclass of json.JSONDecodeError, so it escapes _validate_report_extra and surfaces as an opaque 500 instead of the 422 every other malformed-input case here produces. Widen the except to (json.JSONDecodeError, TypeError), matching the existing precedent in execute.py's _get_url for the same field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #4e30ceActionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44404 +/- ##
==========================================
+ Coverage 80.28% 80.40% +0.11%
==========================================
Files 2928 2934 +6
Lines 174122 174991 +869
Branches 40404 40547 +143
==========================================
+ Hits 139790 140693 +903
+ Misses 31675 31633 -42
- Partials 2657 2665 +8
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:
|
|
@richardfogaca routing this one your way. You reviewed and approved #44204, which added the One behavioral change to ratify (PR is agent-assisted): a non-string You're carrying a few reviews already — glad to re-route if you'd rather pass. |
…ation A non-string anchor (list/dict) makes json.loads raise TypeError, and the except body then hashed the raw value via `anchor not in position_data` (and set.add), leaking a fresh, uncaught TypeError for the same bug class this PR targets. Guard on isinstance(anchor, str) so non-string anchors are recorded via their string form and surface a clean ValidationError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #736ab9Actionable 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 |
SUMMARY
_validate_report_extrainsuperset/commands/report/base.pyreadsextra.dashboard.anchorfrom a report/alert schedule'sextrafield. That field is an untyped marshmallowDict(superset/reports/schemas.py:extra = fields.Dict(dump_default=None)), so an API caller can send any JSON value foranchor, not just a string.The code does:
PROBLEM
json.loads()requires astr/bytes/bytearray. Whenanchoris a non-string scalar (int, float, bool),json.loads(anchor)raises a rawTypeError, which is not a subclass ofjson.JSONDecodeError(JSONDecodeErroris aValueError;TypeErroris unrelated). A related gap: whenanchoris a string that parses to a non-iterable scalar (e.g."42"→42),set(anchor_list)also raisesTypeError. Neither is caught byexcept json.JSONDecodeError.A further gap in the same bug class: for a non-empty
anchorthat is alistordict,json.loads(anchor)raisesTypeError, but theexceptbody'sanchor not in position_datamembership test (and the subsequentset.add) then raises a fresh, uncaughtTypeError: unhashable type: 'list', sinceinvalid_tab_idsis aset[str]and dict/set membership requires hashing the key.The uncaught
TypeErrorpropagates out of_validate_report_extra→validate()→CreateReportScheduleCommand.run()/UpdateReportScheduleCommand.run(). The REST API layer (superset/reports/api.py) only catchesReportScheduleInvalidError/ReportScheduleNotFoundError/ReportScheduleCreateFailedError, so this surfaces as an opaque 500 instead of the 422 validation response every other malformed-input case in this same method produces.FIX
Widen the
exceptto also catchTypeError:and guard the except body on
isinstance(anchor, str)so a non-string (unhashable) anchor is recorded via its string form instead of being hashed in the membership test /set.add:All failure modes then fall through to the existing branch and are collected as a
ValidationErroron theextrafield, yielding a proper 422. Behavior is unchanged for string/int/float/bool anchors (all hashable,isinstance(anchor, str)short-circuits correctly); only the previously-crashing list/dict case changes, turning into a cleanValidationErrorlike every other malformed-anchor case in this method.This is the same sibling-site-drift pattern as #42401 and the validation-time precedent already established in this same file for
position_json/json_metadata(thejson.JSONDecodeErrorhandling a few lines above). Note:superset/commands/report/execute.py's own anchor handling (_get_url) catches onlyjson.JSONDecodeErrorand has the identical unguarded gap for a non-string anchor — left as a follow-up, out of scope for this PR.TESTING INSTRUCTIONS
test_validate_report_extra_anchor_non_string_typeintests/unit_tests/commands/report/create_test.py("extra": {"dashboard": {"anchor": 42}}), asserting oneValidationErroron theextrafield. Confirmed it raisesTypeErroron pre-fix code and passes after the fix.test_validate_report_extra_anchor_non_string_unhashable_type("extra": {"dashboard": {"anchor": [1, 2]}}), asserting oneValidationErroron theextrafield. Confirmed it raisesTypeError: unhashable type: 'list'on the pre-fix code and passes after the fix.pytest tests/unit_tests/commands/report/create_test.py→ 15 passed.ruff check,ruff format --check, andmypyclean on the changed files.ADDITIONAL INFORMATION