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.

Sign up for GitHub

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

Jump to bottom

"WARNING: sanitizing HTML stripped some content" when no content stripped #9392

Closed
wkwiatek opened this issue Jun 21, 2016 · 13 comments
Closed

"WARNING: sanitizing HTML stripped some content" when no content stripped #9392

wkwiatek opened this issue Jun 21, 2016 · 13 comments
Assignees
@mprobst
Labels
area: security Issues related to built-in security features, such as HTML sanitation workaround1: obvious

Comments

@wkwiatek
Copy link
Contributor

wkwiatek commented Jun 21, 2016 •
edited

I'm submitting a ... (check one with "x")

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
Escaped characters passed to sanitize are reported with warning.

Expected/desired behavior
No warning for preescaped html.

Reproduction of the problem
http://plnkr.co/edit/ip37nVScMDxE9KaoP3Ng?p=preview

After this line https://github.com/angular/angular/blob/master/modules/%40angular/platform-browser/src/security/html_sanitizer.ts#L243 unsafeHtml is mutated and contains parsedHtml and then in fact parsedHtml is compared to safeHtml (https://github.com/angular/angular/blob/master/modules/%40angular/platform-browser/src/security/html_sanitizer.ts#L261)

Please tell us about your environment:
Mac OS X 10.11.5

  • Angular version: 2.0.0-rc.2
  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web
    all
  • Language: [all | TypeScript X.X | ES6/7 | ES5 | Dart]
    TypeScript 1.8.10
@zoechi
Copy link
Contributor

zoechi commented Jun 21, 2016

See http://stackoverflow.com/questions/37076867/in-rc-1-some-styles-cant-be-added-using-binding-syntax/37076868#37076868

http://plnkr.co/edit/WdrRVyHr6WUCwMCwhH9F?p=preview

@wkwiatek
Copy link
Contributor Author

I know about "marking as trust" but I guess it's not a case here.

Consider this example: http://plnkr.co/edit/U1N8xgJOUWyemz8oS0RW?p=preview
I'm passing a simple string test here and it's perfectly fine because nothing was really stripped.

In case of the unicode icon there should be no warning because I've passed encoded string (value passed to innerHtml input and finally rendered are exactly the same) so nothing was stripped in fact.

However if I do this: http://plnkr.co/edit/ZSto9Gl03ZzNGVF8E8C2?p=preview then warning is what I'd expect.

So to the point - I think the message is wrong in case you pass encoded value to sanitize.

@zoechi
Copy link
Contributor

zoechi commented Jun 21, 2016

I see, I leave that to the Angular team then.

@vicb vicb added the area: security Issues related to built-in security features, such as HTML sanitation label Jun 21, 2016
@vicb
Copy link
Contributor

vicb commented Jun 21, 2016

@mprobst could you please take a look ?

@mprobst
Copy link
Contributor

mprobst commented Jun 21, 2016

I think this is inherent in how we sanitize. We're using the browser's HTML parser, which of course turns escapes such as 🚀 in the example into their Unicode code point equivalents (or UTF-16, really). After parsing and re-serializing, Angular sees a string that does not match byte by byte, i.e. the escape has been replaced with the actual character (🚀), and thus prints the warning.

Fixing this won't be possible without re-implementing half an HTML parser and expensively re-scanning the string. Given that the message is entirely harmless and only slightly misleading, and given that this is probably somewhat of a corner case, I think the effort is not worth it.

Does that make sense? Please feel free to re-open if this actually causes an issue.

@mprobst mprobst closed this as completed Jun 21, 2016
wkwiatek added a commit to wkwiatek/angular that referenced this issue Jun 21, 2016
@wkwiatek
fix(security): no warning when sanitizing escaped html (angular#9392)
a48f9d8
@wkwiatek wkwiatek mentioned this issue Jun 21, 2016
Merged
3 tasks
@wkwiatek
Copy link
Contributor Author

@mprobst I don't think there is a problem with sanitization but with the error message condition. I've added PR because code shows much more than words. Let me know what do you think about it.

@mprobst
Copy link
Contributor

mprobst commented Jun 21, 2016

@wkwiatek I understand, but I'm surprised the PR fixes this. Let's discuss there.

wkwiatek added a commit to wkwiatek/angular that referenced this issue Jun 23, 2016
@wkwiatek
fix(security): no warning when sanitizing escaped html (angular#9392)
2d30aba
mprobst pushed a commit that referenced this issue Jun 23, 2016
@wkwiatek @mprobst
fix(security): no warning when sanitizing escaped html (#9392) (#9413)
98cef76
@oocx
Copy link
Contributor

oocx commented Jul 12, 2016

I also get this warning when I bind text that contains German Umlaute (ü, ö, ä), because they get replaced with ü ö and ä. This makes the warning useless for me, because I get like 95% false warnings and don't bother to look at them any more.

@mprobst
Copy link
Contributor

mprobst commented Jul 12, 2016

@oocx sorry about that. As I wrote on the PR, Angular would need to re-implement half an HTML parser to avoid this warning, which seems overkill as it is harmless and only supposed to be a development help.

We could consider not escaping at least some unicode characters (e.g. umlauts), but then we'd cause the warning for people passing in ü or &0xDC; and so on. The joys of encoding :-/

@oocx
Copy link
Contributor

oocx commented Jul 13, 2016

@mprobst I understand why it's not easy to fix. Is there a way to disable this warning? It's spamming my console with warnings when I debug my application.

If there is no option to turn it off yet, I could try to implement it and submit a PR if you think it's a useful feature.

@mprobst
Copy link
Contributor

mprobst commented Jul 13, 2016

@oocx there's no feature to disable that. I'm skeptical about making it a feature, too – more options make software harder to use, and I'm not convinced it's enough of a problem to warrant fixing. Binding innerHtml & Co should be rare in any case...

@adrianfaciu adrianfaciu mentioned this issue Jul 23, 2016
Closed
@mprobst
Copy link
Contributor

mprobst commented Jul 26, 2016

@oocx FYI 482c019 should fix this for you.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mprobst mprobst

Labels
area: security Issues related to built-in security features, such as HTML sanitation workaround1: obvious
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mprobst @vicb @zoechi @oocx @wkwiatek