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

fix(tools-api): pasteConfig.tags now supports a sanitize config #2100

Merged
merged 30 commits into from
Nov 21, 2022

Conversation

robonetphy
Copy link
Member

@robonetphy robonetphy commented Jul 16, 2022

depends on next version of image tool and simple-image tool

@neSpecc
Copy link
Member

neSpecc commented Jul 25, 2022

I think, the best soultuion is to support our sanitization config at the pasteConfig.tags configuration:

  static get pasteConfig() {
    return {
      tags: [ 'img[src]' ],  // <-- all attributes except 'src' will be stripped
    };
  }

@jorgectf
Copy link
Contributor

Thank you for addressing this issue! FWIW, I'd suggest using tools focused on XSS sanitization such as dompurify.

@robonetphy
Copy link
Member Author

I think, the best soultuion is to support our sanitization config at the pasteConfig.tags configuration:

  static get pasteConfig() {
    return {
      tags: [ 'img[src]' ],  // <-- all attributes except 'src' will be stripped
    };
  }

@neSpecc I tried this solution but in that case, the configuration for sanitisation is as below:

{
    "p": true,
    "h1": true,
    "h2": true,
    "h3": true,
    "h4": true,
    "h5": true,
    "h6": true,
    "img[src]": true,
    "pre": true,
    "b": {},
    "i": {},
    "a": {
        "href": true,
        "target": "_blank",
        "rel": "nofollow"
    },
    "mark": {
        "class": "cdx-marker"
    },
    "code": {
        "class": "inline-code"
    },
    "br": {}
}```
and in that case, if I paste this correct configuration.
`<img src='https://cdn.pixabay.com/photo/2015/04/23/22/00/tree-736885__480.jpg'/>`
it's not working. the sanitized data comes empty and it pastes as text.

@robonetphy
Copy link
Member Author

robonetphy commented Jul 26, 2022

Thank you for addressing this issue! FWIW, I'd suggest using tools focused on XSS sanitization such as dompurify.

@jorgectf We are planning to integrate the library you recommended in our next release because accordingly, we need to update every tool with the API. So, this is just a hotfix right now.

@neSpecc
Copy link
Member

neSpecc commented Jul 26, 2022

I think, the best soultuion is to support our sanitization config at the pasteConfig.tags configuration:

  static get pasteConfig() {
    return {
      tags: [ 'img[src]' ],  // <-- all attributes except 'src' will be stripped
    };
  }

@neSpecc I tried this solution but in that case, the configuration for sanitisation is as below:

{
    "p": true,
    "h1": true,
    "h2": true,
    "h3": true,
    "h4": true,
    "h5": true,
    "h6": true,
    "img[src]": true,
    "pre": true,
    "b": {},
    "i": {},
    "a": {
        "href": true,
        "target": "_blank",
        "rel": "nofollow"
    },
    "mark": {
        "class": "cdx-marker"
    },
    "code": {
        "class": "inline-code"
    },
    "br": {}
}```
and in that case, if I paste this correct configuration.
`<img src='https://cdn.pixabay.com/photo/2015/04/23/22/00/tree-736885__480.jpg'/>`
it's not working. the sanitized data comes empty and it pastes as text.

You should not create a config manually, it should be collected from tools. For example, Image tool should change img with img[src] here

@robonetphy
Copy link
Member Author

I think, the best soultuion is to support our sanitization config at the pasteConfig.tags configuration:

  static get pasteConfig() {
    return {
      tags: [ 'img[src]' ],  // <-- all attributes except 'src' will be stripped
    };
  }

@neSpecc I tried this solution but in that case, the configuration for sanitisation is as below:

{
    "p": true,
    "h1": true,
    "h2": true,
    "h3": true,
    "h4": true,
    "h5": true,
    "h6": true,
    "img[src]": true,
    "pre": true,
    "b": {},
    "i": {},
    "a": {
        "href": true,
        "target": "_blank",
        "rel": "nofollow"
    },
    "mark": {
        "class": "cdx-marker"
    },
    "code": {
        "class": "inline-code"
    },
    "br": {}
}```
and in that case, if I paste this correct configuration.
`<img src='https://cdn.pixabay.com/photo/2015/04/23/22/00/tree-736885__480.jpg'/>`
it's not working. the sanitized data comes empty and it pastes as text.

You should not create a config manually, it should be collected from tools. For example, Image tool should change img with img[src] here

Yup, I have done the same things I am just doing console.log inside the Paste module as debugging. So I know it will take updated value from simple-image tool

@robonetphy robonetphy changed the title Temporary Resolutions of XSS Problem Temporary resolutions of XSS problem Aug 3, 2022
Copy link
Member

@gohabereg gohabereg left a comment

Choose a reason for hiding this comment

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

Would be great to cover it with unit tests

src/components/modules/paste.ts Outdated Show resolved Hide resolved
src/components/modules/paste.ts Outdated Show resolved Hide resolved
src/components/modules/paste.ts Outdated Show resolved Hide resolved
src/components/modules/paste.ts Outdated Show resolved Hide resolved
src/components/modules/paste.ts Outdated Show resolved Hide resolved
src/components/modules/paste.ts Outdated Show resolved Hide resolved
@neSpecc
Copy link
Member

neSpecc commented Nov 11, 2022

add a changelog describing fix and API change please

@neSpecc
Copy link
Member

neSpecc commented Nov 11, 2022

The solution still does not work as expected. If a Tool does not specify the sanitizer config, all attributes should be removed. For now, it doesn't work.

I'm digging into it.

@neSpecc
Copy link
Member

neSpecc commented Nov 11, 2022

Seems ok for now. Will test it again tomorrow

@robonetphy
Copy link
Member Author

hey @neSpecc, Pls check and let me know when to merge.

@neSpecc
Copy link
Member

neSpecc commented Nov 21, 2022

Sorry, the last test became a problem and I've spent some time on it. I've found the bug with our sanitizing dependency HTMLJanitor. It has a bug with Table sanitizing (guardian/html-janitor#3) so I've added a few lines to handle that case. Now all the tests work fine.

@neSpecc neSpecc changed the title Temporary resolutions of XSS problem fix(tools-api): pasteConfig.tags now supports a sanitize config Nov 21, 2022
@neSpecc neSpecc merged commit f659015 into next Nov 21, 2022
@neSpecc neSpecc deleted the fix/xss-problem branch November 21, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants