-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: added DOMPurify to prevent xss #1894
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
Thank you for following the naming conventions for pull request titles! 🙏 |
apps/formbricks-com/components/dummyUI/HtmlBody.tsxWhile the use of DOMPurify is a good step towards ensuring the security of the application, it would be beneficial to configure it to use a safe subset of HTML to further enhance security. This can be done by passing a configuration object to the const clean = DOMPurify.sanitize(dirty, { ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a'] });
packages/surveys/src/components/general/HtmlBody.tsxWhile the current implementation is correct, it could be optimized by avoiding the dynamic import of DOMPurify in the import DOMPurify from 'dompurify';
|
|
||
export default function HtmlBody({ htmlString, questionId }: { htmlString: string; questionId: string }) { | ||
return ( | ||
<label | ||
htmlFor={questionId} | ||
className="fb-block fb-font-normal fb-leading-6 text-sm text-slate-500 dark:text-slate-300" | ||
dangerouslySetInnerHTML={{ __html: cleanHtml(htmlString) }}></label> | ||
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(htmlString) }}></label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DOMPurify.sanitize function has been configured to only allow a safe subset of HTML tags. This enhances the security of the application by preventing the execution of potentially harmful HTML code.
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(htmlString) }}></label> | |
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(htmlString, { ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a'] }) }} |
import { useEffect, useState } from "react"; | ||
|
||
interface HtmlBodyProps { | ||
htmlString: string | undefined; | ||
questionId: string; | ||
} | ||
|
||
export default function HtmlBody({ htmlString, questionId }: HtmlBodyProps) { | ||
const [safeHtml, setSafeHtml] = useState(""); | ||
|
||
useEffect(() => { | ||
if (htmlString) { | ||
import("dompurify").then((DOMPurify) => { | ||
setSafeHtml(DOMPurify.sanitize(htmlString)); | ||
}); | ||
} | ||
}, [htmlString]); | ||
|
||
export default function HtmlBody({ htmlString, questionId }: { htmlString?: string; questionId: string }) { | ||
if (!htmlString) return null; | ||
|
||
return ( | ||
<label | ||
htmlFor={questionId} | ||
className="fb-htmlbody" // styles are in global.css | ||
dangerouslySetInnerHTML={{ __html: cleanHtml(htmlString) }}></label> | ||
dangerouslySetInnerHTML={{ __html: safeHtml }}></label> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamic import of DOMPurify in the useEffect
hook has been replaced with a static import at the top of the file. This change optimizes the code by avoiding unnecessary network requests, improving performance.
import { useEffect, useState } from "react"; | |
interface HtmlBodyProps { | |
htmlString: string | undefined; | |
questionId: string; | |
} | |
export default function HtmlBody({ htmlString, questionId }: HtmlBodyProps) { | |
const [safeHtml, setSafeHtml] = useState(""); | |
useEffect(() => { | |
if (htmlString) { | |
import("dompurify").then((DOMPurify) => { | |
setSafeHtml(DOMPurify.sanitize(htmlString)); | |
}); | |
} | |
}, [htmlString]); | |
export default function HtmlBody({ htmlString, questionId }: { htmlString?: string; questionId: string }) { | |
if (!htmlString) return null; | |
return ( | |
<label | |
htmlFor={questionId} | |
className="fb-htmlbody" // styles are in global.css | |
dangerouslySetInnerHTML={{ __html: cleanHtml(htmlString) }}></label> | |
dangerouslySetInnerHTML={{ __html: safeHtml }}></label> | |
); | |
} | |
import DOMPurify from 'dompurify'; | |
import { useEffect, useState } from "react"; | |
interface HtmlBodyProps { | |
htmlString: string | undefined; | |
questionId: string; | |
} | |
export default function HtmlBody({ htmlString, questionId }: HtmlBodyProps) { | |
const [safeHtml, setSafeHtml] = useState(""); | |
useEffect(() => { | |
if (htmlString) { | |
setSafeHtml(DOMPurify.sanitize(htmlString)); | |
} | |
}, [htmlString]); | |
if (!htmlString) return null; | |
return ( | |
<label | |
htmlFor={questionId} | |
className="fb-htmlbody" // styles are in global.css | |
dangerouslySetInnerHTML={{ __html: safeHtml }}></label> | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhruwang thank you very much for this security fix :-) just one comment:
} | ||
|
||
export default function HtmlBody({ htmlString, questionId }: HtmlBodyProps) { | ||
const [safeHtml, setSafeHtml] = useState(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a useMemo
instead?
const safeHtml = useMemo(() => DOMPurify.sanitize(htmlString), [htmlString])
@Dhruwang Did the build succeed in your testing?
Can you please take a look :-) Ideally DomPurify will be included into the bundle files of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhruwang thank you for making the changes 😊💪
Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
What does this PR do?
Fixes 1657
How should this be tested?
Test editors
Checklist
Required
pnpm build
console.logs
git pull origin main
Appreciated