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.69k stars 351 forks source link

allowedClasses support for regex expressions #512

Closed alex-rantos closed 2 years ago

alex-rantos commented 2 years ago

Using wildcard on allowedClasses poses a security risk. Having regex support on classes is a better solution where you can be more explicit with the whitelisting.

Disclaimer: This is a performant way of allowing Regular Expressions on the existing codebase in a couple of lines :)

boutell commented 2 years ago

Hang on, how does that create an issue? It's just a weird class name, it doesn't execute as javascript as long as it's properly part of the class attribute — am I mistaken?

On Wed, Nov 10, 2021 at 1:19 PM Alexandros Rantos @.***> wrote:

@.**** commented on this pull request.

In README.md https://github.com/apostrophecms/sanitize-html/pull/512#discussion_r746865279 :

allowedClasses: {
'code': [ 'language-*', 'lang-*' ],
'*': [ 'fancy', 'simple' ]
}

+Furthermore, regular expressions are supported too and they must begin with ^ and end with $ otherwise the regex has no effect:

found out that it doesn't escape ; [image: image] https://user-images.githubusercontent.com/22919198/141169012-061f5664-a4b3-46e6-877f-768924463d84.png

  • will create an issue.

Meanwhile I updated readme suggesting to explicitly declare regExps.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/sanitize-html/pull/512#discussion_r746865279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27OTYW3XYWNZTOV6OFLULKZSRANCNFSM5HP6GJWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

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

boutell commented 2 years ago

I tried this in my browser, the alert did not run.

On Wed, Nov 10, 2021 at 2:24 PM Tom Boutell @.***> wrote:

Hang on, how does that create an issue? It's just a weird class name, it doesn't execute as javascript as long as it's properly part of the class attribute — am I mistaken?

On Wed, Nov 10, 2021 at 1:19 PM Alexandros Rantos < @.***> wrote:

@.**** commented on this pull request.

In README.md https://github.com/apostrophecms/sanitize-html/pull/512#discussion_r746865279 :

allowedClasses: {
'code': [ 'language-*', 'lang-*' ],
'*': [ 'fancy', 'simple' ]
}

+Furthermore, regular expressions are supported too and they must begin with ^ and end with $ otherwise the regex has no effect:

found out that it doesn't escape ; [image: image] https://user-images.githubusercontent.com/22919198/141169012-061f5664-a4b3-46e6-877f-768924463d84.png

  • will create an issue.

Meanwhile I updated readme suggesting to explicitly declare regExps.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/sanitize-html/pull/512#discussion_r746865279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27OTYW3XYWNZTOV6OFLULKZSRANCNFSM5HP6GJWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

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

--

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

alex-rantos commented 2 years ago

I cannot share the exact payload that can trigger the XSS nor I can find an equivalent for non-angular apps. The example was purely for demonstration purposes - sorry for not being clear enough.

boutell commented 2 years ago

This is an HTML sanitizer, it emits standards-compliant HTML in which attributes are properly escaped. Angular and HTML are not the same though — if the "class" attribute is treated as code in Angular, then that's not something sanitize-html can reasonably anticipate. But that's fine, it's just a documentation issue. I'll adjust the README.

Thank you for contributing the feature!

On Thu, Nov 11, 2021 at 12:10 PM Alexandros Rantos @.***> wrote:

I cannot share the exact payload that can trigger the XSS nor I can find an equivalent for non-angular apps. The example was purely for demonstration purposes - sorry for not being clear enough.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/sanitize-html/pull/512#issuecomment-966472527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27PHSYFPSYVVFSSG4ALULP2IJANCNFSM5HP6GJWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

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