fix(SafeMarkdown): guard against malformed attribute overrides, fix misleading 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>

superset-frontend/packages/superset-ui-core/src/components/SafeMarkdown/SafeMarkdown.tsx

@@ -78,19 +78,74 @@ 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) {
+        // htmlSchemaOverrides comes from runtime config and isn't guaranteed
+        // to match the expected shape; fall back to the default definitions
+        // for a tag rather than throwing if an operator supplies something
+        // other than an array of attribute definitions.
+        if (!Array.isArray(srcValue)) return objValue;
+        return mergeAttributeDefinitions(objValue, srcValue);
+      }
+      return objValue.concat(srcValue);
+    },
   );
 }
 

superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx

@@ -83,6 +83,81 @@ 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 alone does not widen li, which has its own default definition', () => {
+    const result = getOverrideHtmlSchema(taskListSchemaFixture, {
+      attributes: { '*': ['className'] },
+    });
+
+    expect(result.attributes?.['*']).toContainEqual('className');
+    // The restrictive `li`-specific default is still present (untouched by
+    // this override), but since findDefinition matches on the specific
+    // `li` entry first, this alone documents why an `li`-specific override
+    // is required to unblock arbitrary classes on `li` -- a `'*'` override
+    // does not, by itself, widen a tag that already has its own definition.
+    expect(result.attributes?.li).toEqual(taskListSchemaFixture.attributes?.li);
+  });
+
+  test('a malformed (non-array) attribute override for a tag falls back to the default definition instead of throwing', () => {
+    expect(() =>
+      getOverrideHtmlSchema(taskListSchemaFixture, {
+        // Runtime config isn't guaranteed to match the expected shape; a
+        // string here (rather than an array of attribute definitions) must
+        // not crash markdown rendering.
+        attributes: { li: 'className' as unknown as string[] },
+      }),
+    ).not.toThrow();
+
+    const result = getOverrideHtmlSchema(taskListSchemaFixture, {
+      attributes: { li: 'className' as unknown as string[] },
+    });
+    expect(result.attributes?.li).toEqual(taskListSchemaFixture.attributes?.li);
+  });
+
+  test('the default task-list class restriction on li/ul/ol still applies with no override', () => {
+    const result = getOverrideHtmlSchema(taskListSchemaFixture, {});
+
+    expect(result.attributes?.li).toEqual([['className', 'task-list-item']]);
+    expect(result.attributes?.ul).toContainEqual([
+      'className',
+      'contains-task-list',
+    ]);
+    expect(result.attributes?.ol).toContainEqual([
+      'className',
+      'contains-task-list',
+    ]);
+  });
 });
 
 describe('transformLinkUri', () => {