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

Implement HTML component and cleanup Text component #999

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Jan 22, 2024

Instead of maintaining our own sanitation code, we can just use DOMPurify. I don't really trust our own code from being purely safe from all injections, and we're also restricting people from doing a lot of stuff in their text-fields, like styling which has been requested a few times.

I've also removed our dependency on preact-markup because I found it hard to justify, at the end of the day it was just doing dangerouslySetHtml with extra steps.

Closes #642
Closes #1004
Closes #1010

Bugs

Html component content isn't considered in getSchemaVariables

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jan 22, 2024
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from c052a0b to 5b67d11 Compare January 22, 2024 05:26
@Skaiir Skaiir changed the title feat: use thorough sanitation method on text component feat: move from allowlists to DOMPurify on text Jan 22, 2024
@Skaiir Skaiir requested a review from vsgoulart January 22, 2024 05:34
@Skaiir Skaiir changed the title feat: move from allowlists to DOMPurify on text feat: move from allowlists to DOMPurify on text component Jan 22, 2024
@Skaiir
Copy link
Contributor Author

Skaiir commented Jan 22, 2024

grafik

@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 23, 2024 10:02 Destroyed
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 23, 2024 10:30 Destroyed
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 23, 2024 11:57 Destroyed
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 5b67d11 to 3ab1a7d Compare January 25, 2024 15:51
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 25, 2024 15:51 Destroyed
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 3ab1a7d to f0ee02d Compare January 25, 2024 17:24
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 25, 2024 17:24 Destroyed
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from f0ee02d to f0efa3b Compare January 26, 2024 03:27
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 26, 2024 03:27 Destroyed
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from f0efa3b to 59e7f17 Compare January 26, 2024 03:50
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 26, 2024 03:50 Destroyed
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 59e7f17 to 8333f61 Compare January 26, 2024 03:53
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 26, 2024 03:53 Destroyed
@Skaiir Skaiir marked this pull request as ready for review January 26, 2024 04:21
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 26, 2024
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 26, 2024 04:21 Destroyed
@Skaiir Skaiir changed the title feat: move from allowlists to DOMPurify on text component Implement HTML component and cleanup Text component Jan 26, 2024
@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 768d064 to 8568751 Compare January 29, 2024 04:18
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 29, 2024 04:18 Destroyed
@Skaiir
Copy link
Contributor Author

Skaiir commented Jan 29, 2024

I added a few things since:

  • escaping html and css characters from template injection sites, to avoid unlikely but theoretically possible clickjacking attacks
  • allowing style tags, but forcing them into a scope via DOM modifications (this one is mostly just a tryout, happy to remove it if we don't like it)
  • schema changes (thanks @vsgoulart for the reminder)
  • minor style changes

Deployment: https://demo-642-relax-text-component--camunda-form-playground.netlify.app/

@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 29, 2024 09:33 Destroyed
@Skaiir
Copy link
Contributor Author

Skaiir commented Jan 29, 2024

Will cleanup the commits a little before I merge :)

@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 29, 2024 14:08 Destroyed
@vsgoulart
Copy link
Contributor

@Skaiir I think you need to update the build config to handle dompurify:

[vite]: Rollup failed to resolve import "dompurify" from "/Users/vinicius/repos/tasklist/client/node_modules/@bpmn-io/form-js-viewer/dist/index.es.js".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`

@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 29, 2024 14:32 Destroyed
Comment on lines 51 to 66
styleTags.forEach(styleTag => {
const scopedCss = styleTag.textContent
.split('}')
.map(rule => {
if (!rule.trim()) return '';
const [ selector, styles ] = rule.split('{');
const scopedSelector = selector
.split(',')
.map(sel => `#${styleScopeId} ${sel.trim()}`)
.join(', ');
return `${scopedSelector} { ${styles}`;
})
.join('}');

styleTag.textContent = scopedCss;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks if we use native CSS nesting

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, this valid CSS:

* {
  color: red;
  & h1 {
    color: blue;
  }
}

becomes this:

#fjs-form-0w50k6a-Field_0pv8mat-style-scope * { 
  color: red;
  & h1 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a new version of the CSS magic, with some decent test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the only change I actually made, the rest is just squashing / reordering some stuff.

@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 1dc9ab8 to 34a9385 Compare January 30, 2024 05:57
@github-actions github-actions bot temporarily deployed to demo-642-relax-text-component- January 30, 2024 05:57 Destroyed
*/
export function escapeHTML(html) {
return html.replace(/[&<>"'{};:]/g, match => {
switch (match) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we didn't use switches 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a map's better here anyways :)

Though an if else + would be insane :D

@Skaiir Skaiir force-pushed the 642-relax-text-component-security branch from 0f76b90 to cab8342 Compare January 30, 2024 11:51
@Skaiir Skaiir merged commit f3ac05c into develop Jan 30, 2024
9 of 12 checks passed
@Skaiir Skaiir deleted the 642-relax-text-component-security branch January 30, 2024 11:51
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants