Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

styles are always removed in browser #547

Closed
Julian-B90 opened this issue Apr 15, 2022 · 26 comments · Fixed by #596
Closed

styles are always removed in browser #547

Julian-B90 opened this issue Apr 15, 2022 · 26 comments · Fixed by #596

Comments

@Julian-B90
Copy link

Julian-B90 commented Apr 15, 2022

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:

  1. Create React App React 17
  2. 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

<p style="color:#c0392b">test</p>

What i get

<p>test</p>

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"
  },
@Julian-B90 Julian-B90 added the bug label Apr 15, 2022
@boutell
Copy link
Member

boutell commented Apr 15, 2022

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
Copy link
Author

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
Copy link
Member

boutell commented Apr 18, 2022

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.

@boutell boutell changed the title styles are always removed in browser (with react) styles are always removed in browser Apr 18, 2022
@kyoncy
Copy link

kyoncy commented Apr 19, 2022

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

boutell commented May 3, 2022 via email

@yauluntang
Copy link

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
Copy link
Member

boutell commented May 3, 2022 via email

@yauluntang
Copy link

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
Copy link
Member

boutell commented May 4, 2022

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
Copy link

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

@edolix
Copy link

edolix commented Jul 14, 2022

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
Copy link

shinxi commented Aug 23, 2022

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
Copy link

mattweberio commented Oct 10, 2022

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:

<p style="color: {{peachColor}};">Test</p>

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
Copy link

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

@NishantDesai1306
Copy link

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

image

image

@bertyhell
Copy link
Contributor

bertyhell commented Dec 13, 2022

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);
	});
});

@boutell
Copy link
Member

boutell commented Dec 13, 2022

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.

@bertyhell
Copy link
Contributor

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

@mattweberio
Copy link

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:

<p style="color: {{peachColor}};">Test</p>

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

@boutell
Copy link
Member

boutell commented Jan 17, 2023 via email

@bertyhell
Copy link
Contributor

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

@bertyhell
Copy link
Contributor

bertyhell commented Jan 23, 2023

I fixed the readme issues

@krassowski
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.