From 0bb81c9b65f5984660e041af02a7a4ae6bc2645a Mon Sep 17 00:00:00 2001 From: Vladimir Airikh Date: Wed, 6 Jun 2018 17:40:34 +0300 Subject: [PATCH] fix `fetch API rasies TypeError when argument is URL object` (close #1613) (#1623) * fix `fetch API rasies TypeError when argument is URL object` * requested changes, fix Safari case * refactoring * requested changes * requested changes * requested changes: fix tests * add URL test case * xhr, location.href, location.assign, location.replace cases * requested changes: fix xhr.open test, remove unnecessary testcase in window-test.js * requested changes: refactor xhr.open test * fix location property case * try to fix tests * try to fix tests * fix location property test * try to fix location property test * try to fix location property test * fix double quotation in location property test * requested changes * requested changes * restore xhr.responseURL test --- .../code-instrumentation/location/wrapper.js | 3 + .../code-instrumentation/properties/index.js | 39 ++-- src/client/sandbox/fetch.js | 32 +-- src/client/sandbox/xhr.js | 6 +- test/client/config-qunit-server-app.js | 14 ++ .../code-instrumentation/location-test.js | 198 ++++++++++++++++++ test/client/fixtures/sandbox/fetch-test.js | 71 ++++++- .../fixtures/sandbox/node/window-test.js | 1 - test/client/fixtures/sandbox/xhr-test.js | 63 ++++++ 9 files changed, 378 insertions(+), 49 deletions(-) diff --git a/src/client/sandbox/code-instrumentation/location/wrapper.js b/src/client/sandbox/code-instrumentation/location/wrapper.js index 6ce516346..a3053737e 100644 --- a/src/client/sandbox/code-instrumentation/location/wrapper.js +++ b/src/client/sandbox/code-instrumentation/location/wrapper.js @@ -61,6 +61,9 @@ export default class LocationWrapper extends EventEmitter { return ensureTrailingSlash(href, locationUrl); }; const getProxiedHref = href => { + if (typeof href !== 'string') + href = String(href); + href = prepareUrl(href); if (isJsProtocol(href)) diff --git a/src/client/sandbox/code-instrumentation/properties/index.js b/src/client/sandbox/code-instrumentation/properties/index.js index 31344c9a9..dfc7af007 100644 --- a/src/client/sandbox/code-instrumentation/properties/index.js +++ b/src/client/sandbox/code-instrumentation/properties/index.js @@ -56,30 +56,29 @@ export default class PropertyAccessorsInstrumentation extends SandboxBase { }, set: (owner, location) => { - if (typeof location === 'string') { - const ownerWindow = domUtils.isWindow(owner) ? owner : owner.defaultView; - const locationWrapper = LocationAccessorsInstrumentation.getLocationWrapper(ownerWindow); - - if (!locationWrapper) { - if (!isJsProtocol(location)) { - const url = prepareUrl(location); - const resourceType = urlUtils.stringifyResourceType({ isIframe: true }); - - owner.location = destLocation.sameOriginCheck(location.toString(), url) - ? urlUtils.getProxyUrl(url, { resourceType }) - : urlUtils.getCrossDomainIframeProxyUrl(url); - } - else - owner.location = processJsAttrValue(location, { isJsProtocol: true, isEventAttr: false }); + const ownerWindow = domUtils.isWindow(owner) ? owner : owner.defaultView; + const locationWrapper = LocationAccessorsInstrumentation.getLocationWrapper(ownerWindow); + + if (!locationWrapper) { + if (typeof location !== 'string') + location = String(location); + + if (!isJsProtocol(location)) { + const url = prepareUrl(location); + const resourceType = urlUtils.stringifyResourceType({ isIframe: true }); + + owner.location = destLocation.sameOriginCheck(location.toString(), url) + ? urlUtils.getProxyUrl(url, { resourceType }) + : urlUtils.getCrossDomainIframeProxyUrl(url); } else - // eslint-disable-next-line no-restricted-properties - locationWrapper.href = location; - - return location; + owner.location = processJsAttrValue(location, { isJsProtocol: true, isEventAttr: false }); } + else + // eslint-disable-next-line no-restricted-properties + locationWrapper.href = location; - return owner.location; + return location; } } }; diff --git a/src/client/sandbox/fetch.js b/src/client/sandbox/fetch.js index f1cafe5b3..b7e27b7a9 100644 --- a/src/client/sandbox/fetch.js +++ b/src/client/sandbox/fetch.js @@ -6,6 +6,7 @@ import { getOriginHeader, sameOriginCheck, get as getDestLocation } from '../uti import { isFetchHeaders, isFetchRequest } from '../utils/dom'; import SAME_ORIGIN_CHECK_FAILED_STATUS_CODE from '../../request-pipeline/xhr/same-origin-check-failed-status-code'; import { overrideDescriptor } from '../utils/property-overriding'; +import * as browserUtils from '../utils/browser'; const DEFAULT_REQUEST_CREDENTIALS = nativeMethods.Request ? new nativeMethods.Request(window.location.toString()).credentials : void 0; @@ -43,23 +44,16 @@ export default class FetchSandbox extends SandboxBase { const inputIsFetchRequest = isFetchRequest(input); let init = args[1]; - if (inputIsString) { - args[0] = getProxyUrl(input); + if (!inputIsFetchRequest) { + args[0] = getProxyUrl(inputIsString ? input : String(input)); init = init || {}; args[1] = FetchSandbox._addSpecialHeadersToRequestInit(init); } - else if (inputIsFetchRequest && init) + else if (init) args[1] = FetchSandbox._addSpecialHeadersToRequestInit(init); } - static _isValidRequestArgs (args) { - return typeof args[0] === 'string' || isFetchRequest(args[0]); - } - - static _requestIsValid (args) { - if (!FetchSandbox._isValidRequestArgs(args)) - return false; - + static _sameOriginCheck (args) { let url = null; let requestMode = null; @@ -68,7 +62,7 @@ export default class FetchSandbox extends SandboxBase { requestMode = args[0].mode; } else { - url = args[0]; + url = parseProxyUrl(args[0]).destUrl; requestMode = (args[1] || {}).mode; } @@ -109,13 +103,19 @@ export default class FetchSandbox extends SandboxBase { window.Request.prototype = nativeMethods.Request.prototype; window.fetch = function (...args) { - if (!args.length) + // NOTE: Safari processed the empty `fetch()` request without `Promise` rejection (GH-1613) + if (!args.length && !browserUtils.isSafari) return nativeMethods.fetch.apply(this); - if (!FetchSandbox._requestIsValid(args)) - return sandbox.window.Promise.reject(new TypeError()); + try { + FetchSandbox._processArguments(args); + } + catch (e) { + return sandbox.window.Promise.reject(e); + } - FetchSandbox._processArguments(args); + if (!FetchSandbox._sameOriginCheck(args)) + return sandbox.window.Promise.reject(new TypeError()); const fetchPromise = nativeMethods.fetch.apply(this, args); diff --git a/src/client/sandbox/xhr.js b/src/client/sandbox/xhr.js index ade10dd32..b0b1020a3 100644 --- a/src/client/sandbox/xhr.js +++ b/src/client/sandbox/xhr.js @@ -111,8 +111,10 @@ export default class XhrSandbox extends SandboxBase { // NOTE: Redirect all requests to the Hammerhead proxy and ensure that requests don't // violate Same Origin Policy. xmlHttpRequestProto.open = function () { - if (typeof arguments[1] === 'string') - arguments[1] = getProxyUrl(arguments[1]); + const url = arguments[1]; + const urlIsString = typeof input === 'string'; + + arguments[1] = getProxyUrl(urlIsString ? url : String(url)); nativeMethods.xhrOpen.apply(this, arguments); }; diff --git a/test/client/config-qunit-server-app.js b/test/client/config-qunit-server-app.js index 815f4d978..10b29be14 100644 --- a/test/client/config-qunit-server-app.js +++ b/test/client/config-qunit-server-app.js @@ -173,4 +173,18 @@ module.exports = function (app) { delete cookies[userAgent]; }); + + // We should add routes for iframe loading in IE 11 ("location" property test) (GH-1613) + var iframeLocationUrlCallback = function (req, res, next) { + var locationPossibleValues = ['null', 'undefined', '[object Object]', 'some-path']; + + if (locationPossibleValues.includes(req.params.url)) + res.send(req.params.url); + else + next(); + }; + + app.get('/fixtures/sandbox/code-instrumentation/:url', iframeLocationUrlCallback); + + app.get('/:url', iframeLocationUrlCallback); }; diff --git a/test/client/fixtures/sandbox/code-instrumentation/location-test.js b/test/client/fixtures/sandbox/code-instrumentation/location-test.js index 865203342..6fc91d43d 100644 --- a/test/client/fixtures/sandbox/code-instrumentation/location-test.js +++ b/test/client/fixtures/sandbox/code-instrumentation/location-test.js @@ -192,6 +192,204 @@ test('special pages (GH-339)', function () { destLocation.forceLocation(storedForcedLocation); }); +test('different url types for locationWrapper methods (href, replace, assign) (GH-1613)', function () { + var testCases = [ + { + url: null, + expectedUrl: 'https://example.com/null' + }, + { + url: void 0, + expectedUrl: 'https://example.com/undefined' + }, + { + url: { url: '/some-path' }, + expectedUrl: 'https://example.com/[object%20Object]' + }, + { + url: { + url: '/some-path', + toString: function () { + return this.url; + } + }, + expectedUrl: 'https://example.com/some-path' + } + ]; + + // NOTE: IE11 doesn't support 'URL()' + if (!browserUtils.isIE11) { + testCases.push({ + url: new URL('https://example.com/some-path'), + expectedUrl: 'https://example.com/some-path' + }); + } + + var windowMock = { + location: { + href: '', + + replace: function (url) { + this.href = url; + }, + + assign: function (url) { + this.href = url; + }, + + toString: function () { + return urlUtils.getProxyUrl(this.location.href); + } + } + }; + + windowMock.top = windowMock; + + var locationWrapper = new LocationWrapper(windowMock); + + testCases.map(function (testCase) { + locationWrapper.href = testCase.url; + strictEqual(windowMock.location.href, urlUtils.getProxyUrl(testCase.expectedUrl)); + + locationWrapper.replace(testCase.url); + strictEqual(windowMock.location.href, urlUtils.getProxyUrl(testCase.expectedUrl)); + + locationWrapper.assign(testCase.url); + strictEqual(windowMock.location.href, urlUtils.getProxyUrl(testCase.expectedUrl)); + }); +}); + +test('throwing errors on calling locationWrapper methods (href, replace, assign) with invalid arguments', function () { + expect(browserUtils.isIE11 ? 1 : 3); + + var invalidUrlObject = { + toString: function () { + return {}; + } + }; + + var windowMock = { + location: { + href: '', + + replace: function (url) { + this.href = url; + }, + + assign: function (url) { + this.href = url; + }, + + toString: function () { + return urlUtils.getProxyUrl(this.location.href); + } + } + }; + + windowMock.top = windowMock; + + var locationWrapper = new LocationWrapper(windowMock); + + try { + locationWrapper.href = invalidUrlObject; + strictEqual(windowMock.location.href, urlUtils.getProxyUrl('')); + } + catch (e) { + ok(true, 'href'); + } + + if (!browserUtils.isIE11) { + try { + locationWrapper.replace(invalidUrlObject); + strictEqual(windowMock.location.href, urlUtils.getProxyUrl('')); + } + catch (e) { + ok(true, 'replace'); + } + + try { + locationWrapper.assign(invalidUrlObject); + strictEqual(windowMock.location.href, urlUtils.getProxyUrl('')); + } + catch (e) { + ok(true, 'assign'); + } + } +}); + +test('different url types for "location" property (GH-1613)', function () { + var checkLocation = function (iframe) { + return new Promise(function (resolve) { + iframe.addEventListener('load', function () { + resolve(iframe.contentWindow.location.toString()); + }); + }); + }; + + var checkLocationAssignment = function (nativeIframeUrl, processedIframeUrl) { + return Promise.all([createTestIframe(), createTestIframe()]) + .then(function (iframes) { + var nativeIframePromise = checkLocation(iframes[0]); + var iframePromise = checkLocation(iframes[1]); + + iframes[0].contentWindow.location = nativeIframeUrl; + eval(processScript('iframes[1].contentWindow.location = ' + processedIframeUrl)); + + return Promise.all([nativeIframePromise, iframePromise]); + }) + .then(function (urls) { + var nativeIframeLocation = urls[0].slice(urls[0].lastIndexOf('/') + 1); + var processedIframeLocation = urls[1].slice(urls[1].lastIndexOf('/') + 1); + + strictEqual(nativeIframeLocation, processedIframeLocation); + }); + }; + + var cases = [ + checkLocationAssignment(null, 'null'), + checkLocationAssignment(void 0, 'void 0'), + checkLocationAssignment({}, '{}'), + checkLocationAssignment({ + toString: function () { + return '/some-path'; + } + }, '{' + + ' toString: function () {\n' + + ' return "/some-path";\n' + + ' }' + + '}') + ]; + + // NOTE: IE11 doesn't support 'URL()' + if (!browserUtils.isIE11) + cases.push(checkLocationAssignment(new URL(location.origin + '/some-path'), 'new URL(location.origin + "/some-path")')); + + return Promise.all(cases); +}); + +test('should not navigate in case of invalid "location" property assigment (GH-1613)', function () { + expect(0); + + var invalidUrlObject = '{ toString: function () { return {} } }'; + + var checkLocation = function (iframe) { + return new Promise(function () { + iframe.addEventListener('load', function () { + ok(false, 'should not navigate'); + }); + }); + }; + + createTestIframe() + .then(function (iframe) { + var iframePromise = checkLocation(iframe); + + eval(processScript('iframe.contentWindow.location = ' + invalidUrlObject)); + + return iframePromise; + }); +}); + test('should ensure a trailing slash on page navigation using href setter, assign and replace methods (GH-1426)', function () { function getExpectedProxyUrl (testCase) { var proxiedUrl = urlUtils.getProxyUrl(testCase.url); diff --git a/test/client/fixtures/sandbox/fetch-test.js b/test/client/fixtures/sandbox/fetch-test.js index d904d06f6..0f4be1500 100644 --- a/test/client/fixtures/sandbox/fetch-test.js +++ b/test/client/fixtures/sandbox/fetch-test.js @@ -62,6 +62,52 @@ if (window.fetch) { }); }); + test('different url types for "fetch" (GH-1613)', function () { + var testCases = [ + { + args: [new URL('https://example.com/some-path')], + expectedUrl: 'https://example.com/some-path' + }, + { + args: [null], + expectedUrl: 'https://example.com/null' + }, + { + args: [void 0], + expectedUrl: 'https://example.com/undefined' + }, + { + args: [{ url: '/some-path' }], + expectedUrl: 'https://example.com/[object%20Object]' + }, + { + args: [{ + url: '/some-path', + toString: function () { + return this.url; + } + }], + expectedUrl: 'https://example.com/some-path' + } + ]; + + if (browserUtils.isSafari) { + testCases.push({ + args: [], + expectedUrl: 'https://example.com/undefined' + }); + } + + var createTestCasePromise = function (testCase) { + return fetch.apply(window, testCase.args) + .then(function (response) { + strictEqual(response.url, testCase.expectedUrl); + }); + }; + + return Promise.all(testCases.map(createTestCasePromise)); + }); + test('the internal 222 status code should be replaced with 0 on the client side', function () { return fetch('/xhr-222/') .then(function (response) { @@ -382,15 +428,8 @@ if (window.fetch) { }); test('request promise should be rejected on invalid calling (GH-939)', function () { - var testCases = [ - 123, - function () { - }, - null - ]; - - var createTestCasePromise = function (firstArg) { - return fetch.call(window, firstArg) + var checkArg = function () { + return fetch.apply(window, arguments) .then(function () { ok(false, 'wrong state of the request promise'); }) @@ -399,7 +438,19 @@ if (window.fetch) { }); }; - return Promise.all(testCases.map(createTestCasePromise)); + var cases = [ + checkArg({ + toString: function () { + return {}; + } + }) + ]; + + // NOTE: Safari processed `fetch()` without `Promise` rejection (GH-1613) + if (!browserUtils.isSafari) + cases.push(checkArg()); + + return Promise.all(cases); }); test('should return non-overridden Promise on calling the "fetch" without parameters (GH-1099)', function () { diff --git a/test/client/fixtures/sandbox/node/window-test.js b/test/client/fixtures/sandbox/node/window-test.js index 1f9aade9d..b87c56c82 100644 --- a/test/client/fixtures/sandbox/node/window-test.js +++ b/test/client/fixtures/sandbox/node/window-test.js @@ -198,7 +198,6 @@ test('parameters passed to the native function in its original form', function ( var xhr = new nativeMethods.XMLHttpRequest(); checkNativeFunctionArgs('abort', 'xhrAbort', xhr); - checkNativeFunctionArgs('open', 'xhrOpen', xhr); nativeMethods.xhrOpen.call(xhr, 'GET', '/path', true); checkNativeFunctionArgs('send', 'xhrSend', xhr); diff --git a/test/client/fixtures/sandbox/xhr-test.js b/test/client/fixtures/sandbox/xhr-test.js index a82b13edf..4b5f76cd7 100644 --- a/test/client/fixtures/sandbox/xhr-test.js +++ b/test/client/fixtures/sandbox/xhr-test.js @@ -3,10 +3,12 @@ var XHR_HEADERS = hammerhead.get('./../request-pipeline/xhr/headers'); var AUTHORIZATION = hammerhead.get('./../request-pipeline/xhr/authorization'); var destLocation = hammerhead.get('./utils/destination-location'); var settings = hammerhead.get('./settings'); +var urlUtils = hammerhead.get('./utils/url'); var nativeMethods = hammerhead.nativeMethods; var xhrSandbox = hammerhead.sandbox.xhr; var Promise = hammerhead.Promise; +var browserUtils = hammerhead.utils.browser; function getPrototypeFromChainContainsProp (obj, prop) { while (obj && !obj.hasOwnProperty(prop)) @@ -76,6 +78,66 @@ test('toString, instanceof, constructor and static properties', function () { strictEqual(XMLHttpRequest.DONE, nativeMethods.XMLHttpRequest.DONE); }); +test('different url types for xhr.open method (GH-1613)', function () { + var storedNativeXhrOpen = nativeMethods.xhrOpen; + var xhr = new XMLHttpRequest(); + + // NOTE: IE11 doesn't support 'URL()' + if (!browserUtils.isIE11) { + nativeMethods.xhrOpen = function () { + strictEqual(arguments[1], urlUtils.getProxyUrl('https://example.com/some-path')); + }; + xhr.open('GET', new URL('https://example.com/some-path')); + } + + nativeMethods.xhrOpen = function () { + strictEqual(arguments[1], urlUtils.getProxyUrl('https://example.com/null')); + }; + xhr.open('GET', null); + + nativeMethods.xhrOpen = function () { + strictEqual(arguments[1], urlUtils.getProxyUrl('https://example.com/undefined')); + }; + xhr.open('GET', void 0); + + nativeMethods.xhrOpen = function () { + strictEqual(arguments[1], urlUtils.getProxyUrl('https://example.com/[object%20Object]')); + }; + xhr.open('GET', { url: '/some-path' }); + + nativeMethods.xhrOpen = function () { + strictEqual(arguments[1], urlUtils.getProxyUrl('https://example.com/some-path')); + }; + xhr.open('GET', { + toString: function () { + return '/some-path'; + } + }); + + nativeMethods.xhrOpen = storedNativeXhrOpen; +}); + +test('throwing an error on invalid calling "open" method (GH-1613)', function () { + var url = { + toString: function () { + return {}; + } + }; + + var exception = false; + var xhr = new XMLHttpRequest(); + + try { + xhr.open('GET', url, false); + } + catch (e) { + exception = true; + } + finally { + ok(exception); + } +}); + module('regression'); asyncTest('unexpected text modifying during typing text in the search input on the http://www.google.co.uk (B238528)', function () { @@ -132,6 +194,7 @@ asyncTest('xhr.responseURL', function () { var testCount = 0; xhr.addEventListener('readystatechange', function () { + // NOTE: IE11 doesn't support 'XMLHttpRequest.responseURL' if (this.responseURL) { strictEqual(this.responseURL, 'https://example.com/xhr-large-response'); ++testCount;