From d2c1fd4287a2f32cf86eb1d6f47f0bc3870838e3 Mon Sep 17 00:00:00 2001 From: Jason Woods Date: Mon, 29 Nov 2021 14:34:45 +0000 Subject: [PATCH 1/3] fix: when crossDomain is using default the X-Requested-With header is no longer added incorrectly When this header is added incorrectly, it triggers preflight requests. This caused a breaking change from rxjs 6.x with any backend request URL that does not support preflight requests (OPTIONS method). Issue traces back to https://github.com/ReactiveX/rxjs/commit/0d66ba458f07fba51cfc73440d01ef453c24cda7 --- spec/observables/dom/ajax-spec.ts | 47 +++++++++++++++++++------------ src/internal/ajax/ajax.ts | 29 +++++++++++-------- 2 files changed, 46 insertions(+), 30 deletions(-) diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index a3457f2d9a..2402036359 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -103,7 +103,6 @@ describe('ajax', () => { 'content-type': 'kenny/loggins', 'fly-into-the': 'Dangah Zone!', 'take-a-ride-into-the': 'Danger ZoooOoone!', - 'x-requested-with': 'XMLHttpRequest', }); // Did not mutate the headers passed expect(obj.headers).to.deep.equal({ @@ -129,6 +128,7 @@ describe('ajax', () => { url: '/some/path', xsrfCookieName: 'foo', xsrfHeaderName: 'Custom-Header-Name', + crossDomain: false, }; ajax(obj).subscribe(); @@ -188,12 +188,33 @@ describe('ajax', () => { const request = MockXMLHttpRequest.mostRecent; expect(request.url).to.equal('/some/page'); - expect(request.requestHeaders).to.deep.equal({ - 'x-requested-with': 'XMLHttpRequest', - }); + expect(request.requestHeaders).to.deep.equal({}); }); }); + it('should not set the X-Requested-With if crossDomain is true', () => { + ajax({ + url: '/test/monkey', + method: 'GET', + crossDomain: true, + }).subscribe(); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.requestHeaders).to.not.have.key('x-requested-with'); + }); + + it('should not set the X-Requested-With if crossDomain is not present (it defaults to true)', () => { + ajax({ + url: '/test/monkey', + method: 'GET', + }).subscribe(); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.requestHeaders).to.not.have.key('x-requested-with'); + }); + it('should set the X-Requested-With if crossDomain is false', () => { ajax({ url: '/test/monkey', @@ -590,9 +611,7 @@ describe('ajax', () => { expect(MockXMLHttpRequest.mostRecent.url).to.equal('/flibbertyJibbet'); expect(MockXMLHttpRequest.mostRecent.data).to.equal(body); - expect(MockXMLHttpRequest.mostRecent.requestHeaders).to.deep.equal({ - 'x-requested-with': 'XMLHttpRequest', - }); + expect(MockXMLHttpRequest.mostRecent.requestHeaders).to.deep.equal({}); }); it('should send the URLSearchParams straight through to the body', () => { @@ -789,7 +808,6 @@ describe('ajax', () => { expect(request.url).to.equal('/flibbertyJibbet'); expect(request.requestHeaders).to.deep.equal({ 'content-type': 'application/json;charset=utf-8', - 'x-requested-with': 'XMLHttpRequest', }); request.respondWith({ @@ -820,9 +838,7 @@ describe('ajax', () => { expect(request.method).to.equal('POST'); expect(request.url).to.equal('/flibbertyJibbet'); - expect(request.requestHeaders).to.deep.equal({ - 'x-requested-with': 'XMLHttpRequest', - }); + expect(request.requestHeaders).to.deep.equal({}); request.respondWith({ status: 204, @@ -913,7 +929,6 @@ describe('ajax', () => { expect(request.url).to.equal('/flibbertyJibbet'); expect(request.requestHeaders).to.deep.equal({ 'content-type': 'application/json;charset=utf-8', - 'x-requested-with': 'XMLHttpRequest', }); request.respondWith({ @@ -1026,9 +1041,7 @@ describe('ajax', () => { async: true, body: undefined, crossDomain: true, - headers: { - 'x-requested-with': 'XMLHttpRequest', - }, + headers: {}, includeDownloadProgress: true, method: 'GET', responseType: 'json', @@ -1154,9 +1167,7 @@ describe('ajax', () => { async: true, body: undefined, crossDomain: true, - headers: { - 'x-requested-with': 'XMLHttpRequest', - }, + headers: {}, includeUploadProgress: true, includeDownloadProgress: true, method: 'GET', diff --git a/src/internal/ajax/ajax.ts b/src/internal/ajax/ajax.ts index e54e2cbd4c..722d9f21fa 100644 --- a/src/internal/ajax/ajax.ts +++ b/src/internal/ajax/ajax.ts @@ -279,7 +279,20 @@ export function fromAjax(config: AjaxConfig): Observable> { // Here we're pulling off each of the configuration arguments // that we don't want to add to the request information we're // passing around. - const { queryParams, body: configuredBody, headers: configuredHeaders, ...remainingConfig } = config; + const configWithDefaults = { + // Default values + async: true, + crossDomain: true, + withCredentials: false, + method: 'GET', + timeout: 0, + responseType: 'json' as XMLHttpRequestResponseType, + + // Override with passed user values + ...config, + }; + + const { queryParams, body: configuredBody, headers: configuredHeaders, ...remainingConfig } = configWithDefaults; let { url } = remainingConfig; if (!url) { @@ -334,7 +347,7 @@ export function fromAjax(config: AjaxConfig): Observable> { // None of this is necessary, it's only being set because it's "the thing libraries do" // Starting back as far as JQuery, and continuing with other libraries such as Angular 1, // Axios, et al. - if (!config.crossDomain && !('x-requested-with' in headers)) { + if (!configWithDefaults.crossDomain && !('x-requested-with' in headers)) { headers['x-requested-with'] = 'XMLHttpRequest'; } @@ -353,16 +366,8 @@ export function fromAjax(config: AjaxConfig): Observable> { const body = extractContentTypeAndMaybeSerializeBody(configuredBody, headers); const _request: AjaxRequest = { - // Default values - async: true, - crossDomain: true, - withCredentials: false, - method: 'GET', - timeout: 0, - responseType: 'json' as XMLHttpRequestResponseType, - - // Override with passed user values - ...remainingConfig, + // Take the final configuration + ...configWithDefaults, // Set values we ensured above url, From 4944814ac37a5a7bb59f2b28752245fcd460c152 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Mon, 29 Nov 2021 17:29:33 -0600 Subject: [PATCH 2/3] chore: Revert unwanted changes to the tests for ajax. --- spec/observables/dom/ajax-spec.ts | 21 +++++++++++++------ src/internal/ajax/ajax.ts | 35 ++++++++++++++++++------------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 2402036359..41ed3a81d0 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -103,6 +103,7 @@ describe('ajax', () => { 'content-type': 'kenny/loggins', 'fly-into-the': 'Dangah Zone!', 'take-a-ride-into-the': 'Danger ZoooOoone!', + 'x-requested-with': 'XMLHttpRequest', }); // Did not mutate the headers passed expect(obj.headers).to.deep.equal({ @@ -128,7 +129,7 @@ describe('ajax', () => { url: '/some/path', xsrfCookieName: 'foo', xsrfHeaderName: 'Custom-Header-Name', - crossDomain: false, + crossDomain: false, // this is the default anyway }; ajax(obj).subscribe(); @@ -611,7 +612,9 @@ describe('ajax', () => { expect(MockXMLHttpRequest.mostRecent.url).to.equal('/flibbertyJibbet'); expect(MockXMLHttpRequest.mostRecent.data).to.equal(body); - expect(MockXMLHttpRequest.mostRecent.requestHeaders).to.deep.equal({}); + expect(MockXMLHttpRequest.mostRecent.requestHeaders).to.deep.equal({ + 'x-requested-with': 'XMLHttpRequest', + }); }); it('should send the URLSearchParams straight through to the body', () => { @@ -808,6 +811,7 @@ describe('ajax', () => { expect(request.url).to.equal('/flibbertyJibbet'); expect(request.requestHeaders).to.deep.equal({ 'content-type': 'application/json;charset=utf-8', + 'x-requested-with': 'XMLHttpRequest', }); request.respondWith({ @@ -838,7 +842,9 @@ describe('ajax', () => { expect(request.method).to.equal('POST'); expect(request.url).to.equal('/flibbertyJibbet'); - expect(request.requestHeaders).to.deep.equal({}); + expect(request.requestHeaders).to.deep.equal({ + 'x-requested-with': 'XMLHttpRequest', + }); request.respondWith({ status: 204, @@ -928,7 +934,8 @@ describe('ajax', () => { expect(request.method).to.equal('PATCH'); expect(request.url).to.equal('/flibbertyJibbet'); expect(request.requestHeaders).to.deep.equal({ - 'content-type': 'application/json;charset=utf-8', + 'content-type': 'application/json;charset=utf-8', + 'x-requested-with': 'XMLHttpRequest', }); request.respondWith({ @@ -1041,7 +1048,9 @@ describe('ajax', () => { async: true, body: undefined, crossDomain: true, - headers: {}, + headers: { + 'x-requested-with': 'XMLHttpRequest', + }, includeDownloadProgress: true, method: 'GET', responseType: 'json', @@ -1166,7 +1175,7 @@ describe('ajax', () => { const request = { async: true, body: undefined, - crossDomain: true, + crossDomain: false, headers: {}, includeUploadProgress: true, includeDownloadProgress: true, diff --git a/src/internal/ajax/ajax.ts b/src/internal/ajax/ajax.ts index 722d9f21fa..b2bee24619 100644 --- a/src/internal/ajax/ajax.ts +++ b/src/internal/ajax/ajax.ts @@ -252,8 +252,8 @@ export const ajax: AjaxCreationMethod = (() => { const config: AjaxConfig = typeof urlOrConfig === 'string' ? { - url: urlOrConfig, - } + url: urlOrConfig, + } : urlOrConfig; return fromAjax(config); }; @@ -276,13 +276,10 @@ const LOAD = 'load'; export function fromAjax(config: AjaxConfig): Observable> { return new Observable((destination) => { - // Here we're pulling off each of the configuration arguments - // that we don't want to add to the request information we're - // passing around. - const configWithDefaults = { + const requestConfig = { // Default values async: true, - crossDomain: true, + crossDomain: false, withCredentials: false, method: 'GET', timeout: 0, @@ -290,11 +287,22 @@ export function fromAjax(config: AjaxConfig): Observable> { // Override with passed user values ...config, - }; + } as const; - const { queryParams, body: configuredBody, headers: configuredHeaders, ...remainingConfig } = configWithDefaults; + // Here we're pulling off each of the configuration arguments + // that we don't want to add to the request information we're + // passing around. + let { + url, + queryParams, + body: configuredBody, + headers: configuredHeaders, + withCredentials, + xsrfCookieName, + xsrfHeaderName, + crossDomain, + } = requestConfig; - let { url } = remainingConfig; if (!url) { throw new TypeError('url is required'); } @@ -347,14 +355,13 @@ export function fromAjax(config: AjaxConfig): Observable> { // None of this is necessary, it's only being set because it's "the thing libraries do" // Starting back as far as JQuery, and continuing with other libraries such as Angular 1, // Axios, et al. - if (!configWithDefaults.crossDomain && !('x-requested-with' in headers)) { + if (!crossDomain && !('x-requested-with' in headers)) { headers['x-requested-with'] = 'XMLHttpRequest'; } // Allow users to provide their XSRF cookie name and the name of a custom header to use to // send the cookie. - const { withCredentials, xsrfCookieName, xsrfHeaderName } = remainingConfig; - if ((withCredentials || !remainingConfig.crossDomain) && xsrfCookieName && xsrfHeaderName) { + if ((withCredentials || !crossDomain) && xsrfCookieName && xsrfHeaderName) { const xsrfCookie = document?.cookie.match(new RegExp(`(^|;\\s*)(${xsrfCookieName})=([^;]*)`))?.pop() ?? ''; if (xsrfCookie) { headers[xsrfHeaderName] = xsrfCookie; @@ -367,7 +374,7 @@ export function fromAjax(config: AjaxConfig): Observable> { const _request: AjaxRequest = { // Take the final configuration - ...configWithDefaults, + ...requestConfig, // Set values we ensured above url, From 325c0cf58009585ab2cc6c7c49d34749de02c675 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 30 Nov 2021 13:09:23 -0600 Subject: [PATCH 3/3] chore: fix lint --- src/internal/ajax/ajax.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/internal/ajax/ajax.ts b/src/internal/ajax/ajax.ts index b2bee24619..a24b70c4ff 100644 --- a/src/internal/ajax/ajax.ts +++ b/src/internal/ajax/ajax.ts @@ -252,8 +252,8 @@ export const ajax: AjaxCreationMethod = (() => { const config: AjaxConfig = typeof urlOrConfig === 'string' ? { - url: urlOrConfig, - } + url: urlOrConfig, + } : urlOrConfig; return fromAjax(config); }; @@ -289,11 +289,7 @@ export function fromAjax(config: AjaxConfig): Observable> { ...config, } as const; - // Here we're pulling off each of the configuration arguments - // that we don't want to add to the request information we're - // passing around. - let { - url, + const { queryParams, body: configuredBody, headers: configuredHeaders, @@ -303,6 +299,8 @@ export function fromAjax(config: AjaxConfig): Observable> { crossDomain, } = requestConfig; + let url = requestConfig.url; + if (!url) { throw new TypeError('url is required'); }