styles are always removed in browser #547

Julian-B90 · 2022-04-15T13:05:38Z

To Reproduce

It always happens in browserland. See this simple codesandbox:

https://codesandbox.io/s/festive-kirch-d1geou?file=/src/index.js

Original message is below but note this is not React-specific. -Tom

Original message

Step by step instructions to reproduce the behavior:

Create React App React 17

call this in first page

console.log(
    'html sanitizeHtml',
    sanitizeHtml('<p style="color:#c0392b">test</p>', {
      allowedTags: sanitizeHtml.defaults.allowedTags.concat([
        'iframe',
        'img',
        'del',
        'ins',
      ]),
      allowedStyles: {
        p: {
          // Match HEX and RGB
          color: [
            /^.*$/i,
            /^rgb\(\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(\d{1,3})\s*\)$/,
          ],
        },
      },
      allowedAttributes: {
        '*': ['style'],
      },
    })
  );

Expected behavior

test

What i get

test

Describe the bug

the style attribute was removed

Details

"dependencies": {
    "@emotion/react": "^11.4.1",
    "@emotion/styled": "^11.6.0",
    "@mui/icons-material": "^5.4.1",
    "@mui/lab": "^5.0.0-alpha.71",
    "@mui/material": "^5.4.1",
    "@mui/styled-engine-sc": "^5.4.1",
    "@mui/x-data-grid": "^5.5.0",
    "date-fns": "^2.28.0",
    "froala-editor": "^4.0.9",
    "i18next": "^21.6.11",
    "react": "^17.0.2",
    "react-beautiful-dnd": "^13.1.0",
    "react-device-detect": "^2.1.2",
    "react-dom": "^17.0.2",
    "react-fast-compare": "^3.2.0",
    "react-froala-wysiwyg": "^4.0.9",
    "react-hook-form": "^7.26.1",
    "react-i18next": "^11.15.4",
    "react-router-dom": "^5.3.0",
    "react-router-hash-link": "^2.4.3",
    "sanitize-html": "^2.7.0",
    "sort-json": "^2.0.0"
  },

The text was updated successfully, but these errors were encountered:

boutell · 2022-04-15T20:42:46Z

Interesting issue. We don't use sanitize-html in the browser ourselves and there's no indication of this problem server-side, so it could be a while before we look at it in-house.

So what I'd suggest is that you set up the simplest possible webpack project that imports sanitize-html and tests this out in vanilla JS browser-side, as a starting point and to make sure you have the latest of everything.

And before that, I'd sanity-check that your code doesn't also fail when added to our regular unit tests and run with node on the command line, as if that fails it has nothing specifically to do with the browser.

Julian-B90 · 2022-04-16T09:34:18Z

Hey @boutell i have create a simple codesanbox https://codesandbox.io/s/festive-kirch-d1geou?file=/src/index.js

When i made a simpe js file an run it directly with node it work correctly

boutell · 2022-04-18T13:01:01Z

OK thanks for clarifying the nature of the issue. So it is not related to React, it happens all the time in the browser. That may be helpful to other interested developers who use sanitize-html on the front end and wish to help debug this problem.

kyoncy · 2022-04-19T07:20:35Z

I also experienced the same behavior.
It seems to have occurred when I raised the react-scripts version from 4 to 5. I am investigating the cause as react-scripts depends on various libraries.

yauluntang · 2022-05-03T03:26:18Z

In the code, stringifyStyleAttributes would fail and go to catch block so the attribute got deleted

if (a === 'style') {
try {
const abstractSyntaxTree = postcssParse(name + ' {' + value + '}');
const filteredAST = filterCss(abstractSyntaxTree, options.allowedStyles);

            value = stringifyStyleAttributes(filteredAST);

            if (value.length === 0) {
              delete frame.attribs[a];
              return;
            }
          } catch (e) {
            delete frame.attribs[a];
            return;
          }
        }

yauluntang · 2022-05-03T03:36:26Z

In the code, stringifyStyleAttributes would fail and go to catch block so the attribute got deleted

if (a === 'style') { try { const abstractSyntaxTree = postcssParse(name + ' {' + value + '}'); const filteredAST = filterCss(abstractSyntaxTree, options.allowedStyles);
            value = stringifyStyleAttributes(filteredAST);

            if (value.length === 0) {
              delete frame.attribs[a];
              return;
            }
          } catch (e) {
            delete frame.attribs[a];
            return;
          }
        }

nanoid is not a function in browser

yauluntang · 2022-05-03T03:42:40Z

I proposed to add an option to patch this issue,

if (a === 'style' && options.processStyle) and for options we set processStyle = false to bypass this issue. I don't have access to create branch and pull request though

boutell · 2022-05-03T14:26:17Z

