sanitizeHtml config not mandatory

[falkodev]

Not sure to understand.

Here is the the "defaults" config in the package:

  options.sanitizeHtml.allowedAttributes = {
      a: ['name', 'target'],
      td: ['colspan', 'rowspan'],
      th: ['colspan', 'rowspan']
    };

Note I removed the href element for a for test purposes.

If at project level, I have no sanitizeHtml config:

// app.js
'apostrophe-tiptap-rich-text-widgets': {}

Here is the returned config from the package:

options.sanitizeHtml {
  allowedAttributes: {
    a: [ 'name', 'target' ],
    td: [ 'colspan', 'rowspan' ],
    th: [ 'colspan', 'rowspan' ]
  }
}

And, if I pass a config at project level:

// app.js
  'apostrophe-tiptap-rich-text-widgets': {
      sanitizeHtml: {
        allowedAttributes: {
          a: ['href'],
          h1: ['test']
        },
        allowedTags: ['b']
      }
    }

Here is the returned config from the package:

options.sanitizeHtml {
  allowedAttributes: {
    a: [ 'name', 'target', 'href' ],
    h1: [ 'test' ],
    td: [ 'colspan', 'rowspan' ],
    th: [ 'colspan', 'rowspan' ]
  },
  allowedTags: [ 'b' ]
}

Note, the href element is merged as expected in a.

[boutell]

The actual sanitize-html npm module has its own defaults, which are used if one of the options, like allowedAttributes, is not specified at all.

You are merging allowedAttributes with your own list, which happens to cover the same attributes for tags that matter in the rich text editor, which is OK so far.

However you are not merging allowedTags, so things become confusing for the developer. When are things merged (allowedAttributes), and when do you have to specify everything (allowedTags)? The developer specifies allowedAttributes, it merges, so they think it is safe to specify allowedTags with just one new tag, and they lose all the other tags. Even though your documentation says it will be merged.

So since we have started down this road, I think you should also supply and merge with a default allowedTags list that covers all the tags that are actually emitted by our tiptap toolbar.

At that point it should be sufficient.

[boutell]

In 3.x we set sanitize html config dynamically based on the actual toolbar so this issue thankfully goes away (: