Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

WIP: fix(toast): sanitize auto wrapped custom toast templates #11652

Closed
wants to merge 1 commit into from

Conversation

Splaktar
Copy link
Member

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

There was an issue filed in g3 about using innerHTML here in md-toast for custom toasts.

Issue Number:
Related to #6494. Related to #6259.

What is the new behavior?

Protect against a possible XSS vector by sanitizing all elements inside of the template's outer <md-toast><sanitize everything here></md-toast> element. This could be multiple DOM elements, comments, etc.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

This may require ngSanitize in some apps that previously didn't use it. In those cases, the apps would break with an $sce exception.

Other information

This uses a similar approach to md-select's approach to sanitizing text that can contain HTML.

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 23, 2019
@Splaktar Splaktar self-assigned this Feb 23, 2019
@Splaktar Splaktar added P2: required Issues that must be fixed. - Breaking Change labels Feb 23, 2019
protect against a possible XSS vector

Related to #6494. Related to #6259.
@Splaktar Splaktar changed the title fix(toast): sanitize custom toast templates fix(toast): sanitize auto wrapped custom toast templates Feb 23, 2019
// templates as well as custom templates. It would remove attributes like
// aria-live and aria-relevant. It would also remove md-buttons.
// TODO Should we go in and call trustAsHtml on those templates farther up the chain?
return template;
Copy link
Member Author

Choose a reason for hiding this comment

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

@mmalerba there's another edge case here where a custom template may specify autoWrap = false. I tested and verified that a <script> could be injected here (but I wasn't able to get it to execute).

I just wanted to confirm that you thought it was worth while to go and try to trust the templates passed in here from $mdToast.simple() and $mdToast.showSimple().

Copy link
Contributor

Choose a reason for hiding this comment

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

Browser will (hopefully) stop injected stuff from being executed, but adding stuff like <img src="/" onerror = "alert(1);"/> will inject and execute

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Feb 23, 2019
@Splaktar Splaktar changed the title fix(toast): sanitize auto wrapped custom toast templates WIP: fix(toast): sanitize auto wrapped custom toast templates Apr 3, 2019
@Splaktar Splaktar added P3: important Important issues that really should be fixed when possible. and removed P2: required Issues that must be fixed. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Apr 3, 2019
@Splaktar Splaktar closed this Apr 3, 2019
@Splaktar Splaktar added the resolution: won't fix There are no resources to fix this issue, the priority is too low, or it doesn't align w/ MD spec. label Feb 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Breaking Change cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P3: important Important issues that really should be fixed when possible. resolution: won't fix There are no resources to fix this issue, the priority is too low, or it doesn't align w/ MD spec.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants