semgrep / semgrep-rules

Semgrep rules registry
https://semgrep.dev/registry
Other
809 stars 396 forks source link

Sanitized HTML still triggering dangerouslysetinnerhtml even with dompurify #2168

Closed sshadmand closed 2 years ago

sshadmand commented 2 years ago

I followed the instructions to use DOMPurify to sanitize the HTML, but it is still triggering the issue?

     typescript.react.security.audit.react-dangerouslysetinnerhtml.react-dangerouslysetinnerhtml
        Detected setting HTML from code. This is risky because it’s easy to inadvertently expose
        your users to a cross-site scripting (XSS) attack. This can lead to attackers accessing
        sensitive information. Instead, do this without dangerouslySetInnerHTML or use DOMPurify to
        santize your HTML.
        Details: https://sg.run/rAx6

         26┆ return <div dangerouslySetInnerHTML={{__html: sanitize(content)}} />

So, my response was...

import {sanitize} from 'dompurify'
...
const SafeCardContent = ({html}) => {
  return <div dangerouslySetInnerHTML={{__html: sanitize(html)}} />
}
import DOMPurify from 'dompurify'
const SafeCardContentAlt = ({html}) => {
  return <div dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(html)}} />
}

But still getting that finding/response from semgrep. Confused as to how I can solve the warning. Any suggestions?

r2c-demo commented 2 years ago

This issue is synced in Linear at https://linear.app/r2c/issue/PA-1567/sanitized-html-still-triggering-dangerouslysetinnerhtml-even-with. Note: this link is for r2c use only and is not accessible publicly.

LewisArdern commented 2 years ago

Hey @sshadmand once https://github.com/returntocorp/semgrep-rules/pull/2136 gets merged, this issue will no longer flag, currently the rule looks for any reference to dangerouslySetInnerHTML. if you scan your code with https://raw.githubusercontent.com/returntocorp/semgrep-rules/f682024a531ffaa305a453ee10a12b7a2027b689/typescript/react/security/audit/react-dangerouslysetinnerhtml.yaml it will no longer flag, if it does let me know!

sshadmand commented 2 years ago

Thanks for the reply @LewisArdern and ref

Will run and let you know :-)

sshadmand commented 2 years ago

No dice.

Ran...

semgrep --config ./rules.yaml ./src

where rules.yaml is https://raw.githubusercontent.com/returntocorp/semgrep-rules/f682024a531ffaa305a453ee10a12b7a2027b689/typescript/react/security/audit/react-dangerouslysetinnerhtml.yaml

Received...

  src/components/common/Correction/Guide.js 
     react-dangerouslysetinnerhtml
        Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently
        expose users to cross-site scripting (XSS) attacks if this comes  from user-provided input.
        If you have to use dangerouslySetInnerHTML, consider  using a sanitization library such as
        DOMPurify to santize your HTML.

         25┆ return <CardContent dangerouslySetInnerHTML={{__html: sanitize(content)}} />

Where sanitize is...

import {sanitize} from 'dompurify'
...
const SafeCardContent = ({html}) => {
  return <div dangerouslySetInnerHTML={{__html: sanitize(html)}} />
}
LewisArdern commented 2 years ago

@sshadmand I see why now.

This will fix it https://semgrep.dev/s/kPe7 this will go into the latest update so this issue will go away soon!

Feel free to give https://raw.githubusercontent.com/returntocorp/semgrep-rules/390aa71870bb29237008f38b4c8fe5b392991648/typescript/react/security/audit/react-dangerouslysetinnerhtml.yaml a whirl

stale[bot] commented 2 years ago

This issue is being marked stale because there hasn't been any activity in 14 days and either it wasn't prioritized or its priority is high. Please apply the priority:low label or one of the other exempt labels listed in .github/stale.yml if the issue is not urgent.

LewisArdern commented 2 years ago

This was resolved with https://github.com/returntocorp/semgrep-rules/pull/2136