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 3 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 _createStyleInstrumentation (property) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_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._createStyleInstrumentation('background'),
backgroundImage: PropertyAccessorsInstrumentation._createStyleInstrumentation('backgroundImage'),
'background-image': PropertyAccessorsInstrumentation._createStyleInstrumentation('background-image'),
borderImage: PropertyAccessorsInstrumentation._createStyleInstrumentation('borderImage'),
'border-image': PropertyAccessorsInstrumentation._createStyleInstrumentation('border-image'),
'borderImageSource': PropertyAccessorsInstrumentation._createStyleInstrumentation('borderImageSource'),
'border-image-source': PropertyAccessorsInstrumentation._createStyleInstrumentation('border-image-source'),
listStyle: PropertyAccessorsInstrumentation._createStyleInstrumentation('listStyle'),
'list-style': PropertyAccessorsInstrumentation._createStyleInstrumentation('list-style'),
listStyleImage: PropertyAccessorsInstrumentation._createStyleInstrumentation('listStyleImage'),
'list-style-image': PropertyAccessorsInstrumentation._createStyleInstrumentation('list-style-image'),
cssText: PropertyAccessorsInstrumentation._createStyleInstrumentation('cssText'),
cursor: PropertyAccessorsInstrumentation._createStyleInstrumentation('cursor'),

styleSheets: {
condition: domUtils.isDocument,
Expand Down
16 changes: 16 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,22 @@ class NativeMethods {
if (textAreaValueDescriptor && typeof textAreaValueDescriptor.set === 'function')
this.textAreaValueSetter = textAreaValueDescriptor.set;

// stylesheets
Copy link
Contributor

Choose a reason for hiding this comment

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

// Stylesheets

if (win.CSSStyleDeclaration) {
this.CSSStyleDeclarationGetPropertyValue = win.CSSStyleDeclaration.prototype.getPropertyValue;
this.CSSStyleDeclarationSetProperty = win.CSSStyleDeclaration.prototype.setProperty;
}

if (win.MSStyleCSSProperties) {
this.MSStyleCSSPropertiesGetPropertyValue = win.MSStyleCSSProperties.prototype.getPropertyValue;
this.MSStyleCSSPropertiesSetProperty = win.MSStyleCSSProperties.prototype.setProperty;
}

if (win.CSS2Property) {
this.CSS2PropertyGetPropertyValue = win.CSS2Property.prototype.getPropertyValue;
this.CSS2PropertySetProperty = win.CSS2Property.prototype.setProperty;
}

this.refreshClasses(win);
}

Expand Down
39 changes: 38 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,29 @@ export default class WindowSandbox extends SandboxBase {
return String(reason);
}

static _wrapCSSGetPropertyValueIfNecessary (constructor, nativeGetPropertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename function parameter to the nativeGetPropertyValueFn

if (nativeGetPropertyValue) {
constructor.prototype.getPropertyValue = function (...args) {
const value = nativeGetPropertyValue.apply(this, args);

return styleProcessor.cleanUp(value, parseProxyUrl);
};
}
}

static _wrapCSSSetPropertyIfNecessary (constructor, nativeSetProperty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nativeSetPropertyFn

if (nativeSetProperty) {
constructor.prototype.setProperty = function (...args) {
const value = args[1];

if (typeof value === 'string')
args[1] = styleProcessor.process(value, getProxyUrl);

return nativeSetProperty.apply(this, args);
};
}
}

handleEvent (event) {
if (event.type === 'unhandledrejection' && !event.defaultPrevented) {
const reason = WindowSandbox._formatUnhandledRejectionReason(event.reason);
Expand Down Expand Up @@ -404,5 +427,19 @@ 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);
}
}
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: We cannot get url through 'borderImage' or 'border-image' properties
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: If we setup borderImageorborder-imageproperty then it affects aborderImageSource` property.

var propForChecking = prop === 'borderImage' || prop === 'border-image' ? 'borderImageSource' : prop;
Copy link
Contributor

Choose a reason for hiding this comment

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

var affectedProp = ...


if (prop === 'cssText')
value = 'background:' + value;

el.style[prop] = value;

var controlValue = el.style[propForChecking];
Copy link
Contributor

Choose a reason for hiding this comment

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

var nativeValue

var proxiedValue = controlValue && controlValue.replace(url, proxyUrl);

eval(processScript('el.style["' + prop + '"]="' + value + '"'));
strictEqual(getProperty(el.style, propForChecking), controlValue, prop);
strictEqual(el.style[propForChecking], proxiedValue, prop);
});
}
});

test('get style body', function () {
var style = document.createElement('style');
Expand Down
12 changes: 12 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,15 @@ 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);
});
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