fix(SafeMarkdown): let htmlSchemaOverrides replace matching default attribute rules#42202
fix(SafeMarkdown): let htmlSchemaOverrides replace matching default attribute rules#42202rusackas wants to merge 2 commits into
Conversation
…ttribute rules hast-util-sanitize's findDefinition returns only the first attribute definition it finds for a given property name, so the previous merge customizer (which concatenated arrays) left the default schema's restrictive tuples (e.g. `li: [['className', 'task-list-item']]`) in charge even after an operator supplied `htmlSchemaOverrides`. Because a failed allowlist check returns `[]` rather than `undefined`, hast-util-sanitize's own `'*'` wildcard fallback never kicked in either, so classes on list elements were silently stripped down to empty strings regardless of HTML_SANITIZATION_SCHEMA_EXTENSIONS. getOverrideHtmlSchema now merges each tag's attribute definitions by property name: an override definition replaces the default definition for the same property instead of being appended alongside it, so operator-supplied overrides for li/ul/ol (or '*') actually take effect. Fixes #34191
Code Review Agent Run #76f418Actionable Suggestions - 0Additional Suggestions - 1
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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42202 +/- ##
==========================================
- Coverage 65.08% 65.08% -0.01%
==========================================
Files 2758 2758
Lines 155229 155242 +13
Branches 35572 35575 +3
==========================================
+ Hits 101026 101033 +7
- Misses 52288 52294 +6
Partials 1915 1915
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:
|
…isleading test name Fall back to the default attribute definitions for a tag instead of throwing when htmlSchemaOverrides.attributes.<tag> isn't an array (operator-supplied runtime config isn't guaranteed to match the expected shape). Also fixes a test title that claimed the opposite of what it asserts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #284592Actionable 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 |
1 participant
SUMMARY
getOverrideHtmlSchemainSafeMarkdown.tsxmerges an operator'shtmlSchemaOverrides/HTML_SANITIZATION_SCHEMA_EXTENSIONSintorehype-sanitize's default schema using amergeWithcustomizer that concatenates attribute-definition arrays. hast-util-sanitize'sfindDefinitiononly ever returns the first definition it finds for a given property name, and the default schema already declares restrictive tuples for some tags, e.g.:So concatenating an operator's override after those defaults never actually widened anything — the restrictive default tuple was always found first. Worse, since a failed allowlist check on
classNamereturns[](an empty array) rather thanundefined, hast-util-sanitize's own'*'wildcard fallback never kicked in either. The net effect (see #34191): any CSS class set on<li>/<ul>/<ol>in a Handlebars/Markdown chart gets silently stripped to an emptyclass="", even withHTML_SANITIZATION_SCHEMA_EXTENSIONSconfigured to allow it.This PR changes the merge so that, within
attributes, an override definition for a property replaces the default definition for that same property (deduped by property name) instead of just being appended alongside it. Other schema arrays (tagNames,protocols.*, etc.) keep the original concat behavior.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — this is a schema-merge logic fix with no visual changes on its own; see #34191 for screenshots of the bug (classes on
<li>/<ul>being stripped toclass="").TESTING INSTRUCTIONS
Added regression tests in
superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx(getOverrideHtmlSchemadescribe block) that:htmlSchemaOverrides: { attributes: { li: ['className'] } }, the merged schema'slidefinition must be replaced by the override, not have the restrictive default tuple win.'*'wildcard case: withhtmlSchemaOverrides: { attributes: { '*': ['className'] } }, the wildcard override is present, and (documented for clarity) anli-specific default still wins unlessliitself is overridden — sincefindDefinitionchecks the tag-specific list before'*'.classNamerestriction onli/ul/olis unchanged when no override is supplied.Manually:
HTML_SANITIZATION_SCHEMA_EXTENSIONS = {"attributes": {"li": ["className"]}}(or"*": ["className"]plus a per-tag override) insuperset_config.py.<li class="my-class">element.<li>keepsclass="my-class"instead of being stripped toclass="".ADDITIONAL INFORMATION