Reduce cases in which browsers would trigger a CORS preflight request

[lonelyteapot]

Certain requests are considered "simple requests" by the CORS spec - they do not require a preflight request. The old logic unconditionally registered XMLHttpRequest.upload progress event listener, which made it impossible for requests to be considered "simple". This mostly affected Firefox, because it correctly followed the spec.

This PR adds more conditions around registering the handler, reducing dio's impact on CORS handling. Now, a request is NOT preflighted if:

• It is a HEAD / GET / POST request
• Its Content-Type is one of text/plain, multipart/form-data, application/x-www-form-urlencoded
• It doesn't contain any headers which are not safelisted
• connectTimeout is not specified
• sendTimeout is not specified
• onSendProgress is not specified
• It otherwise satisfies the spec as determined by the browser

Resolves #1954.

New Pull Request Checklist

• I have read the Documentation
• I have searched for a similar pull request in the project and found none
• I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
• I have added the required tests to prove the fix/feature I'm adding
• I have updated the documentation (if necessary)
• I have run the tests without failures
• I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

This is my first time contributing to dio. I don't really know what I'm doing. I have no idea of the global implications of this change. Any assistance and scrutiny would be appreciated.

This fix has worked for my original use case.

[kuhnroyal]

Interesting, I didn't know this about CORS. I think this makes sense in any case.

Just wondering about the options.onSendProgress, what is the current behavior and how does the change effect people passing this without body data.

[AlexV525]

Can we run tests only for Firefox? I'm not aware of any corresponding flags.

[AlexV525]

From an overall look, this change affects some timeout behavior which might introduce regression. And the change itself requires tests.

[lonelyteapot]

@AlexV525

Can we run tests only for Firefox? I'm not aware of any corresponding flags.

I think we can with @TestOn('firefox') and by adding Firefox to dart test target platforms (right now no tests are run on Firefox as I understand). But I think this behavior should be tested for all browsers, not just Firefox.

I'll start adding a browser test which checks if OPTIONS request gets sent to the server for requests that should've been "simple".

[lonelyteapot]

@kuhnroyal @AlexV525

Just wondering about the options.onSendProgress, what is the current behavior and how does the change effect people passing this without body data.

From an overall look, this change affects some timeout behavior which might introduce regression.

Can you elaborate on that? As I understand from the code comment, the onProgress event only triggers if the request body is not empty. All I did is added the same logical check above registering the event listener, so logic should remain identical. However, I don't know if this is actually true, I haven't found any proof and didn't test it manually (yet). Can you share something that can help?

[kuhnroyal]

@AlexV525 Regarding the timeouts, I think this comment is the key here. If there is no body, the following code never runs. But we should add a general test for that.

[kuhnroyal]

@lonelyteapot It would be great if you can setup firefox handling. I think this should be possible on ubuntu in CI.

As I understand from the code comment, the onProgress event only triggers if the request body is not empty

Yea, after looking at the code again, this became more obvious to me :)
Can you check if we have a test that verifies this behavior and if not, add one?

[lonelyteapot]

I've run into a blocker while enabling tests on Firefox. Basic tests fail with CORS errors. This is due to a problem with httpbun.com which is used in tests. I've filed an issue there: sharat87/httpbun#10.

This error happens in Chrome too, but Dio tests are not affected because they launch Chrome with --disable-web-security argument. There is no such possibility in Firefox, CORS cannot be disabled in any way.

And I think web security shouldn't be disabled, so not to hide issues like this.

[kuhnroyal]

Thanks for creating the httpbun issue. My last tickets there were resolved and deployed quickly.

[kuhnroyal]

I have created #1966 to update test configuration for firefox.

[lonelyteapot]

@kuhnroyal, thanks, but I've already done it locally for this PR. Sorry for not pushing for so long. Right now I'm debugging some stuff and making sure new changes aren't breaking things.

I think you may also want to add --headless argument to Firefox, since chrome runs in headless mode by default, while Firefox does not (ref)

[kuhnroyal]

No worries. I think it would be good to have a baseline from the other PR so we don't mingle changes for the CORS problem with other fixes for Firefox tests.

[lonelyteapot]

I've extended the scope of this PR. Previously there was only a check for empty request body to handle the majority of use cases, but now there are more tighter conditions to register an upload listener. The logic itself should remain unchanged, but a thorough review is needed.

I've manually verified that progress events don't get triggered when the body is empty. I don't think there's a need for an automated test.

I've also noticed that there was a bug which caused receiveTimeout not to work.

Please review

[AlexV525]

I think the above comments are resolved to me gracefully. Thanks!

[kuhnroyal]

This is interesting. Without your changes, there are failing tests on firefox in #1966.
I don't know why exactly these requests fail, but the XMLHttpRequest.onError callback is called. I would assume it is for the same reason, that we attach upload listeners without uploading anything.

[kuhnroyal]

This is great work, thanks!

[lonelyteapot]

@kuhnroyal interested indeed. I don't think lack of my changes could be the cause here. I haven't encountered any issues while running tests locally. That might have been a random failure.

[kuhnroyal]

No, it is not random. All status code tests (4xx/5xx) fail with firefox on develop.
As soon as I add the if (requestStream != null) check, everything works.

[lonelyteapot]

@kuhnroyal oh, yes, you're right. Since in these tests the server expectedly responds with non-successful HTTP status codes, preflight requests fail. Turns out there wasn't a single test for a request that should both be preflighted and fail.