search

cure53 / DOMPurify

DOMPurify - a DOM-only, super-fast, uber-tolerant XSS sanitizer for HTML, MathML and SVG. DOMPurify works with a secure default, but offers a lot of configurability and hooks. Demo:
https://cure53.de/purify
Other
13.61k stars 695 forks source link

Why attitudes are resorted after purified #889

Closed JounQin closed 8 months ago

JounQin commented 8 months ago

This issue proposes a bug

Background & Context

N/A

Bug

Input

<textarea type="text" cols="50" rows="10"></textarea>//["'\`-->]]>]</div>

Given output

<textarea rows="10" cols="50" type="text"></textarea>//["'\`--&gt;]]&gt;]

Expected output

<textarea type="text" cols="50" rows="10"></textarea>//["'\`--&gt;]]&gt;]

Feature

Don't resort attributes

cure53 commented 8 months ago

Again, this is done by the browser.

Please start doing some research what the expected browser behavior is before creating tickets here, this is a bug tracker and not a vending machine to request general HTML knowledge.

JounQin commented 8 months ago

@cure53

I did, but I don't see resorting:

image

And this makes a bit incompatible with my own lib domiso

cure53 commented 8 months ago

Please use this bug tracker for actual bugs and not for random Q&A.

The attribute sorting is nothing we can easily get rid of, also for security reasons. Compatibility with other libraries is not our goal, preventing XSS is.

JounQin commented 8 months ago

I'm posting this issue because I think this is a bug or at least unexpected behavior.

I'm not saying it should be compatible with other libs, actually I'm trying to make domiso being compatible with dompurify, I'm just observing the differences and want to discuss with you about which behavior should be desired.

Maybe we can update the expected value into array?

If my issues are just meaningless to you, I'm sorry, I just want to share what I observe which might be unexpected.

And like https://github.com/cure53/DOMPurify/issues/886#issuecomment-1856123865

We can make dompurify better.

cure53 commented 8 months ago

It's in fact not a bug, and very much expected behavior.

If my issues are just meaningless to you, I'm sorry, I just want to share what I observe which might be unexpected.

They are not meaningless but also in the current form not overly helpful. Some of the questions you could have answered yourself by reading the spec, others by browsing this bug tracker. So, not meaningless - but sadly redundant because you open tickets for issues where some research would have answered all questions already.

Please first check what knowledge is there already before starting to force others to be involved and look into questions that have long been covered by other, meanwhile ancient tickets.

JounQin commented 8 months ago

I did read some spec and context and we may be sharing different opinions on how content should be removed:

  1. non standard tags should not be considered as void, it is the spec, but you said it should be considered as harmless
  2. div + frameset can also be considered as harmless with a <body> prefix, but you said that's not the spec

I'm just confused on your position and points, for the above questions I don't think I can find a spec to convince myself.

cure53 commented 8 months ago

Again, please read what a body tag causes to happen and what a frameset causes to happen and then it becomes clear.

You can see clearly from the code that we don't have our own parser, we rely on the browser and lots of things happen there automatically. Once you understand the browsers behaviors with framesets, tables and many other crazy elements, the test cases will all make sense.

This is not about my position, this is about browser behavior. And about the browser reacting to strange HTML and us then sanitizing the result to prevent XSS.

JounQin commented 8 months ago

Again, I'm using DOMParser which is also just a browser thing.

I don't known if it is a browser version change caused difference?

This is not about my position, this is about browser behavior. And about the browser reacting to strange HTML and us then sanitizing the result to prevent XSS.

If you don't mind, can you help to understand that do you mean non standard tags as void is parsed by browser instead of dompurify itself? That seems different with DOMParser then.

Did you tried to update all browser version for the test cases?

https://github.com/cure53/DOMPurify/blob/d1e4f216664cc9e1ba5498a00b7d88a9cbd4c7f0/test/karma.custom-launchers.config.js#L145

It seems the Chrome version is very outdated.

cure53 commented 8 months ago

Please read the code, this will answer all your questions.

JounQin commented 8 months ago

If you mean I have to read the codes of the whole project before creating any issue which is unexpected, I don't know what to say then, good luck.

I'm here to try to make dompurify better, not an attacker.

cure53 commented 8 months ago

Blocked for ticket spam