From 3cf8837a182d8a1bc4cdffde0336b2b2369943bb Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Wed, 16 Mar 2016 14:54:49 +0100 Subject: [PATCH 1/5] feat($sce): handle URLs through the $sce service. This is a large patch to handle URLs with the $sce service, similarly to HTML context. However, to keep rough compatibility with existing apps, we need to allow URL-context concatenation, since previously $sce contexts prevented sanitization. There's now a set of special contexts (defined in $interpolate) that allow concatenation, in a roughly intuitive way: trusted types alone are trusted, and any concatenation of values results in a non-trusted type that will be handled by getTrusted once the concatenation is done. Thus, "{{safeType}}" will not be sanitized, "{{ 'javascript:foo' }}" will be, and "javascript:{{safeType}}" will also be. There's backwards incompatible changes in there, mostly the fusion of the two URL context sanitization whitelists (the separation is nice, but not important for security). --- src/ng/compile.js | 70 ++++++--------- src/ng/interpolate.js | 70 +++++++++++---- src/ng/sanitizeUri.js | 54 +++--------- src/ng/sce.js | 30 +++++-- src/ngSanitize/sanitize.js | 4 +- test/ng/compileSpec.js | 120 +++++++++++++++++++------- test/ng/directive/booleanAttrsSpec.js | 39 +++++---- test/ng/interpolateSpec.js | 65 +++++++++----- test/ng/sanitizeUriSpec.js | 110 +++++++++++------------ test/ng/sceSpecs.js | 53 +++++++++++- test/ngSanitize/sanitizeSpec.js | 8 +- 11 files changed, 381 insertions(+), 242 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 4baa53f7b79f..a82054daeace 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1167,14 +1167,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { /** * @ngdoc method - * @name $compileProvider#aHrefSanitizationWhitelist + * @name $compileProvider#uriSanitizationWhitelist * @kind function * * @description * Retrieves or overrides the default regular expression that is used for whitelisting of safe - * urls during a[href] sanitization. + * urls during URL-context sanitization. * - * The sanitization is a security measure aimed at preventing XSS attacks via html links. + * The sanitization is a security measure aimed at preventing XSS attacks. * * Any url about to be assigned to a[href] via data-binding is first normalized and turned into * an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist` @@ -1185,45 +1185,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for * chaining otherwise. */ - this.aHrefSanitizationWhitelist = function(regexp) { + this.uriSanitizationWhitelist = function(regexp) { if (isDefined(regexp)) { - $$sanitizeUriProvider.aHrefSanitizationWhitelist(regexp); + $$sanitizeUriProvider.uriSanitizationWhitelist(regexp); return this; } else { - return $$sanitizeUriProvider.aHrefSanitizationWhitelist(); + return $$sanitizeUriProvider.uriSanitizationWhitelist(); } }; - /** - * @ngdoc method - * @name $compileProvider#imgSrcSanitizationWhitelist - * @kind function - * - * @description - * Retrieves or overrides the default regular expression that is used for whitelisting of safe - * urls during img[src] sanitization. - * - * The sanitization is a security measure aimed at prevent XSS attacks via html links. - * - * Any url about to be assigned to img[src] via data-binding is first normalized and turned into - * an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist` - * regular expression. If a match is found, the original url is written into the dom. Otherwise, - * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM. - * - * @param {RegExp=} regexp New regexp to whitelist urls with. - * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for - * chaining otherwise. - */ - this.imgSrcSanitizationWhitelist = function(regexp) { - if (isDefined(regexp)) { - $$sanitizeUriProvider.imgSrcSanitizationWhitelist(regexp); - return this; - } else { - return $$sanitizeUriProvider.imgSrcSanitizationWhitelist(); - } - }; - /** * @ngdoc method * @name $compileProvider#debugInfoEnabled @@ -1287,9 +1258,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { this.$get = [ '$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse', - '$controller', '$rootScope', '$sce', '$animate', '$$sanitizeUri', + '$controller', '$rootScope', '$sce', '$animate', function($injector, $interpolate, $exceptionHandler, $templateRequest, $parse, - $controller, $rootScope, $sce, $animate, $$sanitizeUri) { + $controller, $rootScope, $sce, $animate) { var SIMPLE_ATTR_NAME = /^\w/; var specialAttrHolder = window.document.createElement('div'); @@ -1466,11 +1437,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); - if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) || - (nodeName === 'img' && key === 'src')) { - // sanitize a[href] and img[src] values - this[key] = value = $$sanitizeUri(value, key === 'src'); - } else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) { + // img[srcset] is a bit too weird of a beast to handle through $sce. + // Instead, for now at least, sanitize each of the URIs individually. + // That works even dynamically, but it's not bypassable through the $sce. + // Instead, if you want several unsafe URLs as-is, you should probably + // use trustAsHtml on the whole tag. + if (nodeName === 'img' && key === 'srcset') { + // sanitize img[srcset] values var result = ""; @@ -1488,16 +1461,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { for (var i = 0; i < nbrUrisWith2parts; i++) { var innerIdx = i * 2; // sanitize the uri - result += $$sanitizeUri(trim(rawUris[innerIdx]), true); + result += $sce.getTrustedUrl(trim(rawUris[innerIdx])); // add the descriptor - result += (" " + trim(rawUris[innerIdx + 1])); + result += " " + trim(rawUris[innerIdx + 1]); } // split the last item into uri and descriptor var lastTuple = trim(rawUris[i * 2]).split(/\s/); // sanitize the last uri - result += $$sanitizeUri(trim(lastTuple[0]), true); + result += $sce.getTrustedUrl(trim(lastTuple[0])); // and add the last descriptor if any if (lastTuple.length === 2) { @@ -2972,6 +2945,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { (tag !== "img" && (attrNormalizedName === "src" || attrNormalizedName === "ngSrc"))) { return $sce.RESOURCE_URL; + } else if (attrNormalizedName == "src" || + attrNormalizedName == "href" || + attrNormalizedName == "ngHref" || + attrNormalizedName == "ngSrc") { + // All that is not a RESOURCE_URL but still a URL. This might be overkill for some + // attributes, but the sanitization is permissive, so it should not be troublesome. + return $sce.URL; } } diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index 4ca999e3e0f7..d3ce214e0b85 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -4,8 +4,8 @@ var $interpolateMinErr = angular.$interpolateMinErr = minErr('$interpolate'); $interpolateMinErr.throwNoconcat = function(text) { throw $interpolateMinErr('noconcat', "Error while interpolating: {0}\nStrict Contextual Escaping disallows " + - "interpolations that concatenate multiple expressions when a trusted value is " + - "required. See http://docs.angularjs.org/api/ng.$sce", text); + "interpolations that concatenate multiple expressions in some secure contexts. " + + "See http://docs.angularjs.org/api/ng.$sce", text); }; $interpolateMinErr.interr = function(text, err) { @@ -137,6 +137,10 @@ function $InterpolateProvider() { }, listener, objectEquality); } + function isConcatenationAllowed(context) { + return context == $sce.URL; + } + /** * @ngdoc service * @name $interpolate @@ -278,7 +282,9 @@ function $InterpolateProvider() { textLength = text.length, exp, concat = [], - expressionPositions = []; + expressionPositions = [], + singleExpression = false, + contextAllowsConcatenation = isConcatenationAllowed(trustedContext); while (index < textLength) { if (((startIndex = text.indexOf(startSymbol, index)) !== -1) && @@ -291,7 +297,7 @@ function $InterpolateProvider() { parseFns.push($parse(exp, parseStringifyInterceptor)); index = endIndex + endSymbolLength; expressionPositions.push(concat.length); - concat.push(''); + concat.push(''); // Placeholder that will get replaced with the evaluated expression. } else { // we did not find an interpolation, so we have to add the remainder to the separators array if (index !== textLength) { @@ -301,15 +307,21 @@ function $InterpolateProvider() { } } + if (concat.length == 1 && expressionPositions.length == 1) { + singleExpression = true; + } + // Concatenating expressions makes it hard to reason about whether some combination of // concatenated values are unsafe to use and could easily lead to XSS. By requiring that a - // single expression be used for iframe[src], object[src], etc., we ensure that the value - // that's used is assigned or constructed by some JS code somewhere that is more testable or - // make it obvious that you bound the value to some user controlled value. This helps reduce - // the load when auditing for XSS issues. - if (trustedContext && concat.length > 1) { - $interpolateMinErr.throwNoconcat(text); - } + // single expression be used for some $sce-managed secure contexts (RESOURCE_URLs mostly), + // we ensure that the value that's used is assigned or constructed by some JS code somewhere + // that is more testable or make it obvious that you bound the value to some user controlled + // value. This helps reduce the load when auditing for XSS issues. + // Note that URL-context is dispensed of this, since its getTrusted method can sanitize. + // In that context, .getTrusted will be called on either the single expression or on the + // overall concatenated string (losing trusted types used in the mix, by design). Both these + // methods will sanitize plain strings. Also, HTML could be included, but since it's only used + // in srcdoc attributes, this would not be very useful. if (!mustHaveExpression || expressions.length) { var compute = function(values) { @@ -317,11 +329,32 @@ function $InterpolateProvider() { if (allOrNothing && isUndefined(values[i])) return; concat[expressionPositions[i]] = values[i]; } - return concat.join(''); + + if (contextAllowsConcatenation) { + if (singleExpression) { + // The raw value was left as-is by parseStringifyInterceptor + return $sce.getTrusted(trustedContext, concat[0]); + } else { + return $sce.getTrusted(trustedContext, concat.join('')); + } + } else if (trustedContext) { + if (concat.length > 1) { + // there's at least two parts, so expr + string or exp + exp, and this context + // doesn't allow that. + $interpolateMinErr.throwNoconcat(text); + } else { + return concat.join(''); + } + } else { // In an unprivileged context, just concatenate and return. + return concat.join(''); + } }; var getValue = function(value) { - return trustedContext ? + // In concatenable contexts, getTrusted comes at the end, to avoid sanitizing individual + // parts of a full URL. We don't care about losing the trustedness here, that's handled in + // parseStringifyInterceptor below. + return (trustedContext && !contextAllowsConcatenation) ? $sce.getTrusted(trustedContext, value) : $sce.valueOf(value); }; @@ -338,7 +371,7 @@ function $InterpolateProvider() { return compute(values); } catch (err) { - $exceptionHandler($interpolateMinErr.interr(text, err)); + $exceptionHandler(err); } }, { @@ -360,8 +393,13 @@ function $InterpolateProvider() { function parseStringifyInterceptor(value) { try { - value = getValue(value); - return allOrNothing && !isDefined(value) ? value : stringify(value); + if (contextAllowsConcatenation && singleExpression) { + // No stringification in this case, to keep the trusted value until unwrapping. + return value; + } else { + value = getValue(value); + return allOrNothing && !isDefined(value) ? value : stringify(value); + } } catch (err) { $exceptionHandler($interpolateMinErr.interr(text, err)); } diff --git a/src/ng/sanitizeUri.js b/src/ng/sanitizeUri.js index adf8a7205fc2..de105df26455 100644 --- a/src/ng/sanitizeUri.js +++ b/src/ng/sanitizeUri.js @@ -2,67 +2,41 @@ /** * @description - * Private service to sanitize uris for links and images. Used by $compile and $sanitize. + * Private service to sanitize uris for $sce.URL context. Used by $compile, $sce and $sanitize. */ function $$SanitizeUriProvider() { - var aHrefSanitizationWhitelist = /^\s*(https?|ftp|mailto|tel|file):/, - imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/; - - /** - * @description - * Retrieves or overrides the default regular expression that is used for whitelisting of safe - * urls during a[href] sanitization. - * - * The sanitization is a security measure aimed at prevent XSS attacks via html links. - * - * Any url about to be assigned to a[href] via data-binding is first normalized and turned into - * an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist` - * regular expression. If a match is found, the original url is written into the dom. Otherwise, - * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM. - * - * @param {RegExp=} regexp New regexp to whitelist urls with. - * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for - * chaining otherwise. - */ - this.aHrefSanitizationWhitelist = function(regexp) { - if (isDefined(regexp)) { - aHrefSanitizationWhitelist = regexp; - return this; - } - return aHrefSanitizationWhitelist; - }; + var uriSanitizationWhitelist = /^\s*((https?|ftp|file|blob|tel|mailto):|data:image\/)/i; /** * @description * Retrieves or overrides the default regular expression that is used for whitelisting of safe - * urls during img[src] sanitization. + * urls. * - * The sanitization is a security measure aimed at prevent XSS attacks via html links. + * The sanitization is a security measure aimed at prevent XSS attacks. * - * Any url about to be assigned to img[src] via data-binding is first normalized and turned into - * an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist` + * Any url about to be assigned to URL context via data-binding is first normalized and turned into + * an absolute url. Afterwards, the url is matched against the `urlSanitizationWhitelist` * regular expression. If a match is found, the original url is written into the dom. Otherwise, - * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM. + * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM, + * making it inactive. * * @param {RegExp=} regexp New regexp to whitelist urls with. * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for * chaining otherwise. */ - this.imgSrcSanitizationWhitelist = function(regexp) { + this.uriSanitizationWhitelist = function(regexp) { if (isDefined(regexp)) { - imgSrcSanitizationWhitelist = regexp; + uriSanitizationWhitelist = regexp; return this; } - return imgSrcSanitizationWhitelist; + return uriSanitizationWhitelist; }; this.$get = function() { - return function sanitizeUri(uri, isImage) { - var regex = isImage ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist; - var normalizedVal; - normalizedVal = urlResolve(uri).href; - if (normalizedVal !== '' && !normalizedVal.match(regex)) { + return function sanitizeUri(uri) { + var normalizedVal = urlResolve(uri).href; + if (normalizedVal !== '' && !normalizedVal.match(uriSanitizationWhitelist)) { return 'unsafe:' + normalizedVal; } return uri; diff --git a/src/ng/sce.js b/src/ng/sce.js index f72916455fae..432fdfb3ee9d 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -203,7 +203,7 @@ function $SceDelegateProvider() { return resourceUrlBlacklist; }; - this.$get = ['$injector', function($injector) { + this.$get = ['$injector', '$$sanitizeUri', function($injector, $$sanitizeUri) { var htmlSanitizer = function htmlSanitizer(html) { throw $sceMinErr('unsafe', 'Attempting to use an unsafe value in a safe context.'); @@ -340,9 +340,22 @@ function $SceDelegateProvider() { * @name $sceDelegate#getTrusted * * @description - * Takes the result of a {@link ng.$sceDelegate#trustAs `$sceDelegate.trustAs`} call and - * returns the originally supplied value if the queried context type is a supertype of the - * created type. If this condition isn't satisfied, throws an exception. + * Given an object and a security context in which to assign it, returns a value that's safe to + * use in this context, which was represented by the parameter. To do so, this function either + * unwraps the safe type it has been given (for instance, a {@link ng.$sceDelegate#trustAs + * `$sceDelegate.trustAs`} result), or it might try to sanitize the value given, depending on + * the context and sanitizer availablility. + * + * The two contexts that can be sanitized are $sce.URL and $sce.HTML. The first one is available + * by default, and the second one relies on the $sanitize service (which may be loaded through + * the ngSanitize module). Furthermore, for $sce.RESOURCE_URL context, a plain string may be + * accepted if the resource url policy defined by {@link ng.$sceDelegateProvider#resourceUrlWhitelist + * $sceDelegateProvider.resourceUrlWhitelist} and {@link ng.$sceDelegateProvider#resourceUrlBlacklist + * $sceDelegateProvider.resourceUrlBlacklist} accepts that resource. + * + * This function will throw if the safe type isn't appropriate for this context, or if the + * value given cannot be accepted in the context (which might be caused by sanitization not + * being available, or the value being unsafe). * *
* Disabling auto-escaping is extremely dangerous, it usually creates a Cross Site Scripting @@ -366,7 +379,10 @@ function $SceDelegateProvider() { // If we get here, then we may only take one of two actions. // 1. sanitize the value for the requested type, or // 2. throw an exception. - if (type === SCE_CONTEXTS.RESOURCE_URL) { + + if (type === SCE_CONTEXTS.URL) { + return $$sanitizeUri(maybeTrusted); + } else if (type === SCE_CONTEXTS.RESOURCE_URL) { if (isResourceUrlAllowedByPolicy(maybeTrusted)) { return maybeTrusted; } else { @@ -532,7 +548,7 @@ function $SceDelegateProvider() { * |---------------------|----------------| * | `$sce.HTML` | For HTML that's safe to source into the application. The {@link ng.directive:ngBindHtml ngBindHtml} directive uses this context for bindings. If an unsafe value is encountered and the {@link ngSanitize $sanitize} module is present this will sanitize the value instead of throwing an error. | * | `$sce.CSS` | For CSS that's safe to source into the application. Currently unused. Feel free to use it in your own directives. | - * | `$sce.URL` | For URLs that are safe to follow as links. Currently unused (`
Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. | * | `$sce.JS` | For JavaScript that is safe to execute in your application's context. Currently unused. Feel free to use it in your own directives. | * @@ -708,7 +724,7 @@ function $SceProvider() { * such a value. * * - getTrusted(contextEnum, value) - * This function should return the a value that is safe to use in the context specified by + * This function should return the value that is safe to use in the context specified by * contextEnum or throw and exception otherwise. * * NOTE: This contract deliberately does NOT state that values returned by trustAs() must be diff --git a/src/ngSanitize/sanitize.js b/src/ngSanitize/sanitize.js index d1f752827caa..5142e370105d 100644 --- a/src/ngSanitize/sanitize.js +++ b/src/ngSanitize/sanitize.js @@ -149,7 +149,7 @@ function $SanitizeProvider() { return function(html) { var buf = []; htmlParser(html, htmlSanitizeWriter(buf, function(uri, isImage) { - return !/^unsafe:/.test($$sanitizeUri(uri, isImage)); + return !/^unsafe:/.test($$sanitizeUri(uri)); })); return buf.join(''); }; @@ -445,7 +445,7 @@ function htmlSanitizeWriter(buf, uriValidator) { var lkey=angular.lowercase(key); var isImage = (tag === 'img' && lkey === 'src') || (lkey === 'background'); if (validAttrs[lkey] === true && - (uriAttrs[lkey] !== true || uriValidator(value, isImage))) { + (uriAttrs[lkey] !== true || uriValidator(value))) { out(' '); out(key); out('="'); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c58fc1c8d2e0..8ecd5e46d6a7 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -9771,6 +9771,28 @@ describe('$compile', function() { expect(element.attr('src')).toEqual('http://example.com/image2.png'); })); + it('should accept trusted values for img src', inject(function($rootScope, $compile, $sce) { + /* jshint scripturl:true */ + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('javascript:foo();'); + })); + + it('should sanitize concatenated trusted values for img src', inject(function($rootScope, $compile, $sce) { + /* jshint scripturl:true */ + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('unsafe:javascript:foo();ponies'); + + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://javascript:foo();'); + })); + + it('should not sanitize attributes other than src', inject(function($compile, $rootScope) { /* jshint scripturl:true */ element = $compile('')($rootScope); @@ -9780,16 +9802,34 @@ describe('$compile', function() { expect(element.attr('title')).toBe('javascript:doEvilStuff()'); })); - it('should use $$sanitizeUriProvider for reconfiguration of the src whitelist', function() { + it('should not have endless digests when given arrays in concatenable context', inject(function($compile, $rootScope) { + /* jshint scripturl:true */ + // Any non-resource-url href is URL context + element = $compile('' + + '')($rootScope); + $rootScope.testUrl = [1]; + $rootScope.$digest(); + + $rootScope.testUrl = []; + $rootScope.$digest(); + + $rootScope.testUrl = {a:'b'}; + $rootScope.$digest(); + + $rootScope.testUrl = {}; + $rootScope.$digest(); + })); + + it('should use $$sanitizeUriProvider for reconfiguration of the URL whitelist', function() { module(function($compileProvider, $$sanitizeUriProvider) { var newRe = /javascript:/, returnVal; - expect($compileProvider.imgSrcSanitizationWhitelist()).toBe($$sanitizeUriProvider.imgSrcSanitizationWhitelist()); + expect($compileProvider.uriSanitizationWhitelist()).toBe($$sanitizeUriProvider.uriSanitizationWhitelist()); - returnVal = $compileProvider.imgSrcSanitizationWhitelist(newRe); + returnVal = $compileProvider.uriSanitizationWhitelist(newRe); expect(returnVal).toBe($compileProvider); - expect($$sanitizeUriProvider.imgSrcSanitizationWhitelist()).toBe(newRe); - expect($compileProvider.imgSrcSanitizationWhitelist()).toBe(newRe); + expect($$sanitizeUriProvider.uriSanitizationWhitelist()).toBe(newRe); + expect($compileProvider.uriSanitizationWhitelist()).toBe(newRe); }); inject(function() { // needed to the module definition above is run... @@ -9808,7 +9848,7 @@ describe('$compile', function() { $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.attr('src')).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, true); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); }); @@ -9845,6 +9885,22 @@ describe('$compile', function() { $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.png'); $rootScope.$digest(); expect(element.attr('srcset')).toEqual('http://example.com/image2.png'); + + })); + + // Limitation of the approach used for srcset. Use trustAsHtml and ng-bind-html to bypass it. + it('does not work with trusted values', inject(function($rootScope, $compile, $sce) { + /* jshint scripturl:true */ + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:something'); + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual('unsafe:javascript:something'); + + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:something'); + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual( + 'unsafe:javascript:something ,unsafe:javascript:something'); })); it('should use $$sanitizeUri', function() { @@ -9853,13 +9909,24 @@ describe('$compile', function() { $provide.value('$$sanitizeUri', $$sanitizeUri); }); inject(function($compile, $rootScope) { + /* jshint scripturl:true */ element = $compile('')($rootScope); $rootScope.testUrl = "someUrl"; $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.attr('srcset')).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, true); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); + + element = $compile('')($rootScope); + $rootScope.testUrl = 'javascript:yay'; + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual('someSanitizedUrl ,someSanitizedUrl'); + + element = $compile('')($rootScope); + $rootScope.testUrl = 'script:yay, javascript:nay'; + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual('someSanitizedUrl ,someSanitizedUrl'); }); }); @@ -9905,15 +9972,6 @@ describe('$compile', function() { describe('a[href] sanitization', function() { - it('should not sanitize href on elements other than anchor', inject(function($compile, $rootScope) { - /* jshint scripturl:true */ - element = $compile('
')($rootScope); - $rootScope.testUrl = "javascript:doEvilStuff()"; - $rootScope.$apply(); - - expect(element.attr('href')).toBe('javascript:doEvilStuff()'); - })); - it('should not sanitize attributes other than href', inject(function($compile, $rootScope) { /* jshint scripturl:true */ element = $compile('')($rootScope); @@ -9923,16 +9981,16 @@ describe('$compile', function() { expect(element.attr('title')).toBe('javascript:doEvilStuff()'); })); - it('should use $$sanitizeUriProvider for reconfiguration of the href whitelist', function() { + it('should use $$sanitizeUriProvider for reconfiguration of the uri whitelist', function() { module(function($compileProvider, $$sanitizeUriProvider) { var newRe = /javascript:/, returnVal; - expect($compileProvider.aHrefSanitizationWhitelist()).toBe($$sanitizeUriProvider.aHrefSanitizationWhitelist()); + expect($compileProvider.uriSanitizationWhitelist()).toBe($$sanitizeUriProvider.uriSanitizationWhitelist()); - returnVal = $compileProvider.aHrefSanitizationWhitelist(newRe); + returnVal = $compileProvider.uriSanitizationWhitelist(newRe); expect(returnVal).toBe($compileProvider); - expect($$sanitizeUriProvider.aHrefSanitizationWhitelist()).toBe(newRe); - expect($compileProvider.aHrefSanitizationWhitelist()).toBe(newRe); + expect($$sanitizeUriProvider.uriSanitizationWhitelist()).toBe(newRe); + expect($compileProvider.uriSanitizationWhitelist()).toBe(newRe); }); inject(function() { // needed to the module definition above is run... @@ -9951,7 +10009,7 @@ describe('$compile', function() { $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.attr('href')).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); @@ -9967,7 +10025,7 @@ describe('$compile', function() { $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.attr('href')).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); @@ -9983,7 +10041,7 @@ describe('$compile', function() { $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); @@ -10000,7 +10058,7 @@ describe('$compile', function() { $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); }); @@ -10144,17 +10202,17 @@ describe('$compile', function() { it('should NOT set iframe contents for untrusted values', inject(function($compile, $rootScope, $sce) { element = $compile('')($rootScope); $rootScope.html = '
hello
'; - expect(function() { $rootScope.$digest(); }).toThrowMinErr('$interpolate', 'interr', new RegExp( - /Can't interpolate: {{html}}\n/.source + - /[^[]*\[\$sce:unsafe\] Attempting to use an unsafe value in a safe context./.source)); + expect(function() { $rootScope.$digest(); }).toThrowMinErr( + "$interpolate", "interr", "Can't interpolate: {{html}}\nError: [$sce:unsafe] " + + "Attempting to use an unsafe value in a safe context."); })); it('should NOT set html for wrongly typed values', inject(function($rootScope, $compile, $sce) { element = $compile('')($rootScope); $rootScope.html = $sce.trustAsCss('
hello
'); - expect(function() { $rootScope.$digest(); }).toThrowMinErr('$interpolate', 'interr', new RegExp( - /Can't interpolate: {{html}}\n/.source + - /[^[]*\[\$sce:unsafe\] Attempting to use an unsafe value in a safe context./.source)); + expect(function() { $rootScope.$digest(); }).toThrowMinErr( + "$interpolate", "interr", "Can't interpolate: {{html}}\nError: [$sce:unsafe] " + + "Attempting to use an unsafe value in a safe context."); })); it('should set html for trusted values', inject(function($rootScope, $compile, $sce) { diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js index bab9ab9d97ca..4ba674856bf0 100644 --- a/test/ng/directive/booleanAttrsSpec.js +++ b/test/ng/directive/booleanAttrsSpec.js @@ -118,7 +118,7 @@ describe('boolean attr directives', function() { describe('ngSrc', function() { it('should interpolate the expression and bind to src with raw same-domain value', inject(function($compile, $rootScope) { - var element = $compile('
')($rootScope); + var element = $compile('
')($rootScope); $rootScope.$digest(); expect(element.attr('src')).toBeUndefined(); @@ -133,7 +133,7 @@ describe('ngSrc', function() { it('should interpolate the expression and bind to src with a trusted value', inject(function($compile, $rootScope, $sce) { - var element = $compile('
')($rootScope); + var element = $compile('')($rootScope); + $rootScope.$apply(function() { + $rootScope.id = 1; + }); dealoc(element); }).toThrowMinErr( - "$interpolate", "noconcat", "Error while interpolating: some/{{id}}\nStrict " + - "Contextual Escaping disallows interpolations that concatenate multiple expressions " + - "when a trusted value is required. See http://docs.angularjs.org/api/ng.$sce"); + "$interpolate", "noconcat", "Error"); + })); + + it('should interpolate a multi-part expression for img src attribute (URL context)', inject(function($compile, $rootScope) { + var element = $compile('')($rootScope); + expect(element.attr('src')).toBe(undefined); // URL concatenations are all-or-nothing + $rootScope.$apply(function() { + $rootScope.id = 1; + }); + expect(element.attr('src')).toEqual('some/1'); })); @@ -171,14 +181,13 @@ describe('ngSrc', function() { it('should NOT interpolate a wrongly typed expression', inject(function($compile, $rootScope, $sce) { expect(function() { - var element = $compile('
')($rootScope); + var element = $compile('')($rootScope); + // $rootScope.html = 'noyes'; + // $rootScope.$digest(); + // expect(angular.lowercase(element.attr('srcdoc'))).toEqual('yes'); + // })); + // } }); }); }); diff --git a/test/ngSanitize/sanitizeSpec.js b/test/ngSanitize/sanitizeSpec.js index da1f50cc0cc0..07e03ce44c9c 100644 --- a/test/ngSanitize/sanitizeSpec.js +++ b/test/ngSanitize/sanitizeSpec.js @@ -392,10 +392,10 @@ describe('HTML', function() { describe('uri validation', function() { it('should call the uri validator', function() { writer.start('a', {href:'someUrl'}, false); - expect(uriValidator).toHaveBeenCalledWith('someUrl', false); + expect(uriValidator).toHaveBeenCalledWith('someUrl'); uriValidator.calls.reset(); writer.start('img', {src:'someImgUrl'}, false); - expect(uriValidator).toHaveBeenCalledWith('someImgUrl', true); + expect(uriValidator).toHaveBeenCalledWith('someImgUrl'); uriValidator.calls.reset(); writer.start('someTag', {src:'someNonUrl'}, false); expect(uriValidator).not.toHaveBeenCalled(); @@ -441,7 +441,7 @@ describe('HTML', function() { $$sanitizeUri.and.returnValue('someUri'); expectHTML('').toEqual(''); - expect($$sanitizeUri).toHaveBeenCalledWith('someUri', false); + expect($$sanitizeUri).toHaveBeenCalledWith('someUri'); $$sanitizeUri.and.returnValue('unsafe:someUri'); expectHTML('').toEqual(''); @@ -457,7 +457,7 @@ describe('HTML', function() { $$sanitizeUri.and.returnValue('someUri'); expectHTML('').toEqual(''); - expect($$sanitizeUri).toHaveBeenCalledWith('someUri', true); + expect($$sanitizeUri).toHaveBeenCalledWith('someUri'); $$sanitizeUri.and.returnValue('unsafe:someUri'); expectHTML('').toEqual(''); From 9e31d3c79578848e67c448839d31e3559fc9a27f Mon Sep 17 00:00:00 2001 From: rjamet Date: Tue, 22 Mar 2016 11:42:33 +0100 Subject: [PATCH 2/5] fix(*): Move closer to the old URL sanitization plus misc fixes in tests. Only apply the $sce.URL context where there was previously sanitization, so that the final result matches more closely the old behaviors. Also, fix a few minor mistakes in tests (mismatching tags, wrong expected errors, differences with the surrounding testing style), and mock out $$sanitizeUri where it makes sense. --- src/ng/compile.js | 12 ++-- test/ng/compileSpec.js | 76 ++++++++++++----------- test/ng/directive/booleanAttrsSpec.js | 15 +++-- test/ngMessageFormat/messageFormatSpec.js | 8 +-- 4 files changed, 58 insertions(+), 53 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index a82054daeace..5bf3018ad31c 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1437,6 +1437,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); + // img[srcset] is a bit too weird of a beast to handle through $sce. // Instead, for now at least, sanitize each of the URIs individually. // That works even dynamically, but it's not bypassable through the $sce. @@ -2945,12 +2946,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { (tag !== "img" && (attrNormalizedName === "src" || attrNormalizedName === "ngSrc"))) { return $sce.RESOURCE_URL; - } else if (attrNormalizedName == "src" || - attrNormalizedName == "href" || - attrNormalizedName == "ngHref" || - attrNormalizedName == "ngSrc") { - // All that is not a RESOURCE_URL but still a URL. This might be overkill for some - // attributes, but the sanitization is permissive, so it should not be troublesome. + } else if (tag == "img" && (attrNormalizedName == "src" || + attrNormalizedName == "ngSrc") || + tag == "a" && (attrNormalizedName == "href" || + attrNormalizedName == "xlinkHref" || + attrNormalizedName == "ngHref")) { return $sce.URL; } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 8ecd5e46d6a7..a2f13fd213e2 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -9760,39 +9760,6 @@ describe('$compile', function() { describe('img[src] sanitization', function() { - it('should NOT require trusted values for img src', inject(function($rootScope, $compile, $sce) { - element = $compile('')($rootScope); - $rootScope.testUrl = 'http://example.com/image.png'; - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image.png'); - // But it should accept trusted values anyway. - $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.png'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image2.png'); - })); - - it('should accept trusted values for img src', inject(function($rootScope, $compile, $sce) { - /* jshint scripturl:true */ - element = $compile('')($rootScope); - $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('javascript:foo();'); - })); - - it('should sanitize concatenated trusted values for img src', inject(function($rootScope, $compile, $sce) { - /* jshint scripturl:true */ - element = $compile('')($rootScope); - $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('unsafe:javascript:foo();ponies'); - - element = $compile('')($rootScope); - $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://javascript:foo();'); - })); - - it('should not sanitize attributes other than src', inject(function($compile, $rootScope) { /* jshint scripturl:true */ element = $compile('')($rootScope); @@ -9851,6 +9818,45 @@ describe('$compile', function() { expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl); }); }); + + + it('should sanitize concatenated trusted values', function() { + /* jshint scripturl:true */ + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope, $sce) { + $$sanitizeUri.and.returnValue('someSanitizedUrl'); + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('someSanitizedUrl'); + + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('someSanitizedUrl'); + }); + }); + + it('should not use $$sanitizeUri with trusted values', function() { + /* jshint scripturl:true */ + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope, $sce) { + + element = $compile('')($rootScope); + $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + + $$sanitizeUri.and.throwError('Should not have been called'); + $rootScope.$apply(); + + expect(element.attr('src')).toEqual('javascript:foo();'); + }); + }); }); describe('img[srcset] sanitization', function() { @@ -9920,12 +9926,12 @@ describe('$compile', function() { element = $compile('')($rootScope); $rootScope.testUrl = 'javascript:yay'; - $rootScope.$digest(); + $rootScope.$apply(); expect(element.attr('srcset')).toEqual('someSanitizedUrl ,someSanitizedUrl'); element = $compile('')($rootScope); $rootScope.testUrl = 'script:yay, javascript:nay'; - $rootScope.$digest(); + $rootScope.$apply(); expect(element.attr('srcset')).toEqual('someSanitizedUrl ,someSanitizedUrl'); }); }); diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js index 4ba674856bf0..5bd419a27d53 100644 --- a/test/ng/directive/booleanAttrsSpec.js +++ b/test/ng/directive/booleanAttrsSpec.js @@ -118,7 +118,7 @@ describe('boolean attr directives', function() { describe('ngSrc', function() { it('should interpolate the expression and bind to src with raw same-domain value', inject(function($compile, $rootScope) { - var element = $compile('')($rootScope); + var element = $compile('')($rootScope); $rootScope.$digest(); expect(element.attr('src')).toBeUndefined(); @@ -133,7 +133,7 @@ describe('ngSrc', function() { it('should interpolate the expression and bind to src with a trusted value', inject(function($compile, $rootScope, $sce) { - var element = $compile('')($rootScope); $rootScope.$digest(); expect(element.attr('src')).toBeUndefined(); @@ -181,7 +181,7 @@ describe('ngSrc', function() { it('should NOT interpolate a wrongly typed expression', inject(function($compile, $rootScope, $sce) { expect(function() { - var element = $compile('')($rootScope); $rootScope.$apply(function() { $rootScope.id = $sce.trustAsUrl('http://somewhere'); }); @@ -198,19 +198,19 @@ describe('ngSrc', function() { // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need // to set the property as well to achieve the desired effect - var element = $compile('')($rootScope); + var element = $compile('')($rootScope); $rootScope.$digest(); expect(element.prop('src')).toBeUndefined(); dealoc(element); - element = $compile('')($rootScope); + element = $compile('')($rootScope); $rootScope.$digest(); expect(element.prop('src')).toEqual('some/'); dealoc(element); - element = $compile('')($rootScope); + element = $compile('')($rootScope); $rootScope.$apply(function() { $rootScope.id = $sce.trustAsResourceUrl('http://somewhere'); }); @@ -296,9 +296,8 @@ describe('ngHref', function() { // IE11/10/Edge fail when setting a href to a URL containing a % that isn't a valid escape sequence // See https://github.com/angular/angular.js/issues/13388 it('should throw error if ng-href contains a non-escaped percent symbol', inject(function($rootScope, $compile) { - element = $compile('')($rootScope); - expect(function() { + element = $compile('')($rootScope); $rootScope.$digest(); }).toThrow(); })); diff --git a/test/ngMessageFormat/messageFormatSpec.js b/test/ngMessageFormat/messageFormatSpec.js index e65d5a16401a..daf622e682d0 100644 --- a/test/ngMessageFormat/messageFormatSpec.js +++ b/test/ngMessageFormat/messageFormatSpec.js @@ -537,7 +537,7 @@ describe('$$ngMessageFormat', function() { }).toThrowMinErr( "$interpolate", "noconcat", "Error while interpolating: {{foo}}{{bar}}\n" + "Strict Contextual Escaping disallows interpolations that concatenate multiple " + - "expressions when a trusted value is required. See " + + "expressions in some secure contexts. See " + "http://docs.angularjs.org/api/ng.$sce"); })); }); @@ -626,19 +626,19 @@ describe('$$ngMessageFormat', function() { }).toThrowMinErr( "$interpolate", "noconcat", "Error while interpolating: constant/{{var}}\nStrict " + "Contextual Escaping disallows interpolations that concatenate multiple expressions " + - "when a trusted value is required. See http://docs.angularjs.org/api/ng.$sce"); + "in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); expect(function() { $interpolate('{{var}}/constant', true, isTrustedContext); }).toThrowMinErr( "$interpolate", "noconcat", "Error while interpolating: {{var}}/constant\nStrict " + "Contextual Escaping disallows interpolations that concatenate multiple expressions " + - "when a trusted value is required. See http://docs.angularjs.org/api/ng.$sce"); + "in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); expect(function() { $interpolate('{{foo}}{{bar}}', true, isTrustedContext); }).toThrowMinErr( "$interpolate", "noconcat", "Error while interpolating: {{foo}}{{bar}}\nStrict " + "Contextual Escaping disallows interpolations that concatenate multiple expressions " + - "when a trusted value is required. See http://docs.angularjs.org/api/ng.$sce"); + "in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); })); it('should interpolate a multi-part expression when isTrustedContext is false', inject(function($interpolate) { From 23a548456e73fa4c9a3c2954fc79b5157b1dec5b Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 26 May 2016 17:04:54 +0200 Subject: [PATCH 3/5] fix(IE): Embrace canonicalization of ng-src specified URLs. I'm not really sure why they weren't in the first place, but now they really are, and that doesn't seem like a negative change. --- src/ng/compile.js | 1 - src/ng/directive/attrs.js | 2 +- src/ng/sce.js | 3 -- test/ng/compileSpec.js | 6 ++-- test/ng/directive/booleanAttrsSpec.js | 42 +++++++++++++-------------- 5 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 5bf3018ad31c..3f1079a35f2f 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1437,7 +1437,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); - // img[srcset] is a bit too weird of a beast to handle through $sce. // Instead, for now at least, sanitize each of the URIs individually. // That works even dynamically, but it's not bypassable through the $sce. diff --git a/src/ng/directive/attrs.js b/src/ng/directive/attrs.js index 2f9f54d163fd..c6392d7d7207 100644 --- a/src/ng/directive/attrs.js +++ b/src/ng/directive/attrs.js @@ -415,7 +415,7 @@ forEach(['src', 'srcset', 'href'], function(attrName) { // on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need // to set the property as well to achieve the desired effect. - // we use attr[attrName] value since $set can sanitize the url. + // we reuse the value put in attr[name] since $set might have sanitized the url. if (msie && propName) element.prop(propName, attr[name]); }); } diff --git a/src/ng/sce.js b/src/ng/sce.js index 432fdfb3ee9d..70ab114af499 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -527,9 +527,6 @@ function $SceDelegateProvider() { * call `$sce.trustAs` on them (remember to include the `ngSanitize` module) (e.g. * `
`) just works. * - * Additionally, `a[href]` and `img[src]` automatically sanitize their URLs and do not pass them - * through {@link ng.$sce#getTrusted $sce.getTrusted}. SCE doesn't play a role here. - * * The included {@link ng.$sceDelegate $sceDelegate} comes with sane defaults to allow you to load * templates in `ng-include` from your application's domain without having to even know about SCE. * It blocks loading templates from other domains or loading templates over http from an https diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index a2f13fd213e2..5a37eb3ee716 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -9849,12 +9849,14 @@ describe('$compile', function() { inject(function($compile, $rootScope, $sce) { element = $compile('')($rootScope); - $rootScope.testUrl = $sce.trustAsUrl('javascript:foo();'); + // Assigning javascript:foo to src makes at least IE9-11 complain, so use another + // protocol name. + $rootScope.testUrl = $sce.trustAsUrl('someUnsafeThing:foo();'); $$sanitizeUri.and.throwError('Should not have been called'); $rootScope.$apply(); - expect(element.attr('src')).toEqual('javascript:foo();'); + expect(element.attr('src')).toEqual('someUnsafeThing:foo();'); }); }); }); diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js index 5bd419a27d53..e4aa34f24b31 100644 --- a/test/ng/directive/booleanAttrsSpec.js +++ b/test/ng/directive/booleanAttrsSpec.js @@ -191,34 +191,32 @@ describe('ngSrc', function() { })); - if (msie) { - it('should update the element property as well as the attribute', inject( - function($compile, $rootScope, $sce) { - // on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist - // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need - // to set the property as well to achieve the desired effect + it('should update the element property as well as the attribute', inject( + function($compile, $rootScope, $sce) { + // on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist + // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need + // to set the property as well to achieve the desired effect - var element = $compile('')($rootScope); + var element = $compile('')($rootScope); - $rootScope.$digest(); - expect(element.prop('src')).toBeUndefined(); - dealoc(element); + $rootScope.$digest(); + expect(element.prop('src')).toBe(''); + dealoc(element); - element = $compile('')($rootScope); + element = $compile('')($rootScope); - $rootScope.$digest(); - expect(element.prop('src')).toEqual('some/'); - dealoc(element); + $rootScope.$digest(); + expect(element.prop('src')).toContain('some/'); + dealoc(element); - element = $compile('')($rootScope); - $rootScope.$apply(function() { - $rootScope.id = $sce.trustAsResourceUrl('http://somewhere'); - }); - expect(element.prop('src')).toEqual('http://somewhere'); + element = $compile('')($rootScope); + $rootScope.$apply(function() { + $rootScope.id = $sce.trustAsResourceUrl('http://somewhere'); + }); + expect(element.prop('src')).toEqual('http://somewhere/'); - dealoc(element); - })); - } + dealoc(element); + })); }); From 2b5de99c528ef5e73545af3d2f2992fa14b72a48 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 26 May 2016 17:56:46 +0200 Subject: [PATCH 4/5] fix(*): handle empty srcsets, and jshint prefers === overall --- src/ng/compile.js | 12 ++++++------ src/ng/interpolate.js | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 3f1079a35f2f..0a409d9b7ded 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1442,7 +1442,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // That works even dynamically, but it's not bypassable through the $sce. // Instead, if you want several unsafe URLs as-is, you should probably // use trustAsHtml on the whole tag. - if (nodeName === 'img' && key === 'srcset') { + if (nodeName === 'img' && key === 'srcset' && value) { // sanitize img[srcset] values var result = ""; @@ -2945,11 +2945,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { (tag !== "img" && (attrNormalizedName === "src" || attrNormalizedName === "ngSrc"))) { return $sce.RESOURCE_URL; - } else if (tag == "img" && (attrNormalizedName == "src" || - attrNormalizedName == "ngSrc") || - tag == "a" && (attrNormalizedName == "href" || - attrNormalizedName == "xlinkHref" || - attrNormalizedName == "ngHref")) { + } else if (tag === "img" && (attrNormalizedName === "src" || + attrNormalizedName === "ngSrc") || + tag === "a" && (attrNormalizedName === "href" || + attrNormalizedName === "xlinkHref" || + attrNormalizedName === "ngHref")) { return $sce.URL; } } diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index d3ce214e0b85..e9d76bb5274c 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -138,7 +138,7 @@ function $InterpolateProvider() { } function isConcatenationAllowed(context) { - return context == $sce.URL; + return context === $sce.URL; } /** @@ -307,7 +307,7 @@ function $InterpolateProvider() { } } - if (concat.length == 1 && expressionPositions.length == 1) { + if (concat.length === 1 && expressionPositions.length === 1) { singleExpression = true; } From 4e9267916b681468cdabca562e708a47ce129073 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Tue, 30 Aug 2016 17:14:42 +0200 Subject: [PATCH 5/5] fix(*): Minor fixes addressing gkalpak's comments. Fixes an error message in interpolate, the affected tests, and add URL context to $compile's getTrustedContext. --- src/ng/compile.js | 4 +++- src/ng/interpolate.js | 2 +- test/ng/interpolateSpec.js | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 0a409d9b7ded..c1fc7945305e 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2945,7 +2945,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { (tag !== "img" && (attrNormalizedName === "src" || attrNormalizedName === "ngSrc"))) { return $sce.RESOURCE_URL; - } else if (tag === "img" && (attrNormalizedName === "src" || + } else if ((tag === "img" || tag === "audio" || tag === "video" || + tag === "track" || tag === "source") && + (attrNormalizedName === "src" || attrNormalizedName === "ngSrc") || tag === "a" && (attrNormalizedName === "href" || attrNormalizedName === "xlinkHref" || diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index e9d76bb5274c..96eb6c0ed18f 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -371,7 +371,7 @@ function $InterpolateProvider() { return compute(values); } catch (err) { - $exceptionHandler(err); + $exceptionHandler($interpolateMinErr.interr(text, err)); } }, { diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index d96e74ce5465..8b601c07cd4a 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -263,7 +263,7 @@ describe('$interpolate', function() { expect(function() { return $interpolate('{{foo}}{{bar}}', true, sce.CSS)({foo: foo, bar: bar}); }).toThrowMinErr( - "$interpolate", "noconcat", "Error while interpolating: {{foo}}{{bar}}\n" + + "$interpolate", "interr", "Error while interpolating: {{foo}}{{bar}}\n" + "Strict Contextual Escaping disallows interpolations that concatenate " + "multiple expressions in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); })); @@ -349,19 +349,19 @@ describe('$interpolate', function() { expect(function() { $interpolate('constant/{{var}}', true, isTrustedContext)('val'); }).toThrowMinErr( - "$interpolate", "noconcat", "Error while interpolating: constant/{{var}}\n" + + "$interpolate", "interr", "Error while interpolating: constant/{{var}}\n" + "Strict Contextual Escaping disallows interpolations that concatenate " + "multiple expressions in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); expect(function() { $interpolate('{{var}}/constant', true, isTrustedContext)('val'); }).toThrowMinErr( - "$interpolate", "noconcat", "Error while interpolating: {{var}}/constant\n" + + "$interpolate", "interr", "Error while interpolating: {{var}}/constant\n" + "Strict Contextual Escaping disallows interpolations that concatenate " + "multiple expressions in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); expect(function() { $interpolate('{{foo}}{{bar}}', true, isTrustedContext)('val'); }).toThrowMinErr( - "$interpolate", "noconcat", "Error while interpolating: {{foo}}{{bar}}\n" + + "$interpolate", "interr", "Error while interpolating: {{foo}}{{bar}}\n" + "Strict Contextual Escaping disallows interpolations that concatenate " + "multiple expressions in some secure contexts. See http://docs.angularjs.org/api/ng.$sce"); }));