Text: Support dangerouslySetInnerHTML

[cseas]

Use-case: https://github.com/razorpay/frontend-website/pull/2198#discussion_r1243105412

[chaitanyadeorukhkar]

For such use cases you should be doing. Closing this. Feel free to reopen it if this doesn't satisfy the use-case.

      <Text size="large">
        *International Debit & Credit Cards will be charged at <Text weight='bold'>3%</Text> per transaction
      </Text>

[cseas]

The text we're rendering comes from a json object as a string. Refer: https://github.com/razorpay/frontend-website/blob/master/src/shared/pricing/MY.ts#L209

This is to enable the same code to render content for different countries (right now the same page renders content for both India and Malaysia websites). This use-case is very common in frontend-website and the team had been using dangerouslySetInnerHTML with Chakra UI so far.

Above suggested solution won't work since we can't hardcode what part of the text is supposed to be, say, bold. The text could be completely different based on country and may not have a bold part at all.

[chaitanyadeorukhkar]

@cseas discussed with the team, adding dangerouslySetInnerHTML would be a security concern. We already have internal tools that flag usages of dangerouslySetInnerHTML in our codebases. Adding support for this on Blade would not make sense given it could lead to a security exploit.

@anuraghazra suggested exploring things like Trans components or something similar in whichever i18n library you are using to solve this.

[cseas]

frontend-website doesn't use react-i18next. Please suggest a working alternative.

We're only doing a migration exercise, code that uses dangerouslySetInnerHTML has been in production in frontend-website for more than a year now so the question of whether we should ship this or not is not part of this issue. The production code is on Chakra UI, we need a Blade alternative that serves the same use case. Since the restriction is coming from Blade end, please help in exploring alternative solutions.

[nashcheez]

I am surprised the prop has existed on frontend-website for so long. Our security team advises against the usage of dangerouslySetInnerHTML citing possibilities of XSS attacks. Bhadra catches the occurrences and reports it as a security issue, take this ref example for admin-dashboard.

Just because something has existed doesn't make it the right approach, if it's indeed a security concern then don't think exposing it in the system is a valid ask either. Please explore other alternative solutions from a product standpoint or offer a solution in the system that adheres to our security guidelines.

[cseas]

We might be overanalysing this 😅 dangerouslySetInnerHTML is only a concern when we allow an external source to inject html onto our page. In the case of frontend-website, all the text is sourced at build time, there's no network call or input component for some attacker to inject anything. It's just 2 plain json objects we hardcode as part of the build process. Rest assured, if we ever encounter a case where we use that prop for an externally fetched string, we'll sanitise it before rendering if needed.

Security concerns are the product team's concern. The design system concern here is I'm not able to render bold text from a string using Blade.

See this page for example: https://rzp.curlec.com/payment-pages/ If you view the page source, all text here comes rendered from the server itself. That's the text this issue is about. There's no client-side network call for the json object we fetch the data from. This isn't a security issue.

[chaitanyadeorukhkar]

@cseas, security concerns are all of our concerns :) For now we have a static JSON which very well could be a server-driven JSON in the future. Supporting dangerouslySetInnerHTML isn't a valid ask as it could compromise our product's security (which we care deeply about).

The design system lets you render bold text. This is a product-specific task where you need to analyze how to transform your JSON string to make it renderable with a react component. We are here if you need help with brainstorming, discussing, or adding enhancements to existing components to make this possible.

[cseas]

I just noticed the team has already built a custom typography component as a workaround. Not in favour of building a custom alternative here. Any way we can achieve this with Blade's typography without using a third-party lib? 😕

[cseas]

Looks like the security team's warning is coming from this Semgrep rule.

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 sanitize your HTML.

The check mentions the vulnerability comes when we mix this with user input, which we're sure is not the situation in the i18n use-case. But to cover general projects who might use the prop unknowingly, we can add support for this prop with an ESLint rule in Universe that'll force the devs to sanitize the HTML, which is the same thing Semgrep is recommending too.

It'll add a slight bit of processing overhead to use-cases that don't need it but in turn adds a safeguard covering all use-cases. Semgrep doesn't report warnings on sanitized use of dangerouslySetInnerHTML either. Thoughts?

We've only had a couple of instances where we needed some part of text to be bold so it's not a burning issue, we've just hardcoded the text without loading it from json for now. Need to brainstorm a solution for long term though.

[chaitanyadeorukhkar]

The preferable approach is to restrict this to ensure it can't be misused and lead to a security issue. If a really valid use-case comes up, our approach should be: allow it to be a custom component for that specific use-case. We shouldn't add this unless it becomes an org-wide pattern & requirement

For the use-case of i18n, using dangerouslySetInnerHTML shouldn't be a solution. There should be a transformer that transforms your JSON into appropriate react components. I would suggest exploring Trans components like solution - this will ensure we don't introduce any security risk and have a proper long term solution for all such use-cases.

[cseas]

Ack, will discuss the possibility of adopting react-i18next with the TopF team if more complex use-cases arise. cc: @iAmServerless @anshulsahni