Are you saying there are features that *cannot* work in the browser right now? What would processStyle: false actually do? I'm a little unclear on that. Anyone can submit a PR. The procedure is to "fork" the project to your own github (see the github page for the repo, there's a button for that). Then github will invite you to create a PR from your repo to ours when you have changes.

…

On Mon, May 2, 2022 at 11:42 PM Yau Lun Tang ***@***.***> wrote: I proposed to add an option to patch this issue, if (a === 'style' && options.processStyle) and for options we set processStyle = false to bypass this issue. I don't have access to create branch and pull request though — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

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

yauluntang · 2022-05-03T14:42:36Z

Are you saying there are features that cannot work in the browser right now? What would processStyle: false actually do? I'm a little unclear on that. Anyone can submit a PR. The procedure is to "fork" the project to your own github (see the github page for the repo, there's a button for that). Then github will invite you to create a PR from your repo to ours when you have changes.
…
On Mon, May 2, 2022 at 11:42 PM Yau Lun Tang @.> wrote: I proposed to add an option to patch this issue, if (a === 'style' && options.processStyle) and for options we set processStyle = false to bypass this issue. I don't have access to create branch and pull request though — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ . You are receiving this because you were mentioned.Message ID: @.>
-- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

Looks like there are some dependencies that rely on node only. Browser won't work with those modules. processStyle: false means that it will not try to sanitize or remove the style attribute. I downloaded and modify the code locally and tried and it can workaround the problem.

boutell · 2022-05-03T17:30:46Z

I see. So right now if the style attribute is permitted we immediately begin using the problematic feature? Wouldn't it make more sense if the problematic feature was not even part of the build, if it can't work outside of Node? We don't use sanitize-html in the browser here (it's usually a mistake since you can't ever trust the browser anyway; yes there are some edge cases like using it for output rather than input). So that is work is unlikely to happen on our end, but just asking what you think would be most ideal if it were possible. On Tue, May 3, 2022 at 10:42 AM Yau Lun Tang ***@***.***> wrote:

…

Are you saying there are features that *cannot* work in the browser right now? What would processStyle: false actually do? I'm a little unclear on that. Anyone can submit a PR. The procedure is to "fork" the project to your own github (see the github page for the repo, there's a button for that). Then github will invite you to create a PR from your repo to ours when you have changes. … <#m_5323872795942038585_> On Mon, May 2, 2022 at 11:42 PM Yau Lun Tang *@*.*> wrote: I proposed to add an option to patch this issue, if (a === 'style' && options.processStyle) and for options we set processStyle = false to bypass this issue. I don't have access to create branch and pull request though — Reply to this email directly, view it on GitHub <#547 (comment) <#547 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ <https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ> . You are receiving this because you were mentioned.Message ID: @.*> -- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his Looks like there are some dependencies that rely on node only. Browser won't work with those modules. processStyle: false means that it will not try to sanitize or remove the style attribute. I downloaded and modify the code locally and tried and it can workaround the problem. — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27P4XUBJ4HPOWRUVGTTVIE3OPANCNFSM5TQNA7IQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

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

yauluntang · 2022-05-04T01:35:11Z

I see. So right now if the style attribute is permitted we immediately begin using the problematic feature? Wouldn't it make more sense if the problematic feature was not even part of the build, if it can't work outside of Node? We don't use sanitize-html in the browser here (it's usually a mistake since you can't ever trust the browser anyway; yes there are some edge cases like using it for output rather than input). So that is work is unlikely to happen on our end, but just asking what you think would be most ideal if it were possible. On Tue, May 3, 2022 at 10:42 AM Yau Lun Tang @.> wrote:
…
Are you saying there are features that cannot work in the browser right now? What would processStyle: false actually do? I'm a little unclear on that. Anyone can submit a PR. The procedure is to "fork" the project to your own github (see the github page for the repo, there's a button for that). Then github will invite you to create a PR from your repo to ours when you have changes. … <#m_5323872795942038585_> On Mon, May 2, 2022 at 11:42 PM Yau Lun Tang @.> wrote: I proposed to add an option to patch this issue, if (a === 'style' && options.processStyle) and for options we set processStyle = false to bypass this issue. I don't have access to create branch and pull request though — Reply to this email directly, view it on GitHub <#547 (comment) <#547 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ https://github.com/notifications/unsubscribe-auth/AAAH27LGH7IDIUTX5A5CZBTVICODVANCNFSM5TQNA7IQ . You are receiving this because you were mentioned.Message ID: @.> -- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his Looks like there are some dependencies that rely on node only. Browser won't work with those modules. processStyle: false means that it will not try to sanitize or remove the style attribute. I downloaded and modify the code locally and tried and it can workaround the problem. — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27P4XUBJ4HPOWRUVGTTVIE3OPANCNFSM5TQNA7IQ . You are receiving this because you were mentioned.Message ID: @.**>
-- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

