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

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

Open
MatthiasvB opened this issue Jul 11, 2022 · 10 comments

Comments

@MatthiasvB
Copy link

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

@jfcere
Copy link
Owner

jfcere commented Jul 11, 2022

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!

@MatthiasvB
Copy link
Author

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
Copy link
Author

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

@jfcere
Copy link
Owner

jfcere commented Jul 11, 2022

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
Copy link
Owner

jfcere commented Jul 11, 2022

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

return this.sanitizer.sanitize(this.securityContext, marked) || '';

@MatthiasvB
Copy link
Author

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
Copy link
Author

But this is done already in the MarkdownService

return this.sanitizer.sanitize(this.securityContext, marked) || '';

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
Copy link
Author

MatthiasvB commented Jul 11, 2022

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
Copy link
Author

MatthiasvB commented Jul 11, 2022

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

@evilaliv3
Copy link

Did anyone identify a solution or how the library should be in the future improved to support this?

trusted-types are actually a very interesting addition to browser security and recently firefox have confirmed that next version will be supporting it as well.

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

3 participants