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

Replace html_janitor with sanitize-html #1275

Open
sos-productions opened this issue Aug 18, 2020 · 5 comments
Open

Replace html_janitor with sanitize-html #1275

sos-productions opened this issue Aug 18, 2020 · 5 comments

Comments

@sos-productions
Copy link

Hi, I just wondered if sanitize-html will not be a good candidate to replace html-janitor

I say this because:

  1. html-janitor has a security issue XSS Note :
    This library has not been extensively tested. In particular versions prior to 2.0.3 are vulnerable to XSS attacks.
    This vulnerability is fully described there: https://hackerone.com/reports/308155
  2. sanitze-html is well suited for cleaning up HTML fragments such as those created by ckeditor and other rich text editors. It is especially handy for removing unwanted CSS when copying and pasting from Word.
    https://www.npmjs.com/package/sanitize-html

worth it or not?

@neSpecc
Copy link
Member

neSpecc commented Sep 2, 2020

It will break a backward capability of the API. So it can be done with the major release.

@literakl
Copy link

What is the purpose of having the HTML sanitizer on frontend? You must sanitize the source on backend. Is it for extra safety?

@khaydarov
Copy link
Member

What is the purpose of having the HTML sanitizer on frontend? You must sanitize the source on backend. Is it for extra safety?

yes

@rogueturnip
Copy link

For what it's worth, I was playing around with this. sanitize-html increases the build size by about 200KiB while using something like isomorphic-dompurify is about the same size.

I did a very simple implementation (ignores class and attribute) that didn't break the api by updating the clean method in sanitizer.ts

/**
 * Cleans string from unwanted tags
 * Method allows to use default config
 *
 * @param {string} taintString - taint string
 * @param {SanitizerConfig} customConfig - allowed tags
 * @returns {string} clean HTML
 */
export function clean(
  taintString: string,
  customConfig: SanitizerConfig = {} as SanitizerConfig
): string {
  const sanitizerConfig = {
    ALLOWED_TAGS: Object.keys(customConfig),
  };

  /**
   * API client can use custom config to manage sanitize process
   */
  // return sanitizeHtml(taintString, sanitizerConfig);
  return DOMPurity.sanitize(taintString, sanitizerConfig);
}

@dtdetwiller
Copy link

@neSpecc Hey, thanks for pointing me to this. Is there a plan to get this fixed any time soon?

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

No branches or pull requests

7 participants