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

Title: Sanitization removes valid iframe attributes and changes attribute order #930

Closed Mani9398 closed 5 months ago

Mani9398 commented 5 months ago

Description: I've encountered an issue with DOMPurify's sanitization process where valid attributes on iframe elements are being removed, and the order of attributes in the output is being changed. This behavior is unexpected and may lead to unintended changes in the rendered HTML. I believe this issue needs to be addressed to ensure that DOMPurify accurately preserves valid attributes and maintains the order of attributes as specified in the input HTML.

Steps to Reproduce:

Provide a sample HTML input containing an iframe element with valid attributes such as src, width, height, frameborder, allow, and allowfullscreen. Apply DOMPurify's sanitization process to the input HTML. Compare the output HTML with the input HTML to identify any missing attributes or changes in attribute order. Expected Behavior: DOMPurify should preserve all valid attributes on iframe elements and maintain the order of attributes as specified in the input HTML.

Actual Behavior: Attributes such as frameborder, allow, and allowfullscreen are being removed from the output HTML, and the order of attributes is being changed.

Additional Information:

Version of DOMPurify used: [Insert Version Number] Sample Input HTML:html

Output HTML after sanitization (incorrect):html This issue affects [insert specific use case or application where DOMPurify is being used]. Proposed Solution: Investigate the sanitization process to identify why valid attributes are being removed and the order of attributes is being changed. Implement changes to ensure that all valid attributes are preserved, and the order of attributes is maintained in the output HTML. Impact: This issue may impact applications that rely on DOMPurify for HTML sanitization, potentially causing unexpected rendering behavior or security vulnerabilities due to missing or altered attributes. Priority: [High] - Depending on the severity and impact of the issue.
cure53 commented 5 months ago

Can you please format your ticket a bit, it is very hard to parse, thanks.

Mani9398 commented 5 months ago

this is the html we are passing for DOMPurify.sanitize <iframe width="110" height="160" src="What advice would you give to be successful on the job?" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>

this is the output after sanitization

<iframe src="What advice would you give to be successful on the job?" width="110" height="160" ></iframe>

Sanitize removing valid attributes like 'allow','allowfullscreen' .you can see missing attributes in output and order of attributes after sanitize getting changed.

please check above input and ouput and provide better solution.

cure53 commented 5 months ago

Can you post your DOMPurify config? Are those attributes allow-listed properly?

As far as I can see, all works as expected if done right:

DOMPurify.sanitize(
  '<iframe width="110" height="160" src="What advice would you give to be successful on the job?" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>', 
  {ADD_TAGS: ['iframe'], ADD_ATTR: ['allow', 'frameborder', 'allowfullscreen']}
) 
cure53 commented 5 months ago

the order of attributes in the output is being changed

This part is expected and necessary and will not be changed anytime soon.

Mani9398 commented 5 months ago

config: { ADD_ATTR: ['src', 'width', 'height', 'frameborder', 'allow', 'allowfullscreen'] } this is the config we are passing but html attributes are not limited so that we can add in a list Manually and pass as a config.please provide suggestion that santize should allow all valid html attributes and also is there any option get order of the output properly .

for allowfullscreen attribute it is giving allowfullscreen="" like this so when we are comparing the sanitized html with input html we are getting false.

please provide solution for the above.

cure53 commented 5 months ago

please provide solution for the above.

That cannot be easily done by the core library and is also not its job :slightly_smiling_face: We protect against XSS, we are not a markup manager that caters to each individual need how HTML should look like.

However.

For that exact purpose, meaning to get full control over the HTML, we offer the functionality of hooks. With those you can achieve what you are looking for and customize every bit of sanitization output. Check the hook examples in the demo folder.

ZJaf commented 5 months ago

Hello there, We have exact same issue as Mani when they mentioned: for allowfullscreen attribute it is giving allowfullscreen="" I am writing hooks for us, I managed to whitelist them(yey) but I can not get the equal to empty out! any help would be greatly appreciated.

cure53 commented 5 months ago

Nothing we can do here, this is added by the browser.