apostrophecms / sanitize-html

Clean up user-submitted HTML, preserving whitelisted elements and whitelisted attributes on a per-element basis. Built on htmlparser2 for speed and tolerance
MIT License
3.84k stars 353 forks source link

img allowedTags throwing an error in Nuxt.js #483

Closed mrfrase3 closed 3 years ago

mrfrase3 commented 3 years ago

To Reproduce

Not too sure how to reproduce, Using this in a Nuxt.js application, with the issue occurring during SSR on the latest version of sanitize-html.

Following the docs, I added { allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']) } to allow img tags, this worked fine in version 2.1.0.

Updating to 2.4.0, nuxt starts throwing a very non-descript error "TypeError: isPlainObject is not a function", with no other stack traces.

Assuming this is some typescript querk, I changed the configuration to: { allowedTags: [...sanitizeHtml.defaults.allowedTags, 'img'] } and it now works like before. :man_shrugging:

Expected behavior

The sanitizeHtml.defaults.allowedTags.concat method described in the readme to work, without throwing errors.

Describe the bug

Looks like allowedTags isn't a "plain" array, so it's causing issues when passed as an option.

Details

Version of Node.js: Observed on both 12.16.1 and 14.16.1

Server Operating System: Observed on Linux and MacOS X

Additional context: Nuxt version 2.14.6

Maybe the fix is just to update the readme to not use concat?

mrfrase3 commented 3 years ago

Looks like I was too eager to make this issue, using a spread operator does not work.

This is definitely only happening during SSR on nuxt, doing the following actually worked, but it just disables the img tags when sanitizing on the server-side.

process.client ? { allowedTags: [...sanitizeHtml.defaults.allowedTags, 'img'] } : undefined

So the issue seems to be with allowing image tags whilst doing server-side rendering in nuxt...

boutell commented 3 years ago

Hi, thanks for the report.

This sounds like an issue external to sanitize-html. The system you describe has a lot of parts that are not sanitize-html.

However if you can reproduce it in a unit test in sanitize-html we'll be glad to take a look.

I would also suggest running nuxt in whatever its development build mode is, so that you can see a better stack trace, but I don't have any further expertise with nuxt to offer.

Closing for now for housekeeping, but feel free to reopen if you find a way to reproduce this with just sanitize-html and a known set of inputs and expected outputs.

On Wed, Jun 16, 2021, 12:45 AM Fraser @.***> wrote:

Looks like I was too eager to make this issue, using a spread operator does not work.

This is definitely only happening during SSR on nuxt, doing the following actually worked, but it just disables the img tags when sanitizing on the server-side.

process.client ? { allowedTags: [...sanitizeHtml.defaults.allowedTags, 'img'] } : undefined

So the issue seems to be with allowing image tags whilst doing server-side rendering in nuxt...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/sanitize-html/issues/483#issuecomment-862034763, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27P6HFWUU42NZFPBQDLTTAT6TANCNFSM46YSFNNQ .

williamstein commented 3 years ago

I'm hitting exactly the same problem. I think the last version of the is-plain-object module was changed in a way that is inconsistent with their docs and with how it is used in sanitize-html. The file node_modules/is-plain-object/index.cjs.js has the line module.exports = isPlainObject; and the file node_modules/is-plain-object/index.es.js has the line export default isPlainObject;. The docs say to use isPlainObject like this const { isPlainObject } = require('is-plain-object');, which is just wrong.

I'm puzzled because is-plain-object seems to be downloaded 25 million times a week, so how can it be broken? It doesn't work for me in any situation: not with node on the command line and not with next.js using webpack 5.

Here's with node on the command line:

~/cocalc/src/packages/frontend$ node --version
v14.17.4
~/cocalc/src/packages/frontend$ node
Welcome to Node.js v14.17.4.
Type ".help" for more information.
> require('./node_modules/is-plain-object')     # this is NOT a map with isPlainObject as key.
[Function: isPlainObject]
> 
~/cocalc/src/packages/frontend$ grep exports node_modules/is-plain-object/index.cjs.js
module.exports = isPlainObject;

You should probably re-open this issue. One fix might be to switch to this module:

https://www.npmjs.com/package/is-plain-obj

It seems nearly as popular as is-plain-object, and it's documentation seems correct instead of wrong. Also, it's 6 lines of code total, written 3 years ago instead of 6 years ago.

In the meantime, I'm going to try to hack around this problem using resolve.alias in webpack...

boutell commented 3 years ago

sanitize-html depends on version 5.0.0 of is-plain-object, according to package.json.

The package.json of that version of is-plain-object points to dist/is-plain-object.js as main, so that is what gets required.

And that file ends with:

exports.isPlainObject = isPlainObject;

Which shows isPlainObject is indeed a subproperty as expected.

The newer exports section of package.json also points to this file for "require", as well as an equivalent one for "import." No sign of a function as default export in either.

So there appears to be no problem. Possibly you are forcing the use of a different major version of is-plain-object somehow.

There is just one 5.x version, 5.0.0, so no chance of a bc break within that series, but I removed package-lock and reinstalled just to make sure.

williamstein commented 3 years ago

@boutell you're exactly right. Both version 5.0.0 and version 3.x of is-plain-object are installed, and my webpack config (which is nontrivial for some other complicated reasons) is evidently messing up the module resolution. This is definitely not at all something that is wrong with sanitize-html, and I was wrong.

boutell commented 3 years ago

No worries. I've been in similar messes.

dosstx commented 3 years ago

Nuxt 2 user here. Just to confirm, we can use this in a Nuxt app that is SSR?

abea commented 3 years ago

@dosstx I don't think there's any reason why you couldn't.