-
Notifications
You must be signed in to change notification settings - Fork 353
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
Comments
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. |
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 |
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. |
I also experienced the same behavior. |
In the code, stringifyStyleAttributes would fail and go to catch block so the attribute got deleted if (a === 'style') {
|
nanoid is not a function in browser |
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 |
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. |
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. |
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. |
I too am experiencing this and would like to see it fixed if we can! |
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 |
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., |
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:
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. |
Same here. Please, can someone look into this - kind of a blocker for using this package in the browser |
There is a thread on postcss where they recommend not using postcss in the browser: 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
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. |
PR to add an option to disable style parsing has been opened: #596 |
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:
Is this PR expected to be included in the next release? We'd love to upgrade. |
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:
<p style="color: {{peachColor}};">Test</p>
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
|
yes, i need to tweak the docs a little, i'll put a reminder for this evening to take a look |
I fixed the readme issues |
@boutell just as FYI there are applications which do not have a Node server at all but use |
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:
Expected behavior
What i get
Describe the bug
Details
The text was updated successfully, but these errors were encountered: