rusackas commented on code in PR #42202:
URL: https://github.com/apache/superset/pull/42202#discussion_r3608845622
##########
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:
Good catch — `htmlSchemaOverrides` comes from runtime config, so a malformed
(non-array) attribute value would hit `overrides.map()` and throw. Added an
`Array.isArray(srcValue)` guard that falls back to the default definition for
that tag, plus a regression test.
##########
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:
Fair point, renamed to match what the test actually asserts (that a bare `*`
override does not widen `li`).
--
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]
