Skip to content
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

[search-parts] Sanitized HTML in templates + CSS prefixing #172

Merged
merged 6 commits into from
Apr 7, 2020

Conversation

FranckyC
Copy link
Collaborator

@FranckyC FranckyC commented Apr 3, 2020

  • Added sanitization with DOMPurify for every dangerouslySetInnerHTML
  • Added CSS prefixing with Web Part instance ID for custom CSS in templates. Support regular CSS rules + Media queries.

…DOMPurify to prevent XSS attacks. Now we use 'data-' attributes for all web components + auto CSS prefix for custom CSS classes based on the Web Part instance ID.
- Added support for CSS Media rule.
@FranckyC
Copy link
Collaborator Author

FranckyC commented Apr 3, 2020

@wobba If you could review this one to make sure we are aligned.

@@ -145,6 +145,14 @@ If provided layouts don't meet your requirements, you can modifiy them or start

![Edit Template](../images/edit_template.png)

### Styling

You can write your own CSS styles inside templates. However, all CSS rules (including `@media` rules) will be prefixed automatically by an unique ID according to the follwoing pattern (**pnp-modern-search-template_\<Web Part instance ID\>**) to make sure styles are isolated fronm other Web Parts on the page.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling error "fronm"

attr = matches[1];

// Booleans
if (/^(true|false)$/.test(value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this regex be case insensitive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right this could be /i

@wobba
Copy link
Collaborator

wobba commented Apr 5, 2020

Looks good, but I will do some more tests. Will the ''data" prefix be breaking? If so we should consider how to version it, or if we can auto handle upgrading.

if (template) {

// Sanitize the template HTML
template = template ? this._domPurify.sanitize(`${template}`) : template;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need inline if when you have if check on line above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not ;)

@FranckyC
Copy link
Collaborator Author

FranckyC commented Apr 5, 2020

If people used custom web components, yes it will break. I can remove the check of data- attributes if too critical.

@wobba
Copy link
Collaborator

wobba commented Apr 5, 2020

For custom components it should be ok. What if someone used the oob components manually in a template? Maybe we need to mark some functionality as preview to iron out issues when weintroduce it, and still keep versioning simple :)

Feel free to merge this one.

Spelling fix
Added regex case insensitive
Removed unneeded check
@wobba wobba merged commit a9e35aa into develop Apr 7, 2020
@wobba wobba deleted the feature/sanitize-html branch April 7, 2020 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants