search

google / peoplemath

Web application for team planning
Apache License 2.0
126 stars 35 forks source link

DOM text reinterpreted as HTML Update markdownify.pipe.ts #279

Closed Shivam7-1 closed 5 months ago

Shivam7-1 commented 5 months ago

By using innerText, it will avoid the risk of HTML injection, as these properties automatically escape any HTML special characters in the provided text. This helps prevent cross-site scripting (XSS) vulnerabilities by treating the input as plain text rather than interpreted HTML. Always be cautious when dealing with user input or dynamic content to prevent security risks.

amdw commented 5 months ago

Thanks for sending this pull request.

Unfortunately, this change causes test failures:

Error: src/app/markdown/markdownify.pipe.ts:36:31 - error TS2339: Property 'innerText' does not exist on type 'Element'.

To reproduce the issue, run npx ng test --browsers ChromeHeadless.

Shivam7-1 commented 5 months ago

Hi @amdw Thanks For Reviewing PR I just Check and get to know that HTMLElement property not Present to access innertext

Now I had Done and Commited changes Now it will work perfectly Fine

Thanks

amdw commented 5 months ago

Hi! Thanks for making the tests pass.

I played around with your change a little today, and unfortunately I think it breaks formatting inside links in nolinks mode.

In f8e8c9e3e9e8b835fc8b7838a1e9d5e29e6dbfae I added a couple of test cases to illustrate the problem. These pass on master but fail when merged with your changes:

MarkdownifyPipe should support formatting inside links in nolinks mode FAILED
        Error: Expected '<u>a &lt;strong&gt;formatted&lt;/strong&gt; link</u>' to equal '<u>a <strong>formatted</strong> link</u>'

MarkdownifyPipe should handle unsupported tags inside links in nolinks mode FAILED
        Error: Expected '<u>an &lt;blink&gt;unsupported&lt;/blink&gt; tag</u>' to equal '<u>an unsupported tag</u>'.

The problem is that by setting innerText instead of innerHTML, you're destroying any formatting the user did inside the link text, which is something somebody might reasonably want to do.

I understand you're trying to prevent XSS, but this code is running inside a domPurify hook under a very restrictive config, which should already have sanitised the HTML before it reaches this stage. Therefore I'm not convinced there is actually any XSS problem here.

Can you demonstrate any real XSS vulnerability by adding a test case that shows how to inject unsafe HTML into the DOM via this pipe as currently written?

If not then I'm inclined not to merge this PR, as it would break useful functionality for no clear benefit.