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

Commit

Permalink
fix($sanitize): don't rely on YARR regex engine executing immediately
Browse files Browse the repository at this point in the history
In Safari 7 (and other browsers potentially using the latest YARR JIT library)
regular expressions are not always executed immediately that they are called.
The regex is only evaluated (lazily) when you first access properties on the `matches`
result object returned from the regex call.

In the case of `decodeEntities()`, we were updating this returned object, `parts[0] = ''`,
before accessing it, `if (parts[2])', and so our change was overwritten by the result
of executing the regex.

The solution here is not to modify the match result object at all. We only need to make use
of the three match results directly in code.

Developers should be aware, in the future, when using regex, to read from the result object
before making modifications to it.

There is no additional test committed here, because when run against Safari 7, this
bug caused numerous specs to fail, which are all fixed by this commit.

Closes #5193
Closes #5192
  • Loading branch information
petebacondarwin committed Dec 3, 2013
1 parent fd4b999 commit 81b8185
Showing 1 changed file with 12 additions and 10 deletions.
22 changes: 12 additions & 10 deletions src/ngSanitize/sanitize.js
Expand Up @@ -360,25 +360,27 @@ function htmlParser( html, handler ) {
}
}

var hiddenPre=document.createElement("pre");
var spaceRe = /^(\s*)([\s\S]*?)(\s*)$/;
/**
* decodes all entities into regular string
* @param value
* @returns {string} A string with decoded entities.
*/
var hiddenPre=document.createElement("pre");
function decodeEntities(value) {
if (!value) {
return '';
}
if (!value) { return ''; }

// Note: IE8 does not preserve spaces at the start/end of innerHTML
var spaceRe = /^(\s*)([\s\S]*?)(\s*)$/;
// so we must capture them and reattach them afterward
var parts = spaceRe.exec(value);
parts[0] = '';
if (parts[2]) {
hiddenPre.innerHTML=parts[2].replace(/</g,"&lt;");
parts[2] = hiddenPre.innerText || hiddenPre.textContent;
var spaceBefore = parts[1];
var spaceAfter = parts[3];
var content = parts[2];

This comment has been minimized.

Copy link
@caitp

caitp Dec 3, 2013

Contributor

hmm, this seems like a significantly more complicated solution. Is that necessarily a good thing?

This comment has been minimized.

Copy link
@mbj

mbj Dec 6, 2013

Avoiding mutations where possible is IMHO always a good thing. Reduces cognitive overhead.

This comment has been minimized.

Copy link
@caitp

caitp Dec 6, 2013

Contributor

well, that's one way to look at it. As long as we're working on iOS7 it's all good I suppose. But it's many more bytes than are really needed even to address the webkit bug

if (content) {
hiddenPre.innerHTML=content.replace(/</g,"&lt;");
content = hiddenPre.innerText || hiddenPre.textContent;
}
return parts.join('');
return spaceBefore + content + spaceAfter;
}

/**
Expand Down

0 comments on commit 81b8185

Please sign in to comment.