Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix Images with style=background-image: url("img.png"); aren't loaded (close #1212) #1293

Merged
merged 4 commits into from
Sep 6, 2017
Merged

fix Images with style=background-image: url("img.png"); aren't loaded (close #1212) #1293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd86942
fix `Images with style=background-image: url("img.png"); aren't loade…
LavrovArtem Sep 4, 2017
30c4782
renaming
LavrovArtem Sep 4, 2017
34557f8
fix tests
LavrovArtem Sep 5, 2017
797af00
fix review's issues
LavrovArtem Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 30 additions & 83 deletions src/client/sandbox/code-instrumentation/properties/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';

Expand Down Expand Up @@ -751,89 +768,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,
Expand Down
19 changes: 19 additions & 0 deletions src/client/sandbox/native-methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
56 changes: 55 additions & 1 deletion src/client/sandbox/node/window.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
6 changes: 6 additions & 0 deletions src/processing/script/instrumented.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ export const PROPERTIES = [
'autocomplete',
'background',
'backgroundImage',
'background-image',
'borderImage',
'border-image',
'borderImageSource',
'border-image-source',
'childElementCount',
'cookie',
'cssText',
Expand All @@ -38,7 +42,9 @@ export const PROPERTIES = [
'lastElementChild',
'length',
'listStyle',
'list-style',
'listStyleImage',
'list-style-image',
'localStorage',
'location',
'manifest',
Expand Down
66 changes: 26 additions & 40 deletions test/client/fixtures/sandbox/code-instrumentation/getters-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
22 changes: 22 additions & 0 deletions test/client/fixtures/sandbox/node/window-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also check style.removeProperty.
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/removeProperty

It should returns non-proxied value.

var div = document.createElement('div');
var url = 'http://some.domain.com/image.png';
var proxyUrl = urlUtils.getProxyUrl(url);

div.style.setProperty('background', 'url(' + url + ')', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test for non-string url value case
For example:
div.style.setProperty('background', null)


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'));
});
7 changes: 5 additions & 2 deletions test/server/script-processor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
};
});

Expand Down