I think it is better to use the feature (sanitize style attribute) by default and have an optional parameter to turn it off.

I am trying to make a website that can preview html tags and output it solely in the client side and this sanitize-html module is a perfect fit for me to use in the client side.

boutell · 2022-05-04T12:48:22Z

My concern is that it sounds like the build of this module that you wind up with in the browser probably has a lot of weight for a feature that doesn't even work in the browser, no? Not that this is a use case I'm personally concerned with, but I might be if I were in your position. I do understand that you're using sanitize html for output rather than input in the browser, which can be secure.

AlexSCorey · 2022-05-18T21:15:28Z

I too am experiencing this and would like to see it fixed if we can!

edolix · 2022-07-14T08:39:23Z

I'm getting the same issue trying to sanitize HTML for output. I'll see if i have some time to dig and file a PR

shinxi · 2022-08-23T09:56:13Z

For anyone who is facing the issue in a CRA repo, one workaround is to eject from CRA and add the cjs support, e.g.,
https://github.com/facebook/create-react-app/pull/12021/files#diff-8e25c4f6f592c1fcfc38f0d43d62cbd68399f44f494c1b60f0cf9ccd7344d697R467, another workaround is to customize the webpack configuration via react-app-wired, e.g., facebook/create-react-app#12021 (comment).
See facebook/create-react-app#12021 for detail.

mattweberio · 2022-10-10T20:14:23Z

We have an issue when we use styles with "placeholders." The previous version only checked the "style" tag and did not validate the style tag's contents.

Example:

Test

Please also consider this use case for adding a disable flag.

Can you please recommend which version style values are validated? We want to return to the latest version without validation until there is a way to skip validating the style values.

slavco86 · 2022-11-24T12:48:22Z

Same here. Please, can someone look into this - kind of a blocker for using this package in the browser

NishantDesai1306 · 2022-12-04T21:22:20Z

yup, nanoid used inside postcssParse is causing issue when used in browser

bertyhell · 2022-12-13T12:47:44Z

There is a thread on postcss where they recommend not using postcss in the browser:
postcss/postcss#1727

For now, i'll just switch to the https://github.com/cure53/DOMPurify package

Here is a jest test that works in jest/node but not in the browser

import sanitize from 'sanitize-html';

describe('sanitize', () => {
	it('Should not remove p styles', () => {
		const originalHtml =
			'"<p style="text-align:center">some text</p>"';
		const sanitizedHtml = sanitize(originalHtml, {
			allowedTags: [
				'p',
			],
			allowedAttributes: {
				p: ['style'],
			},
		});
		expect(sanitizedHtml).toEqual(originalHtml);
	});
});

package bug: apostrophecms/sanitize-html#547 postcss should not be used in a browser

boutell · 2022-12-13T14:34:46Z

They're not wrong (: We have always felt that the use of sanitize-html on the browser side is a little bit crazy, since you can never trust a browser anyway, although I accept there's an edge case where you use it for output of otherwise untrusted input... even though that is unnecessary overhead... on every single render! Sanitization and servers go together like bread and jam.

This will fix apostrophecms#547

bertyhell · 2022-12-17T11:59:27Z

PR to add an option to disable style parsing has been opened: #596

mattweberio · 2023-01-17T03:03:06Z

Hi everyone (@boutell and @bertyhell) - Thank you for the PR!!! I wanted to note that this solves the browser use case, but we also have a use case in nodeJS because our application uses "placeholders" in the styles. So the addition of the new flag makes sense for a few valid use cases.

Our example:

Test

Is this PR expected to be included in the next release? We'd love to upgrade.

boutell · 2023-01-17T13:49:51Z

At last count this was nearly through, I had asked for a couple documentation revisions on the PR.

…

On Mon, Jan 16, 2023 at 10:03 PM mattweberio ***@***.***> wrote: Hi everyone ***@***.*** <https://github.com/boutell> and @bertyhell <https://github.com/bertyhell>) - Thank you for the PR!!! I wanted to note that this solves the browser use case, but we also have a use case in nodeJS because our application uses "placeholders" in the styles. So the addition of the new flag makes sense for a few valid use cases. Our example: Test Is this PR expected to be included in the next release? We'd love to upgrade. — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27OBP7JB36W6OQWV2NDWSYDXLANCNFSM5TQNA7IQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

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

bertyhell · 2023-01-23T09:31:47Z

yes, i need to tweak the docs a little, i'll put a reminder for this evening to take a look

bertyhell · 2023-01-23T17:06:17Z

I fixed the readme issues

krassowski · 2024-02-27T10:59:20Z

