diff --git a/plugins/cookie_manager/CHANGELOG.md b/plugins/cookie_manager/CHANGELOG.md index 5c8f50138..1e81ef9ec 100644 --- a/plugins/cookie_manager/CHANGELOG.md +++ b/plugins/cookie_manager/CHANGELOG.md @@ -2,7 +2,8 @@ ## Unreleased -*None.* +- Prevent duplicate cookies when reusing request options while preserving + caller-provided cookies. ## 3.4.0 diff --git a/plugins/cookie_manager/lib/src/cookie_mgr.dart b/plugins/cookie_manager/lib/src/cookie_mgr.dart index a3a79d17a..b904d9c1a 100644 --- a/plugins/cookie_manager/lib/src/cookie_mgr.dart +++ b/plugins/cookie_manager/lib/src/cookie_mgr.dart @@ -21,7 +21,19 @@ const _kIsWeb = _kIsWebInterop || _kIsWebUtil || identical(0, 0.0); /// attribute like "expires=Sun, 19 Feb 3000 01:43:15 GMT", which could also contain commas. final _setCookieReg = RegExp('(?<=)(,)(?=[^;]+?=)'); +class _CookieHeaderState { + const _CookieHeaderState(this.source, this.merged); + + final String? source; + final String merged; +} + /// Cookie manager for HTTP requests based on [CookieJar]. +/// +/// Register this after interceptors that may change [RequestOptions.uri] or +/// the Cookie request header. Cookies are selected from the URI and reused +/// request options are recognized only while the header exactly matches this +/// manager's previous output. class CookieManager extends Interceptor { CookieManager( this.cookieJar, { @@ -38,6 +50,8 @@ class CookieManager extends Interceptor { /// Whether to ignore invalid cookies during parsing or saving. bool ignoreInvalidCookies; + final Expando<_CookieHeaderState> _cookieHeaderStates = Expando(); + /// Merge cookies into a Cookie string. /// Cookies with longer paths are listed before cookies with shorter paths. static String getCookies(List cookies) { @@ -145,18 +159,32 @@ class CookieManager extends Interceptor { } /// Load cookies in cookie string for the request. + /// + /// State is scoped to the identity of [options]. When the same instance is + /// reused, its original Cookie header is restored only if the current header + /// exactly matches the previous result of this method. Any other header is + /// treated as a new source. Incrementally modifying a generated header after + /// this manager can therefore cause saved cookies to be merged again. Future loadCookies(RequestOptions options) async { final savedCookies = await cookieJar.loadForRequest(options.uri); final previousCookies = options.headers[HttpHeaders.cookieHeader] as String?; + final previousState = _cookieHeaderStates[options]; + // Rebuild a header that still exactly matches this manager's last output. + // Any other header becomes the source for the next merge. + final sourceCookies = + previousState != null && previousState.merged == previousCookies + ? previousState.source + : previousCookies; final cookies = getCookies([ - ...?previousCookies + ...?sourceCookies ?.split(';') .where((e) => e.isNotEmpty) .map((c) => _fromSetCookieValue(c)) .whereType(), // Use .nonNulls when the minimum SDK is 3.0. ...savedCookies, ]); + _cookieHeaderStates[options] = _CookieHeaderState(sourceCookies, cookies); return cookies; } diff --git a/plugins/cookie_manager/test/cookies_test.dart b/plugins/cookie_manager/test/cookies_test.dart index b01f8ea41..08dcf51c7 100644 --- a/plugins/cookie_manager/test/cookies_test.dart +++ b/plugins/cookie_manager/test/cookies_test.dart @@ -1,4 +1,5 @@ @TestOn('vm') +import 'dart:async'; import 'dart:io'; import 'dart:typed_data'; @@ -8,21 +9,39 @@ import 'package:dio/io.dart'; import 'package:dio_cookie_manager/dio_cookie_manager.dart'; import 'package:test/test.dart'; -class MockRequestInterceptorHandler extends RequestInterceptorHandler { - MockRequestInterceptorHandler(this.expectResult); +class _TestRequestInterceptorHandler extends RequestInterceptorHandler { + final Completer _result = Completer(); - final String expectResult; + Future get result => _result.future; @override void next(RequestOptions requestOptions) { - final c = requestOptions.headers[HttpHeaders.cookieHeader]; - expect(c == expectResult, true); - super.next(requestOptions); + _result.complete(requestOptions); + } + + @override + void reject( + DioException error, [ + bool callFollowingErrorInterceptor = false, + ]) { + _result.completeError(error, error.stackTrace); } } class MockResponseInterceptorHandler extends ResponseInterceptorHandler {} +Future expectRequestCookies( + CookieManager cookieManager, + RequestOptions options, + String? expected, +) async { + final handler = _TestRequestInterceptorHandler(); + await cookieManager.onRequest(options, handler); + final result = await handler.result; + expect(result, same(options)); + expect(options.headers[HttpHeaders.cookieHeader], expected); +} + class _MockRejectRequestInterceptorHandler extends RequestInterceptorHandler { _MockRejectRequestInterceptorHandler(this.matcher); @@ -118,18 +137,200 @@ void main() { ); // Verify mock cookies. - final mockRequestInterceptorHandler = - MockRequestInterceptorHandler(expectResult); final options = RequestOptions( baseUrl: exampleUrl, headers: { HttpHeaders.cookieHeader: mockSecondRequestCookies, }, ); - await cookieManager.onRequest( - options, - mockRequestInterceptorHandler, - ); + await expectRequestCookies(cookieManager, options, expectResult); + }); + + group('reusing request options', () { + const exampleUrl = 'https://example.com/api/endpoint'; + + test('does not append the cookie jar values again through Dio.fetch', + () async { + final cookieJar = CookieJar(); + await cookieJar.saveFromResponse( + Uri.parse(exampleUrl), + [ + Cookie('session', 'root')..path = '/', + Cookie('session', 'api')..path = '/api', + ], + ); + final cookieManager = CookieManager(cookieJar); + final adapter = _CookieRecordingAdapter(); + final dio = Dio() + ..httpClientAdapter = adapter + ..interceptors.add(cookieManager); + addTearDown(dio.close); + final options = RequestOptions( + baseUrl: exampleUrl, + responseType: ResponseType.plain, + ); + + await dio.fetch(options); + await dio.fetch(options); + await dio.fetch(options); + + expect( + adapter.cookieHeaders, + everyElement( + equals( + 'session=api; session=root', + ), + ), + ); + expect(adapter.cookieHeaders, hasLength(3)); + expect(adapter.requestOptions, everyElement(same(options))); + }); + + test('preserves a same-name cookie supplied by the caller', () async { + final cookieJar = CookieJar(); + await cookieJar.saveFromResponse( + Uri.parse(exampleUrl), + [Cookie('session', 'saved')..path = '/'], + ); + final cookieManager = CookieManager(cookieJar); + final options = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'session=provided'}, + ); + + await expectRequestCookies( + cookieManager, + options, + 'session=provided; session=saved', + ); + await expectRequestCookies( + cookieManager, + options, + 'session=provided; session=saved', + ); + }); + + test('reloads saved cookies without retaining their old values', () async { + final cookieJar = CookieJar(); + final uri = Uri.parse(exampleUrl); + await cookieJar.saveFromResponse( + uri, + [Cookie('session', 'old')..path = '/'], + ); + final cookieManager = CookieManager(cookieJar); + final options = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'provided=value'}, + ); + + await expectRequestCookies( + cookieManager, + options, + 'provided=value; session=old', + ); + await cookieJar.saveFromResponse( + uri, + [Cookie('session', 'new')..path = '/'], + ); + await expectRequestCookies( + cookieManager, + options, + 'provided=value; session=new', + ); + await cookieJar.deleteAll(); + await expectRequestCookies(cookieManager, options, 'provided=value'); + }); + + test('uses a cookie header changed by the caller as the new input', + () async { + final cookieJar = CookieJar(); + await cookieJar.saveFromResponse( + Uri.parse(exampleUrl), + [Cookie('saved', 'value')..path = '/'], + ); + final cookieManager = CookieManager(cookieJar); + final options = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'provided=first'}, + ); + + await expectRequestCookies( + cookieManager, + options, + 'provided=first; saved=value', + ); + options.headers[HttpHeaders.cookieHeader] = 'provided=changed'; + await expectRequestCookies( + cookieManager, + options, + 'provided=changed; saved=value', + ); + await expectRequestCookies( + cookieManager, + options, + 'provided=changed; saved=value', + ); + }); + + test('does not retain saved cookies after the request origin changes', + () async { + final cookieJar = CookieJar(); + await cookieJar.saveFromResponse( + Uri.parse(exampleUrl), + [Cookie('session', 'saved')..path = '/'], + ); + final cookieManager = CookieManager(cookieJar); + final options = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'provided=value'}, + ); + + await expectRequestCookies( + cookieManager, + options, + 'provided=value; session=saved', + ); + options.baseUrl = 'https://other.example.com'; + await expectRequestCookies(cookieManager, options, 'provided=value'); + }); + + test('keeps state separate between request options', () async { + final cookieJar = CookieJar(); + await cookieJar.saveFromResponse( + Uri.parse(exampleUrl), + [Cookie('saved', 'value')..path = '/'], + ); + final cookieManager = CookieManager(cookieJar); + final first = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'provided=first'}, + ); + final second = RequestOptions( + baseUrl: exampleUrl, + headers: {HttpHeaders.cookieHeader: 'provided=second'}, + ); + + await expectRequestCookies( + cookieManager, + first, + 'provided=first; saved=value', + ); + await expectRequestCookies( + cookieManager, + second, + 'provided=second; saved=value', + ); + await expectRequestCookies( + cookieManager, + first, + 'provided=first; saved=value', + ); + await expectRequestCookies( + cookieManager, + second, + 'provided=second; saved=value', + ); + }); }); group('Set-Cookie', () { @@ -162,12 +363,7 @@ void main() { // Verify mock cookies. final options = RequestOptions(baseUrl: exampleUrl); - final mockRequestInterceptorHandler = - MockRequestInterceptorHandler(expectResult); - await cookieManager.onRequest( - options, - mockRequestInterceptorHandler, - ); + await expectRequestCookies(cookieManager, options, expectResult); }); test('can be saved to the location', () async { @@ -427,6 +623,25 @@ void main() { }); } +class _CookieRecordingAdapter implements HttpClientAdapter { + final List cookieHeaders = []; + final List requestOptions = []; + + @override + Future fetch( + RequestOptions options, + Stream? requestStream, + Future? cancelFuture, + ) async { + requestOptions.add(options); + cookieHeaders.add(options.headers[HttpHeaders.cookieHeader] as String?); + return ResponseBody.fromString('', HttpStatus.ok); + } + + @override + void close({bool force = false}) {} +} + class _RedirectAdapter implements HttpClientAdapter { final HttpClientAdapter _adapter = IOHttpClientAdapter();