From 5076208444e2bd31129725328f32f9ec193135eb Mon Sep 17 00:00:00 2001 From: Dmitry Semenov Date: Fri, 15 Sep 2023 16:09:35 +0300 Subject: [PATCH] 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 --- .github/workflows/tests.yml | 2 +- dio/CHANGELOG.md | 3 + dio/dart_test.yaml | 7 +- dio/lib/src/adapters/browser_adapter.dart | 84 +++++++++++++++-------- dio/test/request_integration_test.dart | 81 ++++++++++++++++++++++ dio/test/utils.dart | 10 +++ 6 files changed, 157 insertions(+), 30 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 4a0849e45..0a72dbe28 100644 --- a/.github/workflows/tests.yml +++ b/.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/setup-dart@v1.3 diff --git a/dio/CHANGELOG.md b/dio/CHANGELOG.md index 53788bfe8..bf2f3a650 100644 --- a/dio/CHANGELOG.md +++ b/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 diff --git a/dio/dart_test.yaml b/dio/dart_test.yaml index 149442f40..d46838e77 100644 --- a/dio/dart_test.yaml +++ b/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 diff --git a/dio/lib/src/adapters/browser_adapter.dart b/dio/lib/src/adapters/browser_adapter.dart index 9ae672c1a..eab026f17 100644 --- a/dio/lib/src/adapters/browser_adapter.dart +++ b/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; diff --git a/dio/test/request_integration_test.dart b/dio/test/request_integration_test.dart index 2b747e5c7..e95b409dd 100644 --- a/dio/test/request_integration_test.dart +++ b/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'); }); } diff --git a/dio/test/utils.dart b/dio/test/utils.dart index 6198cde09..90bd036da 100644 --- a/dio/test/utils.dart +++ b/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()); +/// A matcher for functions that throw DioException of type connectionError. +final Matcher throwsDioExceptionConnectionError = throwsA( + allOf([ + isA(), + (DioException e) => e.type == DioExceptionType.connectionError, + ]), +); + /// A stream of chunks of bytes representing a single piece of data. class ByteStream extends StreamView> { ByteStream(Stream> stream) : super(stream);