rusackas commented on code in PR #42202:
URL: https://github.com/apache/superset/pull/42202#discussion_r3624610786
##########
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:
Good catch, fixed. `getAttributeDefinitionKey` now guards with
`Array.isArray` before indexing, so a `null` (or other non-array) element in an
override array is treated as keyless instead of throwing. Added a test for it
in df793d88b7.
##########
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:
I don't think this actually reproduces. That comparison only runs once
`objValue` is already an array, and since `target` is undefined there's nothing
pre-existing anywhere in the tree for `objValue` to match, so the customizer
bails out on the `Array.isArray(objValue)` check before it ever gets there.
Tested it standalone with `target` undefined, no throw.
Also worth noting `originalSchema` in production is always the real
`defaultSchema` import, never undefined.
--
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]
