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

Commit

Permalink
fix(autocomplete): pulls in text content as HTML to prevent it from b…
Browse files Browse the repository at this point in the history 
…eing un-escaped
  • Loading branch information
Robert Messerle committed May 19, 2015
1 parent d24872c commit 33ac259
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/components/autocomplete/js/highlightController.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ angular

function MdHighlightCtrl ($scope, $element, $interpolate) {
var term = $element.attr('md-highlight-text'),
text = $interpolate($element.text())($scope),
text = $interpolate($element.html())($scope),
flags = $element.attr('md-highlight-flags') || '',
watcher = $scope.$watch(term, function (term) {
var regex = getRegExp(term, flags),
Expand Down

5 comments on commit 33ac259

@gkalpak
Copy link
Member

Choose a reason for hiding this comment

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

Where's the test 😛
(Seriously, I think there should be a test guarding against regressions here.)

BTW, I believe there are cases where things can break (not XSS vulnerability, but unexpected/incorrect handling), when $element contains other element nodes.
E.g.:

<md-item-template>
  <span md-highlight-text="...">
    <span>{{ item.id }}</span>
    <span>{{ item.label }}</span>
  </span>
</md-item-template>

// e.g. consider entering "spa" as search text

(A proper solution might be non-trivial though.)

@robertmesserle
Copy link
Contributor

Choose a reason for hiding this comment

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

@gkalpak Right, the intent of md-highlight-text is to ONLY contain text. I think proper handling of this would be to warn the user if they mis-use it. The idea is that your example should look like this:

<md-item-template>
  <span>
    <span md-highlight-text="...">{{ item.id }}</span>
    <span md-highlight-text="...">{{ item.label }}</span>
  </span>
</md-item-template>

@gkalpak
Copy link
Member

Choose a reason for hiding this comment

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

@robertmesserle, this makes perfect sense, in which case I would suggest:

  1. Document it (so people don't get their templates unexpectedly broken).

  2. Make sure the element only contains text before starting to interpolate etc; e.g. with something like:

    ```js
function MdHighlightCtrl ($scope, $element, $interpolate) {
  $element.text($element.text());
  var term = $element.attr('md-highlight-text'),
        text = $interpolate($element.text())($scope),   // notice: no `html()`
  ...

WDYT ?

@gkalpak
Copy link
Member

Choose a reason for hiding this comment

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

BTW, @robertmesserle, this does not fix the problem (I am afraid I kind of mislead you; I seem to be doing this a lot to people lately).

The problem could be caused by an effective $element.html($element.text()), but it's not :D

The real problem comes from text = $interpolate($element.whatever())($scope),. Basically, since $element most probably contains some interpolated text, text comes from some property's value on $scope. Thus, it has not passed through any "filtering" (either by Angular or the browser itself) and can be anything (including <script>...).

So, by using $element.html(text), you are "light-heartedly" putting an arbitrary string from $scope directly as the element's HTML. So, you should first make the original text safe (i.e. encode HTML entities) and then insert <span class="md-highlight">...</span> and pass it to $element.html().

A quick fix that comes into mind would be:

function MdHighlightCtrl ($scope, $element, $interpolate) {
  var term = $element.attr('md-highlight-text'),
      unsafeText = $interpolate($element.text())($scope),
      safeText = $element.text(unsafeText).html(),
      flags = $element.attr('md-highlight-flags') || '',
      watcher = $scope.$watch(term, function (term) {
        var regex = getRegExp(term, flags),
        html = safeText.replace(regex, '<span class="highlight">$&</span>');
        $element.html(html);
      });
  $element.on('$destroy', function () { watcher(); });

  function sanitize (term) {...}

  function getRegExp (text, flags) {...}
}

(I wouldn't be surprised if there was a better solution though 😉)

@gkalpak
Copy link
Member

Choose a reason for hiding this comment

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

@robertmesserle, POC that the proposed fix above fixes the issue: http://codepen.io/ExpertSystem/pen/KpMLYv?editors=001

(If you remove the overwriting MdHighlightCtrl, you'll noticed we get pwned.)

Please sign in to comment.