codeant-ai-for-open-source[bot] commented on code in PR #42202:
URL: https://github.com/apache/superset/pull/42202#discussion_r3608606461
##########
superset-frontend/packages/superset-ui-core/src/components/SafeMarkdown/SafeMarkdown.tsx:
##########
@@ -78,19 +78,69 @@ export function transformLinkUri(uri: string): string {
return DANGEROUS_LINK_PROTOCOLS.includes(scheme) ? '' : url;
}
+// A hast-util-sanitize attribute definition is either a bare property name
+// (any value allowed) or a tuple of `[propertyName, ...allowedValues]` (only
+// the listed values allowed). See hast-util-sanitize's `PropertyDefinition`.
+type AttributeDefinition = string | readonly [string, ...unknown[]];
+
+function getAttributeDefinitionKey(
+ definition: AttributeDefinition,
+): string | undefined {
+ return typeof definition === 'string' ? definition : definition[0];
+}
+
+/**
+ * Merge an operator-supplied list of attribute definitions for a tag (or the
+ * `'*'` wildcard) with the corresponding default definitions.
+ *
+ * hast-util-sanitize's `findDefinition` returns only the FIRST definition it
+ * finds for a given property name, so a naive concat leaves whichever list
+ * happens to declare that property first in charge. The default schema
+ * already declares restrictive tuples for some properties (e.g.
+ * `li: [['className', 'task-list-item']]`), so appending an operator's
+ * override after it never took effect. Because a failed allowlist check
+ * returns `[]` rather than `undefined`, hast-util-sanitize's own `'*'`
+ * fallback never kicked in either.
+ *
+ * To fix that, an override definition replaces the default definition for
+ * the same property (by property name) instead of being appended alongside
+ * it.
+ */
+function mergeAttributeDefinitions(
+ defaults: readonly AttributeDefinition[],
+ overrides: readonly AttributeDefinition[],
+): AttributeDefinition[] {
+ const overriddenKeys = new Set(
+ overrides.map(getAttributeDefinitionKey).filter(Boolean),
+ );
+ const remainingDefaults = defaults.filter(
+ definition => !overriddenKeys.has(getAttributeDefinitionKey(definition)),
+ );
+ return [...remainingDefaults, ...overrides];
+}
+
export function getOverrideHtmlSchema(
originalSchema: typeof defaultSchema,
htmlSchemaOverrides: SafeMarkdownProps['htmlSchemaOverrides'],
) {
- // Merge into a fresh clone: mergeWith mutates its first argument, and the
- // array customizer concatenates, so merging into the shared defaultSchema
- // import would progressively widen the sanitization allowlist for every
- // SafeMarkdown instance app-wide.
+ // Merge into a fresh clone: mergeWith mutates its first argument, so
+ // merging into the shared defaultSchema import would progressively widen
+ // the sanitization allowlist for every SafeMarkdown instance app-wide.
+ const target = cloneDeep(originalSchema);
return mergeWith(
- cloneDeep(originalSchema),
+ target,
htmlSchemaOverrides,
- (objValue, srcValue) =>
- Array.isArray(objValue) ? objValue.concat(srcValue) : undefined,
+ (objValue, srcValue, _key, object) => {
+ if (!Array.isArray(objValue)) return undefined;
+ // Only the per-tag (and `'*'`) arrays nested under `attributes` hold
+ // property definitions that need dedup-by-key; every other array in
+ // the schema (e.g. `tagNames`, `protocols.href`) is a plain list where
+ // concatenation is the correct merge.
+ if (object === target.attributes) {
+ return mergeAttributeDefinitions(objValue, srcValue);
+ }
Review Comment:
**Suggestion:** The `attributes` merge path assumes `srcValue` is always an
array, but `htmlSchemaOverrides` comes from untyped runtime config
(`JsonObject`) and can be malformed. If an operator sets a non-array value for
a tag (for example a string/object), this call will throw at runtime when
`mergeAttributeDefinitions` tries to use array methods. Add an
`Array.isArray(srcValue)` guard before calling the helper (or safely fall back
to default merge behavior) so bad config does not crash markdown rendering.
[type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Dashboard Markdown fails when schema extension shape is invalid.
❌ Handlebars chart viewer fails on malformed HTML schema overrides.
⚠️ Operators misconfiguring schema can silently break markdown-based charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with an invalid HTML_SANITIZATION_SCHEMA_EXTENSIONS
value, e.g. in
`superset/config.py:1413` set `HTML_SANITIZATION_SCHEMA_EXTENSIONS =
{"attributes": {"li":
"className"}}` (string instead of array for `li` attributes).
2. Load a dashboard containing a Markdown component; the Redux-connected
Markdown grid
component at
`superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx:18-26`
maps `state.common.conf.HTML_SANITIZATION_SCHEMA_EXTENSIONS` into the
`htmlSchemaOverrides` prop via `mapStateToProps` at `Markdown.tsx:18-26`.
3. When the Markdown component renders, it creates a `SafeMarkdown` instance
at
`Markdown.tsx:27-37` passing `htmlSchemaOverrides` through to `SafeMarkdown`
at
`packages/superset-ui-core/src/components/SafeMarkdown/SafeMarkdown.tsx:147-151`,
which in
turn calls `getOverrideHtmlSchema(defaultSchema, htmlSchemaOverrides)`
inside the
`rehypePlugins` memo at `SafeMarkdown.tsx:164-173`.
4. In `getOverrideHtmlSchema` at `SafeMarkdown.tsx:122-144`, `mergeWith`
invokes the
customizer for `attributes.li` where `objValue` is the default array from
`defaultSchema.attributes.li` and `srcValue` is the string `"className"`
from the invalid
config; the branch `if (object === target.attributes)` at
`SafeMarkdown.tsx:139-141` calls
`mergeAttributeDefinitions(objValue, srcValue)`, and inside
`mergeAttributeDefinitions` at
`SafeMarkdown.tsx:109-119` the call to `overrides.map(...)` executes on a
string, causing
a `TypeError: overrides.map is not a function` and crashing
Markdown/Handlebars rendering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b81dab7e0b054c31b904466789b5d2e5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b81dab7e0b054c31b904466789b5d2e5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/SafeMarkdown/SafeMarkdown.tsx
**Line:** 139:141
**Comment:**
*Type Error: The `attributes` merge path assumes `srcValue` is always
an array, but `htmlSchemaOverrides` comes from untyped runtime config
(`JsonObject`) and can be malformed. If an operator sets a non-array value for
a tag (for example a string/object), this call will throw at runtime when
`mergeAttributeDefinitions` tries to use array methods. Add an
`Array.isArray(srcValue)` guard before calling the helper (or safely fall back
to default merge behavior) so bad config does not crash markdown rendering.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42202&comment_hash=68278728ba874156a3d2f2d539fd754a5030e1a7285780d76d3f8f0b6e27778f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42202&comment_hash=68278728ba874156a3d2f2d539fd754a5030e1a7285780d76d3f8f0b6e27778f&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx:
##########
@@ -83,6 +83,65 @@ describe('getOverrideHtmlSchema', () => {
(second.tagNames ?? []).filter(name => name === 'iframe'),
).toHaveLength(1);
});
+
+ // Regression tests for #34191: hast-util-sanitize's real default schema
+ // restricts `li`/`ul`/`ol` `className` to specific task-list values, e.g.
+ // `li: [['className', 'task-list-item']]` (see hast-util-sanitize's
+ // `lib/schema.js`). `rehype-sanitize` is mocked globally in this jest
+ // project (see NOTE above), so `defaultSchema` isn't usable here; this
+ // fixture mirrors just the shape that matters for these tests.
+ //
+ // findDefinition only ever returns the FIRST definition it finds for a
+ // given property name, so simply concatenating the operator's override
+ // after the default restrictive tuple left the default in charge: any
+ // other class value was silently dropped, and (since a failed allowlist
+ // check returns `[]` rather than `undefined`) hast-util-sanitize's own
+ // `'*'` wildcard fallback never kicked in either.
+ const taskListSchemaFixture: typeof defaultSchema = {
+ attributes: {
+ li: [['className', 'task-list-item']],
+ ul: [['className', 'contains-task-list']],
+ ol: [['className', 'contains-task-list']],
+ },
+ };
+
+ test('an override for a property replaces the same-named default
definition', () => {
+ const result = getOverrideHtmlSchema(taskListSchemaFixture, {
+ attributes: { li: ['className'] },
+ });
+
+ // The override should fully replace the default `li` className
+ // restriction, not merely be appended after it.
+ expect(result.attributes?.li).toEqual(['className']);
+ });
+
+ test('a "*" wildcard override allows classes on li even though li has its
own default definition', () => {
+ const result = getOverrideHtmlSchema(taskListSchemaFixture, {
+ attributes: { '*': ['className'] },
+ });
Review Comment:
**Suggestion:** The test name states that a wildcard override allows classes
on `li`, but the assertions verify the opposite (`li` remains restricted and
wildcard alone does not widen it). Rename the test description to match the
actual asserted behavior to avoid misleading future maintainers and reviewers.
[comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Test name misrepresents wildcard override behavior on li elements.
⚠️ Future maintainers may misinterpret sanitization schema behavior from
tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the SafeMarkdown test suite, focusing on the `getOverrideHtmlSchema`
describe block
in
`superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx:38-145`
to execute the test named `a "*" wildcard override allows classes on li even
though li has
its own default definition` at line 118.
2. Inspect the test body at `SafeMarkdown.test.tsx:118-121`, which calls
`getOverrideHtmlSchema(taskListSchemaFixture, { attributes: { '*':
['className'] } })`
using the `taskListSchemaFixture` defined earlier at
`SafeMarkdown.test.tsx:100-106` where
`li` already has a restrictive `className` tuple.
3. Observe the assertions at `SafeMarkdown.test.tsx:123-129`: the test
verifies that
`result.attributes?.['*']` contains `'className'` and that
`result.attributes?.li` remains
equal to `taskListSchemaFixture.attributes?.li`, meaning the wildcard
override does not
actually allow arbitrary classes on `li` and leaves the default restriction
intact.
4. Compare the test name with its assertions and comments; the title claims
the wildcard
override "allows classes on li" despite the documented and asserted behavior
that a `'*'`
override alone does not widen tags which already have their own definitions,
demonstrating
a mismatch that can mislead maintainers about the sanitizer semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=51d3ba16cc4f469ea168180955b5014f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=51d3ba16cc4f469ea168180955b5014f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx
**Line:** 118:121
**Comment:**
*Comment Mismatch: The test name states that a wildcard override allows
classes on `li`, but the assertions verify the opposite (`li` remains
restricted and wildcard alone does not widen it). Rename the test description
to match the actual asserted behavior to avoid misleading future maintainers
and reviewers.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42202&comment_hash=ae4c5719c1acbdab939b36867ffac00736b207305cdb72cddbd6f5f196855eb9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42202&comment_hash=ae4c5719c1acbdab939b36867ffac00736b207305cdb72cddbd6f5f196855eb9&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
