From 0676a625b7fee26d9556de48ffe8412fa03033a0 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 11 Aug 2015 15:04:07 -0700 Subject: [PATCH 1/3] fix($compile): properly sanitize xlink:href attribute interoplation Closes #12524 (cherry picked from commit f33ce173c90736e349cf594df717ae3ee41e0f7a) --- src/ng/compile.js | 2 +- test/ng/compileSpec.js | 48 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 051028e338e8..5c1d04a140c6 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1044,7 +1044,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); - if ((nodeName === 'a' && key === 'href') || + 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'); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index da8b52725d71..2621466845e5 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -6322,6 +6322,54 @@ describe('$compile', function() { }); }); + it('should use $$sanitizeUri when declared via ng-href', function() { + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "someUrl"; + + $$sanitizeUri.andReturn('someSanitizedUrl'); + $rootScope.$apply(); + expect(element.attr('href')).toBe('someSanitizedUrl'); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + }); + }); + + it('should use $$sanitizeUri when working with svg and xlink:href', function() { + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "evilUrl"; + + $$sanitizeUri.andReturn('someSanitizedUrl'); + $rootScope.$apply(); + expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl'); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + }); + }); + + + it('should use $$sanitizeUri when working with svg and xlink:href', function() { + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "evilUrl"; + + $$sanitizeUri.andReturn('someSanitizedUrl'); + $rootScope.$apply(); + expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl'); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + }); + }); }); describe('interpolation on HTML DOM event handler attributes onclick, onXYZ, formaction', function() { From b8408af16446caeab41d7a4fbd71e89c452303b5 Mon Sep 17 00:00:00 2001 From: benjaminParisel Date: Wed, 21 Dec 2022 10:26:19 +0100 Subject: [PATCH 2/3] fix(angular.merge): do not merge __proto__ property Report from * https://github.com/angular/angular.js/commit/c0498d45feb913c318224ea70b5adf7112df6bac * https://github.com/angular/angular.js/commit/add78e62004e80bb1e16ab2dfe224afa8e513bc3 --- src/Angular.js | 72 ++++++++++++++++++++++++++++++++-------- src/AngularPublic.js | 1 + test/AngularSpec.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 14 deletions(-) diff --git a/src/Angular.js b/src/Angular.js index e0cc5c934561..ee0900f83f5e 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -30,6 +30,7 @@ extend: true, int: true, inherit: true, + merge: true, noop: true, identity: true, valueFn: true, @@ -327,6 +328,41 @@ function setHashKey(obj, h) { } } + +function baseExtend(dst, objs, deep) { + var h = dst.$$hashKey; + for (var i = 0, ii = objs.length; i < ii; ++i) { + var obj = objs[i]; + if (!isObject(obj) && !isFunction(obj)) continue; + var keys = Object.keys(obj); + for (var j = 0, jj = keys.length; j < jj; j++) { + var key = keys[j]; + var src = obj[key]; + if (deep && isObject(src)) { + if (isDate(src)) { + dst[key] = new Date(src.valueOf()); + } else if (isRegExp(src)) { + dst[key] = new RegExp(src); + } else if (src.nodeName) { + dst[key] = src.cloneNode(true); + } else if (isElement(src)) { + dst[key] = src.clone(); + } else { + if (key !== '__proto__') { + if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {}; + baseExtend(dst[key], [src], true); + } + } + } else { + dst[key] = src; + } + } + } + + setHashKey(dst, h); + return dst; +} + /** * @ngdoc function * @name angular.extend @@ -344,21 +380,29 @@ function setHashKey(obj, h) { * @returns {Object} Reference to `dst`. */ function extend(dst) { - var h = dst.$$hashKey; - - for (var i = 1, ii = arguments.length; i < ii; i++) { - var obj = arguments[i]; - if (obj) { - var keys = Object.keys(obj); - for (var j = 0, jj = keys.length; j < jj; j++) { - var key = keys[j]; - dst[key] = obj[key]; - } - } - } + return baseExtend(dst, slice.call(arguments, 1), false); +} - setHashKey(dst, h); - return dst; +/** + * @ngdoc function + * @name angular.merge + * @module ng + * @kind function + * + * @description + * Deeply extends the destination object `dst` by copying own enumerable properties from the `src` object(s) + * to `dst`. You can specify multiple `src` objects. If you want to preserve original objects, you can do so + * by passing an empty object as the target: `var object = angular.merge({}, object1, object2)`. + * + * Unlike {@link angular.extend extend()}, `merge()` recursively descends into object properties of source + * objects, performing a deep copy. + * + * @param {Object} dst Destination object. + * @param {...Object} src Source object(s). + * @returns {Object} Reference to `dst`. + */ +function merge(dst) { + return baseExtend(dst, slice.call(arguments, 1), true); } function int(str) { diff --git a/src/AngularPublic.js b/src/AngularPublic.js index b81257b9fff7..db9e2be0141c 100644 --- a/src/AngularPublic.js +++ b/src/AngularPublic.js @@ -116,6 +116,7 @@ function publishExternalAPI(angular) { 'bootstrap': bootstrap, 'copy': copy, 'extend': extend, + 'merge': merge, 'equals': equals, 'element': jqLite, 'forEach': forEach, diff --git a/test/AngularSpec.js b/test/AngularSpec.js index 0f353f01fe45..1c79c984565b 100644 --- a/test/AngularSpec.js +++ b/test/AngularSpec.js @@ -1580,3 +1580,82 @@ describe('angular', function() { }); }); }); +describe('merge', function () { + it('should recursively copy objects into dst from left to right', function () { + var dst = {foo: {bar: 'foobar'}}; + var src1 = {foo: {bazz: 'foobazz'}}; + var src2 = {foo: {bozz: 'foobozz'}}; + merge(dst, src1, src2); + expect(dst).toEqual({ + foo: { + bar: 'foobar', + bazz: 'foobazz', + bozz: 'foobozz' + } + }); + }); + + + it('should replace primitives with objects', function () { + var dst = {foo: "bloop"}; + var src = {foo: {bar: {baz: "bloop"}}}; + merge(dst, src); + expect(dst).toEqual({ + foo: { + bar: { + baz: "bloop" + } + } + }); + }); + + + it('should replace null values in destination with objects', function () { + var dst = {foo: null}; + var src = {foo: {bar: {baz: "bloop"}}}; + merge(dst, src); + expect(dst).toEqual({ + foo: { + bar: { + baz: "bloop" + } + } + }); + }); + + it('should copy references to functions by value rather than merging', function () { + function fn() { + } + + var dst = {foo: 1}; + var src = {foo: fn}; + merge(dst, src); + expect(dst).toEqual({ + foo: fn + }); + }); + + + it('should create a new array if destination property is a non-object and source property is an array', function () { + var dst = {foo: NaN}; + var src = {foo: [1, 2, 3]}; + merge(dst, src); + expect(dst).toEqual({ + foo: [1, 2, 3] + }); + expect(dst.foo).not.toBe(src.foo); + }); + + it('should not merge the __proto__ property', function() { + var src = JSON.parse('{ "__proto__": { "xxx": "polluted" } }'); + var dst = {}; + + merge(dst, src); + + if (typeof dst.__proto__ !== 'undefined') { // eslint-disable-line + // Should not overwrite the __proto__ property or pollute the Object prototype + expect(dst.__proto__).toBe(Object.prototype); // eslint-disable-line + } + expect(({}).xxx).toBeUndefined(); + }); +}); From d572d755e5714db090c75ef320897c943556ed33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski-Owczarek?= Date: Tue, 19 May 2020 14:25:02 +0200 Subject: [PATCH 3/3] Fix(jqLite): prevent possible XSS due to regex-based HTML replacement Report from commit [2df43c07779137d1bddf7f3b282a1287a8634acd](https://github.com/angular/angular.js/commit/2df43c07779137d1bddf7f3b282a1287a8634acd) --- src/.jshintrc | 34 ++++- src/Angular.js | 20 +++ src/AngularPublic.js | 3 +- src/jqLite.js | 47 +++++-- test/jqLiteSpec.js | 287 ++++++++++++++++++++++++++----------------- 5 files changed, 267 insertions(+), 124 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index 8860f44d5ba4..747b623cc69f 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -1,5 +1,6 @@ { - "extends": "../.jshintrc-base", + "root": true, + "extends": "../.eslintrc-browser.json", "browser": true, "globals": { /* auto/injector.js */ @@ -14,6 +15,9 @@ "splice": false, "push": false, "toString": false, + "minErrConfig": false, + "errorHandlingConfig": false, + "isValidObjectMaxDepth": false, "ngMinErr": false, "_angular": false, "angularModule": false, @@ -36,6 +40,7 @@ "extend": false, "int": false, "inherit": false, + "merge": false, "noop": false, "identity": false, "valueFn": false, @@ -44,7 +49,9 @@ "isObject": false, "isString": false, "isNumber": false, + "isNumberNaN": false, "isDate": false, + "isError": false, "isArray": false, "isFunction": false, "isRegExp": false, @@ -55,6 +62,7 @@ "isBlob": false, "isBoolean": false, "isPromiseLike": false, + "hasCustomToString": false, "trim": false, "escapeForRegexp": false, "isElement": false, @@ -63,6 +71,7 @@ "arrayRemove": false, "copy": false, "shallowCopy": false, + "simpleCompare": false, "equals": false, "csp": false, "concat": false, @@ -71,6 +80,9 @@ "toJsonReplacer": false, "toJson": false, "fromJson": false, + "addDateMinutes": false, + "convertTimezoneToLocal": false, + "timezoneToOffset": false, "startingTag": false, "tryDecodeURIComponent": false, "parseKeyValue": false, @@ -90,8 +102,9 @@ "createMap": false, "VALIDITY_STATE_PROPERTY": false, "reloadWithDebugInfo": false, - + "stringify": false, "skipDestroyOnNextJQueryCleanData": true, + "UNSAFE_restoreLegacyJqLiteXHTMLReplacement": false, "NODE_TYPE_ELEMENT": false, "NODE_TYPE_ATTRIBUTE": false, @@ -121,6 +134,7 @@ "ALIASED_ATTR": false, "jqNextId": false, "camelCase": false, + "fnCamelCaseReplace": false, "jqLitePatchJQueryRemove": false, "JQLite": false, "jqLiteClone": false, @@ -137,6 +151,7 @@ "jqLiteInheritedData": false, "jqLiteBuildFragment": false, "jqLiteParseHTML": false, + "jqLiteWrapNode": false, "getBooleanAttrName": false, "getAliasedAttrName": false, "createEventHandler": false, @@ -149,10 +164,16 @@ /* apis.js */ "hashKey": false, "HashMap": false, + "NgMap": false, /* urlUtils.js */ "urlResolve": false, "urlIsSameOrigin": false, + "urlIsSameOriginAsBaseUrl": false, + "urlIsAllowedOriginFactory": false, + + /* ng/controller.js */ + "identifierForController": false, /* ng/compile.js */ "directiveNormalize": false, @@ -160,9 +181,18 @@ /* ng/parse.js */ "setter": false, + /* ng/q.js */ + "markQExceptionHandled": false, + + /* sce.js */ + "SCE_CONTEXTS": false, + /* ng/directive/directives.js */ "ngDirective": false, + /* ng/directive/ngEventDirs.js */ + "createEventDirective": false, + /* ng/directive/input.js */ "VALID_CLASS": false, "INVALID_CLASS": false, diff --git a/src/Angular.js b/src/Angular.js index e0cc5c934561..4ba8e36eba0a 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -84,6 +84,7 @@ getBlockNodes: true, hasOwnProperty: true, createMap: true, + UNSAFE_restoreLegacyJqLiteXHTMLReplacement, NODE_TYPE_ELEMENT: true, NODE_TYPE_ATTRIBUTE: true, @@ -1570,6 +1571,25 @@ function bindJQuery() { bindJQueryFired = true; } +/** + * @ngdoc function + * @name angular.UNSAFE_restoreLegacyJqLiteXHTMLReplacement + * @module ng + * @kind function + * + * @description + * Restores the pre-1.8 behavior of jqLite that turns XHTML-like strings like + * `
` to `
` instead of `
`. + * The new behavior is a security fix so if you use this method, please try to adjust + * to the change & remove the call as soon as possible. + * Note that this only patches jqLite. If you use jQuery 3.5.0 or newer, please read + * [jQuery 3.5 upgrade guide](https://jquery.com/upgrade-guide/3.5/) for more details + * about the workarounds. + */ +function UNSAFE_restoreLegacyJqLiteXHTMLReplacement() { + JQLite.legacyXHTMLReplacement = true; +} + /** * throw error if the argument is falsy. */ diff --git a/src/AngularPublic.js b/src/AngularPublic.js index b81257b9fff7..b38623287ea7 100644 --- a/src/AngularPublic.js +++ b/src/AngularPublic.js @@ -141,7 +141,8 @@ function publishExternalAPI(angular) { 'getTestability': getTestability, '$$minErr': minErr, '$$csp': csp, - 'reloadWithDebugInfo': reloadWithDebugInfo + 'reloadWithDebugInfo': reloadWithDebugInfo, + 'UNSAFE_restoreLegacyJqLiteXHTMLReplacement': UNSAFE_restoreLegacyJqLiteXHTMLReplacement }); angularModule = setupModuleLoader(window); diff --git a/src/jqLite.js b/src/jqLite.js index dfdad1c0ee17..bb24caec0905 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -158,7 +158,6 @@ var XHTML_TAG_REGEXP = /<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w var wrapMap = { 'option': [1, ''], - 'thead': [1, '', '
'], 'col': [2, '', '
'], 'tr': [2, '', '
'], @@ -170,6 +169,21 @@ wrapMap.optgroup = wrapMap.option; wrapMap.tbody = wrapMap.tfoot = wrapMap.colgroup = wrapMap.caption = wrapMap.thead; wrapMap.th = wrapMap.td; +// Support: IE <10 only +// IE 9 requires an option wrapper & it needs to have the whole table structure +// set up up front; assigning `""` to `tr.innerHTML` doesn't work, etc. +var wrapMapIE9 = { + option: [1, ''], + _default: [0, '', ''] +}; + +for (var key in wrapMap) { + var wrapMapValueClosing = wrapMap[key]; + var wrapMapValue = wrapMapValueClosing.slice().reverse(); + wrapMapIE9[key] = [wrapMapValue.length, '<' + wrapMapValue.join('><') + '>', '']; +} + +wrapMapIE9.optgroup = wrapMapIE9.option; function jqLiteIsTextNode(html) { return !HTML_REGEXP.test(html); @@ -183,7 +197,7 @@ function jqLiteAcceptsData(node) { } function jqLiteBuildFragment(html, context) { - var tmp, tag, wrap, + var tmp, tag, wrap, finalHtml, fragment = context.createDocumentFragment(), nodes = [], i; @@ -194,13 +208,30 @@ function jqLiteBuildFragment(html, context) { // Convert html into DOM nodes tmp = tmp || fragment.appendChild(context.createElement("div")); tag = (TAG_NAME_REGEXP.exec(html) || ["", ""])[1].toLowerCase(); - wrap = wrapMap[tag] || wrapMap._default; - tmp.innerHTML = wrap[1] + html.replace(XHTML_TAG_REGEXP, "<$1>") + wrap[2]; + finalHtml = JQLite.legacyXHTMLReplacement ? + html.replace(XHTML_TAG_REGEXP, '<$1>') : + html; + + if (msie < 10) { + wrap = wrapMapIE9[tag] || wrapMapIE9._default; + tmp.innerHTML = wrap[1] + finalHtml + wrap[2]; + + // Descend through wrappers to the right content + i = wrap[0]; + while (i--) { + tmp = tmp.firstChild; + } + } else { + wrap = wrapMap[tag] || wrapMap._default; - // Descend through wrappers to the right content - i = wrap[0]; - while (i--) { - tmp = tmp.lastChild; + + tmp.innerHTML = wrap[1] + finalHtml + wrap[2]; + + // Descend through wrappers to the right content + i = wrap[0]; + while (i--) { + tmp = tmp.lastChild; + } } nodes = concat(nodes, tmp.childNodes); diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index a791641bcda0..e31e37648294 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -26,8 +26,8 @@ describe('jqLite', function() { var expect = jqLite(expected[i])[0]; value = value && equals(expect, actual); msg = "Not equal at index: " + i - + " - Expected: " + expect - + " - Actual: " + actual; + + " - Expected: " + expect + + " - Actual: " + actual; } return value; } @@ -129,6 +129,67 @@ describe('jqLite', function() { }); }); + describe('security', function() { + + it('shouldn\'t unsanitize sanitized code', function(done) { + var counter = 0, + assertCount = 13, + container = jqLite('
'); + + function donePartial() { + counter++; + if (counter === assertCount) { + container.remove(); + delete window.xss; + } + } + + jqLite(document.body).append(container); + window.xss = jasmine.createSpy('xss'); + + // Thanks to Masato Kinugawa from Cure53 for providing the following test cases. + // Note: below test cases need to invoke the xss function with consecutive + // decimal parameters for the assertions to be correct. + forEach([ + '<x', + '\n<x', + '