feat(api): sanitize product description #2050
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f2b31d8:
|
icazevedo
requested review from
hellofanny,
ArthurTriis1 and
lucasfp13
and removed request for
a team and
ArthurTriis1
| ...product, | ||
| description: product.description | ||
| ? sanitizeHtml(product.description) | ||
| : product.description, |
There was a problem hiding this comment.
Which product.description values should not be sanitized but escaped?
There was a problem hiding this comment.
We check if product.description is something, if so we sanitize that, but the else case is not clear for me and that's my question: why do we still need to return the product.description if we check if it exists?
There was a problem hiding this comment.
Because it can possibly have different falsy states, such as null or undefined. I don't want to change the default falsy value returned by the headless API, so I return whatever it returns me in these case.
| @@ -77,7 +77,12 @@ function ProductDescription({ | |||
| > | |||
| <UIAccordionButton>{title}</UIAccordionButton> | |||
| <UIAccordionPanel> | |||
| <p className="text__body">{content}</p> | |||
| <div | |||
| // Applies display: contents through FastStore UI | |||
There was a problem hiding this comment.
nit: I think we don't need to explain the applicability of the data-attr here.
There was a problem hiding this comment.
Normally i'd agree but I think this is relevant because we're using a div here. It is expected for a div to have margin, padding, etc, but not in this case. It's a very specific behavior that's not common, so I thought it was worth the comment.
## What's the purpose of this pull request? This PR fixes [this](https://github.com/vtex/faststore/actions/runs/6458890810/job/17533535581) issue that happened while publishing packages due to incompatible `eslint-plugin-prettier` versions with prettier 3.x. ## How it works? In #2050 I upgraded the prettier version to 3.x. That caused the issue because `tsdx` has a dependency on an old `eslint-plugin-prettier` version, which is incompatible with `prettier@3`. I'm rolling back the upgrade and fixing the prettier version on the major `2` which makes it difficult for others to make the same mistake as me. In the future, we remove all uses of `tsdx` as it is no longer supported and has gone 3 years without updates. ## How to test it? By merging this! ## References prettier/eslint-plugin-prettier#562 Thanks @hellofanny for noticing the error and investigating it!
What's the purpose of this pull request?
This PR adds support for HTML in product descriptions.
How it works?
FastStore API sanitizes the product description in the backend. This ensures no possibly vulnerable tags, such as
scripttags are sent and executed on the frontend.The frontend now inserts the description as innerHTML, which allows it to be properly rendered.
How to test it?
Browse the two deploy previews below. Note on how one of the product descriptions, the HTML renders as text, and, in the other that uses this version of the packages it is interpreted as actual HTML.
Starters Deploy Preview
https://sfj-5d275d1--starter.preview.vtex.app/unbranded-frozen-pizza-1670312/p
https://starter.vtex.app/unbranded-frozen-pizza-1670312/p
References
https://www.npmjs.com/package/sanitize-html