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

Sanitizing HTML warning for Cyrillic symbols #10206

Closed
firov opened this issue Jul 21, 2016 · 8 comments · Fixed by #10272
Closed

Sanitizing HTML warning for Cyrillic symbols #10206

firov opened this issue Jul 21, 2016 · 8 comments · Fixed by #10272
Assignees

Comments

@firov
Copy link

firov commented Jul 21, 2016

I'm submitting a bug report (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

Angular html binding with Cyrillic symbols generates warning:
WARNING: sanitizing HTML stripped some content (see http://g.co/ng/security#xss).

Expected/desired behavior
No error

Reproduction of the problem
http://plnkr.co/edit/smQk5PksQGtKLeezdvEr

Please tell us about your environment:

  • Angular version: 2.0.0-rc.4
  • Browser: [Chrome 51 ]
  • Language: [all]
@vicb
Copy link
Contributor

vicb commented Jul 21, 2016

/cc @mprobst

@mprobst
Copy link
Contributor

mprobst commented Jul 21, 2016

@rjamet do you remember/know why our HTML sanitizer turns everything non-ASCII into entity references (ü --> ü)? It seems overkill to me, and is causing these somewhat annoying spurious error messages.

Are we aware of any browser we want to support (> IE9) that would have issues with misinterpreting properly serialized UTF-16 when assigning into innerHtml? It seems to me that we could just as well only escape the usual XML/HTML special characters (< --> &lt;, & --> &amp;).

@mprobst
Copy link
Contributor

mprobst commented Jul 21, 2016

@firov btw the warning is harmless, this is just because cycling through sanitization changes your cyrillic characters from the proper encoded representation into decimal entities. It's annoying, but shouldn't cause any actual issues.

@firov
Copy link
Author

firov commented Jul 21, 2016

@mprobst yeah i understand that, so this is not vital but if you have fully localized application with some 3d party library that uses html binding, that can cause a lot of spam. So may be some opportunity to silent this warnings?

@mprobst
Copy link
Contributor

mprobst commented Jul 21, 2016

@firov ack, we should do something about this, you're not the first person to run into it.

I think our best bet is to simply not escape the unicode characters, which should hopefully cover most use cases. I'm reluctant to make it a setting or something like that, as that just increases conceptual overhead (and then every developer has to enable the setting). I'm deferring the decision on whether that is safe to our friendly neighborhood security reviewer @rjamet though.

@rjamet
Copy link
Contributor

rjamet commented Jul 22, 2016

(+@koto)

This is done to prevent encoding-related bugs, and general mXSS concerns. That canonicalization is useful security-wise, so I'd rather keep it in place. Just to make sure: that step isn't a concern for your use, @firov?

It's probably the warning that needs fixing, but I'm not sure we can untangle safely the encoding from the sanitization. That would probably mean a second pass on the content, since the encoding appears to be done on the DOM serialization step :/

Alternatively, the sanitizer could keep a flag around during sanitization that turns to true only on content removal / sanitization, and let that trigger the warning. This might not warn on everything (like the mXSS checks at the beginning), but it would be faster and less likely to affect security. Sounds like the best solution here.

Relevant file: https://github.com/angular/angular/blob/master/modules/%40angular/platform-browser/src/security/html_sanitizer.ts

@firov
Copy link
Author

firov commented Jul 22, 2016

@rjamet Yes, only warning itself is a problem even if it is visible only in developer mode.

mprobst added a commit to mprobst/angular that referenced this issue Jul 25, 2016
Previously, Angular would warn users when simply re-encoding text
outside of the ASCII range. While harmless, the log spam was annoying.

With this change, Angular specifically tracks whether anything was
stripped during sanitization, and only reports a warning if so.

Fixes angular#10206.
mprobst added a commit to mprobst/angular that referenced this issue Jul 25, 2016
Previously, Angular would warn users when simply re-encoding text
outside of the ASCII range. While harmless, the log spam was annoying.

With this change, Angular specifically tracks whether anything was
stripped during sanitization, and only reports a warning if so.

Fixes angular#10206.
mprobst added a commit that referenced this issue Jul 26, 2016
Previously, Angular would warn users when simply re-encoding text
outside of the ASCII range. While harmless, the log spam was annoying.

With this change, Angular specifically tracks whether anything was
stripped during sanitization, and only reports a warning if so.

Fixes #10206.
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants