Reduce cases in which browsers would trigger a CORS preflight request (…

…#1955)

Certain requests are considered **["simple
requests"](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#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](https://fetch.spec.whatwg.org/#cors-safelisted-request-header)
- `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

- [x] I have read the
[Documentation](https://pub.dev/documentation/dio/latest/)
- [x] I have searched for a similar pull request in the
[project](https://github.com/cfug/dio/pulls) and found none
- [x] I have updated this branch with the latest `main` branch to avoid
conflicts (via merge from master or rebase)
- [x] I have added the required tests to prove the fix/feature I'm
adding
- [ ] I have updated the documentation (if necessary)
- [x] I have run the tests without failures
- [x] 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._

---------

Signed-off-by: Dmitry Semenov <[email protected]>

.github/workflows/tests.yml

@@ -85,7 +85,7 @@ jobs:
       fail-fast: false
       matrix:
         sdk: [ 2.15.0, stable, beta ]
-        platform: [ vm, chrome ]
+        platform: [ vm, chrome, firefox ]
     steps:
       - uses: actions/checkout@v3
       - uses: dart-lang/[email protected]

dio/CHANGELOG.md

@@ -9,6 +9,9 @@ See the [Migration Guide][] for the complete breaking changes list.**
 - Removes the accidentally added `options` argument for `Options.compose`.
 - Fix wrong formatting of multi-value header in `BrowserHttpClientAdapter`.
 - Add warning in debug mode when trying to send data with a `GET` request in web.
+- Reduce cases in which browsers would trigger a CORS preflight request.
+- Add warnings in debug mode when using `sendTimeout` and `onSendProgress` with an empty request body.
+- Fix `receiveTimeout` not working correctly on web.
 
 ## 5.3.2
 

dio/dart_test.yaml

@@ -4,5 +4,8 @@ file_reporters:
 override_platforms:
   chrome:
     settings:
-      # disable web security to allow CORS requests
-      arguments: --disable-web-security
+      headless: true
+  firefox:
+    settings:
+      # headless argument has to be set explicitly for non-chrome browsers
+      arguments: --headless

dio/lib/src/adapters/browser_adapter.dart

@@ -116,39 +116,69 @@ class BrowserHttpClientAdapter implements HttpClientAdapter {
       );
     }
 
-    final uploadStopwatch = Stopwatch();
-    xhr.upload.onProgress.listen((event) {
-      // This event will only be triggered if a request body exists.
+    // This code is structured to call `xhr.upload.onProgress.listen` only when
+    // absolutely necessary, because registering an xhr upload listener prevents
+    // the request from being classified as a "simple request" by the CORS spec.
+    // Reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests
+    // Upload progress events only get triggered if the request body exists,
+    // so we can check it beforehand.
+    if (requestStream != null) {
       if (connectTimeoutTimer != null) {
-        connectTimeoutTimer!.cancel();
-        connectTimeoutTimer = null;
+        xhr.upload.onProgress.listen((event) {
+          connectTimeoutTimer?.cancel();
+          connectTimeoutTimer = null;
+        });
       }
 
       final sendTimeout = options.sendTimeout;
       if (sendTimeout != null) {
-        if (!uploadStopwatch.isRunning) {
-          uploadStopwatch.start();
-        }
+        final uploadStopwatch = Stopwatch();
+        xhr.upload.onProgress.listen((event) {
+          if (!uploadStopwatch.isRunning) {
+            uploadStopwatch.start();
+          }
 
-        final duration = uploadStopwatch.elapsed;
-        if (duration > sendTimeout) {
-          uploadStopwatch.stop();
-          completer.completeError(
-            DioException.sendTimeout(
-              timeout: sendTimeout,
-              requestOptions: options,
-            ),
-            StackTrace.current,
-          );
-          xhr.abort();
-        }
+          final duration = uploadStopwatch.elapsed;
+          if (duration > sendTimeout) {
+            uploadStopwatch.stop();
+            completer.completeError(
+              DioException.sendTimeout(
+                timeout: sendTimeout,
+                requestOptions: options,
+              ),
+              StackTrace.current,
+            );
+            xhr.abort();
+          }
+        });
       }
-      if (options.onSendProgress != null &&
-          event.loaded != null &&
-          event.total != null) {
-        options.onSendProgress!(event.loaded!, event.total!);
+
+      final onSendProgress = options.onSendProgress;
+      if (onSendProgress != null) {
+        xhr.upload.onProgress.listen((event) {
+          if (event.loaded != null && event.total != null) {
+            onSendProgress(event.loaded!, event.total!);
+          }
+        });
       }
-    });
+    } else if (!_kReleaseMode) {
+      if (options.sendTimeout != null) {
+        dev.log(
+          'sendTimeout cannot be used without a request body to send',
+          level: 900,
+          name: '🔔 Dio',
+          stackTrace: StackTrace.current,
+        );
+      }
+      if (options.onSendProgress != null) {
+        dev.log(
+          'onSendProgress cannot be used without a request body to send',
+          level: 900,
+          name: '🔔 Dio',
+          stackTrace: StackTrace.current,
+        );
+      }
+    }
 
     final downloadStopwatch = Stopwatch();
     xhr.onProgress.listen((event) {
@@ -159,8 +189,8 @@ class BrowserHttpClientAdapter implements HttpClientAdapter {
 
       final receiveTimeout = options.receiveTimeout;
       if (receiveTimeout != null) {
-        if (!uploadStopwatch.isRunning) {
-          uploadStopwatch.start();
+        if (!downloadStopwatch.isRunning) {
+          downloadStopwatch.start();
         }
 
         final duration = downloadStopwatch.elapsed;

dio/test/request_integration_test.dart

@@ -330,5 +330,86 @@ void main() {
         expect(response.data![0], 1);
       });
     });
+
+    // Test that browsers can correctly classify requests as
+    // either "simple" or "preflighted". Reference:
+    // https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests
+    group('CORS preflight', () {
+      test('empty GET is not preflighted', () async {
+        // If there is no preflight (OPTIONS) request, the main request
+        // successfully completes with status 418.
+        final response = await dio.get(
+          '/status/418',
+          options: Options(
+            validateStatus: (status) => true,
+          ),
+        );
+        expect(response.statusCode, 418);
+      });
+
+      test('GET with custom headers is preflighted', () async {
+        // If there is a preflight (OPTIONS) request, the server fails it
+        // by responding with status 418. This fails CORS, so the browser
+        // never sends the main request and this code throws.
+        expect(() async {
+          final _ = await dio.get(
+            '/status/418',
+            options: Options(
+              headers: {
+                'x-request-header': 'value',
+              },
+            ),
+          );
+        }, throwsDioExceptionConnectionError);
+      });
+
+      test('POST with text body is not preflighted', () async {
+        // If there is no preflight (OPTIONS) request, the main request
+        // successfully completes with status 418.
+        final response = await dio.post(
+          '/status/418',
+          data: 'body text',
+          options: Options(
+            validateStatus: (status) => true,
+            contentType: Headers.textPlainContentType,
+          ),
+        );
+        expect(response.statusCode, 418);
+      });
+
+      test('POST with sendTimeout is preflighted', () async {
+        // If there is a preflight (OPTIONS) request, the server fails it
+        // by responding with status 418. This fails CORS, so the browser
+        // never sends the main request and this code throws.
+        expect(() async {
+          final _ = await dio.post(
+            '/status/418',
+            data: 'body text',
+            options: Options(
+              validateStatus: (status) => true,
+              contentType: Headers.textPlainContentType,
+              sendTimeout: Duration(seconds: 1),
+            ),
+          );
+        }, throwsDioExceptionConnectionError);
+      });
+
+      test('POST with onSendProgress is preflighted', () async {
+        // If there is a preflight (OPTIONS) request, the server fails it
+        // by responding with status 418. This fails CORS, so the browser
+        // never sends the main request and this code throws.
+        expect(() async {
+          final _ = await dio.post(
+            '/status/418',
+            data: 'body text',
+            options: Options(
+              validateStatus: (status) => true,
+              contentType: Headers.textPlainContentType,
+            ),
+            onSendProgress: (_, __) {},
+          );
+        }, throwsDioExceptionConnectionError);
+      });
+    }, testOn: 'browser');
   });
 }

dio/test/utils.dart

@@ -2,6 +2,8 @@ import 'dart:async';
 import 'dart:convert';
 import 'dart:io';
 import 'dart:typed_data';
+
+import 'package:dio/dio.dart';
 import 'package:test/test.dart';
 
 /// The current server instance.
@@ -155,6 +157,14 @@ void stopServer() {
 final Matcher throwsSocketException =
     throwsA(const TypeMatcher<SocketException>());
 
+/// A matcher for functions that throw DioException of type connectionError.
+final Matcher throwsDioExceptionConnectionError = throwsA(
+  allOf([
+    isA<DioException>(),
+    (DioException e) => e.type == DioExceptionType.connectionError,
+  ]),
+);
+
 /// A stream of chunks of bytes representing a single piece of data.
 class ByteStream extends StreamView<List<int>> {
   ByteStream(Stream<List<int>> stream) : super(stream);