Skip to content

Conversation

@vicb
Copy link
Contributor

@vicb vicb commented Oct 8, 2016

fixes #9695

/cc @mprobst @rjamet for security

The former behavior was to remove all script tags from the template. After this PR, only script tags containing javascript are removed, other inert tags are left untouched. One of the use case is JSON-LD as described in the original issue

@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime area: security Issues related to built-in security features, such as HTML sanitation labels Oct 8, 2016
@rjamet
Copy link
Contributor

rjamet commented Oct 10, 2016

Sounds good in principle, but I'm a little wary of the blacklist of Javascript types: I can't find a source for what is and isn't specified or supported. For instance, you missed text/vbscript, and that would probably be able to XSS IE9. Same thing if script types are added (native Dart or whatever). Would a whitelist of non-script-running types be possible instead?

Also, if a browser ever runs scripts with unknown types, this becomes dangerous. They shouldn't, I think all reasonable ones don't, but I haven't found a good source confirming this :/

Copy link
Contributor

@mprobst mprobst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a general note, the fact that Angular ignores <script> tags is not really a security boundary, more a method of protecting the developer from themselves (footgun prevention).

If an attacker can smuggle code into a template, then they can execute arbitrary JavaScript expressions in the page context, so all is lost anyway.

@rjamet, do you think it's worth maintaining this (ignoring JavaScript), or should we just accept it?

if (lcTagName !== SCRIPT_ELEMENT) return false;
// javascript is the default when no type is specified
if (!type) return true;
return IS_JS_MIME_TYPE.test(type);
Copy link
Contributor

@mprobst mprobst Oct 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit dangerous – I wouldn't be surprised if you could find a MIME type here that doesn't match your regexp, but will lead browsers to execute the <script> content.

What about inverting the check, and whitelisting MIME types we know to be safe? E.g. template, JSON LD, etc?

@mprobst
Copy link
Contributor

mprobst commented Oct 10, 2016

@rjamet Hadn't seen your comment for some reason, agreed on the whitelisting :-)

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 10, 2016
@rjamet
Copy link
Contributor

rjamet commented Oct 11, 2016

@mprobst I think it's both footgun prevention and good practices enforcement, and I can definitely imagine people embedding snippets of JS to bypass the compiler or a good practices checker in their templates :/

@vicb vicb closed this Nov 28, 2016
@vicb vicb deleted the 1008-jscript branch June 9, 2017 17:16
@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 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime area: security Issues related to built-in security features, such as HTML sanitation cla: yes

4 participants