fix(deck.gl): strip all JS-executed form_data keys when JavaScript controls are disabled#40602
Conversation
Code Review Agent Run #498998Actionable 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 |
dosubot
Bot
added
change:backend
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40602 +/- ##
=======================================
Coverage 64.03% 64.03%
=======================================
Files 2663 2663
Lines 143619 143620 +1
Branches 33030 33030
=======================================
+ Hits 91973 91974 +1
Misses 50044 50044
Partials 1602 1602
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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a feature-flag gating gap for deck.gl chart plugins by ensuring all form_data fields that are executed via sandboxedEval are stripped server-side when ENABLE_JAVASCRIPT_CONTROLS is disabled (the default), preventing unintended client-side execution.
Changes:
- Introduces
JS_CONTROL_FORM_DATA_KEYSinsuperset/views/utils.pyand uses it to populateREJECTED_FORM_DATA_KEYSwhen JavaScript controls are disabled. - Expands the rejected/stripped key set to include Geojson layer’s
label_javascript_config_generatorandicon_javascript_config_generator. - Adds unit tests verifying (a) the rejected list covers all JS-executed keys and (b)
get_form_datastrips them while preserving non-JS keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/views/utils.py |
Centralizes the set of JS-evaluated form_data keys and strips them when ENABLE_JAVASCRIPT_CONTROLS is off. |
tests/unit_tests/views/test_utils.py |
Adds regression tests to ensure all JS-executed keys are rejected/stripped under the default (disabled) feature flag. |
sha174n
added
the
merge-if-green
…re off The deck.gl charts execute several form_data fields as JavaScript at render time via the frontend sandboxedEval helper. The backend strips these keys from form_data when ENABLE_JAVASCRIPT_CONTROLS is disabled (the default), but the strip list only covered js_tooltip, js_onclick_href and js_data_mutator. The Geojson layer also evaluates label_javascript_config_generator and icon_javascript_config_generator, which were not in the strip list, so those two fields were preserved in form_data even with the flag disabled. Add both keys to the strip list and centralize the JS-executed keys in a named constant kept in sync with the sandboxedEval call sites. Add unit tests asserting every JS-executed key is rejected when the flag is off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/deckgl-js-control-form-data-stripping
branch
from
c7178f7 to
7761ab2
Compare
github-project-automation
Bot
moved this from Needs Review
to Approved and/or Merged
in Superset Review Help Wanted
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
The deck.gl chart plugins execute several
form_datafields as JavaScript at render time through the frontendsandboxedEvalhelper. To keep this behavior gated behind theENABLE_JAVASCRIPT_CONTROLSfeature flag (which defaults to off), the backend strips those keys fromform_datainget_form_datawhen the flag is disabled.The strip list (
REJECTED_FORM_DATA_KEYSinsuperset/views/utils.py) only covered three of the keys:However, the Geojson layer also evaluates two more fields via
sandboxedEval:label_javascript_config_generatoricon_javascript_config_generator(see
plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.tsxand the legacylegacy-preset-chart-deckglequivalent). Because these two were not in the strip list, they were retained inform_dataand executed client-side even whenENABLE_JAVASCRIPT_CONTROLSis disabled — bypassing the intended gate for those fields.This change:
JS_CONTROL_FORM_DATA_KEYSconstant, with a comment noting it must stay in sync with thesandboxedEval(fd.<key>)call sites in the deck.gl plugins.No behavior change when
ENABLE_JAVASCRIPT_CONTROLSis enabled.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend form_data handling.
TESTING INSTRUCTIONS
test_rejected_form_data_keys_cover_all_js_control_keys— every JS-executed key is in the strip list when the flag is off.test_get_form_data_strips_js_control_keys—get_form_datadrops all of them and preserves non-JS keys.ADDITIONAL INFORMATION
🤖 Generated with Claude Code