From 7b65a97bfa434d69ecce28c6805ddf4096f4c68a Mon Sep 17 00:00:00 2001 From: Peter Leibiger Date: Wed, 28 Feb 2024 02:37:55 +0100 Subject: [PATCH] [http2] Fix Http2Adapter not redirecting for relative URLs (#2101) Fixes a redirect bug discovered during https://github.com/cfug/dio/pull/2077 where the adapter would fail to properly redirect if the location header contains a relative URL. ### 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 - [x] 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) --- .github/workflows/tests.yml | 4 +- .../http2_adapter/lib/src/http2_adapter.dart | 34 +++++++++++- plugins/http2_adapter/pubspec.yaml | 1 + plugins/http2_adapter/test/http2_test.dart | 52 +++++++++++++++++-- plugins/http2_adapter/test/pinning_test.dart | 1 - plugins/http2_adapter/test/redirect_test.dart | 45 ++++++++++++++++ plugins/http2_adapter/test/request_test.dart | 1 - plugins/http2_adapter/test/timeout_test.dart | 1 - 8 files changed, 128 insertions(+), 11 deletions(-) create mode 100644 plugins/http2_adapter/test/redirect_test.dart diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 5ebe99a7b..d5fdcdcec 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -60,8 +60,8 @@ jobs: - name: '[Verify step] Test Dart packages [VM]' run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --no-flutter -- "MELOS_ROOT_PATH/scripts/dart_test.sh --platform=vm" - name: '[Verify step] Test Dart packages [Chrome]' - run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --no-flutter -- "MELOS_ROOT_PATH/scripts/dart_test.sh --platform=chrome" + run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --ignore="*http2*" --no-flutter -- "MELOS_ROOT_PATH/scripts/dart_test.sh --platform=chrome" - name: '[Verify step] Test Dart packages [Firefox]' - run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --no-flutter -- "MELOS_ROOT_PATH/scripts/dart_test.sh --platform=firefox" + run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --ignore="*http2*" --no-flutter -- "MELOS_ROOT_PATH/scripts/dart_test.sh --platform=firefox" - name: '[Verify step] Test Flutter packages' run: melos exec $(eval echo $IGNORED_PACKAGES) --ignore="*example*" --flutter -- "flutter test" diff --git a/plugins/http2_adapter/lib/src/http2_adapter.dart b/plugins/http2_adapter/lib/src/http2_adapter.dart index 0dfc3177c..d6b14fbdd 100644 --- a/plugins/http2_adapter/lib/src/http2_adapter.dart +++ b/plugins/http2_adapter/lib/src/http2_adapter.dart @@ -6,6 +6,7 @@ import 'dart:typed_data'; import 'package:dio/dio.dart'; import 'package:dio/io.dart'; import 'package:http2/http2.dart'; +import 'package:meta/meta.dart'; part 'client_setting.dart'; @@ -219,12 +220,29 @@ class Http2Adapter implements HttpClientAdapter { // Handle redirection. if (needRedirect) { + if (responseHeaders['location'] == null) { + // Redirect without location is illegal. + throw DioException.connectionError( + requestOptions: options, + reason: 'Received redirect without location header.', + ); + } final url = responseHeaders.value('location'); + // An empty `location` header is considered a self redirect. + final uri = Uri.parse(url ?? ''); redirects.add( - RedirectRecord(statusCode, options.method, Uri.parse(url ?? '')), + RedirectRecord( + statusCode, + options.method, + uri, + ), ); + final String path = resolveRedirectUri(options.uri, uri); return _fetch( - options.copyWith(path: url, maxRedirects: --options.maxRedirects), + options.copyWith( + path: path, + maxRedirects: --options.maxRedirects, + ), list != null ? Stream.fromIterable(list) : null, cancelFuture, redirects, @@ -251,6 +269,18 @@ class Http2Adapter implements HttpClientAdapter { statusCodes.contains(status); } + @visibleForTesting + static String resolveRedirectUri(Uri currentUri, Uri redirectUri) { + if (redirectUri.hasScheme) { + /// This is a full URL which has to be redirected to as is. + return redirectUri.toString(); + } + + /// This is relative with or without leading slash and is + /// resolved against the URL of the original request. + return currentUri.resolveUri(redirectUri).toString(); + } + @override void close({bool force = false}) { connectionManager.close(force: force); diff --git a/plugins/http2_adapter/pubspec.yaml b/plugins/http2_adapter/pubspec.yaml index 88788cffc..a2e91a118 100644 --- a/plugins/http2_adapter/pubspec.yaml +++ b/plugins/http2_adapter/pubspec.yaml @@ -16,6 +16,7 @@ environment: dependencies: http2: ^2.1.0 + meta: ^1.9.1 dio: ^5.2.0 dev_dependencies: diff --git a/plugins/http2_adapter/test/http2_test.dart b/plugins/http2_adapter/test/http2_test.dart index c1bb40452..a936f962e 100644 --- a/plugins/http2_adapter/test/http2_test.dart +++ b/plugins/http2_adapter/test/http2_test.dart @@ -1,4 +1,3 @@ -@TestOn('vm') import 'package:dio/dio.dart'; import 'package:dio_http2_adapter/dio_http2_adapter.dart'; import 'package:test/test.dart'; @@ -173,13 +172,58 @@ void main() { await dio.get('/drip?delay=1&numbytes=1'); }); - test('request with redirect', () async { + group('redirects', () { final dio = Dio() ..options.baseUrl = 'https://httpbun.com/' ..httpClientAdapter = Http2Adapter(ConnectionManager()); - final res = await dio.get('absolute-redirect/2'); - expect(res.statusCode, 200); + test('single', () async { + final response = await dio.get( + '/redirect', + queryParameters: {'url': 'https://httpbun.com/get'}, + ); + expect(response.isRedirect, isTrue); + expect(response.redirects.length, 1); + + final ri = response.redirects.first; + expect(ri.statusCode, 302); + expect(ri.location.path, '/get'); + expect(ri.method, 'GET'); + }); + + test( + 'empty location', + () async { + final response = await dio.get( + '/redirect', + ); + expect(response.isRedirect, isTrue); + expect(response.redirects.length, 1); + + final ri = response.redirects.first; + expect(ri.statusCode, 302); + expect(ri.location.path, '/get'); + expect(ri.method, 'GET'); + }, + skip: 'Httpbun does not support empty location redirects', + ); + + test('multiple', () async { + final response = await dio.get( + '/redirect/3', + ); + expect(response.isRedirect, isTrue); + expect(response.redirects.length, 3); + + final ri = response.redirects.first; + expect(ri.statusCode, 302); + expect(ri.method, 'GET'); + }); + + test('request with redirect', () async { + final res = await dio.get('absolute-redirect/2'); + expect(res.statusCode, 200); + }); }); test('header value types implicit support', () async { diff --git a/plugins/http2_adapter/test/pinning_test.dart b/plugins/http2_adapter/test/pinning_test.dart index b669e5c8e..2843aab16 100644 --- a/plugins/http2_adapter/test/pinning_test.dart +++ b/plugins/http2_adapter/test/pinning_test.dart @@ -1,4 +1,3 @@ -@TestOn('vm') import 'dart:io'; import 'package:crypto/crypto.dart'; diff --git a/plugins/http2_adapter/test/redirect_test.dart b/plugins/http2_adapter/test/redirect_test.dart new file mode 100644 index 000000000..d81d4d3ab --- /dev/null +++ b/plugins/http2_adapter/test/redirect_test.dart @@ -0,0 +1,45 @@ +import 'package:dio_http2_adapter/dio_http2_adapter.dart'; +import 'package:test/test.dart'; + +void main() { + group(Http2Adapter.resolveRedirectUri, () { + test('empty location', () async { + final current = Uri.parse('https://example.com'); + final result = Http2Adapter.resolveRedirectUri( + current, + Uri.parse(''), + ); + + expect(result, current.toString()); + }); + + test('relative location 1', () async { + final result = Http2Adapter.resolveRedirectUri( + Uri.parse('https://example.com/foo'), + Uri.parse('/bar'), + ); + + expect(result, 'https://example.com/bar'); + }); + + test('relative location 2', () async { + final result = Http2Adapter.resolveRedirectUri( + Uri.parse('https://example.com/foo'), + Uri.parse('../bar'), + ); + + expect(result, 'https://example.com/bar'); + }); + + test('different location', () async { + final current = Uri.parse('https://example.com/foo'); + final target = 'https://somewhere.com/bar'; + final result = Http2Adapter.resolveRedirectUri( + current, + Uri.parse(target), + ); + + expect(result, target); + }); + }); +} diff --git a/plugins/http2_adapter/test/request_test.dart b/plugins/http2_adapter/test/request_test.dart index b1043004b..dd8ab872d 100644 --- a/plugins/http2_adapter/test/request_test.dart +++ b/plugins/http2_adapter/test/request_test.dart @@ -1,4 +1,3 @@ -@TestOn('vm') import 'dart:io'; import 'package:dio/dio.dart'; diff --git a/plugins/http2_adapter/test/timeout_test.dart b/plugins/http2_adapter/test/timeout_test.dart index 3c2a198d3..21ebbfced 100644 --- a/plugins/http2_adapter/test/timeout_test.dart +++ b/plugins/http2_adapter/test/timeout_test.dart @@ -1,4 +1,3 @@ -@TestOn('vm') import 'dart:async'; import 'package:dio/dio.dart';