Copilot commented on code in PR #42202:
URL: https://github.com/apache/superset/pull/42202#discussion_r3623690871
##########
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) => {
Review Comment:
`getOverrideHtmlSchema` compares `object === target.attributes` inside the
mergeWith customizer. In this repo's Jest setup, `rehype-sanitize` is mocked
without a `defaultSchema` named export (see `spec/helpers/shim.tsx`), so
`originalSchema` can be `undefined` in tests. If `cloneDeep(originalSchema)`
yields `undefined`, accessing `target.attributes` can throw when the customizer
runs. Guard against a nullish target (or default it to `{}`) and compare
against a precomputed `targetAttributes` reference.
##########
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];
+}
Review Comment:
`getAttributeDefinitionKey` assumes any non-string definition is indexable
(tuple), but `htmlSchemaOverrides.attributes.*` comes from runtime config and
may contain `null`/objects. If an override array contains `null`,
`definition[0]` will throw and break markdown rendering. Make the
key-extraction function defensive and treat non-string/non-array values as
having no key.
--
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]
