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.

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

Rxjs 7.0+ breaking change requires preflight requests for Ajax calls that do not specify crossDomain #6663

Closed
driskell opened this issue Nov 4, 2021 · 3 comments · Fixed by #6710

Comments

@driskell
Copy link

driskell commented Nov 4, 2021

Bug Report

Current Behavior

In 6.6 crossDomain is true by default, and therefore the X-Requested-With header is not added, and thus a standard Ajax request does not require preflight requests and one's servers do not need to implement support for OPTIONS requests.

Code in 6.6: https://github.com/ReactiveX/rxjs/blob/6.6.7/src/internal/observable/dom/AjaxObservable.ts#L205
The request object is populated at https://github.com/ReactiveX/rxjs/blob/6.6.7/src/internal/observable/dom/AjaxObservable.ts#L181
And this request object contains defaults, so has crossDomain: true.

In 7.0+ crossDomain is still true by default, however, X-Requested-With header is ALWAYS added to ALL requests made by AJAX, regardless, unless crossDomain is explicitly provided as false. This is because the default value of true is missing at the point in time at which the header is added.

Code in 7.4: https://github.com/ReactiveX/rxjs/blob/7.4.0/src/internal/ajax/ajax.ts#L337
The config object is directly from the caller and does not contain defaults because the defaults are not added until later on: https://github.com/ReactiveX/rxjs/blob/7.4.0/src/internal/ajax/ajax.ts#L355

Expected behavior

No preflight request for a standard simple ajax request with no crossDomain option specified (which should default to true)

Reproduction

RxJS 7.4 Codepen: https://codepen.io/Driskell/pen/MWvQymO?editors=1111
RxJS 6.6 Codepen: https://codepen.io/Driskell/pen/yLovOxK?editors=1111

Open console in your browser and click the button. In Safari on macOS you can see on 7.4 preflight response is not successful, but on 6.6 it does not attempt one.

Environment

  • Runtime: Safari, Chrome, etc.
  • RxJS version: 7.0+

Possible Solution

Check if config.crossDomain is defined first, or destructure it with a default on the first line of the function: https://github.com/ReactiveX/rxjs/blob/7.4.0/src/internal/ajax/ajax.ts#L282

Additional context/Screenshots

N/A

@driskell driskell changed the title Rxjs 7.0+ requires preflight requests for all Ajax calls, in comparison to 6.6 Rxjs 7.0+ breaking change requires preflight requests for Ajax calls that do not specify crossDomain Nov 4, 2021
@driskell
Copy link
Author

driskell commented Nov 30, 2021

@benlesh is it worth pinning the agenda item here? As this is probably the best source of the issue currently encountered and maybe the best starting point for discussion on future of crossDomain.

I have CodePens in here too so it helps reproduce the issue and understand the impacts of the crossDomain parameter and the header that gets added etc.

benlesh added a commit to benlesh/rxjs that referenced this issue Dec 6, 2021
…mers

Fixes an issue where the `crosDomain` flag was being incorrectly reported to users via error objects and response objects as defaulting to `true`, when it was in fact defaulting to `false`.

Deprecates the `crossDomain` flag in favor of allowing the configuration of the request and the browser to dictate whether or not a preflight request is necessary. Adds deprecation messages with advice about how to force CORS preflights. Ultimately, the boolean flag does not make sense, as there are a lot of factors that dictate CORS preflight use and they may vary by browser/environment.

Resolves ReactiveX#6663
benlesh added a commit to benlesh/rxjs that referenced this issue Jan 4, 2022
…mers

Fixes an issue where the `crosDomain` flag was being incorrectly reported to users via error objects and response objects as defaulting to `true`, when it was in fact defaulting to `false`.

Deprecates the `crossDomain` flag in favor of allowing the configuration of the request and the browser to dictate whether or not a preflight request is necessary. Adds deprecation messages with advice about how to force CORS preflights. Ultimately, the boolean flag does not make sense, as there are a lot of factors that dictate CORS preflight use and they may vary by browser/environment.

Resolves ReactiveX#6663
benlesh added a commit that referenced this issue Jan 11, 2022
…mers (#6710)

* fix(ajax): crossDomain flag deprecated and properly reported to consumers

Fixes an issue where the `crosDomain` flag was being incorrectly reported to users via error objects and response objects as defaulting to `true`, when it was in fact defaulting to `false`.

Deprecates the `crossDomain` flag in favor of allowing the configuration of the request and the browser to dictate whether or not a preflight request is necessary. Adds deprecation messages with advice about how to force CORS preflights. Ultimately, the boolean flag does not make sense, as there are a lot of factors that dictate CORS preflight use and they may vary by browser/environment.

Resolves #6663

* chore: Update side effects file

* chore: update side effects files
@driskell
Copy link
Author

@benlesh Does the PR definitely fix this issue? Looking at the code it seems it tries to resolve the confusion around the flag, but it looks like it will still set the header X-Requested-With with default parameters to AJAX where with 6 it never did. And so it will force all AJAX to be CORS unless the crossDomain is explicitly set, and so still break all code from RxJs 6 that didn’t expect this.

The comment against the crossDomain flag seems strange too - it says don’t use this flag to force a cross domain (I assume it means preflight) and instead set a custom header. But actually the code is setting a custom header if it is set to false. So it doesn’t make any difference what you do you will always get an OPTIONS request sent unless you set this flag which is now deprecated... And in fact to STOP it sending the header you need to set it to true.

@driskell
Copy link
Author

driskell commented Jan 11, 2022

@benlesh I added a new codepen to reproduce for 7.5.2 and improved it so it shows on screen results, and also added in crossDomain:true case and also default cases for jQuery and axios for comparison.

6.6.7: https://codepen.io/Driskell/pen/yLovOxK
7.4.0: https://codepen.io/Driskell/pen/MWvQymO
7.5.2: https://codepen.io/Driskell/pen/PoJyRaz

In 6.6.7 it works fine and you don't need to specify anything, much like jQuery and Axios. In 7+ and also after 7.5.2 it's still broken unless I specify crossDomain:true.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Dec 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [rxjs](https://rxjs.dev) ([source](https://github.com/reactivex/rxjs)) | dependencies | minor | [`7.5.7` -> `7.8.0`](https://renovatebot.com/diffs/npm/rxjs/7.5.7/7.8.0) |

---

### Release Notes

<details>
<summary>reactivex/rxjs</summary>

### [`v7.8.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#&#8203;780-httpsgithubcomreactivexrxjscompare770780-2022-12-15)

[Compare Source](ReactiveX/rxjs@7.7.0...7.8.0)

##### Features

-   **buffer:** `closingNotifier` now supports any `ObservableInput` ([#&#8203;7073](ReactiveX/rxjs#7073)) ([61b877a](ReactiveX/rxjs@61b877a))
-   **delayWhen:** `delayWhen`'s `delayDurationSelector` now supports any `ObservableInput` ([#&#8203;7049](ReactiveX/rxjs#7049)) ([dfd95db](ReactiveX/rxjs@dfd95db))
-   **sequenceEqual:** `compareTo` now supports any `ObservableInput` ([#&#8203;7102](ReactiveX/rxjs#7102)) ([d501961](ReactiveX/rxjs@d501961))
-   **share:** `ShareConfig` factory properties now supports any `ObservableInput` ([#&#8203;7093](ReactiveX/rxjs#7093)) ([cc3995a](ReactiveX/rxjs@cc3995a))
-   **skipUntil:** `notifier` now supports any `ObservableInput` ([#&#8203;7091](ReactiveX/rxjs#7091)) ([60d6c40](ReactiveX/rxjs@60d6c40))
-   **window:** `windowBoundaries` now supports any `ObservableInput` ([#&#8203;7088](ReactiveX/rxjs#7088)) ([8c4347c](ReactiveX/rxjs@8c4347c))

### [`v7.7.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#&#8203;770-httpsgithubcomreactivexrxjscompare760770-2022-12-15)

[Compare Source](ReactiveX/rxjs@7.6.0...7.7.0)

##### Features

-   **distinct:** `flush` argument now supports any `ObservableInput` ([#&#8203;7081](ReactiveX/rxjs#7081)) ([74c9ebd](ReactiveX/rxjs@74c9ebd))
-   **repeatWhen:** `notifier` supports `ObservableInput` ([#&#8203;7103](ReactiveX/rxjs#7103)) ([8f1b976](ReactiveX/rxjs@8f1b976))
-   **retryWhen:** `notifier` now supports any `ObservableInput` ([#&#8203;7105](ReactiveX/rxjs#7105)) ([794f806](ReactiveX/rxjs@794f806))
-   **sample:** `notifier` now supports any `ObservableInput` ([#&#8203;7104](ReactiveX/rxjs#7104)) ([b18c2eb](ReactiveX/rxjs@b18c2eb))

### [`v7.6.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#&#8203;760-httpsgithubcomreactivexrxjscompare757760-2022-12-03)

[Compare Source](ReactiveX/rxjs@7.5.7...7.6.0)

##### Bug Fixes

-   **schedulers:** no longer cause TypeScript build failures when Node types aren't included ([c1a07b7](ReactiveX/rxjs@c1a07b7))
-   **types:** Improved subscribe and tap type overloads ([#&#8203;6718](ReactiveX/rxjs#6718)) ([af1a9f4](ReactiveX/rxjs@af1a9f4)), closes [#&#8203;6717](ReactiveX/rxjs#6717)

##### Features

-   **onErrorResumeNextWith:** renamed `onErrorResumeNext` and exported from the top level. (`onErrorResumeNext` operator is stil available, but deprecated) ([#&#8203;6755](ReactiveX/rxjs#6755)) ([51e3b2c](ReactiveX/rxjs@51e3b2c))

#### [7.5.7](ReactiveX/rxjs@7.5.6...7.5.7) (2022-09-25)

##### Bug Fixes

-   **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#&#8203;7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#&#8203;7017](ReactiveX/rxjs#7017), related to [#&#8203;7018](ReactiveX/rxjs#7018) and [#&#8203;6674](ReactiveX/rxjs#6674)

##### Performance Improvements

-   **animationFrames:** uses fewer Subscription instances ([#&#8203;7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#&#8203;7018](ReactiveX/rxjs#7018)

#### [7.5.6](ReactiveX/rxjs@7.5.5...7.5.6) (2022-07-11)

##### Bug Fixes

-   **share:** No longer results in a bad-state observable in an edge case where a synchronous source was shared and refCounted, and the result is subscribed to twice in a row synchronously. ([#&#8203;7005](ReactiveX/rxjs#7005)) ([5d4c1d9](ReactiveX/rxjs@5d4c1d9))
-   **share & connect:** `share` and `connect` no longer bundle scheduling code by default ([#&#8203;6873](ReactiveX/rxjs#6873)) ([9948dc2](ReactiveX/rxjs@9948dc2)), closes [#&#8203;6872](ReactiveX/rxjs#6872)
-   **exhaustAll:** Result will now complete properly when flattening all synchronous observables. ([#&#8203;6911](ReactiveX/rxjs#6911)) ([3c1c6b8](ReactiveX/rxjs@3c1c6b8)), closes [#&#8203;6910](ReactiveX/rxjs#6910)
-   **TypeScript:** Now compatible with TypeScript 4.6 type checks ([#&#8203;6895](ReactiveX/rxjs#6895)) ([fce9aa1](ReactiveX/rxjs@fce9aa1))

#### [7.5.5](ReactiveX/rxjs@7.5.4...7.5.5) (2022-03-08)

##### Bug Fixes

-   **package:** add types to exports ([#&#8203;6802](ReactiveX/rxjs#6802)) ([3750f75](ReactiveX/rxjs@3750f75))
-   **package:** add `require` export condition ([#&#8203;6821](ReactiveX/rxjs#6821)) ([c8955e4](ReactiveX/rxjs@c8955e4))
-   **timeout:** no longer will timeout when receiving the first value synchronously ([#&#8203;6865](ReactiveX/rxjs#6865)) ([2330c96](ReactiveX/rxjs@2330c96)), closes [#&#8203;6862](ReactiveX/rxjs#6862)

##### Performance Improvements

-   Don't clone observers unless you have to ([#&#8203;6842](ReactiveX/rxjs#6842)) ([3289d20](ReactiveX/rxjs@3289d20))

#### [7.5.4](ReactiveX/rxjs@7.5.3...7.5.4) (2022-02-09)

##### Performance Improvements

-   removed code that would `bind` functions passed with observers to `subscribe`. ([#&#8203;6815](ReactiveX/rxjs#6815)) ([fb375a0](ReactiveX/rxjs@fb375a0)), closes [#&#8203;6783](ReactiveX/rxjs#6783)

#### [7.5.3](ReactiveX/rxjs@7.5.2...7.5.3) (2022-02-08)

##### Bug Fixes

-   **subscribe:** allow interop with Monio and other libraries that patch function bind ([0ab91eb](ReactiveX/rxjs@0ab91eb)), closes [#&#8203;6783](ReactiveX/rxjs#6783)

#### [7.5.2](ReactiveX/rxjs@7.5.1...7.5.2) (2022-01-11)

##### Bug Fixes

-   operators that ignore input values now use `unknown` rather than `any`, which should resolve issues with eslint no-unsafe-argument ([#&#8203;6738](ReactiveX/rxjs#6738)) ([67cb317](ReactiveX/rxjs@67cb317)), closes [#&#8203;6536](ReactiveX/rxjs#6536)
-   **ajax:** crossDomain flag deprecated and properly reported to consumers ([#&#8203;6710](ReactiveX/rxjs#6710)) ([7fd0575](ReactiveX/rxjs@7fd0575)), closes [#&#8203;6663](ReactiveX/rxjs#6663)

#### [7.5.1](ReactiveX/rxjs@7.5.0...7.5.1) (2021-12-28)

##### Bug Fixes

-   export supporting interfaces from top-level `rxjs` site. ([#&#8203;6733](ReactiveX/rxjs#6733)) ([299a1e1](ReactiveX/rxjs@299a1e1))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC40OC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1668
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant