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

fix(toast): better toast templating allowing comments and sibling elements #6494

Closed

Conversation

devversion
Copy link
Member

Fixes #6259

@EladBezalel
Copy link
Member

LGTM. @jelbourn thoughts?

@EladBezalel EladBezalel added the needs: review This PR is waiting on review from the team label Dec 31, 2015
@EladBezalel EladBezalel assigned jelbourn and unassigned EladBezalel Dec 31, 2015
@@ -289,18 +289,16 @@ function MdToastProvider($$interimElementProvider) {
// Root element of template will be <md-toast>. We need to wrap all of its content inside of
// of <div class="md-toast-content">. All templates provided here should be static, developer-controlled
// content (meaning we're not attempting to guard against XSS).
var parsedTemplate = angular.element(template);
var wrappedContent = '<div class="md-toast-content">' + parsedTemplate.html() + '</div>';
var parsedTemplate = angular.element(template.replace(/<!--(.*?)-->/g, ''));
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work because Angular actually lets you apply directives as comments.

https://docs.angularjs.org/guide/directive

@devversion
Copy link
Member Author

@jelbourn - But to get our actual target we need to get a element, which allows children. Can you please take a look at my new version?

parsedTemplate.empty().append(wrappedContent);
for (var i = 0; i < parsedTemplate.length; i++) {
if (parsedTemplate[i].nodeType != Node.COMMENT_NODE) {
parsedTemplate = parsedTemplate.eq(i);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this not work for a template like this?

<span>
  An element
</span>
<span>
  A sibling element
</span>

@devversion
Copy link
Member Author

@jelbourn - I need one decision from you. What should we do if we got two silbings as roots. Should we pick the first node, which allows to have childs. Or should we always use md-toast

@jelbourn
Copy link
Member

jelbourn commented Jan 5, 2016

I think a much more straightforward approach would be to change parsedTemplate to a <template> element (using document.createElement) and set its innerHTML instead of using angular.element.

@devversion
Copy link
Member Author

@jelbourn - Now it works perfectly. It allows comments, it allows sibling elements, it allows normal md-toast templates. It removes all the toasts correctly from the DOM - PS: <template> is not good, it is reserved for modern browsers as a directive

@jelbourn
Copy link
Member

jelbourn commented Jan 6, 2016

LGTM

@jelbourn jelbourn added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Jan 6, 2016
ThomasBurleson pushed a commit that referenced this pull request Apr 1, 2016
ThomasBurleson pushed a commit that referenced this pull request Apr 1, 2016
gmoothart pushed a commit to gmoothart/material that referenced this pull request Apr 5, 2016
@devversion devversion deleted the fix/toast-template-comments branch April 19, 2016 19:49
@Splaktar Splaktar changed the title fix(toast): strip comments from template to prevent error throwing fix(toast): handle comments in templates Feb 23, 2019
@Splaktar Splaktar changed the title fix(toast): handle comments in templates fix(toast): better toast templating allowing comments and sibling elements Feb 23, 2019
Splaktar added a commit that referenced this pull request Feb 23, 2019
protect against a possible XSS vector

Related to #6494. Related to #6259.
Splaktar added a commit that referenced this pull request Feb 23, 2019
protect against a possible XSS vector

Related to #6494. Related to #6259.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants