fix: when crossDomain is using default the X-Requested-With header is no longer added incorrectly #6691
Conversation
… 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 ReactiveX@0d66ba4
| headers: { | ||
| 'x-requested-with': 'XMLHttpRequest', | ||
| }, | ||
| headers: {}, |
There was a problem hiding this comment.
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
|
Hi, @driskell! Thank you for finding this issue and coming up with a fix. I've added a commit to what you did and started a new PR here #6696 ... Sorry for the confusion, I was using the Github Web Editor, and it would not let me add a commit to this PR for some reason. Basically, the problem with this PR was that it changed the defaults of |
| const configWithDefaults = { | ||
| // Default values | ||
| async: true, | ||
| crossDomain: true, |
There was a problem hiding this comment.
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! :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Sorry for the multiple comments. Just so it’s tracked in the main comments area - as far as I could tell this resets the defaults back to v6 because as I can tell there shouldn’t have been a change. It doesn’t seem to make sense to me to change crossDomain to default to false. Here is v6: https://github.com/ReactiveX/rxjs/blob/6.x/src/internal/observable/dom/AjaxObservable.ts#L163 I did the cleanup here because I wanted to avoid separating the setting of defaults of crossDomain and other settings and it seemed to make no logic change except to crossDomain which was the issue. It also avoided a check for crossDomain undefined so seemed a cleaner fix. |
| @@ -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. | |||
There was a problem hiding this comment.
Just realised this should move down?
| @@ -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)) { | |||
There was a problem hiding this comment.
Maybe to match the below conditional should use remainingConfig
Description:
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). See the #6663 for more details.
Issue traces back to 0d66ba4 which is really strange as it seems to change tests; likely just an accidental change. As an example just because I know I changed a lot of tests here to fix them... If you look at 0d66ba4#diff-0f6e5e92622cfbc19d5344e07b65569c748bce03635ad9be2ffb87321b12d228R96 you can see that the original test changes because now it expects a header in the request the original did not expect. So possibly it was thought that the header was being set but not checked - but actually due to deep equals it was already correct and the header was not meant to be set. There's likely other commits that further embedded the bad behaviour into the tests. For example the upload/download progress the check on the request object shows
crossDomain: truebut then it shows the header is also verified to be there even though whentrueit should never be there.Some other changes I did to tests are a result of a feature that was built after the broken behaviour was introduced, and that's this feature: 1a2c2e4
BREAKING CHANGE: (I don't believe it is)
This fixes a breaking change from v6->v7. I do not believe this fix itself would break any v7 code as it should only mean code that was preflighting when it wasn't in v6 no longer preflights again. Unless some code is explicitly written that relies on the preflight this shouldn't cause any impact.
Related issue (if exists):
Fixes #6663