Skip to content

fix(cookie_manager): deduplicate cookies when request is retried#2498

Open
ersanKolay wants to merge 6 commits intocfug:mainfrom
ersanKolay:fix/cookie-dedup
Open

fix(cookie_manager): deduplicate cookies when request is retried#2498
ersanKolay wants to merge 6 commits intocfug:mainfrom
ersanKolay:fix/cookie-dedup

Conversation

@ersanKolay
Copy link

Summary

Fixes #2442.

When a request is retried (e.g. after token refresh), loadCookies merges previousCookies from the existing header with savedCookies from the cookie jar without checking for duplicates. This causes cookies to accumulate on each retry attempt:

1st attempt: a=1; b=2
2nd attempt: a=1; b=2; a=1; b=2
3rd attempt: a=1; b=2; a=1; b=2; a=1; b=2

The fix filters out previousCookies entries whose names already exist in savedCookies, so jar cookies take precedence and no duplicates are produced.

The deduplication is done in loadCookies rather than getCookies to preserve the existing RFC 6265 behavior where same-name cookies with different paths are valid.

Test plan

  • Added no duplicate cookies on retry test that simulates the retry scenario
  • All existing tests pass (16/16)
  • dart format and dart analyze clean

When a request is retried with the same RequestOptions, loadCookies
merges previousCookies from headers with savedCookies from the jar
without deduplication, causing cookies to accumulate on each retry.

Filter out previousCookies that already exist in savedCookies by name,
so jar cookies take precedence and no duplicates are produced.

Closes cfug#2442
@ersanKolay ersanKolay requested a review from a team as a code owner March 11, 2026 00:07
@AlexV525
Copy link
Member

Is it possible to deduplicate cookies by checking their equality instead of rely on their name only? Will that (and current) consent any RFCs?

@AlexV525
Copy link
Member

I've check further with RFC 6265, which declared:

  1. If the cookie store contains a cookie with the same name, domain, and path as the newly created cookie:

    1. Let old-cookie be the existing cookie with the same name, domain, and path as the newly created cookie. (Notice that this algorithm maintains the invariant that there is at most one such cookie.)

    2. If the newly created cookie was received from a "non-HTTP" API and the old-cookie's http-only-flag is set, abort these steps and ignore the newly created cookie entirely.

    3. Update the creation-time of the newly created cookie to match the creation-time of the old-cookie.

    4. Remove the old-cookie from the cookie store.

The current deduplicate behavior doesn't seem to be aligned, correct?

Use (name, domain, path) identity per RFC 6265 Section 5.3 for cookie
deduplication. Cookies parsed from the Cookie header lack domain/path
metadata, so those fall back to name-only matching against saved cookies
which are already URI-scoped by cookieJar.loadForRequest.
@ersanKolay
Copy link
Author

Good catch, thanks for referencing RFC 6265 Section 5.3.

I've updated the deduplication to use (name, domain, path) as the cookie identity, which aligns with the RFC.

One practical note: cookies parsed from the Cookie request header are plain name=value pairs — they don't carry domain or path metadata. So when comparing header cookies against saved cookies, the code falls back to name-only matching. This is safe because cookieJar.loadForRequest(uri) already scopes saved cookies to the request URI, meaning domain and path are implicitly matched.

For cookies that do carry full metadata (domain and path set), the deduplication uses the full (name, domain, path) identity per the RFC.

…aths

Verifies that cookies with the same name but different paths (per
RFC 6265 Section 5.3 identity) are both preserved and not incorrectly
deduplicated on retry.
@ersanKolay
Copy link
Author

Minimal reproduction

import 'dart:io';
import 'package:cookie_jar/cookie_jar.dart';
import 'package:dio/dio.dart';
import 'package:dio_cookie_manager/dio_cookie_manager.dart';

void main() async {
  final cookieJar = CookieJar();
  final dio = Dio()..interceptors.add(CookieManager(cookieJar));

  // Server sets two cookies
  await cookieJar.saveFromResponse(
    Uri.parse('https://example.com'),
    [
      Cookie('a', '1')..path = '/',
      Cookie('b', '2')..path = '/',
    ],
  );

  // Simulate what happens on retry:
  // CookieManager.onRequest runs twice on the same RequestOptions.
  final options = RequestOptions(baseUrl: 'https://example.com');
  final mgr = CookieManager(cookieJar);

  // First run: sets Cookie header to "a=1; b=2"
  final cookies1 = await mgr.loadCookies(options);
  options.headers[HttpHeaders.cookieHeader] = cookies1;
  print('1st: $cookies1');
  // → a=1; b=2

  // Second run (retry): header already has "a=1; b=2"
  final cookies2 = await mgr.loadCookies(options);
  print('2nd: $cookies2');
  // Before fix: a=1; b=2; a=1; b=2 (duplicates!)
  // After fix:  a=1; b=2 ✓

  // RFC 6265 case: same name, different paths — both preserved
  final jar2 = CookieJar();
  await jar2.saveFromResponse(
    Uri.parse('https://example.com/api/endpoint'),
    [
      Cookie('session', 'abc')..path = '/',
      Cookie('session', 'xyz')..path = '/api',
    ],
  );
  final mgr2 = CookieManager(jar2);
  final opts2 = RequestOptions(baseUrl: 'https://example.com/api/endpoint');
  final cookies3 = await mgr2.loadCookies(opts2);
  print('RFC 6265: $cookies3');
  // → session=xyz; session=abc (both kept, different paths)
}

@AlexV525
Copy link
Member

Looks like the coverage is dropping.
Additionally, consider writing in a more readable/elegant pattern since the current modification makes the select query inlined too much.

Removed unreachable domain/path branch — per RFC 6265 Section 4.2,
the Cookie header only carries name=value pairs without domain or path
metadata. Name-only matching is sufficient since saved cookies are
already URI-scoped by cookieJar.loadForRequest.

Extracted the previous-cookie filtering into a named variable for
better readability.
@ersanKolay
Copy link
Author

Updated — simplified the dedup logic and improved readability:

  • Removed the _isDuplicate helper and the domain/path branch. Per RFC 6265 Section 4.2, the Cookie header only carries name=value pairs without domain or path metadata, so that branch was unreachable and caused the coverage drop.
  • Extracted the previous-cookie filtering into a named variable instead of inlining everything in the spread.
  • Name-only matching is sufficient here since cookieJar.loadForRequest already scopes saved cookies to the request URI.

@github-actions
Copy link
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
plugins/cookie_manager/lib/src/cookie_mgr.dart 🟢 98.57% 🟢 98.63% 🟢 0.06%
Overall Coverage 🟢 88.44% 🟢 88.46% 🟢 0.02%

Minimum allowed coverage is 0%, this run produced 88.46%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Question][CookieManager] About override the old cookies by new cookies when they have the same keys?

2 participants