They're not wrong (: We have always felt that the use of sanitize-html on the browser side is a little bit crazy, since you can never trust a browser anyway, although I accept there's an edge case where you use it for output of otherwise untrusted input... even though that is unnecessary overhead... on every single render! Sanitization and servers go together like bread and jam.

@boutell just as FYI there are applications which do not have a Node server at all but use sanitize-html on the browser side, for example JupyterLab and Notebook use it because a user might open a notebook with an untrusted HTML. Thank you for your work here!

Julian-B90 added the bug label Apr 15, 2022

boutell added the seeking contributions label Apr 15, 2022

boutell changed the title ~~styles are always removed in browser (with react)~~ styles are always removed in browser Apr 18, 2022

bertyhell added a commit to viaacode/react-admin-core-module that referenced this issue Dec 13, 2022

fix(html-sanitize): replace html-sanitize with dom-purify because of bug

997c4ca

package bug: apostrophecms/sanitize-html#547 postcss should not be used in a browser

bertyhell mentioned this issue Dec 13, 2022

fix(html-sanitize): replace html-sanitize with dom-purify because of bug viaacode/react-admin-core-module#81

Merged

bertyhell added a commit to bertyhell/sanitize-html that referenced this issue Dec 17, 2022

feat(parseStyleAttributes): add option to skip style parsing

1ce3426

This will fix apostrophecms#547

bertyhell added a commit to bertyhell/sanitize-html that referenced this issue Dec 17, 2022

feat(parseStyleAttributes): add option to skip style parsing

58a1017

This will fix apostrophecms#547

bertyhell mentioned this issue Dec 17, 2022

feat(parseStyleAttributes): add option to skip style parsing [fix 547] #596

Merged

boutell closed this as completed in #596 Jan 24, 2023

apeman76 mentioned this issue Apr 12, 2023

Sanitize-html error extractus/article-extractor#339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

styles are always removed in browser #547

styles are always removed in browser #547

Julian-B90 commented Apr 15, 2022 •

edited by boutell

Loading

boutell commented Apr 15, 2022

Julian-B90 commented Apr 16, 2022

boutell commented Apr 18, 2022

kyoncy commented Apr 19, 2022

yauluntang commented May 3, 2022

yauluntang commented May 3, 2022

yauluntang commented May 3, 2022

boutell commented May 3, 2022 via email

yauluntang commented May 3, 2022

boutell commented May 3, 2022 via email

yauluntang commented May 4, 2022

boutell commented May 4, 2022

AlexSCorey commented May 18, 2022

edolix commented Jul 14, 2022

shinxi commented Aug 23, 2022

mattweberio commented Oct 10, 2022 •

edited

Loading

slavco86 commented Nov 24, 2022

NishantDesai1306 commented Dec 4, 2022

bertyhell commented Dec 13, 2022 •

edited

Loading

boutell commented Dec 13, 2022 •

edited

Loading

bertyhell commented Dec 17, 2022

mattweberio commented Jan 17, 2023

boutell commented Jan 17, 2023 via email

bertyhell commented Jan 23, 2023

bertyhell commented Jan 23, 2023 •

edited

Loading

krassowski commented Feb 27, 2024

styles are always removed in browser #547

styles are always removed in browser #547

Comments

Julian-B90 commented Apr 15, 2022 • edited by boutell Loading

To Reproduce

Original message

Expected behavior

What i get

Describe the bug

Details

boutell commented Apr 15, 2022

Julian-B90 commented Apr 16, 2022

boutell commented Apr 18, 2022

kyoncy commented Apr 19, 2022

yauluntang commented May 3, 2022

yauluntang commented May 3, 2022

yauluntang commented May 3, 2022

boutell commented May 3, 2022 via email

yauluntang commented May 3, 2022

boutell commented May 3, 2022 via email

yauluntang commented May 4, 2022

boutell commented May 4, 2022

AlexSCorey commented May 18, 2022

edolix commented Jul 14, 2022

shinxi commented Aug 23, 2022

mattweberio commented Oct 10, 2022 • edited Loading

slavco86 commented Nov 24, 2022

NishantDesai1306 commented Dec 4, 2022

bertyhell commented Dec 13, 2022 • edited Loading

boutell commented Dec 13, 2022 • edited Loading

bertyhell commented Dec 17, 2022

mattweberio commented Jan 17, 2023

boutell commented Jan 17, 2023 via email

bertyhell commented Jan 23, 2023

bertyhell commented Jan 23, 2023 • edited Loading

krassowski commented Feb 27, 2024

Julian-B90 commented Apr 15, 2022 •

edited by boutell

Loading

mattweberio commented Oct 10, 2022 •

edited

Loading

bertyhell commented Dec 13, 2022 •

edited

Loading

boutell commented Dec 13, 2022 •

edited

Loading

bertyhell commented Jan 23, 2023 •

edited

Loading