ngx-markdown icon indicating copy to clipboard operation
ngx-markdown copied to clipboard

Is sanitation actually working? Component does not work when content-security-policy is enforced

Open MatthiasvB opened this issue 2 years ago • 10 comments

I'm working on an app that is served with header

content-security-policy: object-src 'none'; script-src-elem 'self'; base-uri 'self'; require-trusted-types-for 'script'

This should not be an issue if angular's dom sanitizer was used correctly. However, the application breaks (in Chromium browsers) claiming

ERROR TypeError: Failed to set the 'innerHTML' property on 'Element': This document requires 'TrustedHTML' assignment.

After hours of debugging and trying to find the right headers to make this work, I've realized that ngx-markdown simply either does not sanitize the generated HTML, or at least not in a fashion that marks this as a trusted type.

The solution is simple: Don't use the component, but the pipe! Since Angular will automatically sanitize the HTML, when I assign it to [innerHTML], it really does't matter what kind of sanitation this library does or does not do. Suddenly, the app is back to working.

Please fix this, as not being able to use the other methods for anchoring HTML in the template will break many applications and lead to hours of debugging time, as more and more people will start using this security mechanism

MatthiasvB avatar Jul 11 '22 10:07 MatthiasvB

Hi @MatthiasvB,

Could you provide a reproduction example? You can use this StackBlitz sample as a start if you want: https://stackblitz.com/edit/ngx-markdown or provide a GitHub repository.

Thank you!

jfcere avatar Jul 11 '22 11:07 jfcere

Not really, I think. The issue depends on the server configuration. Only if the server sets the header mentioned above will the problem occur. But then always when you use the <markdown/> component.

I doubt I can reproduce this on stackblitz

MatthiasvB avatar Jul 11 '22 11:07 MatthiasvB

But here a hint how it may be fixable:
The error occurs on line 220 of ngx-markdown.js:

this.element.nativeElement.innerHTML = compiled;

I believe if this was replaced with someting like

this.element.nativeElement.innerHTML = sanitizer.sanitize(compiled);

it may work. I'm not deeply into how the sanitizer works, though

MatthiasvB avatar Jul 11 '22 11:07 MatthiasvB

I am not familiar with the header you mention. If have a solution to fix it please feel free to open a PR otherwise I am not sure how to reproduce the problem 😕

jfcere avatar Jul 11 '22 11:07 jfcere

But here a hint how it may be fixable:
The error occurs on line 220 of ngx-markdown.js:

this.element.nativeElement.innerHTML = compiled;

I believe if this was replaced with someting like

this.element.nativeElement.innerHTML = sanitizer.sanitize(compiled);

it may work. I'm not deeply into how the sanitizer works, though

But this is done already in the MarkdownService

https://github.com/jfcere/ngx-markdown/blob/7e7cfe755b5f322994dab9d6d3c2fb155a51561a/lib/src/markdown.service.ts#L168

jfcere avatar Jul 11 '22 11:07 jfcere

It's a fairly now header devised by Google and only implemented by Chromium browsers. However, it's usage is bound to increase, and thus the severity of this issue. Here is some info regarding it

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/trusted-types https://dev-academy.com/trusted-types-to-prevent-dom-xss-in-angular/

MatthiasvB avatar Jul 11 '22 11:07 MatthiasvB

But this is done already in the MarkdownService

https://github.com/jfcere/ngx-markdown/blob/7e7cfe755b5f322994dab9d6d3c2fb155a51561a/lib/src/markdown.service.ts#L168

And the string is final at this point? Not changing afterwards in any way? Then it would be interesting how "marking the string as sanitized" actually works, and whether this can be done right there. At least it's good to know that sanitation does occur, what's missing is just marking it as sanitized

MatthiasvB avatar Jul 11 '22 11:07 MatthiasvB

In an effort to make experiments myself I have cloned the repository. npm i comes up with dependency conflicts?! Well, I'll try with --force

MatthiasvB avatar Jul 11 '22 11:07 MatthiasvB

I have checkout out the project and tried to fix it. It's not easy, as the problem extends into dependencies.

What "works" per instance is to use the package DOMPurify, and to do:

// replace
someElement.innerHTML = someHtmlString;

//with
someElement.innerHTML = DOMPurify.sanitize(someHtmlString, { RETURN_TRUSTED_TYPE: true }) as unknown as string;

however, DOMPurify is not exactly up to date, requires "allowSyntheticDefaultImports": true in the tsconfig.json and returns some weird type which does not play well with the innerHTML.

Given the depth to which this problem permeates, I will not attempt to fix this. @jfcere you probably have way better overview over this project. Do you think it would be possible to refactor this to make sanitation occur automatically by always and only using Angular's [innerHTML] property binding?

Here's a guide to reproduce the issue:

  1. In Chrome, install the extension "Requestly"
  2. In the extension, create rule
    • Add
    • Response
    • Header: content-security-policy
    • Value: object-src 'none'; script-src-elem 'self'; base-uri 'self'; require-trusted-types-for 'script';
    • URL
    • Contains
    • localhost:4200
  3. Run the sample app
  4. See it error

MatthiasvB avatar Jul 11 '22 12:07 MatthiasvB