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: when crossDomain is using default the X-Requested-With header is no longer added incorrectly #6691

Closed
wants to merge 1 commit into from
Closed

fix: when crossDomain is using default the X-Requested-With header is no longer added incorrectly #6691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 29 additions & 18 deletions spec/observables/dom/ajax-spec.ts
View file
Open in desktop
Expand Up @@ -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({
Expand All @@ -129,6 +128,7 @@ describe('ajax', () => {
url: '/some/path',
xsrfCookieName: 'foo',
xsrfHeaderName: 'Custom-Header-Name',
crossDomain: false,
};

ajax(obj).subscribe();
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -1026,9 +1041,7 @@ describe('ajax', () => {
async: true,
body: undefined,
crossDomain: true,
headers: {
'x-requested-with': 'XMLHttpRequest',
},
headers: {},
includeDownloadProgress: true,
method: 'GET',
responseType: 'json',
Expand Down Expand Up @@ -1154,9 +1167,7 @@ describe('ajax', () => {
async: true,
body: undefined,
crossDomain: true,
headers: {
'x-requested-with': 'XMLHttpRequest',
},
headers: {},
Copy link
Author

Choose a reason for hiding this comment

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

This hopefully helps understand why I had to update many tests. It shows the bad behaviour in 4 lines - crossDomain is true but at the same time we were expecting the header that should never be sent when crossDomain is true

includeUploadProgress: true,
includeDownloadProgress: true,
method: 'GET',
Expand Down
29 changes: 17 additions & 12 deletions src/internal/ajax/ajax.ts
View file
Open in desktop
Expand Up @@ -279,7 +279,20 @@ export function fromAjax<T>(config: AjaxConfig): Observable<AjaxResponse<T>> {
// 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.
Copy link
Author

Choose a reason for hiding this comment

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

Just realised this should move down?

const { queryParams, body: configuredBody, headers: configuredHeaders, ...remainingConfig } = config;
const configWithDefaults = {
// Default values
async: true,
crossDomain: true,
Copy link
Member

Choose a reason for hiding this comment

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

Moving this up here changes some of the conditional branches below, and therefor would (from what I can tell) result in a breaking change. Please see the changes to your PR in the new PR (linked in my other comment)

Thanks! :)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @benlesh - Which conditionals does it change? I thought I checked and it will only change the crossDomain conditional to use the correct default.

The default of crossDomain always has been true. Otherwise every single CORS request will default to sending a preflight which is a breaking change from v6.

See here default in v6: https://github.com/ReactiveX/rxjs/blob/6.x/src/internal/observable/dom/AjaxObservable.ts#L163

Copy link
Author

Choose a reason for hiding this comment

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

Checking I can see only two parts of this used below: withCredentials and crossDomain. The former defaults false and so when it was undefined would mean no logic change so exact same behaviour. So the only impact is crossDomain, which as noted should default true not false to match v6 and behave as expected.

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) {
Expand Down Expand Up @@ -334,7 +347,7 @@ export function fromAjax<T>(config: AjaxConfig): Observable<AjaxResponse<T>> {
// 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)) {
Copy link
Author

Choose a reason for hiding this comment

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

Maybe to match the below conditional should use remainingConfig

headers['x-requested-with'] = 'XMLHttpRequest';
}

Expand All @@ -353,16 +366,8 @@ export function fromAjax<T>(config: AjaxConfig): Observable<AjaxResponse<T>> {
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,
Expand Down