From 3da5b2031bd075a88b26943c2757512a4f3a5b23 Mon Sep 17 00:00:00 2001 From: LavrovArtem Date: Wed, 6 Sep 2017 13:29:28 +0300 Subject: [PATCH] fix `Images with style=background-image: url("img.png"); aren't loaded` (close #1212) (#1293) * fix `Images with style=background-image: url("img.png"); aren't loaded` (close #1212) * renaming * fix tests * fix review's issues --- .../code-instrumentation/properties/index.js | 113 +++++------------- src/client/sandbox/native-methods.js | 19 +++ src/client/sandbox/node/window.js | 56 ++++++++- src/processing/script/instrumented.js | 6 + .../code-instrumentation/getters-test.js | 66 ++++------ .../fixtures/sandbox/node/window-test.js | 22 ++++ test/server/script-processor-test.js | 7 +- 7 files changed, 163 insertions(+), 126 deletions(-) diff --git a/src/client/sandbox/code-instrumentation/properties/index.js b/src/client/sandbox/code-instrumentation/properties/index.js index f792366dff..9c633d6a1a 100644 --- a/src/client/sandbox/code-instrumentation/properties/index.js +++ b/src/client/sandbox/code-instrumentation/properties/index.js @@ -129,6 +129,23 @@ export default class PropertyAccessorsInstrumentation extends SandboxBase { return HTML_ELEMENT_TEXT_PROPERTIES[prop]; } + static _createForStyleProperty (property) { + return { + condition: isStyle, + + get: style => styleProcessor.cleanUp(style[property], urlUtils.parseProxyUrl), + + set: (style, value) => { + if (typeof value === 'string') + style[property] = styleProcessor.process(value, urlUtils.getProxyUrl); + else + style[property] = value; + + return value; + } + }; + } + _createPropertyAccessors (window, document) { let storedDomain = ''; @@ -756,89 +773,19 @@ export default class PropertyAccessorsInstrumentation extends SandboxBase { }, // Style - background: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.background, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.background = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.background; - } - }, - - backgroundImage: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.backgroundImage, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.backgroundImage = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.backgroundImage; - } - }, - - borderImage: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.borderImage, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.borderImage = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.borderImage; - } - }, - - cssText: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.cssText, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.cssText = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.cssText; - } - }, - - cursor: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.cursor, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.cursor = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.cursor; - } - }, - - listStyle: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.listStyle, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.listStyle = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.listStyle; - } - }, - - listStyleImage: { - condition: isStyle, - get: style => styleProcessor.cleanUp(style.listStyleImage, urlUtils.parseProxyUrl), - - set: (style, value) => { - if (typeof value === 'string') - style.listStyleImage = styleProcessor.process(value, urlUtils.getProxyUrl); - - return style.listStyleImage; - } - }, + background: PropertyAccessorsInstrumentation._createForStyleProperty('background'), + backgroundImage: PropertyAccessorsInstrumentation._createForStyleProperty('backgroundImage'), + 'background-image': PropertyAccessorsInstrumentation._createForStyleProperty('background-image'), + borderImage: PropertyAccessorsInstrumentation._createForStyleProperty('borderImage'), + 'border-image': PropertyAccessorsInstrumentation._createForStyleProperty('border-image'), + 'borderImageSource': PropertyAccessorsInstrumentation._createForStyleProperty('borderImageSource'), + 'border-image-source': PropertyAccessorsInstrumentation._createForStyleProperty('border-image-source'), + listStyle: PropertyAccessorsInstrumentation._createForStyleProperty('listStyle'), + 'list-style': PropertyAccessorsInstrumentation._createForStyleProperty('list-style'), + listStyleImage: PropertyAccessorsInstrumentation._createForStyleProperty('listStyleImage'), + 'list-style-image': PropertyAccessorsInstrumentation._createForStyleProperty('list-style-image'), + cssText: PropertyAccessorsInstrumentation._createForStyleProperty('cssText'), + cursor: PropertyAccessorsInstrumentation._createForStyleProperty('cursor'), styleSheets: { condition: domUtils.isDocument, diff --git a/src/client/sandbox/native-methods.js b/src/client/sandbox/native-methods.js index e2c4de9b00..7c81c119f6 100644 --- a/src/client/sandbox/native-methods.js +++ b/src/client/sandbox/native-methods.js @@ -193,6 +193,25 @@ class NativeMethods { if (textAreaValueDescriptor && typeof textAreaValueDescriptor.set === 'function') this.textAreaValueSetter = textAreaValueDescriptor.set; + // Stylesheets + if (win.CSSStyleDeclaration) { + this.CSSStyleDeclarationGetPropertyValue = win.CSSStyleDeclaration.prototype.getPropertyValue; + this.CSSStyleDeclarationSetProperty = win.CSSStyleDeclaration.prototype.setProperty; + this.CSSStyleDeclarationRemoveProperty = win.CSSStyleDeclaration.prototype.removeProperty; + } + + if (win.MSStyleCSSProperties) { + this.MSStyleCSSPropertiesGetPropertyValue = win.MSStyleCSSProperties.prototype.getPropertyValue; + this.MSStyleCSSPropertiesSetProperty = win.MSStyleCSSProperties.prototype.setProperty; + this.MSStyleCSSPropertiesRemoveProperty = win.MSStyleCSSProperties.prototype.removeProperty; + } + + if (win.CSS2Property) { + this.CSS2PropertyGetPropertyValue = win.CSS2Property.prototype.getPropertyValue; + this.CSS2PropertySetProperty = win.CSS2Property.prototype.setProperty; + this.CSS2PropertyRemoveProperty = win.CSS2Property.prototype.removeProperty; + } + this.refreshClasses(win); } diff --git a/src/client/sandbox/node/window.js b/src/client/sandbox/node/window.js index e491bf9edc..ab799ac4fc 100644 --- a/src/client/sandbox/node/window.js +++ b/src/client/sandbox/node/window.js @@ -7,7 +7,7 @@ import { processScript } from '../../../processing/script'; import styleProcessor from '../../../processing/style'; import * as destLocation from '../../utils/destination-location'; import { processHtml } from '../../utils/html'; -import { isSubDomain, parseUrl, getProxyUrl, convertToProxyUrl, stringifyResourceType } from '../../utils/url'; +import { isSubDomain, parseUrl, getProxyUrl, parseProxyUrl, convertToProxyUrl, stringifyResourceType } from '../../utils/url'; import { isFirefox, isIE9, isIE } from '../../utils/browser'; import { isCrossDomainWindows, isImgElement, isBlob } from '../../utils/dom'; import { isPrimitiveType } from '../../utils/types'; @@ -68,6 +68,39 @@ export default class WindowSandbox extends SandboxBase { return String(reason); } + static _wrapCSSGetPropertyValueIfNecessary (constructor, nativeGetPropertyValueFn) { + if (nativeGetPropertyValueFn) { + constructor.prototype.getPropertyValue = function (...args) { + const value = nativeGetPropertyValueFn.apply(this, args); + + return styleProcessor.cleanUp(value, parseProxyUrl); + }; + } + } + + static _wrapCSSSetPropertyIfNecessary (constructor, nativeSetPropertyFn) { + if (nativeSetPropertyFn) { + constructor.prototype.setProperty = function (...args) { + const value = args[1]; + + if (typeof value === 'string') + args[1] = styleProcessor.process(value, getProxyUrl); + + return nativeSetPropertyFn.apply(this, args); + }; + } + } + + static _wrapCSSRemovePropertyIfNecessary (constructor, nativeRemovePropertyFn) { + if (nativeRemovePropertyFn) { + constructor.prototype.removeProperty = function (...args) { + const oldValue = nativeRemovePropertyFn.apply(this, args); + + return styleProcessor.cleanUp(oldValue, parseProxyUrl); + }; + } + } + handleEvent (event) { if (event.type === 'unhandledrejection' && !event.defaultPrevented) { const reason = WindowSandbox._formatUnhandledRejectionReason(event.reason); @@ -404,5 +437,26 @@ export default class WindowSandbox extends SandboxBase { // NOTE: stab for ie9 and ie10 (GH-801) if (window.XDomainRequest) window.XDomainRequest = window.XMLHttpRequest; + + WindowSandbox._wrapCSSGetPropertyValueIfNecessary(window.CSSStyleDeclaration, + nativeMethods.CSSStyleDeclarationGetPropertyValue); + WindowSandbox._wrapCSSGetPropertyValueIfNecessary(window.MSStyleCSSProperties, + nativeMethods.MSStyleCSSPropertiesGetPropertyValue); + WindowSandbox._wrapCSSGetPropertyValueIfNecessary(window.CSS2Property, + nativeMethods.CSS2PropertyGetPropertyValue); + + WindowSandbox._wrapCSSSetPropertyIfNecessary(window.CSSStyleDeclaration, + nativeMethods.CSSStyleDeclarationSetProperty); + WindowSandbox._wrapCSSSetPropertyIfNecessary(window.MSStyleCSSProperties, + nativeMethods.MSStyleCSSPropertiesSetProperty); + WindowSandbox._wrapCSSSetPropertyIfNecessary(window.CSS2Property, + nativeMethods.CSS2PropertySetProperty); + + WindowSandbox._wrapCSSRemovePropertyIfNecessary(window.CSSStyleDeclaration, + nativeMethods.CSSStyleDeclarationRemoveProperty); + WindowSandbox._wrapCSSRemovePropertyIfNecessary(window.MSStyleCSSProperties, + nativeMethods.MSStyleCSSPropertiesRemoveProperty); + WindowSandbox._wrapCSSRemovePropertyIfNecessary(window.CSS2Property, + nativeMethods.CSS2PropertyRemoveProperty); } } diff --git a/src/processing/script/instrumented.js b/src/processing/script/instrumented.js index 12d525bffb..a1eef5574d 100644 --- a/src/processing/script/instrumented.js +++ b/src/processing/script/instrumented.js @@ -17,7 +17,11 @@ export const PROPERTIES = [ 'autocomplete', 'background', 'backgroundImage', + 'background-image', 'borderImage', + 'border-image', + 'borderImageSource', + 'border-image-source', 'childElementCount', 'cookie', 'cssText', @@ -38,7 +42,9 @@ export const PROPERTIES = [ 'lastElementChild', 'length', 'listStyle', + 'list-style', 'listStyleImage', + 'list-style-image', 'localStorage', 'location', 'manifest', diff --git a/test/client/fixtures/sandbox/code-instrumentation/getters-test.js b/test/client/fixtures/sandbox/code-instrumentation/getters-test.js index fcbf50bfb6..b332f17b4a 100644 --- a/test/client/fixtures/sandbox/code-instrumentation/getters-test.js +++ b/test/client/fixtures/sandbox/code-instrumentation/getters-test.js @@ -198,47 +198,33 @@ test('CSSStyleSheet.href', function () { urlUtils.parseProxyUrl = storedGetProxyUrl; }); -if (browserUtils.isWebKit) { - test('url in stylesheet properties', function () { - var el = document.createElement('div'); - var url = 'http://some.domain.com/image.png'; - var proxyUrl = urlUtils.getProxyUrl(url); - var quote = (function () { - var div = document.createElement('div'); - - div.style.backgroundImage = 'url(http://example.com/img.jpg)'; - - return div.style.backgroundImage.match(/url\((.*)http:\/\/example.com\/img.jpg/)[1]; - })(); - var getExpected = function (value) { - return 'url(' + quote + value + quote + ')'; - }; - - eval(processScript('el.style.backgroundImage="url(' + url + ')"')); - strictEqual(getProperty(el.style, 'backgroundImage'), getExpected(url), 'backgroundImage'); - strictEqual(el.style.backgroundImage, getExpected(proxyUrl), 'backgroundImage'); - - eval(processScript('el.style.background="url(' + url + ')"')); - strictEqual(getProperty(el.style, 'background'), getExpected(url), 'background'); - strictEqual(el.style.background, getExpected(proxyUrl), 'background'); - - eval(processScript('el.style.listStyle="url(' + url + ')"')); - strictEqual(getProperty(el.style, 'listStyle'), getExpected(url), 'listStyle'); - strictEqual(el.style.listStyle, getExpected(proxyUrl), 'listStyle'); - - eval(processScript('el.style.listStyleImage="url(' + url + ')"')); - strictEqual(getProperty(el.style, 'listStyleImage'), getExpected(url), 'listStyleImage'); - strictEqual(el.style.listStyleImage, getExpected(proxyUrl), 'listStyleImage'); - - eval(processScript('el.style.cssText="background-image: url(' + url + ')"')); - strictEqual(getProperty(el.style, 'cssText'), 'background-image: ' + getExpected(url) + ';', 'cssText'); - strictEqual(el.style.cssText, 'background-image: ' + getExpected(proxyUrl) + ';', 'cssText'); - - eval(processScript('el.style.cursor="url(' + url + '), auto"')); - strictEqual(getProperty(el.style, 'cursor'), getExpected(url) + ', auto', 'cursor'); - strictEqual(el.style.cursor, getExpected(proxyUrl) + ', auto', 'cursor'); +test('url in stylesheet properties', function () { + var el = document.createElement('div'); + var url = 'http://some.domain.com/image.png'; + var proxyUrl = urlUtils.getProxyUrl(url); + var cssProperties = ['background', 'backgroundImage', 'background-image', 'borderImage', 'border-image', + 'borderImageSource', 'border-image-source', 'listStyle', 'list-style', 'listStyleImage', + 'list-style-image', 'cssText', 'cursor']; + + cssProperties.forEach(function (prop) { + var value = 'url(' + url + ')'; + + // NOTE: If we setup `borderImage` or `border-image` property then it affects a `borderImageSource` property. + var affectedProp = prop === 'borderImage' || prop === 'border-image' ? 'borderImageSource' : prop; + + if (prop === 'cssText') + value = 'background:' + value; + + el.style[prop] = value; + + var nativeValue = el.style[affectedProp]; + var proxiedValue = nativeValue && nativeValue.replace(url, proxyUrl); + + eval(processScript('el.style["' + prop + '"]="' + value + '"')); + strictEqual(getProperty(el.style, affectedProp), nativeValue, prop); + strictEqual(el.style[affectedProp], proxiedValue, prop); }); -} +}); test('get style body', function () { var style = document.createElement('style'); diff --git a/test/client/fixtures/sandbox/node/window-test.js b/test/client/fixtures/sandbox/node/window-test.js index dacaa17b77..eb5c819233 100644 --- a/test/client/fixtures/sandbox/node/window-test.js +++ b/test/client/fixtures/sandbox/node/window-test.js @@ -375,3 +375,25 @@ test('the constructor field of a function should return a wrapped Function objec Function.prototype.toString = nativeToString; /*eslint-enable no-extend-native*/ }); + +test('getPropertyValue and setProperty methods of css object should be overridden (GH-1212)', function () { + var div = document.createElement('div'); + var url = 'http://some.domain.com/image.png'; + var proxyUrl = urlUtils.getProxyUrl(url); + + div.style.setProperty('background', 'url(' + url + ')', ''); + + ok(div.style.background.indexOf(proxyUrl) !== -1); + ok(div.style.getPropertyValue('background').indexOf(proxyUrl) === -1); + ok(div.style.getPropertyValue('background').indexOf(url) !== -1); + + var oldValue = div.style.removeProperty('background'); + + ok(oldValue.indexOf(proxyUrl) === -1); + + div.style.setProperty('background', null); + + ok(!div.style.background); + ok(!div.style.getPropertyValue('background')); + ok(!div.style.removeProperty('background')); +}); diff --git a/test/server/script-processor-test.js b/test/server/script-processor-test.js index a49fd49537..ef0d92cb3e 100644 --- a/test/server/script-processor-test.js +++ b/test/server/script-processor-test.js @@ -107,10 +107,13 @@ function testProcessing (testCases) { function testPropertyProcessing (templates) { INSTRUMENTED_PROPERTIES.forEach(propName => { + if (propName.indexOf('-') !== -1) + return; + const testCases = templates.map(template => { return { - src: template.src.replace(/\{0\}/g, propName), - expected: template.expected.replace(/\{0\}/g, propName) + src: template.src.replace(/\{0}/g, propName), + expected: template.expected.replace(/\{0}/g, propName) }; });