search

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.68k stars 349 forks source link

Fix checkmarx vulnerability #648

Closed finbargp closed 4 months ago

finbargp commented 4 months ago

Apologies if this has been reported before, but I couldn't find it. I'm using v2.11 of sanitize-html

To Reproduce

When I attempt to commit any change to my package.json in IntelliJ I get the following warning, which I have to override each time:

Dependency npm:sanitize-html:2.11.0 is vulnerable Cx24228ad1-81fd 6.1 Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') vulnerability Results powered by Checkmarx(c)

Link to issue: https://devhub.checkmarx.com/cve-details/Cx24228ad1-81fd/

Expected behavior

There should not be a checkmarx vulnerability like this in sanitize-html.

Describe the bug

Checkmarx vulnerability.

Details

Version of Node.js: v18.17.1

Server Operating System: MacOS X

boutell commented 4 months ago

Thanks for reporting this, but the vulnerability page in question is very much not accurate.

Only a developer can pass allowedTags, therefore it is not a "network" vulnerability and it certainly is not "privileges required: none."

A person who can change the non-user-input parameters to sanitize-html is a developer, i.e. they can just bypass it completely, not invoke sanitize-html at all, etc. This is not a real vulnerability. If you look at the original issue linked on that page, it's a developer suggesting that the option could be checked for its type, which was actually added for those using typescript. It has nothing to do with any actual security vulnerability.

It's unfortunate that checkmarx seems to provide no way to get in contact regarding these pages, however I have filled out their sales contact form and will try to reach a human to get this removed.

All that said, I do appreciate your concern and your report. I just can't fix a vulnerability that does not exist. I'll do my best to get checkmarx to remove this inaccurate report.

boutell commented 4 months ago

(If you have a means of getting checkmarx' attention, I would appreciate being put in contact - happy to help get this off your plate by getting the inaccurate report corrected.)

finbargp commented 4 months ago

OK thanks for your response and confirming this isn't a real vulnerability. I don't have any means of getting checkmarx' attention. I appreciate it if you're able to get checkmarx to close this so sanitize-html doesn't show up as vulnerable in our IDEs etc.

boutell commented 4 months ago

I reached out to them on twitter and was asked to DM, so I've done that, and we'll see. Thanks for your understanding.

On Thu, Feb 22, 2024 at 4:04 AM Finbar Clenaghan (G-P) < @.***> wrote:

OK thanks for your response and confirming this isn't a real vulnerability. I don't have any means of getting checkmarx' attention. I appreciate it if you're able to get checkmarx to close this so sanitize-html doesn't show up as vulnerable in our IDEs etc.

— Reply to this email directly, view it on GitHub https://github.com/apostrophecms/sanitize-html/issues/648#issuecomment-1958994474, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27O4KLESRWLC4KCPUHDYU4CYRAVCNFSM6AAAAABDRMGN3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYHE4TINBXGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 4 months ago

Good news: today they followed up and removed the vulnerability.

finbargp commented 4 months ago

That's great news @boutell - thanks for following up on this!